Todd Lipcon has posted comments on this change. Change subject: [security] sign/verify data using RSA key pair ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS2, Line 436: default: : CHECK(false) << "unsupported digest operation"; > ok, will remove. You think we event don't need DCHECK() here? nope, because then we'd have to have a 'default:', and then we don't get compilation warnings if we added a new enum case but forgot to update this function PS2, Line 440: const void* mdata = reinterpret_cast<const void*>(data.data()); : auto membio = ssl_make_unique(BIO_new_mem_buf( : #if OPENSSL_VERSION_NUMBER < 0x10002000L : const_cast<void*>(mdata), : #else : mdata, : #endif : data.size())); : BIO* inp = BIO_push(bmd.get(), membio.get()); : while (true) { : unsigned char buf[1024]; : const int rc = BIO_read(inp, buf, sizeof(buf)); : if (rc < 0) { : return Status::RuntimeError(Substitute("error reading data: $0", : GetOpenSSLErrors())); : } : if (rc == 0) { : break; : } : } > I don't think EVP_DigestSignUpdate() would work here in case of big data ch hrm, why not? also, our use case is only signing short token strings (on the order of hundreds of bytes max is my guess) so does it matter? http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: PS2, Line 194: template<typename KeyType> : Status MakeSignature(DigestType digest, const KeyType& key, > I thought we like stand-alone functions more. Don't we? :) I think we like standalone functions for utility code rather than getting utility code by implementation inheritance. But when there's a function that is closely tied to a single class instance, I think it belongs in that class. -- 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: 2 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
