Merge branch 'master' into interactive-webroot

letsencrypt-apache/letsencrypt_apache/tests/apache-conf-files/passing/example-ssl.conf

@@ -39,7 +39,7 @@
     #   certificate chain for the server certificate. Alternatively
     #   the referenced file can be the same as SSLCertificateFile
     #   when the CA certificates are directly appended to the server
-    #   certificate for convinience.
+    #   certificate for convenience.
     #SSLCertificateChainFile /etc/apache2/ssl.crt/server-ca.crt
 
     #   Certificate Authority (CA):

letsencrypt/cli.py

@@ -17,6 +17,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
 
@@ -85,6 +86,48 @@ More detailed help:
 """
 
 
+# These argparse parameters should be removed when detecting defaults.
+ARGPARSE_PARAMS_TO_REMOVE = ("const", "nargs", "type",)
+
+
+# These sets are used when to help detect options set by the user.
+EXIT_ACTIONS = set(("help", "version",))
+
+
+ZERO_ARG_ACTIONS = set(("store_const", "store_true",
+                        "store_false", "append_const", "count",))
+
+
+# Maps a config option to a set of config options that may have modified it.
+# This dictionary is used recursively, so if A modifies B and B modifies C,
+# it is determined that C was modified by the user if A was modified.
+VAR_MODIFIERS = {"account": set(("server",)),
+                 "server": set(("dry_run", "staging",)),
+                 "webroot_map": set(("webroot_path",))}
+
+
+def report_config_interaction(modified, modifiers):
+    """Registers config option interaction to be checked by set_by_cli.
+
+    This function can be called by during the __init__ or
+    add_parser_arguments methods of plugins to register interactions
+    between config options.
+
+    :param modified: config options that can be modified by modifiers
+    :type modified: iterable or str
+    :param modifiers: config options that modify modified
+    :type modifiers: iterable or str
+
+    """
+    if isinstance(modified, str):
+        modified = (modified,)
+    if isinstance(modifiers, str):
+        modifiers = (modifiers,)
+
+    for var in modified:
+        VAR_MODIFIERS.setdefault(var, set()).update(modifiers)
+
+
 def usage_strings(plugins):
     """Make usage strings late so that plugins can be initialised late"""
     if "nginx" in plugins:
@@ -98,6 +141,22 @@ def usage_strings(plugins):
     return USAGE % (apache_doc, nginx_doc), SHORT_USAGE
 
 
+class _Default(object):
+    """A class to use as a default to detect if a value is set by a user"""
+
+    def __bool__(self):
+        return False
+
+    def __eq__(self, other):
+        return isinstance(other, _Default)
+
+    def __hash__(self):
+        return id(_Default)
+
+    def __nonzero__(self):
+        return self.__bool__()
+
+
 def set_by_cli(var):
     """
     Return True if a particular config variable has been set by the user
@@ -114,30 +173,18 @@ def set_by_cli(var):
         detector = set_by_cli.detector = prepare_and_parse_args(
             plugins, reconstructed_args, detect_defaults=True)
         # propagate plugin requests: eg --standalone modifies config.authenticator
-        auth, inst = plugin_selection.cli_plugin_requests(detector)
-        detector.authenticator = auth if auth else ""
-        detector.installer = inst if inst else ""
+        detector.authenticator, detector.installer = (
+            plugin_selection.cli_plugin_requests(detector))
         logger.debug("Default Detector is %r", detector)
 
-    try:
-        # Is detector.var something that isn't false?
-        change_detected = getattr(detector, var)
-    except AttributeError:
-        logger.warning("Missing default analysis for %r", var)
-        return False
+    if not isinstance(getattr(detector, var), _Default):
+        return True
 
-    if change_detected:
-        return True
-    # Special case: we actually want account to be set to "" if the server
-    # the account was on has changed
-    elif var == "account" and (detector.server or detector.dry_run or detector.staging):
-        return True
-    # Special case: vars like --no-redirect that get set True -> False
-    # default to None; False means they were set
-    elif var in detector.store_false_vars and change_detected is not None:
-        return True
-    else:
-        return False
+    for modifier in VAR_MODIFIERS.get(var, []):
+        if set_by_cli(modifier):
+            return True
+
+    return False
 # static housekeeping var
 set_by_cli.detector = None
 
@@ -186,21 +233,23 @@ def config_help(name, hidden=False):
         return interfaces.IConfig[name].__doc__
 
 
-class SilentParser(object):  # pylint: disable=too-few-public-methods
-    """Silent wrapper around argparse.
+class HelpfulArgumentGroup(object):
+    """Emulates an argparse group for use with HelpfulArgumentParser.
 
-    A mini parser wrapper that doesn't print help for its
-    arguments. This is needed for the use of callbacks to define
-    arguments within plugins.
+    This class is used in the add_group method of HelpfulArgumentParser.
+    Command line arguments can be added to the group, but help
+    suppression and default detection is applied by
+    HelpfulArgumentParser when necessary.
 
     """
-    def __init__(self, parser):
-        self.parser = parser
+    def __init__(self, helpful_arg_parser, topic):
+        self._parser = helpful_arg_parser
+        self._topic = topic
 
     def add_argument(self, *args, **kwargs):
-        """Wrap, but silence help"""
-        kwargs["help"] = argparse.SUPPRESS
-        self.parser.add_argument(*args, **kwargs)
+        """Add a new command line argument to the argument group."""
+        self._parser.add(self._topic, *args, **kwargs)
+
 
 class HelpfulArgumentParser(object):
     """Argparse Wrapper.
@@ -233,15 +282,8 @@ class HelpfulArgumentParser(object):
 
         # This is the only way to turn off overly verbose config flag documentation
         self.parser._add_config_file_help = False  # pylint: disable=protected-access
-        self.silent_parser = SilentParser(self.parser)
 
-        # This setting attempts to force all default values to things that are
-        # pythonically false; it is used to detect when values have been
-        # explicitly set by the user, including when they are set to their
-        # normal default value
         self.detect_defaults = detect_defaults
-        if detect_defaults:
-            self.store_false_vars = {}  # vars that use "store_false"
 
         self.args = args
         self.determine_verb()
@@ -267,6 +309,9 @@ class HelpfulArgumentParser(object):
         parsed_args.func = self.VERBS[self.verb]
         parsed_args.verb = self.verb
 
+        if self.detect_defaults:
+            return parsed_args
+
         # Do any post-parsing homework here
 
         if parsed_args.staging or parsed_args.dry_run:
@@ -296,8 +341,7 @@ class HelpfulArgumentParser(object):
                                    "cannot be used with --csr")
             self.handle_csr(parsed_args)
 
-        if self.detect_defaults:  # plumbing
-            parsed_args.store_false_vars = self.store_false_vars
+        hooks.validate_hooks(parsed_args)
 
         return parsed_args
 
@@ -399,7 +443,7 @@ class HelpfulArgumentParser(object):
         """
 
         if self.detect_defaults:
-            kwargs = self.modify_arg_for_default_detection(self, *args, **kwargs)
+            kwargs = self.modify_kwargs_for_default_detection(**kwargs)
 
         if self.visible_topics[topic]:
             if topic in self.groups:
@@ -411,39 +455,28 @@ class HelpfulArgumentParser(object):
             kwargs["help"] = argparse.SUPPRESS
             self.parser.add_argument(*args, **kwargs)
 
+    def modify_kwargs_for_default_detection(self, **kwargs):
+        """Modify an arg so we can check if it was set by the user.
 
-    def modify_arg_for_default_detection(self, *args, **kwargs):
-        """
-        Adding an arg, but ensure that it has a default that evaluates to false,
-        so that set_by_cli can tell if it was set.  Only called if detect_defaults==True.
+        Changes the parameters given to argparse when adding an argument
+        so we can properly detect if the value was set by the user.
 
-        :param list *args: the names of this argument flag
-        :param dict **kwargs: various argparse settings for this argument
+        :param dict kwargs: various argparse settings for this argument
 
         :returns: a modified versions of kwargs
+        :rtype: dict
+
         """
-        # argument either doesn't have a default, or the default doesn't
-        # isn't Pythonically false
-        if kwargs.get("default", True):
-            arg_type = kwargs.get("type", None)
-            if arg_type == int or kwargs.get("action", "") == "count":
-                kwargs["default"] = 0
-            elif arg_type == read_file or "-c" in args:
-                kwargs["default"] = ""
-                kwargs["type"] = str
-            else:
-                kwargs["default"] = ""
-            # This doesn't matter at present (none of the store_false args
-            # are renewal-relevant), but implement it for future sanity:
-            # detect the setting of args whose presence causes True -> False
-        if kwargs.get("action", "") == "store_false":
-            kwargs["default"] = None
-            for var in args:
-                self.store_false_vars[var] = True
+        action = kwargs.get("action", None)
+        if action not in EXIT_ACTIONS:
+            kwargs["action"] = ("store_true" if action in ZERO_ARG_ACTIONS else
+                                "store")
+            kwargs["default"] = _Default()
+            for param in ARGPARSE_PARAMS_TO_REMOVE:
+                kwargs.pop(param, None)
 
         return kwargs
 
-
     def add_deprecated_argument(self, argument_name, num_args):
         """Adds a deprecated argument with the name argument_name.
 
@@ -459,22 +492,22 @@ class HelpfulArgumentParser(object):
             self.parser.add_argument, argument_name, num_args)
 
     def add_group(self, topic, **kwargs):
-        """
+        """Create a new argument group.
 
-        This has to be called once for every topic; but we leave those calls
-        next to the argument definitions for clarity. Return something
-        arguments can be added to if necessary, either the parser or an argument
-        group.
+        This method must be called once for every topic, however, calls
+        to this function are left next to the argument definitions for
+        clarity.
+
+        :param str topic: Name of the new argument group.
+
+        :returns: The new argument group.
+        :rtype: `HelpfulArgumentGroup`
 
         """
         if self.visible_topics[topic]:
-            #print("Adding visible group " + topic)
-            group = self.parser.add_argument_group(topic, **kwargs)
-            self.groups[topic] = group
-            return group
-        else:
-            #print("Invisible group " + topic)
-            return self.silent_parser
+            self.groups[topic] = self.parser.add_argument_group(topic, **kwargs)
+
+        return HelpfulArgumentGroup(self, topic)
 
     def add_plugin_args(self, plugins):
         """
@@ -485,7 +518,6 @@ class HelpfulArgumentParser(object):
         """
         for name, plugin_ep in six.iteritems(plugins):
             parser_or_group = self.add_group(name, description=plugin_ep.description)
-            #print(parser_or_group)
             plugin_ep.plugin_cls.inject_parser_options(parser_or_group, name)
 
     def determine_help_topics(self, chosen_topic):
@@ -542,7 +574,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 "
@@ -679,7 +718,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 contain 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"]
-        _avoid_invalidating_lineage(config, lineage, original_server)
-        # TODO: schoen wishes to reuse key - discussion
-        # https://github.com/letsencrypt/letsencrypt/pull/777/files#r40498574
-        new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
-        # TODO: Check whether it worked! <- or make sure errors are thrown (jdk)
-        if config.dry_run:
-            logger.info("Dry run: skipping updating lineage at %s",
-                        os.path.dirname(lineage.cert))
-        else:
-            lineage.save_successor(
-                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")
+
+    hooks.pre_hook(config)
+    try:
+        if action == "renew":
+            renewal.renew_cert(config, domains, le_client, lineage)
+        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")
+    finally:
+        hooks.post_hook(config)
 
     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
 
     """
@@ -503,41 +471,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."""
-    # pylint: disable=too-many-locals
+    """Authenticate & obtain cert, but do not install it.
+
+    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?
-    le_client = _init_le_client(config, authenticator, installer)
-
-    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:
+    # SHOWTIME: Possibly obtain/renew a cert, and set action to renew | newcert | reinstall
+    if config.csr is None: # the common case
         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:
@@ -549,10 +529,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/plugins/common.py

@@ -45,15 +45,14 @@ class Plugin(object):
     def add_parser_arguments(cls, add):
         """Add plugin arguments to the CLI argument parser.
 
+        NOTE: If some of your flags interact with others, you can
+        use cli.report_config_interaction to register this to ensure
+        values are correctly saved/overridable during renewal.
+
         :param callable add: Function that proxies calls to
             `argparse.ArgumentParser.add_argument` prepending options
             with unique plugin name prefix.
 
-        NOTE: if you add argpase arguments such that users setting them can
-        create a config entry that python's bool() would consider false (ie,
-        the use might set the variable to "", [], 0, etc), please ensure that
-        cli.set_by_cli() works for your variable.
-
         """
 
     @classmethod

letsencrypt/renewal.py

@@ -9,10 +9,16 @@ 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 le_util
+from letsencrypt import hooks
 from letsencrypt import storage
 from letsencrypt.plugins import disco as plugins_disco
 
@@ -73,7 +79,7 @@ def _reconstitute(config, full_path):
         _restore_plugin_configs(config, renewalparams)
     except (ValueError, errors.Error) as error:
         logger.warning(
-            "An error occured while parsing %s. The error was %s. "
+            "An error occurred while parsing %s. The error was %s. "
             "Skipping the file.", full_path, error.message)
         logger.debug("Traceback was:\n%s", traceback.format_exc())
         return None
@@ -97,16 +103,14 @@ def _restore_webroot_config(config, renewalparams):
     form.
     """
     if "webroot_map" in renewalparams:
-        # if the user does anything that would create a new webroot map on the
-        # CLI, don't use the old one
-        if not (cli.set_by_cli("webroot_map") or cli.set_by_cli("webroot_path")):
-            setattr(config.namespace, "webroot_map", renewalparams["webroot_map"])
+        if not cli.set_by_cli("webroot_map"):
+            config.namespace.webroot_map = renewalparams["webroot_map"]
     elif "webroot_path" in renewalparams:
         logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path")
         wp = renewalparams["webroot_path"]
         if isinstance(wp, str):  # prior to 0.1.0, webroot_path was a string
             wp = [wp]
-        setattr(config.namespace, "webroot_path", wp)
+        config.namespace.webroot_path = wp
 
 
 def _restore_plugin_configs(config, renewalparams):
@@ -200,6 +204,51 @@ 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."
+    renewal_params = lineage.configuration["renewalparams"]
+    original_server = renewal_params.get("server", cli.flag_default("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(filename, create_empty=True)
+    config = configobj.ConfigObj(o_filename)
     for kind in ALL_FOUR:
         config[kind] = target[kind]
 
-    if relevant_data:
-        config["renewalparams"] = relevant_data
+    if "renewalparams" not in config:
+        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)
-    config.write()
+    logger.debug("Writing new config %s.", n_filename)
+    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

@@ -12,6 +12,7 @@ import unittest
 
 import mock
 import six
+from six.moves import reload_module  # pylint: disable=import-error
 
 from acme import jose
 
@@ -518,11 +519,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:
@@ -962,5 +963,79 @@ class DuplicativeCertsTest(storage_test.BaseRenewableCertTest):
         self.assertEqual(result, (None, None))
 
 
+class DefaultTest(unittest.TestCase):
+    """Tests for letsencrypt.cli._Default."""
+
+    def setUp(self):
+        # pylint: disable=protected-access
+        self.default1 = cli._Default()
+        self.default2 = cli._Default()
+
+    def test_boolean(self):
+        self.assertFalse(self.default1)
+        self.assertFalse(self.default2)
+
+    def test_equality(self):
+        self.assertEqual(self.default1, self.default2)
+
+    def test_hash(self):
+        self.assertEqual(hash(self.default1), hash(self.default2))
+
+
+class SetByCliTest(unittest.TestCase):
+    """Tests for letsencrypt.set_by_cli and related functions."""
+
+    def setUp(self):
+        reload_module(cli)
+
+    def test_webroot_map(self):
+        args = '-w /var/www/html -d example.com'.split()
+        verb = 'renew'
+        self.assertTrue(_call_set_by_cli('webroot_map', args, verb))
+
+    def test_report_config_interaction_str(self):
+        cli.report_config_interaction('manual_public_ip_logging_ok',
+                                      'manual_test_mode')
+        cli.report_config_interaction('manual_test_mode', 'manual')
+
+        self._test_report_config_interaction_common()
+
+    def test_report_config_interaction_iterable(self):
+        cli.report_config_interaction(('manual_public_ip_logging_ok',),
+                                      ('manual_test_mode',))
+        cli.report_config_interaction(('manual_test_mode',), ('manual',))
+
+        self._test_report_config_interaction_common()
+
+    def _test_report_config_interaction_common(self):
+        """Tests implied interaction between manual flags.
+
+        --manual implies --manual-test-mode which implies
+        --manual-public-ip-logging-ok. These interactions don't actually
+        exist in the client, but are used here for testing purposes.
+
+        """
+
+        args = ['--manual']
+        verb = 'renew'
+        for v in ('manual', 'manual_test_mode', 'manual_public_ip_logging_ok'):
+            self.assertTrue(_call_set_by_cli(v, args, verb))
+
+        cli.set_by_cli.detector = None
+
+        args = ['--manual-test-mode']
+        for v in ('manual_test_mode', 'manual_public_ip_logging_ok'):
+            self.assertTrue(_call_set_by_cli(v, args, verb))
+
+        self.assertFalse(_call_set_by_cli('manual', args, verb))
+
+
+def _call_set_by_cli(var, args, verb):
+    with mock.patch('letsencrypt.cli.helpful_parser') as mock_parser:
+        mock_parser.args = args
+        mock_parser.verb = verb
+        return cli.set_by_cli(var)
+
+
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover

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

tests/letstest/scripts/test_renew_standalone.sh

@@ -14,7 +14,7 @@ elif [ -f /etc/redhat-release ] ; then
   echo "Bootstrapping dependencies for RedHat-based OSes..."
   $SUDO bootstrap/_rpm_common.sh
 else
-  echo "Dont have bootstrapping for this OS!"
+  echo "Don't have bootstrapping for this OS!"
   exit 1
 fi