From 3ee4ee0996bf55e09913d6ca36cb66b2fd868528 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Mon, 12 Oct 2015 18:07:21 -0700
Subject: [PATCH 01/11] Added lockfiles around link update

---
 letsencrypt/storage.py | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 8a0f4829e..354ea6b7c 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -378,9 +378,20 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         :param int version: the desired version"""
 
+        lockfiles = []
+        for kind in ALL_FOUR:
+            lockfile = os.path.join(os.path.dirname(getattr(self, kind)),
+                                    "{0}.lock".format(kind))
+            with open(lockfile, "w") as f:
+                f.write("{0}\n".format(self.current_target(kind)))
+            lockfiles.append(lockfile)
+
         for kind in ALL_FOUR:
             self._update_link_to(kind, version)
 
+        for lockfile in lockfiles:
+            os.unlink(lockfile)
+
     def _notafterbefore(self, method, version):
         """Internal helper function for finding notbefore/notafter."""
         if version is None:

From 15cf1c1a4e226146e560197e0ba85d368de6fbab Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Mon, 12 Oct 2015 18:23:12 -0700
Subject: [PATCH 02/11] Added _lockfiles

---
 letsencrypt/storage.py | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 354ea6b7c..69ff272b3 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -205,6 +205,21 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     #       happen as a result of random tampering by a sysadmin, or
     #       filesystem errors, or crashes.)
 
+    def _lockfiles(self):
+        """Returns the path of all lockfiles used by this lineage.
+
+        :returns: the path of all lockfiles used by this lineage
+        :rtype: list
+
+        """
+        lockfiles = []
+        for kind in ALL_FOUR:
+            lock_dir = os.path.dirname(getattr(self, kind))
+            lock_base = "{0}.lock".format(kind)
+            lockfiles.append(os.path.join(lock_dir, lock_base))
+
+        return lockfiles
+
     def current_target(self, kind):
         """Returns full path to which the specified item currently points.
 
@@ -376,15 +391,14 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     def update_all_links_to(self, version):
         """Change all member objects to point to the specified version.
 
-        :param int version: the desired version"""
+        :param int version: the desired version
 
-        lockfiles = []
-        for kind in ALL_FOUR:
-            lockfile = os.path.join(os.path.dirname(getattr(self, kind)),
-                                    "{0}.lock".format(kind))
+        """
+        lockfiles = self._lockfiles()
+        for lockfile in lockfiles:
+            kind = os.path.splitext(os.path.basename(lockfile))[0]
             with open(lockfile, "w") as f:
                 f.write("{0}\n".format(self.current_target(kind)))
-            lockfiles.append(lockfile)
 
         for kind in ALL_FOUR:
             self._update_link_to(kind, version)

From 49ba0c05e745e2d07fd12f243c11c19112299f12 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Mon, 12 Oct 2015 18:35:07 -0700
Subject: [PATCH 03/11] Added recovery function

---
 letsencrypt/storage.py | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 69ff272b3..4047143a9 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -206,9 +206,9 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     #       filesystem errors, or crashes.)
 
     def _lockfiles(self):
-        """Returns the path of all lockfiles used by this lineage.
+        """Returns the kind and path of all used lockfiles.
 
-        :returns: the path of all lockfiles used by this lineage
+        :returns: list of (kind, lockfile) tuples
         :rtype: list
 
         """
@@ -216,10 +216,30 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         for kind in ALL_FOUR:
             lock_dir = os.path.dirname(getattr(self, kind))
             lock_base = "{0}.lock".format(kind)
-            lockfiles.append(os.path.join(lock_dir, lock_base))
+            lockfiles.append((kind, os.path.join(lock_dir, lock_base)))
 
         return lockfiles
 
+    def _fix_symlinks(self):
+        """Fixes symlinks in the event of an incomplete version update.
+
+        If there is no problem with the current symlinks, this function
+        has no effect.
+
+        """
+        lockfiles = self._lockfiles()
+        if all(os.path.exists(lockfile) for lockfile in lockfiles):
+            for kind, lockfile in lockfiles:
+                link = getattr(self, kind)
+                if os.path.lexists(link):
+                    os.unlink(link)
+                with open(lockfile) as f:
+                    os.symlink(f.readline().rstrip(), link)
+        else:
+            for _, lockfile in lockfiles:
+                if os.path.exists(lockfile):
+                    os.unlink(lockfile)
+
     def current_target(self, kind):
         """Returns full path to which the specified item currently points.
 
@@ -395,15 +415,14 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         """
         lockfiles = self._lockfiles()
-        for lockfile in lockfiles:
-            kind = os.path.splitext(os.path.basename(lockfile))[0]
+        for kind, lockfile in lockfiles:
             with open(lockfile, "w") as f:
                 f.write("{0}\n".format(self.current_target(kind)))
 
         for kind in ALL_FOUR:
             self._update_link_to(kind, version)
 
-        for lockfile in lockfiles:
+        for _, lockfile in lockfiles:
             os.unlink(lockfile)
 
     def _notafterbefore(self, method, version):

From f8f80f5392badbb71cb5e1fe8c6b0da6f64833ed Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Mon, 12 Oct 2015 18:39:17 -0700
Subject: [PATCH 04/11] Added calls to recovery code

---
 letsencrypt/storage.py | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 4047143a9..7d8801f10 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -13,6 +13,7 @@ import pyrfc3339
 from letsencrypt import constants
 from letsencrypt import crypto_util
 from letsencrypt import errors
+from letsencrypt import error_handler
 from letsencrypt import le_util
 
 ALL_FOUR = ("cert", "privkey", "chain", "fullchain")
@@ -129,6 +130,8 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         self.chain = self.configuration["chain"]
         self.fullchain = self.configuration["fullchain"]
 
+        self._fix_symlinks()
+
     def _consistent(self):
         """Are the files associated with this lineage self-consistent?
 
@@ -228,7 +231,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         """
         lockfiles = self._lockfiles()
-        if all(os.path.exists(lockfile) for lockfile in lockfiles):
+        if all(os.path.exists(lockfile[1]) for lockfile in lockfiles):
             for kind, lockfile in lockfiles:
                 link = getattr(self, kind)
                 if os.path.lexists(link):
@@ -414,16 +417,17 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         :param int version: the desired version
 
         """
-        lockfiles = self._lockfiles()
-        for kind, lockfile in lockfiles:
-            with open(lockfile, "w") as f:
-                f.write("{0}\n".format(self.current_target(kind)))
+        with error_handler.ErrorHandler(self._fix_symlinks):
+            lockfiles = self._lockfiles()
+            for kind, lockfile in lockfiles:
+                with open(lockfile, "w") as f:
+                    f.write("{0}\n".format(self.current_target(kind)))
 
-        for kind in ALL_FOUR:
-            self._update_link_to(kind, version)
+            for kind in ALL_FOUR:
+                self._update_link_to(kind, version)
 
-        for _, lockfile in lockfiles:
-            os.unlink(lockfile)
+            for _, lockfile in lockfiles:
+                os.unlink(lockfile)
 
     def _notafterbefore(self, method, version):
         """Internal helper function for finding notbefore/notafter."""

From b90827dc98d5ebe4f7104e1580d295563d33e409 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Mon, 12 Oct 2015 20:40:17 -0700
Subject: [PATCH 05/11] Added tests

---
 letsencrypt/storage.py            |  8 ++++----
 letsencrypt/tests/renewer_test.py | 34 ++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 7d8801f10..76b54fab3 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -208,8 +208,8 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     #       happen as a result of random tampering by a sysadmin, or
     #       filesystem errors, or crashes.)
 
-    def _lockfiles(self):
-        """Returns the kind and path of all used lockfiles.
+    def _symlink_lockfiles(self):
+        """Returns the kind and path of all symlink lockfiles.
 
         :returns: list of (kind, lockfile) tuples
         :rtype: list
@@ -230,7 +230,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         has no effect.
 
         """
-        lockfiles = self._lockfiles()
+        lockfiles = self._symlink_lockfiles()
         if all(os.path.exists(lockfile[1]) for lockfile in lockfiles):
             for kind, lockfile in lockfiles:
                 link = getattr(self, kind)
@@ -418,7 +418,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         """
         with error_handler.ErrorHandler(self._fix_symlinks):
-            lockfiles = self._lockfiles()
+            lockfiles = self._symlink_lockfiles()
             for kind, lockfile in lockfiles:
                 with open(lockfile, "w") as f:
                     f.write("{0}\n".format(self.current_target(kind)))
diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py
index bc8afbcb5..e8ad168e7 100644
--- a/letsencrypt/tests/renewer_test.py
+++ b/letsencrypt/tests/renewer_test.py
@@ -290,7 +290,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertEqual("cert8.pem",
                          os.path.basename(self.test_rc.version("cert", 8)))
 
-    def test_update_all_links_to(self):
+    def test_update_all_links_to_success(self):
         for ver in xrange(1, 6):
             for kind in ALL_FOUR:
                 where = getattr(self.test_rc, kind)
@@ -308,6 +308,38 @@ class RenewableCertTests(BaseRenewableCertTest):
                 self.assertEqual(ver, self.test_rc.current_version(kind))
             self.assertEqual(self.test_rc.latest_common_version(), 5)
 
+    def test_update_all_links_to_partial_failure(self):
+        def unlink_or_raise(path, real_unlink=os.unlink):
+            # pylint: disable=missing-docstring
+            if "fullchain" in path and not path.endswith(".pem"):
+                raise ValueError
+            else:
+                real_unlink(path)
+
+        self._write_out_ex_kinds()
+        with mock.patch("letsencrypt.storage.os.unlink") as mock_unlink:
+            mock_unlink.side_effect = unlink_or_raise
+            self.assertRaises(ValueError, self.test_rc.update_all_links_to, 12)
+
+        for kind in ALL_FOUR:
+            self.assertEqual(self.test_rc.current_version(kind), 12)
+
+    def test_update_all_links_to_full_failure(self):
+        def unlink_or_raise(path, real_unlink=os.unlink):
+            # pylint: disable=missing-docstring
+            if "fullchain" in path:
+                raise ValueError
+            else:
+                real_unlink(path)
+
+        self._write_out_ex_kinds()
+        with mock.patch("letsencrypt.storage.os.unlink") as mock_unlink:
+            mock_unlink.side_effect = unlink_or_raise
+            self.assertRaises(ValueError, self.test_rc.update_all_links_to, 12)
+
+        for kind in ALL_FOUR:
+            self.assertEqual(self.test_rc.current_version(kind), 11)
+
     def test_has_pending_deployment(self):
         for ver in xrange(1, 6):
             for kind in ALL_FOUR:

From acc44f2b659850f6907ea4f7c6ba1a6671eee672 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Mon, 12 Oct 2015 21:04:40 -0700
Subject: [PATCH 06/11] Always delete lockfiles in _fix_symlinks

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

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 76b54fab3..207099e74 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -238,10 +238,10 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
                     os.unlink(link)
                 with open(lockfile) as f:
                     os.symlink(f.readline().rstrip(), link)
-        else:
-            for _, lockfile in lockfiles:
-                if os.path.exists(lockfile):
-                    os.unlink(lockfile)
+
+        for _, lockfile in lockfiles:
+            if os.path.exists(lockfile):
+                os.unlink(lockfile)
 
     def current_target(self, kind):
         """Returns full path to which the specified item currently points.

From 0e6d652ce37514b721fba5ce929caeb792930b85 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Tue, 13 Oct 2015 18:25:31 -0700
Subject: [PATCH 07/11] Move ConfigObj parsing inside of RenewableCert.__init__

---
 letsencrypt/cli.py                | 17 ++++-------
 letsencrypt/renewer.py            | 30 ++++++-------------
 letsencrypt/storage.py            | 48 ++++++++++++++++---------------
 letsencrypt/tests/cli_test.py     | 17 ++++++-----
 letsencrypt/tests/renewer_test.py | 40 +++++++++-----------------
 5 files changed, 62 insertions(+), 90 deletions(-)

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 07ccd38fd..35113be3a 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -12,7 +12,6 @@ import time
 import traceback
 
 import configargparse
-import configobj
 import OpenSSL
 import zope.component
 import zope.interface.exceptions
@@ -167,25 +166,21 @@ def _init_le_client(args, config, authenticator, installer):
     return client.Client(config, acc, authenticator, installer, acme=acme)
 
 
-def _find_duplicative_certs(domains, config, renew_config):
+def _find_duplicative_certs(config, domains):
     """Find existing certs that duplicate the request."""
 
     identical_names_cert, subset_names_cert = None, None
 
-    configs_dir = renew_config.renewal_configs_dir
+    cli_config = configuration.RenewerConfiguration(config)
+    configs_dir = cli_config.renewal_configs_dir
     # Verify the directory is there
     le_util.make_or_verify_dir(configs_dir, mode=0o755, uid=os.geteuid())
 
-    cli_config = configuration.RenewerConfiguration(config)
     for renewal_file in os.listdir(configs_dir):
         try:
             full_path = os.path.join(configs_dir, renewal_file)
-            rc_config = configobj.ConfigObj(renew_config.renewer_config_file)
-            rc_config.merge(configobj.ConfigObj(full_path))
-            rc_config.filename = full_path
-            candidate_lineage = storage.RenewableCert(
-                rc_config, config_opts=None, cli_config=cli_config)
-        except (configobj.ConfigObjError, errors.CertStorageError, IOError):
+            candidate_lineage = storage.RenewableCert(full_path, cli_config)
+        except (errors.CertStorageError, IOError):
             logger.warning("Renewal configuration file %s is broken. "
                            "Skipping.", full_path)
             continue
@@ -217,7 +212,7 @@ def _treat_as_renewal(config, domains):
     # kind of certificate to be obtained with renewal = False.)
     if not config.duplicate:
         ident_names_cert, subset_names_cert = _find_duplicative_certs(
-            domains, config, configuration.RenewerConfiguration(config))
+            config, domains)
         # I am not sure whether that correctly reads the systemwide
         # configuration file.
         question = None
diff --git a/letsencrypt/renewer.py b/letsencrypt/renewer.py
index 8d8540e6f..f91d60833 100644
--- a/letsencrypt/renewer.py
+++ b/letsencrypt/renewer.py
@@ -12,7 +12,6 @@ import logging
 import os
 import sys
 
-import configobj
 import OpenSSL
 import zope.component
 
@@ -141,7 +140,7 @@ def _create_parser():
     return _paths_parser(parser)
 
 
-def main(config=None, cli_args=sys.argv[1:]):
+def main(cli_args=sys.argv[1:]):
     """Main function for autorenewer script."""
     # TODO: Distinguish automated invocation from manual invocation,
     #       perhaps by looking at sys.argv[0] and inhibiting automated
@@ -149,6 +148,11 @@ def main(config=None, cli_args=sys.argv[1:]):
     #       turned it off. (The boolean parameter should probably be
     #       called renewer_enabled.)
 
+    # TODO: When we have a more elaborate renewer command line, we will
+    #       presumably also be able to specify a config file on the
+    #       command line, which, if provided, should take precedence over
+    #       te default config files
+
     zope.component.provideUtility(display_util.FileDisplay(sys.stdout))
 
     args = _create_parser().parse_args(cli_args)
@@ -159,28 +163,12 @@ def main(config=None, cli_args=sys.argv[1:]):
 
     cli_config = configuration.RenewerConfiguration(args)
 
-    config = storage.config_with_defaults(config)
-    # Now attempt to read the renewer config file and augment or replace
-    # the renewer defaults with any options contained in that file.  If
-    # renewer_config_file is undefined or if the file is nonexistent or
-    # empty, this .merge() will have no effect.  TODO: when we have a more
-    # elaborate renewer command line, we will presumably also be able to
-    # specify a config file on the command line, which, if provided, should
-    # take precedence over this one.
-    config.merge(configobj.ConfigObj(cli_config.renewer_config_file))
     # Ensure that all of the needed folders have been created before continuing
     le_util.make_or_verify_dir(cli_config.work_dir,
                                constants.CONFIG_DIRS_MODE, uid)
 
-    for i in os.listdir(cli_config.renewal_configs_dir):
-        print "Processing", i
-        if not i.endswith(".conf"):
-            continue
-        rc_config = configobj.ConfigObj(cli_config.renewer_config_file)
-        rc_config.merge(configobj.ConfigObj(
-            os.path.join(cli_config.renewal_configs_dir, i)))
-        # TODO: this is a dirty hack!
-        rc_config.filename = os.path.join(cli_config.renewal_configs_dir, i)
+    for renewal_file in os.listdir(cli_config.renewal_configs_dir):
+        print "Processing", renewal_file
         try:
             # TODO: Before trying to initialize the RenewableCert object,
             #       we could check here whether the combination of the config
@@ -190,7 +178,7 @@ def main(config=None, cli_args=sys.argv[1:]):
             #       RenewableCert object for this cert at all, which could
             #       dramatically improve performance for large deployments
             #       where autorenewal is widely turned off.
-            cert = storage.RenewableCert(rc_config, cli_config=cli_config)
+            cert = storage.RenewableCert(renewal_file, cli_config)
         except errors.CertStorageError:
             # This indicates an invalid renewal configuration file, such
             # as one missing a required parameter (in the future, perhaps
diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 207099e74..9b3290e2c 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -81,49 +81,49 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         renewal configuration file and/or systemwide defaults.
 
     """
-    def __init__(self, configfile, config_opts=None, cli_config=None):
+    def __init__(self, config_filename, cli_config):
         """Instantiate a RenewableCert object from an existing lineage.
 
-        :param configobj.ConfigObj configfile: an already-parsed
-            ConfigObj object made from reading the renewal config file
+        :param str config_filename: the path to the renewal config file
             that defines this lineage.
-
-        :param configobj.ConfigObj config_opts: systemwide defaults for
-            renewal properties not otherwise specified in the individual
-            renewal config file.
-        :param .RenewerConfiguration cli_config:
+        :param .RenewerConfiguration: parsed command line arguments
 
         :raises .CertStorageError: if the configuration file's name didn't end
             in ".conf", or the file is missing or broken.
-        :raises TypeError: if the provided renewal configuration isn't a
-            ConfigObj object.
 
         """
         self.cli_config = cli_config
-        if isinstance(configfile, configobj.ConfigObj):
-            if not os.path.basename(configfile.filename).endswith(".conf"):
-                raise errors.CertStorageError(
-                    "renewal config file name must end in .conf")
-            self.lineagename = os.path.basename(
-                configfile.filename)[:-len(".conf")]
-        else:
-            raise TypeError("RenewableCert config must be ConfigObj object")
+        if not config_filename.endswith(".conf"):
+            raise errors.CertStorageError(
+                "renewal config file name must end in .conf")
+        self.lineagename = os.path.basename(
+            config_filename[:-len(".conf")])
 
         # self.configuration should be used to read parameters that
         # may have been chosen based on default values from the
         # systemwide renewal configuration; self.configfile should be
         # used to make and save changes.
-        self.configfile = configfile
+        try:
+            self.configfile = configobj.ConfigObj(config_filename)
+        except configobj.ConfigObjError:
+            raise errors.CertStorageError(
+                "error parsing {0}".format(config_filename))
+
         # TODO: Do we actually use anything from defaults and do we want to
         #       read further defaults from the systemwide renewal configuration
         #       file at this stage?
-        self.configuration = config_with_defaults(config_opts)
-        self.configuration.merge(self.configfile)
+        try:
+            self.configuration = config_with_defaults(
+                configobj.ConfigObj(cli_config.renewer_config_file))
+            self.configuration.merge(self.configfile)
+        except:
+            raise errors.CertStorageError(
+                "error parsing {0}".format(cli_config.renewer_config_file))
 
         if not all(x in self.configuration for x in ALL_FOUR):
             raise errors.CertStorageError(
                 "renewal config file {0} is missing a required "
-                "file reference".format(configfile))
+                "file reference".format(self.configfile))
 
         self.cert = self.configuration["cert"]
         self.privkey = self.configuration["privkey"]
@@ -622,6 +622,8 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         :param configobj.ConfigObj config: renewal configuration
             defaults, affecting, for example, the locations of the
             directories where the associated files will be saved
+        :param .RenewerConfiguration cli_config: parsed command line
+            arguments
 
         :returns: the newly-created RenewalCert object
         :rtype: :class:`storage.renewableCert`"""
@@ -691,7 +693,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         # TODO: add human-readable comments explaining other available
         #       parameters
         new_config.write()
-        return cls(new_config, config, cli_config)
+        return cls(new_config.filename, cli_config)
 
     def save_successor(self, prior_version, new_cert, new_privkey, new_chain):
         """Save new cert and chain as a successor of a prior version.
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index d0fae370d..bc65d3599 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -302,26 +302,25 @@ class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest):
             f.write(test_cert)
 
         # No overlap at all
-        result = _find_duplicative_certs(['wow.net', 'hooray.org'],
-                                         self.config, self.cli_config)
+        result = _find_duplicative_certs(
+            self.cli_config, ['wow.net', 'hooray.org'])
         self.assertEqual(result, (None, None))
 
         # Totally identical
-        result = _find_duplicative_certs(['example.com', 'www.example.com'],
-                                         self.config, self.cli_config)
+        result = _find_duplicative_certs(
+            self.cli_config, ['example.com', 'www.example.com'])
         self.assertTrue(result[0].configfile.filename.endswith('example.org.conf'))
         self.assertEqual(result[1], None)
 
         # Superset
-        result = _find_duplicative_certs(['example.com', 'www.example.com',
-                                          'something.new'], self.config,
-                                         self.cli_config)
+        result = _find_duplicative_certs(
+            self.cli_config, ['example.com', 'www.example.com', 'something.new'])
         self.assertEqual(result[0], None)
         self.assertTrue(result[1].configfile.filename.endswith('example.org.conf'))
 
         # Partial overlap doesn't count
-        result = _find_duplicative_certs(['example.com', 'something.new'],
-                                         self.config, self.cli_config)
+        result = _find_duplicative_certs(
+            self.cli_config, ['example.com', 'something.new'])
         self.assertEqual(result, (None, None))
 
 
diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py
index e8ad168e7..652c54577 100644
--- a/letsencrypt/tests/renewer_test.py
+++ b/letsencrypt/tests/renewer_test.py
@@ -63,11 +63,11 @@ class BaseRenewableCertTest(unittest.TestCase):
                                         kind + ".pem")
         config.filename = os.path.join(self.tempdir, "renewal",
                                        "example.org.conf")
+        config.write()
         self.config = config
 
         self.defaults = configobj.ConfigObj()
-        self.test_rc = storage.RenewableCert(
-            self.config, self.defaults, self.cli_config)
+        self.test_rc = storage.RenewableCert(config.filename, self.cli_config)
 
     def tearDown(self):
         shutil.rmtree(self.tempdir)
@@ -99,34 +99,26 @@ class RenewableCertTests(BaseRenewableCertTest):
 
     def test_renewal_bad_config(self):
         """Test that the RenewableCert constructor will complain if
-        the renewal configuration file doesn't end in ".conf" or if it
-        isn't a ConfigObj."""
+        the renewal configuration file doesn't end in ".conf"
+
+        """
         from letsencrypt import storage
-        defaults = configobj.ConfigObj()
-        config = configobj.ConfigObj()
-        # These files don't exist and aren't created here; the point of the test
-        # is to confirm that the constructor rejects them outright because of
-        # the configfile's name.
-        for kind in ALL_FOUR:
-            config["cert"] = "nonexistent_" + kind + ".pem"
-        config.filename = "nonexistent_sillyfile"
-        self.assertRaises(
-            errors.CertStorageError, storage.RenewableCert, config, defaults)
-        self.assertRaises(TypeError, storage.RenewableCert, "fun", defaults)
+        self.assertRaises(errors.CertStorageError, storage.RenewableCert,
+                          "fun", self.cli_config)
 
     def test_renewal_incomplete_config(self):
         """Test that the RenewableCert constructor will complain if
         the renewal configuration file is missing a required file element."""
         from letsencrypt import storage
-        defaults = configobj.ConfigObj()
         config = configobj.ConfigObj()
         config["cert"] = "imaginary_cert.pem"
         # Here the required privkey is missing.
         config["chain"] = "imaginary_chain.pem"
         config["fullchain"] = "imaginary_fullchain.pem"
-        config.filename = "imaginary_config.conf"
-        self.assertRaises(
-            errors.CertStorageError, storage.RenewableCert, config, defaults)
+        config.filename = os.path.join(self.tempdir, "imaginary_config.conf")
+        config.write()
+        self.assertRaises(errors.CertStorageError, storage.RenewableCert,
+                          config.filename, self.cli_config)
 
     def test_consistent(self):
         # pylint: disable=too-many-statements,protected-access
@@ -719,10 +711,6 @@ class RenewableCertTests(BaseRenewableCertTest):
         mock_rc_instance.should_autorenew.return_value = True
         mock_rc_instance.latest_common_version.return_value = 10
         mock_rc.return_value = mock_rc_instance
-        with open(os.path.join(self.cli_config.renewal_configs_dir,
-                               "README"), "w") as f:
-            f.write("This is a README file to make sure that the renewer is")
-            f.write("able to correctly ignore files that don't end in .conf.")
         with open(os.path.join(self.cli_config.renewal_configs_dir,
                                "example.org.conf"), "w") as f:
             # This isn't actually parsed in this test; we have a separate
@@ -734,7 +722,7 @@ class RenewableCertTests(BaseRenewableCertTest):
                                "example.com.conf"), "w") as f:
             f.write("cert = cert.pem\nprivkey = privkey.pem\n")
             f.write("chain = chain.pem\nfullchain = fullchain.pem\n")
-        renewer.main(self.defaults, cli_args=self._common_cli_args())
+        renewer.main(cli_args=self._common_cli_args())
         self.assertEqual(mock_rc.call_count, 2)
         self.assertEqual(mock_rc_instance.update_all_links_to.call_count, 2)
         self.assertEqual(mock_notify.notify.call_count, 4)
@@ -747,7 +735,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         mock_happy_instance.should_autorenew.return_value = False
         mock_happy_instance.latest_common_version.return_value = 10
         mock_rc.return_value = mock_happy_instance
-        renewer.main(self.defaults, cli_args=self._common_cli_args())
+        renewer.main(cli_args=self._common_cli_args())
         self.assertEqual(mock_rc.call_count, 4)
         self.assertEqual(mock_happy_instance.update_all_links_to.call_count, 0)
         self.assertEqual(mock_notify.notify.call_count, 4)
@@ -758,7 +746,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         with open(os.path.join(self.cli_config.renewal_configs_dir,
                                "bad.conf"), "w") as f:
             f.write("incomplete = configfile\n")
-        renewer.main(self.defaults, cli_args=self._common_cli_args())
+        renewer.main(cli_args=self._common_cli_args())
         # The errors.CertStorageError is caught inside and nothing happens.
 
 

From 2239ae9a68d8b8e0b6e4400fe7bb80d3d33e6c54 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Tue, 13 Oct 2015 18:39:23 -0700
Subject: [PATCH 08/11] Don't read global renewer config file

---
 letsencrypt/storage.py | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 9b3290e2c..530cf9d0f 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -108,17 +108,10 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         except configobj.ConfigObjError:
             raise errors.CertStorageError(
                 "error parsing {0}".format(config_filename))
-
         # TODO: Do we actually use anything from defaults and do we want to
         #       read further defaults from the systemwide renewal configuration
         #       file at this stage?
-        try:
-            self.configuration = config_with_defaults(
-                configobj.ConfigObj(cli_config.renewer_config_file))
-            self.configuration.merge(self.configfile)
-        except:
-            raise errors.CertStorageError(
-                "error parsing {0}".format(cli_config.renewer_config_file))
+        self.configuration = config_with_defaults(self.configfile)
 
         if not all(x in self.configuration for x in ALL_FOUR):
             raise errors.CertStorageError(

From 00e5515fe63bc98f6cc7721863a035eae4b2c642 Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Tue, 13 Oct 2015 19:04:25 -0700
Subject: [PATCH 09/11] Added broken config test

---
 letsencrypt/tests/renewer_test.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py
index 652c54577..65ca68d36 100644
--- a/letsencrypt/tests/renewer_test.py
+++ b/letsencrypt/tests/renewer_test.py
@@ -103,6 +103,12 @@ class RenewableCertTests(BaseRenewableCertTest):
 
         """
         from letsencrypt import storage
+        broken = os.path.join(self.tempdir, "broken.conf")
+        with open(broken, "w") as f:
+            f.write("[No closing bracket for you!")
+        self.assertRaises(errors.CertStorageError, storage.RenewableCert,
+                          broken, self.cli_config)
+        os.unlink(broken)
         self.assertRaises(errors.CertStorageError, storage.RenewableCert,
                           "fun", self.cli_config)
 

From 83a7d4d7922efb9ad8211a98985b16fcaaa2561f Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Sat, 17 Oct 2015 17:26:52 -0700
Subject: [PATCH 10/11] changes += schoen's feedback

---
 letsencrypt/storage.py            | 48 +++++++++++++++----------------
 letsencrypt/tests/renewer_test.py |  2 +-
 2 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py
index 530cf9d0f..110b20dd2 100644
--- a/letsencrypt/storage.py
+++ b/letsencrypt/storage.py
@@ -201,20 +201,20 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
     #       happen as a result of random tampering by a sysadmin, or
     #       filesystem errors, or crashes.)
 
-    def _symlink_lockfiles(self):
-        """Returns the kind and path of all symlink lockfiles.
+    def _previous_symlinks(self):
+        """Returns the kind and path of all symlinks used in recovery.
 
-        :returns: list of (kind, lockfile) tuples
+        :returns: list of (kind, symlink) tuples
         :rtype: list
 
         """
-        lockfiles = []
+        previous_symlinks = []
         for kind in ALL_FOUR:
-            lock_dir = os.path.dirname(getattr(self, kind))
-            lock_base = "{0}.lock".format(kind)
-            lockfiles.append((kind, os.path.join(lock_dir, lock_base)))
+            link_dir = os.path.dirname(getattr(self, kind))
+            link_base = "previous_{0}.pem".format(kind)
+            previous_symlinks.append((kind, os.path.join(link_dir, link_base)))
 
-        return lockfiles
+        return previous_symlinks
 
     def _fix_symlinks(self):
         """Fixes symlinks in the event of an incomplete version update.
@@ -223,18 +223,17 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         has no effect.
 
         """
-        lockfiles = self._symlink_lockfiles()
-        if all(os.path.exists(lockfile[1]) for lockfile in lockfiles):
-            for kind, lockfile in lockfiles:
-                link = getattr(self, kind)
-                if os.path.lexists(link):
-                    os.unlink(link)
-                with open(lockfile) as f:
-                    os.symlink(f.readline().rstrip(), link)
+        previous_symlinks = self._previous_symlinks()
+        if all(os.path.exists(link[1]) for link in previous_symlinks):
+            for kind, previous_link in previous_symlinks:
+                current_link = getattr(self, kind)
+                if os.path.lexists(current_link):
+                    os.unlink(current_link)
+                os.symlink(os.readlink(previous_link), current_link)
 
-        for _, lockfile in lockfiles:
-            if os.path.exists(lockfile):
-                os.unlink(lockfile)
+        for _, link in previous_symlinks:
+            if os.path.exists(link):
+                os.unlink(link)
 
     def current_target(self, kind):
         """Returns full path to which the specified item currently points.
@@ -411,16 +410,15 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
 
         """
         with error_handler.ErrorHandler(self._fix_symlinks):
-            lockfiles = self._symlink_lockfiles()
-            for kind, lockfile in lockfiles:
-                with open(lockfile, "w") as f:
-                    f.write("{0}\n".format(self.current_target(kind)))
+            previous_links = self._previous_symlinks()
+            for kind, link in previous_links:
+                os.symlink(self.current_target(kind), link)
 
             for kind in ALL_FOUR:
                 self._update_link_to(kind, version)
 
-            for _, lockfile in lockfiles:
-                os.unlink(lockfile)
+            for _, link in previous_links:
+                os.unlink(link)
 
     def _notafterbefore(self, method, version):
         """Internal helper function for finding notbefore/notafter."""
diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py
index 65ca68d36..91eb36ac8 100644
--- a/letsencrypt/tests/renewer_test.py
+++ b/letsencrypt/tests/renewer_test.py
@@ -309,7 +309,7 @@ class RenewableCertTests(BaseRenewableCertTest):
     def test_update_all_links_to_partial_failure(self):
         def unlink_or_raise(path, real_unlink=os.unlink):
             # pylint: disable=missing-docstring
-            if "fullchain" in path and not path.endswith(".pem"):
+            if "fullchain" in path and "prev" in path:
                 raise ValueError
             else:
                 real_unlink(path)

From ea356b403c0bab88937b31caf4358ab7634399af Mon Sep 17 00:00:00 2001
From: Brad Warren <bradmw@umich.edu>
Date: Thu, 22 Oct 2015 13:06:33 -0700
Subject: [PATCH 11/11] Removed assumptions about temp dir name

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

diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py
index 28ae74b34..a856fcf4f 100644
--- a/letsencrypt/tests/renewer_test.py
+++ b/letsencrypt/tests/renewer_test.py
@@ -308,7 +308,8 @@ class RenewableCertTests(BaseRenewableCertTest):
     def test_update_all_links_to_partial_failure(self):
         def unlink_or_raise(path, real_unlink=os.unlink):
             # pylint: disable=missing-docstring
-            if "fullchain" in path and "prev" in path:
+            basename = os.path.basename(path)
+            if "fullchain" in basename and basename.startswith("prev"):
                 raise ValueError
             else:
                 real_unlink(path)
@@ -324,7 +325,7 @@ class RenewableCertTests(BaseRenewableCertTest):
     def test_update_all_links_to_full_failure(self):
         def unlink_or_raise(path, real_unlink=os.unlink):
             # pylint: disable=missing-docstring
-            if "fullchain" in path:
+            if "fullchain" in os.path.basename(path):
                 raise ValueError
             else:
                 real_unlink(path)