Alexey Serbin has posted comments on this change. Change subject: tls: hook up internal PKI system to TlsContext ......................................................................
Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/5808/2/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: PS2, Line 128: ERR_clear_error(); I see this in different places as well, but why is this necessary? Could you add a comment about this at least some where? Ideally it would be nice to get rid of that, otherwise it looks like some errors are obliterated which could hide a problem. http://gerrit.cloudera.org:8080/#/c/5808/2/src/kudu/security/tls_handshake-test.cc File src/kudu/security/tls_handshake-test.cc: PS2, Line 130: TEST_F(TestTlsHandshake, Test_ClientNone_ServerSelfSigned) { : ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, key_path_)); : ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_)); : : // If the client wants to verify the server, it should fail because : // the server cert is self-signed. : Status s = RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, : TlsVerificationMode::VERIFY_NONE); : ASSERT_STR_MATCHES(s.ToString(), "client error:.*certificate verify failed"); : : // If the client doesn't care, it should succeed against the self-signed : // server. : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, : TlsVerificationMode::VERIFY_NONE)); : : // If the client loads the cert as trusted, then it should succeed : // with verification enabled. : ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, : TlsVerificationMode::VERIFY_NONE)); : : // It should still succeed with verification off, as well. : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, : TlsVerificationMode::VERIFY_NONE)); : : // If the server requires authentication of the client, the handshake should fail. : s = RunHandshake(TlsVerificationMode::VERIFY_NONE, : TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST); : ASSERT_TRUE(s.IsRuntimeError()); : ASSERT_STR_MATCHES(s.ToString(), "server error:.*peer did not return a certificate"); : } : : // TODO(PKI): this test has both the client and server using the same cert, : // which isn't very realistic. We should have this generate self-signed certs : // on the fly. : TEST_F(TestTlsHandshake, Test_ClientSelfSigned_ServerSelfSigned) { : ASSERT_OK(client_tls_.LoadCertificateAndKey(cert_path_, key_path_)); : ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); : ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, key_path_)); : ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_)); : : // This scenario should succeed in all cases. : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, : TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST)); : : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, : TlsVerificationMode::VERIFY_NONE)); : : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, : TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST)); : : ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, : TlsVerificationMode::VERIFY_NONE)); : } : : // TODO(PKI): add test coverage for mismatched common-name in the cert. It seems this has been moved into different changelist, so I anticipate this patch to change. I think it's better to continue when a new version is published for review. -- To view, visit http://gerrit.cloudera.org:8080/5808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a98ef522e51751562d47907aa90a32c2dac3918 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
