Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC ...................................................................... Patch Set 3: (5 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 > Should this be hidden? I dont think we'd want to support users changing it, I actually prefer to keep it public so the user can configure it for cases such as KUDU-2545. I will add a remark about its being advanced option. 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 th Left over from previous patch. Removed. 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 Sure. Will do it as a clean-up in a follow-on patch. That said, my understanding is that the reply itself is processed asynchronously. 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 e Done 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 im When writing this patch, I noticed the previous implementation made two calls split() which seems unnecessary. I fixed it as part of this patch as I was calling it in the previous version of this patch but this is not needed anymore in the new version. It doesn't hurt to keep it though. I am not sure why krb5_parse_name() wasn't used here in the previous implementation. Given the Kudu code has better infrastructure, it seems easier for me to add a utility function to Kudu and call it here instead. Filed IMPALA-7504 for this. -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 29 Aug 2018 00:24:31 +0000 Gerrit-HasComments: Yes
