Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13341 )
Change subject: Support SPNEGO for web server ...................................................................... Patch Set 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt File src/kudu/security/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt@85 PS3, Line 85: set(SECURITY_LIBS : gutil : kudu_util : token_proto : : krb5 : gssapi_krb5 : openssl_crypto : openssl_ssl) > Nit: while you're here, could you resort this alphabetically? done, but left the kudu libs separate from the thirdparty ones. http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h File src/kudu/security/gssapi.h: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@35 PS3, Line 35: header > Nit: did you mean 'token' here? Otherwise there's not enough context to und Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@42 PS3, Line 42: '*complete' indicates whether any further rounds are required. On : // completion of negotiation, > I assume that 'complete' is only touched in the event of an OK status? Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc File src/kudu/security/gssapi.cc: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@54 PS3, Line 54: gss_release_buffer(&minor, &buf); > Maybe also add a comment saying that we can't use the more natural ReleaseB Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144 PS3, Line 144: gss_buffer_desc name_buf {0, nullptr}; > Does this need to be freed after the call to std::string::assign? If not, c interesting. The manpage indicates it does need to be freed. Not sure why LSAN didn't catch it -- perhaps because we have some LSAN suppressions for other known leaks in gssapi. Good catch. http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20 PS3, Line 20: #include <stdlib.h> > Nit: cstdlib instead Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@166 PS3, Line 166: return Status::OK(); > Do you also want coverage for the case where the callback returns a bad Sta the current use case for this thing is just for tests, so I'll make it a void callback instead of returning Status. I'll extend it to return Status later if we find an authorization use case for it. http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@204 PS3, Line 204: > Nit: extra blank line here. Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@238 PS3, Line 238: well-formed token > Should probably prove that you can authenticate using this token first. Oth well, the token here is well-formed but invalid, since each run of the test is a different KDC, principal, etc. But it still turned up overflow bugs just during parsing. I'll add a note indicating this. http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@143 PS3, Line 143: , > Nit: misplaced comma Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@153 PS3, Line 153: neg_token = ""; > This is neg_token's default value; you can invert the condition and combine Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@459 PS3, Line 459: s = Status::RuntimeError("SPNEGO indicated complete, but got empty principal"); > Any particular reason the status message shouldn't also include the address the status here would just get propagated back to the requester as part of the HTTP response, and the requester already knows their own IP. http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h File src/kudu/server/webserver_options.h: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h@23 PS3, Line 23: #include <stdint.h> > Nit: since you're here already, could you convert this into cstdint? Done http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc@111 PS3, Line 111: > Nit: extra blank line here. Done http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh@653 PS3, Line 653: # Configure for a very minimal install - basically only HTTP(S), since we only : # use this for testing our own HTTP/HTTPS endpoints at this point in time. > May want to update this. Done -- To view, visit http://gerrit.cloudera.org:8080/13341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 Gerrit-Change-Number: 13341 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 07 Jun 2019 04:51:02 +0000 Gerrit-HasComments: Yes
