Todd Lipcon has posted comments on this change. Change subject: ca: allow creating a self-signed CA ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5765/1/src/kudu/security/ca/cert_management.cc File src/kudu/security/ca/cert_management.cc: PS1, Line 510: return Status::OK(); > Consider adding a sanity check: make sure the signer object hasn't been ini discussed on slack. Decided to just change this to a CHECK(!is_initialized_) PS1, Line 525: return Status::OK(); > Consider adding a sanity check: make sure the signer hasn't been initialize same Line 529: is_initialized_ = true; > Consider adding a check for !ca_cert_. given the new CHECK it shouldn't be necessary PS1, Line 533: Status CertSigner::InitFromFiles(const string& ca_cert_path, : const string& ca_private_key_path) { : InitializeOpenSSL(); : : std::lock_guard<simple_spinlock> guard(lock_); : if (is_initialized_) { : return Status::OK(); : } : auto cert = std::make_shared<Cert>(); : auto key = std::make_shared<Key>(); : RETURN_NOT_OK(cert->FromFile(ca_cert_path, DataFormat::PEM)); : RETURN_NOT_OK(key->FromFile(ca_private_key_path, : DataFormat::PEM)); : CERT_RET_NOT_OK(X509_check_private_key(cert->GetRawData(), : key->GetRawData()), : Substitute("$0, $1: CA certificate and private key " : "do not match", : ca_cert_path, ca_private_key_path)); : ca_cert_ = std::move(cert); : ca_private_key_ = std::move(key); : is_initialized_ = true; : return Status::OK(); > Yep, eventually this should be gone. At least, from the recent discussions yea, agreed, but leaving for the moment. PS1, Line 668: issuer > nit: consider renaming into 'issuer_cert' Done http://gerrit.cloudera.org:8080/#/c/5765/1/src/kudu/security/ca/cert_management.h File src/kudu/security/ca/cert_management.h: PS1, Line 222: // Initialize the signer from existing Cert/Key objects. : // The passed objects must be initialized. : Status Init(std::shared_ptr<Cert> ca_cert, : std::shared_ptr<Key> ca_private_key); > May be, move these into constructor and make Init() taking no parameters. discussed for too long on Slack :) The summary of the discussion is that if we used constructors we'd lose the nice names, plus we'd have to "save" the strings for the variant that takes paths. Using factory methods would mean we'd need to set the signer as a unique_ptr outparam, etc. Basically none of the alternatives seemed substantially better than what we have now, especially if we change the existing Init() functions to enforce only being called once http://gerrit.cloudera.org:8080/#/c/5765/1/src/kudu/security/test/cert_management-test.cc File src/kudu/security/test/cert_management-test.cc: PS1, Line 384: signer.Sign(ts_csr, &ts_cert) > ASSERT_OK/EXPECT_OK ? woops, good catch -- To view, visit http://gerrit.cloudera.org:8080/5765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8bfa3cc014f1c671ced549a8297ba46065e1124 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
