Todd Lipcon has posted comments on this change. Change subject: Enable GSSAPI for servers and ExternalMiniCluster ......................................................................
Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/integration-tests/external_mini_cluster.cc File src/kudu/integration-tests/external_mini_cluster.cc: Line 145: "could not create client principal"); > Are the status' returned from these methods not detailed enough? Maybe we hrm.. they're not super specific - they'll say the subprocess exited but not why. The advantage of doing it here vs in the underlying method is that we get more context info here (i.e that it's the client principal that failed, and not some server principal), plus we only need one RETURN_NOT_OK_PREPEND whereas the underlying methods have several RETURN_NOT_OK()s http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: Line 123: bool enable_kerberos; > I think we need to have a discussion about what different configuration opt Yea, I agree that this and the gflag that I added elsewhere in this patch probably will end up changing a bit as we add other components of security and want to either have more fine-grained control or an overall "enable all the security stuff" flag. http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 55: DEFINE_bool(server_require_kerberos, false, > Why is 'server' in the flag name? Flags are only used server side. But servers also act as clients of other servers, and this only affects the server side of negotiation. I agree that we need to re-evaluate the flags before shipping, but not sure we could default to requiring authentication. Many people don't have Kerberos infrastructure, and we don't have any other user authentication mechanism in scope (nor is there really any secondary standard that's in use in the Hadoop ecosystem). Are you OK adding this as is, given I marked it experimental with the TODO below? http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: Line 71: void SetEnvVars(std::map<std::string, std::string> env); > This is good, but it looks like we aren't using it anywhere? We are from external_mini_cluster.cc:638 where we pass the appropriate krb5 env vars into the daemons -- To view, visit http://gerrit.cloudera.org:8080/4765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I595469e9cc8b2b2f57e9726014eeeb8112595801 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
