Dan Burkert has posted comments on this change. Change subject: [security] Add require_authentication option to C++ client ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/6121/1/src/kudu/client/client_builder-internal.h File src/kudu/client/client_builder-internal.h: Line 37: std::string authn_creds_; > need to initialize this in the ctor (or here) Done http://gerrit.cloudera.org:8080/#/c/6121/1/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: PS1, Line 178: } else if (boost::iequals(FLAGS_rpc_authentication, "required")) { : new_msgr->authentication_ = RpcAuthentication::REQUIRED; : } else if (boost::iequals(FLAGS_rpc_authentication, "optional")) { : new_msgr->authentication_ = RpcAuthentication::OPTIONAL; : } else if (boost::iequals(FLAGS_rpc_authentication, "disabled")) { : new_msgr->authentication_ = RpcAuthentication::DISABLED; : } else { : return Status::InvalidArgument( : "--rpc_authentication flag must be one of 'required', 'optional', or 'disabled'"); : } > Is it possible to validate these flags regardless of whether the authentica Right now there aren't any codepaths that could use both gflags and the explicit authentication setting. The gflags happen in the servers and cli tools, and the explicit setting happens in client applications. As far as the validation, I'm going to fix that in a followup commit. http://gerrit.cloudera.org:8080/#/c/6121/1/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: PS1, Line 220: if (authentication != RpcAuthentication::DISABLED && !FLAGS_keytab_file.empty()) { : RETURN_NOT_OK(server_negotiation.EnableGSSAPI()); : } : if (authentication != RpcAuthentication::REQUIRED) { : RETURN_NOT_OK(server_negotiation.EnablePlain()); : } > What if FLAGS_keytab_file.empty() and (authentication == RpcAuthentication: Then the server won't have any SASL mechanisms to offer. It could still authenticate via token or cert. Line 227: server_negotiation.set_deadline(deadline); > Is it going to be some warning or something alike if authentication is set Yah, we should check that if servers have RpcAuthentication==REQUIRED, that they also have either kerberos credentials or an external PKI cert/ca. I'm going to add this to messenger.cc, because I think it's the least invasive place to put it. http://gerrit.cloudera.org:8080/#/c/6121/1/src/kudu/rpc/negotiation.h File src/kudu/rpc/negotiation.h: PS1, Line 23: #include "kudu/util/monotime.h" > Would forward declaration of RpcAthentication enum help instead of includin Great idea. -- To view, visit http://gerrit.cloudera.org:8080/6121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7cfec58695df5f277e694735ca885e4e2915ebb Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
