Alexey Serbin has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 5: (33 comments) Thank you for the review. Will post the updated version in a moment. http://gerrit.cloudera.org:8080/#/c/4799/5/CMakeLists.txt File CMakeLists.txt: PS5, Line 796: OpenSSL library. At least, using libcrypto from that. > Just say "OpenSSL" to keep it consistent with the rest. We'll need libssl ok, done. As for adding crypto into the list of libraries to link against, check the update on CMakeLists.txt file in security sub-dir: ADD_EXPORTABLE_LIBRARY(kudu_security SRCS ${SECURITY_SRCS} DEPS kudu_util ${OPENSSL_CRYPTO_LIBRARY} NONLINK_DEPS gen_version_info EXPORTED_DEPS ${EXPORTED_UTIL_LIBS} ) 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 22: #include <vector> > warning: #includes are not sorted properly [llvm-include-order] Done Line 60: const string CertManagementTest::kCaCert_ = R"( > - would be nice to add a comment explaining what command you ran to generat * good idea -- will do * yep, lint build was also not so happy about this. done. Line 144: const string CertManagementTest::kCaPkey_ = R"( > - same Done Line 231: TEST_F(CertManagementTest, SignMultiThreadExclusive) { > a comment would be nice explaining why this test case is interesting. Isn't Done PS5, Line 244: this](size_t thread_id > instead of taking an argument, why not just capture i in the lambda? i.e th Capturing the 'i' in the lambda was a data race when capturing everything be a reference. Instead of having different capturing rules for different items, I thought it would be better to explicitly specify those parameters and pass only the required parts by parameters. OK, I'll roll it back and specify to capture 'i' by value. PS5, Line 247: ASSERT_OK > using ASSERT from a non-main thread can cause gtest to crash. Unfortunately OK, I see. I thought they had implemented that. CHECK() would also lead to a crash but if it's crashing more predictably, that's a better choice, of course. Line 263: TEST_F(CertManagementTest, SignMultiThreadShared) { > can you share code with the above test? eg extract a method Done Line 282: ASSERT_OK(gen.Init()); > I feel like a loop of some sort would probably be nice in these test thread Done PS5, Line 290: i, std::ref(gen), std::ref(signer)) > again I think doing this by lambda capture is clearer Done http://gerrit.cloudera.org:8080/#/c/4799/5/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: Line 47: using std::function; > warning: using decl 'function' is unused [misc-unused-using-decls] Done Line 51: using std::unique_ptr; > warning: using decl 'unique_ptr' is unused [misc-unused-using-decls] Done Line 296: CERT_RET_NOT_OK(BN_to_ASN1_INTEGER(btmp.get(), serial.get()), > warning: suspicious comparison of pointer with zero [misc-pointer-and-integ Done Line 304: Status CertSigner::DoCertify(const EVP_MD* digest, long exp_seconds, int clrext, > warning: consider replacing 'long' with 'int64' [google-runtime-int] Done Line 337: CERT_RET_NOT_OK(X509_gmtime_adj(X509_get_notBefore(ret), 0L), > warning: suspicious comparison of pointer with zero [misc-pointer-and-integ Done Line 339: CERT_RET_NOT_OK(X509_gmtime_adj(X509_get_notAfter(ret), exp_seconds), > warning: suspicious comparison of pointer with zero [misc-pointer-and-integ Done 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 fil good idea! will add that header. 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 i Done PS5, Line 46: R > we only seem to use this with 'void' Not quite so: BIO_free for some reason returns int. It seems I made a typo in one place with BIO_free, and in another it has correct signature. However, it gives a good idea -- if the compiler was happy to consume BIO_free() returning int here for uni_ptr<BIO, void> signature, may be it's worth making it void everywhere. I'll clarify on that. Line 87: explicit CertSignRequest(long expiration_sec = 24 * 60 * 60) > warning: consider replacing 'long' with 'int64' [google-runtime-int] Done Line 87: explicit CertSignRequest(long expiration_sec = 24 * 60 * 60) > worth documenting whether this is the expiration of the CSR, or whether the Done PS5, Line 92: long > we usually don't use 'long', but prefer a specific size (int64_t) Done Line 92: long expiration_sec() const { > warning: consider replacing 'long' with 'int64' [google-runtime-int] Done Line 100: const long expiration_sec_; > warning: consider replacing 'long' with 'int64' [google-runtime-int] Done Line 126: // Generate (private) RSA key. > we'd considered ECC instead of RSA at one point because it has different pe As per our recent HipChat discussion, we decided that since RSA algos are faster in the verification path than their ECC counterparts, we are going to use RSA for a while and then, if needed, add ECC algos as well. PS5, Line 127: uint16_t num_bits > prefer 'int' here so we can do appropriate assertions on it What kind of assertions are needed in this case? http://gerrit.cloudera.org:8080/#/c/4799/5/src/kudu/security/crypto/crypto_engine.cc File src/kudu/security/crypto/crypto_engine.cc: Line 22: #include <vector> > warning: #includes are not sorted properly [llvm-include-order] Done Line 42: static std::vector<std::mutex> kCryptoLocks(CRYPTO_num_locks()); > non-POD static data is dangerous ok, after we know how to work with squeasel, I can update this. Line 57: tid, static_cast<unsigned long>(hasher(std::this_thread::get_id()))); > warning: consider replacing 'unsigned long' with 'uint64' [google-runtime-i Done Line 57: tid, static_cast<unsigned long>(hasher(std::this_thread::get_id()))); > see discussion elsewhere: I dont think hashing guarantees uniqueness Done 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 So far I put it under #ifndef NDEBUG ... Will figure out how to redirect it into our log a bit later (could not find snap-and-go solution right away). Line 69: CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON); > any idea what the perf overhead is for this? should we only enable it for D Not much information about this, but probably it's better to keep it only for DEBUG builds. Will update. Line 94: CRYPTO_THREADID_set_callback(ThreadIdCbk); > see comment in Sailesh's patch -- we need to consolidate this code with his ok, I'll do that as soon as it's clear exactly what to do. -- 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: 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
