Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname > I actually prefer to keep it public so the user can configure it for cases That's fair. Maybe we should add some validation, then, that the path is an absolute path before attempting to use it? My fear is that there are valid CCNAMEs such as MEMORY:foo which a user might be familiar with, and if they try to use that, Impala will have some hard-to-debug issues, since those CCNAMEs are supported by the C++ side but not the Java side. http://gerrit.cloudera.org:8080/#/c/11331/4/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/4/be/src/rpc/rpc-mgr.cc@174 PS4, Line 174: LOG(ERROR) << err_msg; the log message probably include the 'requestor_string' which includes the remote IP address, since the admin probably wants as much detail as possible on the unauthorized access attempt. In the log message maybe also inlcude 'logged_in_username' so it's clear what username was _expected_ for this service (but don't send that back to the "attacker" to avoid giving them any extra info) http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc@84 PS3, Line 84: Status ParseKerberosPrincipal(const string& principal, string* service_name, > When writing this patch, I noticed the previous implementation made two cal In terms of whether to keep the change in the patch, I figured that since this patch has some security implications we may want to keep it minimal. The behavior of the new function and the old function are slightly different, eg in the case that for some reason you had a @ that came before the / or something. I dont think that would be a valid principal, but probably better to avoid any potential for behavior change even in a strange edge case when addressing a bug. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 29 Aug 2018 01:05:45 +0000 Gerrit-HasComments: Yes
