Todd Lipcon has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 5: (20 comments) http://gerrit.cloudera.org:8080/#/c/4799/5/src/kudu/security/crypto/cert_management-test.cc File src/kudu/security/crypto/cert_management-test.cc: Line 60: const string CertManagementTest::kCaCert_ = R"( - would be nice to add a comment explaining what command you ran to generate this - I think a string (non-pod class) should not have static storage. a const char* should work fine here, right? Line 144: const string CertManagementTest::kCaPkey_ = R"( - same - also, I think kCaPrivateKey would be better since 'P' isn't unique between public and private, and it's really not that long to type Line 231: TEST_F(CertManagementTest, SignMultiThreadExclusive) { a comment would be nice explaining why this test case is interesting. Isn't the shared one the interesting one, and this one is somewhat "obviously" correct? PS5, Line 244: this](size_t thread_id instead of taking an argument, why not just capture i in the lambda? i.e the capture list can be [this, i] PS5, Line 247: ASSERT_OK using ASSERT from a non-main thread can cause gtest to crash. Unfortunately have to fall back and use CHECK Line 263: TEST_F(CertManagementTest, SignMultiThreadShared) { can you share code with the above test? eg extract a method Line 282: ASSERT_OK(gen.Init()); I feel like a loop of some sort would probably be nice in these test threads, otherwise if we assume that the contents runs pretty quickly, we probably aren't going to trigger many interesting races. PS5, Line 290: i, std::ref(gen), std::ref(signer)) again I think doing this by lambda capture is clearer http://gerrit.cloudera.org:8080/#/c/4799/5/src/kudu/security/crypto/cert_management.h File src/kudu/security/crypto/cert_management.h: PS5, Line 28: : // Forward declarations for the OpenSSL typedefs. : typedef struct asn1_string_st ASN1_INTEGER; : typedef struct bio_st BIO; : typedef struct env_md_st EVP_MD; : typedef struct evp_pkey_st EVP_PKEY; : typedef struct rsa_st RSA; : typedef struct x509_st X509; : typedef struct X509_req_st X509_REQ; : struct stack_st_X509_EXTENSION; // STACK_OF(X509_EXTENSION) if these are going to show up in lots of headers, maybe we should add a file like 'openssl_fwd.h' to stick them all in? Line 39: typedef std::map<std::string, std::string> SignOptions; move this down closer to where it's used? also needs some docs as to what it might contain PS5, Line 46: R we only seem to use this with 'void' Line 87: explicit CertSignRequest(long expiration_sec = 24 * 60 * 60) worth documenting whether this is the expiration of the CSR, or whether the actual post-signed cert would have an expiration determined by this PS5, Line 92: long we usually don't use 'long', but prefer a specific size (int64_t) Line 126: // Generate (private) RSA key. we'd considered ECC instead of RSA at one point because it has different perf tradeoffs. Should we leave this flexible? PS5, Line 127: uint16_t num_bits prefer 'int' here so we can do appropriate assertions on it http://gerrit.cloudera.org:8080/#/c/4799/5/src/kudu/security/crypto/crypto_engine.cc File src/kudu/security/crypto/crypto_engine.cc: Line 42: static std::vector<std::mutex> kCryptoLocks(CRYPTO_num_locks()); non-POD static data is dangerous Line 57: tid, static_cast<unsigned long>(hasher(std::this_thread::get_id()))); see discussion elsewhere: I dont think hashing guarantees uniqueness Line 65: : bio_err_(BIO_new_fp(stderr, BIO_NOCLOSE)), hrm, is this hooking up the ssl log to our stderr? is there a better way to get a logging callback so we can put it via glog? Line 69: CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON); any idea what the perf overhead is for this? should we only enable it for DEBUG builds or is it minimal? Line 94: CRYPTO_THREADID_set_callback(ThreadIdCbk); see comment in Sailesh's patch -- we need to consolidate this code with his code, and also need to make sure we aren't conflicting with the initialization code in squeasel (the web server library) -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
