This is an automated email from the ASF dual-hosted git repository. bennoe pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 4e21abc1910caa923c5f805bab032d25bd3cf681 Author: Benno Evers <[email protected]> AuthorDate: Fri May 31 16:08:42 2019 +0200 Added unit tests for hostname validation. While going through the existing tests to look for candidates that would benefit from being tested for both hostname validation schemes, I noticed a number of existing tests where test setup did not quite match the comment or test name. I fixed these up in the same review. Review: https://reviews.apache.org/r/70797 --- 3rdparty/libprocess/include/process/ssl/gtest.hpp | 53 +++++-- 3rdparty/libprocess/src/tests/ssl_client.cpp | 14 +- 3rdparty/libprocess/src/tests/ssl_tests.cpp | 172 +++++++++++++++++----- 3 files changed, 187 insertions(+), 52 deletions(-) diff --git a/3rdparty/libprocess/include/process/ssl/gtest.hpp b/3rdparty/libprocess/include/process/ssl/gtest.hpp index 6cdd781..30a15f4 100644 --- a/3rdparty/libprocess/include/process/ssl/gtest.hpp +++ b/3rdparty/libprocess/include/process/ssl/gtest.hpp @@ -333,27 +333,31 @@ protected: * SSL client subprocess with. * @param use_ssl_socket Whether the SSL client will try to connect * using an SSL socket or a POLL socket. + * @param hostname The hostname to use for TLS certificate validation. + * It is passed separately because some tests want to provide the + * "wrong" hostname to test error conditions. * * @return Subprocess if successful otherwise an Error. */ Try<process::Subprocess> launch_client( const std::map<std::string, std::string>& environment, - const process::network::inet::Socket& server, + const Option<std::string>& hostname, + const net::IP& ip, + uint16_t port, bool use_ssl_socket) { - const Try<process::network::inet::Address> address = server.address(); - if (address.isError()) { - return Error(address.error()); - } - // Set up arguments to be passed to the 'client-ssl' binary. - const std::vector<std::string> argv = { + std::vector<std::string> argv = { "ssl-client", "--use_ssl=" + stringify(use_ssl_socket), - "--server=" + stringify(address->ip), - "--port=" + stringify(address->port), + "--server=" + stringify(ip), + "--port=" + stringify(port), "--data=" + data}; + if (hostname.isSome()) { + argv.push_back("--server_hostname=" + hostname.get()); + } + Result<std::string> path = os::realpath(BUILD_DIR); if (!path.isSome()) { return Error("Could not establish build directory path"); @@ -375,6 +379,37 @@ protected: full_environment); } + /** + * Launches a test SSL client as a subprocess connecting to the + * server. This is a convenience overload for `launch_client()` + * that uses the IP address from the passed Socket as the server + * hostname. + * + * @param environment The SSL environment variables to launch the + * SSL client subprocess with. + * @param use_ssl_socket Whether the SSL client will try to connect + * using an SSL socket or a POLL socket. + * + * @return Subprocess if successful otherwise an Error. + */ + Try<process::Subprocess> launch_client( + const std::map<std::string, std::string>& environment, + const process::network::inet::Socket& server, + bool use_ssl_socket) + { + const Try<process::network::inet::Address> address = server.address(); + if (address.isError()) { + return Error(address.error()); + } + + return launch_client( + environment, + None(), + address->ip, + address->port, + use_ssl_socket); + } + static constexpr size_t BACKLOG = 5; const std::string data; diff --git a/3rdparty/libprocess/src/tests/ssl_client.cpp b/3rdparty/libprocess/src/tests/ssl_client.cpp index afa0a9c..1f4470b 100644 --- a/3rdparty/libprocess/src/tests/ssl_client.cpp +++ b/3rdparty/libprocess/src/tests/ssl_client.cpp @@ -70,7 +70,14 @@ public: "The message to send as the client.", "Hello World"); - add(&Flags::server, "server", "IP address of server", "127.0.0.1"); + add(&Flags::server, + "server", + "IP address of server", + "127.0.0.1"); + + add(&Flags::server_hostname, + "server_hostname", + "Hostname of server"); add(&Flags::port, "port", "Port of server", 5050); } @@ -78,6 +85,7 @@ public: bool use_ssl; string data; string server; + Option<string> server_hostname; uint16_t port; }; @@ -142,7 +150,7 @@ TEST_F(SSLClientTest, client) Try<net::IP> ip = net::IP::parse(flags.server, AF_INET); EXPECT_SOME(ip); - // Connect to the server socket located at `ip:port`. + // Connect to the server. Address address(ip.get(), flags.port); Future<Nothing> connect = [&]() { switch(socket.kind()) { @@ -151,7 +159,7 @@ TEST_F(SSLClientTest, client) case SocketImpl::Kind::SSL: return socket.connect( address, - openssl::create_tls_client_config(None())); + openssl::create_tls_client_config(flags.server_hostname)); } UNREACHABLE(); }(); diff --git a/3rdparty/libprocess/src/tests/ssl_tests.cpp b/3rdparty/libprocess/src/tests/ssl_tests.cpp index e1790d5..9d5ab67 100644 --- a/3rdparty/libprocess/src/tests/ssl_tests.cpp +++ b/3rdparty/libprocess/src/tests/ssl_tests.cpp @@ -44,6 +44,10 @@ using std::vector; // We only run these tests if we have configured with '--enable-ssl'. #ifdef USE_SSL_SOCKET +#if OPENSSL_VERSION_NUMBER >= 0x10002000L +#define HOSTNAME_VALIDATION_OPENSSL +#endif + namespace http = process::http; namespace io = process::io; namespace network = process::network; @@ -129,6 +133,13 @@ static const vector<string> protocols = { #endif }; +static const vector<string> hostname_validation_schemes = { + "legacy", +#ifdef HOSTNAME_VALIDATION_OPENSSL + "openssl", +#endif +}; + // Ensure that we can't create an SSL socket when SSL is not enabled. TEST(SSL, Disabled) @@ -193,26 +204,37 @@ TEST_F(SSLTest, NonSSLSocket) } +class SSLTestStringParameter + : public SSLTest, + public ::testing::WithParamInterface<std::string> {}; + + +INSTANTIATE_TEST_CASE_P(HostnameValidationScheme, + SSLTestStringParameter, + ::testing::ValuesIn(hostname_validation_schemes)); + + // Ensure that a certificate that was not generated using the // certificate authority is still allowed to communicate as long as // the LIBPROCESS_SSL_VERIFY_CERT and LIBPROCESS_SSL_REQUIRE_CERT // flags are disabled. -TEST_F(SSLTest, NoVerifyBadCA) +TEST_P(SSLTestStringParameter, NoVerifyBadCA) { Try<Socket> server = setup_server({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, - {"LIBPROCESS_SSL_VERIFY_CERT", "false"}, - {"LIBPROCESS_SSL_REQUIRE_CERT", "false"}}); + {"LIBPROCESS_SSL_REQUIRE_CERT", "false"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", GetParam()}}); ASSERT_SOME(server); Try<Subprocess> client = launch_client({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", scrap_key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", scrap_certificate_path().string()}, - {"LIBPROCESS_SSL_REQUIRE_CERT", "true"}, - {"LIBPROCESS_SSL_CA_FILE", certificate_path().string()}}, + {"LIBPROCESS_SSL_VERIFY_CERT", "false"}, + {"LIBPROCESS_SSL_CA_FILE", certificate_path().string()}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", GetParam()}}, server.get(), true); ASSERT_SOME(client); @@ -228,23 +250,29 @@ TEST_F(SSLTest, NoVerifyBadCA) } -// Ensure that a certificate that was not generated using the +// Ensure that a client certificate that was not generated using the // certificate authority is NOT allowed to communicate when the // LIBPROCESS_SSL_REQUIRE_CERT flag is enabled. +// +// NOTE: We cannot run this test with the 'legacy' hostname +// validation scheme due to MESOS-9867. +#ifdef HOSTNAME_VALIDATION_OPENSSL TEST_F(SSLTest, RequireBadCA) { Try<Socket> server = setup_server({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, - {"LIBPROCESS_SSL_REQUIRE_CERT", "true"}}); + {"LIBPROCESS_SSL_REQUIRE_CERT", "true"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", "openssl"}}); ASSERT_SOME(server); Try<Subprocess> client = launch_client({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", scrap_key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", scrap_certificate_path().string()}, - {"LIBPROCESS_SSL_REQUIRE_CERT", "false"}}, + {"LIBPROCESS_SSL_VERIFY_CERT", "false"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", "openssl"}}, server.get(), true); ASSERT_SOME(client); @@ -254,26 +282,37 @@ TEST_F(SSLTest, RequireBadCA) AWAIT_ASSERT_READY(await_subprocess(client.get(), None())); } +#endif // HOSTNAME_VALIDATION_OPENSSL -// Ensure that a certificate that was not generated using the +// Ensure that a server certificate that was not generated using the // certificate authority is NOT allowed to communicate when the // LIBPROCESS_SSL_VERIFY_CERT flag is enabled. -TEST_F(SSLTest, VerifyBadCA) +TEST_P(SSLTestStringParameter, VerifyBadCA) { Try<Socket> server = setup_server({ {"LIBPROCESS_SSL_ENABLED", "true"}, - {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, - {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, - {"LIBPROCESS_SSL_REQUIRE_CERT", "true"}}); + {"LIBPROCESS_SSL_KEY_FILE", scrap_key_path().string()}, + {"LIBPROCESS_SSL_CERT_FILE", scrap_certificate_path().string()}, + {"LIBPROCESS_SSL_REQUIRE_CERT", "false"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", GetParam()}}); ASSERT_SOME(server); + Try<std::string> hostname = net::getHostname(process::address().ip); + ASSERT_SOME(hostname); + + Try<Address> address = server->address(); + ASSERT_SOME(address); + Try<Subprocess> client = launch_client({ {"LIBPROCESS_SSL_ENABLED", "true"}, - {"LIBPROCESS_SSL_KEY_FILE", scrap_key_path().string()}, - {"LIBPROCESS_SSL_CERT_FILE", scrap_certificate_path().string()}, - {"LIBPROCESS_SSL_VERIFY_CERT", "false"}}, - server.get(), + {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, + {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, + {"LIBPROCESS_SSL_VERIFY_CERT", "true"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", GetParam()}}, + *hostname, + address->ip, + address->port, true); ASSERT_SOME(client); @@ -284,68 +323,120 @@ TEST_F(SSLTest, VerifyBadCA) } -// Ensure that a server that attempts to present no certificate at all -// is NOT allowed to communicate when the LIBPROCESS_SSL_VERIFY_CERT -// flag is enabled in the client. -TEST_F(SSLTest, NoAnonymousCipherIfVerify) +// Ensure that a certificate that WAS generated using the certificate +// authority IS allowed to communicate when the +// LIBPROCESS_SSL_VERIFY_CERT and LIBPROCESS_SSL_REQUIRE_CERT flags are +// enabled. +// +// NOTE: If this test is failing for the 'legacy' scheme, subsequent +// tests may be affected due to MESOS-9867. +TEST_P(SSLTestStringParameter, VerifyCertificate) { Try<Socket> server = setup_server({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, - // ADH stands for "Anonymous Diffie-Hellman", and is the only - // anonymous cipher supported by OpenSSL out of the box. - {"LIBPROCESS_SSL_CIPHERS", "ADH-AES256-SHA"}}); + {"LIBPROCESS_SSL_CA_FILE", certificate_path().string()}, + {"LIBPROCESS_SSL_REQUIRE_CERT", "true"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", GetParam()}}); ASSERT_SOME(server); + Try<std::string> hostname = net::getHostname(process::address().ip); + ASSERT_SOME(hostname); + + Try<Address> address = server->address(); + ASSERT_SOME(address); + Try<Subprocess> client = launch_client({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, + {"LIBPROCESS_SSL_CA_FILE", certificate_path().string()}, {"LIBPROCESS_SSL_VERIFY_CERT", "true"}, - {"LIBPROCESS_SSL_CIPHERS", "ADH-AES256-SHA"}}, - server.get(), + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", GetParam()}}, + *hostname, + address->ip, + address->port, true); ASSERT_SOME(client); Future<Socket> socket = server->accept(); - AWAIT_ASSERT_FAILED(socket); + AWAIT_ASSERT_READY(socket); - AWAIT_ASSERT_READY(await_subprocess(client.get(), None())); + // TODO(jmlvanre): Remove const copy. + AWAIT_ASSERT_EQ(data, Socket(socket.get()).recv()); + AWAIT_ASSERT_READY(Socket(socket.get()).send(data)); + + AWAIT_ASSERT_READY(await_subprocess(client.get(), 0)); } -// Ensure that a certificate that WAS generated using the certificate -// authority IS allowed to communicate when the -// LIBPROCESS_SSL_VERIFY_CERT flag is enabled. -TEST_F(SSLTest, VerifyCertificate) +// Ensure that a server presenting a valid certificate with a not matching +// hostname is NOT allowed to communicate when the +// LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME flag is set to 'openssl'. +#ifdef HOSTNAME_VALIDATION_OPENSSL +TEST_F(SSLTest, HostnameMismatch) { Try<Socket> server = setup_server({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, {"LIBPROCESS_SSL_CA_FILE", certificate_path().string()}, - {"LIBPROCESS_SSL_VERIFY_CERT", "true"}}); + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", "openssl"}}); ASSERT_SOME(server); + Try<Address> address = server->address(); + ASSERT_SOME(address); + Try<Subprocess> client = launch_client({ {"LIBPROCESS_SSL_ENABLED", "true"}, {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, {"LIBPROCESS_SSL_CA_FILE", certificate_path().string()}, - {"LIBPROCESS_SSL_REQUIRE_CERT", "true"}}, - server.get(), + {"LIBPROCESS_SSL_VERIFY_CERT", "true"}, + {"LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", "openssl"}}, + "invalid.example.org", + address->ip, + address->port, true); ASSERT_SOME(client); Future<Socket> socket = server->accept(); - AWAIT_ASSERT_READY(socket); + AWAIT_ASSERT_FAILED(socket); - // TODO(jmlvanre): Remove const copy. - AWAIT_ASSERT_EQ(data, Socket(socket.get()).recv()); - AWAIT_ASSERT_READY(Socket(socket.get()).send(data)); + AWAIT_ASSERT_READY(await_subprocess(client.get(), None())); +} +#endif // HOSTNAME_VALIDATION_OPENSSL - AWAIT_ASSERT_READY(await_subprocess(client.get(), 0)); + +// Ensure that a server that attempts to present no certificate at all +// is NOT allowed to communicate when the LIBPROCESS_SSL_VERIFY_CERT +// flag is enabled in the client. +TEST_F(SSLTest, NoAnonymousCipherIfVerify) +{ + Try<Socket> server = setup_server({ + {"LIBPROCESS_SSL_ENABLED", "true"}, + {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, + {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, + // ADH stands for "Anonymous Diffie-Hellman", and is the only + // anonymous cipher supported by OpenSSL out of the box. + {"LIBPROCESS_SSL_CIPHERS", "ADH-AES256-SHA"}}); + ASSERT_SOME(server); + + Try<Subprocess> client = launch_client({ + {"LIBPROCESS_SSL_ENABLED", "true"}, + {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, + {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}, + {"LIBPROCESS_SSL_VERIFY_CERT", "true"}, + {"LIBPROCESS_SSL_CIPHERS", "ADH-AES256-SHA"}}, + server.get(), + true); + ASSERT_SOME(client); + + Future<Socket> socket = server->accept(); + AWAIT_ASSERT_FAILED(socket); + + AWAIT_ASSERT_READY(await_subprocess(client.get(), None())); } @@ -777,6 +868,7 @@ TEST_P(SSLVerifyIPAddTest, BasicSameProcess) os::setenv("LIBPROCESS_SSL_CA_DIR", os::getcwd()); os::setenv("LIBPROCESS_SSL_CA_FILE", certificate_path().string()); os::setenv("LIBPROCESS_SSL_VERIFY_IPADD", GetParam()); + os::setenv("LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME", "legacy"); openssl::reinitialize();
