Todd Lipcon has posted comments on this change. Change subject: security: initial work on token creation and verification ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token.proto File src/kudu/security/token.proto: PS2, Line 71: TokenSigningKeyPB > Is it just for verification or for signing? If for both, then may be it ma Let me ponder this... I think you're right that right now we only need to pass and persist around the public variant through RPCs and such. PS2, Line 75: optional bytes public_key_der = 2; : optional bytes private_key_der = 3; > If it's necessary to have public key only, why do we need the private part? yea, like you said above probably mutually exclusive. 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()); > This check might present some security complications: if the private key is In general aren't we sort of assuming that core files are going to have sensitive data? eg we have tons of user data in memory, not just these various keys, etc. We could try to get fancy at some point with MADV_DONTDUMP but it's not supported on most of our target operating systems and it's also terribly hard to get this to work all the way down through OpenSSL, etc. Given that, I'd prefer to crash if we are accidentally sending private keys where we meant to be sending public ones, such that we hear about it quickly due to people complaining about crashes (rather than hearing about it from someone finding an exploit) On a side note, I found this an interesting read: https://www.agwa.name/blog/post/protecting_the_openssl_private_key_in_a_separate_process 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 downside is then you end up with the longer name TokenVerifier::VerificationResult::VALID instead of just TokenVerifier::VALID, which is a bit of a mouthful. Suppose I could move it up out of TokenVerifier though, which wouldn't be bad. Will give it a shot. -- 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
