impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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. ( http://gerrit.cloudera.org:8080/8270 )

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


Patch Set 6:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1033
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()) {
RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal));
RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal));
RETURN_IF_ERROR(InitKerberosEnv());
}


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1055
PS6, Line 1055: !kerberos_internal_principal.empty()
if (IsKerberosEnabled()) {
    DCHECK(!kerberos_internal_principal.empty());
    ....
 }


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1070
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() ?


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.h@180
PS6, Line 180:   /// The following strings preserve the Kudu flags original values to restore
in
             :   /// 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 ?


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@75
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.


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@128
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 ?


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h
File be/src/testutil/mini-kdc-wrapper.h:

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@38
PS6, Line 38: /// If the mode is USE_KUDU_KERBEROS or USE_IMPALA_KERBEROS, the MiniKdc which
is a wrapper
nit: long line


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@71
PS6, Line 71:   std::string ticket_lifetime_;
const


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@74
PS6, Line 74:   std::string renew_lifetime_;
const


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@77
PS6, Line 77:   int kdc_port_;
const


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.h
File be/src/util/auth-util.h:

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.h@49
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 auth-util.cc, we may want to call this function only when Kerberos is enabled.


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.cc
File be/src/util/auth-util.cc:

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.cc@75
PS6, Line 75: FLAGS_principal.empty()
!IsKerberosEnabled().

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

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