Merge pull request #970 from letsencrypt/storage_paranoia

Storage paranoia

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)
+            candidate_lineage = storage.RenewableCert(full_path, cli_config)
-            rc_config.merge(configobj.ConfigObj(full_path))
+        except (CertStorageError, IOError):
-            rc_config.filename = full_path
-            candidate_lineage = storage.RenewableCert(
-                rc_config, config_opts=None, cli_config=cli_config)
-        except (configobj.ConfigObjError, 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

letsencrypt/renewer.py

@@ -12,7 +12,6 @@ import logging
 import os
 import sys
 
-import configobj
 import OpenSSL
 import zope.component
 
@@ -142,7 +141,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
@@ -150,6 +149,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)
@@ -160,27 +164,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):
+    for renewal_file in os.listdir(cli_config.renewal_configs_dir):
-        print "Processing", i
+        print "Processing", renewal_file
-        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)))
-        rc_config.filename = os.path.join(cli_config.renewal_configs_dir, i)
         try:
             # TODO: Before trying to initialize the RenewableCert object,
             #       we could check here whether the combination of the config
@@ -190,7 +179,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

letsencrypt/storage.py

@@ -11,6 +11,7 @@ import pytz
 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")
@@ -78,55 +79,50 @@ 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
+        :param str config_filename: the path to the renewal config file
-            ConfigObj object made from reading the renewal config file
             that defines this lineage.
-
+        :param .RenewerConfiguration: parsed command line arguments
-        :param configobj.ConfigObj config_opts: systemwide defaults for
-            renewal properties not otherwise specified in the individual
-            renewal config file.
-        :param .RenewerConfiguration cli_config:
 
         :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 config_filename.endswith(".conf"):
-            if not os.path.basename(configfile.filename).endswith(".conf"):
+            raise errors.CertStorageError(
-                raise errors.CertStorageError(
+                "renewal config file name must end in .conf")
-                    "renewal config file name must end in .conf")
+        self.lineagename = os.path.basename(
-            self.lineagename = os.path.basename(
+            config_filename[:-len(".conf")])
-                configfile.filename)[:-len(".conf")]
-        else:
-            raise TypeError("RenewableCert config must be ConfigObj object")
 
         # 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 = config_with_defaults(self.configfile)
-        self.configuration.merge(self.configfile)
 
         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"]
         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?
 
@@ -203,6 +199,40 @@ 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 _previous_symlinks(self):
+        """Returns the kind and path of all symlinks used in recovery.
+
+        :returns: list of (kind, symlink) tuples
+        :rtype: list
+
+        """
+        previous_symlinks = []
+        for kind in ALL_FOUR:
+            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 previous_symlinks
+
+    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.
+
+        """
+        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 _, 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.
 
@@ -374,10 +404,19 @@ 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
 
-        for kind in ALL_FOUR:
+        """
-            self._update_link_to(kind, version)
+        with error_handler.ErrorHandler(self._fix_symlinks):
+            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 _, link in previous_links:
+                os.unlink(link)
 
     def names(self, version=None):
         """What are the subject names of this certificate?
@@ -532,6 +571,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`"""
@@ -601,7 +642,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.

letsencrypt/tests/cli_test.py

@@ -346,26 +346,25 @@ class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest):
             f.write(test_cert)
 
         # No overlap at all
-        result = _find_duplicative_certs(['wow.net', 'hooray.org'],
+        result = _find_duplicative_certs(
-                                         self.config, self.cli_config)
+            self.cli_config, ['wow.net', 'hooray.org'])
         self.assertEqual(result, (None, None))
 
         # Totally identical
-        result = _find_duplicative_certs(['example.com', 'www.example.com'],
+        result = _find_duplicative_certs(
-                                         self.config, self.cli_config)
+            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',
+        result = _find_duplicative_certs(
-                                          'something.new'], self.config,
+            self.cli_config, ['example.com', 'www.example.com', 'something.new'])
-                                         self.cli_config)
         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'],
+        result = _find_duplicative_certs(
-                                         self.config, self.cli_config)
+            self.cli_config, ['example.com', 'something.new'])
         self.assertEqual(result, (None, None))
 
 

letsencrypt/tests/renewer_test.py

@@ -62,11 +62,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.test_rc = storage.RenewableCert(config.filename, self.cli_config)
-            self.config, self.defaults, self.cli_config)
 
     def tearDown(self):
         shutil.rmtree(self.tempdir)
@@ -98,34 +98,32 @@ 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
+        the renewal configuration file doesn't end in ".conf"
-        isn't a ConfigObj."""
+
+        """
         from letsencrypt import storage
-        defaults = configobj.ConfigObj()
+        broken = os.path.join(self.tempdir, "broken.conf")
-        config = configobj.ConfigObj()
+        with open(broken, "w") as f:
-        # These files don't exist and aren't created here; the point of the test
+            f.write("[No closing bracket for you!")
-        # is to confirm that the constructor rejects them outright because of
+        self.assertRaises(errors.CertStorageError, storage.RenewableCert,
-        # the configfile's name.
+                          broken, self.cli_config)
-        for kind in ALL_FOUR:
+        os.unlink(broken)
-            config["cert"] = "nonexistent_" + kind + ".pem"
+        self.assertRaises(errors.CertStorageError, storage.RenewableCert,
-        config.filename = "nonexistent_sillyfile"
+                          "fun", self.cli_config)
-        self.assertRaises(
-            errors.CertStorageError, storage.RenewableCert, config, defaults)
-        self.assertRaises(TypeError, storage.RenewableCert, "fun", defaults)
 
     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"
+        config.filename = os.path.join(self.tempdir, "imaginary_config.conf")
-        self.assertRaises(
+        config.write()
-            errors.CertStorageError, storage.RenewableCert, config, defaults)
+        self.assertRaises(errors.CertStorageError, storage.RenewableCert,
+                          config.filename, self.cli_config)
 
     def test_consistent(self):
         # pylint: disable=too-many-statements,protected-access
@@ -289,7 +287,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)
@@ -307,6 +305,39 @@ 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
+            basename = os.path.basename(path)
+            if "fullchain" in basename and basename.startswith("prev"):
+                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 os.path.basename(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:
@@ -667,10 +698,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
@@ -682,7 +709,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)
@@ -695,7 +722,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)
@@ -706,7 +733,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.