You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by be...@apache.org on 2019/07/05 12:08:30 UTC
[mesos] 01/12: Changed semantics of TLS certificate verification
flags.
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 bd359291cd0d36a3f42225c910d40cbefb037d4b
Author: Benno Evers <be...@mesosphere.com>
AuthorDate: Mon May 27 17:39:38 2019 +0200
Changed semantics of TLS certificate verification flags.
This commit slightly updates the semants of the
`LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
environment variables. The former now only applies to connections
in client mode and the latter now only applies to connections in
server mode.
In particular, in TLS server mode we now *only* verify client
certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.
In addtion, when in SSL client mode and `LIBPROCESS_SSL_VERIFY_CERT`
has been set to `true`, enforce that the server actually presents a
certificate that can be verified. Note that this is expected to be
not a behavioural change in practice, since the TLS specification
already states that a server MUST always send a certificate unless an
anonymous cipher is used, and most TLS ciphersuites are configured to
exclude anonymous ciphers.
Review: https://reviews.apache.org/r/70748
---
3rdparty/libprocess/src/openssl.cpp | 64 +++++++++++++++-------
3rdparty/libprocess/src/openssl.hpp | 10 ++++
.../src/posix/libevent/libevent_ssl_socket.cpp | 8 ++-
3rdparty/libprocess/src/tests/ssl_tests.cpp | 35 +++++++++++-
4 files changed, 92 insertions(+), 25 deletions(-)
diff --git a/3rdparty/libprocess/src/openssl.cpp b/3rdparty/libprocess/src/openssl.cpp
index 19d25a8..240f468 100644
--- a/3rdparty/libprocess/src/openssl.cpp
+++ b/3rdparty/libprocess/src/openssl.cpp
@@ -87,13 +87,14 @@ Flags::Flags()
add(&Flags::verify_cert,
"verify_cert",
- "Whether or not to verify peer certificates.",
+ "Whether or not to require and verify server certificates for "
+ "connections in client mode.",
false);
add(&Flags::require_cert,
"require_cert",
- "Whether or not to require peer certificates. Requiring a peer "
- "certificate implies verifying it.",
+ "Whether or not to require and verify client certificates for "
+ "connections in server mode.",
false);
add(&Flags::verify_ipadd,
@@ -517,15 +518,8 @@ void reinitialize()
if (ssl_flags->require_cert) {
- LOG(INFO) << "Will require peer certificates for all TLS connections.";
- } else if (ssl_flags->verify_cert) {
- LOG(INFO) << "Will only verify peer certificate if presented!\n"
- << "NOTE: Set LIBPROCESS_SSL_REQUIRE_CERT=1 to require "
- << "peer certificate verification";
- } else {
- LOG(INFO) << "Will not verify peer certificate!\n"
- << "NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable "
- << "peer certificate verification";
+ LOG(INFO) << "Will require client certificates for incoming TLS "
+ << "connections.";
}
if (ssl_flags->verify_ipadd) {
@@ -534,14 +528,26 @@ void reinitialize()
}
if (ssl_flags->require_cert && !ssl_flags->verify_cert) {
- // Requiring a certificate implies that is should be verified.
+ // For backwards compatibility, `require_cert` implies `verify_cert`.
+ //
+ // NOTE: Even without backwards compatility considerations, this would
+ // be a reasonable requirement on the configuration.
ssl_flags->verify_cert = true;
LOG(INFO) << "LIBPROCESS_SSL_REQUIRE_CERT implies "
- << "peer certificate verification.\n"
+ << "server certificate verification.\n"
<< "LIBPROCESS_SSL_VERIFY_CERT set to true";
}
+ if (ssl_flags->verify_cert) {
+ LOG(INFO) << "Will verify server certificates for outgoing TLS "
+ << "connections.";
+ } else {
+ LOG(INFO) << "Will not verify server certificates!\n"
+ << "NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable "
+ << "peer certificate verification";
+ }
+
// Initialize OpenSSL if we've been asked to do verification of peer
// certificates.
if (ssl_flags->verify_cert) {
@@ -602,10 +608,20 @@ void reinitialize()
}
// Set SSL peer verification callback.
- int mode = SSL_VERIFY_PEER;
-
+ 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_FAIL_IF_NO_PEER_CERT;
+ mode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
}
SSL_CTX_set_verify(ctx, mode, &verify_callback);
@@ -723,11 +739,16 @@ SSL_CTX* context()
Try<Nothing> verify(
const SSL* const ssl,
+ Mode mode,
const Option<string>& hostname,
const Option<net::IP>& ip)
{
// Return early if we don't need to verify.
- if (!ssl_flags->verify_cert) {
+ if (mode == Mode::CLIENT && !ssl_flags->verify_cert) {
+ return Nothing();
+ }
+
+ if (mode == Mode::SERVER && !ssl_flags->require_cert) {
return Nothing();
}
@@ -735,10 +756,11 @@ Try<Nothing> verify(
// TODO(jmlvanre): handle this better. How about RAII?
X509* cert = SSL_get_peer_certificate(ssl);
+ // NOTE: Even without this check, the OpenSSL handshake will not complete
+ // when connecting to servers that do not present a certificate, unless an
+ // anonymous cipher is used.
if (cert == nullptr) {
- return ssl_flags->require_cert
- ? Error("Peer did not provide certificate")
- : Try<Nothing>(Nothing());
+ return Error("Peer did not provide certificate");
}
if (SSL_get_verify_result(ssl) != X509_V_OK) {
diff --git a/3rdparty/libprocess/src/openssl.hpp b/3rdparty/libprocess/src/openssl.hpp
index 17bec24..e783e2b 100644
--- a/3rdparty/libprocess/src/openssl.hpp
+++ b/3rdparty/libprocess/src/openssl.hpp
@@ -64,10 +64,20 @@ void initialize();
// Returns the _global_ OpenSSL context.
SSL_CTX* context();
+// An enum to track whether a given SSL object is in client or server mode.
+//
+// TODO(bevers): Once the minimum supported OpenSSL version is at least 1.1.1,
+// we can remove this enum and use the `SSL_is_server(ssl)` function instead.
+enum class Mode {
+ CLIENT,
+ SERVER,
+};
+
// Verify that the hostname is properly associated with the peer
// certificate associated with the specified SSL connection.
Try<Nothing> verify(
const SSL* const ssl,
+ Mode mode,
const Option<std::string>& hostname = None(),
const Option<net::IP>& ip = None());
diff --git a/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp b/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
index 7e2229a..1921d0e 100644
--- a/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
+++ b/3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
@@ -71,6 +71,8 @@
using std::queue;
using std::string;
+using process::network::openssl::Mode;
+
// Specialization of 'synchronize' to use bufferevent with the
// 'synchronized' macro.
static Synchronized<bufferevent> synchronize(bufferevent* bev)
@@ -439,7 +441,9 @@ void LibeventSSLSocketImpl::event_callback(short events)
// Do post-validation of connection.
SSL* ssl = bufferevent_openssl_get_ssl(bev);
- Try<Nothing> verify = openssl::verify(ssl, peer_hostname, peer_ip);
+ Try<Nothing> verify = openssl::verify(
+ ssl, Mode::CLIENT, peer_hostname, peer_ip);
+
if (verify.isError()) {
VLOG(1) << "Failed connect, verification error: " << verify.error();
SSL_free(ssl);
@@ -1184,7 +1188,7 @@ void LibeventSSLSocketImpl::accept_SSL_callback(AcceptRequest* request)
CHECK_NOTNULL(ssl);
Try<Nothing> verify =
- openssl::verify(ssl, peer_hostname, request->ip);
+ openssl::verify(ssl, Mode::SERVER, peer_hostname, request->ip);
if (verify.isError()) {
VLOG(1) << "Failed accept, verification error: " << verify.error();
diff --git a/3rdparty/libprocess/src/tests/ssl_tests.cpp b/3rdparty/libprocess/src/tests/ssl_tests.cpp
index 5d36022..e52451e 100644
--- a/3rdparty/libprocess/src/tests/ssl_tests.cpp
+++ b/3rdparty/libprocess/src/tests/ssl_tests.cpp
@@ -265,14 +265,45 @@ TEST_F(SSLTest, VerifyBadCA)
{"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_REQUIRE_CERT", "true"}});
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"}},
+ server.get(),
+ true);
+ ASSERT_SOME(client);
+
+ Future<Socket> socket = server->accept();
+ AWAIT_ASSERT_FAILED(socket);
+
+ AWAIT_ASSERT_READY(await_subprocess(client.get(), None()));
+}
+
+
+// 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);