Retry errors with subproblems in obtain_certificate with --allow-subset-of-names (#9251) (#9272)

* Handle CAA failure on finalize_order during renewal (#9251)

* Fix CAA error on renewal test

* Attempt to fix failing test in CI

* Retry errors with subproblems in obtain_certificate_from_csr with allow_subset_of_names

Only retry if not all domains succeeded

* Back out renewal changes

* Fix linting error line too long

* Update log message for more general case and only log on retry

* Changelog entry

* Add retry logic to order creation

* Changelog entry wording

* Fix acme error handling when no subproblems provided

* Fix test name

* Use summarize domain list to display list of failed domains

* Tidy up incorrect client tests

* Remove unused var and output all failed domains

* Add logging to failed authorization case

* use _retry_obtain_certificate for failed authorizations

* Fix typo failing in CI

* Retry logic comments

* Preserve original error

* Move changelog entry to latest version

3 files changed, 233 insertions, 8 deletions

diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index f6391d678..996b409e0 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -10,7 +10,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Changed
 
-*
+* `--allow-subset-of-names` will now additionally retry in cases where domains are rejected while creating or finalizing orders. This requires subproblem support from the ACME server.
 
 ### Fixed
 
diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py
index 057b4f059..bcd9713db 100644
--- a/certbot/certbot/_internal/client.py
+++ b/certbot/certbot/_internal/client.py
@@ -438,7 +438,17 @@ class Client:
             csr = crypto_util.generate_csr(key, domains, self.config.csr_dir,
                                            self.config.must_staple, self.config.strict_permissions)
 
-        orderr = self._get_order_and_authorizations(csr.data, self.config.allow_subset_of_names)
+        try:
+            orderr = self._get_order_and_authorizations(csr.data, self.config.allow_subset_of_names)
+        except messages.Error as error:
+            # Some domains may be rejected during order creation.
+            # Certbot can retry the operation without the rejected
+            # domains contained within subproblems.
+            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(key, csr, domains, successful_domains)
+            raise
         authzr = orderr.authorizations
         auth_domains = {a.body.identifier.value for a in authzr}
         successful_domains = [d for d in domains if d in auth_domains]
@@ -449,13 +459,20 @@ 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:
-            if not self.config.dry_run:
-                os.remove(key.file)
-                os.remove(csr.file)
-            return self.obtain_certificate(successful_domains)
+            return self._retry_obtain_certificate(key, csr, domains, successful_domains)
         else:
-            cert, chain = self.obtain_certificate_from_csr(csr, orderr)
-            return cert, chain, key, csr
+            try:
+                cert, chain = self.obtain_certificate_from_csr(csr, orderr)
+                return cert, chain, key, csr
+            except messages.Error as error:
+                # Some domains may be rejected during the very late stage of
+                # order finalization. Certbot can retry the operation without
+                # the rejected domains contained within subproblems.
+                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(key, csr, domains, successful_domains)
+                raise
 
     def _get_order_and_authorizations(self, csr_pem: bytes,
                                       best_effort: bool) -> messages.OrderResource:
@@ -528,6 +545,27 @@ class Client:
             key.pem, chain,
             self.config)
 
+    def _successful_domains_from_error(self, error: messages.Error, domains: List[str],
+                                ) -> List[str]:
+        if error.subproblems is not None:
+            failed_domains = [problem.identifier.value for problem in error.subproblems
+                                if problem.identifier is not None]
+            successful_domains = [x for x in domains if x not in failed_domains]
+            return successful_domains
+        return []
+
+    def _retry_obtain_certificate(self, key: util.Key,
+                                csr: util.CSR, domains: List[str], successful_domains: List[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}")
+        if not self.config.dry_run:
+            os.remove(key.file)
+            os.remove(csr.file)
+        return self.obtain_certificate(successful_domains)
+
     def _choose_lineagename(self, domains: List[str], certname: Optional[str]) -> str:
         """Chooses a name for the new lineage.
 
diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py
index 8d8073f6f..28daefea8 100644
--- a/certbot/tests/client_test.py
+++ b/certbot/tests/client_test.py
@@ -396,6 +396,193 @@ class ClientTest(ClientTestCommon):
         self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 1)
 
     @mock.patch("certbot._internal.client.crypto_util")
+    @mock.patch("certbot.compat.os.remove")
+    def test_obtain_certificate_finalize_order_partial_success(self, mock_remove, mock_crypto_util):
+        from acme import messages
+        csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN)
+        key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN)
+        mock_crypto_util.generate_csr.return_value = csr
+        mock_crypto_util.generate_key.return_value = key
+        self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain)
+
+        self._mock_obtain_certificate()
+        authzr = self._authzr_from_domains(self.eg_domains)
+        self.eg_order.authorizations = authzr
+        self.client.auth_handler.handle_authorizations.return_value = authzr
+
+        identifier = messages.Identifier(typ=messages.IDENTIFIER_FQDN, value='example.com')
+        subproblem = messages.Error.with_code('caa', detail='bar', title='title', identifier=identifier)
+        error_with_subproblems = messages.Error.with_code('malformed', detail='foo', title='title', subproblems=[subproblem])
+        self.client.acme.finalize_order.side_effect = [error_with_subproblems, mock.DEFAULT]
+
+        self.config.allow_subset_of_names = True
+
+        with test_util.patch_display_util():
+            result = self.client.obtain_certificate(self.eg_domains)
+
+        self.assertEqual(
+            result,
+            (mock.sentinel.cert, mock.sentinel.chain, key, csr))
+        self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, 2)
+        self.assertEqual(self.acme.finalize_order.call_count, 2)
+
+        successful_domains = [d for d in self.eg_domains if d != 'example.com']
+        self.assertEqual(mock_crypto_util.generate_key.call_count, 2)
+        mock_crypto_util.generate_csr.assert_has_calls([
+            mock.call(key, self.eg_domains, self.config.csr_dir, self.config.must_staple, self.config.strict_permissions),
+            mock.call(key, successful_domains, self.config.csr_dir, self.config.must_staple, self.config.strict_permissions)])
+        self.assertEqual(mock_remove.call_count, 2)
+        self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 1)
+
+    @mock.patch("certbot._internal.client.crypto_util")
+    def test_obtain_certificate_finalize_order_no_retryable_domains(self, mock_crypto_util):
+        from acme import messages
+        csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN)
+        key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN)
+        mock_crypto_util.generate_csr.return_value = csr
+        mock_crypto_util.generate_key.return_value = key
+        self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain)
+
+        self._mock_obtain_certificate()
+        authzr = self._authzr_from_domains(self.eg_domains)
+        self.eg_order.authorizations = authzr
+        self.client.auth_handler.handle_authorizations.return_value = authzr
+
+        identifier1 = messages.Identifier(typ=messages.IDENTIFIER_FQDN, value='example.com')
+        identifier2 = messages.Identifier(typ=messages.IDENTIFIER_FQDN, value='www.example.com')
+        subproblem1 = messages.Error.with_code('caa', detail='bar', title='title', identifier=identifier1)
+        subproblem2 = messages.Error.with_code('caa', detail='bar', title='title', identifier=identifier2)
+        error_with_subproblems = messages.Error.with_code('malformed', detail='foo', title='title', subproblems=[subproblem1, subproblem2])
+        self.client.acme.finalize_order.side_effect = error_with_subproblems
+
+        self.config.allow_subset_of_names = True
+
+        self.assertRaises(messages.Error, self.client.obtain_certificate, self.eg_domains)
+        self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, 1)
+        self.assertEqual(self.acme.finalize_order.call_count, 1)
+        self.assertEqual(mock_crypto_util.generate_key.call_count, 1)
+        self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 0)
+
+    @mock.patch("certbot._internal.client.crypto_util")
+    def test_obtain_certificate_finalize_order_rejected_identifier_no_subproblems(self, mock_crypto_util):
+        from acme import messages
+        csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN)
+        key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN)
+        mock_crypto_util.generate_csr.return_value = csr
+        mock_crypto_util.generate_key.return_value = key
+        self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain)
+
+        self._mock_obtain_certificate()
+        authzr = self._authzr_from_domains(self.eg_domains)
+        self.eg_order.authorizations = authzr
+        self.client.auth_handler.handle_authorizations.return_value = authzr
+
+        error = messages.Error.with_code('caa', detail='foo', title='title')
+        self.client.acme.finalize_order.side_effect = error
+
+        self.config.allow_subset_of_names = True
+
+        self.assertRaises(messages.Error, self.client.obtain_certificate,
+                          self.eg_domains)
+        self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, 1)
+        self.assertEqual(self.acme.finalize_order.call_count, 1)
+        self.assertEqual(mock_crypto_util.generate_key.call_count, 1)
+        self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 0)
+
+    @mock.patch("certbot._internal.client.crypto_util")
+    @mock.patch("certbot.compat.os.remove")
+    def test_obtain_certificate_get_order_partial_success(self, mock_remove, mock_crypto_util):
+        from acme import messages
+        csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN)
+        key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN)
+        mock_crypto_util.generate_csr.return_value = csr
+        mock_crypto_util.generate_key.return_value = key
+        self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain)
+
+        self._mock_obtain_certificate()
+        authzr = self._authzr_from_domains(self.eg_domains)
+        self.eg_order.authorizations = authzr
+        self.client.auth_handler.handle_authorizations.return_value = authzr
+
+        identifier = messages.Identifier(typ=messages.IDENTIFIER_FQDN, value='example.com')
+        subproblem = messages.Error.with_code('caa', detail='bar', title='title', identifier=identifier)
+        error_with_subproblems = messages.Error.with_code('malformed', detail='foo', title='title', subproblems=[subproblem])
+        self.client.acme.new_order.side_effect = [error_with_subproblems, mock.DEFAULT]
+
+        self.config.allow_subset_of_names = True
+
+        with test_util.patch_display_util():
+            result = self.client.obtain_certificate(self.eg_domains)
+
+        self.assertEqual(
+            result,
+            (mock.sentinel.cert, mock.sentinel.chain, key, csr))
+        self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, 1)
+        self.assertEqual(self.acme.new_order.call_count, 2)
+
+        successful_domains = [d for d in self.eg_domains if d != 'example.com']
+        self.assertEqual(mock_crypto_util.generate_key.call_count, 2)
+        mock_crypto_util.generate_csr.assert_has_calls([
+            mock.call(key, self.eg_domains, self.config.csr_dir, self.config.must_staple, self.config.strict_permissions),
+            mock.call(key, successful_domains, self.config.csr_dir, self.config.must_staple, self.config.strict_permissions)])
+        self.assertEqual(mock_remove.call_count, 2)
+        self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 1)
+
+    @mock.patch("certbot._internal.client.crypto_util")
+    def test_obtain_certificate_get_order_no_retryable_domains(self, mock_crypto_util):
+        from acme import messages
+        csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN)
+        key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN)
+        mock_crypto_util.generate_csr.return_value = csr
+        mock_crypto_util.generate_key.return_value = key
+        self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain)
+
+        self._mock_obtain_certificate()
+        authzr = self._authzr_from_domains(self.eg_domains)
+        self.eg_order.authorizations = authzr
+        self.client.auth_handler.handle_authorizations.return_value = authzr
+
+        identifier1 = messages.Identifier(typ=messages.IDENTIFIER_FQDN, value='example.com')
+        identifier2 = messages.Identifier(typ=messages.IDENTIFIER_FQDN, value='www.example.com')
+        subproblem1 = messages.Error.with_code('caa', detail='bar', title='title', identifier=identifier1)
+        subproblem2 = messages.Error.with_code('caa', detail='bar', title='title', identifier=identifier2)
+        error_with_subproblems = messages.Error.with_code('malformed', detail='foo', title='title', subproblems=[subproblem1, subproblem2])
+        self.client.acme.new_order.side_effect = error_with_subproblems
+
+        self.config.allow_subset_of_names = True
+
+        self.assertRaises(messages.Error, self.client.obtain_certificate, self.eg_domains)
+        self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, 0)
+        self.assertEqual(self.acme.new_order.call_count, 1)
+        self.assertEqual(mock_crypto_util.generate_key.call_count, 1)
+        self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 0)
+
+    @mock.patch("certbot._internal.client.crypto_util")
+    def test_obtain_certificate_get_order_rejected_identifier_no_subproblems(self, mock_crypto_util):
+        from acme import messages
+        csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN)
+        key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN)
+        mock_crypto_util.generate_csr.return_value = csr
+        mock_crypto_util.generate_key.return_value = key
+        self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain)
+
+        self._mock_obtain_certificate()
+        authzr = self._authzr_from_domains(self.eg_domains)
+        self.eg_order.authorizations = authzr
+        self.client.auth_handler.handle_authorizations.return_value = authzr
+
+        error = messages.Error.with_code('caa', detail='foo', title='title')
+        self.client.acme.new_order.side_effect = error
+
+        self.config.allow_subset_of_names = True
+
+        self.assertRaises(messages.Error, self.client.obtain_certificate, self.eg_domains)
+        self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, 0)
+        self.assertEqual(self.acme.new_order.call_count, 1)
+        self.assertEqual(mock_crypto_util.generate_key.call_count, 1)
+        self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 0)
+
+    @mock.patch("certbot._internal.client.crypto_util")
     @mock.patch("certbot._internal.client.acme_crypto_util")
     def test_obtain_certificate_dry_run(self, mock_acme_crypto, mock_crypto):
         csr = util.CSR(form="pem", file=None, data=CSR_SAN)