impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
Date Wed, 29 Nov 2017 02:02:10 GMT
Michael Ho has posted comments on this change. ( )

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

Patch Set 6:

File be/src/rpc/
PS6, Line 1033:   RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal));
              :   RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal));
              :   if (!kerberos_internal_principal.empty()) {
              :     RETURN_IF_ERROR(InitKerberosEnv());
              :   }
Does it make sense to write it as:

if (IsKerberosEnabled()) {
PS6, Line 1055: !kerberos_internal_principal.empty()
if (IsKerberosEnabled()) {
PS6, Line 1070: !kerberos_external_principal.empty()
IsKerberosEnabled(). Should we just declare the following above and use it everywhere in this
function :
 bool use_kerberos = IsKerberosEnabled() ?
File be/src/rpc/rpc-mgr.h:
PS6, Line 180:   /// The following strings preserve the Kudu flags original values to restore
             :   /// Shutdown() as they will be modified by us.
             :   string flag_save_rpc_authentication;
As mentioned in another place, is this actually necessary if we are shutting down ?
File be/src/rpc/
PS6, Line 75:     FLAGS_rpc_authentication = "required";
As described in IMPALA-6256, FLAGS_be_principal may not be picked up by the KRPC stack.

Also, it may not be obvious by reading the code that KRPC code is implicitly relying on FLAGS_principal
to set the Kerberos principal too. May help to document here.
PS6, Line 128: FLAGS_rpc_authentication = flag_save_rpc_authentication;
Is there a reason why we have to restore this flag if we are shutting down ? What could be
the consequence if we don't restore this flag ?
File be/src/testutil/mini-kdc-wrapper.h:
PS6, Line 38: /// If the mode is USE_KUDU_KERBEROS or USE_IMPALA_KERBEROS, the MiniKdc which
is a wrapper
nit: long line
PS6, Line 71:   std::string ticket_lifetime_;
PS6, Line 74:   std::string renew_lifetime_;
PS6, Line 77:   int kdc_port_;
File be/src/util/auth-util.h:
PS6, Line 49: /// 'out_principal' will contain the principal string if the cluster is configured
to use
            : /// kerberos, otherwise it will be an empty string.
As mentioned in, we may want to call this function only when Kerberos is enabled.
File be/src/util/
PS6, Line 75: FLAGS_principal.empty()

Actually, should this actually be a DCHECK(IsKerberosEnabled()) ?
There should be no reason for a caller to obtain the principal if it's not enabled, right

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: Wed, 29 Nov 2017 02:02:10 +0000
Gerrit-HasComments: Yes

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