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 5: (14 comments) Sorry for the delay. Some more 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 ? 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. : virtual Status WrapClientTransport(const std::string& hostname, : boost::shared_ptr<apache::thrift::transport::TTransport> raw_transport, : const std::string& service_name, : boost::shared_ptr<apache::thrift::transport::TTransport>* wrapped_transport); Is this only used for thrift connections ? May be it helps to state it now that we have two different RPC mechanisms in our code. http://gerrit.cloudera.org:8080/#/c/8270/4/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/8270/4/be/src/rpc/authentication.cc@643 PS4, Line 643: kudu::rpc::DisableSaslInitialization() Can you please add a comment as to why this is necessary and how is SASL initialized when KRPC is enabled ? 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_RETURN_IF_ERROR(kudu::rpc::DisableSaslInitialization(), : "Unable to disable Kudu RPC SASL initialization."); Please add a small comment that this overlaps with the initialization below. http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@658 PS5, Line 658: // We assume all impala processes are both server and client. : sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, appname); : sasl::TSaslClient::SaslInit(GENERAL_CALLBACKS); Once we completely move away from Thrift, do we rely on Kudu to initialize the Sasl library ? http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@1034 PS5, Line 1034: RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal)); : RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal)); The internal and external principals look like good candidates to be stored in ExecEnv and should be initialized once. http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@1036 PS5, Line 1036: nit: blank line not needed. 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_principal.empty() IsKerberosEnabled() http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/rpc-mgr.cc@70 PS5, Line 70: bld.set_sasl_proto_name(service_name); It may be better to set FLAGS_rpc_authentication (defined by Kudu) to "required" to make sure that Kerberos must be enabled if it's enabled via FLAGS_principal. 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: /// http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.h@86 PS5, Line 86: Status StopKdc(); Please add comments about what clean up it may do. 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. http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.cc@88 PS5, Line 88: > != 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: const Status BAD_FORMAT_STATUS = Status(strings::Substitute( : "Kerberos principal should be of the form: <service>/<hostname>@<realm> - got: $0", : principal)) Should this live in generate_error_codes.py ? Also, our existing implementation of Status has the unfortunate effect of creating a backtrace and logging to log file when creating a Status object. May be it's not preferable to pre-fabricate one here or it may confuse users to see this error message in the log file. -- 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: 5 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, 23 Nov 2017 02:13:59 +0000 Gerrit-HasComments: Yes
