Alexey Serbin has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 7: (17 comments) http://gerrit.cloudera.org:8080/#/c/4799/7/src/kudu/security/crypto/cert_management-test.cc File src/kudu/security/crypto/cert_management-test.cc: Line 106: SCOPED_TRACE(Substitute("Thread $0", thread_idx)); > I don't think SCOPED_TRACE would actually show up, since we have to use CHE Done PS7, Line 154: encryped > nit: encrypted Done PS7, Line 314: day > nit: days Done PS7, Line 463: behaves predictable way > nit: behaves in a predictable way Done Line 466: CertSigner signer("/bin/sh", "/bin/cat"); > can you add a little comment here that says something like: Done Line 470: // Check that CertSigner behaves predictable way if given non-matching > *in a predictable Done Line 476: ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); > is the error message here something reasonable that you could assert on wit Done Line 481: ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); > same Done Line 503: // in a single-thread fashion. > single-threaded (same below "multi-threaded" in a few places) Done Line 545: // This should be the last test to avoid warning from LSAN to clean up > hrm, I don't think this is reliable. eg on dist-test we sometimes shard tes Thanks for the ideas! The approach with the TearDown() method is the first what I tried -- it does not work since the OpenSSL library cannot be properly 'teared down' and initialized again. Doing init/teardown multiple times does not work for OpenSSL because there are some statics which are left behind and do not reset on teardown, etc. I'll take a look at the LeakCheckDisabler. http://gerrit.cloudera.org:8080/#/c/4799/7/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: Line 85: const_cast<char*>("critical,serverAuth,clientAuth"))); > we may want to re-evaluate the permitted usages to be a bit narrower later, Yep, I removed keyAgreement and dataEncipherment since it's better to use something like ECDHE/ECDSA for key exchange; for ciphers, I think we want to use something like AES-128-GCM-SHA256. I also set the basic key constraint of the key: CA=false, since the generated certs should not be used as CA certs. The rest of that we need to keep: the generated certificates are needed to authenticate tservers both as a server and as a client. Line 112: "Error assigning assign RSA key"); > typo Done http://gerrit.cloudera.org:8080/#/c/4799/7/src/kudu/security/crypto/crypto_engine.cc File src/kudu/security/crypto/crypto_engine.cc: Line 47: vector<Mutex*> CryptoEngine::kLocks = {}; > hrm, is this one unused now with kCryptoLocks? This is used as internal member of the class, which is the actual container of the data pointed by kCryptoLocks. Line 52: static void LockingCbk(int mode, int type, > nit: we usually abbreviate Callback as just 'CB' or spell out Callback Done Line 58: m->lock(); > nit: indentation Done PS7, Line 110: // Do not load OpenSSL config file: nothing is needed from there. : OPENSSL_no_config(); > hrm, you sure about this? Is it possible people might configure something l Yes, probably -- I'll add a special parameter for that. This was written to avoid using default configuration file, from which we need nothing. Line 141: #error missing proper initialization/teardown for versions 1.1.0 and newer > does Xenial have 1.1? this might break some other devs Xenial uses OpenSSL 1.0.2 (1.0.2g precisely for 16.04 LTS). OpenSSL 1.1.x is a major revamp of the threading and initialization of the library, so I don't think major Linux distributions are going to pick it up any soon. If they do, we can just add appropriate code in here when needed. -- 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: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
