diff --git a/lemur/common/celery.py b/lemur/common/celery.py index 195f8355f8..4deb70309a 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -360,7 +360,25 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): error_log["last_error"] = cert.get("last_error") error_log["cn"] = pending_cert.cn - if pending_cert.number_attempts > ACME_ADDITIONAL_ATTEMPTS: + last_error = cert.get("last_error") + if pending_certificate_service.is_terminal_failure(last_error): + # Config / DNS-delegation / credential failure — retrying can never + # succeed and only burns the CA's rate limit. Fail fast. + error_log["message"] = "Terminal failure, resolving pending certificate" + # Persist the terminal state before notifying so a notification + # delivery failure can't leave the pending cert unresolved (and + # retried, re-exhausting the CA rate limit). + pending_certificate_service.update( + pending_cert.id, + status=str(last_error), + resolved=True, + ) + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + if notify_reissue_cert_id is not None: + send_reissue_failed_notification(pending_cert) + elif pending_cert.number_attempts >= ACME_ADDITIONAL_ATTEMPTS: error_log["message"] = "Deleting pending certificate" send_pending_failure_notification( pending_cert, notify_owner=pending_cert.notify @@ -369,12 +387,12 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): send_reissue_failed_notification(pending_cert) # Mark the pending cert as resolved pending_certificate_service.update( - cert.get("pending_cert").id, resolved=True + pending_cert.id, resolved=True ) else: pending_certificate_service.increment_attempt(pending_cert) pending_certificate_service.update( - cert.get("pending_cert").id, status=str(cert.get("last_error")) + pending_cert.id, status=str(last_error) ) # Add failed pending cert task back to queue fetch_acme_cert.delay(id, notify_reissue_cert_id) diff --git a/lemur/exceptions.py b/lemur/exceptions.py index cf05b4b02f..ccca6de12b 100644 --- a/lemur/exceptions.py +++ b/lemur/exceptions.py @@ -45,7 +45,32 @@ def __str__(self): return repr("The field '{0}' is not sortable or filterable".format(self.field)) -class InvalidConfiguration(Exception): +class PendingCertificateTerminalError(Exception): + """ + Base class for pending-certificate failures that retrying can never resolve. + + Terminal failures are configuration, DNS-delegation, or credential problems + (e.g. no DNS provider configured for a domain, broken ACME CNAME delegation, + or invalid ACME account credentials). Re-queueing these only re-runs the ACME + order/challenge and burns the CA's rate limit (Let's Encrypt: 5 duplicate + certificates / failed validations per week per domain), so they must fail fast + and mark the pending certificate resolved instead of retrying. + """ + + +class ACMEAuthenticationError(PendingCertificateTerminalError): + """The ACME CA rejected our credentials (upstream HTTP 401/403).""" + + +class NoDNSProviderError(PendingCertificateTerminalError): + """No DNS provider is authoritative for the validation domain.""" + + +class DNSChallengeSetupError(PendingCertificateTerminalError): + """The DNS-01 challenge could not be set up (delegation/config problem).""" + + +class InvalidConfiguration(PendingCertificateTerminalError): pass diff --git a/lemur/pending_certificates/cli.py b/lemur/pending_certificates/cli.py index 73b0ce2b50..19aafc82eb 100644 --- a/lemur/pending_certificates/cli.py +++ b/lemur/pending_certificates/cli.py @@ -109,17 +109,30 @@ def fetch_all_acme(): error_log["last_error"] = cert.get("last_error") error_log["cn"] = pending_cert.cn - if pending_cert.number_attempts > ACME_ADDITIONAL_ATTEMPTS: + last_error = cert.get("last_error") + if pending_certificate_service.is_terminal_failure(last_error): + error_log["message"] = "Terminal failure, marking pending certificate as resolved" + # Persist the terminal state before notifying so a notification + # delivery failure can't leave the pending cert unresolved. + pending_certificate_service.update( + pending_cert.id, + status=str(last_error), + resolved=True, + ) + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + elif pending_cert.number_attempts > ACME_ADDITIONAL_ATTEMPTS: error_log["message"] = "Marking pending certificate as resolved" send_pending_failure_notification( pending_cert, notify_owner=pending_cert.notify ) # Mark "resolved" as True - pending_certificate_service.update(cert.id, resolved=True) + pending_certificate_service.update(pending_cert.id, resolved=True) else: pending_certificate_service.increment_attempt(pending_cert) pending_certificate_service.update( - cert.get("pending_cert").id, status=str(cert.get("last_error")) + pending_cert.id, status=str(last_error) ) current_app.logger.error(error_log) log_data["message"] = "Complete" diff --git a/lemur/pending_certificates/service.py b/lemur/pending_certificates/service.py index a0dfde80ba..6b18ef9fcd 100644 --- a/lemur/pending_certificates/service.py +++ b/lemur/pending_certificates/service.py @@ -17,6 +17,7 @@ from lemur.common import validators from lemur.destinations.models import Destination from lemur.domains.models import Domain +from lemur.exceptions import PendingCertificateTerminalError from lemur.extensions import metrics from lemur.notifications.models import Notification from lemur.pending_certificates.models import PendingCertificate @@ -180,6 +181,26 @@ def increment_attempt(pending_certificate): return pending_certificate.number_attempts +# Failures that retrying can never resolve: configuration, DNS-delegation, or +# credential problems (e.g. no DNS provider configured for a domain, broken ACME +# CNAME delegation, or invalid ACME account credentials). Re-queueing these only +# re-runs the ACME order/challenge and burns the CA's rate limit (Let's Encrypt: +# 5 duplicate certificates / failed validations per week per domain), so they must +# fail fast and mark the pending certificate resolved instead of retrying. + + +def is_terminal_failure(error): + """ + Return True if a pending-certificate failure is terminal (a configuration, + DNS-delegation, or credential problem that retrying will never resolve). + + Classification is by explicit exception type (``PendingCertificateTerminalError`` + and subclasses) raised at the ACME/DNS boundaries, never by matching on + mutable exception text. + """ + return isinstance(error, PendingCertificateTerminalError) + + def update(pending_cert_id, **kwargs): """ Updates a pending certificate. The allowed fields are validated by diff --git a/lemur/plugins/lemur_acme/acme_handlers.py b/lemur/plugins/lemur_acme/acme_handlers.py index 20e469fbc6..4bac455853 100644 --- a/lemur/plugins/lemur_acme/acme_handlers.py +++ b/lemur/plugins/lemur_acme/acme_handlers.py @@ -33,7 +33,13 @@ from lemur.common.utils import data_encrypt, data_decrypt, is_json from lemur.common.utils import generate_private_key, key_to_alg from lemur.dns_providers import service as dns_provider_service -from lemur.exceptions import InvalidAuthority, UnknownProvider, InvalidConfiguration +from lemur.exceptions import ( + DNSChallengeSetupError, + InvalidAuthority, + InvalidConfiguration, + NoDNSProviderError, + UnknownProvider, +) from lemur.extensions import metrics from lemur.plugins.lemur_acme import cloudflare, dyn, route53, ultradns, powerdns, nsone @@ -425,7 +431,9 @@ def start_dns_challenge( if not dns_challenges: capture_exception() metrics.send("start_dns_challenge_error_no_dns_challenges", "counter", 1) - raise Exception("Unable to determine DNS challenges from authorizations") + raise DNSChallengeSetupError( + "Unable to determine DNS challenges from authorizations" + ) for dns_challenge in dns_challenges: if not cname_delegation: @@ -458,7 +466,7 @@ def complete_dns_challenge(self, acme_client, authz_record): dns_providers = self.dns_providers_for_domain.get(authz_record.target_domain) if not dns_providers: metrics.send("complete_dns_challenge_error_no_dnsproviders", "counter", 1) - raise Exception( + raise NoDNSProviderError( "No DNS providers found for domain: {}".format( authz_record.target_domain ) @@ -535,7 +543,7 @@ def get_authorizations(self, acme_client, order, order_info): metrics.send( "get_authorizations_no_dns_provider_for_domain", "counter", 1 ) - raise Exception( + raise NoDNSProviderError( "No DNS providers found for domain: {}".format(target_domain) ) diff --git a/lemur/plugins/lemur_acme/plugin.py b/lemur/plugins/lemur_acme/plugin.py index e4a821ca92..d0ecb436b8 100644 --- a/lemur/plugins/lemur_acme/plugin.py +++ b/lemur/plugins/lemur_acme/plugin.py @@ -22,7 +22,11 @@ from lemur.common.utils import check_validation, drop_last_cert_from_chain, csr_to_string from lemur.constants import CRLReason, EMAIL_RE from lemur.dns_providers import service as dns_provider_service -from lemur.exceptions import InvalidConfiguration +from lemur.exceptions import ( + ACMEAuthenticationError, + InvalidConfiguration, + PendingCertificateTerminalError, +) from lemur.extensions import metrics from lemur.plugins import lemur_acme as acme @@ -31,6 +35,36 @@ from lemur.plugins.lemur_acme.challenge_types import AcmeHttpChallenge, AcmeDnsChallenge +def _extract_http_status(e): + """Best-effort extraction of an upstream HTTP status code from an exception.""" + # requests.HTTPError + resp = getattr(e, "response", None) + if resp is not None and hasattr(resp, "status_code"): + return resp.status_code + # botocore ClientError + if isinstance(e, ClientError): + try: + return e.response["ResponseMetadata"]["HTTPStatusCode"] + except (KeyError, TypeError): + return None + # acme.messages.Error exposes a `status` attribute + status = getattr(e, "status", None) + if isinstance(status, int): + return status + return None + + +def _classify_pending_error(e): + """Wrap a raised error in a terminal exception type when it is known to be a + configuration/DNS-delegation/credential failure that retrying cannot resolve. + Returns the original error unchanged for transient failures.""" + if isinstance(e, PendingCertificateTerminalError): + return e + if _extract_http_status(e) in (401, 403): + return ACMEAuthenticationError(e) + return e + + class ACMEIssuerPlugin(IssuerPlugin): title = "Acme" slug = "acme-issuer" @@ -228,12 +262,12 @@ def get_ordered_certificates(self, pending_certs): current_app.logger.error( f"Unable to resolve pending cert: {pending_cert}", exc_info=True ) - - error = e - if globals().get("order") and order: - error += f" Order uri: {order.uri}" certs.append( - {"cert": False, "pending_cert": pending_cert, "last_error": e} + { + "cert": False, + "pending_cert": pending_cert, + "last_error": _classify_pending_error(e), + } ) for entry in pending: @@ -263,7 +297,6 @@ def get_ordered_certificates(self, pending_certs): capture_exception() metrics.send("get_ordered_certificates_resolution_error", "counter", 1) order_url = order.uri - error = f"{e}. Order URI: {order_url}" current_app.logger.error( f"Unable to resolve pending cert: {pending_cert}. " f"Check out {order_url} for more information.", @@ -273,7 +306,7 @@ def get_ordered_certificates(self, pending_certs): { "cert": False, "pending_cert": entry["pending_cert"], - "last_error": error, + "last_error": _classify_pending_error(e), } ) # Ensure DNS records get deleted diff --git a/lemur/plugins/lemur_acme/tests/test_acme_dns.py b/lemur/plugins/lemur_acme/tests/test_acme_dns.py index 9ad7494d10..4f4303060e 100644 --- a/lemur/plugins/lemur_acme/tests/test_acme_dns.py +++ b/lemur/plugins/lemur_acme/tests/test_acme_dns.py @@ -415,6 +415,35 @@ def test_get_authorizations(self, mock_start_dns_challenge): ) self.assertEqual(result, ["test"]) + def test_get_authorizations_delegated_cname_no_provider_terminal(self): + """A missing provider is terminal even when delegated CNAME is enabled.""" + from lemur.exceptions import NoDNSProviderError + + current_app.config["ACME_ENABLE_DELEGATED_CNAME"] = True + self.acme.get_cname = Mock(return_value=False) + self.acme.dns_providers_for_domain = {} + + mock_order_info = Mock() + mock_order_info.domains = ["test.fakedomain.net"] + + # No provider (and no resolvable delegation) is a terminal config error — + # retrying would only exhaust the CA rate limit, so fail fast. + with self.assertRaises(NoDNSProviderError): + self.acme.get_authorizations("acme_client", Mock(), mock_order_info) + + def test_get_authorizations_no_provider_terminal(self): + """Without delegated CNAME, a missing provider is a terminal NoDNSProviderError.""" + from lemur.exceptions import NoDNSProviderError + + current_app.config["ACME_ENABLE_DELEGATED_CNAME"] = False + self.acme.dns_providers_for_domain = {} + + mock_order_info = Mock() + mock_order_info.domains = ["test.fakedomain.net"] + + with self.assertRaises(NoDNSProviderError): + self.acme.get_authorizations("acme_client", Mock(), mock_order_info) + @patch( "lemur.plugins.lemur_acme.plugin.AcmeDnsHandler.complete_dns_challenge", return_value="test", diff --git a/lemur/tests/conftest.py b/lemur/tests/conftest.py index 899b91fb2b..419ed91ee2 100644 --- a/lemur/tests/conftest.py +++ b/lemur/tests/conftest.py @@ -5,7 +5,7 @@ from cryptography import x509 from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import hashes -from flask import current_app +from flask import current_app, g from flask_principal import identity_changed, Identity from sqlalchemy.sql import text @@ -104,6 +104,11 @@ def session(db, request): db.session.begin_nested() yield db.session db.session.rollback() + # g is app-context scoped and persists across tests. Clear request-scoped + # state so a stale, detached User from an earlier test isn't reused by a + # later test (which raises DetachedInstanceError when its attributes are + # expired). + g.pop("current_user", None) @pytest.fixture(scope="function") diff --git a/lemur/tests/test_pending_cert_retry.py b/lemur/tests/test_pending_cert_retry.py new file mode 100644 index 0000000000..c52503edf8 --- /dev/null +++ b/lemur/tests/test_pending_cert_retry.py @@ -0,0 +1,175 @@ +"""Tests for fetch_acme_cert fail-fast behavior on terminal pending-cert failures (EVBL-47). + +Verifies that a terminal (config / DNS-delegation / credential) failure marks the +pending certificate resolved immediately and does NOT re-queue, while a transient +failure keeps the existing bounded retry (increment + re-queue). +""" + +import sys +from unittest.mock import MagicMock, patch + +import pytest + +# celery.py connects to Redis at module level; pre-import it with Redis mocked so +# the @patch decorators below don't trigger a real Redis connection on first import. +if "lemur.common.celery" not in sys.modules: + with patch("redis.StrictRedis") as _mock_redis: + _mock_redis.return_value.set.return_value = True + import lemur.common.celery # noqa: F401 + +import lemur.common.celery as _celery_module # noqa: E402 + +from lemur.common.celery import fetch_acme_cert # noqa: E402 + + +@pytest.fixture(autouse=True) +def _mock_celery_current_app(monkeypatch): + """Scope the current_app mock to each test and restore the original on teardown.""" + monkeypatch.setattr(_celery_module, "current_app", MagicMock()) + + +def _pending_cert(id, number_attempts=0): + pc = MagicMock() + pc.id = id + pc.number_attempts = number_attempts + pc.resolved = False + pc.cn = "*.us3.ddbuild.io" + pc.notify = True + pc.owner = "joe@example.com" + return pc + + +def _run_fetch_acme_cert(pc, last_error, notify_side_effect=None): + """Run fetch_acme_cert(id) with get_ordered_certificates returning a single failure. + + Returns the started mocks keyed by name so tests can assert on call behavior. + """ + plugin = MagicMock() + plugin.get_ordered_certificates.return_value = [ + {"cert": False, "pending_cert": pc, "last_error": last_error} + ] + + patchers = [ + patch( + "lemur.common.celery.pending_certificate_service.get_pending_certs", + return_value=[pc], + ), + patch( + "lemur.common.celery.get_authority", + return_value=MagicMock(plugin_name="acme-issuer"), + ), + patch("lemur.common.celery.plugins.get", return_value=plugin), + patch("lemur.common.celery.pending_certificate_service.get", return_value=pc), + patch( + "lemur.common.celery.send_pending_failure_notification", + side_effect=notify_side_effect, + ), + patch("lemur.common.celery.pending_certificate_service.update"), + patch("lemur.common.celery.pending_certificate_service.increment_attempt"), + patch("lemur.common.celery.fetch_acme_cert.delay"), + ] + started = [p.start() for p in patchers] + try: + fetch_acme_cert(pc.id) + except Exception: + # Notification delivery (or other) failure. The helper still returns the + # mocks so tests can assert on persisted state after a raised notification. + pass + finally: + for p in patchers: + p.stop() + + return { + "get_pending_certs": started[0], + "get_authority": started[1], + "plugins.get": started[2], + "pending_certificate_service.get": started[3], + "send_pending_failure_notification": started[4], + "pending_certificate_service.update": started[5], + "pending_certificate_service.increment_attempt": started[6], + "fetch_acme_cert.delay": started[7], + } + + +def _assert_resolved(mocks): + """Assert the pending cert was marked resolved (regardless of status kwarg).""" + update_mock = mocks["pending_certificate_service.update"] + assert any( + call.kwargs.get("resolved") is True for call in update_mock.call_args_list + ) + + +def test_fetch_acme_cert_terminal_failure_marks_resolved_no_requeue(): + from lemur.exceptions import NoDNSProviderError + + pc = _pending_cert(1) + mocks = _run_fetch_acme_cert(pc, NoDNSProviderError("no provider for zone")) + + # Marked resolved + _assert_resolved(mocks) + # Notified + mocks["send_pending_failure_notification"].assert_called_once() + # Did NOT re-queue and did NOT increment attempts + mocks["fetch_acme_cert.delay"].assert_not_called() + mocks["pending_certificate_service.increment_attempt"].assert_not_called() + + +def test_fetch_acme_cert_transient_failure_requeues(): + pc = _pending_cert(1, number_attempts=0) + mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) + + # Incremented attempts and re-queued + mocks["pending_certificate_service.increment_attempt"].assert_called_once() + mocks["fetch_acme_cert.delay"].assert_called_once_with(1, None) + # Did NOT mark resolved + for call in mocks["pending_certificate_service.update"].call_args_list: + assert call.kwargs.get("resolved") is not True + + +def test_fetch_acme_cert_transient_at_max_attempts_resolves_no_requeue(): + """A transient failure at the retry cap resolves instead of re-queuing. + + The retry cap is ACME_ADDITIONAL_ATTEMPTS additional attempts beyond the + initial one (total = ACME_ADDITIONAL_ATTEMPTS + 1). Once number_attempts + reaches the cap, the next failure gives up and marks resolved rather than + burning another attempt against the CA's rate limit. + """ + from lemur.constants import ACME_ADDITIONAL_ATTEMPTS + + pc = _pending_cert(1, number_attempts=ACME_ADDITIONAL_ATTEMPTS) + mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) + + # At the cap: marked resolved, NOT re-queued, NOT incremented further + _assert_resolved(mocks) + mocks["fetch_acme_cert.delay"].assert_not_called() + mocks["pending_certificate_service.increment_attempt"].assert_not_called() + + +def test_fetch_acme_cert_terminal_typed_error_marks_resolved_no_requeue(): + """A typed terminal error is classified via isinstance and fails fast.""" + from lemur.exceptions import NoDNSProviderError + + pc = _pending_cert(1) + mocks = _run_fetch_acme_cert(pc, NoDNSProviderError("no provider for zone")) + + _assert_resolved(mocks) + mocks["send_pending_failure_notification"].assert_called_once() + mocks["fetch_acme_cert.delay"].assert_not_called() + mocks["pending_certificate_service.increment_attempt"].assert_not_called() + + +def test_fetch_acme_cert_terminal_persists_resolved_before_notify(): + """The pending cert is marked resolved even if notification delivery raises.""" + from lemur.exceptions import NoDNSProviderError + + pc = _pending_cert(1) + mocks = _run_fetch_acme_cert( + pc, + NoDNSProviderError("no provider for zone"), + notify_side_effect=RuntimeError("smtp down"), + ) + + # Still marked resolved (persisted before the notification was attempted) + _assert_resolved(mocks) + # Not re-queued despite the notification failure + mocks["fetch_acme_cert.delay"].assert_not_called() diff --git a/lemur/tests/test_pending_certificates.py b/lemur/tests/test_pending_certificates.py index dd56fda1b6..1cde85a57c 100644 --- a/lemur/tests/test_pending_certificates.py +++ b/lemur/tests/test_pending_certificates.py @@ -143,3 +143,102 @@ def test_invalid_pending_upload_with_chain(pending_certificate_from_partial_chai assert str(err.value).startswith( "Incorrect chain certificate(s) provided: '*.wild.example.org' is not signed by 'LemurTrust Unittests Root CA 2018" ) + + +def test_is_terminal_failure_invalid_configuration(): + from lemur.exceptions import InvalidConfiguration + from lemur.pending_certificates.service import is_terminal_failure + + assert is_terminal_failure(InvalidConfiguration("bad config")) + + +def test_is_terminal_failure_no_dns_provider(): + from lemur.exceptions import NoDNSProviderError + from lemur.pending_certificates.service import is_terminal_failure + + assert is_terminal_failure(NoDNSProviderError("no provider for us3.ddbuild.io")) + + +def test_is_terminal_failure_no_dns_challenges(): + from lemur.exceptions import DNSChallengeSetupError + from lemur.pending_certificates.service import is_terminal_failure + + assert is_terminal_failure( + DNSChallengeSetupError("Unable to determine DNS challenges from authorizations") + ) + + +def test_is_terminal_failure_auth(): + from lemur.exceptions import ACMEAuthenticationError + from lemur.pending_certificates.service import is_terminal_failure + + assert is_terminal_failure(ACMEAuthenticationError("unauthorized")) + assert is_terminal_failure(ACMEAuthenticationError("authentication failed")) + assert is_terminal_failure(ACMEAuthenticationError("HTTP 403")) + + +def test_is_terminal_failure_transient_is_false(): + from lemur.pending_certificates.service import is_terminal_failure + + # DNS verification / order-not-ready failures can resolve on retry. + assert not is_terminal_failure(ValueError("Failed verification")) + assert not is_terminal_failure(Exception("order is not ready")) + assert not is_terminal_failure(Exception("some unrelated transient error")) + + +def test_is_terminal_failure_none_is_false(): + from lemur.pending_certificates.service import is_terminal_failure + + # A None / empty error should never be treated as terminal. + assert not is_terminal_failure(None) + assert not is_terminal_failure("") + + +def test_is_terminal_failure_typed_exceptions(): + """Explicit exception types are terminal without any string matching.""" + from lemur.exceptions import ( + ACMEAuthenticationError, + DNSChallengeSetupError, + InvalidConfiguration, + NoDNSProviderError, + ) + from lemur.pending_certificates.service import is_terminal_failure + + assert is_terminal_failure(NoDNSProviderError("no provider")) + assert is_terminal_failure(ACMEAuthenticationError("401")) + assert is_terminal_failure(DNSChallengeSetupError("no challenges")) + assert is_terminal_failure(InvalidConfiguration("bad config")) + + +def test_classify_pending_error_http_auth(): + """Upstream HTTP 401/403 are translated into ACMEAuthenticationError.""" + from lemur.exceptions import ACMEAuthenticationError + from lemur.plugins.lemur_acme.plugin import _classify_pending_error + + class FakeResponse: + status_code = 401 + + class FakeHTTPError(Exception): + response = FakeResponse() + + err = _classify_pending_error(FakeHTTPError("unauthorized")) + assert isinstance(err, ACMEAuthenticationError) + # original exception is preserved as the cause + assert isinstance(err.__cause__, FakeHTTPError) or err.args + + +def test_classify_pending_error_keeps_terminal_type(): + """Already-typed terminal errors pass through unchanged.""" + from lemur.exceptions import NoDNSProviderError + from lemur.plugins.lemur_acme.plugin import _classify_pending_error + + err = NoDNSProviderError("no provider") + assert _classify_pending_error(err) is err + + +def test_classify_pending_error_transient_unchanged(): + """Transient failures are returned unchanged (not wrapped).""" + from lemur.plugins.lemur_acme.plugin import _classify_pending_error + + err = ValueError("Failed verification") + assert _classify_pending_error(err) is err