Alexey Serbin has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 6: (16 comments) Thank you for the review. I'll post new version shortly. http://gerrit.cloudera.org:8080/#/c/4799/6/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: Line 78: const_cast<char*>("critical,digitalSignature,keyEncipherment"))); > Please briefly document what each of these does. Done Line 98: uni_ptr<RSA> rsa(RSA_generate_key(num_bits, RSA_F4, nullptr, nullptr), > This method is deprecated and replaced with RSA_generate_key_ex. If there' Done Line 100: CERT_RET_NOT_OK(EVP_PKEY_assign_RSA(key.get(), rsa.get()), > Could you doc the reason behind assign as opposed to set1? The ownership h Good observation. The main reason what that I saw that pattern in one of OpenSSL apps :) Also, it makes sense to transfer ownership to the parent key: calling lesser number of RSA_free() functions for the success path. BTW, EVP_PKEY_set1_RSA is just a wrapper around EVP_PKEY_assign_RSA: int EVP_PKEY_set1_RSA(EVP_PKEY *pkey, RSA *key) { int ret = EVP_PKEY_assign_RSA(pkey, key); if (ret) RSA_up_ref(key); return ret; } So, in case of a failure in both cases we need to make sure there is no double-free. I took a look at the code: in case of failure the RSA key needs to be freed (i.e. when EVP_PKEY_set_type() failed, the key is not set to the parent key): int EVP_PKEY_assign(EVP_PKEY *pkey, int type, void *key) { if (pkey == NULL || !EVP_PKEY_set_type(pkey, type)) return 0; pkey->pkey.ptr = key; return (key != NULL); } Line 104: if (ret) { > Maybe this should just be a CHECK_NOTNULL at the start? Seems like a bug t It's used with nullptr in tests, so CHECK_NOTNULL is not needed. Do you think having such a convention is too bizarre? Line 146: if (ret) { > same here with CHECK_NOTNULL same here -- it's used in tests with nullptr argument just to make sure it does not return an error, but if you think it's a bizarre convention, I can change it. Line 203: RETURN_NOT_OK(DoCertify(EVP_sha256(), req.expiration_sec(), 0, x509.get())); > Do we check that the request specifies SHA256? Nope, but that's implied -- but it's a good idea to add it for better protection against different versions of client/server in the field. But in general, do we want to check and bail with an error in case of mismatch or we want to support other digests here as well? PS6, Line 260: rc < 0 > != 1, and remove the if below There was a special provision to distinguish between different types of errors: mismatch and some other errors. Why do you think it's not worth it? PS6, Line 261: Cert > Should this be CSR instead of Cert? Done Line 268: pub_key.release(); > Pretty sure this is a memory leak: "X509_set_pubkey() attempts to set the p Good catch -- yes, this is a mistake. Somehow I confused it with and read docs for the X509_PUBKEY_set0_param() API call instead. PS6, Line 272: pkey > Is this a public or private key? Private -- the signer takes private key to make a signature. That pkey notion came from openssl sources, and yes, it's confusing. I'll update the name of the parameter for clarity. PS6, Line 315: utilty > utility Done PS6, Line 326: EVP_PKEY_copy_parameters(pub_key.get(), ca_private_key_.get()); > ??? This is what it is: you can see it in apps/ca.c, line 2096. It copies missing public key parameters (if any is missing) from CA key into the request public key, as from the documentation: "The main purpose of the functions EVP_PKEY_missing_parameters() and EVP_PKEY_copy_parameters() is to handle public keys in certificates where the parameters are sometimes omitted from a public key if they are inherited from the CA that signed it." I included this code as is, just adapted it. Probably, we can drop this part at all with no harm -- let's do that for sanity. Line 343: if (clrext != 0) { > What is this doing? It seems we always call this with clrext == 0. I kept it thinking that we might need those extensions. De facto they are not used -- that's correct. I'll remove that. http://gerrit.cloudera.org:8080/#/c/4799/6/src/kudu/security/crypto/cert_management.h File src/kudu/security/crypto/cert_management.h: Line 118: Status GenerateRequest(const CertSubjectInfo& info, const Key& key, > After calling this, the key must outlive the req, right? May want to doc t RIght -- the key is needed for future use is using the generated certificate. PS6, Line 149: Certify > I think this should be called 'Sign'. 'Certify' could be confused with val Done http://gerrit.cloudera.org:8080/#/c/4799/6/src/kudu/security/crypto/crypto_engine.h File src/kudu/security/crypto/crypto_engine.h: Line 28: typedef struct bio_st BIO; > I think this is already defined in crypto_common.h Done -- To view, visit http://gerrit.cloudera.org:8080/4799 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I69c1da97e6d013a034aefda59988b593ae1d6304 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@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
