Support for password-encrypted SSL private keys (#1782)

chayim · web-flow · commit 18c6809b761b · 2021-12-16T09:36:56.000+02:00
Adding support for SSL private keys with a password.  This PR also adds support for future SSL tests.
diff --git a/.github/workflows/install_and_test.sh b/.github/workflows/install_and_test.sh
@@ -42,4 +42,4 @@ pip install ${PKG}
 pytest -m 'not onlycluster'
 # RedisCluster tests
 CLUSTER_URL="redis://localhost:16379/0"
-pytest -m 'not onlynoncluster and not redismod' --redis-url=${CLUSTER_URL}
+pytest -m 'not onlynoncluster and not redismod and not ssl' --redis-url=${CLUSTER_URL}
diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml
@@ -48,6 +48,7 @@ jobs:
        - name: run tests
          run: |
            pip install -r dev_requirements.txt
+           bash docker/stunnel/create_certs.sh
            tox -e ${{matrix.test-type}}-${{matrix.connection-type}}
        - name: Upload codecov coverage
          uses: codecov/codecov-action@v2
diff --git a/.gitignore b/.gitignore
@@ -17,3 +17,4 @@ coverage.xml
 .venv
 *.xml
 .coverage*
+docker/stunnel/keys
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -58,6 +58,7 @@ can execute docker and its various commands.
 -   A Redis replica node
 -   Three sentinel Redis nodes
 -   A multi-python docker, with your source code mounted in /data
+-   An stunnel docker, fronting the master Redis node
 
 The replica node, is a replica of the master node, using the
 [leader-follower replication](https://redis.io/topics/replication)
@@ -73,7 +74,7 @@ tests as well. With the 'tests' and 'all-tests' targets, all Redis and
 RedisCluster tests will be run. 
 
 It is possible to run only Redis client tests (with cluster mode disabled) by 
-using `invoke redis-tests`; similarly, RedisCluster tests can be run by using 
+using `invoke standalone-tests`; similarly, RedisCluster tests can be run by using 
 `invoke cluster-tests`.
 
 Each run of tox starts and stops the various dockers required. Sometimes
diff --git a/dev_requirements.txt b/dev_requirements.txt
@@ -6,6 +6,7 @@ pytest==6.2.5
 pytest-timeout==2.0.1
 tox==3.24.4
 tox-docker==3.1.0
+tox-run-before==0.1
 invoke==1.6.0
 pytest-cov>=3.0.0
 vulture>=2.3.0
diff --git a/docker/base/Dockerfile b/docker/base/Dockerfile
@@ -1,3 +1,4 @@
+# produces redisfab/redis-py:6.2.6
 FROM redis:6.2.6-buster
 
 CMD ["redis-server", "/redis.conf"]
diff --git a/docker/base/Dockerfile.cluster b/docker/base/Dockerfile.cluster
@@ -1,8 +1,9 @@
+# produces redisfab/redis-py-cluster:6.2.6
 FROM redis:6.2.6-buster
 
 COPY create_cluster.sh /create_cluster.sh
 RUN chmod +x /create_cluster.sh
 
 EXPOSE 16379 16380 16381 16382 16383 16384
 
-CMD [ "/create_cluster.sh"]
+CMD [ "/create_cluster.sh"]
diff --git a/docker/base/Dockerfile.sentinel b/docker/base/Dockerfile.sentinel
@@ -1,3 +1,4 @@
+# produces redisfab/redis-py-sentinel:6.2.6
 FROM redis:6.2.6-buster
 
 CMD ["redis-sentinel", "/sentinel.conf"]
diff --git a/docker/base/Dockerfile.stunnel b/docker/base/Dockerfile.stunnel
@@ -0,0 +1,11 @@
+# produces redisfab/stunnel:latest
+FROM ubuntu:18.04
+
+RUN apt-get update -qq --fix-missing
+RUN apt-get upgrade -qqy
+RUN apt install -qqy stunnel
+RUN mkdir -p /etc/stunnel/conf.d
+RUN echo "foreground = yes\ninclude = /etc/stunnel/conf.d" > /etc/stunnel/stunnel.conf
+RUN chown -R root:root /etc/stunnel/
+
+CMD ["/usr/bin/stunnel"]
diff --git a/docker/stunnel/conf/redis.conf b/docker/stunnel/conf/redis.conf
@@ -0,0 +1,6 @@
+[redis]
+accept = 6666
+connect = master:6379
+cert = /etc/stunnel/keys/server-cert.pem
+key = /etc/stunnel/keys/server-key.pem
+verify = 0
diff --git a/docker/stunnel/create_certs.sh b/docker/stunnel/create_certs.sh
@@ -0,0 +1,46 @@
+#!/bin/bash
+
+set -e
+
+DESTDIR=`dirname "$0"`/keys
+test -d ${DESTDIR} || mkdir ${DESTDIR}
+cd ${DESTDIR}
+
+SSL_SUBJECT="/C=CA/ST=Winnipeg/L=Manitoba/O=Some Corp/OU=IT Department/CN=example.com"
+which openssl &>/dev/null
+if [ $? -ne 0 ]; then
+	echo "No openssl binary present, exiting."
+	exit 1
+fi
+
+openssl genrsa -out ca-key.pem 2048 &>/dev/null
+
+openssl req -new -x509 -nodes -days 365000 \
+   -key ca-key.pem \
+   -out ca-cert.pem \
+   -subj "${SSL_SUBJECT}" &>/dev/null
+
+openssl req -newkey rsa:2048 -nodes -days 365000 \
+   -keyout server-key.pem \
+   -out server-req.pem \
+   -subj "${SSL_SUBJECT}" &>/dev/null
+
+openssl x509 -req -days 365000 -set_serial 01 \
+   -in server-req.pem \
+   -out server-cert.pem \
+   -CA ca-cert.pem \
+   -CAkey ca-key.pem &>/dev/null
+
+openssl req -newkey rsa:2048 -nodes -days 365000 \
+   -keyout client-key.pem \
+   -out client-req.pem \
+   -subj "${SSL_SUBJECT}" &>/dev/null
+
+openssl x509 -req -days 365000 -set_serial 01 \
+   -in client-req.pem \
+   -out client-cert.pem \
+   -CA ca-cert.pem \
+   -CAkey ca-key.pem &>/dev/null
+
+echo "Keys generated in ${DESTDIR}:"
+ls
diff --git a/redis/client.py b/redis/client.py
@@ -873,7 +873,9 @@ def __init__(
         ssl_certfile=None,
         ssl_cert_reqs="required",
         ssl_ca_certs=None,
+        ssl_ca_path=None,
         ssl_check_hostname=False,
+        ssl_password=None,
         max_connections=None,
         single_connection_client=False,
         health_check_interval=0,
@@ -947,6 +949,7 @@ def __init__(
                             "ssl_cert_reqs": ssl_cert_reqs,
                             "ssl_ca_certs": ssl_ca_certs,
                             "ssl_check_hostname": ssl_check_hostname,
+                            "ssl_password": ssl_password,
                         }
                     )
             connection_pool = ConnectionPool(**kwargs)
diff --git a/redis/connection.py b/redis/connection.py
@@ -884,15 +884,36 @@ def pack_commands(self, commands):
 
 
 class SSLConnection(Connection):
+    """Manages SSL connections to and from the Redis server(s).
+    This class extends the Connection class, adding SSL functionality, and making
+    use of ssl.SSLContext (https://docs.python.org/3/library/ssl.html#ssl.SSLContext)
+    """  # noqa
+
     def __init__(
         self,
         ssl_keyfile=None,
         ssl_certfile=None,
         ssl_cert_reqs="required",
         ssl_ca_certs=None,
         ssl_check_hostname=False,
+        ssl_ca_path=None,
+        ssl_password=None,
         **kwargs,
     ):
+        """Constructor
+
+        Args:
+            ssl_keyfile: Path to an ssl private key. Defaults to None.
+            ssl_certfile: Path to an ssl certificate. Defaults to None.
+            ssl_cert_reqs: The string value for the SSLContext.verify_mode (none, optional, required). Defaults to "required".
+            ssl_ca_certs: The path to a file of concatenated CA certificates in PEM format. Defaults to None.
+            ssl_check_hostname: If set, match the hostname during the SSL handshake. Defaults to False.
+            ssl_ca_path: The path to a directory containing several CA certificates in PEM format. Defaults to None.
+            ssl_password: Password for unlocking an encrypted private key. Defaults to None.
+
+        Raises:
+            RedisError
+        """  # noqa
         if not ssl_available:
             raise RedisError("Python wasn't built with SSL support")
 
@@ -915,18 +936,24 @@ def __init__(
             ssl_cert_reqs = CERT_REQS[ssl_cert_reqs]
         self.cert_reqs = ssl_cert_reqs
         self.ca_certs = ssl_ca_certs
+        self.ca_path = ssl_ca_path
         self.check_hostname = ssl_check_hostname
+        self.certificate_password = ssl_password
 
     def _connect(self):
         "Wrap the socket with SSL support"
         sock = super()._connect()
         context = ssl.create_default_context()
         context.check_hostname = self.check_hostname
         context.verify_mode = self.cert_reqs
-        if self.certfile and self.keyfile:
-            context.load_cert_chain(certfile=self.certfile, keyfile=self.keyfile)
-        if self.ca_certs:
-            context.load_verify_locations(self.ca_certs)
+        if self.certfile or self.keyfile:
+            context.load_cert_chain(
+                certfile=self.certfile,
+                keyfile=self.keyfile,
+                password=self.certificate_password,
+            )
+        if self.ca_certs is not None or self.ca_path is not None:
+            context.load_verify_locations(cafile=self.ca_certs, capath=self.ca_path)
         return context.wrap_socket(sock, server_hostname=self.host)
 
 
diff --git a/tasks.py b/tasks.py
@@ -3,6 +3,11 @@
 
 from invoke import run, task
 
+
+def _generate_keys():
+    run("bash docker/stunnel/create_certs.sh")
+
+
 with open("tox.ini") as fp:
     lines = fp.read().split("\n")
     dockers = [line.split("=")[1].strip() for line in lines if line.find("name") != -1]
@@ -14,6 +19,7 @@ def devenv(c):
     specified in the tox.ini file.
     """
     clean(c)
+    _generate_keys()
     cmd = "tox -e devenv"
     for d in dockers:
         cmd += f" --docker-dont-stop={d}"
@@ -29,6 +35,7 @@ def build_docs(c):
 @task
 def linters(c):
     """Run code linters"""
+    _generate_keys()
     run("tox -e linters")
 
 
@@ -37,6 +44,7 @@ def all_tests(c):
     """Run all linters, and tests in redis-py. This assumes you have all
     the python versions specified in the tox.ini file.
     """
+    _generate_keys()
     linters(c)
     tests(c)
 
@@ -47,6 +55,7 @@ def tests(c):
     with and without hiredis.
     """
     print("Starting Redis tests")
+    _generate_keys()
     run("tox -e '{standalone,cluster}'-'{plain,hiredis}'")
 
 
@@ -55,6 +64,7 @@ def standalone_tests(c):
     """Run all Redis tests against the current python,
     with and without hiredis."""
     print("Starting Redis tests")
+    _generate_keys()
     run("tox -e standalone-'{plain,hiredis}'")
 
 
@@ -63,6 +73,7 @@ def cluster_tests(c):
     """Run all Redis Cluster tests against the current python,
     with and without hiredis."""
     print("Starting RedisCluster tests")
+    _generate_keys()
     run("tox -e cluster-'{plain,hiredis}'")
 
 
@@ -74,6 +85,8 @@ def clean(c):
     if os.path.isdir("dist"):
         shutil.rmtree("dist")
     run(f"docker rm -f {' '.join(dockers)}")
+    if os.path.isdir("docker/stunnel/keys"):
+        shutil.rmtree("docker/stunnel/keys")
 
 
 @task
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -14,8 +14,10 @@
 
 REDIS_INFO = {}
 default_redis_url = "redis://localhost:6379/9"
-
 default_redismod_url = "redis://localhost:36379"
+
+# default ssl client ignores verification for the purpose of testing
+default_redis_ssl_url = "rediss://localhost:6666"
 default_cluster_nodes = 6
 
 
@@ -36,6 +38,13 @@ def pytest_addoption(parser):
         " defaults to `%(default)s`",
     )
 
+    parser.addoption(
+        "--redis-ssl-url",
+        default=default_redis_ssl_url,
+        action="store",
+        help="Redis SSL connection string," " defaults to `%(default)s`",
+    )
+
     parser.addoption(
         "--redis-cluster-nodes",
         default=default_cluster_nodes,
@@ -248,6 +257,12 @@ def r2(request):
         yield client
 
 
+@pytest.fixture()
+def sslclient(request):
+    with _get_client(redis.Redis, request, ssl=True) as client:
+        yield client
+
+
 def _gen_cluster_mock_resp(r, response):
     connection = Mock()
     connection.retry = Retry(NoBackoff(), 0)
diff --git a/tests/test_ssl.py b/tests/test_ssl.py
@@ -0,0 +1,61 @@
+import os
+from urllib.parse import urlparse
+
+import pytest
+
+import redis
+from redis.exceptions import ConnectionError
+
+
+@pytest.mark.ssl
+class TestSSL:
+    """Tests for SSL connections
+
+    This relies on the --redis-ssl-url purely for rebuilding the client
+    and connecting to the appropriate port.
+    """
+
+    ROOT = os.path.join(os.path.dirname(__file__), "..")
+    CERT_DIR = os.path.abspath(os.path.join(ROOT, "docker", "stunnel", "keys"))
+    if not os.path.isdir(CERT_DIR):  # github actions package validation case
+        CERT_DIR = os.path.abspath(
+            os.path.join(ROOT, "..", "docker", "stunnel", "keys")
+        )
+        if not os.path.isdir(CERT_DIR):
+            raise IOError(f"No SSL certificates found. They should be in {CERT_DIR}")
+
+    def test_ssl_with_invalid_cert(self, request):
+        ssl_url = request.config.option.redis_ssl_url
+        sslclient = redis.from_url(ssl_url)
+        with pytest.raises(ConnectionError) as e:
+            sslclient.ping()
+            assert "SSL: CERTIFICATE_VERIFY_FAILED" in str(e)
+
+    def test_ssl_connection(self, request):
+        ssl_url = request.config.option.redis_ssl_url
+        p = urlparse(ssl_url)[1].split(":")
+        r = redis.Redis(host=p[0], port=p[1], ssl=True, ssl_cert_reqs="none")
+        assert r.ping()
+
+    def test_ssl_connection_without_ssl(self, request):
+        ssl_url = request.config.option.redis_ssl_url
+        p = urlparse(ssl_url)[1].split(":")
+        r = redis.Redis(host=p[0], port=p[1], ssl=False)
+
+        with pytest.raises(ConnectionError) as e:
+            r.ping()
+            assert "Connection closed by server" in str(e)
+
+    def test_validating_self_signed_certificate(self, request):
+        ssl_url = request.config.option.redis_ssl_url
+        p = urlparse(ssl_url)[1].split(":")
+        r = redis.Redis(
+            host=p[0],
+            port=p[1],
+            ssl=True,
+            ssl_certfile=os.path.join(self.CERT_DIR, "server-cert.pem"),
+            ssl_keyfile=os.path.join(self.CERT_DIR, "server-key.pem"),
+            ssl_cert_reqs="required",
+            ssl_ca_certs=os.path.join(self.CERT_DIR, "server-cert.pem"),
+        )
+        assert r.ping()
diff --git a/tox.ini b/tox.ini

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+# produces redisfab/redis-py:6.2.6`
`1`	`2`	`FROM redis:6.2.6-buster`
`2`	`3`
`3`	`4`	`CMD ["redis-server", "/redis.conf"]`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+# produces redisfab/redis-py-sentinel:6.2.6`
`1`	`2`	`FROM redis:6.2.6-buster`
`2`	`3`
`3`	`4`	`CMD ["redis-sentinel", "/sentinel.conf"]`