impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
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. ( )

Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

Patch Set 6:

File be/src/rpc/auth-provider.h:
PS5, Line 27: #include "util/thread.h"
> What is this for ?
This is for the 'kinit_thread_' below. We hadn't IWYU'd before.
File be/src/rpc/auth-provider.h:
PS4, Line 78:   /// Wrap the client transport with a new TSaslClientTransport.  This is only
            :   /// internal connections.  Since, as a daemon, we only do Kerberos and not
            :   /// 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 
File be/src/rpc/
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
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
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.
PS5, Line 1036:     RETURN_IF_ERROR(InitKerberosEnv());
> nit: blank line not needed.
File be/src/rpc/
PS5, Line 63: FLAGS_num_reactor_thread
> IsKerberosEnabled()
PS5, Line 70:     string service_name, unused_hostname, unused_realm;
> It may be better to set FLAGS_rpc_authentication (defined by Kudu) to "requ
File be/src/testutil/mini-kdc-wrapper.h:
PS5, Line 53: //
> nit: ///
PS5, Line 86:   /// Stops the KDC by terminating the krb5kdc subprocess.
> Please add comments about what clean up it may do.
File be/src/testutil/
PS5, Line 67: !
> != seems to make less assumption about the ordering of the ENUM values.
PS5, Line 88: !
> !=
File be/src/util/
PS5, Line 86:   split(names, principal, is_any_of("/"));
            :   if (names.size() != 2) return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal);
> Should this live in ?
Done. Added to generate_error_codes.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Tue, 28 Nov 2017 15:00:03 +0000
Gerrit-HasComments: Yes

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