From 7a6da028d1d2f26ff542e711dca83861b56209aa Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Wed, 4 Nov 2020 13:20:33 -0800 Subject: [PATCH 1/3] tls: fix detection of the upstream connection close event. (#13858) Fixes #13856. Signed-off-by: Piotr Sikora Signed-off-by: Christoph Pakulski --- .../transport_sockets/tls/ssl_socket.cc | 10 +- .../transport_sockets/tls/ssl_socket_test.cc | 179 ++++++++++++++++++ 2 files changed, 188 insertions(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 00c341a5d734..8a5c0741a044 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -128,10 +128,18 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { case SSL_ERROR_WANT_READ: break; case SSL_ERROR_ZERO_RETURN: + // Graceful shutdown using close_notify TLS alert. end_stream = true; break; + case SSL_ERROR_SYSCALL: + if (result.error_.value() == 0) { + // Non-graceful shutdown by closing the underlying socket. + end_stream = true; + break; + } + FALLTHRU; case SSL_ERROR_WANT_WRITE: - // Renegotiation has started. We don't handle renegotiation so just fall through. + // Renegotiation has started. We don't handle renegotiation so just fall through. default: drainErrorQueue(); action = PostIoAction::Close; diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 76122e141a9b..a15281a76285 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2318,6 +2318,185 @@ TEST_P(SslSocketTest, HalfClose) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +TEST_P(SslSocketTest, ShutdownWithCloseNotify) { + const std::string server_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_certificates.pem" +)EOF"; + + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext server_tls_context; + TestUtility::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_tls_context); + auto server_cfg = std::make_unique(server_tls_context, factory_context_); + ContextManagerImpl manager(time_system_); + Stats::TestUtil::TestStore server_stats_store; + ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, + server_stats_store, std::vector{}); + + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true); + Network::MockTcpListenerCallbacks listener_callbacks; + Network::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = + dispatcher_->createListener(socket, listener_callbacks, true, ENVOY_TCP_BACKLOG_SIZE); + std::shared_ptr server_read_filter(new Network::MockReadFilter()); + std::shared_ptr client_read_filter(new Network::MockReadFilter()); + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + )EOF"; + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + TestUtility::loadFromYaml(TestEnvironment::substitute(client_ctx_yaml), tls_context); + auto client_cfg = std::make_unique(tls_context, factory_context_); + Stats::TestUtil::TestStore client_stats_store; + ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + client_stats_store); + Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + socket->localAddress(), Network::Address::InstanceConstSharedPtr(), + client_ssl_socket_factory.createTransportSocket(nullptr), nullptr); + Network::MockConnectionCallbacks client_connection_callbacks; + client_connection->enableHalfClose(true); + client_connection->addReadFilter(client_read_filter); + client_connection->addConnectionCallbacks(client_connection_callbacks); + client_connection->connect(); + + Network::ConnectionPtr server_connection; + Network::MockConnectionCallbacks server_connection_callbacks; + EXPECT_CALL(listener_callbacks, onAccept_(_)) + .WillOnce(Invoke([&](Network::ConnectionSocketPtr& socket) -> void { + server_connection = dispatcher_->createServerConnection( + std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr), + stream_info_); + server_connection->enableHalfClose(true); + server_connection->addReadFilter(server_read_filter); + server_connection->addConnectionCallbacks(server_connection_callbacks); + })); + EXPECT_CALL(*server_read_filter, onNewConnection()); + EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::Connected)) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { + Buffer::OwnedImpl data("hello"); + server_connection->write(data, true); + EXPECT_EQ(data.length(), 0); + })); + + EXPECT_CALL(*client_read_filter, onNewConnection()) + .WillOnce(Return(Network::FilterStatus::Continue)); + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::Connected)); + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("hello"), true)) + .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus { + read_buffer.drain(read_buffer.length()); + client_connection->close(Network::ConnectionCloseType::NoFlush); + return Network::FilterStatus::StopIteration; + })); + EXPECT_CALL(*server_read_filter, onData(_, true)); + + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)); + EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose)) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { + server_connection->close(Network::ConnectionCloseType::NoFlush); + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + +TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { + const std::string server_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_certificates.pem" +)EOF"; + + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext server_tls_context; + TestUtility::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_tls_context); + auto server_cfg = std::make_unique(server_tls_context, factory_context_); + ContextManagerImpl manager(time_system_); + Stats::TestUtil::TestStore server_stats_store; + ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, + server_stats_store, std::vector{}); + + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true); + Network::MockTcpListenerCallbacks listener_callbacks; + Network::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = + dispatcher_->createListener(socket, listener_callbacks, true, ENVOY_TCP_BACKLOG_SIZE); + std::shared_ptr server_read_filter(new Network::MockReadFilter()); + std::shared_ptr client_read_filter(new Network::MockReadFilter()); + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + )EOF"; + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + TestUtility::loadFromYaml(TestEnvironment::substitute(client_ctx_yaml), tls_context); + auto client_cfg = std::make_unique(tls_context, factory_context_); + Stats::TestUtil::TestStore client_stats_store; + ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + client_stats_store); + Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + socket->localAddress(), Network::Address::InstanceConstSharedPtr(), + client_ssl_socket_factory.createTransportSocket(nullptr), nullptr); + Network::MockConnectionCallbacks client_connection_callbacks; + client_connection->enableHalfClose(true); + client_connection->addReadFilter(client_read_filter); + client_connection->addConnectionCallbacks(client_connection_callbacks); + client_connection->connect(); + + Network::ConnectionPtr server_connection; + Network::MockConnectionCallbacks server_connection_callbacks; + EXPECT_CALL(listener_callbacks, onAccept_(_)) + .WillOnce(Invoke([&](Network::ConnectionSocketPtr& socket) -> void { + server_connection = dispatcher_->createServerConnection( + std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr), + stream_info_); + server_connection->enableHalfClose(true); + server_connection->addReadFilter(server_read_filter); + server_connection->addConnectionCallbacks(server_connection_callbacks); + })); + EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::Connected)) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { + Buffer::OwnedImpl data("hello"); + server_connection->write(data, false); + EXPECT_EQ(data.length(), 0); + // Close without sending close_notify alert. + const SslHandshakerImpl* ssl_socket = + dynamic_cast(server_connection->ssl().get()); + EXPECT_EQ(ssl_socket->state(), Ssl::SocketState::HandshakeComplete); + SSL_set_quiet_shutdown(ssl_socket->ssl(), 1); + server_connection->close(Network::ConnectionCloseType::NoFlush); + })); + + EXPECT_CALL(*client_read_filter, onNewConnection()) + .WillOnce(Return(Network::FilterStatus::Continue)); + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::Connected)); + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("hello"), true)) + .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus { + read_buffer.drain(read_buffer.length()); + client_connection->close(Network::ConnectionCloseType::NoFlush); + return Network::FilterStatus::StopIteration; + })); + + EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)); + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + TEST_P(SslSocketTest, ClientAuthMultipleCAs) { const std::string server_ctx_yaml = R"EOF( common_tls_context: From 3126d3b4c012e416f1a7594be880a7fb12f4231c Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 8 Jan 2021 22:15:09 +0000 Subject: [PATCH 2/3] Fixed tests and added release note. Signed-off-by: Christoph Pakulski --- docs/root/intro/version_history.rst | 1 + .../transport_sockets/tls/ssl_socket_test.cc | 39 ++++++++----------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index c24bf1bd675a..63149af39405 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ================ Changes ------- +* tls: fix detection of the upstream connection close event. 1.13.7 (December 7, 2020) ========================= diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index a15281a76285..5b28d9bb4305 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2323,9 +2323,9 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { common_tls_context: tls_certificates: certificate_chain: - filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + filename: "{{ test_tmpdir }}/unittestcert.pem" private_key: - filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + filename: "{{ test_tmpdir }}/unittestkey.pem" validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_certificates.pem" @@ -2335,16 +2335,15 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { TestUtility::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_tls_context); auto server_cfg = std::make_unique(server_tls_context, factory_context_); ContextManagerImpl manager(time_system_); - Stats::TestUtil::TestStore server_stats_store; + Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true); - Network::MockTcpListenerCallbacks listener_callbacks; + Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = - dispatcher_->createListener(socket, listener_callbacks, true, ENVOY_TCP_BACKLOG_SIZE); + Network::ListenerPtr listener = dispatcher_->createListener(socket, listener_callbacks, true); std::shared_ptr server_read_filter(new Network::MockReadFilter()); std::shared_ptr client_read_filter(new Network::MockReadFilter()); @@ -2355,7 +2354,7 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(client_ctx_yaml), tls_context); auto client_cfg = std::make_unique(tls_context, factory_context_); - Stats::TestUtil::TestStore client_stats_store; + Stats::IsolatedStoreImpl client_stats_store; ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( @@ -2372,8 +2371,7 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { EXPECT_CALL(listener_callbacks, onAccept_(_)) .WillOnce(Invoke([&](Network::ConnectionSocketPtr& socket) -> void { server_connection = dispatcher_->createServerConnection( - std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr), - stream_info_); + std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr)); server_connection->enableHalfClose(true); server_connection->addReadFilter(server_read_filter); server_connection->addConnectionCallbacks(server_connection_callbacks); @@ -2412,9 +2410,9 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { common_tls_context: tls_certificates: certificate_chain: - filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + filename: "{{ test_tmpdir }}/unittestcert.pem" private_key: - filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + filename: "{{ test_tmpdir }}/unittestkey.pem" validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_certificates.pem" @@ -2424,16 +2422,15 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { TestUtility::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_tls_context); auto server_cfg = std::make_unique(server_tls_context, factory_context_); ContextManagerImpl manager(time_system_); - Stats::TestUtil::TestStore server_stats_store; + Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true); - Network::MockTcpListenerCallbacks listener_callbacks; + Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = - dispatcher_->createListener(socket, listener_callbacks, true, ENVOY_TCP_BACKLOG_SIZE); + Network::ListenerPtr listener = dispatcher_->createListener(socket, listener_callbacks, true); std::shared_ptr server_read_filter(new Network::MockReadFilter()); std::shared_ptr client_read_filter(new Network::MockReadFilter()); @@ -2444,7 +2441,7 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(client_ctx_yaml), tls_context); auto client_cfg = std::make_unique(tls_context, factory_context_); - Stats::TestUtil::TestStore client_stats_store; + Stats::IsolatedStoreImpl client_stats_store; ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( @@ -2461,8 +2458,7 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { EXPECT_CALL(listener_callbacks, onAccept_(_)) .WillOnce(Invoke([&](Network::ConnectionSocketPtr& socket) -> void { server_connection = dispatcher_->createServerConnection( - std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr), - stream_info_); + std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr)); server_connection->enableHalfClose(true); server_connection->addReadFilter(server_read_filter); server_connection->addConnectionCallbacks(server_connection_callbacks); @@ -2473,10 +2469,9 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { server_connection->write(data, false); EXPECT_EQ(data.length(), 0); // Close without sending close_notify alert. - const SslHandshakerImpl* ssl_socket = - dynamic_cast(server_connection->ssl().get()); - EXPECT_EQ(ssl_socket->state(), Ssl::SocketState::HandshakeComplete); - SSL_set_quiet_shutdown(ssl_socket->ssl(), 1); + const SslSocketInfo* ssl_socket = + dynamic_cast(server_connection->ssl().get()); + SSL_set_quiet_shutdown(ssl_socket->rawSslForTest(), 1); server_connection->close(Network::ConnectionCloseType::NoFlush); })); From 80680cde59acc27907d21b51dff17bbd1a90c5d3 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 8 Jan 2021 23:20:35 +0000 Subject: [PATCH 3/3] Fixed release notes format. Signed-off-by: Christoph Pakulski --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 3bee5732a016..3634a889cb04 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,8 +5,8 @@ Version history ================ Changes ------- -* tls: fix detection of the upstream connection close event. * http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. +* tls: fix detection of the upstream connection close event. 1.13.7 (December 7, 2020) =========================