tls: SDS support for private key providers

source/common/secret/sds_api.h

@@ -191,7 +191,9 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider
             *sds_tls_certificate_secrets_);
     // We replace path based secrets with inlined secrets on update.
     resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_certificate_chain());
-    resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_private_key());
+    if (sds_tls_certificate_secrets_->has_private_key()) {
+      resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_private_key());
+    }
   }
   void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {}
   std::vector<std::string> getDataSourceFilenames() override;

source/common/ssl/tls_certificate_config_impl.cc

@@ -28,7 +28,7 @@ static const std::string INLINE_STRING = "<inline>";
 
 TlsCertificateConfigImpl::TlsCertificateConfigImpl(
     const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
-    Server::Configuration::TransportSocketFactoryContext* factory_context, Api::Api& api)
+    Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api)
     : certificate_chain_(Config::DataSource::read(config.certificate_chain(), true, api)),
       certificate_chain_path_(
           Config::DataSource::getPath(config.certificate_chain())
@@ -42,19 +42,30 @@ TlsCertificateConfigImpl::TlsCertificateConfigImpl(
       ocsp_staple_(readOcspStaple(config.ocsp_staple(), api)),
       ocsp_staple_path_(Config::DataSource::getPath(config.ocsp_staple())
                             .value_or(ocsp_staple_.empty() ? EMPTY_STRING : INLINE_STRING)),
-      private_key_method_(
-          factory_context != nullptr && config.has_private_key_provider()
-              ? factory_context->sslContextManager()
-                    .privateKeyMethodManager()
-                    .createPrivateKeyMethodProvider(config.private_key_provider(), *factory_context)
-              : nullptr) {
+      private_key_method_(nullptr) {
   if (config.has_private_key_provider() && config.has_private_key()) {
     throw EnvoyException(fmt::format(
         "Certificate configuration can't have both private_key and private_key_provider"));
   }
-  if (certificate_chain_.empty() || (private_key_.empty() && private_key_method_ == nullptr)) {
-    throw EnvoyException(fmt::format("Failed to load incomplete certificate from {}, {}",
-                                     certificate_chain_path_, private_key_path_));
+  if (config.has_private_key_provider()) {
+    private_key_method_ =
+        factory_context.sslContextManager()
+            .privateKeyMethodManager()
+            .createPrivateKeyMethodProvider(config.private_key_provider(), factory_context);
+  }
+  if (certificate_chain_.empty()) {
+    throw EnvoyException(
+        fmt::format("Failed to load incomplete certificate from {}: certificate chain not set",
+                    certificate_chain_path_));
+  }
+  if (private_key_.empty() && private_key_method_ == nullptr) {
+    if (config.has_private_key_provider()) {
+      throw EnvoyException(fmt::format("Failed to load private key provider: {}",
+                                       config.private_key_provider().provider_name()));
+    } else {
+      throw EnvoyException(
+          fmt::format("Failed to load incomplete private key from path: {}", private_key_path_));
+    }
   }
 }
 

source/common/ssl/tls_certificate_config_impl.h

@@ -14,7 +14,7 @@ class TlsCertificateConfigImpl : public TlsCertificateConfig {
 public:
   TlsCertificateConfigImpl(
       const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
-      Server::Configuration::TransportSocketFactoryContext* factory_context, Api::Api& api);
+      Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api);
 
   const std::string& certificateChain() const override { return certificate_chain_; }
   const std::string& certificateChainPath() const override { return certificate_chain_path_; }

source/common/upstream/cluster_factory_impl.cc

@@ -107,13 +107,16 @@ std::pair<ClusterSharedPtr, ThreadAwareLoadBalancerPtr>
 ClusterFactoryImplBase::create(const envoy::config::cluster::v3::Cluster& cluster,
                                ClusterFactoryContext& context) {
   auto stats_scope = generateStatsScope(cluster, context.stats());
-  Server::Configuration::TransportSocketFactoryContextImpl factory_context(
-      context.admin(), context.sslContextManager(), *stats_scope, context.clusterManager(),
-      context.localInfo(), context.dispatcher(), context.stats(), context.singletonManager(),
-      context.tls(), context.messageValidationVisitor(), context.api(), context.options());
+  std::unique_ptr<Server::Configuration::TransportSocketFactoryContextImpl>
+      transport_factory_context =
+          std::make_unique<Server::Configuration::TransportSocketFactoryContextImpl>(
+              context.admin(), context.sslContextManager(), *stats_scope, context.clusterManager(),
+              context.localInfo(), context.dispatcher(), context.stats(),
+              context.singletonManager(), context.tls(), context.messageValidationVisitor(),
+              context.api(), context.options());
 
   std::pair<ClusterImplBaseSharedPtr, ThreadAwareLoadBalancerPtr> new_cluster_pair =
-      createClusterImpl(cluster, context, factory_context, std::move(stats_scope));
+      createClusterImpl(cluster, context, *transport_factory_context, std::move(stats_scope));
 
   if (!cluster.health_checks().empty()) {
     // TODO(htuch): Need to support multiple health checks in v2.
@@ -130,6 +133,8 @@ ClusterFactoryImplBase::create(const envoy::config::cluster::v3::Cluster& cluste
   new_cluster_pair.first->setOutlierDetector(Outlier::DetectorImplFactory::createForCluster(
       *new_cluster_pair.first, cluster, context.dispatcher(), context.runtime(),
       context.outlierEventLogger()));
+
+  new_cluster_pair.first->setTransportFactoryContext(std::move(transport_factory_context));
   return new_cluster_pair;
 }
 

source/common/upstream/cluster_factory_impl.h

@@ -44,6 +44,8 @@
 #include "common/upstream/resource_manager_impl.h"
 #include "common/upstream/upstream_impl.h"
 
+#include "server/transport_socket_config_impl.h"
+
 namespace Envoy {
 namespace Upstream {
 

source/common/upstream/upstream_impl.cc

@@ -1148,6 +1148,12 @@ void ClusterImplBase::setOutlierDetector(const Outlier::DetectorSharedPtr& outli
       [this](const HostSharedPtr& host) -> void { reloadHealthyHosts(host); });
 }
 
+void ClusterImplBase::setTransportFactoryContext(
+    std::unique_ptr<Server::Configuration::TransportSocketFactoryContext>
+        transport_factory_context) {
+  transport_factory_context_ = std::move(transport_factory_context);
+}
+
 void ClusterImplBase::reloadHealthyHosts(const HostSharedPtr& host) {
   // Every time a host changes Health Check state we cause a full healthy host recalculation which
   // for expensive LBs (ring, subset, etc.) can be quite time consuming. During startup, this

source/common/upstream/upstream_impl.h

@@ -790,6 +790,14 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable<Logger::Id::u
    */
   void setOutlierDetector(const Outlier::DetectorSharedPtr& outlier_detector);
 
+  /**
+   * Set the TransportSocketFactoryContext pointer so that the memory isn't lost
+   * for the cluster lifetime in case SDS is used.
+   */
+  void
+  setTransportFactoryContext(std::unique_ptr<Server::Configuration::TransportSocketFactoryContext>
+                                 transport_factory_context);
+
   /**
    * Wrapper around Network::Address::resolveProtoAddress() that provides improved error message
    * based on the cluster's type.
@@ -880,6 +888,8 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable<Logger::Id::u
   const bool local_cluster_;
   Config::ConstMetadataSharedPoolSharedPtr const_metadata_shared_pool_;
   Common::CallbackHandlePtr priority_update_cb_;
+  std::unique_ptr<Server::Configuration::TransportSocketFactoryContext>
+      transport_factory_context_{};
 };
 
 using ClusterImplBaseSharedPtr = std::shared_ptr<ClusterImplBase>;

source/extensions/transport_sockets/tls/context_config_impl.cc

@@ -182,7 +182,8 @@ ContextConfigImpl::ContextConfigImpl(
       min_protocol_version_(tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(),
                                                 default_min_protocol_version)),
       max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(),
-                                                default_max_protocol_version)) {
+                                                default_max_protocol_version)),
+      factory_context_(factory_context) {
   if (certificate_validation_context_provider_ != nullptr) {
     if (default_cvc_) {
       // We need to validate combined certificate validation context.
@@ -213,7 +214,7 @@ ContextConfigImpl::ContextConfigImpl(
   if (!tls_certificate_providers_.empty()) {
     for (auto& provider : tls_certificate_providers_) {
       if (provider->secret() != nullptr) {
-        tls_certificate_configs_.emplace_back(*provider->secret(), &factory_context, api_);
+        tls_certificate_configs_.emplace_back(*provider->secret(), factory_context, api_);
       }
     }
   }
@@ -258,7 +259,7 @@ void ContextConfigImpl::setSecretUpdateCallback(std::function<void()> callback)
           for (const auto& tls_certificate_provider : tls_certificate_providers_) {
             auto* secret = tls_certificate_provider->secret();
             if (secret != nullptr) {
-              tls_certificate_configs_.emplace_back(*secret, nullptr, api_);
+              tls_certificate_configs_.emplace_back(*secret, factory_context_, api_);
             }
           }
           callback();

source/extensions/transport_sockets/tls/context_config_impl.h

@@ -99,6 +99,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {
   Ssl::HandshakerFactoryCb handshaker_factory_cb_;
   Ssl::HandshakerCapabilities capabilities_;
   Ssl::SslCtxCb sslctx_cb_;
+  Server::Configuration::TransportSocketFactoryContext& factory_context_;
 };
 
 class ClientContextConfigImpl : public ContextConfigImpl, public Envoy::Ssl::ClientContextConfig {

source/server/listener_impl.cc

@@ -289,15 +289,23 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config,
       open_connections_(std::make_shared<BasicResourceLimitImpl>(
           std::numeric_limits<uint64_t>::max(), listener_factory_context_->runtime(),
           cx_limit_runtime_key_)),
-      local_init_watcher_(fmt::format("Listener-local-init-watcher {}", name), [this] {
-        if (workers_started_) {
-          parent_.onListenerWarmed(*this);
-        } else {
-          // Notify Server that this listener is
-          // ready.
-          listener_init_target_.ready();
-        }
-      }) {
+      local_init_watcher_(fmt::format("Listener-local-init-watcher {}", name),
+                          [this] {
+                            if (workers_started_) {
+                              parent_.onListenerWarmed(*this);
+                            } else {
+                              // Notify Server that this listener is
+                              // ready.
+                              listener_init_target_.ready();
+                            }
+                          }),
+      transport_factory_context_(
+          std::make_shared<Server::Configuration::TransportSocketFactoryContextImpl>(
+              parent_.server_.admin(), parent_.server_.sslContextManager(), listenerScope(),
+              parent_.server_.clusterManager(), parent_.server_.localInfo(),
+              parent_.server_.dispatcher(), parent_.server_.stats(),
+              parent_.server_.singletonManager(), parent_.server_.threadLocal(),
+              validation_visitor_, parent_.server_.api(), parent_.server_.options())) {
 
   const absl::optional<std::string> runtime_val =
       listener_factory_context_->runtime().snapshot().get(cx_limit_runtime_key_);
@@ -360,10 +368,12 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin,
           origin.listener_factory_context_->listener_factory_context_base_, this, *this)),
       filter_chain_manager_(address_, origin.listener_factory_context_->parentFactoryContext(),
                             initManager(), origin.filter_chain_manager_),
-      local_init_watcher_(fmt::format("Listener-local-init-watcher {}", name), [this] {
-        ASSERT(workers_started_);
-        parent_.inPlaceFilterChainUpdate(*this);
-      }) {
+      local_init_watcher_(fmt::format("Listener-local-init-watcher {}", name),
+                          [this] {
+                            ASSERT(workers_started_);
+                            parent_.inPlaceFilterChainUpdate(*this);
+                          }),
+      transport_factory_context_(origin.transport_factory_context_) {
   buildAccessLog();
   auto socket_type = Network::Utility::protobufAddressSocketType(config.address());
   buildListenSocketOptions(socket_type);
@@ -505,13 +515,8 @@ void ListenerImpl::validateFilterChains(Network::Socket::Type socket_type) {
 }
 
 void ListenerImpl::buildFilterChains() {
-  Server::Configuration::TransportSocketFactoryContextImpl transport_factory_context(
-      parent_.server_.admin(), parent_.server_.sslContextManager(), listenerScope(),
-      parent_.server_.clusterManager(), parent_.server_.localInfo(), parent_.server_.dispatcher(),
-      parent_.server_.stats(), parent_.server_.singletonManager(), parent_.server_.threadLocal(),
-      validation_visitor_, parent_.server_.api(), parent_.server_.options());
-  transport_factory_context.setInitManager(*dynamic_init_manager_);
-  ListenerFilterChainFactoryBuilder builder(*this, transport_factory_context);
+  transport_factory_context_->setInitManager(*dynamic_init_manager_);
+  ListenerFilterChainFactoryBuilder builder(*this, *transport_factory_context_);
   filter_chain_manager_.addFilterChains(
       config_.filter_chains(),
       config_.has_default_filter_chain() ? &config_.default_filter_chain() : nullptr, builder,

source/server/listener_impl.h

@@ -19,6 +19,7 @@
 #include "common/init/target_impl.h"
 
 #include "server/filter_chain_manager_impl.h"
+#include "server/transport_socket_config_impl.h"
 
 #include "absl/base/call_once.h"
 
@@ -426,6 +427,9 @@ class ListenerImpl final : public Network::ListenerConfig,
 
   // to access ListenerManagerImpl::factory_.
   friend class ListenerFilterChainFactoryBuilder;
+
+  std::shared_ptr<Server::Configuration::TransportSocketFactoryContextImpl>
+      transport_factory_context_;
 };
 
 } // namespace Server

test/common/secret/BUILD

@@ -2,19 +2,26 @@ load(
     "//bazel:envoy_build_system.bzl",
     "envoy_cc_test",
     "envoy_package",
+    "envoy_proto_library",
 )
 
 licenses(["notice"])  # Apache 2
 
 envoy_package()
 
+envoy_proto_library(
+    name = "private_key_provider_proto",
+    srcs = ["private_key_provider.proto"],
+)
+
 envoy_cc_test(
     name = "secret_manager_impl_test",
     srcs = ["secret_manager_impl_test.cc"],
     data = [
         "//test/extensions/transport_sockets/tls/test_data:certs",
     ],
     deps = [
+        ":private_key_provider_proto_cc_proto",
         "//source/common/secret:sds_api_lib",
         "//source/common/secret:secret_manager_impl_lib",
         "//source/common/ssl:certificate_validation_context_config_impl_lib",
@@ -50,6 +57,7 @@ envoy_cc_test(
         "//test/mocks/init:init_mocks",
         "//test/mocks/protobuf:protobuf_mocks",
         "//test/mocks/secret:secret_mocks",
+        "//test/mocks/server:transport_socket_factory_context_mocks",
         "//test/test_common:environment_lib",
         "//test/test_common:logging_lib",
         "//test/test_common:registry_lib",

test/common/secret/private_key_provider.proto

@@ -0,0 +1,6 @@
+syntax = "proto3";
+
+package test.common.secret;
+
+message TestPrivateKeyMethodConfig {
+}

test/common/secret/sds_api_test.cc

@@ -20,6 +20,7 @@
 #include "test/mocks/init/mocks.h"
 #include "test/mocks/protobuf/mocks.h"
 #include "test/mocks/secret/mocks.h"
+#include "test/mocks/server/transport_socket_factory_context.h"
 #include "test/test_common/environment.h"
 #include "test/test_common/logging.h"
 #include "test/test_common/utility.h"
@@ -176,7 +177,8 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) {
   EXPECT_CALL(secret_callback, onAddOrUpdateSecret());
   subscription_factory_.callbacks_->onConfigUpdate(decoded_resources.refvec_, "");
 
-  Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), nullptr, *api_);
+  testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
+  Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), ctx, *api_);
   const std::string cert_pem =
       "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_cert.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)),
@@ -572,7 +574,8 @@ TEST_F(SdsApiTest, DeltaUpdateSuccess) {
   initialize();
   subscription_factory_.callbacks_->onConfigUpdate(decoded_resources.refvec_, {}, "");
 
-  Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), nullptr, *api_);
+  testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
+  Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), ctx, *api_);
   const std::string cert_pem =
       "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_cert.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)),