Preserve preferred-challenges on renewal (#4112) (#4116)

* 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

(cherry picked from commit 4d860b37b0cb106d8a1b85dad18db2bc9cd4fd89)

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 31b5bafb5..a61406c79 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
@@ -1201,13 +1202,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 85792ab13..77681e873 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
@@ -157,12 +159,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 07c4eac00..c7711bbfa 100644
--- a/certbot/tests/renewal_test.py
+++ b/certbot/tests/renewal_test.py
@@ -4,6 +4,8 @@ import mock
 import unittest
 import tempfile
 
+from acme import challenges
+
 from certbot import configuration
 from certbot import errors
 from certbot import storage
@@ -54,6 +56,29 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase):
             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
         self._call(self.config, {'must_staple': 'True'})
diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh
index fe2f223ef..6f52522f3 100755
--- a/tests/boulder-integration.sh
+++ b/tests/boulder-integration.sh
@@ -84,7 +84,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" \
@@ -101,29 +101,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
 
@@ -137,6 +138,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 \