From 035a6dcc39bddb445ef62b686bba04b8bfa665ce Mon Sep 17 00:00:00 2001 From: ohemorange Date: Fri, 20 Jun 2025 07:42:20 -0700 Subject: [PATCH] Actually set FAILED_DOMAINS and RENEWED_DOMAINS variables when renewals fail (#10347) Fixes https://github.com/certbot/certbot/issues/10259 This PR moves post-hook execution from `main.renew` to `renewal.handle_renewal_request` so that failed and renewed domains actually get passed into post-hook execution as promised, even when failures happened. I suspect the original PR was being overly cautious by putting the whole thing into a try/finally so that post-hooks definitely happen, but `handle_renewal_request` is already full of exception catching. I understand the worry about executing a pre-hook and then failing to execute its matching post-hook, but the code really is already structured to make sure that that won't happen. And then when we added `FAILED_DOMAINS` and `RENEWED_DOMAINS`, we both kept that overly-cautious hooks execution location, but also kept the error so we have a summary at the end...which meant that if failures happened, the env vars were never set. If we really want to keep the `hooks.run_saved_post_hooks` call on the outside of everything in main, we can, but then we will have to do one of the following: - pass in the output lists to be filled out during execution. not my favorite pattern - throw the output lists in the error object or make a wrapper error, not sure, haven't looked at `errors.py` too closely - stop raising that final error where we report failures at the very bottom. it's a little outdated maybe but I do like it and I think people are used to it - raise that error in main, returning the number of parse and renewal failures. this is my favorite of the options, but I still like it less than what I've implemented here. Here's the integration/regression test failing on main: https://dev.azure.com/certbot/certbot/_build/results?buildId=9237&view=logs&j=fca58cec-e7ce-563a-f36f-5c233894d750 You can see here that that branch just has the integration test without the fix (and removing other tests for efficiency): https://github.com/certbot/certbot/compare/main...test-fail-env-on-main It's the default, but just to be clear, this should definitely have two reviewers. --- .../certbot_tests/test_main.py | 34 +++++++++++++++++++ certbot/CHANGELOG.md | 3 +- certbot/src/certbot/_internal/main.py | 8 +---- certbot/src/certbot/_internal/renewal.py | 8 ++--- 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py b/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py index 6f5ee4ebc..a33d1422d 100644 --- a/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py @@ -442,6 +442,40 @@ def test_renew_hook_override(context: IntegrationTestsContext) -> None: assert_hook_execution(context.hook_probe, 'deploy_override') +def test_renew_hook_env_vars(context: IntegrationTestsContext) -> None: + fail_domain = context.get_domain('fail-env') + context.certbot([ + 'certonly', '-d', fail_domain, + '--preferred-challenges', 'http-01' + ]) + + context.certbot([ + 'renew', + '--post-hook', f'printenv RENEWED_DOMAINS >> {context.hook_probe}' + ]) + + assert_hook_execution(context.hook_probe, fail_domain) + + # Clear probe + with open(context.hook_probe, 'w'): + pass + + # now renew using manual dns, which will fail on renew + # manual_dns_auth_hook from misc is designed to fail if the domain contains 'fail-*'. + with pytest.raises(subprocess.CalledProcessError): + context.certbot([ + 'renew', '--cert-name', fail_domain, + '--preferred-challenges', 'dns', + '--manual-auth-hook', context.manual_dns_auth_hook, + '--manual-cleanup-hook', context.manual_dns_cleanup_hook, + '-a', 'manual', + '--post-hook', f'printenv FAILED_DOMAINS >> {context.hook_probe}', + '--dry-run', # use dry run here to deactivate previous authz, or this will pass + ]) + + assert_hook_execution(context.hook_probe, fail_domain) + + def test_invalid_domain_with_dns_challenge(context: IntegrationTestsContext) -> None: """Test certificate issuance failure with DNS-01 challenge.""" # Manual dns auth hooks from misc are designed to fail if the domain contains 'fail-*'. diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 2d4c118d3..1a8f5929d 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -14,7 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed -* +* Previously, we claimed to set FAILED_DOMAINS and RENEWED_DOMAINS env variables for use by + post-hooks when certificate renewals fail, but we were not actually setting them. Now, we are. More details about these changes can be found on our GitHub repo. diff --git a/certbot/src/certbot/_internal/main.py b/certbot/src/certbot/_internal/main.py index fcac33fd4..3d81f0cfc 100644 --- a/certbot/src/certbot/_internal/main.py +++ b/certbot/src/certbot/_internal/main.py @@ -1620,13 +1620,7 @@ def renew(config: configuration.NamespaceConfig, :rtype: None """ - - renewed_domains: List[str] = [] - failed_domains: List[str] = [] - try: - renewed_domains, failed_domains = renewal.handle_renewal_request(config) - finally: - hooks.run_saved_post_hooks(renewed_domains, failed_domains) + renewal.handle_renewal_request(config) def make_or_verify_needed_dirs(config: configuration.NamespaceConfig) -> None: diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index 19976e881..8536368e2 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -14,7 +14,6 @@ from typing import Iterable from typing import List from typing import Mapping from typing import Optional -from typing import Tuple from typing import Union from cryptography.hazmat.backends import default_backend @@ -553,7 +552,7 @@ def _renew_describe_results(config: configuration.NamespaceConfig, renew_success notify(display_obj.SIDE_FRAME) -def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list, list]: +def handle_renewal_request(config: configuration.NamespaceConfig) -> None: """Examine each lineage; renew if due and report results""" # This is trivially False if config.domains is empty @@ -596,6 +595,7 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list, for renewal_file in conf_files: display_util.notification("Processing " + renewal_file, pause=False) lineage_config = copy.deepcopy(config) + assert renewal_file.endswith(".conf") # make sure lineagename_for_filename will not error lineagename = storage.lineagename_for_filename(renewal_file) # Note that this modifies config (to add back the configuration @@ -659,14 +659,14 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list, _renew_describe_results(config, renew_successes, renew_failures, renew_skipped, parse_failures) + hooks.run_saved_post_hooks(renewed_domains, failed_domains) + if renew_failures or parse_failures: raise errors.Error( f"{len(renew_failures)} renew failure(s), {len(parse_failures)} parse failure(s)") logger.debug("no renewal failures") - return (renewed_domains, failed_domains) - def _update_renewal_params_from_key(key_path: str, config: configuration.NamespaceConfig) -> None: with open(key_path, 'rb') as file_h: