From 68157f21fe8051ebd7eace11012738d8d91a1812 Mon Sep 17 00:00:00 2001 From: Tomas Jelinek Date: Tue, 2 Mar 2021 14:47:27 +0100 Subject: [PATCH 1/2] squash bz1927404: replace pyOpenSSL with python-cryptography python-cryptography requires new mypy improve TLS certificate verification Library methods are now used instead of running openssl processes. That enabled support for Elliptic Curve certificates. cleanup dependencies in spec file --- .gitlab-ci.yml | 6 +- README.md | 2 +- mypy.ini | 7 +- pcs.spec.in | 5 +- pcs/common/ssl.py | 108 +++++++++++++++++------------- pcs/daemon/ssl.py | 48 ++----------- pcs/pcsd.py | 19 +++--- pcs/utils.py | 32 --------- pcs_test/tier0/daemon/test_ssl.py | 64 +++++++++--------- pcsd/pcs.rb | 39 ++++------- pcsd/remote.rb | 2 +- requirements.txt | 2 +- test/centos8/Dockerfile | 2 +- test/fedora31/Dockerfile | 2 +- test/fedora32/Dockerfile | 2 +- 15 files changed, 135 insertions(+), 205 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4cb4c14b..72668787 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -46,12 +46,12 @@ pylint: script: - "dnf install -y python3 + python3-cryptography python3-dateutil python3-distro python3-lxml python3-pip python3-pycurl - python3-pyOpenSSL python3-pyparsing findutils make @@ -66,12 +66,12 @@ mypy: script: - "dnf install -y python3 + python3-cryptography python3-dateutil python3-distro python3-lxml python3-pip python3-pycurl - python3-pyOpenSSL python3-pyparsing git make @@ -111,12 +111,12 @@ python_tier0_tests: - "dnf install -y make python3 + python3-cryptography python3-dateutil python3-distro python3-lxml python3-pip python3-pycurl - python3-pyOpenSSL python3-pyparsing which " diff --git a/README.md b/README.md index fe6eeed6..a0c01c02 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ These are the runtime dependencies of pcs and pcsd: * python3-lxml * python3-pycurl * python3-setuptools -* python3-pyOpenSSL (python3-openssl) +* python3-cryptography * python3-pyparsing * python3-tornado 6.1.0+ * python dataclasses (`pip install dataclasses`; required only for python 3.6, diff --git a/mypy.ini b/mypy.ini index 6d3d2ff9..e3198530 100644 --- a/mypy.ini +++ b/mypy.ini @@ -48,6 +48,10 @@ disallow_untyped_defs = True # this is a temporary solution for legacy code disallow_untyped_defs = False +[mypy-pcs.common.ssl] +disallow_untyped_defs = True +disallow_untyped_calls = True + [mypy-pcs.common.types] disallow_untyped_defs = True disallow_untyped_calls = True @@ -122,9 +126,6 @@ ignore_missing_imports = True [mypy-distro] ignore_missing_imports = True -[mypy-OpenSSL] -ignore_missing_imports = True - [mypy-pyagentx.*] ignore_errors = True diff --git a/pcs.spec.in b/pcs.spec.in index db66c5b0..610fad50 100644 --- a/pcs.spec.in +++ b/pcs.spec.in @@ -139,6 +139,7 @@ BuildRequires: python3-setuptools_scm BuildRequires: python3-devel # for tier0 tests +BuildRequires: python3-cryptography BuildRequires: python3-pyparsing # gcc for compiling custom rubygems @@ -171,6 +172,7 @@ Requires: platform-python Requires: platform-python-setuptools %endif +Requires: python3-cryptography Requires: python3-lxml Requires: python3-pycurl Requires: python3-pyparsing @@ -190,9 +192,6 @@ Requires: ruby >= 2.2.0 Requires: rubygems # for killall Requires: psmisc -# for working with certificates (validation etc.) -Requires: openssl -Requires: python3-pyOpenSSL # cluster stack and related packages Requires: pacemaker >= 2.0.0 Requires: corosync >= 3.0 diff --git a/pcs/common/ssl.py b/pcs/common/ssl.py index 852fea80..74ddd4ec 100644 --- a/pcs/common/ssl.py +++ b/pcs/common/ssl.py @@ -1,45 +1,63 @@ -import time -from OpenSSL import crypto - - -def cert_date_format(timestamp): - return str.encode(time.strftime("%Y%m%d%H%M%SZ", time.gmtime(timestamp))) - - -def generate_key(length=3072): - key = crypto.PKey() - key.generate_key(crypto.TYPE_RSA, length) - return key - - -def generate_cert(key, server_name): - now = time.time() - cert = crypto.X509() - - subject = cert.get_subject() - subject.countryName = "US" - subject.stateOrProvinceName = "MN" - subject.localityName = "Minneapolis" - subject.organizationName = "pcsd" - subject.organizationalUnitName = "pcsd" - subject.commonName = server_name - - cert.set_version(2) - cert.set_serial_number(int(now * 1000)) - cert.set_notBefore(cert_date_format(now)) - cert.set_notAfter( - cert_date_format(now + 60 * 60 * 24 * 365 * 10) - ) # 10 years - cert.set_issuer(subject) - cert.set_pubkey(key) - cert.sign(key, "sha256") - - return cert - - -def dump_cert(certificate): - return crypto.dump_certificate(crypto.FILETYPE_PEM, certificate) - - -def dump_key(key): - return crypto.dump_privatekey(crypto.FILETYPE_PEM, key) +import datetime +import ssl +from typing import List + +from cryptography import x509 +from cryptography.x509.oid import NameOID +from cryptography.hazmat.backends import default_backend +from cryptography.hazmat.primitives import hashes, serialization +from cryptography.hazmat.primitives.asymmetric import rsa + + +def check_cert_key(cert_path: str, key_path: str) -> List[str]: + errors = [] + try: + ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + ssl_context.load_cert_chain(cert_path, key_path) + except ssl.SSLError as e: + errors.append(f"SSL certificate does not match the key: {e}") + except EnvironmentError as e: + errors.append(f"Unable to load SSL certificate and/or key: {e}") + return errors + + +def generate_key(length: int = 3072) -> rsa.RSAPrivateKeyWithSerialization: + return rsa.generate_private_key( + public_exponent=65537, key_size=length, backend=default_backend() + ) + + +def generate_cert(key: rsa.RSAPrivateKey, server_name: str) -> x509.Certificate: + now = datetime.datetime.utcnow() + subject = x509.Name( + [ + x509.NameAttribute(NameOID.COUNTRY_NAME, "US"), + x509.NameAttribute(NameOID.STATE_OR_PROVINCE_NAME, "MN"), + x509.NameAttribute(NameOID.LOCALITY_NAME, "Minneapolis"), + x509.NameAttribute(NameOID.ORGANIZATION_NAME, "pcsd"), + x509.NameAttribute(NameOID.ORGANIZATIONAL_UNIT_NAME, "pcsd"), + x509.NameAttribute(NameOID.COMMON_NAME, server_name), + ] + ) + return ( + x509.CertificateBuilder() + .subject_name(subject) + .issuer_name(subject) + .public_key(key.public_key()) + .serial_number(int(now.timestamp() * 1000)) + .not_valid_before(now) + .not_valid_after(now + datetime.timedelta(days=3650)) + .sign(key, hashes.SHA256(), default_backend()) + ) + + +def dump_cert(certificate: x509.Certificate) -> bytes: + return certificate.public_bytes(serialization.Encoding.PEM) + + +def dump_key(key: rsa.RSAPrivateKeyWithSerialization) -> bytes: + return key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.TraditionalOpenSSL, + serialization.NoEncryption(), + ) diff --git a/pcs/daemon/ssl.py b/pcs/daemon/ssl.py index 40cca314..43865631 100644 --- a/pcs/daemon/ssl.py +++ b/pcs/daemon/ssl.py @@ -1,9 +1,8 @@ import os import ssl -from OpenSSL import crypto, SSL - from pcs.common.ssl import ( + check_cert_key, dump_cert, dump_key, generate_cert, @@ -11,53 +10,15 @@ from pcs.common.ssl import ( ) -def check_cert_key(cert_path, key_path): - errors = [] - - def load(load_ssl_file, label, path): - try: - with open(path) as ssl_file: - return load_ssl_file(crypto.FILETYPE_PEM, ssl_file.read()) - except EnvironmentError as e: - errors.append(f"Unable to read SSL {label} '{path}': '{e}'") - except crypto.Error as e: - msg = "" - if e.args and e.args[0] and e.args[0][0]: - msg = f": '{':'.join(e.args[0][0])}'" - errors.append(f"Invalid SSL {label} '{path}'{msg}") - - cert = load(crypto.load_certificate, "certificate", cert_path) - key = load(crypto.load_privatekey, "key", key_path) - - if errors: - return errors - - try: - context = SSL.Context(SSL.TLSv1_METHOD) - context.use_privatekey(key) - context.use_certificate(cert) - except SSL.Error as e: - errors.append(f"Unable to load SSL certificate and/or key: {e}") - # If we cannot load the files, do not confuse users with other error - # messages. - return errors - try: - context.check_privatekey() - except (crypto.Error, SSL.Error) as e: - errors.append(f"SSL certificate does not match the key: {e}") - - return errors - - -def open_ssl_file_to_rewrite(path): +def _open_ssl_file_to_rewrite(path): return os.fdopen(os.open(path, os.O_CREAT | os.O_WRONLY, 0o600), "wb") def regenerate_cert_key(server_name, cert_path, key_path, key_length=None): key = generate_key(key_length) if key_length else generate_key() - with open_ssl_file_to_rewrite(cert_path) as cert_file: + with _open_ssl_file_to_rewrite(cert_path) as cert_file: cert_file.write(dump_cert(generate_cert(key, server_name))) - with open_ssl_file_to_rewrite(key_path) as key_file: + with _open_ssl_file_to_rewrite(key_path) as key_file: key_file.write(dump_key(key)) @@ -102,7 +63,6 @@ class PcsdSSL: self.__ck_pair = CertKeyPair(cert_location, key_location) def create_context(self) -> ssl.SSLContext: - # pylint: disable=no-member ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) ssl_context.set_ciphers(self.__ssl_ciphers) ssl_context.options = self.__ssl_options diff --git a/pcs/pcsd.py b/pcs/pcsd.py index f3e6bca3..d5ddb443 100644 --- a/pcs/pcsd.py +++ b/pcs/pcsd.py @@ -5,6 +5,7 @@ import sys from pcs import settings from pcs import utils from pcs.cli.common.errors import CmdLineInputError +import pcs.common.ssl def pcsd_certkey(lib, argv, modifiers): @@ -21,13 +22,13 @@ def pcsd_certkey(lib, argv, modifiers): keyfile = argv[1] try: - with open(certfile, "r") as myfile: + with open(certfile, "rb") as myfile: cert = myfile.read() - with open(keyfile, "r") as myfile: + with open(keyfile, "rb") as myfile: key = myfile.read() except IOError as e: utils.err(e) - errors = utils.verify_cert_key_pair(cert, key) + errors = pcs.common.ssl.check_cert_key(certfile, keyfile) if errors: for err in errors: utils.err(err, False) @@ -43,12 +44,12 @@ def pcsd_certkey(lib, argv, modifiers): try: try: - os.chmod(settings.pcsd_cert_location, 0o700) + os.chmod(settings.pcsd_cert_location, 0o600) except OSError: # If the file doesn't exist, we don't care pass try: - os.chmod(settings.pcsd_key_location, 0o700) + os.chmod(settings.pcsd_key_location, 0o600) except OSError: # If the file doesn't exist, we don't care pass @@ -56,9 +57,9 @@ def pcsd_certkey(lib, argv, modifiers): os.open( settings.pcsd_cert_location, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, - 0o700, + 0o600, ), - "w", + "wb", ) as myfile: myfile.write(cert) @@ -66,9 +67,9 @@ def pcsd_certkey(lib, argv, modifiers): os.open( settings.pcsd_key_location, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, - 0o700, + 0o600, ), - "w", + "wb", ) as myfile: myfile.write(key) diff --git a/pcs/utils.py b/pcs/utils.py index 97a04787..59d1b66e 100644 --- a/pcs/utils.py +++ b/pcs/utils.py @@ -2105,38 +2105,6 @@ def is_iso8601_date(var): return retVal == 0 -def verify_cert_key_pair(cert, key): - """ - Commandline options: no options - """ - errors = [] - cert_modulus = "" - key_modulus = "" - - output, retval = run( - ["/usr/bin/openssl", "x509", "-modulus", "-noout"], - string_for_stdin=cert, - ) - if retval != 0: - errors.append("Invalid certificate: {0}".format(output.strip())) - else: - cert_modulus = output.strip() - - output, retval = run( - ["/usr/bin/openssl", "rsa", "-modulus", "-noout"], string_for_stdin=key - ) - if retval != 0: - errors.append("Invalid key: {0}".format(output.strip())) - else: - key_modulus = output.strip() - - if not errors and cert_modulus and key_modulus: - if cert_modulus != key_modulus: - errors.append("Certificate does not match the key") - - return errors - - def err(errorText, exit_after_error=True): sys.stderr.write("Error: %s\n" % errorText) if exit_after_error: diff --git a/pcs_test/tier0/daemon/test_ssl.py b/pcs_test/tier0/daemon/test_ssl.py index e80f7a30..2b2edd36 100644 --- a/pcs_test/tier0/daemon/test_ssl.py +++ b/pcs_test/tier0/daemon/test_ssl.py @@ -1,8 +1,7 @@ import os +import ssl from unittest import mock, TestCase -from OpenSSL import SSL - from pcs_test.tools.misc import get_tmp_dir from pcs.daemon.ssl import PcsdSSL, CertKeyPair, SSLCertKeyException @@ -19,19 +18,6 @@ class SslFilesMixin: self.ssl_dir = get_tmp_dir("tier0_daemon_ssl") self.cert_path = os.path.join(self.ssl_dir.name, "daemon.cert") self.key_path = os.path.join(self.ssl_dir.name, "daemon.key") - # various versions of OpenSSL / PyOpenSSL emit different messages - self.DAMAGED_SSL_FILES_ERRORS_1 = ( - f"Invalid SSL certificate '{self.cert_path}':" - " 'PEM routines:PEM_read_bio:no start line'", - f"Invalid SSL key '{self.key_path}':" - " 'PEM routines:PEM_read_bio:no start line'", - ) - self.DAMAGED_SSL_FILES_ERRORS_2 = ( - f"Invalid SSL certificate '{self.cert_path}':" - " 'PEM routines:get_name:no start line'", - f"Invalid SSL key '{self.key_path}':" - " 'PEM routines:get_name:no start line'", - ) def tearDown(self): # pylint cannot possibly know this is being mixed into TestCase classes @@ -56,21 +42,31 @@ class Pair(SslFilesMixin, TestCase): def test_error_if_files_with_bad_content(self): self.damage_ssl_files() - self.assertTrue( - self.pair.check() - in [ - list(self.DAMAGED_SSL_FILES_ERRORS_1), - list(self.DAMAGED_SSL_FILES_ERRORS_2), - ] + errors = self.pair.check() + self.assertEqual(len(errors), 1) + self.assertRegex( + errors[0], + r"^SSL certificate does not match the key: " + r"\[SSL\] PEM lib \(_ssl\.c:\d+\)", ) - @mock.patch("pcs.daemon.ssl.SSL.Context.use_privatekey") - def test_error_if_short_key(self, mock_use_key): - mock_use_key.side_effect = SSL.Error("reason") + @mock.patch("pcs.daemon.ssl.ssl.SSLContext.load_cert_chain") + def test_error_if_short_key(self, mock_load_cert_chain): + mock_load_cert_chain.side_effect = ssl.SSLError( + # These are the real args of the exception. + 336245135, + "[SSL: EE_KEY_TOO_SMALL] ee key too small (_ssl.c:3542)", + ) + # 512 cannot be used as we would get an error from FIPS and 1024 is + # long enough. So a mock must be used. self.pair.regenerate(SERVER_NAME, 1024) errors = self.pair.check() self.assertEqual( - errors, ["Unable to load SSL certificate and/or key: reason"] + errors, + [ + "SSL certificate does not match the key: " + "[SSL: EE_KEY_TOO_SMALL] ee key too small (_ssl.c:3542)", + ], ) def test_error_if_cert_does_not_match_key(self): @@ -83,8 +79,10 @@ class Pair(SslFilesMixin, TestCase): errors = self.pair.check() self.assertEqual(len(errors), 1) - self.assertTrue( - errors[0].startswith("SSL certificate does not match the key:") + self.assertRegex( + errors[0], + r"SSL certificate does not match the key: " + r"\[X509: KEY_VALUES_MISMATCH\] key values mismatch \(_ssl\.c:\d+\)", ) @@ -102,12 +100,12 @@ class PcsdSSLTest(SslFilesMixin, TestCase): self.damage_ssl_files() with self.assertRaises(SSLCertKeyException) as ctx_manager: self.pcsd_ssl.guarantee_valid_certs() - self.assertTrue( - ctx_manager.exception.args - in [ - self.DAMAGED_SSL_FILES_ERRORS_1, - self.DAMAGED_SSL_FILES_ERRORS_2, - ] + errors = ctx_manager.exception.args + self.assertEqual(len(errors), 1) + self.assertRegex( + errors[0], + r"SSL certificate does not match the key: " + r"\[SSL\] PEM lib \(_ssl\.c:\d+\)", ) def test_context_uses_given_options(self): diff --git a/pcsd/pcs.rb b/pcsd/pcs.rb index bce8e39e..89c26f33 100644 --- a/pcsd/pcs.rb +++ b/pcsd/pcs.rb @@ -12,6 +12,7 @@ require 'fileutils' require 'backports/latest' require 'base64' require 'ethon' +require 'openssl' require 'config.rb' require 'cfgsync.rb' @@ -1170,39 +1171,23 @@ def read_file_lock(path, binary=false) end end -def verify_cert_key_pair(cert, key) +def verify_cert_key_pair(cert_data, key_data) errors = [] - cert_modulus = nil - key_modulus = nil - stdout, stderr, retval = run_cmd_options( - PCSAuth.getSuperuserAuth(), - { - 'stdin' => cert, - }, - '/usr/bin/openssl', 'x509', '-modulus', '-noout' - ) - if retval != 0 - errors << "Invalid certificate: #{stderr.join}" - else - cert_modulus = stdout.join.strip + begin + cert = OpenSSL::X509::Certificate.new(cert_data) + rescue OpenSSL::X509::CertificateError => e + errors << "Invalid certificate: #{e}" end - stdout, stderr, retval = run_cmd_options( - PCSAuth.getSuperuserAuth(), - { - 'stdin' => key, - }, - '/usr/bin/openssl', 'rsa', '-modulus', '-noout' - ) - if retval != 0 - errors << "Invalid key: #{stderr.join}" - else - key_modulus = stdout.join.strip + begin + key = OpenSSL::PKey.read(key_data) + rescue OpenSSL::PKey::PKeyError => e + errors << "Invalid key: #{e}" end - if errors.empty? and cert_modulus and key_modulus - if cert_modulus != key_modulus + if errors.empty? + if not cert.check_private_key(key) errors << 'Certificate does not match the key' end end diff --git a/pcsd/remote.rb b/pcsd/remote.rb index 3361b3f6..c43e3116 100644 --- a/pcsd/remote.rb +++ b/pcsd/remote.rb @@ -694,7 +694,7 @@ def set_certs(params, request, auth_user) if !ssl_cert.empty? and !ssl_key.empty? ssl_errors = verify_cert_key_pair(ssl_cert, ssl_key) if ssl_errors and !ssl_errors.empty? - return [400, ssl_errors.join] + return [400, ssl_errors.join('; ')] end begin write_file_lock(CRT_FILE, 0600, ssl_cert) diff --git a/requirements.txt b/requirements.txt index eb42ce40..2f62b1c3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,7 +2,7 @@ astroid==2.4.2 pylint==2.6.0 tornado>=6.1.0 -mypy==0.790 +mypy==0.812 dacite # temporarily stick to previous version until it's convinient to reformat code black==20.8b1 diff --git a/test/centos8/Dockerfile b/test/centos8/Dockerfile index 910d7652..00c17ffe 100644 --- a/test/centos8/Dockerfile +++ b/test/centos8/Dockerfile @@ -7,11 +7,11 @@ RUN dnf install -y \ --enablerepo=PowerTools \ # python python3 \ + python3-cryptography \ python3-lxml \ python3-mock \ python3-pip \ python3-pycurl \ - python3-pyOpenSSL \ python3-pyparsing \ # ruby ruby \ diff --git a/test/fedora31/Dockerfile b/test/fedora31/Dockerfile index cc94bee2..8d0a0672 100644 --- a/test/fedora31/Dockerfile +++ b/test/fedora31/Dockerfile @@ -5,11 +5,11 @@ ARG src_path RUN dnf install -y \ # python python3 \ + python3-cryptography \ python3-lxml \ python3-mock \ python3-pip \ python3-pycurl \ - python3-pyOpenSSL \ python3-pyparsing \ # ruby ruby \ diff --git a/test/fedora32/Dockerfile b/test/fedora32/Dockerfile index 82bdff74..750ff979 100644 --- a/test/fedora32/Dockerfile +++ b/test/fedora32/Dockerfile @@ -5,12 +5,12 @@ ARG src_path RUN dnf install -y \ # python python3 \ + python3-cryptography \ python3-distro \ python3-lxml \ python3-mock \ python3-pip \ python3-pycurl \ - python3-pyOpenSSL \ python3-pyparsing \ # ruby ruby \ -- 2.26.2