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 01/11] 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 02/11] 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 03/11] 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 04/11] 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 05/11] 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 06/11] 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 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 07/11] 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 08/11] 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 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 09/11] 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 10/11] 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 11/11] 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