Alexey Serbin has posted comments on this change. Change subject: [TLS cert management] TLS cert generation unit tests ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5672/1/src/kudu/security/test/cert_management-test.cc File src/kudu/security/test/cert_management-test.cc: PS1, Line 115: .ca_cert_path = ca_cert_file_, : .ca_private_key_path = ca_private_key_file_, > in real usage, we'll be using a CA cert generated programmatically instead In case of our own CA, yes. In case of keys deployed by some external PKI -- read them from a file, I think. But in this context, at this point they should be already read into memory, so here should be strings/buffers with some data -- you are right. Will fix. PS1, Line 140: size_t > I think int is more appropriate here Why? The 'i' is of size_t type. Or you mean the type for 'i' should be int as well? Line 143: CHECK_OK(GenerateKey(key_bits, &key)); > curious: when we generate a new key, does that drain system entropy in such This test runs not so fast, so probably it does drain the entropy, but I do not know how to measure that. However, timewise it's not like reading many bytes from /dev/urandom and I haven't seen this test ever hung. PS1, Line 297: aExp > can you rename this to avoid abbreviating 'Expired'? Done PS1, Line 352: : // A test which verifies that non-initialized CertRequestGenerator and : // CertSigner behave as expected. > per comments on the code, I dont think it's worth coding for or testing the I thought it's better to avoid crash if it's possible consistently resolve this error for both the caller and the callee, at least in release mode. The idea is that we cannot guarantee we cover all code paths in debug mode with our tests. Crashing in production in cases when it's possible to consistently handle the error is not enticing to me. Do you think it's better to crash regardless? PS1, Line 382: static const struct TestKeyInfo { : const string fpath; : const char* data; : } kKeyInfo[] = { : { : ca_private_key_file_, : kCaPrivateKey_, : }, : { : ca_exp_private_key_file_, : kCaExpPrivateKey_, : }, : }; > hrm, I think this would be simpler to read in a more C++-style: I think having proper named field is more readable than first, second. But I like the idea of simple foreach loop. Will update. PS1, Line 614: transformtion > typo Done -- To view, visit http://gerrit.cloudera.org:8080/5672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
