Alexey Serbin has posted comments on this change. Change subject: [security] sign/verify data using RSA key pair ......................................................................
Patch Set 10: (10 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 removed Line 265: TEST_F(CryptoTest, VerifySignatureRef) { > maybe just combine this with the top test case that generates the signature Done 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 Done 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 sing I separated them for better readability. ok, if it does not look good, I'll inline them. Line 141: auto buf = ssl_make_unique(OPENSSL_malloc(sig_len)); > why use OPENSSL_malloc here instead of just a normal C++ allocation? (even It's a good question. I was thinking about replacing it with memory on the stack, but decided to keep OPENSSL_malloc (i.e. CRYPTO_malloc) because I thought there were some restrictions on how the memory for EVP_DigestSignXXX() should be allocated. However, after replacing it with stack-allocated memory everything was OK. Signature length depends on the message digest, key type, and number of bits in the key. The wiki page says it's 64 bytes for SHA2 digests, but openssl produces 256 bytes for SHA512 and RSA2048 bit key (64 bytes is for RSA512 and SHA224). For SHA512 and RSA4096 -- 512 bytes, SHA512 and RSA8192 -- 1024 bytes. I think we can safely put 4K for now -- the EVP_DigestSignFinal() should report an error if the buffer is too short. 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 separ yes, I would better do that in a separate patch. Line 174: if (rc < 0) { > being extra paranoid here: EVP_DigestVerifyFinal explicitly says that it re Done 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 guide SHA1 is faster than SHA2 digests -- we could use it for tests. However, if we are about to use it only for tests, may be we can drop it at all. Line 84: virtual Status MakeSignature(DigestType digest, > can you add a note whether the signature is already base64-ed or whether th Done http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS2, Line 436: : > btw http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-f sounds good -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
