From 8cf3bcd3f3120041d070b0fc8acb4029e1dd1b4b Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 9 Jan 2019 21:52:53 +0100 Subject: [PATCH] [Windows|Unix] Avoid to re-execute challenges already validated (#6551) In response to #5342. Currently, certbot will execute the operations necessary to validate a challenge even if the challenge has already been validated before against the acme ca server. This can occur for instance if a certificate is asked and issue correctly, then deleted locally, then asked again. It is a corner case, but it will lead to some heavy operations (like updating a DNS zone, or creating an HTTP server) that are not needed. This PR corrects this behavior by not executing challenges already validated, and use them directly instead to issue the certificate. Fixes #5342 * Avoid to execute a given challenge that have been already validated by acme ca server. * Execute tls challenge on a separate dns name, to avoid reusing the existing valid http challenge. * Align with master * Improve log * Simplify the implementation * Update changelog * Add a unit test to ensure that validated challenges are not rerun --- CHANGELOG.md | 4 ++- .../tests/boulder-integration.conf.sh | 3 +- certbot-nginx/tests/boulder-integration.sh | 4 +-- certbot/auth_handler.py | 31 ++++++++++++------- certbot/tests/auth_handler_test.py | 20 +++++++++++- 5 files changed, 45 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 724820356..07d00e3fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added -* +* Avoid to process again challenges that are already validated + when a certificate is issued. ### Changed @@ -22,6 +23,7 @@ all Certbot components during releases for the time being, however, the only package with changes other than its version number was: * acme +* certbot More details about these changes can be found on our GitHub repo. diff --git a/certbot-nginx/tests/boulder-integration.conf.sh b/certbot-nginx/tests/boulder-integration.conf.sh index 4374f9094..470eab28e 100755 --- a/certbot-nginx/tests/boulder-integration.conf.sh +++ b/certbot-nginx/tests/boulder-integration.conf.sh @@ -1,3 +1,4 @@ +#!/usr/bin/env bash # Based on # https://www.exratione.com/2014/03/running-nginx-as-a-non-root-user/ # https://github.com/exratione/non-root-nginx/blob/9a77f62e5d5cb9c9026fd62eece76b9514011019/nginx.conf @@ -52,7 +53,7 @@ http { listen 5002 $default_server; # IPv6. listen [::]:5002 $default_server; - server_name nginx.wtf nginx2.wtf; + server_name nginx.wtf nginx-tls.wtf nginx2.wtf; root $root/webroot; diff --git a/certbot-nginx/tests/boulder-integration.sh b/certbot-nginx/tests/boulder-integration.sh index 2a24e645f..03425734d 100755 --- a/certbot-nginx/tests/boulder-integration.sh +++ b/certbot-nginx/tests/boulder-integration.sh @@ -39,8 +39,8 @@ nginx -v reload_nginx certbot_test_nginx --domains nginx.wtf run test_deployment_and_rollback nginx.wtf -certbot_test_nginx --domains nginx.wtf run --preferred-challenges tls-sni -test_deployment_and_rollback nginx.wtf +certbot_test_nginx --domains nginx-tls.wtf run --preferred-challenges tls-sni +test_deployment_and_rollback nginx-tls.wtf certbot_test_nginx --domains nginx2.wtf --preferred-challenges http test_deployment_and_rollback nginx2.wtf # Overlapping location block and server-block-level return 301 diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index efee49143..3dfaaf26f 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -31,7 +31,7 @@ class AuthHandler(object): :class:`~acme.challenges.Challenge` types :type auth: :class:`certbot.interfaces.IAuthenticator` - :ivar acme.client.BackwardsCompatibleClientV2 acme: ACME client API. + :ivar acme.client.BackwardsCompatibleClientV2 acme_client: ACME client API. :ivar account: Client's Account :type account: :class:`certbot.account.Account` @@ -40,9 +40,9 @@ class AuthHandler(object): type strings with the most preferred challenge listed first """ - def __init__(self, auth, acme, account, pref_challs): + def __init__(self, auth, acme_client, account, pref_challs): self.auth = auth - self.acme = acme + self.acme = acme_client self.account = account self.pref_challs = pref_challs @@ -85,19 +85,26 @@ class AuthHandler(object): self.verify_authzr_complete(aauthzrs) # Only return valid authorizations - retVal = [aauthzr.authzr for aauthzr in aauthzrs - if aauthzr.authzr.body.status == messages.STATUS_VALID] + ret_val = [aauthzr.authzr for aauthzr in aauthzrs + if aauthzr.authzr.body.status == messages.STATUS_VALID] - if not retVal: + if not ret_val: raise errors.AuthorizationError( "Challenges failed for all domains") - return retVal + return ret_val def _choose_challenges(self, aauthzrs): - """Retrieve necessary challenges to satisfy server.""" - logger.info("Performing the following challenges:") - for aauthzr in aauthzrs: + """ + Retrieve necessary and pending challenges to satisfy server. + NB: Necessary and already validated challenges are not retrieved, + as they can be reused for a certificate issuance. + """ + pending_authzrs = [aauthzr for aauthzr in aauthzrs + if aauthzr.authzr.body.status != messages.STATUS_VALID] + if pending_authzrs: + logger.info("Performing the following challenges:") + for aauthzr in pending_authzrs: aauthzr_challenges = aauthzr.authzr.body.challenges if self.acme.acme_version == 1: combinations = aauthzr.authzr.body.combinations @@ -125,7 +132,7 @@ class AuthHandler(object): def _solve_challenges(self, aauthzrs): """Get Responses for challenges from authenticators.""" - resp = [] # type: Collection[acme.challenges.ChallengeResponse] + resp = [] # type: Collection[challenges.ChallengeResponse] all_achalls = self._get_all_achalls(aauthzrs) try: if all_achalls: @@ -531,7 +538,7 @@ def _report_failed_challs(failed_achalls): """ problems = collections.defaultdict(list)\ - # type: DefaultDict[str, List[achallenges.KeyAuthorizationAnnotatedChallenge]] + # type: DefaultDict[str, List[achallenges.KeyAuthorizationAnnotatedChallenge]] for achall in failed_achalls: if achall.error: problems[achall.error.typ].append(achall) diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index e1319b614..fe0ece12e 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -57,7 +57,7 @@ class ChallengeFactoryTest(unittest.TestCase): errors.Error, self.handler._challenge_factory, authzr, [0]) -class HandleAuthorizationsTest(unittest.TestCase): +class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-public-methods """handle_authorizations test. This tests everything except for all functions under _poll_challenges. @@ -316,6 +316,24 @@ class HandleAuthorizationsTest(unittest.TestCase): self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01") + def test_validated_challenge_not_rerun(self): + # With pending challenge, we expect the challenge to be tried, and fail. + authzr = acme_util.gen_authzr( + messages.STATUS_PENDING, "0", + [acme_util.HTTP01], + [messages.STATUS_PENDING], False) + mock_order = mock.MagicMock(authorizations=[authzr]) + self.assertRaises( + errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + + # With validated challenge; we expect the challenge not be tried again, and succeed. + authzr = acme_util.gen_authzr( + messages.STATUS_VALID, "0", + [acme_util.HTTP01], + [messages.STATUS_VALID], False) + mock_order = mock.MagicMock(authorizations=[authzr]) + self.handler.handle_authorizations(mock_order) + def _validate_all(self, aauthzrs, unused_1, unused_2): for i, aauthzr in enumerate(aauthzrs): azr = aauthzr.authzr