Merge branch 'master' into reset_by_cli

letsencrypt-apache/letsencrypt_apache/configurator.py

@@ -345,7 +345,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
     def included_in_wildcard(self, names, target_name):
         """Helper function to see if alias is covered by wildcard"""
         target_name = target_name.split(".")[::-1]
-        wildcards = [domain.split(".")[1:] for domain in names if domain.startswith("*")]
+        wildcards = [domain.split(".")[1:] for domain in
+                     names if domain.startswith("*")]
         for wildcard in wildcards:
             if len(wildcard) > len(target_name):
                 continue
@@ -545,7 +546,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             paths = self.aug.match(
                 ("/files%s//*[label()=~regexp('%s')]" %
                     (vhost_path, parser.case_i("VirtualHost"))))
-            paths = [path for path in paths if os.path.basename(path) == "VirtualHost"]
+            paths = [path for path in paths if
+                     os.path.basename(path) == "VirtualHost"]
             for path in paths:
                 new_vhost = self._create_vhost(path)
                 realpath = os.path.realpath(new_vhost.filep)
@@ -890,10 +892,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         if not vh_p:
             return
         vh_path = vh_p[0]
-        if (self.parser.find_dir("ServerName", target_name, start=vh_path, exclude=False)
+        if (self.parser.find_dir("ServerName", target_name,
-           or self.parser.find_dir("ServerAlias", target_name, start=vh_path, exclude=False)):
+                                 start=vh_path, exclude=False) or
+            self.parser.find_dir("ServerAlias", target_name,
+                                 start=vh_path, exclude=False)):
             return
-        if not self.parser.find_dir("ServerName", None, start=vh_path, exclude=False):
+        if not self.parser.find_dir("ServerName", None,
+                                    start=vh_path, exclude=False):
             self.parser.add_dir(vh_path, "ServerName", target_name)
         else:
             self.parser.add_dir(vh_path, "ServerAlias", target_name)

letsencrypt-apache/letsencrypt_apache/constants.py

@@ -18,7 +18,7 @@ CLI_DEFAULTS_DEBIAN = dict(
     handle_sites=True,
     challenge_location="/etc/apache2",
     MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-    "letsencrypt_apache", "options-ssl-apache.conf")
+        "letsencrypt_apache", "options-ssl-apache.conf")
 )
 CLI_DEFAULTS_CENTOS = dict(
     server_root="/etc/httpd",
@@ -35,7 +35,7 @@ CLI_DEFAULTS_CENTOS = dict(
     handle_sites=False,
     challenge_location="/etc/httpd/conf.d",
     MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-    "letsencrypt_apache", "centos-options-ssl-apache.conf")
+        "letsencrypt_apache", "centos-options-ssl-apache.conf")
 )
 CLI_DEFAULTS_GENTOO = dict(
     server_root="/etc/apache2",
@@ -52,7 +52,7 @@ CLI_DEFAULTS_GENTOO = dict(
     handle_sites=False,
     challenge_location="/etc/apache2/vhosts.d",
     MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-    "letsencrypt_apache", "options-ssl-apache.conf")
+        "letsencrypt_apache", "options-ssl-apache.conf")
 )
 CLI_DEFAULTS_DARWIN = dict(
     server_root="/etc/apache2",
@@ -69,7 +69,7 @@ CLI_DEFAULTS_DARWIN = dict(
     handle_sites=False,
     challenge_location="/etc/apache2/other",
     MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-    "letsencrypt_apache", "options-ssl-apache.conf")
+        "letsencrypt_apache", "options-ssl-apache.conf")
 )
 CLI_DEFAULTS = {
     "debian": CLI_DEFAULTS_DEBIAN,

letsencrypt-apache/letsencrypt_apache/obj.py

@@ -208,7 +208,8 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
             # If equal and set is not empty... assume same server
             if self.name is not None or self.aliases:
                 return True
-        # If we're looking for a generic vhost, don't return one with a ServerName
+        # If we're looking for a generic vhost,
+        # don't return one with a ServerName
         elif self.name:
             return False
 

letsencrypt-apache/letsencrypt_apache/tls_sni_01.py

@@ -145,7 +145,7 @@ class ApacheTlsSni01(common.TLSSNI01):
                 parser.case_i("Include"), self.challenge_conf)) == 0:
             # print "Including challenge virtual host(s)"
             logger.debug("Adding Include %s to %s",
-                          self.challenge_conf, parser.get_aug_path(main_config))
+                         self.challenge_conf, parser.get_aug_path(main_config))
             self.configurator.parser.add_dir(
                 parser.get_aug_path(main_config),
                 "Include", self.challenge_conf)

letsencrypt/cli.py

@@ -18,6 +18,7 @@ import letsencrypt
 from letsencrypt import constants
 from letsencrypt import crypto_util
 from letsencrypt import errors
+from letsencrypt import hooks
 from letsencrypt import interfaces
 from letsencrypt import le_util
 
@@ -347,6 +348,8 @@ class HelpfulArgumentParser(object):
                                    "cannot be used with --csr")
             self.handle_csr(parsed_args)
 
+        hooks.validate_hooks(parsed_args)
+
         return parsed_args
 
     def handle_csr(self, parsed_args):
@@ -584,7 +587,14 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
         None, "--dry-run", action="store_true", dest="dry_run",
         help="Perform a test run of the client, obtaining test (invalid) certs"
              " but not saving them to disk. This can currently only be used"
-             " with the 'certonly' subcommand.")
+             " with the 'certonly' and 'renew' subcommands. \nNote: Although --dry-run"
+             " tries to avoid making any persistent changes on a system, it "
+             " is not completely side-effect free: if used with webserver authenticator plugins"
+             " like apache and nginx, it makes and then reverts temporary config changes"
+             " in order to obtain test certs, and reloads webservers to deploy and then"
+             " roll back those changes.  It also calls --pre-hook and --post-hook commands"
+             " if they are defined because they may be necessary to accurately simulate"
+             " renewal. --renew-hook commands are not called.")
     helpful.add(
         None, "--register-unsafely-without-email", action="store_true",
         help="Specifying this flag enables registering an account with no "
@@ -721,7 +731,26 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
         " used to create obtain or most recently successfully renew each"
         " certificate lineage. You can try it with `--dry-run` first. For"
         " more fine-grained control, you can renew individual lineages with"
-        " the `certonly` subcommand.")
+        " the `certonly` subcommand. Hooks are available to run commands "
+        " before and after renewal; see XXX for more information on these.")
+
+    helpful.add(
+        "renew", "--pre-hook",
+        help="Command to be run in a shell before obtaining any certificates. Intended"
+        " primarily for renewal, where it can be used to temporarily shut down a"
+        " webserver that might conflict with the standalone plugin. This will "
+        " only be called if a certificate is actually to be obtained/renewed. ")
+    helpful.add(
+        "renew", "--post-hook",
+        help="Command to be run in a shell after attempting to obtain/renew "
+        " certificates. Can be used to deploy renewed certificates, or to restart"
+        " any servers that were stopped by --pre-hook.")
+    helpful.add(
+        "renew", "--renew-hook",
+        help="Command to be run in a shell once for each successfully renewed certificate."
+        "For this command, the shell variable $RENEWED_LINEAGE will point to the"
+        "config live subdirectory containing the new certs and keys; the shell variable "
+        "$RENEWED_DOMAINS will conatain a space-delimited list of renewed cert domains")
 
     helpful.add_deprecated_argument("--agree-dev-preview", 0)
 

letsencrypt/errors.py

@@ -25,6 +25,10 @@ class CertStorageError(Error):
     """Generic `.CertStorage` error."""
 
 
+class HookCommandNotFound(Error):
+    """Failed to find a hook command in the PATH."""
+
+
 # Auth Handler Errors
 class AuthorizationError(Error):
     """Authorization error."""

letsencrypt/hooks.py

@@ -0,0 +1,98 @@
+"""Facilities for implementing hooks that call shell commands."""
+from __future__ import print_function
+
+import logging
+import os
+
+from subprocess import Popen, PIPE
+
+from letsencrypt import errors
+
+logger = logging.getLogger(__name__)
+
+def validate_hooks(config):
+    """Check hook commands are executable."""
+    _validate_hook(config.pre_hook, "pre")
+    _validate_hook(config.post_hook, "post")
+    _validate_hook(config.renew_hook, "renew")
+
+def _prog(shell_cmd):
+    """Extract the program run by a shell command"""
+    cmd = _which(shell_cmd)
+    return os.path.basename(cmd) if cmd else None
+
+def _validate_hook(shell_cmd, hook_name):
+    """Check that a command provided as a hook is plausibly executable.
+
+    :raises .errors.HookCommandNotFound: if the command is not found
+    """
+    if shell_cmd:
+        cmd = shell_cmd.partition(" ")[0]
+        if not _prog(cmd):
+            path = os.environ["PATH"]
+            msg = "Unable to find {2}-hook command {0} in the PATH.\n(PATH is {1})".format(
+                cmd, path, hook_name)
+            raise errors.HookCommandNotFound(msg)
+
+def pre_hook(config):
+    "Run pre-hook if it's defined and hasn't been run."
+    if config.pre_hook and not pre_hook.already:
+        logger.info("Running pre-hook command: %s", config.pre_hook)
+        _run_hook(config.pre_hook)
+        pre_hook.already = True
+
+pre_hook.already = False
+
+def post_hook(config, final=False):
+    """Run post hook if defined.
+
+    If the verb is renew, we might have more certs to renew, so we wait until
+    we're called with final=True before actually doing anything.
+    """
+    if config.post_hook:
+        if final or config.verb != "renew":
+            logger.info("Running post-hook command: %s", config.post_hook)
+            _run_hook(config.post_hook)
+
+def renew_hook(config, domains, lineage_path):
+    "Run post-renewal hook if defined."
+    if config.renew_hook:
+        if not config.dry_run:
+            os.environ["RENEWED_DOMAINS"] = " ".join(domains)
+            os.environ["RENEWED_LINEAGE"] = lineage_path
+            _run_hook(config.renew_hook)
+        else:
+            print("Dry run: skipping renewal hook command: {0}".format(config.renew_hook))
+
+def _run_hook(shell_cmd):
+    """Run a hook command.
+
+    :returns: stderr if there was any"""
+
+    cmd = Popen(shell_cmd, shell=True, stdout=PIPE, stderr=PIPE, stdin=PIPE)
+    _out, err = cmd.communicate()
+    if cmd.returncode != 0:
+        logger.error('Hook command "%s" returned error code %d', shell_cmd, cmd.returncode)
+    if err:
+        logger.error('Error output from %s:\n%s', _prog(shell_cmd), err)
+
+def _is_exe(fpath):
+    return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
+
+def _which(program):
+    """Test if program is in the path."""
+    # Borrowed from:
+    # https://stackoverflow.com/questions/377017/test-if-executable-exists-in-python
+    # XXX May need more porting to handle .exe extensions on Windows
+
+    fpath, _fname = os.path.split(program)
+    if fpath:
+        if _is_exe(program):
+            return program
+    else:
+        for path in os.environ["PATH"].split(os.pathsep):
+            exe_file = os.path.join(path, program)
+            if _is_exe(exe_file):
+                return exe_file
+
+    return None

letsencrypt/main.py

@@ -8,7 +8,6 @@ import sys
 import time
 import traceback
 
-import OpenSSL
 import zope.component
 
 from acme import jose
@@ -23,6 +22,7 @@ from letsencrypt import colored_logging
 from letsencrypt import configuration
 from letsencrypt import constants
 from letsencrypt import errors
+from letsencrypt import hooks
 from letsencrypt import interfaces
 from letsencrypt import le_util
 from letsencrypt import log
@@ -52,28 +52,6 @@ def _suggest_donation_if_appropriate(config, action):
     reporter_util.add_message(msg, reporter_util.LOW_PRIORITY)
 
 
-def _avoid_invalidating_lineage(config, lineage, original_server):
-    "Do not renew a valid cert with one from a staging server!"
-    def _is_staging(srv):
-        return srv == constants.STAGING_URI or "staging" in srv
-
-    # Some lineages may have begun with --staging, but then had production certs
-    # added to them
-    latest_cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM,
-                                                  open(lineage.cert).read())
-    # all our test certs are from happy hacker fake CA, though maybe one day
-    # we should test more methodically
-    now_valid = "fake" not in repr(latest_cert.get_issuer()).lower()
-
-    if _is_staging(config.server):
-        if not _is_staging(original_server) or now_valid:
-            if not config.break_my_certs:
-                names = ", ".join(lineage.names())
-                raise errors.Error(
-                    "You've asked to renew/replace a seemingly valid certificate with "
-                    "a test certificate (domains: {0}). We will not do that "
-                    "unless you use the --break-my-certs flag!".format(names))
-
 
 def _report_successful_dry_run(config):
     reporter_util = zope.component.getUtility(interfaces.IReporter)
@@ -82,6 +60,7 @@ def _report_successful_dry_run(config):
                                   reporter_util.HIGH_PRIORITY, on_crash=False)
 
 
+
 def _auth_from_domains(le_client, config, domains, lineage=None):
     """Authenticate and enroll certificate."""
     # Note: This can raise errors... caught above us though. This is now
@@ -105,31 +84,18 @@ def _auth_from_domains(le_client, config, domains, lineage=None):
         # The lineage already exists; allow the caller to try installing
         # it without getting a new certificate at all.
         return lineage, "reinstall"
-    elif action == "renew":
+
-        original_server = lineage.configuration["renewalparams"]["server"]
+    hooks.pre_hook(config)
-        _avoid_invalidating_lineage(config, lineage, original_server)
+    try:
-        # TODO: schoen wishes to reuse key - discussion
+        if action == "renew":
-        # https://github.com/letsencrypt/letsencrypt/pull/777/files#r40498574
+            renewal.renew_cert(config, domains, le_client, lineage)
-        new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
+        elif action == "newcert":
-        # TODO: Check whether it worked! <- or make sure errors are thrown (jdk)
+            # TREAT AS NEW REQUEST
-        if config.dry_run:
+            lineage = le_client.obtain_and_enroll_certificate(domains)
-            logger.info("Dry run: skipping updating lineage at %s",
+            if lineage is False:
-                        os.path.dirname(lineage.cert))
+                raise errors.Error("Certificate could not be obtained")
-        else:
+    finally:
-            lineage.save_successor(
+        hooks.post_hook(config)
-                lineage.latest_common_version(), OpenSSL.crypto.dump_certificate(
-                    OpenSSL.crypto.FILETYPE_PEM, new_certr.body.wrapped),
-                new_key.pem, crypto_util.dump_pyopenssl_chain(new_chain),
-                configuration.RenewerConfiguration(config.namespace))
-            lineage.update_all_links_to(lineage.latest_common_version())
-        # TODO: Check return value of save_successor
-        # TODO: Also update lineage renewal config with any relevant
-        #       configuration values from this attempt? <- Absolutely (jdkasten)
-    elif action == "newcert":
-        # TREAT AS NEW REQUEST
-        lineage = le_client.obtain_and_enroll_certificate(domains)
-        if lineage is False:
-            raise errors.Error("Certificate could not be obtained")
 
     if not config.dry_run and not config.verb == "renew":
         _report_new_cert(lineage.cert, lineage.fullchain)
@@ -142,7 +108,8 @@ def _handle_subset_cert_request(config, domains, cert):
 
     :param storage.RenewableCert cert:
 
-    :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+    :returns: Tuple of (str action, cert_or_None) as per _treat_as_renewal
+              action can be: "newcert" | "renew" | "reinstall"
     :rtype: tuple
 
     """
@@ -183,7 +150,8 @@ def _handle_identical_cert_request(config, cert):
 
     :param storage.RenewableCert cert:
 
-    :returns: Tuple of (string, cert_or_None) as per _treat_as_renewal
+    :returns: Tuple of (str action, cert_or_None) as per _treat_as_renewal
+              action can be: "newcert" | "renew" | "reinstall"
     :rtype: tuple
 
     """
@@ -507,41 +475,53 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
     _suggest_donation_if_appropriate(config, action)
 
 
+def _csr_obtain_cert(config, le_client):
+    """Obtain a cert using a user-supplied CSR
+
+    This works differently in the CSR case (for now) because we don't
+    have the privkey, and therefore can't construct the files for a lineage.
+    So we just save the cert & chain to disk :/
+    """
+    csr, typ = config.actual_csr
+    certr, chain = le_client.obtain_certificate_from_csr(config.domains, csr, typ)
+    if config.dry_run:
+        logger.info(
+            "Dry run: skipping saving certificate to %s", config.cert_path)
+    else:
+        cert_path, _, cert_fullchain = le_client.save_certificate(
+            certr, chain, config.cert_path, config.chain_path, config.fullchain_path)
+        _report_new_cert(cert_path, cert_fullchain)
+
+
 def obtain_cert(config, plugins, lineage=None):
-    """Implements "certonly": authenticate & obtain cert, but do not install it."""
+    """Authenticate & obtain cert, but do not install it.
-    # pylint: disable=too-many-locals
+
+    This implements the 'certonly' subcommand, and is also called from within the
+    'renew' command."""
+
+    # SETUP: Select plugins and construct a client instance
     try:
         # installers are used in auth mode to determine domain names
-        installer, authenticator = plug_sel.choose_configurator_plugins(config, plugins, "certonly")
+        installer, auth = plug_sel.choose_configurator_plugins(config, plugins, "certonly")
     except errors.PluginSelectionError as e:
         logger.info("Could not choose appropriate plugin: %s", e)
         raise
+    le_client = _init_le_client(config, auth, installer)
 
-    # TODO: Handle errors from _init_le_client?
+    # SHOWTIME: Possibly obtain/renew a cert, and set action to renew | newcert | reinstall
-    le_client = _init_le_client(config, authenticator, installer)
+    if config.csr is None: # the common case
-
-    action = "newcert"
-    # This is a special case; cert and chain are simply saved
-    if config.csr is not None:
-        assert lineage is None, "Did not expect a CSR with a RenewableCert"
-        csr, typ = config.actual_csr
-        certr, chain = le_client.obtain_certificate_from_csr(config.domains, csr, typ)
-        if config.dry_run:
-            logger.info(
-                "Dry run: skipping saving certificate to %s", config.cert_path)
-        else:
-            cert_path, _, cert_fullchain = le_client.save_certificate(
-                certr, chain, config.cert_path, config.chain_path, config.fullchain_path)
-            _report_new_cert(cert_path, cert_fullchain)
-    else:
         domains = _find_domains(config, installer)
         _, action = _auth_from_domains(le_client, config, domains, lineage)
+    else:
+        assert lineage is None, "Did not expect a CSR with a RenewableCert"
+        _csr_obtain_cert(config, le_client)
+        action = "newcert"
 
+    # POSTPRODUCTION: Cleanup, deployment & reporting
     if config.dry_run:
         _report_successful_dry_run(config)
     elif config.verb == "renew":
         if installer is None:
-            # Tell the user that the server was not restarted.
             print("new certificate deployed without reload, fullchain is",
                   lineage.fullchain)
         else:
@@ -553,10 +533,13 @@ def obtain_cert(config, plugins, lineage=None):
                   config.installer, "server; fullchain is", lineage.fullchain)
     _suggest_donation_if_appropriate(config, action)
 
+
 def renew(config, unused_plugins):
     """Renew previously-obtained certificates."""
-    renewal.renew_all_lineages(config)
+    try:
-
+        renewal.renew_all_lineages(config)
+    finally:
+        hooks.post_hook(config, final=True)
 
 
 def setup_log_file_handler(config, logfile, fmt):

letsencrypt/renewal.py

@@ -9,9 +9,15 @@ import traceback
 import six
 import zope.component
 
+import OpenSSL
+
 from letsencrypt import configuration
 from letsencrypt import cli
+from letsencrypt import constants
+
+from letsencrypt import crypto_util
 from letsencrypt import errors
+from letsencrypt import hooks
 from letsencrypt import storage
 from letsencrypt.plugins import disco as plugins_disco
 
@@ -197,6 +203,50 @@ def should_renew(config, lineage):
     return False
 
 
+def _avoid_invalidating_lineage(config, lineage, original_server):
+    "Do not renew a valid cert with one from a staging server!"
+    def _is_staging(srv):
+        return srv == constants.STAGING_URI or "staging" in srv
+
+    # Some lineages may have begun with --staging, but then had production certs
+    # added to them
+    latest_cert = OpenSSL.crypto.load_certificate(
+        OpenSSL.crypto.FILETYPE_PEM, open(lineage.cert).read())
+    # all our test certs are from happy hacker fake CA, though maybe one day
+    # we should test more methodically
+    now_valid = "fake" not in repr(latest_cert.get_issuer()).lower()
+
+    if _is_staging(config.server):
+        if not _is_staging(original_server) or now_valid:
+            if not config.break_my_certs:
+                names = ", ".join(lineage.names())
+                raise errors.Error(
+                    "You've asked to renew/replace a seemingly valid certificate with "
+                    "a test certificate (domains: {0}). We will not do that "
+                    "unless you use the --break-my-certs flag!".format(names))
+
+
+def renew_cert(config, domains, le_client, lineage):
+    "Renew a certificate lineage."
+    original_server = lineage.configuration["renewalparams"]["server"]
+    _avoid_invalidating_lineage(config, lineage, original_server)
+    new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
+    if config.dry_run:
+        logger.info("Dry run: skipping updating lineage at %s",
+                    os.path.dirname(lineage.cert))
+    else:
+        prior_version = lineage.latest_common_version()
+        new_cert = OpenSSL.crypto.dump_certificate(
+            OpenSSL.crypto.FILETYPE_PEM, new_certr.body.wrapped)
+        new_chain = crypto_util.dump_pyopenssl_chain(new_chain)
+        renewal_conf = configuration.RenewerConfiguration(config.namespace)
+        lineage.save_successor(prior_version, new_cert, new_key.pem, new_chain, renewal_conf)
+        lineage.update_all_links_to(lineage.latest_common_version())
+
+    hooks.renew_hook(config, domains, lineage.live_dir)
+    # TODO: Check return value of save_successor
+
+
 def _renew_describe_results(config, renew_successes, renew_failures,
                             renew_skipped, parse_failures):
     def _status(msgs, category):

letsencrypt/storage.py

@@ -50,10 +50,11 @@ 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, relevant_data):
+def write_renewal_config(o_filename, n_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 str o_filename: Absolute path to the previous version of config file
+    :param str n_filename: Absolute path to the new destination of config file
     :param dict target: Maps ALL_FOUR to their symlink paths
     :param dict relevant_data: Renewal configuration options to save
 
@@ -61,21 +62,27 @@ def write_renewal_config(filename, target, relevant_data):
     :rtype: configobj.ConfigObj
 
     """
-    # create_empty creates a new config file if filename does not exist
+    config = configobj.ConfigObj(o_filename)
-    config = configobj.ConfigObj(filename, create_empty=True)
     for kind in ALL_FOUR:
         config[kind] = target[kind]
 
-    if relevant_data:
+    if "renewalparams" not in config:
-        config["renewalparams"] = relevant_data
+        config["renewalparams"] = {}
         config.comments["renewalparams"] = ["",
                                             "Options used in "
                                             "the renewal process"]
 
+    config["renewalparams"].update(relevant_data)
+
+    for k in config["renewalparams"].keys():
+        if k not in relevant_data:
+            del config["renewalparams"][k]
+
     # TODO: add human-readable comments explaining other available
     #       parameters
-    logger.debug("Writing new config %s.", filename)
+    logger.debug("Writing new config %s.", n_filename)
-    config.write()
+    with open(n_filename, "w") as f:
+        config.write(outfile=f)
     return config
 
 
@@ -101,7 +108,7 @@ def update_configuration(lineagename, 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)
+    write_renewal_config(config_filename, temp_filename, target, values)
     os.rename(temp_filename, config_filename)
 
     return configobj.ConfigObj(config_filename)
@@ -252,6 +259,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         self.privkey = self.configuration["privkey"]
         self.chain = self.configuration["chain"]
         self.fullchain = self.configuration["fullchain"]
+        self.live_dir = os.path.dirname(self.cert)
 
         self._fix_symlinks()
         self._check_symlinks()
@@ -798,7 +806,7 @@ 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))
 
-        new_config = write_renewal_config(config_filename, target, values)
+        new_config = write_renewal_config(config_filename, config_filename, target, values)
         return cls(new_config.filename, cli_config)
 
     def save_successor(self, prior_version, new_cert,

letsencrypt/tests/cli_test.py

@@ -580,11 +580,11 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
                     mock_init.return_value = mock_client
                     get_utility_path = 'letsencrypt.main.zope.component.getUtility'
                     with mock.patch(get_utility_path) as mock_get_utility:
-                        with mock.patch('letsencrypt.main.OpenSSL') as mock_ssl:
+                        with mock.patch('letsencrypt.main.renewal.OpenSSL') as mock_ssl:
                             mock_latest = mock.MagicMock()
                             mock_latest.get_issuer.return_value = "Fake fake"
                             mock_ssl.crypto.load_certificate.return_value = mock_latest
-                            with mock.patch('letsencrypt.main.crypto_util'):
+                            with mock.patch('letsencrypt.main.renewal.crypto_util'):
                                 if not args:
                                     args = ['-d', 'isnot.org', '-a', 'standalone', 'certonly']
                                 if extra_args:

letsencrypt/tests/hook_test.py

@@ -0,0 +1,105 @@
+"""Tests for hooks.py"""
+# pylint: disable=protected-access
+
+import os
+import unittest
+import sys
+
+import mock
+
+from letsencrypt import errors
+from letsencrypt import hooks
+
+class HookTest(unittest.TestCase):
+    def setUp(self):
+        pass
+
+    def tearDown(self):
+        pass
+
+    @mock.patch('letsencrypt.hooks._prog')
+    def test_validate_hooks(self, mock_prog):
+        config = mock.MagicMock(pre_hook="", post_hook="ls -lR", renew_hook="uptime")
+        hooks.validate_hooks(config)
+        self.assertEqual(mock_prog.call_count, 2)
+        self.assertEqual(mock_prog.call_args_list[1][0][0], 'uptime')
+        self.assertEqual(mock_prog.call_args_list[0][0][0], 'ls')
+        mock_prog.return_value = None
+        config = mock.MagicMock(pre_hook="explodinator", post_hook="", renew_hook="")
+        self.assertRaises(errors.HookCommandNotFound, hooks.validate_hooks, config)
+
+    @mock.patch('letsencrypt.hooks._is_exe')
+    def test_which(self, mock_is_exe):
+        mock_is_exe.return_value = True
+        self.assertEqual(hooks._which("/path/to/something"), "/path/to/something")
+
+        with mock.patch.dict('os.environ', {"PATH": "/floop:/fleep"}):
+            mock_is_exe.return_value = True
+            self.assertEqual(hooks._which("pingify"), "/floop/pingify")
+            mock_is_exe.return_value = False
+            self.assertEqual(hooks._which("pingify"), None)
+        self.assertEqual(hooks._which("/path/to/something"), None)
+
+    @mock.patch('letsencrypt.hooks._which')
+    def test_prog(self, mockwhich):
+        mockwhich.return_value = "/very/very/funky"
+        self.assertEqual(hooks._prog("funky"), "funky")
+        mockwhich.return_value = None
+        self.assertEqual(hooks._prog("funky"), None)
+
+    def _test_a_hook(self, config, hook_function, calls_expected):
+        with mock.patch('letsencrypt.hooks.logger'):
+            with mock.patch('letsencrypt.hooks._run_hook') as mock_run_hook:
+                hook_function(config)
+                hook_function(config)
+                self.assertEqual(mock_run_hook.call_count, calls_expected)
+
+    def test_pre_hook(self):
+        config = mock.MagicMock(pre_hook="true")
+        self._test_a_hook(config, hooks.pre_hook, 1)
+        config = mock.MagicMock(pre_hook="")
+        self._test_a_hook(config, hooks.pre_hook, 0)
+
+    def test_post_hook(self):
+        config = mock.MagicMock(post_hook="true", verb="splonk")
+        self._test_a_hook(config, hooks.post_hook, 2)
+        config = mock.MagicMock(post_hook="true", verb="renew")
+        self._test_a_hook(config, hooks.post_hook, 0)
+
+    def test_renew_hook(self):
+        with mock.patch.dict('os.environ', {}):
+            domains = ["a", "b"]
+            lineage = "thing"
+            rhook = lambda x: hooks.renew_hook(x, domains, lineage)
+
+            config = mock.MagicMock(renew_hook="true", dry_run=False)
+            self._test_a_hook(config, rhook, 2)
+            self.assertEqual(os.environ["RENEWED_DOMAINS"], "a b")
+            self.assertEqual(os.environ["RENEWED_LINEAGE"], "thing")
+
+            config = mock.MagicMock(renew_hook="true", dry_run=True)
+            if sys.version_info < (2, 7):
+                # the print() function is not mockable in py26
+                self._test_a_hook(config, rhook, 0)
+            else:
+                with mock.patch("letsencrypt.hooks.print") as mock_print:
+                    self._test_a_hook(config, rhook, 0)
+                    self.assertEqual(mock_print.call_count, 2)
+
+    @mock.patch('letsencrypt.hooks.Popen')
+    def test_run_hook(self, mock_popen):
+        with mock.patch('letsencrypt.hooks.logger.error') as mock_error:
+            mock_cmd = mock.MagicMock()
+            mock_cmd.returncode = 1
+            mock_cmd.communicate.return_value = ("", "")
+            mock_popen.return_value = mock_cmd
+            hooks._run_hook("ls")
+            self.assertEqual(mock_error.call_count, 1)
+        with mock.patch('letsencrypt.hooks.logger.error') as mock_error:
+            mock_cmd.communicate.return_value = ("", "thing")
+            hooks._run_hook("ls")
+            self.assertEqual(mock_error.call_count, 2)
+
+
+if __name__ == '__main__':
+    unittest.main()  # pragma: no cover

letsencrypt/tests/storage_test.py

@@ -1,4 +1,5 @@
 """Tests for letsencrypt.storage."""
+# pylint disable=protected-access
 import datetime
 import os
 import shutil
@@ -741,6 +742,29 @@ class RenewableCertTests(BaseRenewableCertTest):
                           storage.RenewableCert,
                           self.config.filename, self.cli_config)
 
+    def test_write_renewal_config(self):
+        # Mostly tested by the process of creating and updating lineages,
+        # but we can test that this successfully creates files, removes
+        # unneeded items, and preserves comments.
+        temp = os.path.join(self.tempdir, "sample-file")
+        temp2 = os.path.join(self.tempdir, "sample-file.new")
+        with open(temp, "w") as f:
+            f.write("[renewalparams]\nuseful = value # A useful value\n"
+                    "useless = value # Not needed\n")
+        target = {}
+        for x in ALL_FOUR:
+            target[x] = "somewhere"
+        relevant_data = {"useful": "new_value"}
+        from letsencrypt import storage
+        storage.write_renewal_config(temp, temp2, target, relevant_data)
+        with open(temp2, "r") as f:
+            content = f.read()
+        # useful value was updated
+        assert "useful = new_value" in content
+        # associated comment was preserved
+        assert "A useful value" in content
+        # useless value was deleted
+        assert "useless" not in content
 
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover