From e3020e43344ddbc32e62e06bbbf88a4f5d7cdc82 Mon Sep 17 00:00:00 2001 From: Mohit Agrawal Date: Fri, 10 May 2019 11:13:45 +0530 Subject: [PATCH 141/141] socket/ssl: fix crl handling Problem: Just setting the path to the CRL directory in socket_init() wasn't working. Solution: Need to use special API to retrieve and set X509_VERIFY_PARAM and set the CRL checking flags explicitly. Also, setting the CRL checking flags is a big pain, since the connection is declared as failed if any CRL isn't found in the designated file or directory. A comment has been added to the code appropriately. > Change-Id: I8a8ed2ddaf4b5eb974387d2f7b1a85c1ca39fe79 > fixes: bz#1687326 > Signed-off-by: Milind Changire > (Cherry pick from commit 06fa261207f0f0625c52fa977b96e5875e9a91e0) > (Reviewed on upstream link https://review.gluster.org/#/c/glusterfs/+/22334/) Change-Id: I0958e9890035fd376f1e1eafc1452caf3edd184b BUG: 1583585 Signed-off-by: Mohit Agrawal Reviewed-on: https://code.engineering.redhat.com/gerrit/166458 Tested-by: RHGS Build Bot Reviewed-by: Sunil Kumar Heggodu Gopala Acharya --- configure.ac | 2 + rpc/rpc-transport/socket/src/socket.c | 110 ++++++++++++++++++++++++++++------ rpc/rpc-transport/socket/src/socket.h | 2 + tests/features/ssl-ciphers.t | 13 +++- 4 files changed, 107 insertions(+), 20 deletions(-) diff --git a/configure.ac b/configure.ac index 3065077..0e11d4c 100644 --- a/configure.ac +++ b/configure.ac @@ -491,6 +491,8 @@ AC_CHECK_HEADERS([openssl/dh.h]) AC_CHECK_HEADERS([openssl/ecdh.h]) +AC_CHECK_LIB([ssl], [SSL_CTX_get0_param], [AC_DEFINE([HAVE_SSL_CTX_GET0_PARAM], [1], [define if found OpenSSL SSL_CTX_get0_param])]) + dnl Math library AC_CHECK_LIB([m], [pow], [MATH_LIB='-lm'], [MATH_LIB='']) AC_SUBST(MATH_LIB) diff --git a/rpc/rpc-transport/socket/src/socket.c b/rpc/rpc-transport/socket/src/socket.c index f6de1d3..bf2fa71 100644 --- a/rpc/rpc-transport/socket/src/socket.c +++ b/rpc/rpc-transport/socket/src/socket.c @@ -308,8 +308,65 @@ out: #define ssl_write_one(t, b, l) \ ssl_do((t), (b), (l), (SSL_trinary_func *)SSL_write) +/* set crl verify flags only for server */ +/* see man X509_VERIFY_PARAM_SET_FLAGS(3) + * X509_V_FLAG_CRL_CHECK enables CRL checking for the certificate chain + * leaf certificate. An error occurs if a suitable CRL cannot be found. + * Since we're never going to revoke a gluster node cert, we better disable + * CRL check for server certs to avoid getting error and failed connection + * attempts. + */ +static void +ssl_clear_crl_verify_flags(SSL_CTX *ssl_ctx) +{ +#ifdef X509_V_FLAG_CRL_CHECK_ALL +#ifdef HAVE_SSL_CTX_GET0_PARAM + X509_VERIFY_PARAM *vpm; + + vpm = SSL_CTX_get0_param(ssl_ctx); + if (vpm) { + X509_VERIFY_PARAM_clear_flags( + vpm, (X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL)); + } +#else + /* CRL verify flag need not be cleared for rhel6 kind of clients */ +#endif +#else + gf_log(this->name, GF_LOG_ERROR, "OpenSSL version does not support CRL"); +#endif + return; +} + +/* set crl verify flags only for server */ +static void +ssl_set_crl_verify_flags(SSL_CTX *ssl_ctx) +{ +#ifdef X509_V_FLAG_CRL_CHECK_ALL +#ifdef HAVE_SSL_CTX_GET0_PARAM + X509_VERIFY_PARAM *vpm; + + vpm = SSL_CTX_get0_param(ssl_ctx); + if (vpm) { + unsigned long flags; + + flags = X509_VERIFY_PARAM_get_flags(vpm); + flags |= (X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); + X509_VERIFY_PARAM_set_flags(vpm, flags); + } +#else + X509_STORE *x509store; + + x509store = SSL_CTX_get_cert_store(ssl_ctx); + X509_STORE_set_flags(x509store, + X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); +#endif +#else + gf_log(this->name, GF_LOG_ERROR, "OpenSSL version does not support CRL"); +#endif +} + int -ssl_setup_connection_prefix(rpc_transport_t *this) +ssl_setup_connection_prefix(rpc_transport_t *this, gf_boolean_t server) { int ret = -1; socket_private_t *priv = NULL; @@ -332,6 +389,9 @@ ssl_setup_connection_prefix(rpc_transport_t *this) priv->ssl_accepted = _gf_false; priv->ssl_context_created = _gf_false; + if (!server && priv->crl_path) + ssl_clear_crl_verify_flags(priv->ssl_ctx); + priv->ssl_ssl = SSL_new(priv->ssl_ctx); if (!priv->ssl_ssl) { gf_log(this->name, GF_LOG_ERROR, "SSL_new failed"); @@ -2664,7 +2724,7 @@ ssl_handle_server_connection_attempt(rpc_transport_t *this) fd = priv->sock; if (!priv->ssl_context_created) { - ret = ssl_setup_connection_prefix(this); + ret = ssl_setup_connection_prefix(this, _gf_true); if (ret < 0) { gf_log(this->name, GF_LOG_TRACE, "> ssl_setup_connection_prefix() failed!"); @@ -2718,7 +2778,7 @@ ssl_handle_client_connection_attempt(rpc_transport_t *this) ret = -1; } else { if (!priv->ssl_context_created) { - ret = ssl_setup_connection_prefix(this); + ret = ssl_setup_connection_prefix(this, _gf_false); if (ret < 0) { gf_log(this->name, GF_LOG_TRACE, "> ssl_setup_connection_prefix() " @@ -3085,7 +3145,30 @@ socket_server_event_handler(int fd, int idx, int gen, void *data, int poll_in, gf_log(this->name, GF_LOG_TRACE, "XXX server:%s, client:%s", new_trans->myinfo.identifier, new_trans->peerinfo.identifier); + /* Make options available to local socket_init() to create new + * SSL_CTX per transport. A separate SSL_CTX per transport is + * required to avoid setting crl checking options for client + * connections. The verification options eventually get copied + * to the SSL object. Unfortunately, there's no way to identify + * whether socket_init() is being called after a client-side + * connect() or a server-side accept(). Although, we could pass + * a flag from the transport init() to the socket_init() and + * from this place, this doesn't identify the case where the + * server-side transport loading is done for the first time. + * Also, SSL doesn't apply for UNIX sockets. + */ + if (new_sockaddr.ss_family != AF_UNIX) + new_trans->options = dict_ref(this->options); + new_trans->ctx = this->ctx; + ret = socket_init(new_trans); + + /* reset options to NULL to avoid double free */ + if (new_sockaddr.ss_family != AF_UNIX) { + dict_unref(new_trans->options); + new_trans->options = NULL; + } + if (ret != 0) { gf_log(this->name, GF_LOG_WARNING, "initialization of new_trans " @@ -4150,7 +4233,6 @@ ssl_setup_connection_params(rpc_transport_t *this) char *cipher_list = DEFAULT_CIPHER_LIST; char *dh_param = DEFAULT_DH_PARAM; char *ec_curve = DEFAULT_EC_CURVE; - char *crl_path = NULL; priv = this->private; @@ -4192,6 +4274,7 @@ ssl_setup_connection_params(rpc_transport_t *this) } priv->ssl_ca_list = gf_strdup(priv->ssl_ca_list); + optstr = NULL; if (dict_get_str(this->options, SSL_CRL_PATH_OPT, &optstr) == 0) { if (!priv->ssl_enabled) { gf_log(this->name, GF_LOG_WARNING, @@ -4199,9 +4282,9 @@ ssl_setup_connection_params(rpc_transport_t *this) SSL_ENABLED_OPT); } if (strcasecmp(optstr, "NULL") == 0) - crl_path = NULL; + priv->crl_path = NULL; else - crl_path = optstr; + priv->crl_path = gf_strdup(optstr); } gf_log(this->name, priv->ssl_enabled ? GF_LOG_INFO : GF_LOG_DEBUG, @@ -4343,24 +4426,15 @@ ssl_setup_connection_params(rpc_transport_t *this) } if (!SSL_CTX_load_verify_locations(priv->ssl_ctx, priv->ssl_ca_list, - crl_path)) { + priv->crl_path)) { gf_log(this->name, GF_LOG_ERROR, "could not load CA list"); goto err; } SSL_CTX_set_verify_depth(priv->ssl_ctx, cert_depth); - if (crl_path) { -#ifdef X509_V_FLAG_CRL_CHECK_ALL - X509_STORE *x509store; - - x509store = SSL_CTX_get_cert_store(priv->ssl_ctx); - X509_STORE_set_flags( - x509store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); -#else - gf_log(this->name, GF_LOG_ERROR, - "OpenSSL version does not support CRL"); -#endif + if (priv->crl_path) { + ssl_set_crl_verify_flags(priv->ssl_ctx); } priv->ssl_session_id = session_id++; diff --git a/rpc/rpc-transport/socket/src/socket.h b/rpc/rpc-transport/socket/src/socket.h index e1ccae2..e7c0090 100644 --- a/rpc/rpc-transport/socket/src/socket.h +++ b/rpc/rpc-transport/socket/src/socket.h @@ -14,6 +14,7 @@ #include #include #include +#include #ifdef HAVE_OPENSSL_DH_H #include #endif @@ -246,6 +247,7 @@ typedef struct { char *ssl_own_cert; char *ssl_private_key; char *ssl_ca_list; + char *crl_path; int pipe[2]; struct gf_sock_incoming incoming; /* -1 = not connected. 0 = in progress. 1 = connected */ diff --git a/tests/features/ssl-ciphers.t b/tests/features/ssl-ciphers.t index 563d37c..7e1e199 100644 --- a/tests/features/ssl-ciphers.t +++ b/tests/features/ssl-ciphers.t @@ -175,8 +175,6 @@ BRICK_PORT=`brick_port $V0` EXPECT "Y" openssl_connect -cipher EECDH -connect $H0:$BRICK_PORT # test revocation -# no need to restart the volume since the options are used -# by the client here. TEST $CLI volume set $V0 ssl.crl-path $TMPDIR EXPECT $TMPDIR volume_option $V0 ssl.crl-path $GFS --volfile-id=$V0 --volfile-server=$H0 $M0 @@ -189,14 +187,25 @@ TEST openssl ca -batch -config $SSL_CFG -revoke $SSL_CERT 2>&1 TEST openssl ca -config $SSL_CFG -gencrl -out $SSL_CRL 2>&1 # Failed once revoked +# Although client fails to mount without restarting the server after crl-path +# is set when no actual crl file is found on the client, it would also fail +# when server is restarted for the same reason. Since the socket initialization +# code is the same for client and server, the crl verification flags need to +# be turned off for the client to avoid SSL searching for CRLs in the +# ssl.crl-path. If no CRL files are found in the ssl.crl-path, SSL fails the +# connect() attempt on the client. +TEST $CLI volume stop $V0 +TEST $CLI volume start $V0 $GFS --volfile-id=$V0 --volfile-server=$H0 $M0 EXPECT "N" wait_mount $M0 TEST ! test -f $TEST_FILE EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0 # Succeed with CRL disabled +TEST $CLI volume stop $V0 TEST $CLI volume set $V0 ssl.crl-path NULL EXPECT NULL volume_option $V0 ssl.crl-path +TEST $CLI volume start $V0 $GFS --volfile-id=$V0 --volfile-server=$H0 $M0 EXPECT "Y" wait_mount $M0 TEST test -f $TEST_FILE -- 1.8.3.1