From 138d3d1ef00391d290d37412d3cfbac0a2e3d53f Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Thu, 24 Sep 2020 16:53:38 +0000 Subject: [PATCH 1/4] Windows: Fix ssl_socket_test - SmallReadsIntoSameSlice test: use write method on connection instead of transport socket so SSL_write failures will be properly retried, also otherwise empty writes from connection will trigger failed assertions in buffer::linearize - When SSL shutdown is not appropriate, perform basic socket shutdown; relevant when client certificate is invalid and connection should be dropped by server, however we still need to ensure TLS alerts are able to be read by client, shutdown on socket ensures this can happen - Open files for test in text mode to take care of line ending issues Signed-off-by: Sunjay Bhatia --- .../extensions/transport_sockets/tls/ssl_socket.cc | 13 +++++++++++++ .../extensions/transport_sockets/tls/ssl_socket.h | 1 + test/extensions/transport_sockets/tls/BUILD | 2 -- .../transport_sockets/tls/ssl_socket_test.cc | 2 +- test/test_common/environment.cc | 2 +- 5 files changed, 16 insertions(+), 4 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 4854684430963..29edfa412de35 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -291,6 +291,15 @@ void SslSocket::shutdownSsl() { } } +void SslSocket::shutdownBasic() { + if (info_->state() != Ssl::SocketState::ShutdownSent && + callbacks_->connection().state() != Network::Connection::State::Closed) { + callbacks_->ioHandle().shutdown(ENVOY_SHUT_WR); + drainErrorQueue(); + info_->setState(Ssl::SocketState::ShutdownSent); + } +} + void SslSocket::closeSocket(Network::ConnectionEvent) { // Unregister the SSL connection object from private key method providers. for (auto const& provider : ctx_->getPrivateKeyMethodProviders()) { @@ -303,6 +312,10 @@ void SslSocket::closeSocket(Network::ConnectionEvent) { if (info_->state() == Ssl::SocketState::HandshakeInProgress || info_->state() == Ssl::SocketState::HandshakeComplete) { shutdownSsl(); + } else { + // We're not in a state to do the full SSL shutdown so perform a basic shutdown to flush any + // outstanding alerts + shutdownBasic(); } } diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index c14cb502bed15..0e539324f91f1 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -85,6 +85,7 @@ class SslSocket : public Network::TransportSocket, Network::PostIoAction doHandshake(); void drainErrorQueue(); void shutdownSsl(); + void shutdownBasic(); bool isThreadSafe() const { return callbacks_ != nullptr && callbacks_->connection().dispatcher().isThreadSafe(); } diff --git a/test/extensions/transport_sockets/tls/BUILD b/test/extensions/transport_sockets/tls/BUILD index 4a26a237b6c11..aee6a525930cf 100644 --- a/test/extensions/transport_sockets/tls/BUILD +++ b/test/extensions/transport_sockets/tls/BUILD @@ -24,8 +24,6 @@ envoy_cc_test( ], external_deps = ["ssl"], shard_count = 4, - # TODO(wrowe): Diagnose timeout error on Windows (skipped for the moment) - tags = ["fails_on_windows"], deps = [ ":test_private_key_method_provider_test_lib", "//include/envoy/network:transport_socket_interface", diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 92e1fcd98af63..32032cb0dbf91 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4840,7 +4840,7 @@ TEST_P(SslReadBufferLimitTest, SmallReadsIntoSameSlice) { for (uint32_t i = 0; i < num_writes; i++) { Buffer::OwnedImpl data(std::string(write_size, 'a')); - client_transport_socket_->doWrite(data, false); + client_connection_->write(data, false); } dispatcher_->run(Event::Dispatcher::RunType::Block); diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index b70a9d9c8cf7b..14759027896e9 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -343,7 +343,7 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path, std::string TestEnvironment::readFileToStringForTest(const std::string& filename, bool require_existence) { - std::ifstream file(filename, std::ios::binary); + std::ifstream file(filename); if (file.fail()) { if (!require_existence) { return ""; From 8331df9ec7f37c3d8458f25ed486a65d6524d045 Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Fri, 25 Sep 2020 15:52:01 +0000 Subject: [PATCH 2/4] Reverse decision on binary output Signed-off-by: Sunjay Bhatia --- source/common/filesystem/win32/filesystem_impl.cc | 2 +- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 4 ++-- test/test_common/environment.cc | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index cfdb3098fe1e2..44498d200c49e 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -129,7 +129,7 @@ std::string InstanceImplWin32::fileReadToEnd(const std::string& path) { // On Windows, we need to explicitly set the file mode as binary. Otherwise, // 0x1a will be treated as EOF - std::ifstream file(path, std::ios_base::binary); + std::ifstream file(path, std::ios::binary); if (file.fail()) { auto last_error = ::GetLastError(); if (last_error == ERROR_FILE_NOT_FOUND) { diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 32032cb0dbf91..95ae1428bc8a2 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -377,7 +377,7 @@ void testUtil(const TestUtilOptions& options) { if (!options.expectedPeerCert().empty()) { std::string urlencoded = absl::StrReplaceAll( options.expectedPeerCert(), - {{"\n", "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); + {{"\r", ""}, {"\n", "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); // Assert twice to ensure a cached value is returned and still valid. EXPECT_EQ(urlencoded, server_connection->ssl()->urlEncodedPemEncodedPeerCertificate()); EXPECT_EQ(urlencoded, server_connection->ssl()->urlEncodedPemEncodedPeerCertificate()); @@ -385,7 +385,7 @@ void testUtil(const TestUtilOptions& options) { if (!options.expectedPeerCertChain().empty()) { std::string cert_chain = absl::StrReplaceAll( options.expectedPeerCertChain(), - {{"\n", "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); + {{"\r", ""}, {"\n", "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); // Assert twice to ensure a cached value is returned and still valid. EXPECT_EQ(cert_chain, server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain()); EXPECT_EQ(cert_chain, server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain()); diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index 14759027896e9..b70a9d9c8cf7b 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -343,7 +343,7 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path, std::string TestEnvironment::readFileToStringForTest(const std::string& filename, bool require_existence) { - std::ifstream file(filename); + std::ifstream file(filename, std::ios::binary); if (file.fail()) { if (!require_existence) { return ""; From 144c7ae696e1de50bba78f1880a38f33abd8d613 Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Fri, 25 Sep 2020 17:50:48 +0000 Subject: [PATCH 3/4] Use cleaner \r replacement Signed-off-by: Sunjay Bhatia --- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 95ae1428bc8a2..1ce4c67210478 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -377,7 +377,7 @@ void testUtil(const TestUtilOptions& options) { if (!options.expectedPeerCert().empty()) { std::string urlencoded = absl::StrReplaceAll( options.expectedPeerCert(), - {{"\r", ""}, {"\n", "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); + {{TestEnvironment::newLine, "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); // Assert twice to ensure a cached value is returned and still valid. EXPECT_EQ(urlencoded, server_connection->ssl()->urlEncodedPemEncodedPeerCertificate()); EXPECT_EQ(urlencoded, server_connection->ssl()->urlEncodedPemEncodedPeerCertificate()); @@ -385,7 +385,7 @@ void testUtil(const TestUtilOptions& options) { if (!options.expectedPeerCertChain().empty()) { std::string cert_chain = absl::StrReplaceAll( options.expectedPeerCertChain(), - {{"\r", ""}, {"\n", "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); + {{TestEnvironment::newLine, "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); // Assert twice to ensure a cached value is returned and still valid. EXPECT_EQ(cert_chain, server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain()); EXPECT_EQ(cert_chain, server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain()); From ef8db9cee8587b7a6389afa73322e806d373ba69 Mon Sep 17 00:00:00 2001 From: William A Rowe Jr Date: Fri, 25 Sep 2020 13:54:49 -0400 Subject: [PATCH 4/4] Fix format Signed-off-by: William A Rowe Jr --- .../transport_sockets/tls/ssl_socket_test.cc | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 1ce4c67210478..21dd228179bba 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -375,17 +375,23 @@ void testUtil(const TestUtilOptions& options) { server_connection->ssl()->subjectLocalCertificate()); } if (!options.expectedPeerCert().empty()) { - std::string urlencoded = absl::StrReplaceAll( - options.expectedPeerCert(), - {{TestEnvironment::newLine, "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); + std::string urlencoded = + absl::StrReplaceAll(options.expectedPeerCert(), {{TestEnvironment::newLine, "%0A"}, + {" ", "%20"}, + {"+", "%2B"}, + {"/", "%2F"}, + {"=", "%3D"}}); // Assert twice to ensure a cached value is returned and still valid. EXPECT_EQ(urlencoded, server_connection->ssl()->urlEncodedPemEncodedPeerCertificate()); EXPECT_EQ(urlencoded, server_connection->ssl()->urlEncodedPemEncodedPeerCertificate()); } if (!options.expectedPeerCertChain().empty()) { - std::string cert_chain = absl::StrReplaceAll( - options.expectedPeerCertChain(), - {{TestEnvironment::newLine, "%0A"}, {" ", "%20"}, {"+", "%2B"}, {"/", "%2F"}, {"=", "%3D"}}); + std::string cert_chain = + absl::StrReplaceAll(options.expectedPeerCertChain(), {{TestEnvironment::newLine, "%0A"}, + {" ", "%20"}, + {"+", "%2B"}, + {"/", "%2F"}, + {"=", "%3D"}}); // Assert twice to ensure a cached value is returned and still valid. EXPECT_EQ(cert_chain, server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain()); EXPECT_EQ(cert_chain, server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain());