From 9a607425abe4f882a3a434a5050f95dbced7ecce Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 2 Nov 2020 09:19:52 +0000 Subject: [PATCH 1/6] tls: fix detection of the upstream connection close event. Fixes #13856. Signed-off-by: Piotr Sikora --- docs/root/version_history/current.rst | 1 + source/extensions/transport_sockets/tls/ssl_socket.cc | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0f331b036a46..067fa33970fb 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -25,6 +25,7 @@ Bug Fixes * dns: fix a bug where custom resolvers provided in configuration were not preserved after network issues. * http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. +* tls: fix detection of the upstream connection close event. * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. Removed Config or Runtime diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 99da62f9ae10..26466e9c53fa 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -142,8 +142,14 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { case SSL_ERROR_ZERO_RETURN: end_stream = true; break; + case SSL_ERROR_SYSCALL: + if (result.error_.value() == 0) { + 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; From 47f12b9996bff15a557aaf9e0b8009dec2bf6489 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 2 Nov 2020 14:00:55 +0000 Subject: [PATCH 2/6] review: add tests. Signed-off-by: Piotr Sikora --- .../transport_sockets/tls/ssl_socket_test.cc | 167 ++++++++++++++++++ 1 file changed, 167 insertions(+) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index d7718e17997b..5338c3988633 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2540,6 +2540,173 @@ 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->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->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); + 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"), false)) + .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus { + read_buffer.drain(read_buffer.length()); + return Network::FilterStatus::Continue; + })); + + EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)); + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose)) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { 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->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->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); + // Close without sending close_notify alert. + const SslHandshakerImpl* ssl_socket = + dynamic_cast(server_connection->ssl().get()); + 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"), false)) + .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus { + read_buffer.drain(read_buffer.length()); + return Network::FilterStatus::Continue; + })); + + EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)); + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose)) + .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 ea472af37930e5861a6b842a4481047e5d29a83e Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 3 Nov 2020 00:03:47 +0000 Subject: [PATCH 3/6] review: add comments. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/ssl_socket.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 26466e9c53fa..9d0c08815522 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -140,10 +140,12 @@ 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; } From 0ece50f2c7ab31b8d91acac4f7ebd33d28def206 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 3 Nov 2020 05:29:57 +0000 Subject: [PATCH 4/6] review: enable half-close in tests. Signed-off-by: Piotr Sikora --- .../transport_sockets/tls/ssl_socket_test.cc | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 5338c3988633..5ed4880e15b5 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2584,6 +2584,7 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { 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(); @@ -2595,6 +2596,7 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { 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); })); @@ -2608,14 +2610,15 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { 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"), false)) + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("hello"), true)) .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus { read_buffer.drain(read_buffer.length()); - return Network::FilterStatus::Continue; + 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::RemoteClose)) + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)) .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); dispatcher_->run(Event::Dispatcher::RunType::Block); @@ -2665,6 +2668,7 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { 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(); @@ -2676,10 +2680,10 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { 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"); @@ -2694,14 +2698,15 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { 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"), false)) + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("hello"), true)) .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus { read_buffer.drain(read_buffer.length()); - return Network::FilterStatus::Continue; + 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::RemoteClose)) + EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)) .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); dispatcher_->run(Event::Dispatcher::RunType::Block); From a99576d2f1a6bb404030d75b0885f411b251b305 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 3 Nov 2020 23:50:30 +0000 Subject: [PATCH 5/6] review: apply Greg's patch. Signed-off-by: Piotr Sikora --- .../transport_sockets/tls/ssl_socket_test.cc | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 0ef42abeba07..6fc875eea10f 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2600,11 +2600,11 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { 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, false); - server_connection->close(Network::ConnectionCloseType::NoFlush); + server_connection->write(data, true); })); EXPECT_CALL(*client_read_filter, onNewConnection()) @@ -2616,10 +2616,14 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { client_connection->close(Network::ConnectionCloseType::NoFlush); return Network::FilterStatus::StopIteration; })); + EXPECT_CALL(*server_read_filter, onData(_, true)); - 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(); })); + 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); } From ebfe0d4fabe9f0c309134341c34dac0216202aa8 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Wed, 4 Nov 2020 00:12:01 +0000 Subject: [PATCH 6/6] review: add some expectations. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/ssl_handshaker.h | 2 +- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/ssl_handshaker.h b/source/extensions/transport_sockets/tls/ssl_handshaker.h index 8eaec861a8f1..50090f6f43a7 100644 --- a/source/extensions/transport_sockets/tls/ssl_handshaker.h +++ b/source/extensions/transport_sockets/tls/ssl_handshaker.h @@ -67,7 +67,7 @@ class SslHandshakerImpl : public Ssl::ConnectionInfo, public Ssl::Handshaker { // Ssl::Handshaker Network::PostIoAction doHandshake() override; - Ssl::SocketState state() { return state_; } + Ssl::SocketState state() const { return state_; } void setState(Ssl::SocketState state) { state_ = state; } SSL* ssl() const { return ssl_.get(); } Ssl::HandshakeCallbacks* handshakeCallbacks() { return handshake_callbacks_; } diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 6fc875eea10f..6ba2b698f305 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2605,6 +2605,7 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) { .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()) @@ -2692,9 +2693,11 @@ TEST_P(SslSocketTest, ShutdownWithoutCloseNotify) { .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); }));