From 388baa5a1eb49f22187548a6a7dee5d38dfe98aa Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 12:29:31 -0800
Subject: [PATCH 01/29] Start splitting renew.py out of cli.py

---
 letsencrypt/cli.py            | 306 +---------------------------------
 letsencrypt/main.py           |  14 +-
 letsencrypt/plugins/common.py |   2 +-
 letsencrypt/renew.py          | 304 +++++++++++++++++++++++++++++++++
 letsencrypt/tests/cli_test.py |  24 +--
 5 files changed, 333 insertions(+), 317 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index b76311777..e34bd971b 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -2,7 +2,6 @@
 # pylint: disable=too-many-lines
 from __future__ import print_function
 import argparse
-import copy
 import glob
 import json
 import logging
@@ -14,19 +13,14 @@ import traceback
 import configargparse
 import OpenSSL
 import six
-import zope.component
-import zope.interface.exceptions
-import zope.interface.verify
 
 import letsencrypt
 
-from letsencrypt import configuration
 from letsencrypt import constants
 from letsencrypt import crypto_util
 from letsencrypt import errors
 from letsencrypt import interfaces
 from letsencrypt import le_util
-from letsencrypt import storage
 
 from letsencrypt.display import ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
@@ -35,16 +29,7 @@ from letsencrypt.plugins import disco as plugins_disco
 logger = logging.getLogger(__name__)
 
 # Global, to save us from a lot of argument passing within the scope of this module
-_parser = None
-
-# These are the items which get pulled out of a renewal configuration
-# file's renewalparams and actually used in the client configuration
-# during the renewal process. We have to record their types here because
-# the renewal configuration process loses this information.
-STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent",
-                    "server", "account", "authenticator", "installer",
-                    "standalone_supported_challenges"]
-INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"]
+helpful_parser = None
 
 # For help strings, figure out how the user ran us.
 # When invoked from letsencrypt-auto, sys.argv[0] is something like:
@@ -115,21 +100,6 @@ def usage_strings(plugins):
     return USAGE % (apache_doc, nginx_doc), SHORT_USAGE
 
 
-def should_renew(config, lineage):
-    "Return true if any of the circumstances for automatic renewal apply."
-    if config.renew_by_default:
-        logger.info("Auto-renewal forced with --force-renewal...")
-        return True
-    if lineage.should_autorenew(interactive=True):
-        logger.info("Cert is due for renewal, auto-renewing...")
-        return True
-    if config.dry_run:
-        logger.info("Cert not due for renewal, but simulating renewal for dry run")
-        return True
-    logger.info("Cert not yet due for renewal")
-    return False
-
-
 def diagnose_configurator_problem(cfg_type, requested, plugins):
     """
     Raise the most helpful error message about a plugin being unavailable
@@ -271,20 +241,20 @@ def record_chosen_plugins(config, plugins, auth, inst):
     cn.installer = plugins.find_init(inst).name if inst else "none"
 
 
-def _set_by_cli(var):
+def set_by_cli(var):
     """
     Return True if a particular config variable has been set by the user
     (CLI or config file) including if the user explicitly set it to the
     default.  Returns False if the variable was assigned a default value.
     """
-    detector = _set_by_cli.detector
+    detector = set_by_cli.detector
     if detector is None:
         # Setup on first run: `detector` is a weird version of config in which
         # the default value of every attribute is wrangled to be boolean-false
         plugins = plugins_disco.PluginsRegistry.find_all()
         # reconstructed_args == sys.argv[1:], or whatever was passed to main()
-        reconstructed_args = _parser.args + [_parser.verb]
-        detector = _set_by_cli.detector = prepare_and_parse_args(
+        reconstructed_args = helpful_parser.args + [helpful_parser.verb]
+        detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
         auth, inst = cli_plugin_requests(detector)
@@ -312,265 +282,7 @@ def _set_by_cli(var):
     else:
         return False
 # static housekeeping var
-_set_by_cli.detector = None
-
-def _restore_required_config_elements(config, renewalparams):
-    """Sets non-plugin specific values in config from renewalparams
-
-    :param configuration.NamespaceConfig config: configuration for the
-        current lineage
-    :param configobj.Section renewalparams: parameters from the renewal
-        configuration file that defines this lineage
-
-    """
-    # string-valued items to add if they're present
-    for config_item in STR_CONFIG_ITEMS:
-        if config_item in renewalparams and not _set_by_cli(config_item):
-            value = renewalparams[config_item]
-            # Unfortunately, we've lost type information from ConfigObj,
-            # so we don't know if the original was NoneType or str!
-            if value == "None":
-                value = None
-            setattr(config.namespace, config_item, value)
-    # int-valued items to add if they're present
-    for config_item in INT_CONFIG_ITEMS:
-        if config_item in renewalparams and not _set_by_cli(config_item):
-            config_value = renewalparams[config_item]
-            # the default value for http01_port was None during private beta
-            if config_item == "http01_port" and config_value == "None":
-                logger.info("updating legacy http01_port value")
-                int_value = flag_default("http01_port")
-            else:
-                try:
-                    int_value = int(config_value)
-                except ValueError:
-                    raise errors.Error(
-                        "Expected a numeric value for {0}".format(config_item))
-            setattr(config.namespace, config_item, int_value)
-
-
-def _restore_plugin_configs(config, renewalparams):
-    """Sets plugin specific values in config from renewalparams
-
-    :param configuration.NamespaceConfig config: configuration for the
-        current lineage
-    :param configobj.Section renewalparams: Parameters from the renewal
-        configuration file that defines this lineage
-
-    """
-    # Now use parser to get plugin-prefixed items with correct types
-    # XXX: the current approach of extracting only prefixed items
-    #      related to the actually-used installer and authenticator
-    #      works as long as plugins don't need to read plugin-specific
-    #      variables set by someone else (e.g., assuming Apache
-    #      configurator doesn't need to read webroot_ variables).
-    # Note: if a parameter that used to be defined in the parser is no
-    #      longer defined, stored copies of that parameter will be
-    #      deserialized as strings by this logic even if they were
-    #      originally meant to be some other type.
-    if renewalparams["authenticator"] == "webroot":
-        _restore_webroot_config(config, renewalparams)
-        plugin_prefixes = []
-    else:
-        plugin_prefixes = [renewalparams["authenticator"]]
-
-    if renewalparams.get("installer", None) is not None:
-        plugin_prefixes.append(renewalparams["installer"])
-    for plugin_prefix in set(plugin_prefixes):
-        for config_item, config_value in six.iteritems(renewalparams):
-            if config_item.startswith(plugin_prefix + "_") and not _set_by_cli(config_item):
-                # Values None, True, and False need to be treated specially,
-                # As they don't get parsed correctly based on type
-                if config_value in ("None", "True", "False"):
-                    # bool("False") == True
-                    # pylint: disable=eval-used
-                    setattr(config.namespace, config_item, eval(config_value))
-                    continue
-                for action in _parser.parser._actions:  # pylint: disable=protected-access
-                    if action.type is not None and action.dest == config_item:
-                        setattr(config.namespace, config_item,
-                                action.type(config_value))
-                        break
-                else:
-                    setattr(config.namespace, config_item, str(config_value))
-
-def _restore_webroot_config(config, renewalparams):
-    """
-    webroot_map is, uniquely, a dict, and the general-purpose configuration
-    restoring logic is not able to correctly parse it from the serialized
-    form.
-    """
-    if "webroot_map" in renewalparams:
-        # if the user does anything that would create a new webroot map on the
-        # CLI, don't use the old one
-        if not (_set_by_cli("webroot_map") or _set_by_cli("webroot_path")):
-            setattr(config.namespace, "webroot_map", renewalparams["webroot_map"])
-    elif "webroot_path" in renewalparams:
-        logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path")
-        wp = renewalparams["webroot_path"]
-        if isinstance(wp, str):  # prior to 0.1.0, webroot_path was a string
-            wp = [wp]
-        setattr(config.namespace, "webroot_path", wp)
-
-
-def _reconstitute(config, full_path):
-    """Try to instantiate a RenewableCert, updating config with relevant items.
-
-    This is specifically for use in renewal and enforces several checks
-    and policies to ensure that we can try to proceed with the renwal
-    request. The config argument is modified by including relevant options
-    read from the renewal configuration file.
-
-    :param configuration.NamespaceConfig config: configuration for the
-        current lineage
-    :param str full_path: Absolute path to the configuration file that
-        defines this lineage
-
-    :returns: the RenewableCert object or None if a fatal error occurred
-    :rtype: `storage.RenewableCert` or NoneType
-
-    """
-    try:
-        renewal_candidate = storage.RenewableCert(
-            full_path, configuration.RenewerConfiguration(config))
-    except (errors.CertStorageError, IOError):
-        logger.warning("Renewal configuration file %s is broken. Skipping.", full_path)
-        logger.debug("Traceback was:\n%s", traceback.format_exc())
-        return None
-    if "renewalparams" not in renewal_candidate.configuration:
-        logger.warning("Renewal configuration file %s lacks "
-                       "renewalparams. Skipping.", full_path)
-        return None
-    renewalparams = renewal_candidate.configuration["renewalparams"]
-    if "authenticator" not in renewalparams:
-        logger.warning("Renewal configuration file %s does not specify "
-                       "an authenticator. Skipping.", full_path)
-        return None
-    # Now restore specific values along with their data types, if
-    # those elements are present.
-    try:
-        _restore_required_config_elements(config, renewalparams)
-        _restore_plugin_configs(config, renewalparams)
-    except (ValueError, errors.Error) as error:
-        logger.warning(
-            "An error occured while parsing %s. The error was %s. "
-            "Skipping the file.", full_path, error.message)
-        logger.debug("Traceback was:\n%s", traceback.format_exc())
-        return None
-
-    try:
-        for d in renewal_candidate.names():
-            process_domain(config, d)
-    except errors.ConfigurationError as error:
-        logger.warning("Renewal configuration file %s references a cert "
-                       "that contains an invalid domain name. The problem "
-                       "was: %s. Skipping.", full_path, error)
-        return None
-
-    return renewal_candidate
-
-def _renewal_conf_files(config):
-    """Return /path/to/*.conf in the renewal conf directory"""
-    return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf"))
-
-
-def _renew_describe_results(config, renew_successes, renew_failures,
-                            renew_skipped, parse_failures):
-    status = lambda x, msg: "  " + "\n  ".join(i + " (" + msg +")" for i in x)
-    if config.dry_run:
-        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
-        print("**          (The test certificates below have not been saved.)")
-    print()
-    if renew_skipped:
-        print("The following certs are not due for renewal yet:")
-        print(status(renew_skipped, "skipped"))
-    if not renew_successes and not renew_failures:
-        print("No renewals were attempted.")
-    elif renew_successes and not renew_failures:
-        print("Congratulations, all renewals succeeded. The following certs "
-              "have been renewed:")
-        print(status(renew_successes, "success"))
-    elif renew_failures and not renew_successes:
-        print("All renewal attempts failed. The following certs could not be "
-              "renewed:")
-        print(status(renew_failures, "failure"))
-    elif renew_failures and renew_successes:
-        print("The following certs were successfully renewed:")
-        print(status(renew_successes, "success"))
-        print("\nThe following certs could not be renewed:")
-        print(status(renew_failures, "failure"))
-
-    if parse_failures:
-        print("\nAdditionally, the following renewal configuration files "
-              "were invalid: ")
-        print(status(parse_failures, "parsefail"))
-
-    if config.dry_run:
-        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
-        print("**          (The test certificates above have not been saved.)")
-
-
-def renew(config, unused_plugins):
-    """Renew previously-obtained certificates."""
-
-    if config.domains != []:
-        raise errors.Error("Currently, the renew verb is only capable of "
-                           "renewing all installed certificates that are due "
-                           "to be renewed; individual domains cannot be "
-                           "specified with this action. If you would like to "
-                           "renew specific certificates, use the certonly "
-                           "command. The renew verb may provide other options "
-                           "for selecting certificates to renew in the future.")
-    renewer_config = configuration.RenewerConfiguration(config)
-    renew_successes = []
-    renew_failures = []
-    renew_skipped = []
-    parse_failures = []
-    for renewal_file in _renewal_conf_files(renewer_config):
-        print("Processing " + renewal_file)
-        lineage_config = copy.deepcopy(config)
-
-        # Note that this modifies config (to add back the configuration
-        # elements from within the renewal configuration file).
-        try:
-            renewal_candidate = _reconstitute(lineage_config, renewal_file)
-        except Exception as e:  # pylint: disable=broad-except
-            logger.warning("Renewal configuration file %s produced an "
-                           "unexpected error: %s. Skipping.", renewal_file, e)
-            logger.debug("Traceback was:\n%s", traceback.format_exc())
-            parse_failures.append(renewal_file)
-            continue
-
-        try:
-            if renewal_candidate is None:
-                parse_failures.append(renewal_file)
-            else:
-                # XXX: ensure that each call here replaces the previous one
-                zope.component.provideUtility(lineage_config)
-                if should_renew(lineage_config, renewal_candidate):
-                    plugins = plugins_disco.PluginsRegistry.find_all()
-                    from letsencrypt import main
-                    main.obtain_cert(lineage_config, plugins, renewal_candidate)
-                    renew_successes.append(renewal_candidate.fullchain)
-                else:
-                    renew_skipped.append(renewal_candidate.fullchain)
-        except Exception as e:  # pylint: disable=broad-except
-            # obtain_cert (presumably) encountered an unanticipated problem.
-            logger.warning("Attempting to renew cert from %s produced an "
-                           "unexpected error: %s. Skipping.", renewal_file, e)
-            logger.debug("Traceback was:\n%s", traceback.format_exc())
-            renew_failures.append(renewal_candidate.fullchain)
-
-    # Describe all the results
-    _renew_describe_results(config, renew_successes, renew_failures,
-                            renew_skipped, parse_failures)
-
-    if renew_failures or parse_failures:
-        raise errors.Error("{0} renew failure(s), {1} parse failure(s)".format(
-            len(renew_failures), len(parse_failures)))
-    else:
-        logger.debug("no renewal failures")
-
+set_by_cli.detector = None
 
 def read_file(filename, mode="rb"):
     """Returns the given file's contents.
@@ -839,7 +551,7 @@ class HelpfulArgumentParser(object):
     def modify_arg_for_default_detection(self, *args, **kwargs):
         """
         Adding an arg, but ensure that it has a default that evaluates to false,
-        so that _set_by_cli can tell if it was set.  Only called if detect_defaults==True.
+        so that set_by_cli can tell if it was set.  Only called if detect_defaults==True.
 
         :param list *args: the names of this argument flag
         :param dict **kwargs: various argparse settings for this argument
@@ -1108,8 +820,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
     _plugins_parsing(helpful, plugins)
 
     if not detect_defaults:
-        global _parser # pylint: disable=global-statement
-        _parser = helpful
+        global helpful_parser # pylint: disable=global-statement
+        helpful_parser = helpful
     return helpful.parse_args()
 
 
diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index 56725d300..8d59993df 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -20,9 +20,9 @@ from letsencrypt import interfaces
 from letsencrypt import le_util
 from letsencrypt import log
 from letsencrypt import reporter
+from letsencrypt import renew
 from letsencrypt import storage
 
-from letsencrypt.cli import choose_configurator_plugins, _renewal_conf_files, should_renew
 from letsencrypt.display import util as display_util, ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
 
@@ -184,7 +184,7 @@ def _handle_identical_cert_request(config, cert):
     :rtype: tuple
 
     """
-    if should_renew(config, cert):
+    if renew.should_renew(config, cert):
         return "renew", cert
     if config.reinstall:
         # Set with --reinstall, force an identical certificate to be
@@ -261,7 +261,7 @@ def _find_duplicative_certs(config, domains):
     # Verify the directory is there
     le_util.make_or_verify_dir(configs_dir, mode=0o755, uid=os.geteuid())
 
-    for renewal_file in _renewal_conf_files(cli_config):
+    for renewal_file in renew.renewal_conf_files(cli_config):
         try:
             candidate_lineage = storage.RenewableCert(renewal_file, cli_config)
         except (errors.CertStorageError, IOError):
@@ -404,7 +404,7 @@ def install(config, plugins):
     # this function ...
 
     try:
-        installer, _ = choose_configurator_plugins(config, plugins, "install")
+        installer, _ = cli.choose_configurator_plugins(config, plugins, "install")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -479,7 +479,7 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     # TODO: Make run as close to auth + install as possible
     # Possible difficulties: config.csr was hacked into auth
     try:
-        installer, authenticator = choose_configurator_plugins(config, plugins, "run")
+        installer, authenticator = cli.choose_configurator_plugins(config, plugins, "run")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -509,7 +509,7 @@ def obtain_cert(config, plugins, lineage=None):
     # pylint: disable=too-many-locals
     try:
         # installers are used in auth mode to determine domain names
-        installer, authenticator = choose_configurator_plugins(config, plugins, "certonly")
+        installer, authenticator = cli.choose_configurator_plugins(config, plugins, "certonly")
     except errors.PluginSelectionError as e:
         logger.info("Could not choose appropriate plugin: %s", e)
         raise
@@ -705,7 +705,7 @@ def main(cli_args=sys.argv[1:]):
 # due to issues with import cycles and testing, it lives here
 VERBS = {"auth": obtain_cert, "certonly": obtain_cert,
          "config_changes": config_changes, "everything": run,
-         "install": install, "plugins": plugins_cmd, "renew": cli.renew,
+         "install": install, "plugins": plugins_cmd, "renew": renew.renew,
          "revoke": revoke, "rollback": rollback, "run": run}
 
 
diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py
index 319692344..f6a2c3d76 100644
--- a/letsencrypt/plugins/common.py
+++ b/letsencrypt/plugins/common.py
@@ -52,7 +52,7 @@ class Plugin(object):
         NOTE: if you add argpase arguments such that users setting them can
         create a config entry that python's bool() would consider false (ie,
         the use might set the variable to "", [], 0, etc), please ensure that
-        cli._set_by_cli() works for your variable.
+        cli.set_by_cli() works for your variable.
 
         """
 
diff --git a/letsencrypt/renew.py b/letsencrypt/renew.py
index e69de29bb..6eb3fa27c 100644
--- a/letsencrypt/renew.py
+++ b/letsencrypt/renew.py
@@ -0,0 +1,304 @@
+"""Functionality for autorenewal and associated juggling of configurations"""
+from __future__ import print_function
+import copy
+import glob
+import logging
+import os
+import traceback
+
+import six
+import zope.component
+
+from letsencrypt import configuration
+from letsencrypt import cli
+from letsencrypt import errors
+from letsencrypt import storage
+from letsencrypt.plugins import disco as plugins_disco
+
+logger = logging.getLogger(__name__)
+
+# These are the items which get pulled out of a renewal configuration
+# file's renewalparams and actually used in the client configuration
+# during the renewal process. We have to record their types here because
+# the renewal configuration process loses this information.
+STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent",
+                    "server", "account", "authenticator", "installer",
+                    "standalone_supported_challenges"]
+INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"]
+
+
+def _renew_describe_results(config, renew_successes, renew_failures,
+                            renew_skipped, parse_failures):
+    status = lambda x, msg: "  " + "\n  ".join(i + " (" + msg +")" for i in x)
+    if config.dry_run:
+        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
+        print("**          (The test certificates below have not been saved.)")
+    print()
+    if renew_skipped:
+        print("The following certs are not due for renewal yet:")
+        print(status(renew_skipped, "skipped"))
+    if not renew_successes and not renew_failures:
+        print("No renewals were attempted.")
+    elif renew_successes and not renew_failures:
+        print("Congratulations, all renewals succeeded. The following certs "
+              "have been renewed:")
+        print(status(renew_successes, "success"))
+    elif renew_failures and not renew_successes:
+        print("All renewal attempts failed. The following certs could not be "
+              "renewed:")
+        print(status(renew_failures, "failure"))
+    elif renew_failures and renew_successes:
+        print("The following certs were successfully renewed:")
+        print(status(renew_successes, "success"))
+        print("\nThe following certs could not be renewed:")
+        print(status(renew_failures, "failure"))
+
+    if parse_failures:
+        print("\nAdditionally, the following renewal configuration files "
+              "were invalid: ")
+        print(status(parse_failures, "parsefail"))
+
+    if config.dry_run:
+        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
+        print("**          (The test certificates above have not been saved.)")
+
+
+def renewal_conf_files(config):
+    """Return /path/to/*.conf in the renewal conf directory"""
+    return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf"))
+
+
+def _reconstitute(config, full_path):
+    """Try to instantiate a RenewableCert, updating config with relevant items.
+
+    This is specifically for use in renewal and enforces several checks
+    and policies to ensure that we can try to proceed with the renwal
+    request. The config argument is modified by including relevant options
+    read from the renewal configuration file.
+
+    :param configuration.NamespaceConfig config: configuration for the
+        current lineage
+    :param str full_path: Absolute path to the configuration file that
+        defines this lineage
+
+    :returns: the RenewableCert object or None if a fatal error occurred
+    :rtype: `storage.RenewableCert` or NoneType
+
+    """
+    try:
+        renewal_candidate = storage.RenewableCert(
+            full_path, configuration.RenewerConfiguration(config))
+    except (errors.CertStorageError, IOError):
+        logger.warning("Renewal configuration file %s is broken. Skipping.", full_path)
+        logger.debug("Traceback was:\n%s", traceback.format_exc())
+        return None
+    if "renewalparams" not in renewal_candidate.configuration:
+        logger.warning("Renewal configuration file %s lacks "
+                       "renewalparams. Skipping.", full_path)
+        return None
+    renewalparams = renewal_candidate.configuration["renewalparams"]
+    if "authenticator" not in renewalparams:
+        logger.warning("Renewal configuration file %s does not specify "
+                       "an authenticator. Skipping.", full_path)
+        return None
+    # Now restore specific values along with their data types, if
+    # those elements are present.
+    try:
+        _restore_required_config_elements(config, renewalparams)
+        _restore_plugin_configs(config, renewalparams)
+    except (ValueError, errors.Error) as error:
+        logger.warning(
+            "An error occured while parsing %s. The error was %s. "
+            "Skipping the file.", full_path, error.message)
+        logger.debug("Traceback was:\n%s", traceback.format_exc())
+        return None
+
+    try:
+        for d in renewal_candidate.names():
+            cli.process_domain(config, d)
+    except errors.ConfigurationError as error:
+        logger.warning("Renewal configuration file %s references a cert "
+                       "that contains an invalid domain name. The problem "
+                       "was: %s. Skipping.", full_path, error)
+        return None
+
+    return renewal_candidate
+
+
+def _restore_webroot_config(config, renewalparams):
+    """
+    webroot_map is, uniquely, a dict, and the general-purpose configuration
+    restoring logic is not able to correctly parse it from the serialized
+    form.
+    """
+    if "webroot_map" in renewalparams:
+        # if the user does anything that would create a new webroot map on the
+        # CLI, don't use the old one
+        if not (cli.set_by_cli("webroot_map") or cli.set_by_cli("webroot_path")):
+            setattr(config.namespace, "webroot_map", renewalparams["webroot_map"])
+    elif "webroot_path" in renewalparams:
+        logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path")
+        wp = renewalparams["webroot_path"]
+        if isinstance(wp, str):  # prior to 0.1.0, webroot_path was a string
+            wp = [wp]
+        setattr(config.namespace, "webroot_path", wp)
+
+
+def _restore_plugin_configs(config, renewalparams):
+    """Sets plugin specific values in config from renewalparams
+
+    :param configuration.NamespaceConfig config: configuration for the
+        current lineage
+    :param configobj.Section renewalparams: Parameters from the renewal
+        configuration file that defines this lineage
+
+    """
+    # Now use parser to get plugin-prefixed items with correct types
+    # XXX: the current approach of extracting only prefixed items
+    #      related to the actually-used installer and authenticator
+    #      works as long as plugins don't need to read plugin-specific
+    #      variables set by someone else (e.g., assuming Apache
+    #      configurator doesn't need to read webroot_ variables).
+    # Note: if a parameter that used to be defined in the parser is no
+    #      longer defined, stored copies of that parameter will be
+    #      deserialized as strings by this logic even if they were
+    #      originally meant to be some other type.
+    if renewalparams["authenticator"] == "webroot":
+        _restore_webroot_config(config, renewalparams)
+        plugin_prefixes = []
+    else:
+        plugin_prefixes = [renewalparams["authenticator"]]
+
+    if renewalparams.get("installer", None) is not None:
+        plugin_prefixes.append(renewalparams["installer"])
+    for plugin_prefix in set(plugin_prefixes):
+        for config_item, config_value in six.iteritems(renewalparams):
+            if config_item.startswith(plugin_prefix + "_") and not cli.set_by_cli(config_item):
+                # Values None, True, and False need to be treated specially,
+                # As they don't get parsed correctly based on type
+                if config_value in ("None", "True", "False"):
+                    # bool("False") == True
+                    # pylint: disable=eval-used
+                    setattr(config.namespace, config_item, eval(config_value))
+                    continue
+                # If argparse has a type for this variable, use it:
+                # pylint: disable=protected-access
+                for action in cli.helpful_parser.parser._actions:
+                    if action.type is not None and action.dest == config_item:
+                        setattr(config.namespace, config_item,
+                                action.type(config_value))
+                        break
+                else:
+                    setattr(config.namespace, config_item, str(config_value))
+
+
+def _restore_required_config_elements(config, renewalparams):
+    """Sets non-plugin specific values in config from renewalparams
+
+    :param configuration.NamespaceConfig config: configuration for the
+        current lineage
+    :param configobj.Section renewalparams: parameters from the renewal
+        configuration file that defines this lineage
+
+    """
+    # string-valued items to add if they're present
+    for config_item in STR_CONFIG_ITEMS:
+        if config_item in renewalparams and not cli.set_by_cli(config_item):
+            value = renewalparams[config_item]
+            # Unfortunately, we've lost type information from ConfigObj,
+            # so we don't know if the original was NoneType or str!
+            if value == "None":
+                value = None
+            setattr(config.namespace, config_item, value)
+    # int-valued items to add if they're present
+    for config_item in INT_CONFIG_ITEMS:
+        if config_item in renewalparams and not cli.set_by_cli(config_item):
+            config_value = renewalparams[config_item]
+            # the default value for http01_port was None during private beta
+            if config_item == "http01_port" and config_value == "None":
+                logger.info("updating legacy http01_port value")
+                int_value = cli.flag_default("http01_port")
+            else:
+                try:
+                    int_value = int(config_value)
+                except ValueError:
+                    raise errors.Error(
+                        "Expected a numeric value for {0}".format(config_item))
+            setattr(config.namespace, config_item, int_value)
+
+
+def should_renew(config, lineage):
+    "Return true if any of the circumstances for automatic renewal apply."
+    if config.renew_by_default:
+        logger.info("Auto-renewal forced with --force-renewal...")
+        return True
+    if lineage.should_autorenew(interactive=True):
+        logger.info("Cert is due for renewal, auto-renewing...")
+        return True
+    if config.dry_run:
+        logger.info("Cert not due for renewal, but simulating renewal for dry run")
+        return True
+    logger.info("Cert not yet due for renewal")
+    return False
+
+
+def renew(config, unused_plugins):
+    """Renew previously-obtained certificates."""
+
+    if config.domains != []:
+        raise errors.Error("Currently, the renew verb is only capable of "
+                           "renewing all installed certificates that are due "
+                           "to be renewed; individual domains cannot be "
+                           "specified with this action. If you would like to "
+                           "renew specific certificates, use the certonly "
+                           "command. The renew verb may provide other options "
+                           "for selecting certificates to renew in the future.")
+    renewer_config = configuration.RenewerConfiguration(config)
+    renew_successes = []
+    renew_failures = []
+    renew_skipped = []
+    parse_failures = []
+    for renewal_file in renewal_conf_files(renewer_config):
+        print("Processing " + renewal_file)
+        lineage_config = copy.deepcopy(config)
+
+        # Note that this modifies config (to add back the configuration
+        # elements from within the renewal configuration file).
+        try:
+            renewal_candidate = _reconstitute(lineage_config, renewal_file)
+        except Exception as e:  # pylint: disable=broad-except
+            logger.warning("Renewal configuration file %s produced an "
+                           "unexpected error: %s. Skipping.", renewal_file, e)
+            logger.debug("Traceback was:\n%s", traceback.format_exc())
+            parse_failures.append(renewal_file)
+            continue
+
+        try:
+            if renewal_candidate is None:
+                parse_failures.append(renewal_file)
+            else:
+                # XXX: ensure that each call here replaces the previous one
+                zope.component.provideUtility(lineage_config)
+                if should_renew(lineage_config, renewal_candidate):
+                    plugins = plugins_disco.PluginsRegistry.find_all()
+                    from letsencrypt import main
+                    main.obtain_cert(lineage_config, plugins, renewal_candidate)
+                    renew_successes.append(renewal_candidate.fullchain)
+                else:
+                    renew_skipped.append(renewal_candidate.fullchain)
+        except Exception as e:  # pylint: disable=broad-except
+            # obtain_cert (presumably) encountered an unanticipated problem.
+            logger.warning("Attempting to renew cert from %s produced an "
+                           "unexpected error: %s. Skipping.", renewal_file, e)
+            logger.debug("Traceback was:\n%s", traceback.format_exc())
+            renew_failures.append(renewal_candidate.fullchain)
+
+    # Describe all the results
+    _renew_describe_results(config, renew_successes, renew_failures,
+                            renew_skipped, parse_failures)
+
+    if renew_failures or parse_failures:
+        raise errors.Error("{0} renew failure(s), {1} parse failure(s)".format(
+            len(renew_failures), len(parse_failures)))
+    else:
+        logger.debug("no renewal failures")
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index c5865206d..e79bcb95a 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -1,5 +1,4 @@
 """Tests for letsencrypt.cli."""
-
 from __future__ import print_function
 
 import argparse
@@ -24,6 +23,7 @@ from letsencrypt import crypto_util
 from letsencrypt import errors
 from letsencrypt import le_util
 from letsencrypt import main
+from letsencrypt import renew
 from letsencrypt import storage
 
 from letsencrypt.plugins import disco
@@ -555,7 +555,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
                           self._certonly_new_request_common, mock_client)
 
     def _test_renewal_common(self, due_for_renewal, extra_args, log_out=None,
-                             args=None, renew=True, error_expected=False):
+                             args=None, should_renew=True, error_expected=False):
         # pylint: disable=too-many-locals,too-many-arguments
         cert_path = 'letsencrypt/tests/testdata/cert.pem'
         chain_path = '/etc/letsencrypt/live/foo.bar/fullchain.pem'
@@ -594,7 +594,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
                                             "Unexpected renewal error:\n" +
                                             traceback.format_exc())
 
-            if renew:
+            if should_renew:
                 mock_client.obtain_certificate.assert_called_once_with(['isnot.org'])
             else:
                 self.assertEqual(mock_client.obtain_certificate.call_count, 0)
@@ -629,7 +629,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertEqual(get_utility().add_message.call_count, 1)
 
         _, _ = self._test_renewal_common(False, ['-tvv', '--debug', '--keep'],
-                                        log_out="not yet due", renew=False)
+                                        log_out="not yet due", should_renew=False)
 
     def _dump_log(self):
         with open(os.path.join(self.logs_dir, "letsencrypt.log")) as lf:
@@ -652,9 +652,9 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
     def test_renew_verb(self):
         self._make_test_renewal_conf('sample-renewal.conf')
         args = ["renew", "--dry-run", "-tvv"]
-        self._test_renewal_common(True, [], args=args, renew=True)
+        self._test_renewal_common(True, [], args=args, should_renew=True)
 
-    @mock.patch("letsencrypt.cli._set_by_cli")
+    @mock.patch("letsencrypt.cli.set_by_cli")
     def test_ancient_webroot_renewal_conf(self, mock_set_by_cli):
         mock_set_by_cli.return_value = False
         rc_path = self._make_test_renewal_conf('sample-renewal-ancient.conf')
@@ -664,7 +664,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
             configuration.RenewerConfiguration(config))
         renewalparams = lineage.configuration["renewalparams"]
         # pylint: disable=protected-access
-        cli._restore_webroot_config(config, renewalparams)
+        renew._restore_webroot_config(config, renewalparams)
         self.assertEqual(config.webroot_path, ["/var/www/"])
 
     def test_renew_verb_empty_config(self):
@@ -674,7 +674,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         with open(os.path.join(rd, 'empty.conf'), 'w'):
             pass  # leave the file empty
         args = ["renew", "--dry-run", "-tvv"]
-        self._test_renewal_common(False, [], args=args, renew=False, error_expected=True)
+        self._test_renewal_common(False, [], args=args, should_renew=False, error_expected=True)
 
     def _make_dummy_renewal_config(self):
         renewer_configs_dir = os.path.join(self.config_dir, 'renewal')
@@ -695,7 +695,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
             mock_rc.return_value = mock_lineage
             with mock.patch('letsencrypt.main.obtain_cert') as mock_obtain_cert:
                 self._test_renewal_common(True, None, error_expected=error_expected,
-                                          args=['renew'], renew=False)
+                                          args=['renew'], should_renew=False)
             if assert_oc_called is not None:
                 if assert_oc_called:
                     self.assertTrue(mock_obtain_cert.called)
@@ -751,13 +751,13 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
             with mock.patch('letsencrypt.main.obtain_cert') as mock_obtain_cert:
                 mock_obtain_cert.side_effect = Exception
                 self._test_renewal_common(True, None, error_expected=True,
-                                          args=['renew'], renew=False)
+                                          args=['renew'], should_renew=False)
 
     def test_renew_with_bad_cli_args(self):
         self._test_renewal_common(True, None, args='renew -d example.com'.split(),
-                                  renew=False, error_expected=True)
+                                  should_renew=False, error_expected=True)
         self._test_renewal_common(True, None, args='renew --csr {0}'.format(CSR).split(),
-                                  renew=False, error_expected=True)
+                                  should_renew=False, error_expected=True)
 
     @mock.patch('letsencrypt.cli.zope.component.getUtility')
     @mock.patch('letsencrypt.main._treat_as_renewal')

From 4ca25828b25c6f3ee3d5ce0591ccdbb4718f24e1 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 13:46:37 -0800
Subject: [PATCH 02/29] Get tests passing

---
 letsencrypt/cli.py            | 1 -
 letsencrypt/plugins/disco.py  | 1 +
 letsencrypt/tests/cli_test.py | 8 ++++----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index e34bd971b..bea8b198d 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -1,5 +1,4 @@
 """Let's Encrypt command CLI argument processing."""
-# pylint: disable=too-many-lines
 from __future__ import print_function
 import argparse
 import glob
diff --git a/letsencrypt/plugins/disco.py b/letsencrypt/plugins/disco.py
index 9ed6ac596..27d2fb541 100644
--- a/letsencrypt/plugins/disco.py
+++ b/letsencrypt/plugins/disco.py
@@ -4,6 +4,7 @@ import logging
 import pkg_resources
 
 import zope.interface
+import zope.interface.verify
 
 from letsencrypt import constants
 from letsencrypt import errors
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index e79bcb95a..f1f539016 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -517,7 +517,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
                 args += '-d foo.bar -a standalone certonly'.split()
                 self._call(args)
 
-    @mock.patch('letsencrypt.cli.zope.component.getUtility')
+    @mock.patch('letsencrypt.main.zope.component.getUtility')
     def test_certonly_dry_run_new_request_success(self, mock_get_utility):
         mock_client = mock.MagicMock()
         mock_client.obtain_and_enroll_certificate.return_value = None
@@ -530,7 +530,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertEqual(mock_get_utility().add_message.call_count, 1)
 
     @mock.patch('letsencrypt.crypto_util.notAfter')
-    @mock.patch('letsencrypt.cli.zope.component.getUtility')
+    @mock.patch('letsencrypt.main.zope.component.getUtility')
     def test_certonly_new_request_success(self, mock_get_utility, mock_notAfter):
         cert_path = '/etc/letsencrypt/live/foo.bar'
         date = '1970-01-01'
@@ -736,7 +736,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
 
     def test_renew_reconstitute_error(self):
         # pylint: disable=protected-access
-        with mock.patch('letsencrypt.cli._reconstitute') as mock_reconstitute:
+        with mock.patch('letsencrypt.main.renew._reconstitute') as mock_reconstitute:
             mock_reconstitute.side_effect = Exception
             self._test_renew_common(assert_oc_called=False, error_expected=True)
 
@@ -759,7 +759,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self._test_renewal_common(True, None, args='renew --csr {0}'.format(CSR).split(),
                                   should_renew=False, error_expected=True)
 
-    @mock.patch('letsencrypt.cli.zope.component.getUtility')
+    @mock.patch('letsencrypt.main.zope.component.getUtility')
     @mock.patch('letsencrypt.main._treat_as_renewal')
     @mock.patch('letsencrypt.main._init_le_client')
     def test_certonly_reinstall(self, mock_init, mock_renewal, mock_get_utility):

From e644560a55087a5c0fbef7aff25d9c5ab7bec134 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 14:00:57 -0800
Subject: [PATCH 03/29] neaten

---
 letsencrypt/renew.py | 72 ++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/letsencrypt/renew.py b/letsencrypt/renew.py
index 6eb3fa27c..bd00543c8 100644
--- a/letsencrypt/renew.py
+++ b/letsencrypt/renew.py
@@ -27,42 +27,6 @@ STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent",
 INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"]
 
 
-def _renew_describe_results(config, renew_successes, renew_failures,
-                            renew_skipped, parse_failures):
-    status = lambda x, msg: "  " + "\n  ".join(i + " (" + msg +")" for i in x)
-    if config.dry_run:
-        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
-        print("**          (The test certificates below have not been saved.)")
-    print()
-    if renew_skipped:
-        print("The following certs are not due for renewal yet:")
-        print(status(renew_skipped, "skipped"))
-    if not renew_successes and not renew_failures:
-        print("No renewals were attempted.")
-    elif renew_successes and not renew_failures:
-        print("Congratulations, all renewals succeeded. The following certs "
-              "have been renewed:")
-        print(status(renew_successes, "success"))
-    elif renew_failures and not renew_successes:
-        print("All renewal attempts failed. The following certs could not be "
-              "renewed:")
-        print(status(renew_failures, "failure"))
-    elif renew_failures and renew_successes:
-        print("The following certs were successfully renewed:")
-        print(status(renew_successes, "success"))
-        print("\nThe following certs could not be renewed:")
-        print(status(renew_failures, "failure"))
-
-    if parse_failures:
-        print("\nAdditionally, the following renewal configuration files "
-              "were invalid: ")
-        print(status(parse_failures, "parsefail"))
-
-    if config.dry_run:
-        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
-        print("**          (The test certificates above have not been saved.)")
-
-
 def renewal_conf_files(config):
     """Return /path/to/*.conf in the renewal conf directory"""
     return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf"))
@@ -242,6 +206,42 @@ def should_renew(config, lineage):
     return False
 
 
+def _renew_describe_results(config, renew_successes, renew_failures,
+                            renew_skipped, parse_failures):
+    status = lambda ms, category: "  " + "\n  ".join(m + " (%s)" % category for m in ms)
+    if config.dry_run:
+        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
+        print("**          (The test certificates below have not been saved.)")
+    print()
+    if renew_skipped:
+        print("The following certs are not due for renewal yet:")
+        print(status(renew_skipped, "skipped"))
+    if not renew_successes and not renew_failures:
+        print("No renewals were attempted.")
+    elif renew_successes and not renew_failures:
+        print("Congratulations, all renewals succeeded. The following certs "
+              "have been renewed:")
+        print(status(renew_successes, "success"))
+    elif renew_failures and not renew_successes:
+        print("All renewal attempts failed. The following certs could not be "
+              "renewed:")
+        print(status(renew_failures, "failure"))
+    elif renew_failures and renew_successes:
+        print("The following certs were successfully renewed:")
+        print(status(renew_successes, "success"))
+        print("\nThe following certs could not be renewed:")
+        print(status(renew_failures, "failure"))
+
+    if parse_failures:
+        print("\nAdditionally, the following renewal configuration files "
+              "were invalid: ")
+        print(status(parse_failures, "parsefail"))
+
+    if config.dry_run:
+        print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
+        print("**          (The test certificates above have not been saved.)")
+
+
 def renew(config, unused_plugins):
     """Renew previously-obtained certificates."""
 

From 6f7f036e2cac155b3bfddd614fb603bd7ce0ac9d Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 14:07:35 -0800
Subject: [PATCH 04/29] Neaten further

---
 letsencrypt/renew.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/letsencrypt/renew.py b/letsencrypt/renew.py
index bd00543c8..367eda5b0 100644
--- a/letsencrypt/renew.py
+++ b/letsencrypt/renew.py
@@ -208,34 +208,35 @@ def should_renew(config, lineage):
 
 def _renew_describe_results(config, renew_successes, renew_failures,
                             renew_skipped, parse_failures):
-    status = lambda ms, category: "  " + "\n  ".join(m + " (%s)" % category for m in ms)
+    def _status(msgs, category):
+        return "  " + "\n  ".join("%s (%s)" % (m, category) for m in msgs)
     if config.dry_run:
         print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")
         print("**          (The test certificates below have not been saved.)")
     print()
     if renew_skipped:
         print("The following certs are not due for renewal yet:")
-        print(status(renew_skipped, "skipped"))
+        print(_status(renew_skipped, "skipped"))
     if not renew_successes and not renew_failures:
         print("No renewals were attempted.")
     elif renew_successes and not renew_failures:
         print("Congratulations, all renewals succeeded. The following certs "
               "have been renewed:")
-        print(status(renew_successes, "success"))
+        print(_status(renew_successes, "success"))
     elif renew_failures and not renew_successes:
         print("All renewal attempts failed. The following certs could not be "
               "renewed:")
-        print(status(renew_failures, "failure"))
+        print(_status(renew_failures, "failure"))
     elif renew_failures and renew_successes:
         print("The following certs were successfully renewed:")
-        print(status(renew_successes, "success"))
+        print(_status(renew_successes, "success"))
         print("\nThe following certs could not be renewed:")
-        print(status(renew_failures, "failure"))
+        print(_status(renew_failures, "failure"))
 
     if parse_failures:
         print("\nAdditionally, the following renewal configuration files "
               "were invalid: ")
-        print(status(parse_failures, "parsefail"))
+        print(_status(parse_failures, "parsefail"))
 
     if config.dry_run:
         print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry")

From 50881cbb35d8e4a814691eb448ab3363878c01ae Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 14:49:12 -0800
Subject: [PATCH 05/29] Start splitting plugins.selection out of cli

---
 letsencrypt/cli.py               | 152 ++-----------------------------
 letsencrypt/main.py              |   9 +-
 letsencrypt/plugins/selection.py | 146 +++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+), 150 deletions(-)
 create mode 100644 letsencrypt/plugins/selection.py

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index bea8b198d..2b45b180d 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -13,7 +13,6 @@ import configargparse
 import OpenSSL
 import six
 
-import letsencrypt
 
 from letsencrypt import constants
 from letsencrypt import crypto_util
@@ -23,6 +22,7 @@ from letsencrypt import le_util
 
 from letsencrypt.display import ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
+from letsencrypt.plugin.selection import cli_plugin_requests
 
 
 logger = logging.getLogger(__name__)
@@ -33,9 +33,10 @@ helpful_parser = None
 # For help strings, figure out how the user ran us.
 # When invoked from letsencrypt-auto, sys.argv[0] is something like:
 # "/home/user/.local/share/letsencrypt/bin/letsencrypt"
-# Note that this won't work if the user set VENV_PATH or XDG_DATA_HOME before running
-# letsencrypt-auto (and sudo stops us from seeing if they did), so it should only be used
-# for purposes where inability to detect letsencrypt-auto fails safely
+# Note that this won't work if the user set VENV_PATH or XDG_DATA_HOME before
+# running letsencrypt-auto (and sudo stops us from seeing if they did), so it
+# should only be used for purposes where inability to detect letsencrypt-auto
+# fails safely
 
 fragment = os.path.join(".local", "share", "letsencrypt")
 cli_command = "letsencrypt-auto" if fragment in sys.argv[0] else "letsencrypt"
@@ -99,145 +100,6 @@ def usage_strings(plugins):
     return USAGE % (apache_doc, nginx_doc), SHORT_USAGE
 
 
-def diagnose_configurator_problem(cfg_type, requested, plugins):
-    """
-    Raise the most helpful error message about a plugin being unavailable
-
-    :param str cfg_type: either "installer" or "authenticator"
-    :param str requested: the plugin that was requested
-    :param .PluginsRegistry plugins: available plugins
-
-    :raises error.PluginSelectionError: if there was a problem
-    """
-
-    if requested:
-        if requested not in plugins:
-            msg = "The requested {0} plugin does not appear to be installed".format(requested)
-        else:
-            msg = ("The {0} plugin is not working; there may be problems with "
-                   "your existing configuration.\nThe error was: {1!r}"
-                   .format(requested, plugins[requested].problem))
-    elif cfg_type == "installer":
-        if os.path.exists("/etc/debian_version"):
-            # Debian... installers are at least possible
-            msg = ('No installers seem to be present and working on your system; '
-                   'fix that or try running letsencrypt with the "certonly" command')
-        else:
-            # XXX update this logic as we make progress on #788 and nginx support
-            msg = ('No installers are available on your OS yet; try running '
-                   '"letsencrypt-auto certonly" to get a cert you can install manually')
-    else:
-        msg = "{0} could not be determined or is not installed".format(cfg_type)
-    raise errors.PluginSelectionError(msg)
-
-
-def set_configurator(previously, now):
-    """
-    Setting configurators multiple ways is okay, as long as they all agree
-    :param str previously: previously identified request for the installer/authenticator
-    :param str requested: the request currently being processed
-    """
-    if now is None:
-        # we're not actually setting anything
-        return previously
-    if previously:
-        if previously != now:
-            msg = "Too many flags setting configurators/installers/authenticators {0} -> {1}"
-            raise errors.PluginSelectionError(msg.format(repr(previously), repr(now)))
-    return now
-
-
-def cli_plugin_requests(config):
-    """
-    Figure out which plugins the user requested with CLI and config options
-
-    :returns: (requested authenticator string or None, requested installer string or None)
-    :rtype: tuple
-    """
-    req_inst = req_auth = config.configurator
-    req_inst = set_configurator(req_inst, config.installer)
-    req_auth = set_configurator(req_auth, config.authenticator)
-    if config.nginx:
-        req_inst = set_configurator(req_inst, "nginx")
-        req_auth = set_configurator(req_auth, "nginx")
-    if config.apache:
-        req_inst = set_configurator(req_inst, "apache")
-        req_auth = set_configurator(req_auth, "apache")
-    if config.standalone:
-        req_auth = set_configurator(req_auth, "standalone")
-    if config.webroot:
-        req_auth = set_configurator(req_auth, "webroot")
-    if config.manual:
-        req_auth = set_configurator(req_auth, "manual")
-    logger.debug("Requested authenticator %s and installer %s", req_auth, req_inst)
-    return req_auth, req_inst
-
-
-noninstaller_plugins = ["webroot", "manual", "standalone"]
-
-
-def choose_configurator_plugins(config, plugins, verb):
-    """
-    Figure out which configurator we're going to use, modifies
-    config.authenticator and config.istaller strings to reflect that choice if
-    necessary.
-
-    :raises errors.PluginSelectionError if there was a problem
-
-    :returns: (an `IAuthenticator` or None, an `IInstaller` or None)
-    :rtype: tuple
-    """
-
-    req_auth, req_inst = cli_plugin_requests(config)
-
-    # Which plugins do we need?
-    if verb == "run":
-        need_inst = need_auth = True
-        if req_auth in noninstaller_plugins and not req_inst:
-            msg = ('With the {0} plugin, you probably want to use the "certonly" command, eg:{1}'
-                   '{1}    {2} certonly --{0}{1}{1}'
-                   '(Alternatively, add a --installer flag. See https://eff.org/letsencrypt-plugins'
-                   '{1} and "--help plugins" for more information.)'.format(
-                       req_auth, os.linesep, cli_command))
-
-            raise errors.MissingCommandlineFlag(msg)
-    else:
-        need_inst = need_auth = False
-    if verb == "certonly":
-        need_auth = True
-    if verb == "install":
-        need_inst = True
-        if config.authenticator:
-            logger.warn("Specifying an authenticator doesn't make sense in install mode")
-
-    # Try to meet the user's request and/or ask them to pick plugins
-    authenticator = installer = None
-    if verb == "run" and req_auth == req_inst:
-        # Unless the user has explicitly asked for different auth/install,
-        # only consider offering a single choice
-        authenticator = installer = display_ops.pick_configurator(config, req_inst, plugins)
-    else:
-        if need_inst or req_inst:
-            installer = display_ops.pick_installer(config, req_inst, plugins)
-        if need_auth:
-            authenticator = display_ops.pick_authenticator(config, req_auth, plugins)
-    logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
-
-    # Report on any failures
-    if need_inst and not installer:
-        diagnose_configurator_problem("installer", req_inst, plugins)
-    if need_auth and not authenticator:
-        diagnose_configurator_problem("authenticator", req_auth, plugins)
-
-    record_chosen_plugins(config, plugins, authenticator, installer)
-    return installer, authenticator
-
-
-def record_chosen_plugins(config, plugins, auth, inst):
-    "Update the config entries to reflect the plugins we actually selected."
-    cn = config.namespace
-    cn.authenticator = plugins.find_init(auth).name if auth else "none"
-    cn.installer = plugins.find_init(inst).name if inst else "none"
 
 
 def set_by_cli(var):
@@ -256,7 +118,7 @@ def set_by_cli(var):
         detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
-        auth, inst = cli_plugin_requests(detector)
+        auth, inst = plugin_selection.cli_plugin_requests(detector)
         detector.authenticator = auth if auth else ""
         detector.installer = inst if inst else ""
         logger.debug("Default Detector is %r", detector)
@@ -308,7 +170,7 @@ def flag_default(name):
 
 
 def config_help(name, hidden=False):
-    """Help message for `.IConfig` attribute."""
+    """Extract the help message for an `.IConfig` attribute."""
     if hidden:
         return argparse.SUPPRESS
     else:
diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index 8d59993df..48e6e8505 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -6,8 +6,6 @@ import os
 import sys
 import zope.component
 
-import letsencrypt
-
 from letsencrypt import account
 from letsencrypt import client
 from letsencrypt import cli
@@ -25,6 +23,7 @@ from letsencrypt import storage
 
 from letsencrypt.display import util as display_util, ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
+from letsencrypt.plugins.selection import choose_configurator_plugins
 
 import traceback
 import logging.handlers
@@ -404,7 +403,7 @@ def install(config, plugins):
     # this function ...
 
     try:
-        installer, _ = cli.choose_configurator_plugins(config, plugins, "install")
+        installer, _ = choose_configurator_plugins(config, plugins, "install")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -479,7 +478,7 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     # TODO: Make run as close to auth + install as possible
     # Possible difficulties: config.csr was hacked into auth
     try:
-        installer, authenticator = cli.choose_configurator_plugins(config, plugins, "run")
+        installer, authenticator = choose_configurator_plugins(config, plugins, "run")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -509,7 +508,7 @@ def obtain_cert(config, plugins, lineage=None):
     # pylint: disable=too-many-locals
     try:
         # installers are used in auth mode to determine domain names
-        installer, authenticator = cli.choose_configurator_plugins(config, plugins, "certonly")
+        installer, authenticator = choose_configurator_plugins(config, plugins, "certonly")
     except errors.PluginSelectionError as e:
         logger.info("Could not choose appropriate plugin: %s", e)
         raise
diff --git a/letsencrypt/plugins/selection.py b/letsencrypt/plugins/selection.py
new file mode 100644
index 000000000..fc7274a24
--- /dev/null
+++ b/letsencrypt/plugins/selection.py
@@ -0,0 +1,146 @@
+from __future__ import print_function
+import os
+from letsencrypt import errors
+from letsencrypt.display import ops as display_ops
+
+logger = logging.getLogger(__name__)
+
+noninstaller_plugins = ["webroot", "manual", "standalone"]
+
+def record_chosen_plugins(config, plugins, auth, inst):
+    "Update the config entries to reflect the plugins we actually selected."
+    cn = config.namespace
+    cn.authenticator = plugins.find_init(auth).name if auth else "none"
+    cn.installer = plugins.find_init(inst).name if inst else "none"
+
+
+def choose_configurator_plugins(config, plugins, verb):
+    """
+    Figure out which configurator we're going to use, modifies
+    config.authenticator and config.istaller strings to reflect that choice if
+    necessary.
+
+    :raises errors.PluginSelectionError if there was a problem
+
+    :returns: (an `IAuthenticator` or None, an `IInstaller` or None)
+    :rtype: tuple
+    """
+
+    req_auth, req_inst = cli_plugin_requests(config)
+
+    # Which plugins do we need?
+    if verb == "run":
+        need_inst = need_auth = True
+        from letsencrypt.cli import cli_command
+        if req_auth in noninstaller_plugins and not req_inst:
+            msg = ('With the {0} plugin, you probably want to use the "certonly" command, eg:{1}'
+                   '{1}    {2} certonly --{0}{1}{1}'
+                   '(Alternatively, add a --installer flag. See https://eff.org/letsencrypt-plugins'
+                   '{1} and "--help plugins" for more information.)'.format(
+                       req_auth, os.linesep, cli_command))
+
+            raise errors.MissingCommandlineFlag(msg)
+    else:
+        need_inst = need_auth = False
+    if verb == "certonly":
+        need_auth = True
+    if verb == "install":
+        need_inst = True
+        if config.authenticator:
+            logger.warn("Specifying an authenticator doesn't make sense in install mode")
+
+    # Try to meet the user's request and/or ask them to pick plugins
+    authenticator = installer = None
+    if verb == "run" and req_auth == req_inst:
+        # Unless the user has explicitly asked for different auth/install,
+        # only consider offering a single choice
+        authenticator = installer = display_ops.pick_configurator(config, req_inst, plugins)
+    else:
+        if need_inst or req_inst:
+            installer = display_ops.pick_installer(config, req_inst, plugins)
+        if need_auth:
+            authenticator = display_ops.pick_authenticator(config, req_auth, plugins)
+    logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
+
+    # Report on any failures
+    if need_inst and not installer:
+        diagnose_configurator_problem("installer", req_inst, plugins)
+    if need_auth and not authenticator:
+        diagnose_configurator_problem("authenticator", req_auth, plugins)
+
+    record_chosen_plugins(config, plugins, authenticator, installer)
+    return installer, authenticator
+
+
+def set_configurator(previously, now):
+    """
+    Setting configurators multiple ways is okay, as long as they all agree
+    :param str previously: previously identified request for the installer/authenticator
+    :param str requested: the request currently being processed
+    """
+    if now is None:
+        # we're not actually setting anything
+        return previously
+    if previously:
+        if previously != now:
+            msg = "Too many flags setting configurators/installers/authenticators {0} -> {1}"
+            raise errors.PluginSelectionError(msg.format(repr(previously), repr(now)))
+    return now
+
+
+def cli_plugin_requests(config):
+    """
+    Figure out which plugins the user requested with CLI and config options
+
+    :returns: (requested authenticator string or None, requested installer string or None)
+    :rtype: tuple
+    """
+    req_inst = req_auth = config.configurator
+    req_inst = set_configurator(req_inst, config.installer)
+    req_auth = set_configurator(req_auth, config.authenticator)
+    if config.nginx:
+        req_inst = set_configurator(req_inst, "nginx")
+        req_auth = set_configurator(req_auth, "nginx")
+    if config.apache:
+        req_inst = set_configurator(req_inst, "apache")
+        req_auth = set_configurator(req_auth, "apache")
+    if config.standalone:
+        req_auth = set_configurator(req_auth, "standalone")
+    if config.webroot:
+        req_auth = set_configurator(req_auth, "webroot")
+    if config.manual:
+        req_auth = set_configurator(req_auth, "manual")
+    logger.debug("Requested authenticator %s and installer %s", req_auth, req_inst)
+    return req_auth, req_inst
+
+
+def diagnose_configurator_problem(cfg_type, requested, plugins):
+    """
+    Raise the most helpful error message about a plugin being unavailable
+
+    :param str cfg_type: either "installer" or "authenticator"
+    :param str requested: the plugin that was requested
+    :param .PluginsRegistry plugins: available plugins
+
+    :raises error.PluginSelectionError: if there was a problem
+    """
+
+    if requested:
+        if requested not in plugins:
+            msg = "The requested {0} plugin does not appear to be installed".format(requested)
+        else:
+            msg = ("The {0} plugin is not working; there may be problems with "
+                   "your existing configuration.\nThe error was: {1!r}"
+                   .format(requested, plugins[requested].problem))
+    elif cfg_type == "installer":
+        if os.path.exists("/etc/debian_version"):
+            # Debian... installers are at least possible
+            msg = ('No installers seem to be present and working on your system; '
+                   'fix that or try running letsencrypt with the "certonly" command')
+        else:
+            # XXX update this logic as we make progress on #788 and nginx support
+            msg = ('No installers are available on your OS yet; try running '
+                   '"letsencrypt-auto certonly" to get a cert you can install manually')
+    else:
+        msg = "{0} could not be determined or is not installed".format(cfg_type)
+    raise errors.PluginSelectionError(msg)

From 86ab35df4f66dfc15a99cf0c581b9100b77cc643 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 15:07:13 -0800
Subject: [PATCH 06/29] Move argparse type extraction back into cli.py

---
 letsencrypt/cli.py   | 14 ++++++++++++++
 letsencrypt/renew.py | 13 +++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index bea8b198d..017e0a62e 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -283,6 +283,16 @@ def set_by_cli(var):
 # static housekeeping var
 set_by_cli.detector = None
 
+
+def argparse_type(variable):
+    "Return our argparse type function for a config variable (default: str)"
+    # pylint: disable=protected-access
+    for action in helpful_parser.parser._actions:
+        if action.type is not None and action.dest == variable:
+            return action.type
+    return str
+
+
 def read_file(filename, mode="rb"):
     """Returns the given file's contents.
 
@@ -304,6 +314,10 @@ def read_file(filename, mode="rb"):
 
 def flag_default(name):
     """Default value for CLI flag."""
+    # XXX: this is an internal housekeeping notion of defaults before
+    # argparse has been set up; it is not accurate for all flags.  Call it
+    # with caution.  Plugin defaults are missing, and some things are using
+    # defaults defined in this file, not in constants.py :(
     return constants.CLI_DEFAULTS[name]
 
 
diff --git a/letsencrypt/renew.py b/letsencrypt/renew.py
index 367eda5b0..9dc1ff4df 100644
--- a/letsencrypt/renew.py
+++ b/letsencrypt/renew.py
@@ -139,21 +139,14 @@ def _restore_plugin_configs(config, renewalparams):
         for config_item, config_value in six.iteritems(renewalparams):
             if config_item.startswith(plugin_prefix + "_") and not cli.set_by_cli(config_item):
                 # Values None, True, and False need to be treated specially,
-                # As they don't get parsed correctly based on type
+                # As their types aren't handled correctly by configobj
                 if config_value in ("None", "True", "False"):
                     # bool("False") == True
                     # pylint: disable=eval-used
                     setattr(config.namespace, config_item, eval(config_value))
-                    continue
-                # If argparse has a type for this variable, use it:
-                # pylint: disable=protected-access
-                for action in cli.helpful_parser.parser._actions:
-                    if action.type is not None and action.dest == config_item:
-                        setattr(config.namespace, config_item,
-                                action.type(config_value))
-                        break
                 else:
-                    setattr(config.namespace, config_item, str(config_value))
+                    cast = cli.argparse_type(config_item)
+                    setattr(config.namespace, config_item, cast(config_value))
 
 
 def _restore_required_config_elements(config, renewalparams):

From c6fece8b404e5662772484d394490bf7ff3444b4 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 15:24:57 -0800
Subject: [PATCH 07/29] Also move plugin selection logic from display.ops

---
 letsencrypt/client.py                 |   5 +-
 letsencrypt/display/ops.py            | 136 ++------------------------
 letsencrypt/plugins/selection.py      | 131 ++++++++++++++++++++++++-
 letsencrypt/tests/display/ops_test.py |  12 +--
 4 files changed, 143 insertions(+), 141 deletions(-)

diff --git a/letsencrypt/client.py b/letsencrypt/client.py
index 6134c4e6e..1655c1fa5 100644
--- a/letsencrypt/client.py
+++ b/letsencrypt/client.py
@@ -11,8 +11,6 @@ from acme import client as acme_client
 from acme import jose
 from acme import messages
 
-import letsencrypt
-
 from letsencrypt import account
 from letsencrypt import auth_handler
 from letsencrypt import configuration
@@ -27,6 +25,7 @@ from letsencrypt import storage
 
 from letsencrypt.display import ops as display_ops
 from letsencrypt.display import enhancements
+from letsencrypt.plugins import selection as plugin_selection
 
 
 logger = logging.getLogger(__name__)
@@ -524,7 +523,7 @@ def rollback(default_installer, checkpoints, config, plugins):
 
     """
     # Misconfigurations are only a slight problems... allow the user to rollback
-    installer = display_ops.pick_installer(
+    installer = plugin_selection.pick_installer(
         config, default_installer, plugins, question="Which installer "
         "should be used for rollback?")
 
diff --git a/letsencrypt/display/ops.py b/letsencrypt/display/ops.py
index f0dec8b06..d60073ce0 100644
--- a/letsencrypt/display/ops.py
+++ b/letsencrypt/display/ops.py
@@ -8,131 +8,13 @@ from letsencrypt import errors
 from letsencrypt import interfaces
 from letsencrypt import le_util
 from letsencrypt.display import util as display_util
+import letsencrypt.plugins.selection
 
 
 logger = logging.getLogger(__name__)
 
 # Define a helper function to avoid verbose code
-util = zope.component.getUtility
-
-
-def choose_plugin(prepared, question):
-    """Allow the user to choose their plugin.
-
-    :param list prepared: List of `~.PluginEntryPoint`.
-    :param str question: Question to be presented to the user.
-
-    :returns: Plugin entry point chosen by the user.
-    :rtype: `~.PluginEntryPoint`
-
-    """
-    opts = [plugin_ep.description_with_name +
-            (" [Misconfigured]" if plugin_ep.misconfigured else "")
-            for plugin_ep in prepared]
-
-    while True:
-        disp = util(interfaces.IDisplay)
-        code, index = disp.menu(question, opts, help_label="More Info")
-
-        if code == display_util.OK:
-            plugin_ep = prepared[index]
-            if plugin_ep.misconfigured:
-                util(interfaces.IDisplay).notification(
-                    "The selected plugin encountered an error while parsing "
-                    "your server configuration and cannot be used. The error "
-                    "was:\n\n{0}".format(plugin_ep.prepare()),
-                    height=display_util.HEIGHT, pause=False)
-            else:
-                return plugin_ep
-        elif code == display_util.HELP:
-            if prepared[index].misconfigured:
-                msg = "Reported Error: %s" % prepared[index].prepare()
-            else:
-                msg = prepared[index].init().more_info()
-            util(interfaces.IDisplay).notification(
-                msg, height=display_util.HEIGHT)
-        else:
-            return None
-
-
-def pick_plugin(config, default, plugins, question, ifaces):
-    """Pick plugin.
-
-    :param letsencrypt.interfaces.IConfig: Configuration
-    :param str default: Plugin name supplied by user or ``None``.
-    :param letsencrypt.plugins.disco.PluginsRegistry plugins:
-        All plugins registered as entry points.
-    :param str question: Question to be presented to the user in case
-        multiple candidates are found.
-    :param list ifaces: Interfaces that plugins must provide.
-
-    :returns: Initialized plugin.
-    :rtype: IPlugin
-
-    """
-    if default is not None:
-        # throw more UX-friendly error if default not in plugins
-        filtered = plugins.filter(lambda p_ep: p_ep.name == default)
-    else:
-        if config.noninteractive_mode:
-            # it's really bad to auto-select the single available plugin in
-            # non-interactive mode, because an update could later add a second
-            # available plugin
-            raise errors.MissingCommandlineFlag(
-                "Missing command line flags. For non-interactive execution, "
-                "you will need to specify a plugin on the command line.  Run "
-                "with '--help plugins' to see a list of options, and see "
-                "https://eff.org/letsencrypt-plugins for more detail on what "
-                "the plugins do and how to use them.")
-
-        filtered = plugins.visible().ifaces(ifaces)
-
-    filtered.init(config)
-    verified = filtered.verify(ifaces)
-    verified.prepare()
-    prepared = verified.available()
-
-    if len(prepared) > 1:
-        logger.debug("Multiple candidate plugins: %s", prepared)
-        plugin_ep = choose_plugin(prepared.values(), question)
-        if plugin_ep is None:
-            return None
-        else:
-            return plugin_ep.init()
-    elif len(prepared) == 1:
-        plugin_ep = prepared.values()[0]
-        logger.debug("Single candidate plugin: %s", plugin_ep)
-        if plugin_ep.misconfigured:
-            return None
-        return plugin_ep.init()
-    else:
-        logger.debug("No candidate plugin")
-        return None
-
-
-def pick_authenticator(
-        config, default, plugins, question="How would you "
-        "like to authenticate with the Let's Encrypt CA?"):
-    """Pick authentication plugin."""
-    return pick_plugin(
-        config, default, plugins, question, (interfaces.IAuthenticator,))
-
-
-def pick_installer(config, default, plugins,
-                   question="How would you like to install certificates?"):
-    """Pick installer plugin."""
-    return pick_plugin(
-        config, default, plugins, question, (interfaces.IInstaller,))
-
-
-def pick_configurator(
-        config, default, plugins,
-        question="How would you like to authenticate and install "
-                 "certificates?"):
-    """Pick configurator plugin."""
-    return pick_plugin(
-        config, default, plugins, question,
-        (interfaces.IAuthenticator, interfaces.IInstaller))
+z_util = zope.component.getUtility
 
 
 def get_email(more=False, invalid=False):
@@ -182,7 +64,7 @@ def choose_account(accounts):
     # Note this will get more complicated once we start recording authorizations
     labels = [acc.slug for acc in accounts]
 
-    code, index = util(interfaces.IDisplay).menu(
+    code, index = z_util(interfaces.IDisplay).menu(
         "Please choose an account", labels)
     if code == display_util.OK:
         return accounts[index]
@@ -208,7 +90,7 @@ def choose_names(installer):
     names = get_valid_domains(domains)
 
     if not names:
-        manual = util(interfaces.IDisplay).yesno(
+        manual = z_util(interfaces.IDisplay).yesno(
             "No names were found in your configuration files.{0}You should "
             "specify ServerNames in your config files in order to allow for "
             "accurate installation of your certificate.{0}"
@@ -256,7 +138,7 @@ def _filter_names(names):
     :rtype: tuple
 
     """
-    code, names = util(interfaces.IDisplay).checklist(
+    code, names = z_util(interfaces.IDisplay).checklist(
         "Which names would you like to activate HTTPS for?",
         tags=names, cli_flag="--domains")
     return code, [str(s) for s in names]
@@ -265,7 +147,7 @@ def _filter_names(names):
 def _choose_names_manually():
     """Manually input names for those without an installer."""
 
-    code, input_ = util(interfaces.IDisplay).input(
+    code, input_ = z_util(interfaces.IDisplay).input(
         "Please enter in your domain name(s) (comma and/or space separated) ",
         cli_flag="--domains")
 
@@ -300,7 +182,7 @@ def _choose_names_manually():
 
         if retry_message:
             # We had error in input
-            retry = util(interfaces.IDisplay).yesno(retry_message)
+            retry = z_util(interfaces.IDisplay).yesno(retry_message)
             if retry:
                 return _choose_names_manually()
         else:
@@ -316,7 +198,7 @@ def success_installation(domains):
     :param list domains: domain names which were enabled
 
     """
-    util(interfaces.IDisplay).notification(
+    z_util(interfaces.IDisplay).notification(
         "Congratulations! You have successfully enabled {0}{1}{1}"
         "You should test your configuration at:{1}{2}".format(
             _gen_https_names(domains),
@@ -335,7 +217,7 @@ def success_renewal(domains, action):
     :param str action: can be "reinstall" or "renew"
 
     """
-    util(interfaces.IDisplay).notification(
+    z_util(interfaces.IDisplay).notification(
         "Your existing certificate has been successfully {3}ed, and the "
         "new certificate has been installed.{1}{1}"
         "The new certificate covers the following domains: {0}{1}{1}"
diff --git a/letsencrypt/plugins/selection.py b/letsencrypt/plugins/selection.py
index fc7274a24..bf6187950 100644
--- a/letsencrypt/plugins/selection.py
+++ b/letsencrypt/plugins/selection.py
@@ -1,7 +1,128 @@
 from __future__ import print_function
 import os
-from letsencrypt import errors
-from letsencrypt.display import ops as display_ops
+from letsencrypt import errors, interfaces
+from letsencrypt.display import util as display_util
+
+logger = logging.getLogger(__name__)
+z_util = zope.component.getUtility
+
+def pick_configurator(
+        config, default, plugins,
+        question="How would you like to authenticate and install "
+                 "certificates?"):
+    """Pick configurator plugin."""
+    return pick_plugin(
+        config, default, plugins, question,
+        (interfaces.IAuthenticator, interfaces.IInstaller))
+
+
+def pick_installer(config, default, plugins,
+                   question="How would you like to install certificates?"):
+    """Pick installer plugin."""
+    return pick_plugin(
+        config, default, plugins, question, (interfaces.IInstaller,))
+
+
+def pick_authenticator(
+        config, default, plugins, question="How would you "
+        "like to authenticate with the Let's Encrypt CA?"):
+    """Pick authentication plugin."""
+    return pick_plugin(
+        config, default, plugins, question, (interfaces.IAuthenticator,))
+
+
+def pick_plugin(config, default, plugins, question, ifaces):
+    """Pick plugin.
+
+    :param letsencrypt.interfaces.IConfig: Configuration
+    :param str default: Plugin name supplied by user or ``None``.
+    :param letsencrypt.plugins.disco.PluginsRegistry plugins:
+        All plugins registered as entry points.
+    :param str question: Question to be presented to the user in case
+        multiple candidates are found.
+    :param list ifaces: Interfaces that plugins must provide.
+
+    :returns: Initialized plugin.
+    :rtype: IPlugin
+
+    """
+    if default is not None:
+        # throw more UX-friendly error if default not in plugins
+        filtered = plugins.filter(lambda p_ep: p_ep.name == default)
+    else:
+        if config.noninteractive_mode:
+            # it's really bad to auto-select the single available plugin in
+            # non-interactive mode, because an update could later add a second
+            # available plugin
+            raise errors.MissingCommandlineFlag(
+                "Missing command line flags. For non-interactive execution, "
+                "you will need to specify a plugin on the command line.  Run "
+                "with '--help plugins' to see a list of options, and see "
+                "https://eff.org/letsencrypt-plugins for more detail on what "
+                "the plugins do and how to use them.")
+
+        filtered = plugins.visible().ifaces(ifaces)
+
+    filtered.init(config)
+    verified = filtered.verify(ifaces)
+    verified.prepare()
+    prepared = verified.available()
+
+    if len(prepared) > 1:
+        logger.debug("Multiple candidate plugins: %s", prepared)
+        plugin_ep = choose_plugin(prepared.values(), question)
+        if plugin_ep is None:
+            return None
+        else:
+            return plugin_ep.init()
+    elif len(prepared) == 1:
+        plugin_ep = prepared.values()[0]
+        logger.debug("Single candidate plugin: %s", plugin_ep)
+        if plugin_ep.misconfigured:
+            return None
+        return plugin_ep.init()
+    else:
+        logger.debug("No candidate plugin")
+        return None
+
+
+def choose_plugin(prepared, question):
+    """Allow the user to choose their plugin.
+
+    :param list prepared: List of `~.PluginEntryPoint`.
+    :param str question: Question to be presented to the user.
+
+    :returns: Plugin entry point chosen by the user.
+    :rtype: `~.PluginEntryPoint`
+
+    """
+    opts = [plugin_ep.description_with_name +
+            (" [Misconfigured]" if plugin_ep.misconfigured else "")
+            for plugin_ep in prepared]
+
+    while True:
+        disp = z_util(interfaces.IDisplay)
+        code, index = disp.menu(question, opts, help_label="More Info")
+
+        if code == display_util.OK:
+            plugin_ep = prepared[index]
+            if plugin_ep.misconfigured:
+                z_util(interfaces.IDisplay).notification(
+                    "The selected plugin encountered an error while parsing "
+                    "your server configuration and cannot be used. The error "
+                    "was:\n\n{0}".format(plugin_ep.prepare()),
+                    height=display_util.HEIGHT, pause=False)
+            else:
+                return plugin_ep
+        elif code == display_util.HELP:
+            if prepared[index].misconfigured:
+                msg = "Reported Error: %s" % prepared[index].prepare()
+            else:
+                msg = prepared[index].init().more_info()
+            z_util(interfaces.IDisplay).notification(
+                msg, height=display_util.HEIGHT)
+        else:
+            return None
 
 logger = logging.getLogger(__name__)
 
@@ -54,12 +175,12 @@ def choose_configurator_plugins(config, plugins, verb):
     if verb == "run" and req_auth == req_inst:
         # Unless the user has explicitly asked for different auth/install,
         # only consider offering a single choice
-        authenticator = installer = display_ops.pick_configurator(config, req_inst, plugins)
+        authenticator = installer = pick_configurator(config, req_inst, plugins)
     else:
         if need_inst or req_inst:
-            installer = display_ops.pick_installer(config, req_inst, plugins)
+            installer = pick_installer(config, req_inst, plugins)
         if need_auth:
-            authenticator = display_ops.pick_authenticator(config, req_auth, plugins)
+            authenticator = pick_authenticator(config, req_auth, plugins)
     logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
 
     # Report on any failures
diff --git a/letsencrypt/tests/display/ops_test.py b/letsencrypt/tests/display/ops_test.py
index 9a39e1538..a7e8e1d74 100644
--- a/letsencrypt/tests/display/ops_test.py
+++ b/letsencrypt/tests/display/ops_test.py
@@ -76,7 +76,7 @@ class PickPluginTest(unittest.TestCase):
         self.ifaces = []
 
     def _call(self):
-        from letsencrypt.display.ops import pick_plugin
+        from letsencrypt.plugins.selection import pick_plugin
         return pick_plugin(self.config, self.default, self.reg,
                            self.question, self.ifaces)
 
@@ -149,17 +149,17 @@ class ConveniencePickPluginTest(unittest.TestCase):
             config, default, plugins, "Question?", ifaces)
 
     def test_authenticator(self):
-        from letsencrypt.display.ops import pick_authenticator
+        from letsencrypt.plugins.selection import pick_authenticator
         self._test(pick_authenticator, (interfaces.IAuthenticator,))
 
     def test_installer(self):
-        from letsencrypt.display.ops import pick_installer
+        from letsencrypt.plugins.selection import pick_installer
         self._test(pick_installer, (interfaces.IInstaller,))
 
     def test_configurator(self):
-        from letsencrypt.display.ops import pick_configurator
-        self._test(pick_configurator, (
-            interfaces.IAuthenticator, interfaces.IInstaller))
+        from letsencrypt.plugins.selection import pick_configurator
+        self._test(pick_configurator,
+            (interfaces.IAuthenticator, interfaces.IInstaller))
 
 
 class GetEmailTest(unittest.TestCase):

From 1c652716a25e0369fe5c3088b50a7c6032eb42a4 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 15:37:24 -0800
Subject: [PATCH 08/29] Start splitting out tests for plugins.selection

---
 letsencrypt/cli.py                    |   6 +-
 letsencrypt/client.py                 |   2 +
 letsencrypt/display/ops.py            |   2 -
 letsencrypt/main.py                   |   2 +
 letsencrypt/plugins/selection.py      |  10 +-
 letsencrypt/plugins/selection_test.py | 149 ++++++++++++++++++++++++++
 letsencrypt/tests/display/ops_test.py | 140 ------------------------
 7 files changed, 165 insertions(+), 146 deletions(-)
 create mode 100644 letsencrypt/plugins/selection_test.py

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 4788a41e5..cb6f66be9 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -13,6 +13,7 @@ import configargparse
 import OpenSSL
 import six
 
+import letsencrypt
 
 from letsencrypt import constants
 from letsencrypt import crypto_util
@@ -20,9 +21,8 @@ from letsencrypt import errors
 from letsencrypt import interfaces
 from letsencrypt import le_util
 
-from letsencrypt.display import ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
-from letsencrypt.plugin.selection import cli_plugin_requests
+from letsencrypt.plugins.selection import cli_plugin_requests
 
 
 logger = logging.getLogger(__name__)
@@ -118,7 +118,7 @@ def set_by_cli(var):
         detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
-        auth, inst = plugin_selection.cli_plugin_requests(detector)
+        auth, inst = cli_plugin_requests(detector)
         detector.authenticator = auth if auth else ""
         detector.installer = inst if inst else ""
         logger.debug("Default Detector is %r", detector)
diff --git a/letsencrypt/client.py b/letsencrypt/client.py
index 1655c1fa5..d559b6311 100644
--- a/letsencrypt/client.py
+++ b/letsencrypt/client.py
@@ -11,6 +11,8 @@ from acme import client as acme_client
 from acme import jose
 from acme import messages
 
+import letsencrypt
+
 from letsencrypt import account
 from letsencrypt import auth_handler
 from letsencrypt import configuration
diff --git a/letsencrypt/display/ops.py b/letsencrypt/display/ops.py
index d60073ce0..302051b1b 100644
--- a/letsencrypt/display/ops.py
+++ b/letsencrypt/display/ops.py
@@ -8,8 +8,6 @@ from letsencrypt import errors
 from letsencrypt import interfaces
 from letsencrypt import le_util
 from letsencrypt.display import util as display_util
-import letsencrypt.plugins.selection
-
 
 logger = logging.getLogger(__name__)
 
diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index 48e6e8505..153e118a4 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -6,6 +6,8 @@ import os
 import sys
 import zope.component
 
+import letsencrypt
+
 from letsencrypt import account
 from letsencrypt import client
 from letsencrypt import cli
diff --git a/letsencrypt/plugins/selection.py b/letsencrypt/plugins/selection.py
index bf6187950..9c5a7804a 100644
--- a/letsencrypt/plugins/selection.py
+++ b/letsencrypt/plugins/selection.py
@@ -1,6 +1,14 @@
+"""Decide which plugins to use for authentication & installation"""
 from __future__ import print_function
+
 import os
-from letsencrypt import errors, interfaces
+import logging
+
+import zope.component
+
+from letsencrypt import errors
+from letsencrypt import interfaces
+
 from letsencrypt.display import util as display_util
 
 logger = logging.getLogger(__name__)
diff --git a/letsencrypt/plugins/selection_test.py b/letsencrypt/plugins/selection_test.py
new file mode 100644
index 000000000..0beaab076
--- /dev/null
+++ b/letsencrypt/plugins/selection_test.py
@@ -0,0 +1,149 @@
+"""Tests for letsenecrypt.plugins.selection"""
+import sys
+import unittest
+
+import mock
+import zope.component
+
+from letsencrypt.display import util as display_util
+from letsencrypt import interfaces
+
+
+class ConveniencePickPluginTest(unittest.TestCase):
+    """Tests for letsencrypt.plugins.selection.pick_*."""
+
+    def _test(self, fun, ifaces):
+        config = mock.Mock()
+        default = mock.Mock()
+        plugins = mock.Mock()
+
+        with mock.patch("letsencrypt.plugins.selection.pick_plugin") as mock_p:
+            mock_p.return_value = "foo"
+            self.assertEqual("foo", fun(config, default, plugins, "Question?"))
+        mock_p.assert_called_once_with(
+            config, default, plugins, "Question?", ifaces)
+
+    def test_authenticator(self):
+        from letsencrypt.plugins.selection import pick_authenticator
+        self._test(pick_authenticator, (interfaces.IAuthenticator,))
+
+    def test_installer(self):
+        from letsencrypt.plugins.selection import pick_installer
+        self._test(pick_installer, (interfaces.IInstaller,))
+
+    def test_configurator(self):
+        from letsencrypt.plugins.selection import pick_configurator
+        self._test(pick_configurator,
+            (interfaces.IAuthenticator, interfaces.IInstaller))
+
+
+class PickPluginTest(unittest.TestCase):
+    """Tests for letsencrypt.plugins.selection.pick_plugin."""
+
+    def setUp(self):
+        self.config = mock.Mock(noninteractive_mode=False)
+        self.default = None
+        self.reg = mock.MagicMock()
+        self.question = "Question?"
+        self.ifaces = []
+
+    def _call(self):
+        from letsencrypt.plugins.selection import pick_plugin
+        return pick_plugin(self.config, self.default, self.reg,
+                           self.question, self.ifaces)
+
+    def test_default_provided(self):
+        self.default = "foo"
+        self._call()
+        self.assertEqual(1, self.reg.filter.call_count)
+
+    def test_no_default(self):
+        self._call()
+        self.assertEqual(1, self.reg.visible().ifaces.call_count)
+
+    def test_no_candidate(self):
+        self.assertTrue(self._call() is None)
+
+    def test_single(self):
+        plugin_ep = mock.MagicMock()
+        plugin_ep.init.return_value = "foo"
+        plugin_ep.misconfigured = False
+
+        self.reg.visible().ifaces().verify().available.return_value = {
+            "bar": plugin_ep}
+        self.assertEqual("foo", self._call())
+
+    def test_single_misconfigured(self):
+        plugin_ep = mock.MagicMock()
+        plugin_ep.init.return_value = "foo"
+        plugin_ep.misconfigured = True
+
+        self.reg.visible().ifaces().verify().available.return_value = {
+            "bar": plugin_ep}
+        self.assertTrue(self._call() is None)
+
+    def test_multiple(self):
+        plugin_ep = mock.MagicMock()
+        plugin_ep.init.return_value = "foo"
+        self.reg.visible().ifaces().verify().available.return_value = {
+            "bar": plugin_ep,
+            "baz": plugin_ep,
+        }
+        with mock.patch("letsencrypt.plugins.selection.choose_plugin") as mock_choose:
+            mock_choose.return_value = plugin_ep
+            self.assertEqual("foo", self._call())
+        mock_choose.assert_called_once_with(
+            [plugin_ep, plugin_ep], self.question)
+
+    def test_choose_plugin_none(self):
+        self.reg.visible().ifaces().verify().available.return_value = {
+            "bar": None,
+            "baz": None,
+        }
+
+        with mock.patch("letsencrypt.plugins.selection.choose_plugin") as mock_choose:
+            mock_choose.return_value = None
+            self.assertTrue(self._call() is None)
+
+
+class ChoosePluginTest(unittest.TestCase):
+    """Tests for letsencrypt.plugins.selection.choose_plugin."""
+
+    def setUp(self):
+        zope.component.provideUtility(display_util.FileDisplay(sys.stdout))
+        self.mock_apache = mock.Mock(
+            description_with_name="a", misconfigured=True)
+        self.mock_stand = mock.Mock(
+            description_with_name="s", misconfigured=False)
+        self.mock_stand.init().more_info.return_value = "standalone"
+        self.plugins = [
+            self.mock_apache,
+            self.mock_stand,
+        ]
+
+    def _call(self):
+        from letsencrypt.plugins.selection import choose_plugin
+        return choose_plugin(self.plugins, "Question?")
+
+    @mock.patch("letsencrypt.plugins.selection.z_util")
+    def test_selection(self, mock_util):
+        mock_util().menu.side_effect = [(display_util.OK, 0),
+                                        (display_util.OK, 1)]
+        self.assertEqual(self.mock_stand, self._call())
+        self.assertEqual(mock_util().notification.call_count, 1)
+
+    @mock.patch("letsencrypt.plugins.selection.z_util")
+    def test_more_info(self, mock_util):
+        mock_util().menu.side_effect = [
+            (display_util.HELP, 0),
+            (display_util.HELP, 1),
+            (display_util.OK, 1),
+        ]
+
+        self.assertEqual(self.mock_stand, self._call())
+        self.assertEqual(mock_util().notification.call_count, 2)
+
+    @mock.patch("letsencrypt.plugins.selection.z_util")
+    def test_no_choice(self, mock_util):
+        mock_util().menu.return_value = (display_util.CANCEL, 0)
+        self.assertTrue(self._call() is None)
diff --git a/letsencrypt/tests/display/ops_test.py b/letsencrypt/tests/display/ops_test.py
index a7e8e1d74..8a52c872b 100644
--- a/letsencrypt/tests/display/ops_test.py
+++ b/letsencrypt/tests/display/ops_test.py
@@ -22,146 +22,6 @@ from letsencrypt.tests import test_util
 KEY = jose.JWKRSA.load(test_util.load_vector("rsa512_key.pem"))
 
 
-class ChoosePluginTest(unittest.TestCase):
-    """Tests for letsencrypt.display.ops.choose_plugin."""
-
-    def setUp(self):
-        zope.component.provideUtility(display_util.FileDisplay(sys.stdout))
-        self.mock_apache = mock.Mock(
-            description_with_name="a", misconfigured=True)
-        self.mock_stand = mock.Mock(
-            description_with_name="s", misconfigured=False)
-        self.mock_stand.init().more_info.return_value = "standalone"
-        self.plugins = [
-            self.mock_apache,
-            self.mock_stand,
-        ]
-
-    def _call(self):
-        from letsencrypt.display.ops import choose_plugin
-        return choose_plugin(self.plugins, "Question?")
-
-    @mock.patch("letsencrypt.display.ops.util")
-    def test_selection(self, mock_util):
-        mock_util().menu.side_effect = [(display_util.OK, 0),
-                                        (display_util.OK, 1)]
-        self.assertEqual(self.mock_stand, self._call())
-        self.assertEqual(mock_util().notification.call_count, 1)
-
-    @mock.patch("letsencrypt.display.ops.util")
-    def test_more_info(self, mock_util):
-        mock_util().menu.side_effect = [
-            (display_util.HELP, 0),
-            (display_util.HELP, 1),
-            (display_util.OK, 1),
-        ]
-
-        self.assertEqual(self.mock_stand, self._call())
-        self.assertEqual(mock_util().notification.call_count, 2)
-
-    @mock.patch("letsencrypt.display.ops.util")
-    def test_no_choice(self, mock_util):
-        mock_util().menu.return_value = (display_util.CANCEL, 0)
-        self.assertTrue(self._call() is None)
-
-
-class PickPluginTest(unittest.TestCase):
-    """Tests for letsencrypt.display.ops.pick_plugin."""
-
-    def setUp(self):
-        self.config = mock.Mock(noninteractive_mode=False)
-        self.default = None
-        self.reg = mock.MagicMock()
-        self.question = "Question?"
-        self.ifaces = []
-
-    def _call(self):
-        from letsencrypt.plugins.selection import pick_plugin
-        return pick_plugin(self.config, self.default, self.reg,
-                           self.question, self.ifaces)
-
-    def test_default_provided(self):
-        self.default = "foo"
-        self._call()
-        self.assertEqual(1, self.reg.filter.call_count)
-
-    def test_no_default(self):
-        self._call()
-        self.assertEqual(1, self.reg.visible().ifaces.call_count)
-
-    def test_no_candidate(self):
-        self.assertTrue(self._call() is None)
-
-    def test_single(self):
-        plugin_ep = mock.MagicMock()
-        plugin_ep.init.return_value = "foo"
-        plugin_ep.misconfigured = False
-
-        self.reg.visible().ifaces().verify().available.return_value = {
-            "bar": plugin_ep}
-        self.assertEqual("foo", self._call())
-
-    def test_single_misconfigured(self):
-        plugin_ep = mock.MagicMock()
-        plugin_ep.init.return_value = "foo"
-        plugin_ep.misconfigured = True
-
-        self.reg.visible().ifaces().verify().available.return_value = {
-            "bar": plugin_ep}
-        self.assertTrue(self._call() is None)
-
-    def test_multiple(self):
-        plugin_ep = mock.MagicMock()
-        plugin_ep.init.return_value = "foo"
-        self.reg.visible().ifaces().verify().available.return_value = {
-            "bar": plugin_ep,
-            "baz": plugin_ep,
-        }
-        with mock.patch("letsencrypt.display.ops.choose_plugin") as mock_choose:
-            mock_choose.return_value = plugin_ep
-            self.assertEqual("foo", self._call())
-        mock_choose.assert_called_once_with(
-            [plugin_ep, plugin_ep], self.question)
-
-    def test_choose_plugin_none(self):
-        self.reg.visible().ifaces().verify().available.return_value = {
-            "bar": None,
-            "baz": None,
-        }
-
-        with mock.patch("letsencrypt.display.ops.choose_plugin") as mock_choose:
-            mock_choose.return_value = None
-            self.assertTrue(self._call() is None)
-
-
-class ConveniencePickPluginTest(unittest.TestCase):
-    """Tests for letsencrypt.display.ops.pick_*."""
-
-    def _test(self, fun, ifaces):
-        config = mock.Mock()
-        default = mock.Mock()
-        plugins = mock.Mock()
-
-        with mock.patch("letsencrypt.display.ops.pick_plugin") as mock_p:
-            mock_p.return_value = "foo"
-            self.assertEqual("foo", fun(config, default, plugins, "Question?"))
-        mock_p.assert_called_once_with(
-            config, default, plugins, "Question?", ifaces)
-
-    def test_authenticator(self):
-        from letsencrypt.plugins.selection import pick_authenticator
-        self._test(pick_authenticator, (interfaces.IAuthenticator,))
-
-    def test_installer(self):
-        from letsencrypt.plugins.selection import pick_installer
-        self._test(pick_installer, (interfaces.IInstaller,))
-
-    def test_configurator(self):
-        from letsencrypt.plugins.selection import pick_configurator
-        self._test(pick_configurator,
-            (interfaces.IAuthenticator, interfaces.IInstaller))
-
-
 class GetEmailTest(unittest.TestCase):
     """Tests for letsencrypt.display.ops.get_email."""
 

From 6e9d2b7116c0dcc6cb327c7fd3180b0bf78dac47 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 15:59:11 -0800
Subject: [PATCH 09/29] Adjust mockery...

---
 letsencrypt/cli.py                    |  5 ++---
 letsencrypt/main.py                   |  8 ++++----
 letsencrypt/tests/cli_test.py         | 10 +++++-----
 letsencrypt/tests/display/ops_test.py | 24 ++++++++++++------------
 4 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index cb6f66be9..1922cf73b 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -22,8 +22,7 @@ from letsencrypt import interfaces
 from letsencrypt import le_util
 
 from letsencrypt.plugins import disco as plugins_disco
-from letsencrypt.plugins.selection import cli_plugin_requests
-
+import letsencrypt.plugins.selection as plugin_selection 
 
 logger = logging.getLogger(__name__)
 
@@ -118,7 +117,7 @@ def set_by_cli(var):
         detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
-        auth, inst = cli_plugin_requests(detector)
+        auth, inst = plugin_selection.cli_plugin_requests(detector)
         detector.authenticator = auth if auth else ""
         detector.installer = inst if inst else ""
         logger.debug("Default Detector is %r", detector)
diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index 153e118a4..d9e1456ad 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -25,7 +25,7 @@ from letsencrypt import storage
 
 from letsencrypt.display import util as display_util, ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
-from letsencrypt.plugins.selection import choose_configurator_plugins
+from letsencrypt.plugins import selection as ps
 
 import traceback
 import logging.handlers
@@ -405,7 +405,7 @@ def install(config, plugins):
     # this function ...
 
     try:
-        installer, _ = choose_configurator_plugins(config, plugins, "install")
+        installer, _ = ps.choose_configurator_plugins(config, plugins, "install")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -480,7 +480,7 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     # TODO: Make run as close to auth + install as possible
     # Possible difficulties: config.csr was hacked into auth
     try:
-        installer, authenticator = choose_configurator_plugins(config, plugins, "run")
+        installer, authenticator = ps.choose_configurator_plugins(config, plugins, "run")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -510,7 +510,7 @@ def obtain_cert(config, plugins, lineage=None):
     # pylint: disable=too-many-locals
     try:
         # installers are used in auth mode to determine domain names
-        installer, authenticator = choose_configurator_plugins(config, plugins, "certonly")
+        installer, authenticator = ps.choose_configurator_plugins(config, plugins, "certonly")
     except errors.PluginSelectionError as e:
         logger.info("Could not choose appropriate plugin: %s", e)
         raise
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index f1f539016..918addcf8 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -201,12 +201,12 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertEqual(args.chain_path, os.path.abspath(chain))
         self.assertEqual(args.fullchain_path, os.path.abspath(fullchain))
 
-    @mock.patch('letsencrypt.main.cli.record_chosen_plugins')
-    @mock.patch('letsencrypt.main.cli.display_ops')
-    def test_installer_selection(self, mock_display_ops, _rec):
+    @mock.patch('letsencrypt.main.ps.record_chosen_plugins')
+    @mock.patch('letsencrypt.main.ps.pick_installer')
+    def test_installer_selection(self, mock_pick_installer, _rec):
         self._call(['install', '--domains', 'foo.bar', '--cert-path', 'cert',
                     '--key-path', 'key', '--chain-path', 'chain'])
-        self.assertEqual(mock_display_ops.pick_installer.call_count, 1)
+        self.assertEqual(mock_pick_installer.call_count, 1)
 
     @mock.patch('letsencrypt.le_util.exe_exists')
     def test_configurator_selection(self, mock_exe_exists):
@@ -493,7 +493,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self._webroot_map_test(simple_map, "/tmp2", "eg2.com,eg.com", expected_map, domains)
 
         # test inclusion of interactively specified domains in the webroot map
-        with mock.patch('letsencrypt.cli.display_ops.choose_names') as mock_choose:
+        with mock.patch('letsencrypt.display.ops.choose_names') as mock_choose:
             mock_choose.return_value = domains
             expected_map["eg2.com"] = "/tmp"
             self._webroot_map_test(None, "/tmp", None, expected_map, domains)
diff --git a/letsencrypt/tests/display/ops_test.py b/letsencrypt/tests/display/ops_test.py
index 8a52c872b..0dacdfea8 100644
--- a/letsencrypt/tests/display/ops_test.py
+++ b/letsencrypt/tests/display/ops_test.py
@@ -101,17 +101,17 @@ class ChooseAccountTest(unittest.TestCase):
         from letsencrypt.display import ops
         return ops.choose_account(accounts)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_one(self, mock_util):
         mock_util().menu.return_value = (display_util.OK, 0)
         self.assertEqual(self._call([self.acc1]), self.acc1)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_two(self, mock_util):
         mock_util().menu.return_value = (display_util.OK, 1)
         self.assertEqual(self._call([self.acc1, self.acc2]), self.acc2)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_cancel(self, mock_util):
         mock_util().menu.return_value = (display_util.CANCEL, 1)
         self.assertTrue(self._call([self.acc1, self.acc2]) is None)
@@ -199,12 +199,12 @@ class ChooseNamesTest(unittest.TestCase):
         self._call(None)
         self.assertEqual(mock_manual.call_count, 1)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_no_installer_cancel(self, mock_util):
         mock_util().input.return_value = (display_util.CANCEL, [])
         self.assertEqual(self._call(None), [])
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_no_names_choose(self, mock_util):
         self.mock_install().get_all_names.return_value = set()
         mock_util().yesno.return_value = True
@@ -215,14 +215,14 @@ class ChooseNamesTest(unittest.TestCase):
         self.assertEqual(mock_util().input.call_count, 1)
         self.assertEqual(actual_doms, [domain])
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_no_names_quit(self, mock_util):
         self.mock_install().get_all_names.return_value = set()
         mock_util().yesno.return_value = False
 
         self.assertEqual(self._call(self.mock_install), [])
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_filter_names_valid_return(self, mock_util):
         self.mock_install.get_all_names.return_value = set(["example.com"])
         mock_util().checklist.return_value = (display_util.OK, ["example.com"])
@@ -231,14 +231,14 @@ class ChooseNamesTest(unittest.TestCase):
         self.assertEqual(names, ["example.com"])
         self.assertEqual(mock_util().checklist.call_count, 1)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_filter_names_nothing_selected(self, mock_util):
         self.mock_install.get_all_names.return_value = set(["example.com"])
         mock_util().checklist.return_value = (display_util.OK, [])
 
         self.assertEqual(self._call(self.mock_install), [])
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_filter_names_cancel(self, mock_util):
         self.mock_install.get_all_names.return_value = set(["example.com"])
         mock_util().checklist.return_value = (
@@ -257,7 +257,7 @@ class ChooseNamesTest(unittest.TestCase):
         self.assertEqual(get_valid_domains(all_invalid), [])
         self.assertEqual(len(get_valid_domains(two_valid)), 2)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_choose_manually(self, mock_util):
         from letsencrypt.display.ops import _choose_names_manually
         # No retry
@@ -305,7 +305,7 @@ class SuccessInstallationTest(unittest.TestCase):
         from letsencrypt.display.ops import success_installation
         success_installation(names)
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_success_installation(self, mock_util):
         mock_util().notification.return_value = None
         names = ["example.com", "abc.com"]
@@ -327,7 +327,7 @@ class SuccessRenewalTest(unittest.TestCase):
         from letsencrypt.display.ops import success_renewal
         success_renewal(names, "renew")
 
-    @mock.patch("letsencrypt.display.ops.util")
+    @mock.patch("letsencrypt.display.ops.z_util")
     def test_success_renewal(self, mock_util):
         mock_util().notification.return_value = None
         names = ["example.com", "abc.com"]

From fcf1ea32d89c942dc65fb8e877c9d577dd63ecb7 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 16:01:52 -0800
Subject: [PATCH 10/29] fixup

---
 letsencrypt/cli.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 1922cf73b..f0ea7153b 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -22,7 +22,7 @@ from letsencrypt import interfaces
 from letsencrypt import le_util
 
 from letsencrypt.plugins import disco as plugins_disco
-import letsencrypt.plugins.selection as plugin_selection 
+import letsencrypt.plugins.selection as plugin_selection
 
 logger = logging.getLogger(__name__)
 

From aac692f8ca812e62ba599ddddfe67eff23786e83 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 17:27:10 -0800
Subject: [PATCH 11/29] Mock-picking fix

---
 letsencrypt/tests/client_test.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py
index 8be2178b9..4331be59d 100644
--- a/letsencrypt/tests/client_test.py
+++ b/letsencrypt/tests/client_test.py
@@ -422,9 +422,8 @@ class RollbackTest(unittest.TestCase):
     @classmethod
     def _call(cls, checkpoints, side_effect):
         from letsencrypt.client import rollback
-        with mock.patch("letsencrypt.client"
-                        ".display_ops.pick_installer") as mock_pick_installer:
-            mock_pick_installer.side_effect = side_effect
+        with mock.patch("letsencrypt.client.plugin_selection.pick_installer") as mpi
+            mpi.side_effect = side_effect
             rollback(None, checkpoints, {}, mock.MagicMock())
 
     def test_no_problems(self):

From b888a2bd526d758d5c5f2d23c16870db1212b186 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Fri, 11 Mar 2016 17:30:02 -0800
Subject: [PATCH 12/29] Fixup

---
 letsencrypt/tests/client_test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py
index 4331be59d..f14a52407 100644
--- a/letsencrypt/tests/client_test.py
+++ b/letsencrypt/tests/client_test.py
@@ -422,7 +422,7 @@ class RollbackTest(unittest.TestCase):
     @classmethod
     def _call(cls, checkpoints, side_effect):
         from letsencrypt.client import rollback
-        with mock.patch("letsencrypt.client.plugin_selection.pick_installer") as mpi
+        with mock.patch("letsencrypt.client.plugin_selection.pick_installer") as mpi:
             mpi.side_effect = side_effect
             rollback(None, checkpoints, {}, mock.MagicMock())
 

From 2054150abb430e125b89cde7eaa03a8b34896932 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Mon, 14 Mar 2016 15:35:20 -0700
Subject: [PATCH 13/29] Work in progress

---
 letsencrypt/cli.py     | 14 ++++++++
 letsencrypt/storage.py | 77 +++++++++++++++++++++++++++++++++---------
 2 files changed, 75 insertions(+), 16 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 024f53d0b..0992f0877 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -537,6 +537,20 @@ def diagnose_configurator_problem(cfg_type, requested, plugins):
     raise errors.PluginSelectionError(msg)
 
 
+def _relevant(option):
+    """
+    Is this option one that could be restored and used for future renewal purposes?
+    :param str option: the name of the option
+
+    :rtype: bool
+    """
+    # The list() here produces a list of the plugin names as strings.
+    plugins = list(plugins_disco.PluginsRegistry.find_all())
+    return (option in STR_CONFIG_ITEMS
+            or option in INT_CONFIG_ITEMS
+            or any(option.startswith(x + "_") for x in plugins))
+
+
 def set_configurator(previously, now):
     """
     Setting configurators multiple ways is okay, as long as they all agree
diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index cff2d53e1..bea686e19 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -50,13 +50,12 @@ def add_time_interval(base_time, interval, textparser=parsedatetime.Calendar()):
     return textparser.parseDT(interval, base_time, tzinfo=tzinfo)[0]
 
 
-def write_renewal_config(filename, target, cli_config):
+def write_renewal_config(filename, target, relevant_data):
     """Writes a renewal config file with the specified name and values.
 
     :param str filename: Absolute path to the config file
     :param dict target: Maps ALL_FOUR to their symlink paths
-    :param .RenewerConfiguration cli_config: parsed command line
-        arguments
+    :param dict relevant_data: Renewal configuration options to save
 
     :returns: Configuration object for the new config file
     :rtype: configobj.ConfigObj
@@ -67,18 +66,11 @@ def write_renewal_config(filename, target, cli_config):
     for kind in ALL_FOUR:
         config[kind] = target[kind]
 
-    # XXX: We clearly need a more general and correct way of getting
-    # options into the configobj for the RenewableCert instance.
-    # This is a quick-and-dirty way to do it to allow integration
-    # testing to start.  (Note that the config parameter to new_lineage
-    # ideally should be a ConfigObj, but in this case a dict will be
-    # accepted in practice.)
-    renewalparams = vars(cli_config.namespace)
-    if renewalparams:
-        config["renewalparams"] = renewalparams
+    if relevant_data:
+        config["renewalparams"] = relevant_data
         config.comments["renewalparams"] = ["",
-                                            "Options and defaults used"
-                                            " in the renewal process"]
+                                            "Options used in "
+                                            "the renewal process"]
 
     # TODO: add human-readable comments explaining other available
     #       parameters
@@ -106,7 +98,10 @@ def update_configuration(lineagename, target, cli_config):
     # If an existing tempfile exists, delete it
     if os.path.exists(temp_filename):
         os.unlink(temp_filename)
-    write_renewal_config(temp_filename, target, cli_config)
+
+    # Save only the config items that are relevant to renewal
+    values = relevant_values(vars(cli_config.namespace))
+    write_renewal_config(temp_filename, target, values)
     os.rename(temp_filename, config_filename)
 
     return configobj.ConfigObj(config_filename)
@@ -127,6 +122,50 @@ def get_link_target(link):
     return os.path.abspath(target)
 
 
+def relevant_values(all_values):
+    """Return a new dict containing only items relevant for renewal.
+
+    :param dict all_values: The original values.
+
+    :returns: A new dictionary containing items that can be used in renewal.
+    :rtype dict:"""
+
+    from letsencrypt import cli
+
+    import code
+    code.interact(local=locals())
+    def _is_cli_default(option, value):
+        # Look through the CLI parser defaults and see if this option is
+        # both present and equal to the specified value. If not, return
+        # False.
+        for x in cli._parser.parser._actions:
+            if x.dest == option:
+                if x.default == value:
+                    return True
+                else:
+                    break
+        return False
+
+    values = dict()
+    for option, value in all_values.iteritems():
+        # Try to find reasons to store this item in the
+        # renewal config.  It can be stored if it is relevant and
+        # (it is _set_by_cli() or flag_default() is different
+        # from the value or flag_default() doesn't exist).
+        if cli._relevant(option):
+            if (cli._set_by_cli(option)
+                or not _is_cli_default(option, value)):
+#                or option not in constants.CLI_DEFAULTS
+#                or constants.CLI_DEFAULTS[option] != value):
+                values[option] = value
+                print option, value
+            else:
+                print "not saving", option, value
+        else:
+            print "not relevant", option, value
+    return values
+
+
 class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     """Renewable certificate.
 
@@ -690,6 +729,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         :rtype: :class:`storage.renewableCert`
 
         """
+
         # Examine the configuration and find the new lineage's name
         for i in (cli_config.renewal_configs_dir, cli_config.archive_dir,
                   cli_config.live_dir):
@@ -744,7 +784,12 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         # Document what we've done in a new renewal config file
         config_file.close()
-        new_config = write_renewal_config(config_filename, target, cli_config)
+
+        # Save only the config items that are relevant to renewal
+        values = relevant_values(vars(cli_config.namespace))
+        print (values)
+
+        new_config = write_renewal_config(config_filename, target, values)
         return cls(new_config.filename, cli_config)
 
     def save_successor(self, prior_version, new_cert,

From bd1b5229406991a03dcccea01f483607bb26fd10 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Mon, 14 Mar 2016 17:11:09 -0700
Subject: [PATCH 14/29] Remove interact()

---
 letsencrypt/storage.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index bea686e19..672d3ff1f 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -132,8 +132,6 @@ def relevant_values(all_values):
 
     from letsencrypt import cli
 
-    import code
-    code.interact(local=locals())
     def _is_cli_default(option, value):
         # Look through the CLI parser defaults and see if this option is
         # both present and equal to the specified value. If not, return

From b31372b2714f00db21cdfb44369b27b64ab4ea4f Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Mon, 14 Mar 2016 17:13:52 -0700
Subject: [PATCH 15/29] Remove debug prints

---
 letsencrypt/storage.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 672d3ff1f..263803d7f 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -157,10 +157,6 @@ def relevant_values(all_values):
 #                or constants.CLI_DEFAULTS[option] != value):
                 values[option] = value
                 print option, value
-            else:
-                print "not saving", option, value
-        else:
-            print "not relevant", option, value
     return values
 
 

From 62679b2b21c4ee49a4be055e68828a00aa1ed115 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Mon, 14 Mar 2016 17:28:35 -0700
Subject: [PATCH 16/29] Test coverage for storage.py changes

---
 letsencrypt/tests/storage_test.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/letsencrypt/tests/storage_test.py b/letsencrypt/tests/storage_test.py
index 7bc31dab3..0d89156d3 100644
--- a/letsencrypt/tests/storage_test.py
+++ b/letsencrypt/tests/storage_test.py
@@ -557,6 +557,22 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.exists(temp_config_file))
 
+    def test_relevant_values(self):
+        """Test that relevant_values() can reject an irrelevant value."""
+        from letsencrypt import storage
+        self.assertEqual(storage.relevant_values({"hello": "there"}), {})
+
+    def test_relevant_values_default(self):
+        """Test that relevant_values() can reject a default value."""
+        from letsencrypt import storage
+        self.assertEqual(storage.relevant_values({"rsa_key_size": 2048}), {})
+
+    def test_relevant_values_nondefault(self):
+        """Test that relevant_values() can retain a non-default value."""
+        from letsencrypt import storage
+        self.assertEqual(storage.relevant_values({"rsa_key_size": 12}),
+                         {"rsa_key_size": 12})
+
     def test_new_lineage(self):
         """Test for new_lineage() class method."""
         from letsencrypt import storage

From 29a4b2a37e557c97e864e2c38abce0de72966978 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Mon, 14 Mar 2016 17:40:26 -0700
Subject: [PATCH 17/29] Remove two more debug prints

---
 letsencrypt/storage.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 263803d7f..5f5064f7d 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -156,7 +156,6 @@ def relevant_values(all_values):
 #                or option not in constants.CLI_DEFAULTS
 #                or constants.CLI_DEFAULTS[option] != value):
                 values[option] = value
-                print option, value
     return values
 
 
@@ -781,7 +780,6 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         # Save only the config items that are relevant to renewal
         values = relevant_values(vars(cli_config.namespace))
-        print (values)
 
         new_config = write_renewal_config(config_filename, target, values)
         return cls(new_config.filename, cli_config)

From 1f22b3cbefab8e9100e6d2e6a5f502ea1f4cf6ae Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Mon, 14 Mar 2016 20:58:20 -0700
Subject: [PATCH 18/29] Move _relevant from cli.py to storage.py

---
 letsencrypt/cli.py     | 14 --------------
 letsencrypt/storage.py | 18 +++++++++++++++++-
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 12ed74edc..8e545a5de 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -537,20 +537,6 @@ def diagnose_configurator_problem(cfg_type, requested, plugins):
     raise errors.PluginSelectionError(msg)
 
 
-def _relevant(option):
-    """
-    Is this option one that could be restored and used for future renewal purposes?
-    :param str option: the name of the option
-
-    :rtype: bool
-    """
-    # The list() here produces a list of the plugin names as strings.
-    plugins = list(plugins_disco.PluginsRegistry.find_all())
-    return (option in STR_CONFIG_ITEMS
-            or option in INT_CONFIG_ITEMS
-            or any(option.startswith(x + "_") for x in plugins))
-
-
 def set_configurator(previously, now):
     """
     Setting configurators multiple ways is okay, as long as they all agree
diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 5f5064f7d..5e5d70518 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -122,6 +122,22 @@ def get_link_target(link):
     return os.path.abspath(target)
 
 
+def _relevant(option):
+    """
+    Is this option one that could be restored for future renewal purposes?
+    :param str option: the name of the option
+
+    :rtype: bool
+    """
+    # The list() here produces a list of the plugin names as strings.
+    from letsencrypt import cli
+    from letsencrypt.plugins import disco as plugins_disco
+    plugins = list(plugins_disco.PluginsRegistry.find_all())
+    return (option in cli.STR_CONFIG_ITEMS
+            or option in cli.INT_CONFIG_ITEMS
+            or any(option.startswith(x + "_") for x in plugins))
+
+
 def relevant_values(all_values):
     """Return a new dict containing only items relevant for renewal.
 
@@ -150,7 +166,7 @@ def relevant_values(all_values):
         # renewal config.  It can be stored if it is relevant and
         # (it is _set_by_cli() or flag_default() is different
         # from the value or flag_default() doesn't exist).
-        if cli._relevant(option):
+        if _relevant(option):
             if (cli._set_by_cli(option)
                 or not _is_cli_default(option, value)):
 #                or option not in constants.CLI_DEFAULTS

From 0847c73fd8c6626e3d64f99930cef934224a1649 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Thu, 17 Mar 2016 16:22:29 -0700
Subject: [PATCH 19/29] Fix dependency issue

---
 letsencrypt/cli.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 9bbbe923a..0ba5d321e 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -355,10 +355,11 @@ class HelpfulArgumentParser(object):
 
     def __init__(self, args, plugins, detect_defaults=False):
         from letsencrypt import main
+        from letsencrypt import renew
         self.VERBS = {"auth": main.obtain_cert, "certonly": main.obtain_cert,
                       "config_changes": main.config_changes, "run": main.run,
                       "install": main.install, "plugins": main.plugins_cmd,
-                      "renew": renew, "revoke": main.revoke,
+                      "renew": renew.renew, "revoke": main.revoke,
                       "rollback": main.rollback, "everything": main.run}
 
         # List of topics for which additional help can be provided

From 2d211c7aef4951e9147d35369e96b2e522c9f233 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Tue, 22 Mar 2016 12:44:40 -0700
Subject: [PATCH 20/29] Mention that we're talking about Unix OSes

---
 README.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/README.rst b/README.rst
index 874b0c450..75de094a8 100644
--- a/README.rst
+++ b/README.rst
@@ -23,11 +23,11 @@ configuring webservers to use them.
 Installation
 ------------
 
-If ``letsencrypt`` is packaged for your OS, you can install it from there, and
-run it by typing ``letsencrypt``.  Because not all operating systems have
-packages yet, we provide a temporary solution via the ``letsencrypt-auto``
-wrapper script, which obtains some dependencies from your OS and puts others
-in a python virtual environment::
+If ``letsencrypt`` is packaged for your Unix OS, you can install it from
+there, and run it by typing ``letsencrypt``.  Because not all operating
+systems have packages yet, we provide a temporary solution via the
+``letsencrypt-auto`` wrapper script, which obtains some dependencies
+from your OS and puts others in a python virtual environment::
 
   user@webserver:~$ git clone https://github.com/letsencrypt/letsencrypt
   user@webserver:~$ cd letsencrypt

From 46ef3e6374358dc65c0ee07fccc82332b7af5a20 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Tue, 22 Mar 2016 12:49:39 -0700
Subject: [PATCH 21/29] More explicit

---
 README.rst | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/README.rst b/README.rst
index 75de094a8..050cde82b 100644
--- a/README.rst
+++ b/README.rst
@@ -18,7 +18,8 @@ The Let's Encrypt Client is a fully-featured, extensible client for the Let's
 Encrypt CA (or any other CA that speaks the `ACME
 <https://github.com/ietf-wg-acme/acme/blob/master/draft-ietf-acme-acme.md>`_
 protocol) that can automate the tasks of obtaining certificates and
-configuring webservers to use them.
+configuring webservers to use them. This client runs on Unix-based operating
+systems.
 
 Installation
 ------------

From 278b8852fd3844f66af03d3ee6586ac10aee6c32 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Wed, 23 Mar 2016 14:26:58 -0700
Subject: [PATCH 22/29] Merge snafu

---
 letsencrypt/tests/cli_test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index adbe72701..55a504f82 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -54,7 +54,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         shutil.rmtree(self.tmp_dir)
         # Reset globals in cli
         # pylint: disable=protected-access
-        cli._parser = cli._set_by_cli.detector = None
+        cli._parser = cli.set_by_cli.detector = None
 
     def _call(self, args):
         "Run the cli with output streams and actual client mocked out"

From 4bc8e9a44ac7b7dcd9a96eee0a34203850d6e1ea Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Wed, 23 Mar 2016 14:30:40 -0700
Subject: [PATCH 23/29] Improve mocking

---
 letsencrypt/tests/storage_test.py | 42 ++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/letsencrypt/tests/storage_test.py b/letsencrypt/tests/storage_test.py
index 0d89156d3..9660e2688 100644
--- a/letsencrypt/tests/storage_test.py
+++ b/letsencrypt/tests/storage_test.py
@@ -493,7 +493,12 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertTrue(self.test_rc.should_autorenew())
         mock_ocsp.return_value = False
 
-    def test_save_successor(self):
+    @mock.patch("letsencrypt.storage.relevant_values")
+    def test_save_successor(self, mock_rv):
+        # Mock relevant_values() to claim that all values are relevant here
+        # (to avoid instantiating parser)
+        mock_rv.side_effect = lambda x: x
+
         for ver in xrange(1, 6):
             for kind in ALL_FOUR:
                 where = getattr(self.test_rc, kind)
@@ -557,24 +562,44 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.exists(temp_config_file))
 
-    def test_relevant_values(self):
+    @mock.patch("letsencrypt.cli._parser")
+    def test_relevant_values(self, mock_parser):
         """Test that relevant_values() can reject an irrelevant value."""
         from letsencrypt import storage
+        mock_parser.verb = "certonly"
+        mock_parser.args = ["--standalone"]
+        mock_action = mock.Mock(dest="rsa_key_size", default=2048)
+        mock_parser.parser._actions = [mock_action]
         self.assertEqual(storage.relevant_values({"hello": "there"}), {})
 
-    def test_relevant_values_default(self):
+    @mock.patch("letsencrypt.cli._parser")
+    def test_relevant_values_default(self, mock_parser):
         """Test that relevant_values() can reject a default value."""
         from letsencrypt import storage
+        mock_parser.verb = "certonly"
+        mock_parser.args = ["--standalone"]
+        mock_action = mock.Mock(dest="rsa_key_size", default=2048)
+        mock_parser.parser._actions = [mock_action]
         self.assertEqual(storage.relevant_values({"rsa_key_size": 2048}), {})
 
-    def test_relevant_values_nondefault(self):
+    @mock.patch("letsencrypt.cli._parser")
+    def test_relevant_values_nondefault(self, mock_parser):
         """Test that relevant_values() can retain a non-default value."""
         from letsencrypt import storage
+        mock_parser.verb = "certonly"
+        mock_parser.args = ["--standalone"]
+        mock_action = mock.Mock(dest="rsa_key_size", default=2048)
+        mock_parser.parser._actions = [mock_action]
         self.assertEqual(storage.relevant_values({"rsa_key_size": 12}),
                          {"rsa_key_size": 12})
 
-    def test_new_lineage(self):
+    @mock.patch("letsencrypt.storage.relevant_values")
+    def test_new_lineage(self, mock_rv):
         """Test for new_lineage() class method."""
+        # Mock relevant_values to say everything is relevant here (so we
+        # don't have to mock the parser to help it decide!)
+        mock_rv.side_effect = lambda x: x
+
         from letsencrypt import storage
         result = storage.RenewableCert.new_lineage(
             "the-lineage.com", "cert", "privkey", "chain", self.cli_config)
@@ -608,8 +633,13 @@ class RenewableCertTests(BaseRenewableCertTest):
         # TODO: Conceivably we could test that the renewal parameters actually
         #       got saved
 
-    def test_new_lineage_nonexistent_dirs(self):
+    @mock.patch("letsencrypt.storage.relevant_values")
+    def test_new_lineage_nonexistent_dirs(self, mock_rv):
         """Test that directories can be created if they don't exist."""
+        # Mock relevant_values to say everything is relevant here (so we
+        # don't have to mock the parser to help it decide!)
+        mock_rv.side_effect = lambda x: x
+
         from letsencrypt import storage
         shutil.rmtree(self.cli_config.renewal_configs_dir)
         shutil.rmtree(self.cli_config.archive_dir)

From 4f7c5b32e87809e650ff05d9a326295283fcad71 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Wed, 23 Mar 2016 14:57:01 -0700
Subject: [PATCH 24/29] Cleanup responding to protected-access problems

---
 letsencrypt/cli.py                | 18 +++++++++---------
 letsencrypt/plugins/common.py     |  2 +-
 letsencrypt/storage.py            |  6 +++---
 letsencrypt/tests/cli_test.py     |  4 ++--
 letsencrypt/tests/storage_test.py |  3 +++
 5 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index fccebe02f..fa1e0090e 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -271,20 +271,20 @@ def record_chosen_plugins(config, plugins, auth, inst):
     cn.installer = plugins.find_init(inst).name if inst else "none"
 
 
-def _set_by_cli(var):
+def set_by_cli(var):
     """
     Return True if a particular config variable has been set by the user
     (CLI or config file) including if the user explicitly set it to the
     default.  Returns False if the variable was assigned a default value.
     """
-    detector = _set_by_cli.detector
+    detector = set_by_cli.detector
     if detector is None:
         # Setup on first run: `detector` is a weird version of config in which
         # the default value of every attribute is wrangled to be boolean-false
         plugins = plugins_disco.PluginsRegistry.find_all()
         # reconstructed_args == sys.argv[1:], or whatever was passed to main()
         reconstructed_args = _parser.args + [_parser.verb]
-        detector = _set_by_cli.detector = prepare_and_parse_args(
+        detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
         auth, inst = cli_plugin_requests(detector)
@@ -312,7 +312,7 @@ def _set_by_cli(var):
     else:
         return False
 # static housekeeping var
-_set_by_cli.detector = None
+set_by_cli.detector = None
 
 def _restore_required_config_elements(config, renewalparams):
     """Sets non-plugin specific values in config from renewalparams
@@ -325,7 +325,7 @@ def _restore_required_config_elements(config, renewalparams):
     """
     # string-valued items to add if they're present
     for config_item in STR_CONFIG_ITEMS:
-        if config_item in renewalparams and not _set_by_cli(config_item):
+        if config_item in renewalparams and not set_by_cli(config_item):
             value = renewalparams[config_item]
             # Unfortunately, we've lost type information from ConfigObj,
             # so we don't know if the original was NoneType or str!
@@ -334,7 +334,7 @@ def _restore_required_config_elements(config, renewalparams):
             setattr(config.namespace, config_item, value)
     # int-valued items to add if they're present
     for config_item in INT_CONFIG_ITEMS:
-        if config_item in renewalparams and not _set_by_cli(config_item):
+        if config_item in renewalparams and not set_by_cli(config_item):
             config_value = renewalparams[config_item]
             # the default value for http01_port was None during private beta
             if config_item == "http01_port" and config_value == "None":
@@ -378,7 +378,7 @@ def _restore_plugin_configs(config, renewalparams):
         plugin_prefixes.append(renewalparams["installer"])
     for plugin_prefix in set(plugin_prefixes):
         for config_item, config_value in six.iteritems(renewalparams):
-            if config_item.startswith(plugin_prefix + "_") and not _set_by_cli(config_item):
+            if config_item.startswith(plugin_prefix + "_") and not set_by_cli(config_item):
                 # Values None, True, and False need to be treated specially,
                 # As they don't get parsed correctly based on type
                 if config_value in ("None", "True", "False"):
@@ -403,7 +403,7 @@ def _restore_webroot_config(config, renewalparams):
     if "webroot_map" in renewalparams:
         # if the user does anything that would create a new webroot map on the
         # CLI, don't use the old one
-        if not (_set_by_cli("webroot_map") or _set_by_cli("webroot_path")):
+        if not (set_by_cli("webroot_map") or set_by_cli("webroot_path")):
             setattr(config.namespace, "webroot_map", renewalparams["webroot_map"])
     elif "webroot_path" in renewalparams:
         logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path")
@@ -844,7 +844,7 @@ class HelpfulArgumentParser(object):
     def modify_arg_for_default_detection(self, *args, **kwargs):
         """
         Adding an arg, but ensure that it has a default that evaluates to false,
-        so that _set_by_cli can tell if it was set.  Only called if detect_defaults==True.
+        so that set_by_cli can tell if it was set.  Only called if detect_defaults==True.
 
         :param list *args: the names of this argument flag
         :param dict **kwargs: various argparse settings for this argument
diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py
index 319692344..f6a2c3d76 100644
--- a/letsencrypt/plugins/common.py
+++ b/letsencrypt/plugins/common.py
@@ -52,7 +52,7 @@ class Plugin(object):
         NOTE: if you add argpase arguments such that users setting them can
         create a config entry that python's bool() would consider false (ie,
         the use might set the variable to "", [], 0, etc), please ensure that
-        cli._set_by_cli() works for your variable.
+        cli.set_by_cli() works for your variable.
 
         """
 
diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 5e5d70518..3807cb63d 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -152,7 +152,7 @@ def relevant_values(all_values):
         # Look through the CLI parser defaults and see if this option is
         # both present and equal to the specified value. If not, return
         # False.
-        for x in cli._parser.parser._actions:
+        for x in cli._parser.parser._actions: # pylint: disable=protected-access
             if x.dest == option:
                 if x.default == value:
                     return True
@@ -164,10 +164,10 @@ def relevant_values(all_values):
     for option, value in all_values.iteritems():
         # Try to find reasons to store this item in the
         # renewal config.  It can be stored if it is relevant and
-        # (it is _set_by_cli() or flag_default() is different
+        # (it is set_by_cli() or flag_default() is different
         # from the value or flag_default() doesn't exist).
         if _relevant(option):
-            if (cli._set_by_cli(option)
+            if (cli.set_by_cli(option)
                 or not _is_cli_default(option, value)):
 #                or option not in constants.CLI_DEFAULTS
 #                or constants.CLI_DEFAULTS[option] != value):
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index 920bc2d99..74229cb6b 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -54,7 +54,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         shutil.rmtree(self.tmp_dir)
         # Reset globals in cli
         # pylint: disable=protected-access
-        cli._parser = cli._set_by_cli.detector = None
+        cli._parser = cli.set_by_cli.detector = None
 
     def _call(self, args):
         "Run the cli with output streams and actual client mocked out"
@@ -660,7 +660,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         args = ["renew", "--dry-run", "-tvv"]
         self._test_renewal_common(True, [], args=args, renew=True)
 
-    @mock.patch("letsencrypt.cli._set_by_cli")
+    @mock.patch("letsencrypt.cli.set_by_cli")
     def test_ancient_webroot_renewal_conf(self, mock_set_by_cli):
         mock_set_by_cli.return_value = False
         rc_path = self._make_test_renewal_conf('sample-renewal-ancient.conf')
diff --git a/letsencrypt/tests/storage_test.py b/letsencrypt/tests/storage_test.py
index 9660e2688..0d007a831 100644
--- a/letsencrypt/tests/storage_test.py
+++ b/letsencrypt/tests/storage_test.py
@@ -565,6 +565,7 @@ class RenewableCertTests(BaseRenewableCertTest):
     @mock.patch("letsencrypt.cli._parser")
     def test_relevant_values(self, mock_parser):
         """Test that relevant_values() can reject an irrelevant value."""
+        # pylint: disable=protected-access
         from letsencrypt import storage
         mock_parser.verb = "certonly"
         mock_parser.args = ["--standalone"]
@@ -575,6 +576,7 @@ class RenewableCertTests(BaseRenewableCertTest):
     @mock.patch("letsencrypt.cli._parser")
     def test_relevant_values_default(self, mock_parser):
         """Test that relevant_values() can reject a default value."""
+        # pylint: disable=protected-access
         from letsencrypt import storage
         mock_parser.verb = "certonly"
         mock_parser.args = ["--standalone"]
@@ -585,6 +587,7 @@ class RenewableCertTests(BaseRenewableCertTest):
     @mock.patch("letsencrypt.cli._parser")
     def test_relevant_values_nondefault(self, mock_parser):
         """Test that relevant_values() can retain a non-default value."""
+        # pylint: disable=protected-access
         from letsencrypt import storage
         mock_parser.verb = "certonly"
         mock_parser.args = ["--standalone"]

From a9faa3b4ba9b3353e3eb66dda48e303dc881312c Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Wed, 23 Mar 2016 16:14:41 -0700
Subject: [PATCH 25/29] Shim renew() in main.py, keep the work in renewal.py

---
 letsencrypt/cli.py                   |  3 +--
 letsencrypt/main.py                  | 11 ++++++++---
 letsencrypt/{renew.py => renewal.py} |  4 ++--
 letsencrypt/tests/cli_test.py        |  6 +++---
 4 files changed, 14 insertions(+), 10 deletions(-)
 rename letsencrypt/{renew.py => renewal.py} (99%)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 933b51432..bc39c3a8d 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -355,11 +355,10 @@ class HelpfulArgumentParser(object):
 
     def __init__(self, args, plugins, detect_defaults=False):
         from letsencrypt import main
-        from letsencrypt import renew
         self.VERBS = {"auth": main.obtain_cert, "certonly": main.obtain_cert,
                       "config_changes": main.config_changes, "run": main.run,
                       "install": main.install, "plugins": main.plugins_cmd,
-                      "renew": renew.renew, "revoke": main.revoke,
+                      "renew": main.renew, "revoke": main.revoke,
                       "rollback": main.rollback, "everything": main.run}
 
         # List of topics for which additional help can be provided
diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index 90dfb456a..a3ebde64e 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -27,7 +27,7 @@ from letsencrypt import interfaces
 from letsencrypt import le_util
 from letsencrypt import log
 from letsencrypt import reporter
-from letsencrypt import renew
+from letsencrypt import renewal
 from letsencrypt import storage
 
 from letsencrypt.display import util as display_util, ops as display_ops
@@ -186,7 +186,7 @@ def _handle_identical_cert_request(config, cert):
     :rtype: tuple
 
     """
-    if renew.should_renew(config, cert):
+    if renewal.should_renew(config, cert):
         return "renew", cert
     if config.reinstall:
         # Set with --reinstall, force an identical certificate to be
@@ -263,7 +263,7 @@ def _find_duplicative_certs(config, domains):
     # Verify the directory is there
     le_util.make_or_verify_dir(configs_dir, mode=0o755, uid=os.geteuid())
 
-    for renewal_file in renew.renewal_conf_files(cli_config):
+    for renewal_file in renewal.renewal_conf_files(cli_config):
         try:
             candidate_lineage = storage.RenewableCert(renewal_file, cli_config)
         except (errors.CertStorageError, IOError):
@@ -552,6 +552,11 @@ def obtain_cert(config, plugins, lineage=None):
                   config.installer, "server; fullchain is", lineage.fullchain)
     _suggest_donation_if_appropriate(config, action)
 
+def renew(config, unused_plugins):
+    """Renew previously-obtained certificates."""
+    renewal.renew_all_lineages(config)
+
+
 
 def setup_log_file_handler(config, logfile, fmt):
     """Setup file debug logging."""
diff --git a/letsencrypt/renew.py b/letsencrypt/renewal.py
similarity index 99%
rename from letsencrypt/renew.py
rename to letsencrypt/renewal.py
index 9dc1ff4df..27546bec9 100644
--- a/letsencrypt/renew.py
+++ b/letsencrypt/renewal.py
@@ -236,8 +236,8 @@ def _renew_describe_results(config, renew_successes, renew_failures,
         print("**          (The test certificates above have not been saved.)")
 
 
-def renew(config, unused_plugins):
-    """Renew previously-obtained certificates."""
+def renew_all_lineages(config):
+    """Examine each lineage; renew if due and report results"""
 
     if config.domains != []:
         raise errors.Error("Currently, the renew verb is only capable of "
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index 55a504f82..0c2bf5d54 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -23,7 +23,7 @@ from letsencrypt import crypto_util
 from letsencrypt import errors
 from letsencrypt import le_util
 from letsencrypt import main
-from letsencrypt import renew
+from letsencrypt import renewal
 from letsencrypt import storage
 
 from letsencrypt.plugins import disco
@@ -666,7 +666,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
             configuration.RenewerConfiguration(config))
         renewalparams = lineage.configuration["renewalparams"]
         # pylint: disable=protected-access
-        renew._restore_webroot_config(config, renewalparams)
+        renewal._restore_webroot_config(config, renewalparams)
         self.assertEqual(config.webroot_path, ["/var/www/"])
 
     def test_renew_verb_empty_config(self):
@@ -745,7 +745,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
 
     def test_renew_reconstitute_error(self):
         # pylint: disable=protected-access
-        with mock.patch('letsencrypt.main.renew._reconstitute') as mock_reconstitute:
+        with mock.patch('letsencrypt.main.renewal._reconstitute') as mock_reconstitute:
             mock_reconstitute.side_effect = Exception
             self._test_renew_common(assert_oc_called=False, error_expected=True)
 

From f3362d99784eb865c9cef7612566db8a73222fb0 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Wed, 23 Mar 2016 17:41:56 -0700
Subject: [PATCH 26/29] Disabling a protected access complaint

---
 letsencrypt/storage.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 838ec868f..da5e5f665 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -148,7 +148,7 @@ def relevant_values(all_values):
 
     from letsencrypt import cli
 
-    def _is_cli_default(option, value):
+    def _is_cli_default(option, value): # pylint: disable=protected-access
         # Look through the CLI parser defaults and see if this option is
         # both present and equal to the specified value. If not, return
         # False.

From a9e0b0f6ab07e5b5272d16768448e3e390333729 Mon Sep 17 00:00:00 2001
From: Seth Schoen <schoen@eff.org>
Date: Thu, 24 Mar 2016 12:02:11 -0700
Subject: [PATCH 27/29] Another try on protected access lint error

---
 letsencrypt/storage.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index da5e5f665..59daa1a0d 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -148,10 +148,11 @@ def relevant_values(all_values):
 
     from letsencrypt import cli
 
-    def _is_cli_default(option, value): # pylint: disable=protected-access
+    def _is_cli_default(option, value):
         # Look through the CLI parser defaults and see if this option is
         # both present and equal to the specified value. If not, return
         # False.
+        # pylint: disable=protected-access
         for x in cli.helpful_parser.parser._actions:
             if x.dest == option:
                 if x.default == value:

From f3f665bf63f256c66ea1f3b1b03a665be6310d0e Mon Sep 17 00:00:00 2001
From: Brad Warren <bmw@eff.org>
Date: Thu, 24 Mar 2016 12:35:52 -0700
Subject: [PATCH 28/29] s/none/None

---
 letsencrypt/cli.py                                     | 4 ++--
 letsencrypt/main.py                                    | 2 +-
 letsencrypt/tests/testdata/sample-renewal-ancient.conf | 2 +-
 letsencrypt/tests/testdata/sample-renewal.conf         | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index e10a531ab..1af4ef898 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -236,8 +236,8 @@ def choose_configurator_plugins(config, plugins, verb):
 def record_chosen_plugins(config, plugins, auth, inst):
     "Update the config entries to reflect the plugins we actually selected."
     cn = config.namespace
-    cn.authenticator = plugins.find_init(auth).name if auth else "none"
-    cn.installer = plugins.find_init(inst).name if inst else "none"
+    cn.authenticator = plugins.find_init(auth).name if auth else "None"
+    cn.installer = plugins.find_init(inst).name if inst else "None"
 
 
 def set_by_cli(var):
diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index a3ebde64e..fc6540dcf 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -462,7 +462,7 @@ def config_changes(config, unused_plugins):
 def revoke(config, unused_plugins):  # TODO: coop with renewal config
     """Revoke a previously obtained certificate."""
     # For user-agent construction
-    config.namespace.installer = config.namespace.authenticator = "none"
+    config.namespace.installer = config.namespace.authenticator = "None"
     if config.key_path is not None:  # revocation by cert key
         logger.debug("Revoking %s using cert key %s",
                      config.cert_path[0], config.key_path[0])
diff --git a/letsencrypt/tests/testdata/sample-renewal-ancient.conf b/letsencrypt/tests/testdata/sample-renewal-ancient.conf
index ff246ba7c..dd3075b8e 100644
--- a/letsencrypt/tests/testdata/sample-renewal-ancient.conf
+++ b/letsencrypt/tests/testdata/sample-renewal-ancient.conf
@@ -14,7 +14,7 @@ apache_dismod = a2dismod
 register_unsafely_without_email = False
 apache_handle_modules = True
 uir = None
-installer = none
+installer = None
 nginx_ctl = nginx
 config_dir = MAGICDIR
 text_mode = False
diff --git a/letsencrypt/tests/testdata/sample-renewal.conf b/letsencrypt/tests/testdata/sample-renewal.conf
index d6ebbd845..08032af86 100644
--- a/letsencrypt/tests/testdata/sample-renewal.conf
+++ b/letsencrypt/tests/testdata/sample-renewal.conf
@@ -14,7 +14,7 @@ apache_dismod = a2dismod
 register_unsafely_without_email = False
 apache_handle_modules = True
 uir = None
-installer = none
+installer = None
 nginx_ctl = nginx
 config_dir = MAGICDIR
 text_mode = False

From 7b434c5a88aff2d6102680eb2b118e5a99db3620 Mon Sep 17 00:00:00 2001
From: Peter Eckersley <pde@eff.org>
Date: Thu, 24 Mar 2016 17:51:48 -0700
Subject: [PATCH 29/29] Address review comments

---
 letsencrypt/main.py              | 8 ++++----
 letsencrypt/plugins/selection.py | 4 +---
 letsencrypt/tests/cli_test.py    | 4 ++--
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/letsencrypt/main.py b/letsencrypt/main.py
index 15c2de3ee..4e35a5227 100644
--- a/letsencrypt/main.py
+++ b/letsencrypt/main.py
@@ -32,7 +32,7 @@ from letsencrypt import storage
 
 from letsencrypt.display import util as display_util, ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
-from letsencrypt.plugins import selection as ps
+from letsencrypt.plugins import selection as plug_sel
 
 
 logger = logging.getLogger(__name__)
@@ -407,7 +407,7 @@ def install(config, plugins):
     # this function ...
 
     try:
-        installer, _ = ps.choose_configurator_plugins(config, plugins, "install")
+        installer, _ = plug_sel.choose_configurator_plugins(config, plugins, "install")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -482,7 +482,7 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     # TODO: Make run as close to auth + install as possible
     # Possible difficulties: config.csr was hacked into auth
     try:
-        installer, authenticator = ps.choose_configurator_plugins(config, plugins, "run")
+        installer, authenticator = plug_sel.choose_configurator_plugins(config, plugins, "run")
     except errors.PluginSelectionError as e:
         return e.message
 
@@ -512,7 +512,7 @@ def obtain_cert(config, plugins, lineage=None):
     # pylint: disable=too-many-locals
     try:
         # installers are used in auth mode to determine domain names
-        installer, authenticator = ps.choose_configurator_plugins(config, plugins, "certonly")
+        installer, authenticator = plug_sel.choose_configurator_plugins(config, plugins, "certonly")
     except errors.PluginSelectionError as e:
         logger.info("Could not choose appropriate plugin: %s", e)
         raise
diff --git a/letsencrypt/plugins/selection.py b/letsencrypt/plugins/selection.py
index 6f86075cd..057a09768 100644
--- a/letsencrypt/plugins/selection.py
+++ b/letsencrypt/plugins/selection.py
@@ -132,8 +132,6 @@ def choose_plugin(prepared, question):
         else:
             return None
 
-logger = logging.getLogger(__name__)
-
 noninstaller_plugins = ["webroot", "manual", "standalone"]
 
 def record_chosen_plugins(config, plugins, auth, inst):
@@ -146,7 +144,7 @@ def record_chosen_plugins(config, plugins, auth, inst):
 def choose_configurator_plugins(config, plugins, verb):
     """
     Figure out which configurator we're going to use, modifies
-    config.authenticator and config.istaller strings to reflect that choice if
+    config.authenticator and config.installer strings to reflect that choice if
     necessary.
 
     :raises errors.PluginSelectionError if there was a problem
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index b69716924..04b5a2f3c 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -203,8 +203,8 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertEqual(args.chain_path, os.path.abspath(chain))
         self.assertEqual(args.fullchain_path, os.path.abspath(fullchain))
 
-    @mock.patch('letsencrypt.main.ps.record_chosen_plugins')
-    @mock.patch('letsencrypt.main.ps.pick_installer')
+    @mock.patch('letsencrypt.main.plug_sel.record_chosen_plugins')
+    @mock.patch('letsencrypt.main.plug_sel.pick_installer')
     def test_installer_selection(self, mock_pick_installer, _rec):
         self._call(['install', '--domains', 'foo.bar', '--cert-path', 'cert',
                     '--key-path', 'key', '--chain-path', 'chain'])