Alexey Serbin has posted comments on this change. Change subject: Allow configuring TlsContext with key wrappers ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/5845/4/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: PS4, Line 95: err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT In what case we would get a self-signed certificate here which is not in the list of trusted certificates? I mean, does this error happen even if calling AddTrustedCertificate() for the self-signed cert? PS4, Line 101: cur_cert might it happen that it's nullptr? PS4, Line 112: ERR_clear_error(); consider removing this here and elsewhere: if using OPENSSL_RET_NOT_OK() wrapper macro while calling OpenSSL functions, the stack of errors should not contain any left-over errors. Line 122: paranoic nit: consider adding has_cert_.Store(false) before calling pair of SSL_CTX_use_{PrivateKey, certificate}() methods http://gerrit.cloudera.org:8080/#/c/5845/4/src/kudu/security/tls_context.h File src/kudu/security/tls_context.h: PS4, Line 67: oad the server certificate. Could you please document that the key and cert in those files should be PEM-encoded? http://gerrit.cloudera.org:8080/#/c/5845/4/src/kudu/security/tls_handshake-test.cc File src/kudu/security/tls_handshake-test.cc: PS4, Line 165: ASSERT_OK(client_tls_.LoadCertificateAndKey(cert_path_, key_path_)); : ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); Should this sequence be reversed? I.e., I would expect the cert authority to loaded first, so the loaded certificate would be checked to be trusted by the CA. -- To view, visit http://gerrit.cloudera.org:8080/5845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idb82427aea5f1bd29bad7529f2d54638b90811e2 Gerrit-PatchSet: 4 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: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
