Todd Lipcon 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 CHECK inside the thread PS7, Line 154: encryped nit: encrypted PS7, Line 314: day nit: days PS7, Line 463: behaves predictable way nit: behaves in a predictable way Line 466: CertSigner signer("/bin/sh", "/bin/cat"); can you add a little comment here that says something like: // provide some files which are guaranteed to exist, but won't contain valid key data or something? At first I was confused why the parameters here were executables. Line 470: // Check that CertSigner behaves predictable way if given non-matching *in a predictable Line 476: ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); is the error message here something reasonable that you could assert on with ASSERT_STR_MATCHES or ASSERT_STR_CONTAINS? Line 481: ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); same Line 503: // in a single-thread fashion. single-threaded (same below "multi-threaded" in a few places) 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 tests, and it's not guaranteed that every execution runs every test. I think better to use LeakCheckDisabler or an lsan suppression. Or maybe a gtest Teardown method? 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, but I think this is fine for now Line 112: "Error assigning assign RSA key"); typo 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? Line 52: static void LockingCbk(int mode, int type, nit: we usually abbreviate Callback as just 'CB' or spell out Callback Line 58: m->lock(); nit: indentation 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 like a hardware PRNG and expect us to use it? 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 -- 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 <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
