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