You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/01/02 20:41:38 UTC
[kudu-CR] KUDU-2543 pt 1: basic checks for authz tokens
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 )
Change subject: KUDU-2543 pt 1: basic checks for authz tokens
......................................................................
Patch Set 7:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_header.proto@316
PS7, Line 316: // The authorization token is invalid or expired; the client should obtain
: // a new one.
: ERROR_INVALID_AUTHORIZATION_TOKEN = 17;
Curious why an invalid authz token is just an error while an invalid authn token is fatal?
Also, is it important to distinguish between an authn and an authz token in the errors? Or could we generalize the existing error into "ERROR_INVALID_TOKEN"? I guess that would only work if the two were non-fatal errors.
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc
File src/kudu/rpc/rpc_verification_util.cc:
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc@59
PS7, Line 59: return Status::NotAuthorized(VerificationResultToString(result));
Looks like this can be pulled out of the three cases where it's explicitly set and combined.
Also, should there be a catch-all that does something useful?
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/CMakeLists.txt
File src/kudu/tserver/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/CMakeLists.txt@180
PS7, Line 180: ADD_KUDU_TEST(tserver_authorization-test)
Nit: maybe tablet_server_authorization-test, or ts_authorization-test? Just thinking we should avoid introducing a new "tablet server" naming prefix for tests.
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@1438
PS7, Line 1438: if (!VerifyAuthzTokenOrRespond(server_->token_verifier(),
: &req->new_scan_request(), context, &token)) {
: return;
: }
: const auto& privilege = token.authz().table_privilege();
: if (!MayHaveScanPrivileges(privilege)) {
: context->RespondRpcFailure(rpc::ErrorStatusPB::FATAL_UNAUTHORIZED,
: Status::NotAuthorized("not authorized to scan"));
: return;
: }
: // TODO(awong): check the privileges required for the contents of the scan
: // request by pulling out the columns and checking against individual
: // column privileges.
Looks like this can be shared amongst Scan/Checksum/SplitKeyRange?
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@1562
PS7, Line 1562: scan
"split key range" I guess.
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@1703
PS7, Line 1703: scan
checksum
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver.proto@170
PS7, Line 170: message ListTabletsRequestPB {
Does this RPC need authorization since there's table metadata in the response?
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver_authorization-test.cc
File src/kudu/tserver/tserver_authorization-test.cc:
http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver_authorization-test.cc@227
PS7, Line 227: for (const auto& send_req : requestors) {
Doesn't look like there's any shared state between requestors, so why not parameterize the test on this, so that each requestor winds up in its own gtest? Is the key setup at the beginning expensive?
--
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: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Jan 2019 20:41:38 +0000
Gerrit-HasComments: Yes