Todd Lipcon has posted comments on this change. Change subject: [security] sign/verify data using RSA key pair ......................................................................
Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto-test.cc File src/kudu/security/crypto-test.cc: PS10, Line 231: // data. The idea is to verify the modified data yields different signatures. : T I feel this test is a little excessive -- we should already be able to rely on the fact that, if OpenSSL is telling us it's doing a signature, then it has the properties of doing a signature. You already verify that the signature generated by OpenSSL matches known good values in the test case above. So, here, I don't think we're really covering any new interesting code paths. (I think we can trust that OpenSSL hasn't "gamed the system" by hard-coding the results for our test cases ;-)" Line 265: TEST_F(CryptoTest, VerifySignatureRef) { maybe just combine this with the top test case that generates the signatures for each bit of reference data? PS10, Line 299: // Corrupt the reference data a bit. : const int num_changes = rand() % iter_num + 1; : for (int j = 0; j < num_changes; ++j) { : const size_t idx = rand() % data.size(); : // Using the fact that the reference data contains only ASCII symbols. : data[idx] |= 0x80; : } what about just using a constant string data = "not-the-original-data"? Or use the original data but a constant signature? Either one would shorten the test without any loss of coverage (since again, per above, we should be trusting the underlying library that a single bit changing is no different than all the bits changing) http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.cc File src/kudu/security/crypto.cc: Line 127: Status DoMakeSignature(DigestType digest, what's the purpose of these helper methods? they're only called from a single place each, unless I missed something. Line 141: auto buf = ssl_make_unique(OPENSSL_malloc(sig_len)); why use OPENSSL_malloc here instead of just a normal C++ allocation? (even on the stack uint8_t buf[sig_len] since we know that the signature lengths are 64 bytes max) PS10, Line 166: #if OPENSSL_VERSION_NUMBER < 0x10002000L : unsigned char* sig_data = reinterpret_cast<unsigned char*>( : const_cast<char*>(signature.data())); : #else : const unsigned char* sig_data = reinterpret_cast<const unsigned char*>( : signature.data()); : #endif we have this kind of messiness in a bunch of places. let's consider a separate patch later to add some kind of utility function like OPENSSL_CONST_BUFFER(const char* foo) which cleans them up? Line 174: if (rc < 0) { being extra paranoid here: EVP_DigestVerifyFinal explicitly says that it returns 1 for success, and any other value is failure to verify. Here, if it happened to return 2, we'd still act as if it were a success. I don't think it can actually return 2, but I think given the results would be silent signature verification skipping if it did, we should do a CHECK_LE(rc, 1); or adjust the if statement to 'if (rc < 0 || rc > 1)' http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.h File src/kudu/security/crypto.h: Line 36: SHA1, Do we need SHA1 at all? It's known to be relatively weak. eg the NIST guidelines say to use SHA-256 for digital signatures. (https://www.keylength.com/en/4/) Line 84: virtual Status MakeSignature(DigestType digest, can you add a note whether the signature is already base64-ed or whether the output is binary? -- To view, visit http://gerrit.cloudera.org:8080/5805 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f Gerrit-PatchSet: 10 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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
