Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC ...................................................................... Patch Set 3: (6 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 > nit: Maybe krb5_ccpath ? Or krb5ccname_path Seems more intuitive to have a name which resembles the env var KRB5CCNAME. That env variable indicates the location of the credentials cache so it seems a bit redundant to add path to its name as we also don't use krb_conf_path below. Please let me know if you feel strongly about it though. KRB5CCNAME Location of the Kerberos 5 credentials (ticket) cache. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h@216 PS2, Line 216: > Just to add, if we're going to keep the service name fixed, this should pro Variable removed as this is no longer necessary per suggestion to use GetLoggedInUserNameFromKeytab(). http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@118 PS2, Line 118: nused_realm)); > I think this should actually be hardcoded to "impala" or something rather t For all practical purposes, that's true. However, doing so will also require hardcoding part of FLAGS_principal / FLAGS_be_principal as that's what's used for kinit. Hardcoding the principal is almost the same as deprecating FLAGS_principal / FLAGS_be_principal and switching to using a boolean flag FLAGS_kerberos_enabled. This seems to be a larger scope than I feel comfortable with for this change. I need more thought about this as I have to understand the historical context for having FLAGS_principal / FLAGS_be_principal to begin with. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@161 PS2, Line 161: // Authorization is enforced iff Kerberos is enabled. > invert this condition so that you can un-nest the majority of the function. Done http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@164 PS2, Line 164: ername matches that of the kinit'ed principal. : const RemoteUser& remote_user = context->remote_user > sorry I skipped half of what I meant to say in this comment. What I meant i Good point. Done. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/testutil/mini-kdc-wrapper.h File be/src/testutil/mini-kdc-wrapper.h: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/testutil/mini-kdc-wrapper.h@43 PS2, Line 43: static Status SetupAndStartMiniKDC(std::string realm, : std::string ticket_lifetime, std::string renew_lifetime, > why these changes? it's better to take the arguments by copy, and then std: Done -- 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: 3 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: Tue, 28 Aug 2018 18:27:24 +0000 Gerrit-HasComments: Yes
