From 88f47c70b40cde34823199291587bff6520353f4 Mon Sep 17 00:00:00 2001 From: Yang Guan Date: Wed, 29 Mar 2017 11:29:35 -0700 Subject: [PATCH 1/2] Require peer certificate to be present when ca_cert_file is set Fixes #615 --- source/common/ssl/context_impl.cc | 6 +++++ test/common/ssl/context_impl_test.cc | 36 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index b0fbd53b0ae1..bb16b0ad6ebf 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -229,6 +229,12 @@ bool ContextImpl::verifyPeer(SSL* ssl) const { if (!cert.get()) { stats_.no_certificate_.inc(); + + if (ca_cert_) { + // In case that ca_cert_ exists, reject the connection when peer + // certificate is not present. + verified = false; + } } if (!verify_subject_alt_name_list_.empty()) { diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 0b5aa77a901a..076a013b5f08 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -154,4 +154,40 @@ TEST(SslContextImplTest, TestNoCert) { EXPECT_EQ("", context->getCertChainInformation()); } +TEST(SslContextImplTest, TestVerifyPeer) { + std::string json = R"EOF( + { + "ca_cert_file": "test/common/ssl/test_data/ca.crt" + } + )EOF"; + + Json::ObjectPtr loader = TestEnvironment::jsonLoadFromString(json); + ContextConfigImpl cfg(*loader); + Runtime::MockLoader runtime; + ContextManagerImpl manager(runtime); + Stats::IsolatedStoreImpl store; + ClientContextPtr context(manager.createSslClientContext(store, cfg)); + ClientContextImpl* contextImpl = dynamic_cast(context.get()); + + SSL_CTX* ctx = SSL_CTX_new(SSLv23_server_method()); + SSL* ssl = SSL_new(ctx); + SSL_SESSION* session = SSL_SESSION_new(); + SSL_set_session(ssl, session); + + // Test that verifyPeer returns false when peer certificate is not set. + EXPECT_FALSE(contextImpl->verifyPeer(ssl)); + + // Set peer certificate and make sure verifyPeer returns true. + FILE* fp = fopen("test/common/ssl/test_data/san_dns.crt", "r"); + EXPECT_NE(fp, nullptr); + X509* cert = PEM_read_X509(fp, nullptr, nullptr, nullptr); + session->x509_peer = cert; + EXPECT_TRUE(contextImpl->verifyPeer(ssl)); + + fclose(fp); + SSL_SESSION_free(session); + SSL_free(ssl); + SSL_CTX_free(ctx); +} + } // Ssl From 646fd03c80ef5bb5e93ca5a4a338d1dba97f43dc Mon Sep 17 00:00:00 2001 From: Yang Guan Date: Wed, 29 Mar 2017 13:21:06 -0700 Subject: [PATCH 2/2] docs and comments --- docs/configuration/cluster_manager/cluster_ssl.rst | 3 ++- docs/configuration/listeners/ssl.rst | 3 ++- source/common/ssl/context_impl.cc | 3 +-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster_ssl.rst b/docs/configuration/cluster_manager/cluster_ssl.rst index e93ceffb22a3..3dd77530b416 100644 --- a/docs/configuration/cluster_manager/cluster_ssl.rst +++ b/docs/configuration/cluster_manager/cluster_ssl.rst @@ -34,7 +34,8 @@ private_key_file ca_cert_file *(optional, string)* A file containing certificate authority certificates to use in verifying - a presented server certificate. + a presented server certificate. If specified, a server must present a valid certificate or the + connection will be rejected. verify_certificate_hash *(optional, string)* If specified, Envoy will verify (pin) the hash of the presented server diff --git a/docs/configuration/listeners/ssl.rst b/docs/configuration/listeners/ssl.rst index 1aa4e1d129f0..86441a48b6b2 100644 --- a/docs/configuration/listeners/ssl.rst +++ b/docs/configuration/listeners/ssl.rst @@ -42,7 +42,8 @@ alt_alpn_protocols ca_cert_file *(optional, string)* A file containing certificate authority certificates to use in verifying a presented client side certificate. If not specified and a client certificate is presented it - will not be verified. + will not be verified. If specified, a client must present a valid certificate or the + connection will be rejected. verify_certificate_hash *(optional, string)* If specified, Envoy will verify (pin) the hash of the presented client diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index bb16b0ad6ebf..25657d74796b 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -231,8 +231,7 @@ bool ContextImpl::verifyPeer(SSL* ssl) const { stats_.no_certificate_.inc(); if (ca_cert_) { - // In case that ca_cert_ exists, reject the connection when peer - // certificate is not present. + // In case that ca_cert_ exists, reject the connection when peer certificate is not present. verified = false; } }