Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11750 )
Change subject: KUDU-2542: add initial authorization token impl ...................................................................... Patch Set 2: (26 comments) 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@45 PS1, Line 45: > warning: using decl 'make_shared' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@56 PS1, Line 56: namespace security { > warning: 'kNumBits' is a static definition in anonymous namespace; static i Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@306 PS1, Line 306: ASSERT_OK(signer.CheckNeedKey(&key)); : ASSERT_NE(nullptr, key.get()); : ASSERT_EQ(kE > I think it's simpler: TokenSigner::AddKey() method requires monotonically i Nice, thanks! http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@358 PS1, Line 358: // Test importing expired keys. The signer should be OK with it. > Also, it would be nice to add a few scenarios related to expired authz toke Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@362 PS1, Line 362: ASSERT_OK(GeneratePrivateKey > nit: move closer to the point of usage. Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@364 PS1, Line 364: ASSERT_OK(private_key.ToString(&private > Is this important for the test case? If not, remove this. Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@395 PS1, Line 395: ST_F(TokenTest, TestGenerateAuthzToken) { > I'm not sure it's worth adding a method just for this piece. Done 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@31 PS1, Line 31: apply. > 'apply' Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@37 PS1, Line 37: // privileges specified in `column_privileges`. > I'm weakly in favor of not having an 'all_privileges' flag, and instead tra SGTM http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@39 PS1, Line 39: : // If set, the user is authorized to insert rows into the table. : optional bool insert_privilege= 4; : : // If set, the user is authorized > Maybe, use oneof (union-like message) for 'scan_privilege' and 'column_priv Hrm, I'm somewhat on the fence about this. As implemented, that would make sense. OTOH, if we ever decide to implement per-column write privileges, or other kinds of column-level privileges, it might be a hassle to untangle it. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@49 PS1, Line 49: // Per-column privileges, indexed b > We don't support authz on per-column basis for update, right? Right, if a user wants to update a single column, they will need update privileges on the entire table. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@64 PS1, Line 64: // unix > That was a generic question regarding current authz token design and whethe It's a good question; I chatted a bit about this with Dan in person. I think we're fine leaving it as is, since we don't expect single-table tokens to change. Generally they're used to authorize requests to tservers, which are unaware of tables anyway. From there, we can assume requests to tservers are either scoped to a tablet (and thus scoped to a table), or are not scoped to a single tablet and in fact originate from tooling. Also, a nice thing about protobuf is that optional fields are compatible with repeated fields if we choose to make this repeated in the future. See bullet 10 here: https://developers.google.com/protocol-buffers/docs/proto#updating optional is compatible with repeated. Given serialized data of a repeated field as input, clients that expect this field to be optional will take the last input value if it's a primitive type field or merge all input elements if it's a message type field. 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 Ah, no, I was unsure of why this sentence was important so I removed it. I'll add something to the effect of "It's not crucial so we don't verify the beginning of the period" http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@31 PS1, Line 31: namespace kudu { > although this looks like it may not even be needed; I don't see optional us Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@84 PS1, Line 84: ding public part of th > I don't quite follow the need to add this sentence. Is this a term mentione Indeed http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@109 PS1, Line 109: ---- > Why is 'max' important here? Is there a new configuration parameter 'max' Oops, I wrote this when first grokking the code and conflated the key validity period with the token validity period. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@124 PS1, Line 124: consider tokens signed by the given key as valid. At the end : // of this > I am not sure why is that necessary? >From the naming and the existing descriptions, it wasn't obvious to me that >the activity interval and rotation interval are referring to the same thing: a >token is active for a given amount of time, after which, we rotate keys. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@141 PS1, Line 141: 'max_ > nit: drop Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@197 PS1, Line 197: > nit: neither 'key_validity_second' nor 'validity_seconds' is a parameter an Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@211 PS1, Line 211: > I know there is a lot of detailed description of why we end it up with this I agree, but in this case I'd prefer to avoid making this longer than it already is, particularly since the information is already all here. Instead, I added a pointer to the other relevant equation up above. If anything else could use clarification, please call it out. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@289 PS1, Line 289: rname' and table p > nit: add a comment? Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@304 PS1, Line 304: > Why is it needed in public section? Done 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@69 PS1, Line 69: authn_token_validity_seconds_, authz_token_validity_seconds)) > The braces shouldn't be necessary here. Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@150 PS1, Line 150: authz->set_username(std::move(username)); > c++11 modernize nit: would Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@151 PS1, Line 151: *authz->mutable_table_privilege() = std::move(privile > c++11 modernize nit: Done http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@158 PS1, Line 158: *signed_token = std::move > c++11 modernize nit: I think *signed_token = std::move(ret) could work here Done -- 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: 2 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: Wed, 24 Oct 2018 23:59:20 +0000 Gerrit-HasComments: Yes
