Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC ...................................................................... Patch Set 3: (6 comments) Didn't review the test changes much since I'm not as familiar with that code, hoping sailesh looked at those. 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 > Seems more intuitive to have a name which resembles the env var KRB5CCNAME. Should this be hidden? I dont think we'd want to support users changing it, and it's just for tests, right? http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.h@25 PS3, Line 25: #include "rpc/authentication.h" : #include "rpc/impala-service-pool.h" : #include "util/auth-util.h" are these new includes necessary? I don't see any new usages of types in this file. Would be better to include them from the .cc if necessary to cut down on compilation time. 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)); > For all practical purposes, that's true. Oh, I forgot that the sasl protocol name ends up being the expected remote principal for mutual auth. Never mind my comment, you're right. http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc@172 PS3, Line 172: mem_tracker->Release(context->GetTransferSize()); slight nit (though I guess this happens elsewhere also): shouldn't we wait until we've responded before we release memtracker consumption? the actual memory isn't freed until we have responded. I guess the code becomes a little messier because we have to grab context->GetTransferSize() before we respond. Not worth addressing here, but worth considering in other spots perhaps? http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc@173 PS3, Line 173: context->RespondFailure(kudu::Status::NotAuthorized( should we also log the unauthorized access attempt (perhaps including the expected username, for easier debugging if this goes wrong?) 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, I wonder whether we should just be using krb5_parse_name here instead of implementing our own parsing? According to http://web.mit.edu/kerberos/krb5-1.15/doc/appdev/refs/api/krb5_parse_name.html there are various escapings, etc, that this function isn't currently supporting. That said, why did you have to change this function for this patch? -- 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 20:28:09 +0000 Gerrit-HasComments: Yes
