tls: making tls_certificate_config_impl exception freee (envoyproxy#3…

…3262)

Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <[email protected]>

source/common/ssl/tls_certificate_config_impl.cc

@@ -12,13 +12,31 @@ namespace Envoy {
 namespace Ssl {
 
 namespace {
-std::vector<uint8_t> readOcspStaple(const envoy::config::core::v3::DataSource& source,
-                                    Api::Api& api) {
-  std::string staple =
-      THROW_OR_RETURN_VALUE(Config::DataSource::read(source, true, api), std::string);
+
+// Either return the supplied string, or set error and return an empty string
+// string.
+std::string maybeSet(absl::StatusOr<std::string> to_set, absl::Status error) {
+  if (to_set.status().ok()) {
+    return to_set.value();
+  }
+  error = to_set.status();
+  return "";
+}
+
+std::vector<uint8_t> maybeReadOcspStaple(const envoy::config::core::v3::DataSource& source,
+                                         Api::Api& api, absl::Status& creation_status) {
+  auto staple_or_error = Config::DataSource::read(source, true, api);
+  if (!staple_or_error.ok()) {
+    creation_status = staple_or_error.status();
+    return {};
+  }
+  const std::string& staple = staple_or_error.value();
+
   if (source.specifier_case() ==
       envoy::config::core::v3::DataSource::SpecifierCase::kInlineString) {
-    throwEnvoyExceptionOrPanic("OCSP staple cannot be provided via inline_string");
+    creation_status =
+        absl::InvalidArgumentError("OCSP staple cannot be provided via inline_string");
+    return {};
   }
 
   return {staple.begin(), staple.end()};
@@ -27,41 +45,50 @@ std::vector<uint8_t> readOcspStaple(const envoy::config::core::v3::DataSource& s
 
 static const std::string INLINE_STRING = "<inline>";
 
+absl::StatusOr<std::unique_ptr<TlsCertificateConfigImpl>> TlsCertificateConfigImpl::create(
+    const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
+    Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api) {
+  absl::Status creation_status = absl::OkStatus();
+  std::unique_ptr<TlsCertificateConfigImpl> ret(
+      new TlsCertificateConfigImpl(config, factory_context, api, creation_status));
+  RETURN_IF_NOT_OK(creation_status);
+  return ret;
+}
+
 TlsCertificateConfigImpl::TlsCertificateConfigImpl(
     const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
-    Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api)
-    : certificate_chain_(THROW_OR_RETURN_VALUE(
-          Config::DataSource::read(config.certificate_chain(), true, api), std::string)),
+    Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api,
+    absl::Status& creation_status)
+    : certificate_chain_(maybeSet(Config::DataSource::read(config.certificate_chain(), true, api),
+                                  creation_status)),
       certificate_chain_path_(
           Config::DataSource::getPath(config.certificate_chain())
               .value_or(certificate_chain_.empty() ? EMPTY_STRING : INLINE_STRING)),
-      private_key_(THROW_OR_RETURN_VALUE(Config::DataSource::read(config.private_key(), true, api),
-                                         std::string)),
+      private_key_(
+          maybeSet(Config::DataSource::read(config.private_key(), true, api), creation_status)),
       private_key_path_(Config::DataSource::getPath(config.private_key())
                             .value_or(private_key_.empty() ? EMPTY_STRING : INLINE_STRING)),
-      pkcs12_(
-          THROW_OR_RETURN_VALUE(Config::DataSource::read(config.pkcs12(), true, api), std::string)),
+      pkcs12_(maybeSet(Config::DataSource::read(config.pkcs12(), true, api), creation_status)),
       pkcs12_path_(Config::DataSource::getPath(config.pkcs12())
                        .value_or(pkcs12_.empty() ? EMPTY_STRING : INLINE_STRING)),
-      password_(THROW_OR_RETURN_VALUE(Config::DataSource::read(config.password(), true, api),
-                                      std::string)),
+      password_(maybeSet(Config::DataSource::read(config.password(), true, api), creation_status)),
       password_path_(Config::DataSource::getPath(config.password())
                          .value_or(password_.empty() ? EMPTY_STRING : INLINE_STRING)),
-      ocsp_staple_(readOcspStaple(config.ocsp_staple(), api)),
+      ocsp_staple_(maybeReadOcspStaple(config.ocsp_staple(), api, creation_status)),
       ocsp_staple_path_(Config::DataSource::getPath(config.ocsp_staple())
                             .value_or(ocsp_staple_.empty() ? EMPTY_STRING : INLINE_STRING)),
       private_key_method_(nullptr) {
   if (config.has_pkcs12()) {
     if (config.has_private_key()) {
-      throwEnvoyExceptionOrPanic(
+      creation_status = absl::InvalidArgumentError(
           fmt::format("Certificate configuration can't have both pkcs12 and private_key"));
     }
     if (config.has_certificate_chain()) {
-      throwEnvoyExceptionOrPanic(
+      creation_status = absl::InvalidArgumentError(
           fmt::format("Certificate configuration can't have both pkcs12 and certificate_chain"));
     }
     if (config.has_private_key_provider()) {
-      throwEnvoyExceptionOrPanic(
+      creation_status = absl::InvalidArgumentError(
           fmt::format("Certificate configuration can't have both pkcs12 and private_key_provider"));
     }
   } else {
@@ -72,22 +99,24 @@ TlsCertificateConfigImpl::TlsCertificateConfigImpl(
               .createPrivateKeyMethodProvider(config.private_key_provider(), factory_context);
       if (private_key_method_ == nullptr ||
           (!private_key_method_->isAvailable() && !config.private_key_provider().fallback())) {
-        throwEnvoyExceptionOrPanic(fmt::format("Failed to load private key provider: {}",
-                                               config.private_key_provider().provider_name()));
+        creation_status =
+            absl::InvalidArgumentError(fmt::format("Failed to load private key provider: {}",
+                                                   config.private_key_provider().provider_name()));
+        return;
       }
 
       if (!private_key_method_->isAvailable()) {
         private_key_method_ = nullptr;
       }
     }
     if (certificate_chain_.empty()) {
-      throwEnvoyExceptionOrPanic(
+      creation_status = absl::InvalidArgumentError(
           fmt::format("Failed to load incomplete certificate from {}: certificate chain not set",
                       certificate_chain_path_));
     }
 
     if (private_key_.empty() && private_key_method_ == nullptr) {
-      throwEnvoyExceptionOrPanic(
+      creation_status = absl::InvalidArgumentError(
           fmt::format("Failed to load incomplete private key from path: {}", private_key_path_));
     }
   }

source/common/ssl/tls_certificate_config_impl.h

@@ -12,9 +12,9 @@ namespace Ssl {
 
 class TlsCertificateConfigImpl : public TlsCertificateConfig {
 public:
-  TlsCertificateConfigImpl(
-      const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
-      Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api);
+  static absl::StatusOr<std::unique_ptr<TlsCertificateConfigImpl>>
+  create(const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
+         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_; }
@@ -31,6 +31,11 @@ class TlsCertificateConfigImpl : public TlsCertificateConfig {
   }
 
 private:
+  TlsCertificateConfigImpl(
+      const envoy::extensions::transport_sockets::tls::v3::TlsCertificate& config,
+      Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api,
+      absl::Status& creation_status);
+
   const std::string certificate_chain_;
   const std::string certificate_chain_path_;
   const std::string private_key_;

source/common/tls/context_config_impl.cc

@@ -228,7 +228,9 @@ 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(THROW_OR_RETURN_VALUE(
+            Ssl::TlsCertificateConfigImpl::create(*provider->secret(), factory_context, api_),
+            std::unique_ptr<Ssl::TlsCertificateConfigImpl>));
       }
     }
   }
@@ -279,7 +281,9 @@ 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, factory_context_, api_);
+              tls_certificate_configs_.emplace_back(THROW_OR_RETURN_VALUE(
+                  Ssl::TlsCertificateConfigImpl::create(*secret, factory_context_, api_),
+                  std::unique_ptr<Ssl::TlsCertificateConfigImpl>));
             }
           }
           callback();

source/common/tls/context_config_impl.h

@@ -32,7 +32,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {
   tlsCertificates() const override {
     std::vector<std::reference_wrapper<const Envoy::Ssl::TlsCertificateConfig>> configs;
     for (const auto& config : tls_certificate_configs_) {
-      configs.push_back(config);
+      configs.push_back(*config);
     }
     return configs;
   }
@@ -89,7 +89,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {
   const std::string ecdh_curves_;
   const std::string signature_algorithms_;
 
-  std::vector<Ssl::TlsCertificateConfigImpl> tls_certificate_configs_;
+  std::vector<std::unique_ptr<Ssl::TlsCertificateConfigImpl>> tls_certificate_configs_;
   Ssl::CertificateValidationContextConfigPtr validation_context_config_;
   // If certificate validation context type is combined_validation_context. default_cvc_
   // holds a copy of CombinedCertificateValidationContext::default_validation_context.

test/common/secret/sds_api_test.cc

@@ -179,14 +179,14 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) {
   EXPECT_TRUE(subscription_factory_.callbacks_->onConfigUpdate(decoded_resources.refvec_, "").ok());
 
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
-  Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), ctx, *api_);
+  auto tls_config = Ssl::TlsCertificateConfigImpl::create(*sds_api.secret(), ctx, *api_).value();
   const std::string cert_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_cert.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)),
-            tls_config.certificateChain());
+            tls_config->certificateChain());
 
   const std::string key_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_key.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)),
-            tls_config.privateKey());
+            tls_config->privateKey());
 }
 
 class SdsRotationApiTest : public SdsApiTestBase {
@@ -598,14 +598,14 @@ TEST_F(SdsApiTest, DeltaUpdateSuccess) {
       subscription_factory_.callbacks_->onConfigUpdate(decoded_resources.refvec_, {}, "").ok());
 
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
-  Ssl::TlsCertificateConfigImpl tls_config(*sds_api.secret(), ctx, *api_);
+  auto tls_config = Ssl::TlsCertificateConfigImpl::create(*sds_api.secret(), ctx, *api_).value();
   const std::string cert_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_cert.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)),
-            tls_config.certificateChain());
+            tls_config->certificateChain());
 
   const std::string key_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_key.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)),
-            tls_config.privateKey());
+            tls_config->privateKey());
 }
 
 // Validate that CertificateValidationContextSdsApi updates secrets successfully if

test/common/secret/secret_manager_impl_test.cc

@@ -83,15 +83,17 @@ name: "abc.com"
   ASSERT_NE(secret_manager->findStaticTlsCertificateProvider("abc.com"), nullptr);
 
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
-  Ssl::TlsCertificateConfigImpl tls_config(
-      *secret_manager->findStaticTlsCertificateProvider("abc.com")->secret(), ctx, *api_);
+  auto tls_config =
+      Ssl::TlsCertificateConfigImpl::create(
+          *secret_manager->findStaticTlsCertificateProvider("abc.com")->secret(), ctx, *api_)
+          .value();
   const std::string cert_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_cert.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)),
-            tls_config.certificateChain());
+            tls_config->certificateChain());
 
   const std::string key_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_key.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)),
-            tls_config.privateKey());
+            tls_config->privateKey());
 }
 
 // Validate that secret manager throws an exception when adding duplicated static TLS certificate
@@ -382,13 +384,14 @@ name: "abc.com"
                   ->onConfigUpdate(decoded_resources.refvec_, "")
                   .ok());
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
-  Ssl::TlsCertificateConfigImpl tls_config(*secret_provider->secret(), ctx, *api_);
+  auto tls_config =
+      Ssl::TlsCertificateConfigImpl::create(*secret_provider->secret(), ctx, *api_).value();
   const std::string cert_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_cert.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)),
-            tls_config.certificateChain());
+            tls_config->certificateChain());
   const std::string key_pem = "{{ test_rundir }}/test/common/tls/test_data/selfsigned_key.pem";
   EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)),
-            tls_config.privateKey());
+            tls_config->privateKey());
 }
 
 TEST_F(SecretManagerImplTest, SdsDynamicGenericSecret) {
@@ -480,10 +483,11 @@ name: "abc.com"
                   ->onConfigUpdate(decoded_resources.refvec_, "keycert-v1")
                   .ok());
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> ctx;
-  Ssl::TlsCertificateConfigImpl tls_config(*secret_provider->secret(), ctx, *api_);
-  EXPECT_EQ("DUMMY_INLINE_BYTES_FOR_CERT_CHAIN", tls_config.certificateChain());
-  EXPECT_EQ("DUMMY_INLINE_BYTES_FOR_PRIVATE_KEY", tls_config.privateKey());
-  EXPECT_EQ("DUMMY_PASSWORD", tls_config.password());
+  auto tls_config =
+      Ssl::TlsCertificateConfigImpl::create(*secret_provider->secret(), ctx, *api_).value();
+  EXPECT_EQ("DUMMY_INLINE_BYTES_FOR_CERT_CHAIN", tls_config->certificateChain());
+  EXPECT_EQ("DUMMY_INLINE_BYTES_FOR_PRIVATE_KEY", tls_config->privateKey());
+  EXPECT_EQ("DUMMY_PASSWORD", tls_config->password());
 
   // Private key and password are removed.
   const std::string expected_secrets_config_dump = R"EOF(
@@ -1119,9 +1123,10 @@ name: "abc.com"
   EXPECT_CALL(ssl_context_manager, privateKeyMethodManager())
       .WillRepeatedly(ReturnRef(private_key_method_manager));
   EXPECT_CALL(ctx, sslContextManager()).WillRepeatedly(ReturnRef(ssl_context_manager));
-  EXPECT_THROW_WITH_MESSAGE(
-      Ssl::TlsCertificateConfigImpl tls_config(*secret_provider->secret(), ctx, *api_),
-      EnvoyException, "Failed to load private key provider: test");
+  EXPECT_EQ(Ssl::TlsCertificateConfigImpl::create(*secret_provider->secret(), ctx, *api_)
+                .status()
+                .message(),
+            "Failed to load private key provider: test");
 }
 
 // Verify that using the match_subject_alt_names will result in a typed matcher, one for each of

tools/code_format/config.yaml

@@ -109,7 +109,6 @@ paths:
     - source/common/network/address_impl.cc
     - source/common/network/utility.cc
     - source/common/network/dns_resolver/dns_factory_util.cc
-    - source/common/ssl/tls_certificate_config_impl.cc
     - source/common/formatter/http_specific_formatter.cc
     - source/common/formatter/stream_info_formatter.h
     - source/common/formatter/stream_info_formatter.cc