From 4d860b37b0cb106d8a1b85dad18db2bc9cd4fd89 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 Jan 2017 18:40:22 -0800 Subject: [PATCH] Preserve preferred-challenges on renewal (#4112) * use challenge type strings, not objectS * Factor out parse_preferred_challenges * restore pref_challs * save pref_challs * Make CheckCertCount more flexible * improve integration tests * Make pref_challs more flexible --- certbot/auth_handler.py | 9 ++++--- certbot/cli.py | 39 ++++++++++++++++++++++-------- certbot/renewal.py | 25 ++++++++++++++++++- certbot/tests/auth_handler_test.py | 5 ++-- certbot/tests/cli_test.py | 8 +++--- certbot/tests/renewal_test.py | 25 +++++++++++++++++++ tests/boulder-integration.sh | 31 +++++++++++++++--------- 7 files changed, 110 insertions(+), 32 deletions(-) diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index aad971eb6..6e9ab25a7 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -34,8 +34,7 @@ class AuthHandler(object): :ivar list achalls: DV challenges in the form of :class:`certbot.achallenges.AnnotatedChallenge` :ivar list pref_challs: sorted user specified preferred challenges - in the form of subclasses of :class:`acme.challenges.Challenge` - with the most preferred challenge listed first + type strings with the most preferred challenge listed first """ def __init__(self, auth, acme, account, pref_challs): @@ -252,8 +251,10 @@ class AuthHandler(object): # Make sure to make a copy... plugin_pref = self.auth.get_chall_pref(domain) if self.pref_challs: - chall_prefs.extend(pref for pref in self.pref_challs - if pref in plugin_pref) + plugin_pref_types = set(chall.typ for chall in plugin_pref) + for typ in self.pref_challs: + if typ in plugin_pref_types: + chall_prefs.append(challenges.Challenge.TYPES[typ]) if chall_prefs: return chall_prefs raise errors.AuthorizationError( diff --git a/certbot/cli.py b/certbot/cli.py index 7d146e33b..88d6d39db 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1,4 +1,5 @@ """Certbot command line argument & config processing.""" +# pylint: disable=too-many-lines from __future__ import print_function import argparse import copy @@ -1236,13 +1237,31 @@ class _PrefChallAction(argparse.Action): """Action class for parsing preferred challenges.""" def __call__(self, parser, namespace, pref_challs, option_string=None): - aliases = {"dns": "dns-01", "http": "http-01", "tls-sni": "tls-sni-01"} - challs = [c.strip() for c in pref_challs.split(",")] - challs = [aliases[c] if c in aliases else c for c in challs] - unrecognized = ", ".join(name for name in challs - if name not in challenges.Challenge.TYPES) - if unrecognized: - raise argparse.ArgumentTypeError( - "Unrecognized challenges: {0}".format(unrecognized)) - namespace.pref_challs.extend(challenges.Challenge.TYPES[name] - for name in challs) + try: + challs = parse_preferred_challenges(pref_challs.split(",")) + except errors.Error as error: + raise argparse.ArgumentTypeError(str(error)) + namespace.pref_challs.extend(challs) + + +def parse_preferred_challenges(pref_challs): + """Translate and validate preferred challenges. + + :param pref_challs: list of preferred challenge types + :type pref_challs: `list` of `str` + + :returns: validated list of preferred challenge types + :rtype: `list` of `str` + + :raises errors.Error: if pref_challs is invalid + + """ + aliases = {"dns": "dns-01", "http": "http-01", "tls-sni": "tls-sni-01"} + challs = [c.strip() for c in pref_challs] + challs = [aliases.get(c, c) for c in challs] + unrecognized = ", ".join(name for name in challs + if name not in challenges.Challenge.TYPES) + if unrecognized: + raise errors.Error( + "Unrecognized challenges: {0}".format(unrecognized)) + return challs diff --git a/certbot/renewal.py b/certbot/renewal.py index bd07cfd07..6b61b0841 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -35,7 +35,7 @@ INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"] BOOL_CONFIG_ITEMS = ["must_staple", "allow_subset_of_names"] CONFIG_ITEMS = set(itertools.chain( - BOOL_CONFIG_ITEMS, INT_CONFIG_ITEMS, STR_CONFIG_ITEMS)) + BOOL_CONFIG_ITEMS, INT_CONFIG_ITEMS, STR_CONFIG_ITEMS, ('pref_challs',))) def _reconstitute(config, full_path): @@ -165,6 +165,7 @@ def restore_required_config_elements(config, renewalparams): """ required_items = itertools.chain( + (("pref_challs", _restore_pref_challs),), six.moves.zip(BOOL_CONFIG_ITEMS, itertools.repeat(_restore_bool)), six.moves.zip(INT_CONFIG_ITEMS, itertools.repeat(_restore_int)), six.moves.zip(STR_CONFIG_ITEMS, itertools.repeat(_restore_str))) @@ -174,6 +175,28 @@ def restore_required_config_elements(config, renewalparams): setattr(config.namespace, item_name, value) +def _restore_pref_challs(unused_name, value): + """Restores preferred challenges from a renewal config file. + + If value is a `str`, it should be a single challenge type. + + :param str unused_name: option name + :param value: option value + :type value: `list` of `str` or `str` + + :returns: converted option value to be stored in the runtime config + :rtype: `list` of `str` + + :raises errors.Error: if value can't be converted to an bool + + """ + # If pref_challs has only one element, configobj saves the value + # with a trailing comma so it's parsed as a list. If this comma is + # removed by the user, the value is parsed as a str. + value = [value] if isinstance(value, str) else value + return cli.parse_preferred_challenges(value) + + def _restore_bool(name, value): """Restores an boolean key-value pair from a renewal config file. diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 441550fc8..046eb5ef1 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -176,7 +176,8 @@ class GetAuthorizationsTest(unittest.TestCase): mock_poll.side_effect = self._validate_all self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) - self.handler.pref_challs.extend((challenges.HTTP01, challenges.DNS01,)) + self.handler.pref_challs.extend((challenges.HTTP01.typ, + challenges.DNS01.typ,)) self.handler.get_authorizations(["0"]) @@ -187,7 +188,7 @@ class GetAuthorizationsTest(unittest.TestCase): def test_preferred_challenges_not_supported(self): self.mock_net.request_domain_challenges.side_effect = functools.partial( gen_dom_authzr, challs=acme_util.CHALLENGES) - self.handler.pref_challs.append(challenges.HTTP01) + self.handler.pref_challs.append(challenges.HTTP01.typ) self.assertRaises( errors.AuthorizationError, self.handler.get_authorizations, ["0"]) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 1fa2004d3..aff1bcc99 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -8,6 +8,8 @@ import mock import six from six.moves import reload_module # pylint: disable=import-error +from acme import challenges + from certbot import cli from certbot import constants from certbot import errors @@ -178,12 +180,12 @@ class ParseTest(unittest.TestCase): self.assertEqual(namespace.domains, ['example.com', 'another.net']) def test_preferred_challenges(self): - from acme.challenges import HTTP01, TLSSNI01, DNS01 - short_args = ['--preferred-challenges', 'http, tls-sni-01, dns'] namespace = self.parse(short_args) - self.assertEqual(namespace.pref_challs, [HTTP01, TLSSNI01, DNS01]) + expected = [challenges.HTTP01.typ, + challenges.TLSSNI01.typ, challenges.DNS01.typ] + self.assertEqual(namespace.pref_challs, expected) short_args = ['--preferred-challenges', 'jumping-over-the-moon'] self.assertRaises(argparse.ArgumentTypeError, self.parse, short_args) diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index 2fe4eab68..cd53aa91c 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -5,6 +5,8 @@ import unittest import shutil import tempfile +from acme import challenges + from certbot import configuration from certbot import errors from certbot import storage @@ -59,6 +61,29 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase): self.assertRaises( errors.Error, self._call, self.config, renewalparams) + @mock.patch('certbot.renewal.cli.set_by_cli') + def test_pref_challs_list(self, mock_set_by_cli): + mock_set_by_cli.return_value = False + renewalparams = {'pref_challs': 'tls-sni, http-01, dns'.split(',')} + self._call(self.config, renewalparams) + expected = [challenges.TLSSNI01.typ, + challenges.HTTP01.typ, challenges.DNS01.typ] + self.assertEqual(self.config.namespace.pref_challs, expected) + + @mock.patch('certbot.renewal.cli.set_by_cli') + def test_pref_challs_str(self, mock_set_by_cli): + mock_set_by_cli.return_value = False + renewalparams = {'pref_challs': 'dns'} + self._call(self.config, renewalparams) + expected = [challenges.DNS01.typ] + self.assertEqual(self.config.namespace.pref_challs, expected) + + @mock.patch('certbot.renewal.cli.set_by_cli') + def test_pref_challs_failure(self, mock_set_by_cli): + mock_set_by_cli.return_value = False + renewalparams = {'pref_challs': 'finding-a-shrubbery'} + self.assertRaises(errors.Error, self._call, self.config, renewalparams) + @mock.patch('certbot.renewal.cli.set_by_cli') def test_must_staple_success(self, mock_set_by_cli): mock_set_by_cli.return_value = False diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 9c6bc7708..930cf1c0e 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -96,7 +96,7 @@ common certonly -a manual -d le.wtf --rsa-key-size 4096 \ --pre-hook 'echo wtf2.pre >> "$HOOK_TEST"' \ --post-hook 'echo wtf2.post >> "$HOOK_TEST"' -common certonly -a manual -d dns.le.wtf --preferred-challenges dns-01 \ +common certonly -a manual -d dns.le.wtf --preferred-challenges dns,tls-sni \ --manual-auth-hook ./tests/manual-dns-auth.sh export CSR_PATH="${root}/csr.der" KEY_PATH="${root}/key.pem" \ @@ -113,29 +113,30 @@ common --domains le3.wtf install \ --key-path "${root}/csr/key.pem" CheckCertCount() { - CERTCOUNT=`ls "${root}/conf/archive/le.wtf/cert"* | wc -l` - if [ "$CERTCOUNT" -ne "$1" ] ; then - echo Wrong cert count, not "$1" `ls "${root}/conf/archive/le.wtf/"*` + CERTCOUNT=`ls "${root}/conf/archive/$1/cert"* | wc -l` + if [ "$CERTCOUNT" -ne "$2" ] ; then + echo Wrong cert count, not "$2" `ls "${root}/conf/archive/$1/"*` exit 1 fi } -CheckCertCount 1 +CheckCertCount "le.wtf" 1 # This won't renew (because it's not time yet) common_no_force_renew renew -CheckCertCount 1 +CheckCertCount "le.wtf" 1 -# --renew-by-default is used, so renewal should occur -[ -f "$HOOK_TEST" ] && rm -f "$HOOK_TEST" -common renew -CheckCertCount 2 -CheckHooks +# renew using HTTP manual auth hooks +common renew --cert-name le.wtf --authenticator manual +CheckCertCount "le.wtf" 2 +# renew using DNS manual auth hooks +common renew --cert-name dns.le.wtf --authenticator manual +CheckCertCount "dns.le.wtf" 2 # This will renew because the expiry is less than 10 years from now sed -i "4arenew_before_expiry = 4 years" "$root/conf/renewal/le.wtf.conf" common_no_force_renew renew --rsa-key-size 2048 -CheckCertCount 3 +CheckCertCount "le.wtf" 3 # The 4096 bit setting should persist to the first renewal, but be overriden in the second @@ -149,6 +150,12 @@ if [ "$size1" -lt 3000 ] || [ "$size2" -lt 3000 ] || [ "$size3" -gt 1800 ] ; the exit 1 fi +# --renew-by-default is used, so renewal should occur +[ -f "$HOOK_TEST" ] && rm -f "$HOOK_TEST" +common renew +CheckCertCount "le.wtf" 4 +CheckHooks + # ECDSA openssl ecparam -genkey -name secp384r1 -out "${root}/privkey-p384.pem" SAN="DNS:ecdsa.le.wtf" openssl req -new -sha256 \