Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8270 )
Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos ...................................................................... Patch Set 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@63 PS3, Line 63: if (!FLAGS_principal.empty() || !FLAGS_be_principal.empty()) { > Actually, https://github.com/apache/incubator-impala/blob/master/be/src/rpc You're right, FLAGS_principal is the "switch" to turn on kerberos. So there's no reason to check be_principal here. Done. http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@64 PS3, Line 64: std::string > string Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@67 PS3, Line 67: std::string > string Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/thrift-server-test.cc File be/src/rpc/thrift-server-test.cc: http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/thrift-server-test.cc@99 PS3, Line 99: kdc_wrapper_.reset(new MiniKdcWrapper("impala/localhost", "KRBTEST.COM", "24h", "7d", kdc_port)); > nit: long line Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h File be/src/util/auth-util.h: http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@48 PS3, Line 48: backend connections > Actually, does this may also apply to impala <-> statestore connections ? This applies to all daemon-daemon connections, so that includes impalad, statestored and catalogd. http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@49 PS3, Line 49: string > principal string Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@54 PS3, Line 54: > nit: blank space Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@62 PS3, Line 62: DissectKerberosPrincipal > Will ParseKerberosPrincipal() a more appropriate name ? Yup, changed it. http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@63 PS3, Line 63: > nit: blank space Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@55 PS3, Line 55: std::string > nit: string. Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@66 PS3, Line 66: std::string > nit: string Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@67 PS3, Line 67: *out_principal = std::string(); : if (FLAGS_principal.empty()) return Status::OK(); : *out_principal = FLAGS_principal; > Is there any reason we cannot just write: No, I changed it. http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@76 PS3, Line 76: if (FLAGS_principal.empty()) return Status::OK(); > Please see comments in rpc-mgr.cc. Not sure if it's possible to have the ca As mentioned there, FLAGS_principal is the switch to turn on kerberos, so setting FLAGS_be_principal without setting FLAGS_principal is a startup error. http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@77 PS3, Line 77: if (FLAGS_be_principal.empty()) { : *out_principal = FLAGS_principal; : RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal)); : } else { : *out_principal = FLAGS_be_principal; : RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal)); : } > *out_principal = !FLAGS_be_principal.empty() ? FLAGS_be_principal : FLAGS_p Done http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@90 PS3, Line 90: split(names, principal, is_any_of("/@")); > Will this generate wrong result if the input principal is of the form '<hos Yes, this function won't return an error for that. Should we do more in depth checking for things like this? Or do we expect the user to follow the format? How much ever checking we add, there will still be some cases where they can enter wrong input. -- To view, visit http://gerrit.cloudera.org:8080/8270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd Gerrit-Change-Number: 8270 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Fri, 17 Nov 2017 00:28:41 +0000 Gerrit-HasComments: Yes
