Alexey Serbin 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 initialized for self-signing. PS1, Line 525: return Status::OK(); Consider adding a sanity check: make sure the signer hasn't been initialized for anything else but self-signing. I think checking for ca_cart_ would be OK. Line 529: is_initialized_ = true; Consider adding a check for !ca_cert_. 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 I don't recall we were about to support cases when it would be necessary to read private keys and certificates from files. PS1, Line 668: issuer nit: consider renaming into 'issuer_cert' 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. I.e., add multiple constructors for different use-cases. Then there is no issues with checking on already initialized object on sub-sequent calls Init(). 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 ? -- 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-HasComments: Yes
