diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce..8cdc4928cd36 100644 --- 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 ` + if the downstream request ``:authority`` was longer than 255 characters. + removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index f3132d2ffb9b..e3c7ddb37812 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -452,7 +452,7 @@ std::vector ContextImpl::parseAlpnProtocols(const std::string& alpn_pro return out; } -bssl::UniquePtr +absl::StatusOr> 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& alpn, SSL& ssl return false; } -bssl::UniquePtr +absl::StatusOr> ClientContextImpl::newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) { - bssl::UniquePtr ssl_con(ContextImpl::newSsl(options)); + absl::StatusOr> ssl_con_or_status(ContextImpl::newSsl(options)); + if (!ssl_con_or_status.ok()) { + return ssl_con_or_status; + } + + bssl::UniquePtr 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 index 8654d44e706e..f0b89a55e122 100644 --- 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 { public: - virtual bssl::UniquePtr newSsl(const Network::TransportSocketOptionsConstSharedPtr& options); + virtual absl::StatusOr> + 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 + absl::StatusOr> 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 index 3a982d5a23a7..6a23adb03d9f 100644 --- 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> +SslSocket::create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state, + const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, + Ssl::HandshakerFactoryCb handshaker_factory_cb) { + std::unique_ptr 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(ctx)), - info_(std::dynamic_pointer_cast(handshaker_factory_cb( - ctx_->newSsl(transport_socket_options_), ctx_->sslExtendedSocketInfoIndex(), this))) { + ctx_(std::dynamic_pointer_cast(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(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(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(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(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(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 index ea9213ffe6d1..f0c679b23e70 100644 --- 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 { public: - SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state, - const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, - Ssl::HandshakerFactoryCb handshaker_factory_cb); + static absl::StatusOr> + 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 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 index d632e083e978..fdaf0f720639 100644 --- 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;