Andrew Wong 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 10: (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 Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@118 PS9, Line 118: return proxy->Write(req, &resp, rpc); > nit: Does this information log message and others that accompany RPCs below I think it is useful; _because_ our tests are generally chatty, IMO it's important to have a few lines like these indicating when something interesting is happening, as a sort of checkpoint when going through the logs. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@128 PS9, Line 128: if (token) { > This can be dropped: it's already READ_LATEST by default. Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@138 PS9, Line 138: : req.set_tablet_id(TabletServerTes > nit: is LINT happy enough with this way of breaking the line? Done, seems LINT was indeed happy, interestingly enough. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@142 PS9, Line 142: } > Is setting this hint is really crucial? If yes, please add a comment why; Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@156 PS9, Line 156: } > I think this can be omitted since it's READ_LATEST by default. Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@171 PS9, Line 171: > nit: drop 'virtual' since 'override' is present Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@204 PS9, Line 204: // Flip the bits in the signature. > nit here and below: do these LOG(INFO) add useful information into the test Yes, specifically when debugging. Since each of these TokenCreators are run, if any fail, these logs are helpful in figuring out what went wrong. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@208 PS9, Line 208: } : token.set_token_data(std::move(bad_signature)); : return token; : }); > Isn't it just zeroing the signature? If so, maybe memset() would be easier Ah, I meant to flip the bits. I'll add a comment. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@212 PS9, Line 212: > Does it makes sense to add a scenario supplying an authz token without a si Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@227 PS9, Line 227: SignedTokenPB token; : CHECK_OK(signer.GenerateAuthnToken(kUser, &token)); : return token; : }); : 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; : C > Does it make sense to add a scenario to sign authz token with a signing key It does, I have that in pt 2 of this patch series here https://gerrit.cloudera.org/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc#366 http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@250 PS9, Line 250: r (const auto& token_creator : > Wrap into NO_FATALS()? Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@258 PS9, Line 258: > Wrap into NO_FATALS()? Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@284 PS9, Line 284: SignedTokenPB token; > Wrap into NO_FATALS()? Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@298 PS9, Line 298: : } > Would Values(&WriteGenerator, &ScanGenerator, &SplitKeyRangeGenerator, &Che Done 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 o It might be nice, but I don't think it's necessary. We don't enforce such consistency for other related flags, like `superuser_acl` or `user_acl`. http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@390 PS9, Line 390: if (privilege.scan_privilege()) { : return true; : } : if (privilege.column_privileges_size() > 0) { : for (const auto& col_id_and_privilege : privilege.column_privileges()) { : if (col_id_and_privilege.second.scan_privilege()) { : return true; : } : } : } > nit: maybe, short-circuit it like the following: Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@406 PS9, Line 406: const TokenVerifier& tok > Would 'const AuthorizableRequest&' suffice here? Why is it necessary to pa Done http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_service.cc@436 PS9, Line 436: } > nit: I think std::move() should work as well since we compile protobuf with Done 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. I'm not sure what benefit that would provide, considering it's normal to send a token to a tserver that doesn't support tokens (e.g. if communicating with an older versioned tserver). 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: : : > I'm not sure I understand this -- that's the response part, right? Why don Done; good catch, this doesn't belong at all. It's a part of the ChecksumRequestPB via NewScanRequestPB. Yes, see https://github.com/apache/kudu/commit/e172df405aef47b1339c9879b835baf69b539f8c -- 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: 10 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 08:08:39 +0000 Gerrit-HasComments: Yes
