From cda56361ad46b02f986e1cd72da558e1b5b7a621 Mon Sep 17 00:00:00 2001 From: Jonathan Vanasco Date: Mon, 24 Feb 2025 13:54:17 -0500 Subject: [PATCH] Fix deprecate pyopenssl (#10186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uploading for tests; These deprecations are a precursor to #10174 In addition to the previously discussed `acme` functions, the `certbot` functions were deprecated as they are primarily used for testing and support. Marking them deprecated now will allow them to be removed in the next major release, as they will no-longer be used. --- acme/acme/_internal/tests/crypto_util_test.py | 34 ++++++++--- acme/acme/crypto_util.py | 26 +++++++++ certbot/CHANGELOG.md | 6 +- .../_internal/tests/crypto_util_test.py | 16 +++++- certbot/certbot/_internal/tests/main_test.py | 56 ++++++++++++------- certbot/certbot/crypto_util.py | 17 ++++++ 6 files changed, 125 insertions(+), 30 deletions(-) diff --git a/acme/acme/_internal/tests/crypto_util_test.py b/acme/acme/_internal/tests/crypto_util_test.py index b272712cc..fc37f591f 100644 --- a/acme/acme/_internal/tests/crypto_util_test.py +++ b/acme/acme/_internal/tests/crypto_util_test.py @@ -104,7 +104,12 @@ class PyOpenSSLCertOrReqAllNamesTest(unittest.TestCase): def _call(cls, loader, name): # pylint: disable=protected-access from acme.crypto_util import _pyopenssl_cert_or_req_all_names - return _pyopenssl_cert_or_req_all_names(loader(name)) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='acme.crypto_util._pyopenssl_cert_or_req_all_names is deprecated *' + ) + return _pyopenssl_cert_or_req_all_names(loader(name)) def _call_cert(self, name): return self._call(test_util.load_cert, name) @@ -128,7 +133,12 @@ class PyOpenSSLCertOrReqSANTest(unittest.TestCase): def _call(cls, loader, name): # pylint: disable=protected-access from acme.crypto_util import _pyopenssl_cert_or_req_san - return _pyopenssl_cert_or_req_san(loader(name)) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='acme.crypto_util._pyopenssl_cert_or_req_san is deprecated *' + ) + return _pyopenssl_cert_or_req_san(loader(name)) @classmethod def _get_idn_names(cls): @@ -392,7 +402,12 @@ class DumpPyopensslChainTest(unittest.TestCase): def _call(cls, loaded): # pylint: disable=protected-access from acme.crypto_util import dump_pyopenssl_chain - return dump_pyopenssl_chain(loaded) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='acme.crypto_util.dump_pyopenssl_chain is *' + ) + return dump_pyopenssl_chain(loaded) def test_dump_pyopenssl_chain(self): names = ['cert.pem', 'cert-san.pem', 'cert-idnsans.pem'] @@ -406,10 +421,15 @@ class DumpPyopensslChainTest(unittest.TestCase): names = ['cert.pem', 'cert-san.pem', 'cert-idnsans.pem'] loaded = [test_util.load_cert(name) for name in names] wrap_func = jose.ComparableX509 - wrapped = [wrap_func(cert) for cert in loaded] - dump_func = OpenSSL.crypto.dump_certificate - length = sum(len(dump_func(OpenSSL.crypto.FILETYPE_PEM, cert)) for cert in loaded) - assert len(self._call(wrapped)) == length + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='The next major version of josepy *' + ) + wrapped = [wrap_func(cert) for cert in loaded] + dump_func = OpenSSL.crypto.dump_certificate + length = sum(len(dump_func(OpenSSL.crypto.FILETYPE_PEM, cert)) for cert in loaded) + assert len(self._call(wrapped)) == length if __name__ == '__main__': diff --git a/acme/acme/crypto_util.py b/acme/acme/crypto_util.py index 20d5d68e3..bf6628c29 100644 --- a/acme/acme/crypto_util.py +++ b/acme/acme/crypto_util.py @@ -373,6 +373,16 @@ def get_names_from_subject_and_extensions( def _pyopenssl_cert_or_req_all_names(loaded_cert_or_req: Union[crypto.X509, crypto.X509Req] ) -> List[str]: + """ + Deprecated + .. deprecated: 3.2.1 + """ + warnings.warn( + "acme.crypto_util._pyopenssl_cert_or_req_all_names is deprecated and " + "will be removed in the next major release of Certbot.", + DeprecationWarning, + stacklevel=2 + ) cert_or_req = loaded_cert_or_req.to_cryptography() return get_names_from_subject_and_extensions( cert_or_req.subject, cert_or_req.extensions @@ -391,7 +401,15 @@ def _pyopenssl_cert_or_req_san(cert_or_req: Union[crypto.X509, crypto.X509Req]) :returns: A list of Subject Alternative Names that is DNS. :rtype: `list` of `str` + Deprecated + .. deprecated: 3.2.1 """ + warnings.warn( + "acme.crypto_util._pyopenssl_cert_or_req_san is deprecated and " + "will be removed in the next major release of Certbot.", + DeprecationWarning, + stacklevel=2 + ) exts = cert_or_req.to_cryptography().extensions try: san_ext = exts.get_extension_for_class(x509.SubjectAlternativeName) @@ -563,7 +581,15 @@ def dump_pyopenssl_chain(chain: Union[List[jose.ComparableX509], List[crypto.X50 :returns: certificate chain bundle :rtype: bytes + Deprecated + .. deprecated: 3.2.1 """ + warnings.warn( + "acme.crypto_util.dump_pyopenssl_chain is deprecated and " + "will be removed in the next major release of Certbot.", + DeprecationWarning, + stacklevel=2 + ) # XXX: returns empty string when no chain is available, which # shuts up RenewableCert, but might not be the best solution... diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 2b53ef84b..9aa5cac04 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -10,7 +10,11 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed -* +* deprecated `acme.crypto_util.dump_pyopenssl_chain` +* deprecated `acme.crypto_util._pyopenssl_cert_or_req_all_names` +* deprecated `acme.crypto_util._pyopenssl_cert_or_req_san` +* deprecated `certbot.crypto_util.dump_pyopenssl_chain` +* deprecated `certbot.crypto_util.pyopenssl_load_certificate` ### Fixed diff --git a/certbot/certbot/_internal/tests/crypto_util_test.py b/certbot/certbot/_internal/tests/crypto_util_test.py index 9e5fda6ce..f8531f382 100644 --- a/certbot/certbot/_internal/tests/crypto_util_test.py +++ b/certbot/certbot/_internal/tests/crypto_util_test.py @@ -5,6 +5,7 @@ import re import sys import unittest from unittest import mock +import warnings import OpenSSL import pytest @@ -405,7 +406,13 @@ class CertLoaderTest(unittest.TestCase): def test_load_valid_cert(self): from certbot.crypto_util import pyopenssl_load_certificate - cert, file_type = pyopenssl_load_certificate(CERT) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='certbot.crypto_util.pyopenssl_load_certificate is *' + ) + cert, file_type = pyopenssl_load_certificate(CERT) + assert file_type == OpenSSL.crypto.FILETYPE_PEM assert binascii.unhexlify( cert.digest("sha256").replace(b":", b"") @@ -415,7 +422,12 @@ class CertLoaderTest(unittest.TestCase): from certbot.crypto_util import pyopenssl_load_certificate bad_cert_data = CERT.replace(b"BEGIN CERTIFICATE", b"ASDFASDFASDF!!!") with pytest.raises(errors.Error): - pyopenssl_load_certificate(bad_cert_data) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='certbot.crypto_util.pyopenssl_load_certificate is *' + ) + pyopenssl_load_certificate(bad_cert_data) class NotBeforeTest(unittest.TestCase): diff --git a/certbot/certbot/_internal/tests/main_test.py b/certbot/certbot/_internal/tests/main_test.py index 343f7f135..5fa499efb 100644 --- a/certbot/certbot/_internal/tests/main_test.py +++ b/certbot/certbot/_internal/tests/main_test.py @@ -14,6 +14,7 @@ import traceback from typing import List import unittest from unittest import mock +import warnings import configobj import josepy as jose @@ -435,7 +436,12 @@ class RevokeTest(test_util.TempDirTestCase): config = cli.prepare_and_parse_args(plugins, args) from certbot._internal.main import revoke - revoke(config, plugins) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='certbot.crypto_util.pyopenssl_load_certificate is *' + ) + revoke(config, plugins) @mock.patch('certbot._internal.main._delete_if_appropriate') @mock.patch('certbot._internal.main.client.acme_client') @@ -1797,18 +1803,23 @@ class MainTest(test_util.ConfigTestCase): mock_delete_if_appropriate): mock_delete_if_appropriate.return_value = False server = 'foo.bar' - self._call_no_clientmock(['--cert-path', SS_CERT_PATH, '--key-path', RSA2048_KEY_PATH, - '--server', server, 'revoke']) - with open(RSA2048_KEY_PATH, 'rb') as f: - assert mock_acme_client.ClientV2.call_count == 1 - assert mock_acme_client.ClientNetwork.call_args[0][0] == \ - jose.JWK.load(f.read()) - with open(SS_CERT_PATH, 'rb') as f: - cert = crypto_util.pyopenssl_load_certificate(f.read())[0] - mock_revoke = mock_acme_client.ClientV2().revoke - mock_revoke.assert_called_once_with( - jose.ComparableX509(cert), - mock.ANY) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='certbot.crypto_util.pyopenssl_load_certificate is *' + ) + self._call_no_clientmock(['--cert-path', SS_CERT_PATH, '--key-path', RSA2048_KEY_PATH, + '--server', server, 'revoke']) + with open(RSA2048_KEY_PATH, 'rb') as f: + assert mock_acme_client.ClientV2.call_count == 1 + assert mock_acme_client.ClientNetwork.call_args[0][0] == \ + jose.JWK.load(f.read()) + with open(SS_CERT_PATH, 'rb') as f: + cert = crypto_util.pyopenssl_load_certificate(f.read())[0] + mock_revoke = mock_acme_client.ClientV2().revoke + mock_revoke.assert_called_once_with( + jose.ComparableX509(cert), + mock.ANY) def test_revoke_with_key_mismatch(self): server = 'foo.bar' @@ -1822,13 +1833,18 @@ class MainTest(test_util.ConfigTestCase): mock_delete_if_appropriate): mock_delete_if_appropriate.return_value = False mock_determine_account.return_value = (mock.MagicMock(), None) - _, _, _, client = self._call(['--cert-path', CERT, 'revoke']) - with open(CERT) as f: - cert = crypto_util.pyopenssl_load_certificate(f.read())[0] - mock_revoke = client.acme_from_config_key().revoke - mock_revoke.assert_called_once_with( - jose.ComparableX509(cert), - mock.ANY) + with warnings.catch_warnings(): + warnings.filterwarnings( + 'ignore', + message='certbot.crypto_util.pyopenssl_load_certificate is *' + ) + _, _, _, client = self._call(['--cert-path', CERT, 'revoke']) + with open(CERT) as f: + cert = crypto_util.pyopenssl_load_certificate(f.read())[0] + mock_revoke = client.acme_from_config_key().revoke + mock_revoke.assert_called_once_with( + jose.ComparableX509(cert), + mock.ANY) @mock.patch('certbot._internal.log.post_arg_parse_setup') def test_register(self, _): diff --git a/certbot/certbot/crypto_util.py b/certbot/certbot/crypto_util.py index 4dd062ab2..076dadd21 100644 --- a/certbot/certbot/crypto_util.py +++ b/certbot/certbot/crypto_util.py @@ -14,6 +14,7 @@ from typing import Set from typing import Tuple from typing import TYPE_CHECKING from typing import Union +import warnings from cryptography import x509 from cryptography.exceptions import InvalidSignature @@ -395,7 +396,15 @@ def pyopenssl_load_certificate(data: bytes) -> Tuple[crypto.X509, int]: :raises errors.Error: + Deprecated + .. deprecated: 3.2.1 """ + warnings.warn( + "certbot.crypto_util.pyopenssl_load_certificate is deprecated and " + "will be removed in the next major release of Certbot.", + DeprecationWarning, + stacklevel=2 + ) openssl_errors = [] @@ -491,7 +500,15 @@ def dump_pyopenssl_chain( :param list chain: List of `crypto.X509` (or wrapped in :class:`josepy.util.ComparableX509`). + Deprecated + .. deprecated: 3.2.1 """ + warnings.warn( + "certbot.crypto_util.dump_pyopenssl_chain is deprecated and " + "will be removed in the next major release of Certbot.", + DeprecationWarning, + stacklevel=2 + ) # XXX: returns empty string when no chain is available, which # shuts up RenewableCert, but might not be the best solution... return acme_crypto_util.dump_pyopenssl_chain(chain, filetype)