Todd Lipcon has posted comments on this change. Change subject: [TLS certs management] initial commit ......................................................................
Patch Set 1: (32 comments) http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: PS1, Line 55: !(call) can you check (call) == nullptr instead? PS1, Line 80: FromBIO(bio.get(), format), : Substitute("$0: unable to load data key from file: $1", : fpath, GetErrors() it seems odd to be calling GetErrors() here instead of FromBIO itself being the one to call it. In other words, I expect to only see GetErrors directly after a failed direct call of the SSL API, and anywhere we call our own wrapper, the wrapper should be responsible for converting the errors to Status. Looking at the implementations, it seems like they do already fetch the errors so this is redundant. PS1, Line 96: U here and elsewhere, we've been trying to start status messages with lower case, so they look better when chained to each other. PS1, Line 102: if (PREDICT_FALSE(!data)) { : static const string kErrMsg = "Null placeholder for output data"; : LOG(DFATAL) << kErrMsg; : return Status::InvalidArgument(kErrMsg); : } this would just be a programmer error, no? i think CHECK(data) is fine, or even just let it crash PS1, Line 109: Error writing data to memory buffer: this is an odd looking error, because it's impossible to have an error writing to memory. The real error would be if for some reason the serialization failed, right? maybe just "error serializing" or something? Line 113: "BIO_get_mem_ptr() failed"); can we make this a CHECK as well? Line 123: if (PREDICT_FALSE(!bio)) { see above Line 149: if (PREDICT_FALSE(!elem_)) { same (just CHECK) Line 163: "Error exporing private key in DER format"); exporting PS1, Line 304: uint16_t num_bits, change this to an int, and do some basic bounds checking, like CHECK_GE(num_bits, 512); CHECK_LE(num_bits, 4096); or something PS1, Line 305: if (!ret) { : static const string kErrMsg = "Null placeholder for the result"; : LOG(DFATAL) << kErrMsg; : return Status::InvalidArgument(kErrMsg); : } : CHECK(ret) Line 315: "Error setting parameters for RSA key generation"); this should be able to be a CHECK too PS1, Line 342: if (!Initialized()) { : return Status::IllegalState("Not initialized"); : } CHECK Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR"); this should always succeed too, right? PS1, Line 555: RETURN_NOT_OK(CryptoEngine::get()->Init()); : RETURN_NOT_OK(ca_cert_.FromFile(config_.ca_cert_path, PEM)); : RETURN_NOT_OK(ca_private_key_.FromFile(config_.ca_private_key_path, PEM)); : CERT_RET_NOT_OK(X509_check_private_key(ca_cert_.get(), ca_private_key_.get()), : Substitute("$0, $1: CA certificate and private key " : "do not match", : config_.ca_cert_path, config_.ca_private_key_path)); this is somewhat temporary, right? in the real implementation we'll need to pass these in because they'll be stored in our tables, not in a file on disk, right? PS1, Line 606: STACK_OF(X509_EXTENSION)* exts = X509_REQ_get_extensions(req); : auto exts_cleanup = MakeScopedCleanup([&exts]() { : sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free); : }); : for (size_t i = 0; i < sk_X509_EXTENSION_num(exts); ++i) { : X509_EXTENSION* ext = sk_X509_EXTENSION_value(exts, i); : ASN1_OBJECT* obj = X509_EXTENSION_get_object(ext); : int32_t idx = X509_get_ext_by_OBJ(x, obj, -1); : if (idx != -1) { : // If extension exits, delete all extensions of same type. : do { : uni_ptr<X509_EXTENSION> tmpext(X509_get_ext(x, idx), : X509_EXTENSION_free); : X509_delete_ext(x, idx); : idx = X509_get_ext_by_OBJ(x, obj, -1); : } while (idx != -1); : } : CERT_RET_NOT_OK(X509_add_ext(x, ext, -1), "Error adding extension"); : } this stuff is somewhat black-boxy to me. Can you add a comment pointing to the reference code for this? I'm guessing it is modeled after the usage in the utilities included with OpenSSL. Perhaps a link to http://wiki.nikhef.nl/grid/How_to_handle_OpenSSL_and_not_get_hurt_and_what_does_that_library_call_really_do%3F for the free-related stuff? PS1, Line 635: if (!req->req_info || : !req->req_info->pubkey || : !req->req_info->pubkey->public_key || : !req->req_info->pubkey->public_key->data) { : return Status::RuntimeError("Corrupted CSR: no public key"); : } isn't this redundant with the get_pubkey call below? Line 680: CHECK_NOTNULL(ret); this will give a warning in gcc I think. instead you can use this inline below like X509_set_issuer_name(CHECK_NOTNULL(ret)) PS1, Line 686: v3 : CERT_RET_NOT_OK(X509_set_version(ret, 2 says v3 but you set to 2? Line 692: CERT_RET_IF_NULL(X509_gmtime_adj(X509_get_notAfter(ret), exp_seconds), http://openssl.6102.n7.nabble.com/overflow-when-calling-X509-gmtime-adj-on-32-bit-systems-td43468.html recommends using the following instead: X509_time_adj_ex(X509_get_notAfter(x), days, 0, NULL); to avoid potential overflow issues.. but maybe it's not an issue given we only support 64-bit? have you tried large values of 'seconds'? http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h File src/kudu/security/crypto/cert_management.h: PS1, Line 37: DER = 0, // DER/ASN1 format (binary) : PEM = 1, // PEM format (ASCII) Why do we care about supporting both formats? Is there a use case? If not it seems like we should just pick one and be able to eliminate a bit of code. As for which one, I think PEM is more typically used by users, and it would be nice to be able to print out the PEM format from debugging tools, etc. Of course it's slightly larger than DER but I don't think we transfer around keys frequently enough that it's a big issue, right? (I may be wrong and missing some reason why it's good to support both) Line 62: class Wrapper { having a little trouble understanding what the advantage of this is over just using its underlying unique_ptr? PS1, Line 65: elemFreeFunc_ nit: no camelCase in C++ style PS1, Line 87: public BasicWrapper, public Wrapper<EVP_PKEY> Multiple inheritance isn't allowed: https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance why not have the Wrapper<EVP_PKEY> as a member? Or just directly hold the unique_ptr<>? Or perhaps the 'BasicWrapper' should actually be something like a pure interface 'SSLSerializable', and the FromFile/FromString/ToString can be static methods? PS1, Line 120: uint16_t I think better to take 'int' and check input (https://google.github.io/styleguide/cppguide.html#Integer_Types) Perhaps better to call this GeneratePrivateKey or GenerateRandomPrivateKey? PS1, Line 144: method it's a function rather than a method (it's not part of any class) PS1, Line 144: The generated key will be needed later on : // when using the X509 certificate generated from the CSR. not sure this extra sentence adds much value -- I think it's sort of implicit in the knowledge of what a CSR does. Or, if you think it's useful to have some context in the code here, maybe a class-level or file-level doc comment explaining typical usage, like: // On the client: // ----------------- // Key key; // CHECK_OK(GenerateKey(2048, &key)); // CertSignRequest csr; // CHECK_OK(csr_gen->GenerateRequest(key, &csr)); // ... transfer the CSR to a CA ... (example code how to serialize the CSR) // // On the CA: // ------------- // // receive the CSR from the client // <appropriate code to deserialize and sign the CSR> // // send back the signed cert to the client // // On the client after signature received: // --------------- // <example what to do with the signed cert response> PS1, Line 174: // The configuration and hostnames/ips are made separate parameters to : // work around limitations of old gcc 4.4.x compiler. Ideally, both hostnames : // and ips should have been fields of the Config structure. confused about this - we don't support gcc <4.8 http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/crypto_common.cc File src/kudu/security/crypto/crypto_common.cc: PS1, Line 38: 256 curious why 256 - we have good evidence that no errors are longer than this? Line 44: serr << " "; a little funny that we end with a whitespace http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/crypto_common.h File src/kudu/security/crypto/crypto_common.h: PS1, Line 38: uni_ptr this was confusing me a bit in the other files -- it wasn't obvious what this was relative to a normal unique_ptr. Perhaps something like "c_unique_ptr' or 'unique_ptr_with_free' or something a bit more descriptive? or even ssl_unique_ptr PS1, Line 41: GetErrors please rename to something like GetSSLErrors() or SSLErrorString(). Even though it's in the crypto:: namespace, typical callers will also be in this namespace and it isn't super obvious where it's coming from otherwise. -- To view, visit http://gerrit.cloudera.org:8080/5671 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
