ssl: split out client (config) context, small cleanups. (envoyproxy#1318

)

* To support the TLS context stuff in the v2 CDS update, we need to cleanly distinguish between client, server and common context.

* Constify various members of ContextConfigImpl etc.

include/envoy/ssl/context_config.h

@@ -62,7 +62,10 @@ class ContextConfig {
    * Otherwise, ""
    */
   virtual const std::string& verifyCertificateHash() const PURE;
+};
 
+class ClientContextConfig : public virtual ContextConfig {
+public:
   /**
    * @return The server name indication if it's set and ssl enabled
    * Otherwise, ""

include/envoy/ssl/context_manager.h

@@ -17,18 +17,19 @@ class ContextManager {
   virtual ~ContextManager() {}
 
   /**
-   * Builds an ClientContext from an ContextConfig
+   * Builds a ClientContext from a ClientContextConfig.
    */
-  virtual ClientContextPtr createSslClientContext(Stats::Scope& scope, ContextConfig& config) PURE;
+  virtual ClientContextPtr createSslClientContext(Stats::Scope& scope,
+                                                  ClientContextConfig& config) PURE;
 
   /**
-   * Builds an ServerContext from an ContextConfig
+   * Builds a ServerContext from a ServerContextConfig.
    */
   virtual ServerContextPtr createSslServerContext(Stats::Scope& scope,
                                                   ServerContextConfig& config) PURE;
 
   /**
-   * @return the number of days until the next certificate being managed will expire
+   * @return the number of days until the next certificate being managed will expire.
    */
   virtual size_t daysUntilFirstCertExpires() PURE;
 

source/common/ssl/context_config_impl.cc

@@ -27,22 +27,21 @@ const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES =
 
 const std::string ContextConfigImpl::DEFAULT_ECDH_CURVES = "X25519:P-256";
 
-ContextConfigImpl::ContextConfigImpl(const Json::Object& config) {
-  alpn_protocols_ = config.getString("alpn_protocols", "");
-  alt_alpn_protocols_ = config.getString("alt_alpn_protocols", "");
-  cipher_suites_ = config.getString("cipher_suites", DEFAULT_CIPHER_SUITES);
-  ecdh_curves_ = config.getString("ecdh_curves", DEFAULT_ECDH_CURVES);
-  ca_cert_file_ = config.getString("ca_cert_file", "");
-  if (config.hasObject("cert_chain_file")) {
-    cert_chain_file_ = config.getString("cert_chain_file");
-    private_key_file_ = config.getString("private_key_file");
-  }
-  if (config.hasObject("verify_subject_alt_name")) {
-    verify_subject_alt_name_list_ = config.getStringArray("verify_subject_alt_name");
-  }
-  verify_certificate_hash_ = config.getString("verify_certificate_hash", "");
-  server_name_indication_ = config.getString("sni", "");
-}
+ContextConfigImpl::ContextConfigImpl(const Json::Object& config)
+    : alpn_protocols_(config.getString("alpn_protocols", "")),
+      alt_alpn_protocols_(config.getString("alt_alpn_protocols", "")),
+      cipher_suites_(config.getString("cipher_suites", DEFAULT_CIPHER_SUITES)),
+      ecdh_curves_(config.getString("ecdh_curves", DEFAULT_ECDH_CURVES)),
+      ca_cert_file_(config.getString("ca_cert_file", "")),
+      cert_chain_file_(config.getString("cert_chain_file", "")),
+      private_key_file_(config.getString("private_key_file", "")),
+      verify_subject_alt_name_list_(config.hasObject("verify_subject_alt_name")
+                                        ? config.getStringArray("verify_subject_alt_name")
+                                        : std::vector<std::string>()),
+      verify_certificate_hash_(config.getString("verify_certificate_hash", "")) {}
+
+ClientContextConfigImpl::ClientContextConfigImpl(const Json::Object& config)
+    : ContextConfigImpl(config), server_name_indication_(config.getString("sni", "")) {}
 
 ServerContextConfigImpl::ServerContextConfigImpl(const Json::Object& config)
     : ContextConfigImpl(config),

source/common/ssl/context_config_impl.h

@@ -12,8 +12,6 @@ namespace Ssl {
 
 class ContextConfigImpl : public virtual Ssl::ContextConfig {
 public:
-  ContextConfigImpl(const Json::Object& config);
-
   // Ssl::ContextConfig
   const std::string& alpnProtocols() const override { return alpn_protocols_; }
   const std::string& altAlpnProtocols() const override { return alt_alpn_protocols_; }
@@ -26,22 +24,35 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {
     return verify_subject_alt_name_list_;
   };
   const std::string& verifyCertificateHash() const override { return verify_certificate_hash_; };
-  const std::string& serverNameIndication() const override { return server_name_indication_; }
+
+protected:
+  ContextConfigImpl(const Json::Object& config);
 
 private:
   static const std::string DEFAULT_CIPHER_SUITES;
   static const std::string DEFAULT_ECDH_CURVES;
 
-  std::string alpn_protocols_;
-  std::string alt_alpn_protocols_;
-  std::string cipher_suites_;
-  std::string ecdh_curves_;
-  std::string ca_cert_file_;
-  std::string cert_chain_file_;
-  std::string private_key_file_;
-  std::vector<std::string> verify_subject_alt_name_list_;
-  std::string verify_certificate_hash_;
-  std::string server_name_indication_;
+  const std::string alpn_protocols_;
+  const std::string alt_alpn_protocols_;
+  const std::string cipher_suites_;
+  const std::string ecdh_curves_;
+  const std::string ca_cert_file_;
+  const std::string cert_chain_file_;
+  const std::string private_key_file_;
+  const std::vector<std::string> verify_subject_alt_name_list_;
+  const std::string verify_certificate_hash_;
+  const std::string server_name_indication_;
+};
+
+class ClientContextConfigImpl : public ContextConfigImpl, public ClientContextConfig {
+public:
+  ClientContextConfigImpl(const Json::Object& config);
+
+  // Ssl::ClientContextConfig
+  const std::string& serverNameIndication() const override { return server_name_indication_; }
+
+private:
+  const std::string server_name_indication_;
 };
 
 class ServerContextConfigImpl : public ContextConfigImpl, public ServerContextConfig {

source/common/ssl/context_impl.cc

@@ -319,7 +319,7 @@ bssl::UniquePtr<X509> ContextImpl::loadCert(const std::string& cert_file) {
 };
 
 ClientContextImpl::ClientContextImpl(ContextManagerImpl& parent, Stats::Scope& scope,
-                                     ContextConfig& config)
+                                     ClientContextConfig& config)
     : ContextImpl(parent, scope, config) {
   if (!parsed_alpn_protocols_.empty()) {
     int rc = SSL_CTX_set_alpn_protos(ctx_.get(), &parsed_alpn_protocols_[0],

source/common/ssl/context_impl.h

@@ -122,7 +122,7 @@ class ContextImpl : public virtual Context {
 
 class ClientContextImpl : public ContextImpl, public ClientContext {
 public:
-  ClientContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, ContextConfig& config);
+  ClientContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, ClientContextConfig& config);
 
   bssl::UniquePtr<SSL> newSsl() const override;
 

source/common/ssl/context_manager_impl.cc

@@ -18,7 +18,7 @@ void ContextManagerImpl::releaseContext(Context* context) {
 }
 
 ClientContextPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope,
-                                                            ContextConfig& config) {
+                                                            ClientContextConfig& config) {
 
   ClientContextPtr context(new ClientContextImpl(*this, scope, config));
   std::unique_lock<std::mutex> lock(contexts_lock_);

source/common/ssl/context_manager_impl.h

@@ -30,7 +30,8 @@ class ContextManagerImpl final : public ContextManager {
   void releaseContext(Context* context);
 
   // Ssl::ContextManager
-  Ssl::ClientContextPtr createSslClientContext(Stats::Scope& scope, ContextConfig& config) override;
+  Ssl::ClientContextPtr createSslClientContext(Stats::Scope& scope,
+                                               ClientContextConfig& config) override;
   Ssl::ServerContextPtr createSslServerContext(Stats::Scope& scope,
                                                ServerContextConfig& config) override;
   size_t daysUntilFirstCertExpires() override;

source/common/upstream/upstream_impl.cc

@@ -71,7 +71,7 @@ ClusterInfoImpl::ClusterInfoImpl(const Json::Object& config, Runtime::Loader& ru
 
   ssl_ctx_ = nullptr;
   if (config.hasObject("ssl_context")) {
-    Ssl::ContextConfigImpl context_config(*config.getObject("ssl_context"));
+    Ssl::ClientContextConfigImpl context_config(*config.getObject("ssl_context"));
     ssl_ctx_ = ssl_context_manager.createSslClientContext(*stats_scope_, context_config);
   }
 

test/common/ssl/connection_impl_test.cc

@@ -56,7 +56,7 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_
                                    Network::ListenerOptions::listenerOptionsWithBindToPort());
 
   Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json);
-  ContextConfigImpl client_ctx_config(*client_ctx_loader);
+  ClientContextConfigImpl client_ctx_config(*client_ctx_loader);
   ClientContextPtr client_ctx(manager.createSslClientContext(stats_store, client_ctx_config));
   Network::ClientConnectionPtr client_connection =
       dispatcher.createSslClientConnection(*client_ctx, socket.localAddress());
@@ -338,7 +338,7 @@ TEST_P(SslConnectionImplTest, ClientAuthMultipleCAs) {
   )EOF";
 
   Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json);
-  ContextConfigImpl client_ctx_config(*client_ctx_loader);
+  ClientContextConfigImpl client_ctx_config(*client_ctx_loader);
   ClientContextPtr client_ctx(manager.createSslClientContext(stats_store, client_ctx_config));
   Network::ClientConnectionPtr client_connection =
       dispatcher.createSslClientConnection(*client_ctx, socket.localAddress());
@@ -446,7 +446,7 @@ class SslReadBufferLimitTest : public SslCertsTest,
          .per_connection_buffer_limit_bytes_ = read_buffer_limit});
 
     client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_);
-    client_ctx_config_.reset(new ContextConfigImpl(*client_ctx_loader_));
+    client_ctx_config_.reset(new ClientContextConfigImpl(*client_ctx_loader_));
     client_ctx_ = manager_->createSslClientContext(stats_store_, *client_ctx_config_);
 
     client_connection_ =
@@ -583,7 +583,7 @@ class SslReadBufferLimitTest : public SslCertsTest,
   ServerContextPtr server_ctx_;
   Network::ListenerPtr listener_;
   Json::ObjectSharedPtr client_ctx_loader_;
-  std::unique_ptr<ContextConfigImpl> client_ctx_config_;
+  std::unique_ptr<ClientContextConfigImpl> client_ctx_config_;
   ClientContextPtr client_ctx_;
   Network::ClientConnectionPtr client_connection_;
   Network::ConnectionPtr server_connection_;

test/common/ssl/context_impl_test.cc

@@ -76,7 +76,7 @@ TEST_F(SslContextImplTest, TestCipherSuites) {
   )EOF";
 
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json);
-  ContextConfigImpl cfg(*loader);
+  ClientContextConfigImpl cfg(*loader);
   Runtime::MockLoader runtime;
   ContextManagerImpl manager(runtime);
   Stats::IsolatedStoreImpl store;
@@ -92,7 +92,7 @@ TEST_F(SslContextImplTest, TestExpiringCert) {
   )EOF";
 
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json);
-  ContextConfigImpl cfg(*loader);
+  ClientContextConfigImpl cfg(*loader);
   Runtime::MockLoader runtime;
   ContextManagerImpl manager(runtime);
   Stats::IsolatedStoreImpl store;
@@ -115,7 +115,7 @@ TEST_F(SslContextImplTest, TestExpiredCert) {
   )EOF";
 
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json);
-  ContextConfigImpl cfg(*loader);
+  ClientContextConfigImpl cfg(*loader);
   Runtime::MockLoader runtime;
   ContextManagerImpl manager(runtime);
   Stats::IsolatedStoreImpl store;
@@ -133,7 +133,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) {
   )EOF";
 
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json);
-  ContextConfigImpl cfg(*loader);
+  ClientContextConfigImpl cfg(*loader);
   Runtime::MockLoader runtime;
   ContextManagerImpl manager(runtime);
   Stats::IsolatedStoreImpl store;
@@ -159,7 +159,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) {
 
 TEST_F(SslContextImplTest, TestNoCert) {
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString("{}");
-  ContextConfigImpl cfg(*loader);
+  ClientContextConfigImpl cfg(*loader);
   Runtime::MockLoader runtime;
   ContextManagerImpl manager(runtime);
   Stats::IsolatedStoreImpl store;

test/integration/ssl_utility.cc

@@ -56,7 +56,7 @@ ClientContextPtr createClientSslContext(bool alpn, bool san, ContextManager& con
     target = san ? json_san : json_plain;
   }
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target);
-  ContextConfigImpl cfg(*loader);
+  ClientContextConfigImpl cfg(*loader);
   static auto* client_stats_store = new Stats::TestIsolatedStoreImpl();
   return context_manager.createSslClientContext(*client_stats_store, cfg);
 }

test/integration/xfcc_integration_test.cc

@@ -68,7 +68,7 @@ Ssl::ClientContextPtr XfccIntegrationTest::createClientSslContext(bool mtls) {
     target = json_tls;
   }
   Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target);
-  Ssl::ContextConfigImpl cfg(*loader);
+  Ssl::ClientContextConfigImpl cfg(*loader);
   static auto* client_stats_store = new Stats::TestIsolatedStoreImpl();
   return context_manager_->createSslClientContext(*client_stats_store, cfg);
 }

test/mocks/ssl/mocks.h

@@ -19,7 +19,8 @@ class MockContextManager : public ContextManager {
   MockContextManager();
   ~MockContextManager();
 
-  ClientContextPtr createSslClientContext(Stats::Scope& scope, ContextConfig& config) override {
+  ClientContextPtr createSslClientContext(Stats::Scope& scope,
+                                          ClientContextConfig& config) override {
     return ClientContextPtr{createSslClientContext_(scope, config)};
   }
 
@@ -28,7 +29,8 @@ class MockContextManager : public ContextManager {
     return ServerContextPtr{createSslServerContext_(scope, config)};
   }
 
-  MOCK_METHOD2(createSslClientContext_, ClientContext*(Stats::Scope& scope, ContextConfig& config));
+  MOCK_METHOD2(createSslClientContext_,
+               ClientContext*(Stats::Scope& scope, ClientContextConfig& config));
   MOCK_METHOD2(createSslServerContext_,
                ServerContext*(Stats::Scope& stats, ServerContextConfig& config));
   MOCK_METHOD0(daysUntilFirstCertExpires, size_t());