From 680729655e57ae9f9ea5610780c1b3abc06a58c5 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Wed, 15 Jan 2025 17:41:57 -0800 Subject: [PATCH] Honor --reuse-key when --allow-subset-of-names is set (#10138) Fixes #10109. We were not previously doing so, and that was an oversight. Adds regression tests in unit tests and integration tests. Integration regression test failing without the fix is here: https://dev.azure.com/certbot/certbot/_build/results?buildId=8463&view=logs&j=fca58cec-e7ce-563a-f36f-5c233894d750&t=8c19ffdb-5db1-573e-d81e-907ba1b3cfee --- .../certbot_tests/context.py | 8 +++++ .../certbot_tests/test_main.py | 36 +++++++++++++++++++ certbot/CHANGELOG.md | 1 + certbot/certbot/_internal/client.py | 12 ++++--- .../certbot/_internal/tests/client_test.py | 29 +++++++++++++++ 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/context.py b/certbot-ci/certbot_integration_tests/certbot_tests/context.py index bc893d8d9..120e0e9d2 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/context.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/context.py @@ -47,6 +47,14 @@ class IntegrationTestsContext: "request.raise_for_status(); " '"' ).format(sys.executable, self.challtestsrv_url) + self.manual_dns_auth_hook_allow_fail = ( + '{0} -c "import os; import requests; import json; ' + "data = {{'host':'_acme-challenge.{{0}}.'.format(os.environ.get('CERTBOT_DOMAIN'))," + "'value':os.environ.get('CERTBOT_VALIDATION')}}; " + "request = requests.post('{1}/set-txt', data=json.dumps(data)); " + "request.raise_for_status(); " + '"' + ).format(sys.executable, self.challtestsrv_url) self.manual_dns_cleanup_hook = ( '{0} -c "import os; import requests; import json; ' "data = {{'host':'_acme-challenge.{{0}}.'.format(os.environ.get('CERTBOT_DOMAIN'))}}; " diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py index 54ff2d588..f32b46f78 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -468,6 +468,42 @@ def test_reuse_key(context: IntegrationTestsContext) -> None: assert len({cert1, cert2, cert3}) == 3 +def test_reuse_key_allow_subset_of_names(context: IntegrationTestsContext) -> None: + """Test that key is reused when allow_subset_of_names is set.""" + certname = context.get_domain('dns1') + domains = ','.join([certname, context.get_domain('fail-dns1')]) + context.certbot([ + '-a', 'manual', '-d', domains, + '--allow-subset-of-names', + '--preferred-challenges', 'dns', + '--manual-auth-hook', context.manual_dns_auth_hook_allow_fail, + '--manual-cleanup-hook', context.manual_dns_cleanup_hook, + '--reuse-key' + ]) + + stdout, _ = context.certbot(['certificates']) + assert context.get_domain('fail-dns1') in stdout + certname = context.get_domain('dns1') + + # manual_dns_auth_hook from misc is designed to fail if the domain contains 'fail-*'. + context.certbot([ + 'reconfigure', + '--cert-name', certname, + '--manual-auth-hook', context.manual_dns_auth_hook, + '-a', 'manual' # needed to override --standalone passed automatically + ]) + + context.certbot(['renew', '--cert-name', certname, '--force-renewal', '-a', 'manual']) + stdout, _ = context.certbot(['certificates']) + assert context.get_domain('fail-dns1') not in stdout + + with open(join(context.config_dir, 'archive/{0}/privkey1.pem').format(certname), 'r') as file: + privkey1 = file.read() + with open(join(context.config_dir, 'archive/{0}/privkey2.pem').format(certname), 'r') as file: + privkey2 = file.read() + assert privkey1 == privkey2 + + def test_new_key(context: IntegrationTestsContext) -> None: """Tests --new-key and its interactions with --reuse-key""" def private_key(generation: int) -> Tuple[str, str]: diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 434ecc1ec..5bb02a3be 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -17,6 +17,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed * Allow nginx plugin to parse non-breaking spaces in nginx configuration files. +* Honor --reuse-key when --allow-subset-of-names is set * More details about these changes can be found on our GitHub repo. diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index 9bd4c682c..e51842be6 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -434,7 +434,7 @@ class Client: if self.config.allow_subset_of_names: successful_domains = self._successful_domains_from_error(error, domains) if successful_domains != domains and len(successful_domains) != 0: - return self._retry_obtain_certificate(domains, successful_domains) + return self._retry_obtain_certificate(domains, successful_domains, old_keypath) raise authzr = orderr.authorizations auth_domains = {a.body.identifier.value for a in authzr} @@ -446,7 +446,7 @@ class Client: # domains contains a wildcard because the ACME spec forbids identifiers # in authzs from containing a wildcard character. if self.config.allow_subset_of_names and successful_domains != domains: - return self._retry_obtain_certificate(domains, successful_domains) + return self._retry_obtain_certificate(domains, successful_domains, old_keypath) else: try: cert, chain = self.obtain_certificate_from_csr(csr, orderr) @@ -458,7 +458,8 @@ class Client: if self.config.allow_subset_of_names: successful_domains = self._successful_domains_from_error(error, domains) if successful_domains != domains and len(successful_domains) != 0: - return self._retry_obtain_certificate(domains, successful_domains) + return self._retry_obtain_certificate( + domains, successful_domains, old_keypath) raise def _get_order_and_authorizations(self, csr_pem: bytes, @@ -540,13 +541,14 @@ class Client: return successful_domains return [] - def _retry_obtain_certificate(self, domains: List[str], successful_domains: List[str] + def _retry_obtain_certificate(self, domains: List[str], successful_domains: List[str], + old_keypath: Optional[str] ) -> Tuple[bytes, bytes, util.Key, util.CSR]: failed_domains = [d for d in domains if d not in successful_domains] domains_list = ", ".join(failed_domains) display_util.notify("Unable to obtain a certificate with every requested " f"domain. Retrying without: {domains_list}") - return self.obtain_certificate(successful_domains) + return self.obtain_certificate(successful_domains, old_keypath) def _choose_lineagename(self, domains: List[str], certname: Optional[str]) -> str: """Chooses a name for the new lineage. diff --git a/certbot/certbot/_internal/tests/client_test.py b/certbot/certbot/_internal/tests/client_test.py index 7e7bb9018..75ddfa948 100644 --- a/certbot/certbot/_internal/tests/client_test.py +++ b/certbot/certbot/_internal/tests/client_test.py @@ -646,6 +646,35 @@ class ClientTest(ClientTestCommon): " every domain. The dry run will continue, but results" " may not be accurate.") + @mock.patch("certbot._internal.client.crypto_util") + def test_obtain_certificate_reuse_key_with_allow_subset_of_names(self, mock_crypto_util): + csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN) + old_key = util.Key(file="old_key_file", pem="old_key_pem") + new_key = util.Key(file="new_key_file", pem="new_key_pem") + mock_crypto_util.generate_csr.return_value = csr + mock_crypto_util.generate_key.return_value = new_key + self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) + + authzr = self._authzr_from_domains(["example.com"]) + self.config.allow_subset_of_names = True + self.config.reuse_key = True + + self._mock_obtain_certificate() + + self.eg_order.authorizations = authzr + self.client.auth_handler.handle_authorizations.return_value = authzr + + with test_util.patch_display_util(): + mocked_open = mock.mock_open(read_data="old_key_pem") + with mock.patch('builtins.open', mocked_open): + result = self.client.obtain_certificate(self.eg_domains, "old_key_file") + + assert result == \ + (mock.sentinel.cert, mock.sentinel.chain, old_key, csr) + self._check_obtain_certificate(2) + + assert mock_crypto_util.generate_key.call_count == 0 + def _set_mock_from_fullchain(self, mock_from_fullchain): mock_cert = mock.Mock() mock_cert.encode.return_value = mock.sentinel.cert