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 0a081e01a3f4af8141a8085ed2f97ee85ea48fe1 Author: Benno Evers <[email protected]> AuthorDate: Wed Jun 19 15:49:11 2019 +0200 Introduced RFC6125-compliant hostname validation scheme. This commit introduces a new libprocess SSL flag `hostname_validation_scheme`, which can be set to 'legacy' to select the previous hostname validation behaviour or to 'openssl' to use standardized OpenSSL algorithms to handle hostname validation as part of the TLS handshake. As a nice side-effect, the new scheme gets rid of reverse DNS lookups during TLS connection establishment, which used to be a common source of hard-to-debug unresponsiveness in Mesos components. See `docs/ssl.md` in the follow-up commit for details of and differences between the schemes. Review: https://reviews.apache.org/r/70749 --- 3rdparty/libprocess/include/process/ssl/flags.hpp | 1 + 3rdparty/libprocess/src/openssl.cpp | 205 +++++++++++++++++---- 3rdparty/libprocess/src/openssl.hpp | 10 + .../src/posix/libevent/libevent_ssl_socket.cpp | 99 +++++----- .../src/posix/libevent/libevent_ssl_socket.hpp | 4 +- 5 files changed, 230 insertions(+), 89 deletions(-) diff --git a/3rdparty/libprocess/include/process/ssl/flags.hpp b/3rdparty/libprocess/include/process/ssl/flags.hpp index f3483f9..1a0e382 100644 --- a/3rdparty/libprocess/include/process/ssl/flags.hpp +++ b/3rdparty/libprocess/include/process/ssl/flags.hpp @@ -46,6 +46,7 @@ public: Option<std::string> ca_file; std::string ciphers; std::string ecdh_curves; + std::string hostname_validation_scheme; bool enable_ssl_v3; bool enable_tls_v1_0; bool enable_tls_v1_1; diff --git a/3rdparty/libprocess/src/openssl.cpp b/3rdparty/libprocess/src/openssl.cpp index 240f468..fb03032 100644 --- a/3rdparty/libprocess/src/openssl.cpp +++ b/3rdparty/libprocess/src/openssl.cpp @@ -32,6 +32,8 @@ #include <stout/os.hpp> #include <stout/strings.hpp> +#include <stout/stopwatch.hpp> +#include <stout/try.hpp> #ifdef __WINDOWS__ // OpenSSL on Windows requires this adapter module to be compiled as part of the @@ -42,6 +44,10 @@ #include <openssl/applink.c> #endif // __WINDOWS__ +// Smallest OpenSSL version number to which the `X509_VERIFY_PARAM_*()` +// family of functions was backported. (OpenSSL 1.0.2) +#define MIN_VERSION_X509_VERIFY_PARAM 0x10002000L + using std::map; using std::ostringstream; using std::string; @@ -137,6 +143,18 @@ Flags::Flags() "the documentation of your OpenSSL.", "auto"); + add(&Flags::hostname_validation_scheme, + "hostname_validation_scheme", + "Select the scheme used to perform hostname validation when" + " verifying certificates.\n" + "Possible values: 'legacy', 'openssl'\n" + "See `docs/ssl.md` for details on the individual algorithms.\n" +#if OPENSSL_VERSION_NUMBER < MIN_VERSION_X509_VERIFY_PARAM + "NOTE: The currently linked version of OpenSSL is too old to support" + " the 'openssl' scheme, version 1.0.2 or higher is required.\n" +#endif + , "legacy"); + // We purposely don't have a flag for SSLv2. We do this because most // systems have disabled SSLv2 at compilation due to having so many // security vulnerabilities. @@ -494,6 +512,16 @@ void reinitialize() "Failed SSL connections will be downgraded to a non-SSL socket"; } + // Print an additional warning if certificate verification is enabled while + // supporting downgrades, since this is most likely a misconfiguration. + if ((ssl_flags->require_cert || ssl_flags->verify_cert) && + ssl_flags->support_downgrade) { + LOG(WARNING) + << "TLS certificate verification was enabled by setting one of" + << " LIBPROCESS_SSL_VERIFY_CERT or LIBPROCESS_SSL_REQUIRE_CERT, but" + << " can be bypassed because TLS downgrades are enabled."; + } + // Now do some validation of the flags/environment variables. if (ssl_flags->key_file.isNone()) { EXIT(EXIT_FAILURE) @@ -516,7 +544,6 @@ void reinitialize() << "Set CA directory path with LIBPROCESS_SSL_CA_DIR=<dirpath>"; } - if (ssl_flags->require_cert) { LOG(INFO) << "Will require client certificates for incoming TLS " << "connections."; @@ -548,6 +575,22 @@ void reinitialize() << "peer certificate verification"; } + if (ssl_flags->hostname_validation_scheme != "legacy" && + ssl_flags->hostname_validation_scheme != "openssl") { + EXIT(EXIT_FAILURE) << "Unknown value for hostname_validation_scheme: " + << ssl_flags->hostname_validation_scheme; + } + + if (ssl_flags->hostname_validation_scheme == "openssl" && + OPENSSL_VERSION_NUMBER < MIN_VERSION_X509_VERIFY_PARAM) { + EXIT(EXIT_FAILURE) + << "The 'openssl' hostname validation scheme requires OpenSSL" + " version 1.0.2 or higher"; + } + + LOG(INFO) << "Using '" << ssl_flags->hostname_validation_scheme + << "' scheme for hostname validation"; + // Initialize OpenSSL if we've been asked to do verification of peer // certificates. if (ssl_flags->verify_cert) { @@ -607,30 +650,11 @@ void reinitialize() << "' and/or directory '" << ca_dir << "'"; } - // Set SSL peer verification callback. - int mode = SSL_VERIFY_NONE; - - // We need `SSL_VERIFY_PEER` in server mode, otherwise the server - // will not send a client certificate request. - // - // NOTE: This relies on the implication `ssl_flags->require_cert` => - // `ssl_flags->verify_cert`, since it changes behaviour for both - // server and client mode. - // - // NOTE: Don't worry too much about the logic here since all - // of the mode-setting code is going to be moved and simplified - // as part of this chain, just a few commits down the road. - if (ssl_flags->require_cert) { - mode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; - } - - SSL_CTX_set_verify(ctx, mode, &verify_callback); - SSL_CTX_set_verify_depth(ctx, ssl_flags->verification_depth); - } else { - SSL_CTX_set_verify(ctx, SSL_VERIFY_NONE, nullptr); } + SSL_CTX_set_verify(ctx, SSL_VERIFY_NONE, nullptr); + // Set certificate chain. if (SSL_CTX_use_certificate_chain_file( ctx, @@ -768,7 +792,45 @@ Try<Nothing> verify( return Error("Could not verify peer certificate"); } - if (!ssl_flags->verify_ipadd && hostname.isNone()) { + // When using the 'openssl' scheme, hostname validation was already + // performed during the TLS handshake so we don't have to do it again + // here. + // + // NOTE: When using the 'openssl' scheme, we technically dont need + // to call the `openssl::verify()` function *at all*. + if (ssl_flags->hostname_validation_scheme == "openssl") { + return Try<Nothing>(Nothing()); + } + + // For backwards compatibility, the 'libprocess' scheme will attempt to get + // the peer hostname using a reverse DNS lookup if connecting via IP address. + Option<std::string> peer_hostname = hostname; + if (!hostname.isSome() && ip.isSome()) { + VLOG(1) << "Doing rDNS lookup for 'libprocess' hostname validation"; + Stopwatch watch; + + watch.start(); + Try<string> lookup = net::getHostname(ip.get()); + watch.stop(); + + // Due to MESOS-9339, a slow reverse DNS lookup will cause + // serious issues as it blocks the event loop thread. + if (watch.elapsed() > Milliseconds(100)) { + LOG(WARNING) << "Reverse DNS lookup for '" << ip.get() << "'" + << " took " << watch.elapsed().ms() << "ms" + << ", slowness is problematic (see MESOS-9339)"; + } + + if (lookup.isError()) { + VLOG(2) << "Could not determine hostname of peer: " + << lookup.error(); + } else { + VLOG(2) << "Accepting from " << lookup.get(); + peer_hostname = lookup.get(); + } + } + + if (!ssl_flags->verify_ipadd && peer_hostname.isNone()) { X509_free(cert); return ssl_flags->require_cert ? Error("Cannot verify peer certificate: peer hostname unknown") @@ -777,7 +839,8 @@ Try<Nothing> verify( // From https://wiki.openssl.org/index.php/Hostname_validation. // Check the Subject Alternate Name extension (SAN). This is useful - // for certificates that serve multiple physical hosts. + // for certificates where multiple domains are served from the same + // physical host. STACK_OF(GENERAL_NAME)* san_names = reinterpret_cast<STACK_OF(GENERAL_NAME)*>(X509_get_ext_d2i( reinterpret_cast<X509*>(cert), @@ -794,7 +857,7 @@ Try<Nothing> verify( switch(current_name->type) { case GEN_DNS: { - if (hostname.isSome()) { + if (peer_hostname.isSome()) { // Current name is a DNS name, let's check it. const string dns_name = reinterpret_cast<char*>(ASN1_STRING_data( @@ -812,11 +875,11 @@ Try<Nothing> verify( VLOG(2) << "Matching dNSName(" << i << "): " << dns_name; // Compare expected hostname with the DNS name. - if (hostname.get() == dns_name) { + if (peer_hostname.get() == dns_name) { sk_GENERAL_NAME_pop_free(san_names, GENERAL_NAME_free); X509_free(cert); - VLOG(2) << "dNSName match found for " << hostname.get(); + VLOG(2) << "dNSName match found for " << peer_hostname.get(); return Nothing(); } @@ -855,7 +918,7 @@ Try<Nothing> verify( sk_GENERAL_NAME_pop_free(san_names, GENERAL_NAME_free); } - if (hostname.isSome()) { + if (peer_hostname.isSome()) { // If we still haven't verified the hostname, try doing it via // the certificate subject name. X509_NAME* name = X509_get_subject_name(cert); @@ -870,14 +933,14 @@ Try<Nothing> verify( sizeof(text)) > 0) { VLOG(2) << "Matching common name: " << text; - if (hostname.get() != text) { + if (peer_hostname.get() != text) { X509_free(cert); return Error( "Presented Certificate Name: " + stringify(text) + - " does not match peer hostname name: " + hostname.get()); + " does not match peer hostname name: " + peer_hostname.get()); } - VLOG(2) << "Common name match found for " << hostname.get(); + VLOG(2) << "Common name match found for " << peer_hostname.get(); X509_free(cert); return Nothing(); @@ -890,8 +953,8 @@ Try<Nothing> verify( std::vector<string> details; - if (hostname.isSome()) { - details.push_back("hostname " + hostname.get()); + if (peer_hostname.isSome()) { + details.push_back("hostname " + peer_hostname.get()); } if (ip.isSome()) { @@ -903,6 +966,82 @@ Try<Nothing> verify( strings::join(", ", details)); } + +// A callback to configure the `SSL` object before the connection is +// established. +Try<Nothing> configure_socket( + SSL* ssl, + openssl::Mode mode, + const Address& peer_address, + const Option<std::string>& peer_hostname) +{ + if (mode == Mode::CLIENT && ssl_flags->verify_cert) { + SSL_set_verify( + ssl, + SSL_VERIFY_PEER, + &verify_callback); + } + + if (mode == Mode::SERVER && ssl_flags->require_cert) { + SSL_set_verify( + ssl, + SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, + &verify_callback); + } + + if (ssl_flags->hostname_validation_scheme == "openssl") { +#if OPENSSL_VERSION_NUMBER < MIN_VERSION_X509_VERIFY_PARAM + // We should have already checked this during startup. + EXIT(EXIT_FAILURE) << + "The linked OpenSSL library does not support `X509_VERIFY_PARAM` for" + " hostname validation. OpenSSL >= 1.0.2 is required."; +#else + if (mode == openssl::Mode::SERVER) { + // We don't do client hostname validation, because the application layer + // should set the policy on which certificate fields are considered a + // valid proof of identity. + // + // TODO(bevers): Provide hooks to the application code to make these + // policy decisions, for example via a Mesos module. + return Nothing(); + } + + if (mode == openssl::Mode::CLIENT && !ssl_flags->verify_cert) { + return Nothing(); + } + + // Decide whether we want to verify the peer's IP or DNS name. + X509_VERIFY_PARAM *param = SSL_get0_param(ssl); + if (peer_hostname.isSome()) { + if (!X509_VERIFY_PARAM_set1_host(param, peer_hostname->c_str(), 0)) { + return Error("Could not enable x509 hostname check."); + } + } else { + if (!ssl_flags->verify_ipadd) { + return Error("No DNS name given and IP address verification is " + " disabled. I cannot work like this :("); + } + + if (peer_address.family() != Address::Family::INET4 && + peer_address.family() != Address::Family::INET6) { + return Error("Can only use IPv4 or IPv6 addresses for IP address" + " validation."); + } + + Try<inet::Address> inetAddress = + network::convert<inet::Address>(peer_address); + + string ip = stringify(inetAddress->ip); + if (!X509_VERIFY_PARAM_set1_ip_asc(param, ip.c_str())) { + return Error("Could not enable x509 IP check."); + } + } +#endif + } + + return Nothing(); +} + } // namespace openssl { } // namespace network { } // namespace process { diff --git a/3rdparty/libprocess/src/openssl.hpp b/3rdparty/libprocess/src/openssl.hpp index e783e2b..d4ddbff 100644 --- a/3rdparty/libprocess/src/openssl.hpp +++ b/3rdparty/libprocess/src/openssl.hpp @@ -28,6 +28,8 @@ #include <stout/option.hpp> #include <stout/try.hpp> +#include <process/network.hpp> + namespace process { namespace network { namespace openssl { @@ -81,6 +83,14 @@ Try<Nothing> verify( const Option<std::string>& hostname = None(), const Option<net::IP>& ip = None()); +// Callback for setting SSL options after the TCP connection was +// established but before the TLS handshake has started. +Try<Nothing> configure_socket( + SSL* ssl, + Mode mode, + const Address& peer, + const Option<std::string>& peer_hostname); + } // namespace openssl { } // namespace network { } // namespace process { diff --git a/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp b/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp index 13aaa23..8f3d8d9 100644 --- a/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp +++ b/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp @@ -441,8 +441,13 @@ void LibeventSSLSocketImpl::event_callback(short events) // Do post-validation of connection. SSL* ssl = bufferevent_openssl_get_ssl(bev); + // We intentionally don't store the hostname passed to + // `connect()`: The 'openssl' hostname validation already + // verified the hostname if we get here, and the 'libprocess' + // algorithm should always use rDNS lookups on the IP address + // for backwards compatibility with the previous behaviour. Try<Nothing> verify = openssl::verify( - ssl, Mode::CLIENT, peer_hostname, peer_ip); + ssl, Mode::CLIENT, None(), peer_ip); if (verify.isError()) { VLOG(1) << "Failed connect, verification error: " << verify.error(); @@ -499,21 +504,19 @@ LibeventSSLSocketImpl::LibeventSSLSocketImpl(int_fd _s) LibeventSSLSocketImpl::LibeventSSLSocketImpl( int_fd _s, - bufferevent* _bev, - Option<string>&& _peer_hostname) + bufferevent* _bev) : SocketImpl(_s), bev(_bev), listener(nullptr), recv_request(nullptr), send_request(nullptr), connect_request(nullptr), - event_loop_handle(nullptr), - peer_hostname(std::move(_peer_hostname)) {} + event_loop_handle(nullptr) {} Future<Nothing> LibeventSSLSocketImpl::connect( const Address& address, - const Option<string>& peer_hostname_) + const Option<string>& peer_hostname) { if (bev != nullptr) { return Failure("Socket is already connected"); @@ -528,6 +531,13 @@ Future<Nothing> LibeventSSLSocketImpl::connect( return Failure("Failed to connect: SSL_new"); } + Try<Nothing> configured = openssl::configure_socket( + ssl, openssl::Mode::CLIENT, address, peer_hostname); + + if (configured.isError()) { + return Failure("Failed to configure socket: " + configured.error()); + } + // Construct the bufferevent in the connecting state. // We set 'BEV_OPT_DEFER_CALLBACKS' to avoid calling the // 'event_callback' before 'bufferevent_socket_connect' returns. @@ -548,22 +558,17 @@ Future<Nothing> LibeventSSLSocketImpl::connect( if (address.family() == Address::Family::INET4 || address.family() == Address::Family::INET6) { - // Try and determine the 'peer_hostname' from the address we're - // connecting to in order to properly verify the certificate - // later. - const Try<string> hostname = - network::convert<inet::Address>(address)->lookup_hostname(); - - if (hostname.isError()) { - VLOG(2) << "Could not determine hostname of peer: " << hostname.error(); - } else { - VLOG(2) << "Connecting to " << hostname.get(); - peer_hostname = hostname.get(); - } + inet::Address inetAddress = network::convert<inet::Address>(address).get(); // Determine the 'peer_ip' from the address we're connecting to in // order to properly verify the certificate later. - peer_ip = network::convert<inet::Address>(address)->ip; + peer_ip = inetAddress.ip; + } + + if (peer_hostname.isSome()) { + VLOG(2) << "Connecting to " << peer_hostname.get() << " at " << address; + } else { + VLOG(2) << "Connecting to " << address << " with no hostname specified"; } // Optimistically construct a 'ConnectRequest' and future. @@ -1119,6 +1124,25 @@ void LibeventSSLSocketImpl::accept_SSL_callback(AcceptRequest* request) return; } + Try<Address> peer_address = network::peer(request->socket); + if (!peer_address.isSome()) { + request->promise.fail("Could not determine peer IP for connection."); + delete request; + return; + } + + // NOTE: Right now, the configure callback does not do anything in server + // mode, but we still pass the correct peer address to enable modules to + // implement application-level logic in the future. + Try<Nothing> configured = openssl::configure_socket( + ssl, openssl::Mode::SERVER, peer_address.get(), None()); + + if (configured.isError()) { + request->promise.fail("Could not configure socket: " + configured.error()); + delete request; + return; + } + // We use 'request->listener' because 'this->listener' may not have // been set by the time this function is executed. See comment in // the lambda for evconnlistener_new in @@ -1156,41 +1180,11 @@ void LibeventSSLSocketImpl::accept_SSL_callback(AcceptRequest* request) if (events & BEV_EVENT_EOF) { request->promise.fail("Failed accept: connection closed"); } else if (events & BEV_EVENT_CONNECTED) { - // We will receive a 'CONNECTED' state on an accepting socket - // once the connection is established. Time to do - // post-verification. First, we need to determine the peer - // hostname. - Option<string> peer_hostname = None(); - - if (request->ip.isSome()) { - Stopwatch watch; - - watch.start(); - Try<string> hostname = net::getHostname(request->ip.get()); - watch.stop(); - - // Due to MESOS-9339, a slow reverse DNS lookup will cause - // serious issues as it blocks the event loop thread. - if (watch.elapsed() > Milliseconds(100)) { - LOG(WARNING) << "Reverse DNS lookup for '" << *request->ip << "'" - << " took " << watch.elapsed().ms() << "ms" - << ", slowness is problematic (see MESOS-9339)"; - } - - if (hostname.isError()) { - VLOG(2) << "Could not determine hostname of peer: " - << hostname.error(); - } else { - VLOG(2) << "Accepting from " << hostname.get(); - peer_hostname = hostname.get(); - } - } - SSL* ssl = bufferevent_openssl_get_ssl(bev); CHECK_NOTNULL(ssl); - Try<Nothing> verify = - openssl::verify(ssl, Mode::SERVER, peer_hostname, request->ip); + Try<Nothing> verify = openssl::verify( + ssl, Mode::SERVER, None(), request->ip); if (verify.isError()) { VLOG(1) << "Failed accept, verification error: " << verify.error(); @@ -1213,8 +1207,7 @@ void LibeventSSLSocketImpl::accept_SSL_callback(AcceptRequest* request) auto impl = std::shared_ptr<LibeventSSLSocketImpl>( new LibeventSSLSocketImpl( request->socket, - bev, - std::move(peer_hostname))); + bev)); // See comment at 'initialize' declaration for why we call // this. diff --git a/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp b/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp index ecb8a55..b781d6a 100644 --- a/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp +++ b/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp @@ -114,8 +114,7 @@ private: // functions. LibeventSSLSocketImpl( int_fd _s, - bufferevent* bev, - Option<std::string>&& peer_hostname); + bufferevent* bev); // This is called when the equivalent of 'accept' returns. The role // of this function is to set up the SSL object and bev. If we @@ -190,7 +189,6 @@ private: // discards through. Queue<Future<std::shared_ptr<SocketImpl>>> accept_queue; - Option<std::string> peer_hostname; Option<net::IP> peer_ip; };
