Skip to content

tls: fix detection of the upstream connection close event. #13858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggreenway merged 8 commits into from
Nov 4, 2020
Merged

tls: fix detection of the upstream connection close event. #13858

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a60742
tls: fix detection of the upstream connection close event.
PiotrSikora Nov 2, 2020
47f12b9
review: add tests.
PiotrSikora Nov 2, 2020
ea472af
review: add comments.
PiotrSikora Nov 3, 2020
0ece50f
review: enable half-close in tests.
PiotrSikora Nov 3, 2020
ca79634
Merge remote-tracking branch 'origin/master' into PiotrSikora/tls_close
PiotrSikora Nov 3, 2020
a99576d
review: apply Greg's patch.
PiotrSikora Nov 3, 2020
ebfe0d4
review: add some expectations.
PiotrSikora Nov 4, 2020
28ac59f
Merge remote-tracking branch 'origin/master' into PiotrSikora/tls_close
PiotrSikora Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,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.
Comment thread
PiotrSikora marked this conversation as resolved.
* 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
Expand Down
10 changes: 9 additions & 1 deletion source/extensions/transport_sockets/tls/ssl_socket.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,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;
Comment thread
PiotrSikora marked this conversation as resolved.
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.
Comment thread
PiotrSikora marked this conversation as resolved.
default:
drainErrorQueue();
action = PostIoAction::Close;
Expand Down
172 changes: 172 additions & 0 deletions test/extensions/transport_sockets/tls/ssl_socket_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2540,6 +2540,178 @@ 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<ServerContextConfigImpl>(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<std::string>{});

auto socket = std::make_shared<Network::TcpListenSocket>(
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<Network::MockReadFilter> server_read_filter(new Network::MockReadFilter());
std::shared_ptr<Network::MockReadFilter> 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<ClientContextConfigImpl>(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);
Comment thread
PiotrSikora marked this conversation as resolved.
Outdated
server_connection->close(Network::ConnectionCloseType::NoFlush);

@ggreenway ggreenway Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure this always happens gracefully, close with FlushWrite. Otherwise, it's best-effort to send the close notify.

@PiotrSikora PiotrSikora Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that NoFlush is best-effort in this case, and the test is not flaky at all:

$ bazel test --config=libc++ //test/extensions/transport_sockets/tls:ssl_socket_test --test_arg="--gtest_filter=*SslSocketTest.Shutdown*" --runs_per_test=1000

//test/extensions/transport_sockets/tls:ssl_socket_test                  PASSED in 1.7s
  Stats over 4000 runs: max = 1.7s, min = 0.5s, avg = 0.9s, dev = 0.2s

$ rg 'SSL_ERROR_ZERO_RETURN|SSL_ERROR_SYSCALL_EOF' bazel-testlogs/test/extensions/transport_sockets/tls/ssl_socket_test/ | cut -d" " -f5 | sort | uniq -c

   2000 SSL_ERROR_SYSCALL_EOF
   2000 SSL_ERROR_ZERO_RETURN

Also, changing this to FlushWrite:

--- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc
+++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
@@ -2603,8 +2603,8 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) {
   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);
+        server_connection->close(Network::ConnectionCloseType::FlushWrite);
       }));

   EXPECT_CALL(*client_read_filter, onNewConnection())
@@ -2613,11 +2613,11 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) {
   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);
+        client_connection->close(Network::ConnectionCloseType::FlushWrite);
         return Network::FilterStatus::StopIteration;
       }));

-  EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose));
+  EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose));
   EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose))
       .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); }));

results in "hello" being lost, and only onData("", true) being delivered. It looks like there is another bug there, but it's unrelated to this PR.

@PiotrSikora PiotrSikora Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: #13890.

@ggreenway ggreenway Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot that we're using half-open here. close() should not be called at all on the server connection. However, with that change, I'm still seeing some odd behavior in the test. I'm still trying to figure out what's wrong.

@PiotrSikora PiotrSikora Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we can test this behavior without close().

@ggreenway ggreenway Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this patch makes the test correctly cover what we want it to cover:

diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
index 0ef42abeba..6fc875eea1 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);
 }

@ggreenway ggreenway Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra call to onData("", true) was actually on the server connection, in response to the client finishing the close.

@ggreenway ggreenway Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close() isn't needed when half-close is enabled. The true for end_stream is the close.

@PiotrSikora PiotrSikora Nov 4, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch applied, thanks!

}));

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, 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<ServerContextConfigImpl>(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<std::string>{});

auto socket = std::make_shared<Network::TcpListenSocket>(
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<Network::MockReadFilter> server_read_filter(new Network::MockReadFilter());
std::shared_ptr<Network::MockReadFilter> 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<ClientContextConfigImpl>(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);
// Close without sending close_notify alert.
const SslHandshakerImpl* ssl_socket =
dynamic_cast<const SslHandshakerImpl*>(server_connection->ssl().get());
SSL_set_quiet_shutdown(ssl_socket->ssl(), 1);
server_connection->close(Network::ConnectionCloseType::NoFlush);
Comment thread
PiotrSikora marked this conversation as resolved.
}));

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;
}));

Comment thread
PiotrSikora marked this conversation as resolved.
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:
Expand Down