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-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'))}}; "

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]:

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.

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.

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