Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 )
Change subject: WIP KUDU-2543: pass around default authz tokens ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@483 PS1, Line 483: n z ? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc@422 PS1, Line 422: LOG(FATAL) This behavior assumes other error cases (like empty remote_user().username()) are filtered out at an earlier phase. However, in the new code I don't see it verified. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h@175 PS1, Line 175: template <class Server, class RequestPB, class ResponsePB> : void RetriableRpc<Server, RequestPB, ResponsePB>::GetNewAuthzTokenAndRetryCb( : const Status& status) { : if (status.ok()) { : // Perform the RPC call with the newly fetched authz token. : mutable_retrier()->mutable_controller()->Reset(); : SendRpc(); : } else { : // Back to the retry sequence, hoping for better conditions after some time. : VLOG(1) << "Failed to get new authz token: " << status.ToString(); : mutable_retrier()->DelayedRetry(this, status); : } : } This looks identical to GetNewAuthnTokenAndRetryCb() with exception of VLOG() message. Does it make sense to unify those two methods? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h File src/kudu/rpc/rpc_verification_util.h: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@37 PS1, Line 37: RPC RPC error code? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc File src/kudu/rpc/rpc_verification_util.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@45 PS1, Line 45: Status s nit here and below: why this variable is necessary? Could it be just *error = ...; return Status::NotAuthorized(...); ? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@46 PS1, Line 46: *error = retry_error; std::move ? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@388 PS1, Line 388: FATAL_INVALID_AUTHORIZATION_TOKEN I would expect it to be FATAL_UNAUTHORIZED instead. Could you add the explanation into a comment regarding this choice? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@402 PS1, Line 402: ErrorStatusPB::FATAL_UNAUTHORIZED Why is FATAL_UNAUTHORIZED code chosen here instead of INVALID_AUTHZ_TOKEN? It would be nice to explain that in a comment. Is that related to the authz token re-acquisition logic? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1390 PS1, Line 1390: req->has_new_scan_request() It seems already requests to existing scanners are not going to be examined regarding the access permissions. Does it sound like a security issue (e.g., someone could grab already existing scan knowing its ID even if they don't have proper scan privileges)? If it does not sound like an issue, why not to move the authz_token into the NewScanRequestPB instead of keeping it in ScanRequestPB? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1489 PS1, Line 1489: TabletServiceImpl::SplitKeyRange Maybe, add a TODO into this method regarding verification of authz token in the request? -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Oct 2018 17:59:16 +0000 Gerrit-HasComments: Yes
