From 168764479000909930a0c9764a43904b4e276fef Mon Sep 17 00:00:00 2001
From: Yang Guan <yangguan@google.com>
Date: Wed, 29 Mar 2017 14:24:32 -0700
Subject: [PATCH] Require peer certificate to be present when ca_cert_file is
 set (#648)

Fixes #615
---
 .../cluster_manager/cluster_ssl.rst           |  3 +-
 docs/configuration/listeners/ssl.rst          |  3 +-
 source/common/ssl/context_impl.cc             |  5 +++
 test/common/ssl/context_impl_test.cc          | 36 +++++++++++++++++++
 4 files changed, 45 insertions(+), 2 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 b0fbd53b0ae1..25657d74796b 100644
--- a/source/common/ssl/context_impl.cc
+++ b/source/common/ssl/context_impl.cc
@@ -229,6 +229,11 @@ 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<ClientContextImpl*>(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