tls: fix RELEASE_ASSERT when using auto_sni (#33637)

* tls: fix RELEASE_ASSERT when using `auto_sni` If the `:authority` was longer than 255 characters, Envoy would RELEASE_ASSERT when creating an upstream TLS connection when `auto_sni` (https://www.envoyproxy.io/docs/envoy/v1.30.0/api-v3/config/core/v3/protocol.proto.html#config-core-v3-upstreamhttpprotocoloptions) was used. Signed-off-by: Greg Greenway <[email protected]> Signed-off-by: Ryan Northey <[email protected]>
envoyproxy · Apr 18, 2024 · 81fb519 · 81fb519
1 parent 577a0ca
commit 81fb519
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 22 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -8,6 +8,11 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: tls
+  change: |
+    Fix a RELEASE_ASSERT when using :ref:`auto_sni <envoy_v3_api_field_config.core.v3.UpstreamHttpProtocolOptions.auto_sni>`
+    if the downstream request ``:authority`` was longer than 255 characters.
+
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc
@@ -452,7 +452,7 @@ std::vector<uint8_t> ContextImpl::parseAlpnProtocols(const std::string& alpn_pro
   return out;
 }
 
-bssl::UniquePtr<SSL>
+absl::StatusOr<bssl::UniquePtr<SSL>>
 ContextImpl::newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) {
   // We use the first certificate for a new SSL object, later in the
   // SSL_CTX_set_select_certificate_cb() callback following ClientHello, we replace with the
@@ -680,16 +680,25 @@ bool ContextImpl::parseAndSetAlpn(const std::vector<std::string>& alpn, SSL& ssl
   return false;
 }
 
-bssl::UniquePtr<SSL>
+absl::StatusOr<bssl::UniquePtr<SSL>>
 ClientContextImpl::newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) {
-  bssl::UniquePtr<SSL> ssl_con(ContextImpl::newSsl(options));
+  absl::StatusOr<bssl::UniquePtr<SSL>> ssl_con_or_status(ContextImpl::newSsl(options));
+  if (!ssl_con_or_status.ok()) {
+    return ssl_con_or_status;
+  }
+
+  bssl::UniquePtr<SSL> ssl_con = std::move(ssl_con_or_status.value());
 
   const std::string server_name_indication = options && options->serverNameOverride().has_value()
                                                  ? options->serverNameOverride().value()
                                                  : server_name_indication_;
   if (!server_name_indication.empty()) {
     const int rc = SSL_set_tlsext_host_name(ssl_con.get(), server_name_indication.c_str());
-    RELEASE_ASSERT(rc, Utility::getLastCryptoError().value_or(""));
+    if (rc != 1) {
+      return absl::InvalidArgumentError(
+          absl::StrCat("Failed to create upstream TLS due to failure setting SNI: ",
+                       Utility::getLastCryptoError().value_or("unknown")));
+    }
   }
 
   if (options && !options->verifySubjectAltNameListOverride().empty()) {

diff --git a/source/extensions/transport_sockets/tls/context_impl.h b/source/extensions/transport_sockets/tls/context_impl.h
@@ -66,7 +66,8 @@ struct TlsContext {
 class ContextImpl : public virtual Envoy::Ssl::Context,
                     protected Logger::Loggable<Logger::Id::config> {
 public:
-  virtual bssl::UniquePtr<SSL> newSsl(const Network::TransportSocketOptionsConstSharedPtr& options);
+  virtual absl::StatusOr<bssl::UniquePtr<SSL>>
+  newSsl(const Network::TransportSocketOptionsConstSharedPtr& options);
 
   /**
    * Logs successful TLS handshake and updates stats.
@@ -163,7 +164,7 @@ class ClientContextImpl : public ContextImpl, public Envoy::Ssl::ClientContext {
   ClientContextImpl(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config,
                     TimeSource& time_source);
 
-  bssl::UniquePtr<SSL>
+  absl::StatusOr<bssl::UniquePtr<SSL>>
   newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) override;
 
 private:

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc
@@ -26,13 +26,11 @@ namespace {
 
 constexpr absl::string_view NotReadyReason{"TLS error: Secret is not supplied by SDS"};
 
-// This SslSocket will be used when SSL secret is not fetched from SDS server.
-class NotReadySslSocket : public Network::TransportSocket {
+class InvalidSslSocket : public Network::TransportSocket {
 public:
   // Network::TransportSocket
   void setTransportSocketCallbacks(Network::TransportSocketCallbacks&) override {}
   std::string protocol() const override { return EMPTY_STRING; }
-  absl::string_view failureReason() const override { return NotReadyReason; }
   bool canFlushClose() override { return true; }
   void closeSocket(Network::ConnectionEvent) override {}
   Network::IoResult doRead(Buffer::Instance&) override { return {PostIoAction::Close, 0, false}; }
@@ -45,21 +43,62 @@ class NotReadySslSocket : public Network::TransportSocket {
   void configureInitialCongestionWindow(uint64_t, std::chrono::microseconds) override {}
 };
 
+// This SslSocket will be used when SSL secret is not fetched from SDS server.
+class NotReadySslSocket : public InvalidSslSocket {
+public:
+  // Network::TransportSocket
+  absl::string_view failureReason() const override { return NotReadyReason; }
+};
+
+class ErrorSslSocket : public InvalidSslSocket {
+public:
+  ErrorSslSocket(absl::string_view error) : error_(error) {}
+
+  // Network::TransportSocket
+  absl::string_view failureReason() const override { return error_; }
+
+private:
+  std::string error_;
+};
+
 } // namespace
 
-SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
-                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
-                     Ssl::HandshakerFactoryCb handshaker_factory_cb)
+absl::StatusOr<std::unique_ptr<SslSocket>>
+SslSocket::create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
+                  const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
+                  Ssl::HandshakerFactoryCb handshaker_factory_cb) {
+  std::unique_ptr<SslSocket> socket(new SslSocket(ctx, transport_socket_options));
+  auto status = socket->initialize(state, handshaker_factory_cb);
+  if (status.ok()) {
+    return socket;
+  } else {
+    return status;
+  }
+}
+
+SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx,
+                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options)
     : transport_socket_options_(transport_socket_options),
-      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)),
-      info_(std::dynamic_pointer_cast<SslHandshakerImpl>(handshaker_factory_cb(
-          ctx_->newSsl(transport_socket_options_), ctx_->sslExtendedSocketInfoIndex(), this))) {
+      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)) {}
+
+absl::Status SslSocket::initialize(InitialState state,
+                                   Ssl::HandshakerFactoryCb handshaker_factory_cb) {
+  auto status_or_ssl = ctx_->newSsl(transport_socket_options_);
+  if (!status_or_ssl.ok()) {
+    return status_or_ssl.status();
+  }
+
+  info_ = std::dynamic_pointer_cast<SslHandshakerImpl>(handshaker_factory_cb(
+      std::move(status_or_ssl.value()), ctx_->sslExtendedSocketInfoIndex(), this));
+
   if (state == InitialState::Client) {
     SSL_set_connect_state(rawSsl());
   } else {
     ASSERT(state == InitialState::Server);
     SSL_set_accept_state(rawSsl());
   }
+
+  return absl::OkStatus();
 }
 
 void SslSocket::setTransportSocketCallbacks(Network::TransportSocketCallbacks& callbacks) {
@@ -394,8 +433,13 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket(
     ssl_ctx = ssl_ctx_;
   }
   if (ssl_ctx) {
-    return std::make_unique<SslSocket>(std::move(ssl_ctx), InitialState::Client,
-                                       transport_socket_options, config_->createHandshaker());
+    auto status_or_socket =
+        SslSocket::create(std::move(ssl_ctx), InitialState::Client, transport_socket_options,
+                          config_->createHandshaker());
+    if (status_or_socket.ok()) {
+      return std::move(status_or_socket.value());
+    }
+    return std::make_unique<ErrorSslSocket>(status_or_socket.status().message());
   } else {
     ENVOY_LOG(debug, "Create NotReadySslSocket");
     stats_.upstream_context_secrets_not_ready_.inc();
@@ -443,8 +487,12 @@ Network::TransportSocketPtr ServerSslSocketFactory::createDownstreamTransportSoc
     ssl_ctx = ssl_ctx_;
   }
   if (ssl_ctx) {
-    return std::make_unique<SslSocket>(std::move(ssl_ctx), InitialState::Server, nullptr,
-                                       config_->createHandshaker());
+    auto status_or_socket = SslSocket::create(std::move(ssl_ctx), InitialState::Server, nullptr,
+                                              config_->createHandshaker());
+    if (status_or_socket.ok()) {
+      return std::move(status_or_socket.value());
+    }
+    return std::make_unique<ErrorSslSocket>(status_or_socket.status().message());
   } else {
     ENVOY_LOG(debug, "Create NotReadySslSocket");
     stats_.downstream_context_secrets_not_ready_.inc();

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h
@@ -48,9 +48,10 @@ class SslSocket : public Network::TransportSocket,
                   public Ssl::HandshakeCallbacks,
                   protected Logger::Loggable<Logger::Id::connection> {
 public:
-  SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
-            const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
-            Ssl::HandshakerFactoryCb handshaker_factory_cb);
+  static absl::StatusOr<std::unique_ptr<SslSocket>>
+  create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
+         const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
+         Ssl::HandshakerFactoryCb handshaker_factory_cb);
 
   // Network::TransportSocket
   void setTransportSocketCallbacks(Network::TransportSocketCallbacks& callbacks) override;
@@ -79,6 +80,10 @@ class SslSocket : public Network::TransportSocket,
   SSL* rawSsl() const { return info_->ssl(); }
 
 private:
+  SslSocket(Envoy::Ssl::ContextSharedPtr ctx,
+            const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options);
+  absl::Status initialize(InitialState state, Ssl::HandshakerFactoryCb handshaker_factory_cb);
+
   struct ReadResult {
     uint64_t bytes_read_{0};
     absl::optional<int> error_;

diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
@@ -580,6 +580,24 @@ TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithIpHost) {
   checkSimpleRequestSuccess(0, 0, response.get());
 }
 
+TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithTooLongSni) {
+  upstream_tls_ = true;
+  initializeWithArgs(1024, 1024, "x-host");
+  std::string too_long_sni(300, 'a');
+  ASSERT_EQ(too_long_sni.size(), 300); // Validate that the expected constructor was run.
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  const Http::TestRequestHeaderMapImpl request_headers{{":method", "POST"},
+                                                       {":path", "/test/long/url"},
+                                                       {":scheme", "http"},
+                                                       {":authority", "localhost"},
+                                                       {"x-host", too_long_sni}};
+
+  auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("503", response->headers().getStatusValue());
+  // TODO(ggreenway): validate (in access logs probably) that failure reason is set appropriately.
+}
+
 // Verify that auto-SAN verification fails with an incorrect certificate.
 TEST_P(ProxyFilterIntegrationTest, UpstreamTlsInvalidSAN) {
   upstream_tls_ = true;