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 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/auth-provider.h@27 PS5, Line 27: #include "util/thread.h" > What is this for ? This is for the 'kinit_thread_' below. We hadn't IWYU'd before. http://gerrit.cloudera.org:8080/#/c/8270/4/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: http://gerrit.cloudera.org:8080/#/c/8270/4/be/src/rpc/auth-provider.h@78 PS4, Line 78: /// Wrap the client transport with a new TSaslClientTransport. This is only for : /// internal connections. Since, as a daemon, we only do Kerberos and not LDAP, : /// we can go straight to Kerberos. : /// This is only applicable to Thrift connections and not KRPC connections. : virtual Status WrapClientTransport(const std::string& hostname, : boost::shared_ptr<apache::thrift::transport::TTransport> raw_transport, : const std::string& service_name, > Is this only used for thrift connections ? May be it helps to state it now Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@643 PS5, Line 643: // Kudu Client and Kudu RPC shouldn't attempt to initialize SASL which would conflict : // with Impala's SASL initialization. This must be call > Please add a small comment that this overlaps with the initialization below Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@658 PS5, Line 658: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, appname); : sasl::TSaslClient::SaslInit(GENERAL_CALLBACKS); : } catch (sasl::SaslServerImplException& e) { > Once we completely move away from Thrift, do we rely on Kudu to initialize We will still have Thrift for external connections, so my take is that we can leave all the SASL initialization in Impala's control so we can add the kinds of callbacks, etc. that we like. http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@1034 PS5, Line 1034: RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal)); : if (!kerberos_internal_principal.empty()) { > The internal and external principals look like good candidates to be stored We can do that, however, I think it adds some unnecessary complexity for the authentication classes to depend on the ExecEnv. For eg, some of the backend tests (including the rpc-mgr-test and thrift-server-test) do not have an ExecEnv but use the authentication classes. In that case, we'd need to add ExecEnv objects to the tests just for this. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@1036 PS5, Line 1036: RETURN_IF_ERROR(InitKerberosEnv()); > nit: blank line not needed. Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/rpc-mgr.cc@63 PS5, Line 63: FLAGS_num_reactor_thread > IsKerberosEnabled() Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/rpc-mgr.cc@70 PS5, Line 70: string service_name, unused_hostname, unused_realm; > It may be better to set FLAGS_rpc_authentication (defined by Kudu) to "requ Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.h File be/src/testutil/mini-kdc-wrapper.h: http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.h@53 PS5, Line 53: // > nit: /// Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.h@86 PS5, Line 86: /// Stops the KDC by terminating the krb5kdc subprocess. > Please add comments about what clean up it may do. Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.cc File be/src/testutil/mini-kdc-wrapper.cc: http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.cc@67 PS5, Line 67: ! > != seems to make less assumption about the ordering of the ENUM values. Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.cc@88 PS5, Line 88: ! > != Done http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/util/auth-util.cc@86 PS5, Line 86: split(names, principal, is_any_of("/")); : if (names.size() != 2) return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal); : > Should this live in generate_error_codes.py ? Done. Added to generate_error_codes. -- 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: 6 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Tue, 28 Nov 2017 15:00:03 +0000 Gerrit-HasComments: Yes
