[kudu-CR] [util/crypto] certificate management (part 1)

["Dan Burkert (Code Review)" <ge...@cloudera.org>]

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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes