Alexey Serbin has posted comments on this change. Change subject: [security] do actual token signing/verification ......................................................................
Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/5812/2/src/kudu/security/token_signing_key.cc File src/kudu/security/token_signing_key.cc: Line 35: TokenSigningPublicKey::TokenSigningPublicKey(const TokenSigningPublicKeyPB& pb) > oh, I missed your response to Dan. I think it's fishy here because this is It does not make much sense to me: there are multiple CHECK() statements in TokenVerifier::ImportPublicKeys(). If we are so concerned about externally-provided data, I suggest to address that separately and fix that part as well. http://gerrit.cloudera.org:8080/#/c/5812/6/src/kudu/security/token_signing_key.cc File src/kudu/security/token_signing_key.cc: Line 37: CHECK(pb_.has_rsa_key_der()); > +1 to what Dan said about doing an Init() method (or a factory function) There are CHECK() statements in TokenVerifie::ImportPublicKeys(). I don't think it's worth dancing around Init() here since those CHECKS() are in place anyway -- it will crash first there even if I added that Init(). Let's fix that in a separate changelist. Line 56: CHECK_OK(public_key.ToString(&public_key_der_, DataFormat::DER)); > can you make public_key_der_ a 'const' member so it's clear it just gets se I'm not sure this makes sense given the current signatures of these methods. If doing so, it would be necessary to do const_cast, which does not look good to me. Line 66: token->set_signature(signature); > I think you can save an allocation by doing: token->mutable_signature()->as Done http://gerrit.cloudera.org:8080/#/c/5812/6/src/kudu/security/token_signing_key.h File src/kudu/security/token_signing_key.h: Line 53: RsaPublicKey key_; > worth doccing that this is just a cached "parsed" version of the PB Done Line 78: std::string public_key_der_; > worth adding a doc that this is just a "cache" Done -- To view, visit http://gerrit.cloudera.org:8080/5812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
