Michael Ho 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: (16 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()) { Based on the logic in GetInternalKerberosPrincipal(), we will not set the internal_principal if FLAGS_principal is empty even if FLAGS_be_principal is not empty. What's the expected relationship between FLAGS_principal and FLAGS_be_principal ? http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@64 PS3, Line 64: std::string string http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@67 PS3, Line 67: std::string string 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 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 connections between Impala backend nodes http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@49 PS3, Line 49: string principal string http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@54 PS3, Line 54: nit: blank space 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 ? http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@63 PS3, Line 63: nit: blank space http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/util/auth-util.h File be/src/util/auth-util.h: http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/util/auth-util.h@55 PS2, Line 55: t_principal' will contai Will ParseKerberosPrincipal() be a more suitable name ? 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. http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@66 PS3, Line 66: std::string nit: string 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: *out_principal = FLAGS_principal; if (out_principal->empty()) return Status::OK(); 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 case : FLAGS_principal.empty() && !FLAGS_be_principal.empty() 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_principal; RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal)); 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 '<hostname>@<service>/<realm> or any permutation of the expected form ? -- 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: Thu, 16 Nov 2017 08:53:16 +0000 Gerrit-HasComments: Yes
