Dan Burkert has posted comments on this change. Change subject: security: initial work on token creation and verification ......................................................................
Patch Set 2: (8 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 all of the features are valid enum variants? PS2, Line 47: incompatible_features Maybe this should be 'required_features'? 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. PS2, Line 98: virtual Why virtual? 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. 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()); > Yep, of course we assume the core files are going to have sensitive data. I agree in principal that this is a serious enough check that its fine to crash a production server over it, but that being said I'm not sure why there is even a private_key_der field to begin with. When would we ever want to send the private key? Line 107: if (tsk->pb().expire_unix_epoch_seconds() < now) { It's probably worth a DCHECK here to ensure that tsk->pb().expire_unix_epoch_seconds() < token.expire_unix_epoch_seconds(), since the signer should never break that constraint. 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 > nit: if it makes sense, consider using strictly types enums I agree, would be better to have this be an enum class defined outside of TokenVerifier. -- 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
