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 Fri, 17 Nov 2017 00:28:41 GMT
Sailesh Mukil has posted comments on this change. ( )

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

Patch Set 3:

File be/src/rpc/
PS3, Line 63: if (!FLAGS_principal.empty() || !FLAGS_be_principal.empty()) {
> Actually,
You're right, FLAGS_principal is the "switch" to turn on kerberos. So there's no reason to
check be_principal here. Done.
PS3, Line 64: std::string
> string
PS3, Line 67: std::string
> string
File be/src/rpc/
PS3, Line 99:     kdc_wrapper_.reset(new MiniKdcWrapper("impala/localhost", "KRBTEST.COM",
"24h", "7d", kdc_port));
> nit: long line
File be/src/util/auth-util.h:
PS3, Line 48: backend connections
> Actually, does this may also apply to impala <-> statestore connections ?
This applies to all daemon-daemon connections, so that includes impalad, statestored and catalogd.
PS3, Line 49: string
> principal string
PS3, Line 54:  
> nit: blank space
PS3, Line 62: DissectKerberosPrincipal
> Will ParseKerberosPrincipal() a more appropriate name ?
Yup, changed it.
PS3, Line 63:  
> nit: blank space
File be/src/util/
PS3, Line 55: std::string
> nit: string.
PS3, Line 66: std::string
> nit: string
PS3, Line 67:   *out_principal = std::string();
            :   if (FLAGS_principal.empty()) return Status::OK();
            :   *out_principal = FLAGS_principal;
> Is there any reason we cannot just write:
No, I changed it.
PS3, Line 76:  if (FLAGS_principal.empty()) return Status::OK();
> Please see comments in Not sure if it's possible to have the ca
As mentioned there, FLAGS_principal is the switch to turn on kerberos, so setting FLAGS_be_principal
without setting FLAGS_principal is a startup error.
PS3, Line 77:   if (FLAGS_be_principal.empty()) {
            :     *out_principal = FLAGS_principal;
            :     RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal));
            :   } else {
            :     *out_principal = FLAGS_be_principal;
            :     RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal));
            :   }
> *out_principal = !FLAGS_be_principal.empty() ? FLAGS_be_principal : FLAGS_p
PS3, Line 90: split(names, principal, is_any_of("/@"));
> Will this generate wrong result if the input principal is of the form '<hos
Yes, this function won't return an error for that. Should we do more in depth checking for
things like this? Or do we expect the user to follow the format? How much ever checking we
add, there will still be some cases where they can enter wrong input.

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: 3
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Fri, 17 Nov 2017 00:28:41 +0000
Gerrit-HasComments: Yes

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