Dan Burkert has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 6: (16 comments) 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. 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's a reason to use this one in particular, please doc it. 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 here is a bit hard to follow. In particular, are you sure that EVP_PKEY_assign_RSA doesn't free the RSA on failure? If so we have a double free here. Line 104: if (ret) { Maybe this should just be a CHECK_NOTNULL at the start? Seems like a bug to call this with nullptr. Line 146: if (ret) { same here with CHECK_NOTNULL Line 203: RETURN_NOT_OK(DoCertify(EVP_sha256(), req.expiration_sec(), 0, x509.get())); Do we check that the request specifies SHA256? PS6, Line 260: rc < 0 != 1, and remove the if below PS6, Line 261: Cert Should this be CSR instead of Cert? Line 268: pub_key.release(); Pretty sure this is a memory leak: "X509_set_pubkey() attempts to set the public key for certificate x to pkey. The key pkey should be freed up after use." PS6, Line 272: pkey Is this a public or private key? PS6, Line 315: utilty utility PS6, Line 326: EVP_PKEY_copy_parameters(pub_key.get(), ca_private_key_.get()); ??? Line 343: if (clrext != 0) { What is this doing? It seems we always call this with clrext == 0. 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, > warning: function 'kudu::crypto::CertRequestGenerator::GenerateRequest' has After calling this, the key must outlive the req, right? May want to doc that. PS6, Line 149: Certify I think this should be called 'Sign'. 'Certify' could be confused with validation, plus this is the main action of this CertSigner class. 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 -- 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
