Adar Dembo 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? 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 understand what 'header' means. 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? 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 ReleaseBufferOrWarn here because it'd call right back into ErrorToString. 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, could you add a comment explaining? 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 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 Status and causes authentication to fail? http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@204 PS3, Line 204: Nit: extra blank line here. 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. Otherwise the various token failure tests below are less credible because the token was never going to work anyway. 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 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 it with the condition in the else if. 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 as per the DFATAL just below? 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? 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. 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. -- 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 03:53:28 +0000 Gerrit-HasComments: Yes
