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 Fri, 17 Nov 2017 00:28:41 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 3:

(15 comments)

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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@63
PS3, Line 63: if (!FLAGS_principal.empty() || !FLAGS_be_principal.empty()) {
> Actually, https://github.com/apache/incubator-impala/blob/master/be/src/rpc
You're right, FLAGS_principal is the "switch" to turn on kerberos. So there's no reason to
check be_principal here. Done.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@64
PS3, Line 64: std::string
> string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@67
PS3, Line 67: std::string
> string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/thrift-server-test.cc@99
PS3, Line 99:     kdc_wrapper_.reset(new MiniKdcWrapper("impala/localhost", "KRBTEST.COM",
"24h", "7d", kdc_port));
> nit: long line
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@48
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.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@49
PS3, Line 49: string
> principal string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@54
PS3, Line 54:  
> nit: blank space
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@62
PS3, Line 62: DissectKerberosPrincipal
> Will ParseKerberosPrincipal() a more appropriate name ?
Yup, changed it.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@63
PS3, Line 63:  
> nit: blank space
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@55
PS3, Line 55: std::string
> nit: string.
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@66
PS3, Line 66: std::string
> nit: string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@67
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.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@76
PS3, Line 76:  if (FLAGS_principal.empty()) return Status::OK();
> Please see comments in rpc-mgr.cc. 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.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@77
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
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@90
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 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: 3
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Nov 2017 00:28:41 +0000
Gerrit-HasComments: Yes

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