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 12: Code-Review+2 (7 comments) http://gerrit.cloudera.org:8080/#/c/11751/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11751/12//COMMIT_MSG@13 PS12, Line 13: negotiation nit: drop? http://gerrit.cloudera.org:8080/#/c/11751/12/src/kudu/tserver/tablet_server-test-base.h File src/kudu/tserver/tablet_server-test-base.h: http://gerrit.cloudera.org:8080/#/c/11751/12/src/kudu/tserver/tablet_server-test-base.h@127 PS12, Line 127: static const char* kTableId; : static const char* kTabletId; nit: if making this public, maybe make sure it's not possible to change the pointers as well? static const char* const k{Table,Talbet}Id; 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@204 PS9, Line 204: CHECK_OK(signer.GenerateAuthzToken(kUser, privilege, > Yes, specifically when debugging. Since each of these TokenCreators are run SGTM http://gerrit.cloudera.org:8080/#/c/11751/9/src/kudu/tserver/tablet_server_authorization-test.cc@212 PS9, Line 212: > Done Thanks! 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 authn token instead of authz token"; : 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)); : > It does, I have that in pt 2 of this patch series here https://gerrit.cloud SGTM 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, > It might be nice, but I don't think it's necessary. We don't enforce such c SGTM 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: optional ResourceMetricsPB resource_metrics = 7; : } : > Done; good catch, this doesn't belong at all. It's a part of the ChecksumRe Great, thank for the clarification. -- 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: 12 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: Sun, 27 Jan 2019 22:35:11 +0000 Gerrit-HasComments: Yes
