Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11750 )
Change subject: WIP KUDU-2542: add initial authorization token impl ...................................................................... Patch Set 1: (15 comments) TokenSignerITest.AuthnTokenLifecycle failed because it relies on the custom setting for the FLAGS_authn_token_validity_seconds (custom value is much less than the default). Probably, it's necessary to set FLAGS_authz_token_validity_seconds to some lower value (maybe just 0). http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc File src/kudu/security/token-test.cc: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@306 PS1, Line 306: // If the generated key doesn't have the appropriate sequence number (i.e. : // not the one it should have gotten from CheckNeedKey(), the signer will : // complain. I think it's simpler: TokenSigner::AddKey() method requires monotonically increasing TSK sequence numbers. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@358 PS1, Line 358: TEST_F(TokenTest, TestGenerateAuthzToken) { > Add a test case covering different validity periods for authn and authz, ba Also, it would be nice to add a few scenarios related to expired authz tokens. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@362 PS1, Line 362: SignedTokenPB signed_token_pb; nit: move closer to the point of usage. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@364 PS1, Line 364: table_privilege.set_all_privileges(true); Is this important for the test case? If not, remove this. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@395 PS1, Line 395: static const int64_t kKeyValiditySeconds = signer.key_validity_seconds(); I'm not sure it's worth adding a method just for this piece. Also, what about line 247? If it's not possible to exist without key_validity_seconds() method, maybe make it private at least and make a this test a friend? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto File src/kudu/security/token.proto: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@39 PS1, Line 39: // If set, the user is authorized to select and apply predicates to all : // columns when scanning the table, and `column_privileges` is ignored. If : // unset, the user may only scan and apply predicates to columns with the : // privileges specified in `column_privileges`. : optional bool scan_privilege = 3; Maybe, use oneof (union-like message) for 'scan_privilege' and 'column_privileges'? https://developers.google.com/protocol-buffers/docs/proto#oneof http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@64 PS1, Line 64: optional Do we ever need to have privileges for multiple tables in one authz token? If yes, then maybe use 'repeated' here. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h File src/kudu/security/token_signer.h: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@a119 PS1, Line 119: : I see this piece has been dropped. Does it mean verifiers now enforce some constraints on the recipient usage period? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@109 PS1, Line 109: max Why is 'max' important here? Is there a new configuration parameter 'max' validity period? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@141 PS1, Line 141: I.e. nit: drop http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@197 PS1, Line 197: validity_seconds' nit: neither 'key_validity_second' nor 'validity_seconds' is a parameter anymore. It seems that was the case at some previous revision. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@304 PS1, Line 304: int64_t key_validity_seconds() const { return key_validity_seconds_; } Why is it needed in public section? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc File src/kudu/security/token_signer.cc: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@150 PS1, Line 150: authz->mutable_username()->assign(std::move(username)); c++11 modernize nit: would authz->set_username(std::move(username)) work here? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@151 PS1, Line 151: authz->mutable_table_privilege()->Swap(&(privilege)); c++11 modernize nit: *authz->mutable_table_privilege() = std::move(privilege); http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@158 PS1, Line 158: signed_token->Swap(&ret); c++11 modernize nit: I think *signed_token = std::move(ret) could work here as well. -- To view, visit http://gerrit.cloudera.org:8080/11750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id28747ec38675abdf50dce1e7c176d29213e370f Gerrit-Change-Number: 11750 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Oct 2018 03:59:38 +0000 Gerrit-HasComments: Yes
