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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 02 Jan 2019 20:41:38 +0000 Gerrit-HasComments: Yes
