impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
Date Tue, 28 Nov 2017 15:00:03 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2017 15:00:03 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message