Dan Burkert has posted comments on this change. Change subject: security: generate certs on the tserver, sign them on the master ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master_cert_manager.cc File src/kudu/master/master_cert_manager.cc: PS2, Line 36: master_ca_rsa_key_length_bits I think 'master' in this config flag is redundant, really it's just CA vs not CA that makes this flag distinguished from the other key_length_bits flag. http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master_cert_manager.h File src/kudu/master/master_cert_manager.h: Line 46: explicit MasterCertManager(std::string uuid); Could you add a note about what the UUID signifies, or consider renaming to server_uuid if that's what it's for. http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/tablet_server.h File src/kudu/tserver/tablet_server.h: Line 108: std::unique_ptr<TSCertManager> cert_manager_; Looks like this doesn't necessarily need to be wrapped in unique_ptr, any reason to prefer it? http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/ts_cert_manager.h File src/kudu/tserver/ts_cert_manager.h: Line 47: class TSCertManager { I think we will want to use this class on the master as well to generate a cert for TLS, right? Or is the plan to just use the CA cert? Line 86: std::unique_ptr<security::ca::Cert> signed_cert_; Cert and Key are already effectively newtype'd unique_ptrs, so this is just adding an unecessary level of indirection. -- To view, visit http://gerrit.cloudera.org:8080/5766 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
