Alexey Serbin has posted comments on this change. Change subject: [security] Add require_authentication option to C++ client ......................................................................
Patch Set 1: (4 comments) 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 authentication has been set explicitly? Also, I remember there was some good idea from Adar to make that check as early as possible, not waiting for the execution of this method. 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::REQUIRED) ? Line 227: server_negotiation.set_deadline(deadline); Is it going to be some warning or something alike if authentication is set to RpcAuthentication::REQUIRED but FLAGS_keytab_file is empty? http://gerrit.cloudera.org:8080/#/c/6121/1/src/kudu/rpc/negotiation.h File src/kudu/rpc/negotiation.h: PS1, Line 23: #include "kudu/rpc/messenger.h" Would forward declaration of RpcAthentication enum help instead of including this header file? -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
