Todd Lipcon has posted comments on this change. Change subject: security: initial work on token creation and verification ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token.proto File src/kudu/security/token.proto: Line 33: UNKNOWN = 999; > What's the point of the UNKNOWN variant, aren't we just going to check that it wouldn't let me define an enum with no values :( http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signer.cc File src/kudu/security/token_signer.cc: Line 65: unique_ptr<TokenSigningKey> new_tsk(new TokenSigningKey(std::move(tsk_pb))); > warning: passing result of std::move() as a const reference argument; no mo Done http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signer.h File src/kudu/security/token_signer.h: PS2, Line 70: . > Missing close paren. Done PS2, Line 98: virtual > Why virtual? my little yasnippet-mode template does this by default since non-virtual constructors are bad news with inheritance... but we don't intend this to be inherited so I'll remove. http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signing_key.cc File src/kudu/security/token_signing_key.cc: Line 22: #include "kudu/util/status.h" > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signing_key.h File src/kudu/security/token_signing_key.h: Line 36: explicit TokenSigningKey(TokenSigningKeyPB pb); > If PBs can't be moved this may as well take a const ref. Done http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.cc File src/kudu/security/token_verifier.cc: PS2, Line 60: // Sanity check the key. : CHECK(!pb.has_private_key_der()); > I agree in principal that this is a serious enough check that its fine to c yea, Alexey brought that up elsewhere, so I'll work on removing it Line 107: if (tsk->pb().expire_unix_epoch_seconds() < now) { > It's probably worth a DCHECK here to ensure that tsk->pb().expire_unix_epoc hrm, it's useful in tests to break that constraint though, to force the code path of an expired key. Even though it's a constraint that is necessary to "make sense", the only thing that happens due to it is early expiration, so I'd be hesitant to actually crash a server. http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.h File src/kudu/security/token_verifier.h: PS2, Line 52: enum > I agree, would be better to have this be an enum class defined outside of T Done -- To view, visit http://gerrit.cloudera.org:8080/5796 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
