Alexey Serbin 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 9: (21 comments) http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc File src/kudu/tserver/tablet_server_authorization-test.cc: http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@113 PS9, Line 113: nit: alignment http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@118 PS9, Line 118: LOG(INFO) << "Sending write request"; nit: Does this information log message and others that accompany RPCs below add some useful info? If not, maybe drop those since our test are chatty enough already? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@128 PS9, Line 128: scan->set_read_mode(READ_LATEST); This can be dropped: it's already READ_LATEST by default. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@138 PS9, Line 138: const SignedTokenPB* : token nit: is LINT happy enough with this way of breaking the line? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@142 PS9, Line 142: req.set_target_chunk_size_bytes(1); Is setting this hint is really crucial? If yes, please add a comment why; otherwise don't set it, maybe? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@156 PS9, Line 156: scan->set_read_mode(READ_LATEST); I think this can be omitted since it's READ_LATEST by default. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@171 PS9, Line 171: virtual nit: drop 'virtual' since 'override' is present http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@204 PS9, Line 204: LOG(INFO) << "Generating token with a bad signature"; nit here and below: do these LOG(INFO) add useful information into the test? If not so much, maybe convert them into comments. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@208 PS9, Line 208: for (int i = 0; i < bad_signature.length(); i++) { : char* byte = &bad_signature[i]; : *byte ^= *byte; : } Isn't it just zeroing the signature? If so, maybe memset() would be easier to write and comprehend here? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@212 PS9, Line 212: bad_signature Does it makes sense to add a scenario supplying an authz token without a signature? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@227 PS9, Line 227: token_creators.emplace_back([&] { : LOG(INFO) << "Generating expired authz token"; : TokenSigningPrivateKeyPB tsk = GetTokenSigningPrivateKey(2); : shared_ptr<TokenVerifier> verifier(new TokenVerifier()); : TokenSigner expired_signer(3600, /*authz_token_validity_seconds=*/1, 3600, verifier); : CHECK_OK(expired_signer.ImportKeys({ tsk })); : vector<TokenSigningPublicKeyPB> expired_public_keys = verifier->ExportKeys(); : CHECK_OK(mini_server_->server()->mutable_token_verifier()->ImportKeys(public_keys)); : : SignedTokenPB token; : CHECK_OK(expired_signer.GenerateAuthzToken(kUser, privilege, &token)); : // Wait for the token to expire. : SleepFor(MonoDelta::FromSeconds(3)); : return token; : }); Does it make sense to add a scenario to sign authz token with a signing key that isn't known to the verifier? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@250 PS9, Line 250: CheckInvalidAuthzToken(s, rpc) Wrap into NO_FATALS()? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@258 PS9, Line 258: CheckInvalidAuthzToken(s, rpc) Wrap into NO_FATALS()? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@284 PS9, Line 284: CheckInvalidAuthzToken(s, rpc) Wrap into NO_FATALS()? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@298 PS9, Line 298: ValuesIn(vector<RequestorFunc>({ &WriteGenerator, &ScanGenerator, : &SplitKeyRangeGenerator, &ChecksumGenerator }) Would Values(&WriteGenerator, &ScanGenerator, &SplitKeyRangeGenerator, &ChecksumGenerator) work here? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@146 PS9, Line 146: DEFINE_bool(tserver_enforce_access_control, false, Do we want to ensure that all tablet servers are run with the same values of the FLAGS_tserver_enforce_access_control? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@390 PS9, Line 390: bool has_column_privileges = false; : if (privilege.column_privileges_size() > 0) { : for (const auto& col_id_and_privilege : privilege.column_privileges()) { : if (col_id_and_privilege.second.scan_privilege()) { : has_column_privileges = true; : break; : } : } : } : return privilege.scan_privilege() || has_column_privileges; nit: maybe, short-circuit it like the following: if (privilege.scan_privilege()) { return true; } ... the code to find at least one column priviledge ... http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@406 PS9, Line 406: AuthorizableRequest* req Would 'const AuthorizableRequest&' suffice here? Why is it necessary to pass a pointer here? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@436 PS9, Line 436: token->Swap(&token_pb); nit: I think std::move() should work as well since we compile protobuf with C++11 support. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tserver.proto@429 PS9, Line 429: enum TabletServerFeatures { : UNKNOWN_FEATURE = 0; : COLUMN_PREDICATES = 1; : // Whether the server supports padding UNIXTIME_MICROS slots to 16 bytes. : PAD_UNIXTIME_MICROS_TO_16_BYTES = 2; Do we want to ensure a tablet server is be able to verify authz tokens? I.e., do we want to introduce a tablet server feature signalling about ability of tablet server to verify authz tokens? http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tserver_service.proto File src/kudu/tserver/tserver_service.proto: http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tserver_service.proto@100 PS9, Line 100: : // An authorization token with which to authorize this request. : optional security.SignedTokenPB authz_token = 8; I'm not sure I understand this -- that's the response part, right? Why don't we have authz_token in ChecksumRequestPB? >From another chapter, but do we make sure that user_id for >ContinueChecksumRequestPB is the same as in the ChecksumRequestPB that >originated the original checksum scan? -- 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: 9 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: Mon, 07 Jan 2019 01:03:55 +0000 Gerrit-HasComments: Yes
