Alexey Serbin has posted comments on this change. Change subject: [TLS certs management] initial commit ......................................................................
Patch Set 1: (33 comments) Thank you for the review. I'll post a new version soon. 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? Done 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 Good catch! Indeed, that's redundant. Will update. PS1, Line 96: U > here and elsewhere, we've been trying to start status messages with lower c Ha, that's fun: some time ago I tried to do that (since it's the policy in Go and easier to type as well), but my Kudu reviewers told me otherwise. It's time do to switch to other convention. :) Will update. 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 The main premise was to avoid crash in production given the fact we cannot guarantee our tests give 100% path coverage. ok, it seems the consensus is that we prefer the brevity of the code to such defensive techniques. PS1, Line 109: Error writing data to memory buffer: > this is an odd looking error, because it's impossible to have an error writ Done Line 113: "BIO_get_mem_ptr() failed"); > can we make this a CHECK as well? Yes, but why is it better than just returning an error? Line 123: if (PREDICT_FALSE(!bio)) { > see above Done Line 149: if (PREDICT_FALSE(!elem_)) { > same (just CHECK) Done Line 163: "Error exporing private key in DER format"); > exporting Done PS1, Line 304: uint16_t num_bits, > change this to an int, and do some basic bounds checking, like CHECK_GE(num I'll replace the type with 'int'. The OpenSSL implementation is supposed to return an error for unsupported key length, so I think check for x < num_bits < y is redundant. There are a lot more non-supported key lengths even in interval of [512, 4096] -- let OpenSSL handle that since it knows best. PS1, Line 305: if (!ret) { : static const string kErrMsg = "Null placeholder for the result"; : LOG(DFATAL) << kErrMsg; : return Status::InvalidArgument(kErrMsg); : } : > CHECK(ret) Done Line 315: "Error setting parameters for RSA key generation"); > this should be able to be a CHECK too Yes, it's possible to replace it with CHECK, but I would prefer to keep it as is: BN_set_word() could return error if OPENSSL_malloc() could not allocate memory, which might be a transient error. Do you think it's better to crash in that case? PS1, Line 342: if (!Initialized()) { : return Status::IllegalState("Not initialized"); : } > CHECK Done Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR"); > this should always succeed too, right? That field might be null in case of transient memory allocation failure. Do you think it's better to crash in that case? 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 Sure -- that's the intermediate state before the functionality to load that data from the system tables is implemented. As for the external PKI mode, this code should not be used at all, 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 Right -- the most of the code is modeled after code in some utilities in $OPENSSL_ROOT/apps. It's from apps.ccopy_extensions() in apps.c. I'll add a comment. 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? X509_REQ_get_pubkey() implementation in 1.0.2 does not check for req->req_info->pubkey->public_key->data Line 680: CHECK_NOTNULL(ret); > this will give a warning in gcc I think. instead you can use this inline be Good catch! Yes, that's a typo. PS1, Line 686: v3 : CERT_RET_NOT_OK(X509_set_version(ret, 2 > says v3 but you set to 2? Yes, it is what it is -- this is not a typo. May be, they have a proper constant for that -- will take a look. https://www.openssl.org/docs/manmaster/man3/X509_set_version.html 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- The second parameter of X509_time_adj() is long, so it should not be a problem given the fact Kudu works only on 64-bit machines. I tried up to 20 years of certificate validity, but it was still under 2^31. I'm not sure anybody is about to generate internal Kudu certs which would be valid for 70 years or more :) http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h File src/kudu/security/crypto/cert_management.h: Line 36: enum DataFormat { > Consider making this an enum class. Done 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 i As you mentioned, PEM is good for print-outs and reading from a file, DER is good for passing data on the wire. So I thought it's worth supporting both -- one binary, one text-oriented. Also, I could not see all use-cases and supporting just a couple didn't seem to be a daunting task when I implemented this. BTW, I recall you also mentioned most of the Java packages you looked at recently do not support PEM format. Probably, we need to discuss this in more details before making a decision. Line 62: class Wrapper { > having a little trouble understanding what the advantage of this is over ju Using unique_ptr would require mentioning elem_free_func or calling get_deleter() every time while constructing or adopting/swapping an object this type. That's the only reason. PS1, Line 65: elemFreeFunc_ > nit: no camelCase in C++ style Done PS1, Line 87: public BasicWrapper, public Wrapper<EVP_PKEY> > Multiple inheritance isn't allowed: https://google.github.io/styleguide/cpp Aggregating Wrapper<> or unique_ptr<> would require proxying a couple of methods. Besides, Wrapper<> might be useful in other scenarios, that's why I separated its interface. However, in the end it turned out there is no need for separate Wrapper<> interface elsewhere. So, I'll aggregate the Wrapper just as unique_ptr<>, etc. Thanks for pointing at this. PS1, Line 120: uint16_t > I think better to take 'int' and check input (https://google.github.io/styl Done PS1, Line 144: method > it's a function rather than a method (it's not part of any class) Done 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 implic Done 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 I had some issues compiling the described alternative on ve0518.halxg.cloudera.com with the default gcc compiler (not sure we have 4.8 there). I'll remove this misleading comment. 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 I saw this in err_prn.c, ERR_print_errors_cb() function in OpenSSL code, so it should be enough, I think -- it does not include any file/path. But it would not hurt to put 1024 or 4096, if we want to be really paranoid. Line 44: serr << " "; > a little funny that we end with a whitespace fixed 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 th Yep, that's openssl_unique_ptr_with_custom_deleter_and_other_perks :) I didn't realize uni_ptr might be so confusing. I think it would be less confusing if you looked into this file first :) I'll rename it into c_unique_ptr. PS1, Line 41: GetErrors > please rename to something like GetSSLErrors() or SSLErrorString(). Even th Dan already did so in openssl_util.h -- I'll merge with that code. -- 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: 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
