Dan Burkert has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 7: (8 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: PS7, Line 45: Substitute Use JoinPathSegments from path_util.h here and below. PS7, Line 84: generatation generation http://gerrit.cloudera.org:8080/#/c/4799/6/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: Line 100: return Status::IllegalState("Not initialized"); > Good observation. > Also, it makes sense to transfer ownership to the parent key: calling lesser > number of RSA_free() functions for the success path. Using set1 instead of assign would mean you could get rid of the rsa.release() call, right? Line 203: } > Nope, but that's implied -- but it's a good idea to add it for better prote Yah I think it would be prudent to check explicitly in order to ensure forwards compatibility. PS6, Line 326: ptr<ASN1_INTEGER> serial; > This is what it is: you can see it in apps/ca.c, line 2096. My confusion was that you copied into the pub_key scope local variable, and then never used that variable. I agree it's better to remove if it's not doing anything. http://gerrit.cloudera.org:8080/#/c/4799/7/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: PS7, Line 79: https://www.openssl.org/docs/manmaster/apps/x509v3_config.html This URL doesn't resolve. Perhaps https://www.openssl.org/docs/man1.0.1/apps/x509v3_config.html Line 85: const_cast<char*>("critical,serverAuth,clientAuth"))); > Yep, I removed keyAgreement and dataEncipherment since it's better to use s It looks like those weren't actually removed? There are now two calls to AddExtensions. Line 318: if (!ret) { I think this would be simpler as a CHECK_NOTNULL. Having a return statement following a LOG(DFATAL) is misleading. -- 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
