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 @@ -395,6 +395,193 @@ class ClientTest(ClientTestCommon): 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") + @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):