From 2d7f6d7d9290d48b60fd2c823a748f8a7dceb095 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 12 Dec 2016 17:20:52 -0800 Subject: [PATCH 01/44] Ensure apt-cache is always running in English if we're going to grep its output (#3900) --- letsencrypt-auto-source/letsencrypt-auto | 59 +++++++++++++++---- .../pieces/bootstrappers/deb_common.sh | 13 ++-- 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 3d2db3065..d62a642ea 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -15,9 +15,13 @@ set -e # Work even if somebody does "sh thisscript.sh". # Note: you can set XDG_DATA_HOME or VENV_PATH before running this script, # if you want to change where the virtual environment will be installed -XDG_DATA_HOME=${XDG_DATA_HOME:-~/.local/share} +if [ -z "$XDG_DATA_HOME" ]; then + XDG_DATA_HOME="~/.local/share" +fi VENV_NAME="letsencrypt" -VENV_PATH=${VENV_PATH:-"$XDG_DATA_HOME/$VENV_NAME"} +if [ -z "$VENV_PATH" ]; then + VENV_PATH="$XDG_DATA_HOME/$VENV_NAME" +fi VENV_BIN="$VENV_PATH/bin" LE_AUTO_VERSION="0.10.0.dev0" BASENAME=$(basename $0) @@ -80,6 +84,17 @@ if [ $BASENAME = "letsencrypt-auto" ]; then HELP=0 fi +# Support for busybox and others where there is no "command", +# but "which" instead +if command -v command > /dev/null 2>&1 ; then + export EXISTS="command -v" +elif which which > /dev/null 2>&1 ; then + export EXISTS="which" +else + echo "Cannot find command nor which... please install one!" + exit 1 +fi + # certbot-auto needs root access to bootstrap OS dependencies, and # certbot itself needs root access for almost all modes of operation # The "normal" case is that sudo is used for the steps that need root, but @@ -127,7 +142,7 @@ if [ -n "${LE_AUTO_SUDO+x}" ]; then echo "Using preset root authorization mechanism '$LE_AUTO_SUDO'." else if test "`id -u`" -ne "0" ; then - if command -v sudo 1>/dev/null 2>&1; then + if $EXISTS sudo 1>/dev/null 2>&1; then SUDO=sudo SUDO_ENV="CERTBOT_AUTO=$0" else @@ -157,7 +172,7 @@ ExperimentalBootstrap() { DeterminePythonVersion() { for LE_PYTHON in "$LE_PYTHON" python2.7 python27 python2 python; do # Break (while keeping the LE_PYTHON value) if found. - command -v "$LE_PYTHON" > /dev/null && break + $EXISTS "$LE_PYTHON" > /dev/null && break done if [ "$?" != "0" ]; then echo "Cannot find any Pythons; please install one!" @@ -198,19 +213,22 @@ BootstrapDebCommon() { # distro version (#346) virtualenv= - if apt-cache show virtualenv > /dev/null 2>&1 && ! apt-cache --quiet=0 show virtualenv 2>&1 | grep -q 'No packages found'; then - virtualenv="virtualenv" + # virtual env is known to apt and is installable + if apt-cache show virtualenv > /dev/null 2>&1 ; then + if ! LC_ALL=C apt-cache --quiet=0 show virtualenv 2>&1 | grep -q 'No packages found'; then + virtualenv="virtualenv" + fi fi if apt-cache show python-virtualenv > /dev/null 2>&1; then - virtualenv="$virtualenv python-virtualenv" + virtualenv="$virtualenv python-virtualenv" fi augeas_pkg="libaugeas0 augeas-lenses" - AUGVERSION=`apt-cache show --no-all-versions libaugeas0 | grep ^Version: | cut -d" " -f2` + AUGVERSION=`LC_ALL=C apt-cache show --no-all-versions libaugeas0 | grep ^Version: | cut -d" " -f2` if [ "$ASSUME_YES" = 1 ]; then - YES_FLAG="-y" + YES_FLAG="-y" fi AddBackportRepo() { @@ -276,7 +294,7 @@ BootstrapDebCommon() { - if ! command -v virtualenv > /dev/null ; then + if ! $EXISTS virtualenv > /dev/null ; then echo Failed to install a working \"virtualenv\" command, exiting exit 1 fi @@ -960,7 +978,28 @@ UNLIKELY_EOF # Report error. (Otherwise, be quiet.) echo "Had a problem while installing Python packages." if [ "$VERBOSE" != 1 ]; then + echo + echo "pip prints the following errors: " + echo "=====================================================" echo "$PIP_OUT" + echo "=====================================================" + echo + echo "Certbot has problem setting up the virtual environment." + + if `echo $PIP_OUT | grep -q Killed` || `echo $PIP_OUT | grep -q "allocate memory"` ; then + echo + echo "Based on your pip output, the problem can likely be fixed by " + echo "increasing the available memory." + else + echo + echo "We were not be able to guess the right solution from your pip " + echo "output." + fi + + echo + echo "Consult https://certbot.eff.org/docs/install.html#problems-with-python-virtual-environment" + echo "for possible solutions." + echo "You may also find some support resources at https://certbot.eff.org/support/ ." fi rm -rf "$VENV_PATH" exit 1 diff --git a/letsencrypt-auto-source/pieces/bootstrappers/deb_common.sh b/letsencrypt-auto-source/pieces/bootstrappers/deb_common.sh index 747ab8c8d..27919b67b 100644 --- a/letsencrypt-auto-source/pieces/bootstrappers/deb_common.sh +++ b/letsencrypt-auto-source/pieces/bootstrappers/deb_common.sh @@ -23,19 +23,22 @@ BootstrapDebCommon() { # distro version (#346) virtualenv= - if apt-cache show virtualenv > /dev/null 2>&1 && ! apt-cache --quiet=0 show virtualenv 2>&1 | grep -q 'No packages found'; then - virtualenv="virtualenv" + # virtual env is known to apt and is installable + if apt-cache show virtualenv > /dev/null 2>&1 ; then + if ! LC_ALL=C apt-cache --quiet=0 show virtualenv 2>&1 | grep -q 'No packages found'; then + virtualenv="virtualenv" + fi fi if apt-cache show python-virtualenv > /dev/null 2>&1; then - virtualenv="$virtualenv python-virtualenv" + virtualenv="$virtualenv python-virtualenv" fi augeas_pkg="libaugeas0 augeas-lenses" - AUGVERSION=`apt-cache show --no-all-versions libaugeas0 | grep ^Version: | cut -d" " -f2` + AUGVERSION=`LC_ALL=C apt-cache show --no-all-versions libaugeas0 | grep ^Version: | cut -d" " -f2` if [ "$ASSUME_YES" = 1 ]; then - YES_FLAG="-y" + YES_FLAG="-y" fi AddBackportRepo() { From dc81c291b40a1676b5f56bd76e3169514afe98a4 Mon Sep 17 00:00:00 2001 From: Maarten Date: Tue, 13 Dec 2016 22:13:55 +0100 Subject: [PATCH 02/44] Change link of haproxy plugin to new version (#3904) Greenhost has rewritten their HAProxy plugin and it's hosted on a different location. The original URL also points to this new location: https://code.greenhost.net/open/letsencrypt-haproxy --- docs/using.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/using.rst b/docs/using.rst index 7c1fac003..7764408bf 100644 --- a/docs/using.rst +++ b/docs/using.rst @@ -193,7 +193,7 @@ postfix_ N Y STARTTLS Everywhere is becoming a Certbot Postfix/Exim plu =========== ==== ==== =============================================================== .. _plesk: https://github.com/plesk/letsencrypt-plesk -.. _haproxy: https://code.greenhost.net/open/letsencrypt-haproxy +.. _haproxy: https://github.com/greenhost/certbot-haproxy .. _s3front: https://github.com/dlapiduz/letsencrypt-s3front .. _gandi: https://github.com/Gandi/letsencrypt-gandi .. _icecast: https://github.com/e00E/lets-encrypt-icecast From 0464ba2c4b6afed56ecb72286b3cf01a54889baa Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 13 Dec 2016 14:19:47 -0800 Subject: [PATCH 03/44] Implement our fancy new --help output (#3883) * Start reorganising -h output * Fix the --debug flag - Currently exceptions are often caught and burried in log files, even if this flag is provided! * Explain the insanity * Parallalelise nosetests from tox (#3836) * Parallalelise nosetests from tox * Parallelise even more things, break even more things * Now unbreak all the tests that aren't ready for ||ism * Try to pass tests! - Remove non-working hack in reporter_test - also be selective about ||ism in the cover environment * Try again * certbot-apache tests also work, given enough time * Nginx may need more time in Travis's cloud * Unbreak reporter_test under ||ism * More timeout * Working again? * This goes way faster * Another big win * Split a couple more large test suites * A last improvement * More ||ism! * ||ise lint too * Allow nosetests to figure out how many cores to use * simplify merge * Mark the new CLI tests as ||izable * Simplify reporter_test changes * Rationalise ||ism flags * Re-up coverage * Clean up reporter tests * Stop modifying testdata during tests * remove unused os * Improve the "certbot certificates" output (#3846) * Begin making "certbot certificates" future safe * Handle the case where a renewal conf file has no "server" entry * Improvements, tweaks * Capitalise on things * Print the command summary for -h and -h all, but not otherwise Also, update nginx not installed CLI hint * Add a "certificates" help section * Clean up usage string construction * Greatly improve "certbot -h TOPIC" - subcommands now get their own usage headings if they want them - added "certbot -h commands" * A few more cli formatting tests * Auto-populate the verb subgroups from the docs * Show the new help output * Lint, tweak * More lint, and cleanup * Infinite lint * Add rename to command summary; sort "-h commands" output * Use fancy string formatting * More space * Implement --help manage Also, implement a general mechanism for documenting subcommands within topics * Remove one comma * Only create weird parser structures if -h is provided :) * Update sample cli out * Lint * Revert cli-help.txt to previous release version * Grammar & style --- certbot/cli.py | 305 +++++++++++++++++++++++++------------- certbot/main.py | 1 + certbot/tests/cli_test.py | 10 +- 3 files changed, 213 insertions(+), 103 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 356a03764..259953f5e 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -51,51 +51,55 @@ cli_command = LEAUTO if fragment in sys.argv[0] else "certbot" # to replace as much of it as we can... # This is the stub to include in help generated by argparse - SHORT_USAGE = """ - {0} [SUBCOMMAND] [options] [-d domain] [-d domain] ... + {0} [SUBCOMMAND] [options] [-d DOMAIN] [-d DOMAIN] ... Certbot can obtain and install HTTPS/TLS/SSL certificates. By default, it will attempt to use a webserver both for obtaining and installing the -cert. Major SUBCOMMANDS are: +cert. """.format(cli_command) - (default) run Obtain & install a cert in your current webserver - certonly Obtain cert, but do not install it (aka "auth") - install Install a previously obtained cert in a server - renew Renew previously obtained certs that are near expiry - revoke Revoke a previously obtained certificate - register Perform tasks related to registering with the CA - rollback Rollback server configuration changes made during install - config_changes Show changes made to server config during installation - update_symlinks Update cert symlinks based on renewal config file - rename Update a certificate's name - plugins Display information about installed plugins - certificates Display information about certs configured with Certbot +# This section is used for --help and --help all ; it needs information +# about installed plugins to be fully formatted +COMMAND_OVERVIEW = """The most common SUBCOMMANDS and flags are: -""".format(cli_command) - -# This is the short help for certbot --help, where we disable argparse -# altogether -USAGE = SHORT_USAGE + """Choice of server plugins for obtaining and installing cert: +obtain, install, and renew certificates: + (default) run Obtain & install a cert in your current webserver + certonly Obtain or renew a cert, but do not install it + renew Renew all previously obtained certs that are near expiry + -d DOMAINS Comma-separated list of domains to obtain a cert for %s --standalone Run a standalone webserver for authentication %s --webroot Place files in a server's webroot folder for authentication - --script User provided shell scripts for authentication + --manual Obtain certs interactively, or using shell script hoooks -OR use different plugins to obtain (authenticate) the cert and then install it: + -n Run non-interactively + --test-cert Obtain a test cert from a staging server + --dry-run Test "renew" or "certonly" without saving any certs to disk - --authenticator standalone --installer apache +manage certificates: + certificates Display information about certs you have from Certbot + revoke Revoke a certificate (supply --cert-path) + rename Rename a certificate +manage your account with Let's Encrypt: + register Create a Let's Encrypt ACME account + --agree-tos Agree to the ACME server's Subscriber Agreement + -m EMAIL Email address for important account notifications +""" + +# This is the short help for certbot --help, where we disable argparse +# altogether +HELP_USAGE = """ More detailed help: - -h, --help [topic] print this message, or detailed help on a topic; - the available topics are: + -h, --help [TOPIC] print this message, or detailed help on a topic; + the available TOPICS are: - all, automation, paths, security, testing, or any of the subcommands or - plugins (certonly, renew, install, register, nginx, apache, standalone, - webroot, script, etc.) + all, automation, commands, paths, security, testing, or any of the + subcommands or plugins (certonly, renew, install, register, nginx, + apache, standalone, webroot, script, etc.) """ @@ -141,19 +145,6 @@ def report_config_interaction(modified, modifiers): 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: - nginx_doc = "--nginx Use the Nginx plugin for authentication & installation" - else: - nginx_doc = "(nginx support is experimental, buggy, and not installed by default)" - if "apache" in plugins: - apache_doc = "--apache Use the Apache plugin for authentication & installation" - else: - apache_doc = "(the apache plugin is not installed)" - return USAGE % (apache_doc, nginx_doc), SHORT_USAGE - - def possible_deprecation_warning(config): "A deprecation warning for users with the old, not-self-upgrading letsencrypt-auto." if cli_command != LEAUTO: @@ -309,6 +300,82 @@ class HelpfulArgumentGroup(object): """Add a new command line argument to the argument group.""" self._parser.add(self._topic, *args, **kwargs) +# The attributes here are: +# short: a string that will be displayed by "certbot -h commands" +# opts: a string that heads the section of flags with which this command is documented, +# both for "cerbot -h SUBCOMMAND" and "certbot -h all" +# usage: an optional string that overrides the header of "certbot -h SUBCOMMAND" +VERB_HELP = [ + ("run (default)", { + "short": "Obtain/renew a certificate, and install it", + "opts": "Options for obtaining & installing certs", + "usage": SHORT_USAGE.replace("[SUBCOMMAND]", ""), + "realname": "run" + }), + ("certonly", { + "short": "Obtain or renew a certificate, but do not install it", + "opts": "Options for modifying how a cert is obtained", + "usage": ("\n\n certbot certonly [options] [-d DOMAIN] [-d DOMAIN] ...\n\n" + "This command obtains a TLS/SSL certificate without installing it anywhere.") + }), + ("renew", { + "short": "Renew all certificates (or one specifed with --cert-name)", + "opts": ("The 'renew' subcommand will attempt to renew all" + " certificates (or more precisely, certificate lineages) you have" + " previously obtained if they are close to expiry, and print a" + " summary of the results. By default, 'renew' will reuse the options" + " 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. Hooks are available to run commands" + " before and after renewal; see" + " https://certbot.eff.org/docs/using.html#renewal for more" + " information on these."), + "usage": "\n\n certbot renew [--cert-name NAME] [options]\n\n" + }), + ("certificates", { + "short": "List all certificates managed by Certbot", + "opts": "List all certificates managed by Certbot" + }), + ("revoke", { + "short": "Revoke a certificate specified with --cert-path", + "opts": "Options for revocation of certs" + }), + ("rename", { + "short": "Change a certificate's name (for management purposes)", + "opts": "Options changing certificate names" + }), + ("register", { + "short": "Register for account with Let's Encrypt / other ACME server", + "opts": "Options for account registration & modification" + }), + ("install", { + "short": "Install an arbitrary cert in a server", + "opts": "Options for modifying how a cert is deployed" + }), + ("config_changes", { + "short": "Show changes that Certbot has made to server configurations", + "opts": "Options for controlling which changes are displayed" + }), + ("rollback", { + "short": "Roll back server conf changes made during cert installation", + "opts": "Options for rolling back server configuration changes" + }), + ("plugins", { + "short": "List plugins that are installed and available on your system", + "opts": 'Options for for the "plugins" subcommand' + }), + ("update_symlinks", { + "short": "Recreate symlinks in your /live/ directory", + "opts": ("Recreates cert and key symlinks in {0}, if you changed them by hand " + "or edited a renewal configuration file".format( + os.path.join(flag_default("config_dir"), "live"))) + }), + +] +# VERB_HELP is a list in order to preserve order, but a dict is sometimes useful +VERB_HELP_MAP = dict(VERB_HELP) + class HelpfulArgumentParser(object): """Argparse Wrapper. @@ -319,6 +386,7 @@ class HelpfulArgumentParser(object): """ + def __init__(self, args, plugins, detect_defaults=False): from certbot import main self.VERBS = {"auth": main.obtain_cert, "certonly": main.obtain_cert, @@ -331,22 +399,12 @@ class HelpfulArgumentParser(object): # List of topics for which additional help can be provided HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + list(self.VERBS) + HELP_TOPICS += self.COMMANDS_TOPICS + ["manage"] plugin_names = list(plugins) self.help_topics = HELP_TOPICS + plugin_names + [None] - usage, short_usage = usage_strings(plugins) - self.parser = configargparse.ArgParser( - prog="certbot", - usage=short_usage, - formatter_class=argparse.ArgumentDefaultsHelpFormatter, - args_for_setting_config_path=["-c", "--config"], - default_config_files=flag_default("config_files")) - - # 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.detect_defaults = detect_defaults - self.args = args self.determine_verb() help1 = self.prescan_for_flag("-h", self.help_topics) @@ -355,13 +413,72 @@ class HelpfulArgumentParser(object): self.help_arg = help1 or help2 else: self.help_arg = help1 if isinstance(help1, str) else help2 - if self.help_arg is True: - # just --help with no topic; avoid argparse altogether - print(usage) - sys.exit(0) + + short_usage = self._usage_string(plugins, self.help_arg) + self.visible_topics = self.determine_help_topics(self.help_arg) self.groups = {} # elements are added by .add_group() - self.defaults = {} # elements are added by .parse_args() + self.defaults = {} # elements are added by .parse_args() + + self.parser = configargparse.ArgParser( + prog="certbot", + usage=short_usage, + formatter_class=argparse.ArgumentDefaultsHelpFormatter, + args_for_setting_config_path=["-c", "--config"], + default_config_files=flag_default("config_files"), + config_arg_help_message="path to config file (default: {0})".format( + " and ".join(flag_default("config_files")))) + + # This is the only way to turn off overly verbose config flag documentation + self.parser._add_config_file_help = False # pylint: disable=protected-access + + # Help that are synonyms for --help subcommands + COMMANDS_TOPICS = ["command", "commands", "subcommand", "subcommands", "verbs"] + def _list_subcommands(self): + longest = max(len(v) for v in VERB_HELP_MAP.keys()) + + text = "The full list of available SUBCOMMANDS is:\n\n" + for verb, props in sorted(VERB_HELP): + doc = props.get("short", "") + text += '{0:<{length}} {1}\n'.format(verb, doc, length=longest) + + text += "\nYou can get more help on a specific subcommand with --help SUBCOMMAND\n" + return text + + def _usage_string(self, plugins, help_arg): + """Make usage strings late so that plugins can be initialised late + + :param plugins: all discovered plugins + :param help_arg: False for none; True for --help; "TOPIC" for --help TOPIC + :rtype: str + :returns: a short usage string for the top of --help TOPIC) + """ + if "nginx" in plugins: + nginx_doc = "--nginx Use the Nginx plugin for authentication & installation" + else: + nginx_doc = "(the certbot nginx plugin is not installed)" + if "apache" in plugins: + apache_doc = "--apache Use the Apache plugin for authentication & installation" + else: + apache_doc = "(the cerbot apache plugin is not installed)" + + usage = SHORT_USAGE + if help_arg == True: + print(usage + COMMAND_OVERVIEW % (apache_doc, nginx_doc) + HELP_USAGE) + sys.exit(0) + elif help_arg in self.COMMANDS_TOPICS: + print(usage + self._list_subcommands()) + sys.exit(0) + elif help_arg == "all": + # if we're doing --help all, the OVERVIEW is part of the SHORT_USAGE at + # the top; if we're doing --help someothertopic, it's OT so it's not + usage += COMMAND_OVERVIEW % (apache_doc, nginx_doc) + else: + custom = VERB_HELP_MAP.get(help_arg, {}).get("usage", None) + usage = custom if custom else usage + + return usage + def parse_args(self): """Parses command line arguments and returns the result. @@ -566,7 +683,7 @@ class HelpfulArgumentParser(object): util.add_deprecated_argument( self.parser.add_argument, argument_name, num_args) - def add_group(self, topic, **kwargs): + def add_group(self, topic, verbs=(), **kwargs): """Create a new argument group. This method must be called once for every topic, however, calls @@ -574,6 +691,8 @@ class HelpfulArgumentParser(object): clarity. :param str topic: Name of the new argument group. + :param str verbs: List of subcommands that should be documented as part of + this help group / topic :returns: The new argument group. :rtype: `HelpfulArgumentGroup` @@ -581,6 +700,9 @@ class HelpfulArgumentParser(object): """ if self.visible_topics[topic]: self.groups[topic] = self.parser.add_argument_group(topic, **kwargs) + if self.help_arg: + for v in verbs: + self.groups[topic].add_argument(v, help=VERB_HELP_MAP[v]["short"]) return HelpfulArgumentGroup(self, topic) @@ -621,32 +743,17 @@ class HelpfulArgumentParser(object): def _add_all_groups(helpful): helpful.add_group("automation", description="Arguments for automating execution & other tweaks") helpful.add_group("security", description="Security parameters & server settings") - helpful.add_group( - "testing", description="The following flags are meant for " - "testing purposes only! Do NOT change them, unless you " - "really know what you're doing!") - # VERBS - helpful.add_group( - "renew", description="The 'renew' subcommand will attempt to renew all" - " certificates (or more precisely, certificate lineages) you have" - " previously obtained if they are close to expiry, and print a" - " summary of the results. By default, 'renew' will reuse the options" - " 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. Hooks are available to run commands" - " before and after renewal; see" - " https://certbot.eff.org/docs/using.html#renewal for more" - " information on these.") - - helpful.add_group("certonly", description="Options for modifying how a cert is obtained") - helpful.add_group("install", description="Options for modifying how a cert is deployed") - helpful.add_group("revoke", description="Options for revocation of certs") - helpful.add_group("rollback", description="Options for reverting config changes") - helpful.add_group("plugins", description='Options for the "plugins" subcommand') - helpful.add_group("config_changes", - description="Options for showing a history of config changes") + helpful.add_group("testing", + description="The following flags are meant for testing and integration purposes only.") helpful.add_group("paths", description="Arguments changing execution paths & servers") + helpful.add_group("manage", + description="Various subcommands and flags are available for managing your certificates:", + verbs=["certificates", "renew", "revoke", "rename"]) + + # VERBS + for verb, docs in VERB_HELP: + name = docs.get("realname", verb) + helpful.add_group(name, description=docs["opts"]) def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: disable=too-many-statements @@ -675,7 +782,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis None, "-t", "--text", dest="text_mode", action="store_true", help=argparse.SUPPRESS) helpful.add( - [None, "automation"], "-n", "--non-interactive", "--noninteractive", + [None, "automation", "run", "certonly"], "-n", "--non-interactive", "--noninteractive", dest="noninteractive_mode", action="store_true", help="Run without ever asking for user input. This may require " "additional command line flags; the client will try to explain " @@ -688,15 +795,15 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "multiple -d flags or enter a comma separated list of domains " "as a parameter.") helpful.add( - [None, "run", "certonly"], + [None, "run", "certonly", "manage"], "--cert-name", dest="certname", metavar="CERTNAME", default=None, help="Certificate name to apply. Only one certificate name can be used " - "per Certbot run. To see certificate names, run 'certbot certificates'." + "per Certbot run. To see certificate names, run 'certbot certificates'. " "If there is no existing certificate with this name and " "domains are requested, create a new certificate with this name.") helpful.add( - "rename", + ["rename", "manage"], "--updated-cert-name", dest="new_certname", metavar="NEW_CERTNAME", default=None, help="New name for the certificate. Must be a valid filename.") @@ -728,9 +835,9 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="With the register verb, indicates that details associated " "with an existing registration, such as the e-mail address, " "should be updated, rather than registering a new account.") - helpful.add(None, "-m", "--email", help=config_help("email")) + helpful.add(["register", "automation"], "-m", "--email", help=config_help("email")) helpful.add( - ["automation", "renew", "certonly", "run"], + ["automation", "certonly", "run"], "--keep-until-expiring", "--keep", "--reinstall", dest="reinstall", action="store_true", help="If the requested cert matches an existing cert, always keep the " @@ -784,7 +891,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="(certbot-auto only) prevent the certbot-auto script from" " upgrading itself to newer released versions") helpful.add( - ["automation", "renew", "certonly"], + ["automation", "renew", "certonly", "run"], "-q", "--quiet", dest="quiet", action="store_true", help="Silence all output except errors. Useful for automation via cron." " Implies --non-interactive.") @@ -801,11 +908,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help=config_help("no_verify_ssl"), default=flag_default("no_verify_ssl")) helpful.add( - ["certonly", "renew", "run"], "--tls-sni-01-port", type=int, + ["testing", "standalone", "apache", "nginx"], "--tls-sni-01-port", type=int, default=flag_default("tls_sni_01_port"), help=config_help("tls_sni_01_port")) helpful.add( - ["certonly", "renew", "run", "manual"], "--http-01-port", type=int, + ["testing", "standalone", "manual"], "--http-01-port", type=int, dest="http01_port", default=flag_default("http01_port"), help=config_help("http01_port")) helpful.add( @@ -859,7 +966,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="Require that all configuration files are owned by the current " "user; only needed if your config is somewhere unsafe like /tmp/") helpful.add( - ["manual", "standalone", "certonly", "renew", "run"], + ["manual", "standalone", "certonly", "renew"], "--preferred-challenges", dest="pref_challs", action=_PrefChallAction, default=[], help='A sorted, comma delimited list of the preferred challenge to ' @@ -950,10 +1057,8 @@ def _paths_parser(helpful): if verb == "help": verb = helpful.help_arg - cph = "Path to where cert is saved (with auth --csr), installed from or revoked." - section = "paths" - if verb in ("install", "revoke", "certonly"): - section = verb + cph = "Path to where cert is saved (with auth --csr), installed from, or revoked." + section = ["paths", "install", "revoke", "certonly", "manage"] if verb == "certonly": add(section, "--cert-path", type=os.path.abspath, default=flag_default("auth_cert_path"), help=cph) @@ -975,7 +1080,7 @@ def _paths_parser(helpful): default_cp = None if verb == "certonly": default_cp = flag_default("auth_chain_path") - add("paths", "--fullchain-path", default=default_cp, type=os.path.abspath, + add(["install", "paths"], "--fullchain-path", default=default_cp, type=os.path.abspath, help="Accompanying path to a full certificate chain (cert plus chain).") add("paths", "--chain-path", default=default_cp, type=os.path.abspath, help="Accompanying path to a certificate chain.") @@ -1006,10 +1111,10 @@ def _plugins_parsing(helpful, plugins): "plugins", "--configurator", help="Name of the plugin that is " "both an authenticator and an installer. Should not be used " "together with --authenticator or --installer.") - helpful.add(["plugins", "certonly", "run", "install"], + helpful.add(["plugins", "certonly", "run", "install", "config_changes"], "--apache", action="store_true", help="Obtain and install certs using Apache") - helpful.add(["plugins", "certonly", "run", "install"], + helpful.add(["plugins", "certonly", "run", "install", "config_changes"], "--nginx", action="store_true", help="Obtain and install certs using Nginx") helpful.add(["plugins", "certonly"], "--standalone", action="store_true", diff --git a/certbot/main.py b/certbot/main.py index 2baab9670..24f38172a 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -712,6 +712,7 @@ def _handle_exception(exc_type, exc_value, trace, config): with open(logfile, "w") as logfd: traceback.print_exception( exc_type, exc_value, trace, file=logfd) + assert "--debug" not in sys.argv # config is None if this explodes except: # pylint: disable=bare-except sys.exit(tb_str) if "--debug" in sys.argv: diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 72aea50ea..2755d992c 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -84,6 +84,8 @@ class ParseTest(unittest.TestCase): self.assertTrue("--manual-test-mode" in out) self.assertTrue("--text" not in out) self.assertTrue("--dialog" not in out) + self.assertTrue("%s" not in out) + self.assertTrue("{0}" not in out) out = self._help_output(['-h', 'nginx']) if "nginx" in self.plugins: @@ -97,7 +99,7 @@ class ParseTest(unittest.TestCase): if "nginx" in self.plugins: self.assertTrue("Use the Nginx plugin" in out) else: - self.assertTrue("(nginx support is experimental" in out) + self.assertTrue("(the certbot nginx plugin is not" in out) out = self._help_output(['--help', 'plugins']) self.assertTrue("--manual-test-mode" not in out) @@ -125,8 +127,10 @@ class ParseTest(unittest.TestCase): self.assertTrue("--key-path" not in out) out = self._help_output(['-h']) - - self.assertTrue(cli.usage_strings(self.plugins)[0] in out) + self.assertTrue(cli.SHORT_USAGE in out) + self.assertTrue(cli.COMMAND_OVERVIEW[:100] in out) + self.assertTrue("%s" not in out) + self.assertTrue("{0}" not in out) def test_parse_domains(self): short_args = ['-d', 'example.com'] From ad53c80c1e95a81d5878b4d7ac0095c3b2046451 Mon Sep 17 00:00:00 2001 From: Clif Houck Date: Tue, 13 Dec 2016 16:38:57 -0600 Subject: [PATCH 04/44] Fix certbox-nginx address equality check (#3886) 0.0.0.0, *, and '' are equivalent hosts to nginx. Changes Addr object's equality testing to treat them as equal. Fixes #3855 --- certbot-nginx/certbot_nginx/obj.py | 11 +++++++++++ certbot-nginx/certbot_nginx/tests/obj_test.py | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/certbot-nginx/certbot_nginx/obj.py b/certbot-nginx/certbot_nginx/obj.py index 98bf86f5c..29fa976f3 100644 --- a/certbot-nginx/certbot_nginx/obj.py +++ b/certbot-nginx/certbot_nginx/obj.py @@ -29,10 +29,14 @@ class Addr(common.Addr): :param bool default: Whether the directive includes 'default_server' """ + UNSPECIFIED_IPV4_ADDRESSES = ('', '*', '0.0.0.0') + CANONICAL_UNSPECIFIED_ADDRESS = UNSPECIFIED_IPV4_ADDRESSES[0] + def __init__(self, host, port, ssl, default): super(Addr, self).__init__((host, port)) self.ssl = ssl self.default = default + self.unspecified_address = host in self.UNSPECIFIED_IPV4_ADDRESSES @classmethod def fromstring(cls, str_addr): @@ -96,6 +100,13 @@ class Addr(common.Addr): def super_eq(self, other): """Check ip/port equality, with IPv6 support. """ + # If both addresses got an unspecified address, then make sure the + # host representation in each match when doing the comparison. + if self.unspecified_address and other.unspecified_address: + return common.Addr((self.CANONICAL_UNSPECIFIED_ADDRESS, + self.tup[1]), self.ipv6) == \ + common.Addr((other.CANONICAL_UNSPECIFIED_ADDRESS, + other.tup[1]), other.ipv6) # Nginx plugin currently doesn't support IPv6 but this will # future-proof it return super(Addr, self).__eq__(other) diff --git a/certbot-nginx/certbot_nginx/tests/obj_test.py b/certbot-nginx/certbot_nginx/tests/obj_test.py index b153db8d4..b0a2d5ad8 100644 --- a/certbot-nginx/certbot_nginx/tests/obj_test.py +++ b/certbot-nginx/certbot_nginx/tests/obj_test.py @@ -1,5 +1,6 @@ """Test the helper objects in certbot_nginx.obj.""" import unittest +import itertools class AddrTest(unittest.TestCase): @@ -72,6 +73,24 @@ class AddrTest(unittest.TestCase): self.assertNotEqual(self.addr1, self.addr2) self.assertFalse(self.addr1 == 3333) + def test_equivalent_any_addresses(self): + from certbot_nginx.obj import Addr + any_addresses = ("0.0.0.0:80 default_server ssl", + "80 default_server ssl", + "*:80 default_server ssl") + for first, second in itertools.combinations(any_addresses, 2): + self.assertEqual(Addr.fromstring(first), Addr.fromstring(second)) + + # Also, make sure ports are checked. + self.assertNotEqual(Addr.fromstring(any_addresses[0]), + Addr.fromstring("0.0.0.0:443 default_server ssl")) + + # And they aren't equivalent to a specified address. + for any_address in any_addresses: + self.assertNotEqual( + Addr.fromstring("192.168.1.2:80 default_server ssl"), + Addr.fromstring(any_address)) + def test_set_inclusion(self): from certbot_nginx.obj import Addr set_a = set([self.addr1, self.addr2]) From 107851ee9b5d13e42e8e76e39ffcced418a7b0e1 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 13 Dec 2016 17:32:46 -0800 Subject: [PATCH 05/44] Document defaults (#3863) * Begin fixing incorrect defaults * Fix more defaults * Make more defaults correct * Update cli-help.txt (To show what this PR does) * Lint * Extend argparse rather than vendoring it * lint * Move sample User Agent generation into the same module as UA generation * Revert cli-help.txt to previous release version * Slightly more consistent linebreaks --- certbot/cli.py | 68 +++++++++++++++++++++++--------------- certbot/client.py | 14 ++++++-- certbot/interfaces.py | 2 +- certbot/plugins/manual.py | 2 +- certbot/plugins/webroot.py | 2 +- 5 files changed, 56 insertions(+), 32 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 259953f5e..ef6257304 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -300,6 +300,21 @@ class HelpfulArgumentGroup(object): """Add a new command line argument to the argument group.""" self._parser.add(self._topic, *args, **kwargs) +class CustomHelpFormatter(argparse.HelpFormatter): + """This is a clone of ArgumentDefaultsHelpFormatter, with bugfixes. + + In particular we fix https://bugs.python.org/issue28742 + """ + + def _get_help_string(self, action): + helpstr = action.help + if '%(default)' not in action.help and '(default:' not in action.help: + if action.default != argparse.SUPPRESS: + defaulting_nargs = [argparse.OPTIONAL, argparse.ZERO_OR_MORE] + if action.option_strings or action.nargs in defaulting_nargs: + helpstr += ' (default: %(default)s)' + return helpstr + # The attributes here are: # short: a string that will be displayed by "certbot -h commands" # opts: a string that heads the section of flags with which this command is documented, @@ -423,7 +438,7 @@ class HelpfulArgumentParser(object): self.parser = configargparse.ArgParser( prog="certbot", usage=short_usage, - formatter_class=argparse.ArgumentDefaultsHelpFormatter, + formatter_class=CustomHelpFormatter, args_for_setting_config_path=["-c", "--config"], default_config_files=flag_default("config_files"), config_arg_help_message="path to config file (default: {0})".format( @@ -793,7 +808,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis metavar="DOMAIN", action=_DomainsAction, default=[], help="Domain names to apply. For multiple domains you can use " "multiple -d flags or enter a comma separated list of domains " - "as a parameter.") + "as a parameter. (default: Ask)") helpful.add( [None, "run", "certonly", "manage"], "--cert-name", dest="certname", @@ -842,11 +857,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis dest="reinstall", action="store_true", help="If the requested cert matches an existing cert, always keep the " "existing one until it is due for renewal (for the " - "'run' subcommand this means reinstall the existing cert)") + "'run' subcommand this means reinstall the existing cert). (default: Ask)") helpful.add( "automation", "--expand", action="store_true", help="If an existing cert covers some subset of the requested names, " - "always expand and replace it with the additional names.") + "always expand and replace it with the additional names. (default: Ask)") helpful.add( "automation", "--version", action="version", version="%(prog)s {0}".format(certbot.__version__), @@ -875,7 +890,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "at this system. This option cannot be used with --csr.") helpful.add( "automation", "--agree-tos", dest="tos", action="store_true", - help="Agree to the ACME Subscriber Agreement") + help="Agree to the ACME Subscriber Agreement (default: Ask)") helpful.add( "automation", "--account", metavar="ACCOUNT_ID", help="Account ID to use") @@ -889,7 +904,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis helpful.add( "automation", "--no-self-upgrade", action="store_true", help="(certbot-auto only) prevent the certbot-auto script from" - " upgrading itself to newer released versions") + " upgrading itself to newer released versions (default: Upgrade" + " automatically)") helpful.add( ["automation", "renew", "certonly", "run"], "-q", "--quiet", dest="quiet", action="store_true", @@ -928,11 +944,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis helpful.add( "security", "--redirect", action="store_true", help="Automatically redirect all HTTP traffic to HTTPS for the newly " - "authenticated vhost.", dest="redirect", default=None) + "authenticated vhost. (default: Ask)", dest="redirect", default=None) helpful.add( "security", "--no-redirect", action="store_false", help="Do not automatically redirect all HTTP traffic to HTTPS for the newly " - "authenticated vhost.", dest="redirect", default=None) + "authenticated vhost. (default: Ask)", dest="redirect", default=None) helpful.add( "security", "--hsts", action="store_true", help="Add the Strict-Transport-Security header to every HTTP response." @@ -940,8 +956,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " Defends against SSL Stripping.", dest="hsts", default=False) helpful.add( "security", "--no-hsts", action="store_false", - help="Do not automatically add the Strict-Transport-Security header" - " to every HTTP response.", dest="hsts", default=False) + help=argparse.SUPPRESS, dest="hsts", default=False) helpful.add( "security", "--uir", action="store_true", help="Add the \"Content-Security-Policy: upgrade-insecure-requests\"" @@ -949,9 +964,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " https:// for every http:// resource.", dest="uir", default=None) helpful.add( "security", "--no-uir", action="store_false", - help="Do not automatically set the \"Content-Security-Policy:" - " upgrade-insecure-requests\" header to every HTTP response.", - dest="uir", default=None) + help=argparse.SUPPRESS, dest="uir", default=None) helpful.add( "security", "--staple-ocsp", action="store_true", help="Enables OCSP Stapling. A valid OCSP response is stapled to" @@ -959,8 +972,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis dest="staple", default=None) helpful.add( "security", "--no-staple-ocsp", action="store_false", - help="Do not automatically enable OCSP Stapling.", - dest="staple", default=None) + help=argparse.SUPPRESS, dest="staple", default=None) helpful.add( "security", "--strict-permissions", action="store_true", help="Require that all configuration files are owned by the current " @@ -1005,7 +1017,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " see if the programs being run are in the $PATH, so that mistakes can" " be caught early, even when the hooks aren't being run just yet. The" " validation is rather simplistic and fails if you use more advanced" - " shell constructs, so you can use this switch to disable it.") + " shell constructs, so you can use this switch to disable it." + " (default: False)") helpful.add_deprecated_argument("--agree-dev-preview", 0) helpful.add_deprecated_argument("--dialog", 0) @@ -1025,12 +1038,15 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis def _create_subparsers(helpful): helpful.add("config_changes", "--num", type=int, help="How many past revisions you want to be displayed") + + from certbot.client import sample_user_agent # avoid import loops helpful.add( None, "--user-agent", default=None, help="Set a custom user agent string for the client. User agent strings allow " "the CA to collect high level statistics about success rates by OS and " "plugin. If you wish to hide your server OS version from the Let's " - 'Encrypt server, set this to "".') + 'Encrypt server, set this to "". ' + '(default: {0})'.format(sample_user_agent())) helpful.add("certonly", "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER or PEM format." @@ -1103,20 +1119,18 @@ def _plugins_parsing(helpful, plugins): "a particular plugin by setting options provided below. Running " "--help will list flags specific to that plugin.") - helpful.add( - "plugins", "-a", "--authenticator", help="Authenticator plugin name.") - helpful.add( - "plugins", "-i", "--installer", help="Installer plugin name (also used to find domains).") - helpful.add( - "plugins", "--configurator", help="Name of the plugin that is " - "both an authenticator and an installer. Should not be used " - "together with --authenticator or --installer.") + helpful.add("plugins", "--configurator", + help="Name of the plugin that is both an authenticator and an installer." + " Should not be used together with --authenticator or --installer. " + "(default: Ask)") + helpful.add("plugins", "-a", "--authenticator", help="Authenticator plugin name.") + helpful.add("plugins", "-i", "--installer", + help="Installer plugin name (also used to find domains).") helpful.add(["plugins", "certonly", "run", "install", "config_changes"], "--apache", action="store_true", help="Obtain and install certs using Apache") helpful.add(["plugins", "certonly", "run", "install", "config_changes"], - "--nginx", action="store_true", - help="Obtain and install certs using Nginx") + "--nginx", action="store_true", help="Obtain and install certs using Nginx") helpful.add(["plugins", "certonly"], "--standalone", action="store_true", help='Obtain certs using a "standalone" webserver.') helpful.add(["plugins", "certonly"], "--script", action="store_true", diff --git a/certbot/client.py b/certbot/client.py index d58f9457f..6ff14bc56 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -38,11 +38,11 @@ def acme_from_config_key(config, key): "Wrangle ACME client construction" # TODO: Allow for other alg types besides RS256 net = acme_client.ClientNetwork(key, verify_ssl=(not config.no_verify_ssl), - user_agent=_determine_user_agent(config)) + user_agent=determine_user_agent(config)) return acme_client.Client(config.server, key=key, net=net) -def _determine_user_agent(config): +def determine_user_agent(config): """ Set a user_agent string in the config based on the choice of plugins. (this wasn't knowable at construction time) @@ -59,6 +59,16 @@ def _determine_user_agent(config): ua = config.user_agent return ua +def sample_user_agent(): + "Document what this Certbot's user agent string will be like." + class DummyConfig(object): + "Shim for computing a sample user agent." + def __init__(self): + self.authenticator = "XXX" + self.installer = "YYY" + self.user_agent = None + return determine_user_agent(DummyConfig()) + def register(config, account_storage, tos_cb=None): """Register new account with an ACME CA. diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 8e7d887f0..6388bf936 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -201,7 +201,7 @@ class IConfig(zope.interface.Interface): """ server = zope.interface.Attribute("ACME Directory Resource URI.") email = zope.interface.Attribute( - "Email used for registration and recovery contact.") + "Email used for registration and recovery contact. (default: Ask)") rsa_key_size = zope.interface.Attribute("Size of the RSA key.") must_staple = zope.interface.Attribute( "Adds the OCSP Must Staple extension to the certificate. " diff --git a/certbot/plugins/manual.py b/certbot/plugins/manual.py index c124ce048..5f933f8bc 100644 --- a/certbot/plugins/manual.py +++ b/certbot/plugins/manual.py @@ -98,7 +98,7 @@ s.serve_forever()" """ add("test-mode", action="store_true", help="Test mode. Executes the manual command in subprocess.") add("public-ip-logging-ok", action="store_true", - help="Automatically allows public IP logging.") + help="Automatically allows public IP logging. (default: Ask)") def prepare(self): # pylint: disable=missing-docstring,no-self-use if self.config.noninteractive_mode and not self.conf("test-mode"): diff --git a/certbot/plugins/webroot.py b/certbot/plugins/webroot.py index 2c449fdca..e9c3bcdda 100644 --- a/certbot/plugins/webroot.py +++ b/certbot/plugins/webroot.py @@ -45,7 +45,7 @@ to serve all files under specified web root ({0}).""" "times to handle different domains; each domain will have " "the webroot path that preceded it. For instance: `-w " "/var/www/example -d example.com -d www.example.com -w " - "/var/www/thing -d thing.net -d m.thing.net`") + "/var/www/thing -d thing.net -d m.thing.net` (default: Ask)") add("map", default={}, action=_WebrootMapAction, help="JSON dictionary mapping domains to webroot paths; this " "implies -d for each entry. You may need to escape this from " From 27525fb205b70db40b1f8f1264028e20d56b7dfb Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Thu, 15 Dec 2016 11:00:07 -0800 Subject: [PATCH 06/44] Use relative paths for livedir symlinks (#3914) * Use relative paths for livedir symlinks * switch directory back for the rest of the tests --- certbot/storage.py | 16 ++++++++++++++-- certbot/tests/cert_manager_test.py | 14 ++++++++++---- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/certbot/storage.py b/certbot/storage.py index 61ab69ff7..671922bee 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -198,6 +198,10 @@ def lineagename_for_filename(config_filename): "renewal config file name must end in .conf") return os.path.basename(config_filename[:-len(".conf")]) +def _relpath_from_file(archive_dir, from_file): + """Path to a directory from a file""" + return os.path.relpath(archive_dir, os.path.dirname(from_file)) + class RenewableCert(object): # pylint: disable=too-many-instance-attributes,too-many-public-methods @@ -305,6 +309,13 @@ class RenewableCert(object): return os.path.join( self.cli_config.default_archive_dir, self.lineagename) + def relative_archive_dir(self, from_file): + """Returns the default or specified archive directory as a relative path + + Used for creating symbolic links. + """ + return _relpath_from_file(self.archive_dir, from_file) + @property def is_test_cert(self): """Returns true if this is a test cert from a staging server.""" @@ -331,7 +342,8 @@ class RenewableCert(object): for kind in ALL_FOUR: link = getattr(self, kind) previous_link = get_link_target(link) - new_link = os.path.join(self.archive_dir, os.path.basename(previous_link)) + new_link = os.path.join(self.relative_archive_dir(link), + os.path.basename(previous_link)) os.unlink(link) os.symlink(new_link, link) @@ -856,7 +868,7 @@ class RenewableCert(object): target = dict([(kind, os.path.join(live_dir, kind + ".pem")) for kind in ALL_FOUR]) for kind in ALL_FOUR: - os.symlink(os.path.join(archive, kind + "1.pem"), + os.symlink(os.path.join(_relpath_from_file(archive, target[kind]), kind + "1.pem"), target[kind]) with open(target["cert"], "wb") as f: logger.debug("Writing certificate to %s.", target["cert"]) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index f3569dc00..bffa5298f 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -99,10 +99,16 @@ class UpdateLiveSymlinksTest(BaseCertManagerTest): cert_manager.update_live_symlinks(self.cli_config) # check that symlinks go where they should - for domain in self.domains: - for kind in ALL_FOUR: - self.assertEqual(os.readlink(self.configs[domain][kind]), - archive_paths[domain][kind]) + prev_dir = os.getcwd() + try: + for domain in self.domains: + for kind in ALL_FOUR: + os.chdir(os.path.dirname(self.configs[domain][kind])) + self.assertEqual( + os.path.realpath(os.readlink(self.configs[domain][kind])), + os.path.realpath(archive_paths[domain][kind])) + finally: + os.chdir(prev_dir) class CertificatesTest(BaseCertManagerTest): From 16361bfd0628de97d83f91733b9db3ce1bea50e7 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Thu, 15 Dec 2016 19:41:42 -0800 Subject: [PATCH 07/44] test using os.path.sep not hardcoded / (#3920) --- certbot/tests/cert_manager_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index bffa5298f..e04e25da8 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -383,7 +383,7 @@ class RenameLineageTest(BaseCertManagerTest): mock_config.new_certname = "example.org" self.assertRaises(errors.ConfigurationError, self._call, mock_config) - mock_config.new_certname = "one/two" + mock_config.new_certname = "one{0}two".format(os.path.sep) self.assertRaises(errors.ConfigurationError, self._call, mock_config) From 81fd0cd32c1e5191621d2961a4f8cc8c1fec3160 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Thu, 15 Dec 2016 20:23:02 -0800 Subject: [PATCH 08/44] Implement delete command (#3913) * organize cert_manager.py * add delete files to cert manager and storage * add tests * add to main and cli * Clean up all related files we can find, even if some are missing. * error messages, debug logs, and remove RenewerConfiguration * add logs for failure to remove * remove renewer_config_file --- certbot/cert_manager.py | 205 +++++++++++++++------------- certbot/cli.py | 17 ++- certbot/client.py | 3 +- certbot/configuration.py | 24 ++-- certbot/constants.py | 3 - certbot/interfaces.py | 3 - certbot/main.py | 8 ++ certbot/renewal.py | 25 +--- certbot/storage.py | 140 +++++++++++++++---- certbot/tests/cert_manager_test.py | 80 ++++++----- certbot/tests/configuration_test.py | 19 +-- certbot/tests/main_test.py | 8 +- certbot/tests/renewal_test.py | 3 +- certbot/tests/storage_test.py | 104 +++++++++++++- 14 files changed, 416 insertions(+), 226 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 0f6f4c730..d8752554a 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -6,10 +6,8 @@ import pytz import traceback import zope.component -from certbot import configuration from certbot import errors from certbot import interfaces -from certbot import renewal from certbot import storage from certbot import util @@ -17,6 +15,10 @@ from certbot.display import util as display_util logger = logging.getLogger(__name__) +################### +# Commands +################### + def update_live_symlinks(config): """Update the certificate file family symlinks to use archive_dir. @@ -26,36 +28,22 @@ def update_live_symlinks(config): .. note:: This assumes that the installation is using a Reverter object. :param config: Configuration. - :type config: :class:`certbot.interfaces.IConfig` + :type config: :class:`certbot.configuration.NamespaceConfig` """ - renewer_config = configuration.RenewerConfiguration(config) - for renewal_file in renewal.renewal_conf_files(renewer_config): - storage.RenewableCert(renewal_file, - configuration.RenewerConfiguration(renewer_config), - update_symlinks=True) + for renewal_file in storage.renewal_conf_files(config): + storage.RenewableCert(renewal_file, config, update_symlinks=True) def rename_lineage(config): """Rename the specified lineage to the new name. :param config: Configuration. - :type config: :class:`certbot.interfaces.IConfig` + :type config: :class:`certbot.configuration.NamespaceConfig` """ disp = zope.component.getUtility(interfaces.IDisplay) - renewer_config = configuration.RenewerConfiguration(config) - certname = config.certname - if not certname: - filenames = renewal.renewal_conf_files(renewer_config) - choices = [storage.lineagename_for_filename(name) for name in filenames] - if not choices: - raise errors.Error("No existing certificates found.") - code, index = disp.menu("Which certificate would you like to rename?", - choices, ok_label="Select", flag="--cert-name") - if code != display_util.OK or not index in range(0, len(choices)): - raise errors.Error("User ended interaction.") - certname = choices[index] + certname = _get_certname(config, "rename") new_certname = config.new_certname if not new_certname: @@ -68,10 +56,110 @@ def rename_lineage(config): if not lineage: raise errors.ConfigurationError("No existing certificate with name " "{0} found.".format(certname)) - storage.rename_renewal_config(certname, new_certname, renewer_config) + storage.rename_renewal_config(certname, new_certname, config) disp.notification("Successfully renamed {0} to {1}." .format(certname, new_certname), pause=False) +def certificates(config): + """Display information about certs configured with Certbot + + :param config: Configuration. + :type config: :class:`certbot.configuration.NamespaceConfig` + """ + parsed_certs = [] + parse_failures = [] + for renewal_file in storage.renewal_conf_files(config): + try: + renewal_candidate = storage.RenewableCert(renewal_file, config) + parsed_certs.append(renewal_candidate) + except Exception as e: # pylint: disable=broad-except + logger.warning("Renewal configuration file %s produced an " + "unexpected error: %s. Skipping.", renewal_file, e) + logger.debug("Traceback was:\n%s", traceback.format_exc()) + parse_failures.append(renewal_file) + + # Describe all the certs + _describe_certs(parsed_certs, parse_failures) + +def delete(config): + """Delete Certbot files associated with a certificate lineage.""" + certname = _get_certname(config, "delete") + storage.delete_files(config, certname) + disp = zope.component.getUtility(interfaces.IDisplay) + disp.notification("Deleted all files relating to certificate {0}." + .format(certname), pause=False) + +################### +# Public Helpers +################### + +def lineage_for_certname(config, certname): + """Find a lineage object with name certname.""" + def update_cert_for_name_match(candidate_lineage, rv): + """Return cert if it has name certname, else return rv + """ + matching_lineage_name_cert = rv + if candidate_lineage.lineagename == certname: + matching_lineage_name_cert = candidate_lineage + return matching_lineage_name_cert + return _search_lineages(config, update_cert_for_name_match, None) + +def domains_for_certname(config, certname): + """Find the domains in the cert with name certname.""" + def update_domains_for_name_match(candidate_lineage, rv): + """Return domains if certname matches, else return rv + """ + matching_domains = rv + if candidate_lineage.lineagename == certname: + matching_domains = candidate_lineage.names() + return matching_domains + return _search_lineages(config, update_domains_for_name_match, None) + +def find_duplicative_certs(config, domains): + """Find existing certs that duplicate the request.""" + def update_certs_for_domain_matches(candidate_lineage, rv): + """Return cert as identical_names_cert if it matches, + or subset_names_cert if it matches as subset + """ + # TODO: Handle these differently depending on whether they are + # expired or still valid? + identical_names_cert, subset_names_cert = rv + candidate_names = set(candidate_lineage.names()) + if candidate_names == set(domains): + identical_names_cert = candidate_lineage + elif candidate_names.issubset(set(domains)): + # This logic finds and returns the largest subset-names cert + # in the case where there are several available. + if subset_names_cert is None: + subset_names_cert = candidate_lineage + elif len(candidate_names) > len(subset_names_cert.names()): + subset_names_cert = candidate_lineage + return (identical_names_cert, subset_names_cert) + + return _search_lineages(config, update_certs_for_domain_matches, (None, None)) + + +################### +# Private Helpers +################### + +def _get_certname(config, verb): + """Get certname from flag, interactively, or error out. + """ + certname = config.certname + if not certname: + disp = zope.component.getUtility(interfaces.IDisplay) + filenames = storage.renewal_conf_files(config) + choices = [storage.lineagename_for_filename(name) for name in filenames] + if not choices: + raise errors.Error("No existing certificates found.") + code, index = disp.menu("Which certificate would you like to {0}?".format(verb), + choices, ok_label="Select", flag="--cert-name") + if code != display_util.OK or not index in range(0, len(choices)): + raise errors.Error("User ended interaction.") + certname = choices[index] + return certname + def _report_lines(msgs): """Format a results report for a category of single-line renewal outcomes""" return " " + "\n ".join(str(msg) for msg in msgs) @@ -126,42 +214,18 @@ def _describe_certs(parsed_certs, parse_failures): disp = zope.component.getUtility(interfaces.IDisplay) disp.notification("\n".join(out), pause=False, wrap=False) -def certificates(config): - """Display information about certs configured with Certbot - - :param config: Configuration. - :type config: :class:`certbot.interfaces.IConfig` - """ - renewer_config = configuration.RenewerConfiguration(config) - parsed_certs = [] - parse_failures = [] - for renewal_file in renewal.renewal_conf_files(renewer_config): - try: - renewal_candidate = storage.RenewableCert(renewal_file, - configuration.RenewerConfiguration(config)) - parsed_certs.append(renewal_candidate) - except Exception as e: # pylint: disable=broad-except - logger.warning("Renewal configuration file %s produced an " - "unexpected error: %s. Skipping.", renewal_file, e) - logger.debug("Traceback was:\n%s", traceback.format_exc()) - parse_failures.append(renewal_file) - - # Describe all the certs - _describe_certs(parsed_certs, parse_failures) - -def _search_lineages(config, func, initial_rv): +def _search_lineages(cli_config, func, initial_rv): """Iterate func over unbroken lineages, allowing custom return conditions. Allows flexible customization of return values, including multiple return values and complex checks. """ - cli_config = configuration.RenewerConfiguration(config) configs_dir = cli_config.renewal_configs_dir # Verify the directory is there util.make_or_verify_dir(configs_dir, mode=0o755, uid=os.geteuid()) rv = initial_rv - for renewal_file in renewal.renewal_conf_files(cli_config): + for renewal_file in storage.renewal_conf_files(cli_config): try: candidate_lineage = storage.RenewableCert(renewal_file, cli_config) except (errors.CertStorageError, IOError): @@ -170,48 +234,3 @@ def _search_lineages(config, func, initial_rv): continue rv = func(candidate_lineage, rv) return rv - -def lineage_for_certname(config, certname): - """Find a lineage object with name certname.""" - def update_cert_for_name_match(candidate_lineage, rv): - """Return cert if it has name certname, else return rv - """ - matching_lineage_name_cert = rv - if candidate_lineage.lineagename == certname: - matching_lineage_name_cert = candidate_lineage - return matching_lineage_name_cert - return _search_lineages(config, update_cert_for_name_match, None) - -def domains_for_certname(config, certname): - """Find the domains in the cert with name certname.""" - def update_domains_for_name_match(candidate_lineage, rv): - """Return domains if certname matches, else return rv - """ - matching_domains = rv - if candidate_lineage.lineagename == certname: - matching_domains = candidate_lineage.names() - return matching_domains - return _search_lineages(config, update_domains_for_name_match, None) - -def find_duplicative_certs(config, domains): - """Find existing certs that duplicate the request.""" - def update_certs_for_domain_matches(candidate_lineage, rv): - """Return cert as identical_names_cert if it matches, - or subset_names_cert if it matches as subset - """ - # TODO: Handle these differently depending on whether they are - # expired or still valid? - identical_names_cert, subset_names_cert = rv - candidate_names = set(candidate_lineage.names()) - if candidate_names == set(domains): - identical_names_cert = candidate_lineage - elif candidate_names.issubset(set(domains)): - # This logic finds and returns the largest subset-names cert - # in the case where there are several available. - if subset_names_cert is None: - subset_names_cert = candidate_lineage - elif len(candidate_names) > len(subset_names_cert.names()): - subset_names_cert = candidate_lineage - return (identical_names_cert, subset_names_cert) - - return _search_lineages(config, update_certs_for_domain_matches, (None, None)) diff --git a/certbot/cli.py b/certbot/cli.py index ef6257304..89c171829 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -82,6 +82,7 @@ manage certificates: certificates Display information about certs you have from Certbot revoke Revoke a certificate (supply --cert-path) rename Rename a certificate + delete Delete a certificate manage your account with Let's Encrypt: register Create a Let's Encrypt ACME account @@ -352,13 +353,17 @@ VERB_HELP = [ "short": "List all certificates managed by Certbot", "opts": "List all certificates managed by Certbot" }), + ("delete", { + "short": "Clean up all files related to a certificate", + "opts": "Options for deleting a certificate" + }), ("revoke", { "short": "Revoke a certificate specified with --cert-path", "opts": "Options for revocation of certs" }), ("rename", { "short": "Change a certificate's name (for management purposes)", - "opts": "Options changing certificate names" + "opts": "Options for changing certificate names" }), ("register", { "short": "Register for account with Let's Encrypt / other ACME server", @@ -410,7 +415,8 @@ class HelpfulArgumentParser(object): "register": main.register, "renew": main.renew, "revoke": main.revoke, "rollback": main.rollback, "everything": main.run, "update_symlinks": main.update_symlinks, - "certificates": main.certificates, "rename": main.rename} + "certificates": main.certificates, "rename": main.rename, + "delete": main.delete} # List of topics for which additional help can be provided HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + list(self.VERBS) @@ -763,7 +769,7 @@ def _add_all_groups(helpful): helpful.add_group("paths", description="Arguments changing execution paths & servers") helpful.add_group("manage", description="Various subcommands and flags are available for managing your certificates:", - verbs=["certificates", "renew", "revoke", "rename"]) + verbs=["certificates", "delete", "renew", "revoke", "rename"]) # VERBS for verb, docs in VERB_HELP: @@ -810,13 +816,12 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "multiple -d flags or enter a comma separated list of domains " "as a parameter. (default: Ask)") helpful.add( - [None, "run", "certonly", "manage"], + [None, "run", "certonly", "manage", "rename", "delete"], "--cert-name", dest="certname", metavar="CERTNAME", default=None, help="Certificate name to apply. Only one certificate name can be used " "per Certbot run. To see certificate names, run 'certbot certificates'. " - "If there is no existing certificate with this name and " - "domains are requested, create a new certificate with this name.") + "When creating a new certificate, specifies the new certificate's name.") helpful.add( ["rename", "manage"], "--updated-cert-name", dest="new_certname", diff --git a/certbot/client.py b/certbot/client.py index 6ff14bc56..cfd2b8487 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -15,7 +15,6 @@ import certbot from certbot import account from certbot import auth_handler -from certbot import configuration from certbot import constants from certbot import crypto_util from certbot import errors @@ -307,7 +306,7 @@ class Client(object): new_name, OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped), key.pem, crypto_util.dump_pyopenssl_chain(chain), - configuration.RenewerConfiguration(self.config.namespace)) + self.config) def save_certificate(self, certr, chain_cert, cert_path, chain_path, fullchain_path): diff --git a/certbot/configuration.py b/certbot/configuration.py index 1d4243272..d25378922 100644 --- a/certbot/configuration.py +++ b/certbot/configuration.py @@ -25,9 +25,16 @@ class NamespaceConfig(object): - `csr_dir` - `in_progress_dir` - `key_dir` - - `renewer_config_file` - `temp_checkpoint_dir` + And the following paths are dynamically resolved using + :attr:`~certbot.interfaces.IConfig.config_dir` and relative + paths defined in :py:mod:`certbot.constants`: + + - `default_archive_dir` + - `live_dir` + - `renewal_configs_dir` + :ivar namespace: Namespace typically produced by :meth:`argparse.ArgumentParser.parse_args`. :type namespace: :class:`argparse.Namespace` @@ -85,16 +92,6 @@ class NamespaceConfig(object): new_ns = copy.deepcopy(self.namespace) return type(self)(new_ns) - -class RenewerConfiguration(object): - """Configuration wrapper for renewer.""" - - def __init__(self, namespace): - self.namespace = namespace - - def __getattr__(self, name): - return getattr(self.namespace, name) - @property def default_archive_dir(self): # pylint: disable=missing-docstring return os.path.join(self.namespace.config_dir, constants.ARCHIVE_DIR) @@ -108,11 +105,6 @@ class RenewerConfiguration(object): return os.path.join( self.namespace.config_dir, constants.RENEWAL_CONFIGS_DIR) - @property - def renewer_config_file(self): # pylint: disable=missing-docstring - return os.path.join( - self.namespace.config_dir, constants.RENEWER_CONFIG_FILENAME) - def check_config_sanity(config): """Validate command line options and display error message if diff --git a/certbot/constants.py b/certbot/constants.py index 117301380..2e74cfd86 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -92,6 +92,3 @@ TEMP_CHECKPOINT_DIR = "temp_checkpoint" RENEWAL_CONFIGS_DIR = "renewal" """Renewal configs directory, relative to `IConfig.config_dir`.""" - -RENEWER_CONFIG_FILENAME = "renewer.conf" -"""Renewer config file name (relative to `IConfig.config_dir`).""" diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 6388bf936..f2b3faf21 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -223,9 +223,6 @@ class IConfig(zope.interface.Interface): temp_checkpoint_dir = zope.interface.Attribute( "Temporary checkpoint directory.") - renewer_config_file = zope.interface.Attribute( - "Location of renewal configuration file.") - no_verify_ssl = zope.interface.Attribute( "Disable verification of the ACME server's certificate.") tls_sni_01_port = zope.interface.Attribute( diff --git a/certbot/main.py b/certbot/main.py index 24f38172a..6a1653193 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -511,6 +511,14 @@ def rename(config, unused_plugins): """ cert_manager.rename_lineage(config) +def delete(config, unused_plugins): + """Delete a certificate + + Use the information in the config file to delete an existing + lineage. + """ + cert_manager.delete(config) + def certificates(config, unused_plugins): """Display information about certs configured with Certbot """ diff --git a/certbot/renewal.py b/certbot/renewal.py index a057a63a9..c9eca9a3a 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -1,7 +1,6 @@ """Functionality for autorenewal and associated juggling of configurations""" from __future__ import print_function import copy -import glob import logging import os import traceback @@ -11,7 +10,6 @@ import zope.component import OpenSSL -from certbot import configuration from certbot import cli from certbot import crypto_util @@ -34,17 +32,6 @@ STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent", INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"] -def renewal_conf_files(config): - """Return /path/to/*.conf in the renewal conf directory""" - return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf")) - -def renewal_file_for_certname(config, certname): - """Return /path/to/certname.conf in the renewal conf directory""" - path = os.path.join(config.renewal_configs_dir, "{0}.conf".format(certname)) - if not os.path.exists(path): - raise errors.CertStorageError("No certificate found with name {0}.".format(certname)) - return path - def _reconstitute(config, full_path): """Try to instantiate a RenewableCert, updating config with relevant items. @@ -63,8 +50,7 @@ def _reconstitute(config, full_path): """ try: - renewal_candidate = storage.RenewableCert( - full_path, configuration.RenewerConfiguration(config)) + renewal_candidate = storage.RenewableCert(full_path, config) except (errors.CertStorageError, IOError) as exc: logger.warning(exc) logger.warning("Renewal configuration file %s is broken. Skipping.", full_path) @@ -246,9 +232,8 @@ def renew_cert(config, le_client, lineage): 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) # TODO: Check return value of save_successor - lineage.save_successor(prior_version, new_cert, new_key.pem, new_chain, renewal_conf) + lineage.save_successor(prior_version, new_cert, new_key.pem, new_chain, config) lineage.update_all_links_to(lineage.latest_common_version()) hooks.renew_hook(config, lineage.names(), lineage.live_dir) @@ -317,12 +302,10 @@ def handle_renewal_request(config): "command. The renew verb may provide other options " "for selecting certificates to renew in the future.") - renewer_config = configuration.RenewerConfiguration(config) - if config.certname: - conf_files = [renewal_file_for_certname(renewer_config, config.certname)] + conf_files = [storage.renewal_file_for_certname(config, config.certname)] else: - conf_files = renewal_conf_files(renewer_config) + conf_files = storage.renewal_conf_files(config) renew_successes = [] renew_failures = [] diff --git a/certbot/storage.py b/certbot/storage.py index 671922bee..e4fc21a85 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -1,5 +1,6 @@ """Renewable certificates storage.""" import datetime +import glob import logging import os import re @@ -7,6 +8,7 @@ import re import configobj import parsedatetime import pytz +import shutil import six import certbot @@ -20,9 +22,22 @@ from certbot import util logger = logging.getLogger(__name__) ALL_FOUR = ("cert", "privkey", "chain", "fullchain") +README = "README" CURRENT_VERSION = util.get_strict_version(certbot.__version__) +def renewal_conf_files(config): + """Return /path/to/*.conf in the renewal conf directory""" + return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf")) + +def renewal_file_for_certname(config, certname): + """Return /path/to/certname.conf in the renewal conf directory""" + path = os.path.join(config.renewal_configs_dir, "{0}.conf".format(certname)) + if not os.path.exists(path): + raise errors.CertStorageError("No certificate found with name {0} (expected " + "{1}).".format(certname, path)) + return path + def config_with_defaults(config=None): """Merge supplied config, if provided, on top of builtin defaults.""" defaults_copy = configobj.ConfigObj(constants.RENEWER_DEFAULTS) @@ -99,13 +114,11 @@ def write_renewal_config(o_filename, n_filename, archive_dir, target, relevant_d def rename_renewal_config(prev_name, new_name, cli_config): """Renames cli_config.certname's config to cli_config.new_certname. - :param .RenewerConfiguration cli_config: parsed command line + :param .NamespaceConfig cli_config: parsed command line arguments """ - prev_filename = os.path.join( - cli_config.renewal_configs_dir, prev_name) + ".conf" - new_filename = os.path.join( - cli_config.renewal_configs_dir, new_name) + ".conf" + prev_filename = renewal_filename_for_lineagename(cli_config, prev_name) + new_filename = renewal_filename_for_lineagename(cli_config, new_name) if os.path.exists(new_filename): raise errors.ConfigurationError("The new certificate name " "is already in use.") @@ -122,15 +135,14 @@ def update_configuration(lineagename, archive_dir, target, cli_config): :param str lineagename: Name of the lineage being modified :param str archive_dir: Absolute path to the archive directory :param dict target: Maps ALL_FOUR to their symlink paths - :param .RenewerConfiguration cli_config: parsed command line + :param .NamespaceConfig cli_config: parsed command line arguments :returns: Configuration object for the updated config file :rtype: configobj.ConfigObj """ - config_filename = os.path.join( - cli_config.renewal_configs_dir, lineagename) + ".conf" + config_filename = renewal_filename_for_lineagename(cli_config, lineagename) temp_filename = config_filename + ".new" # If an existing tempfile exists, delete it @@ -198,10 +210,98 @@ def lineagename_for_filename(config_filename): "renewal config file name must end in .conf") return os.path.basename(config_filename[:-len(".conf")]) +def renewal_filename_for_lineagename(config, lineagename): + """Returns the lineagename for a configuration filename. + """ + return os.path.join(config.renewal_configs_dir, lineagename) + ".conf" + def _relpath_from_file(archive_dir, from_file): """Path to a directory from a file""" return os.path.relpath(archive_dir, os.path.dirname(from_file)) +def _full_archive_path(config_obj, cli_config, lineagename): + """Returns the full archive path for a lineagename + + Uses cli_config to determine archive path if not available from config_obj. + + :param configobj.ConfigObj config_obj: Renewal conf file contents (can be None) + :param configuration.NamespaceConfig cli_config: Main config file + :param str lineagename: Certificate name + """ + if config_obj and "archive_dir" in config_obj: + return config_obj["archive_dir"] + else: + return os.path.join(cli_config.default_archive_dir, lineagename) + +def _full_live_path(cli_config, lineagename): + """Returns the full default live path for a lineagename""" + return os.path.join(cli_config.live_dir, lineagename) + +def delete_files(config, certname): + """Delete all files related to the certificate. + + If some files are not found, ignore them and continue. + """ + renewal_filename = renewal_file_for_certname(config, certname) + # file exists + full_default_archive_dir = _full_archive_path(None, config, certname) + full_default_live_dir = _full_live_path(config, certname) + try: + renewal_config = configobj.ConfigObj(renewal_filename) + except configobj.ConfigObjError: + # config is corrupted + logger.warning("Could not parse %s. You may wish to manually " + "delete the contents of %s and %s.", renewal_filename, + full_default_live_dir, full_default_archive_dir) + raise errors.CertStorageError( + "error parsing {0}".format(renewal_filename)) + finally: + # we couldn't read it, but let's at least delete it + # if this was going to fail, it already would have. + os.remove(renewal_filename) + logger.debug("Removed %s", renewal_filename) + + # cert files and (hopefully) live directory + # it's not guaranteed that the files are in our default storage + # structure. so, first delete the cert files. + directory_names = set() + for kind in ALL_FOUR: + link = renewal_config.get(kind) + try: + os.remove(link) + logger.debug("Removed %s", link) + except OSError: + logger.debug("Unable to delete %s", link) + directory = os.path.dirname(link) + directory_names.add(directory) + + # if all four were in the same directory, and the only thing left + # is the README file (or nothing), delete that directory. + # this will be wrong in very few but some cases. + if len(directory_names) == 1: + # delete the README file + directory = directory_names.pop() + readme_path = os.path.join(directory, README) + try: + os.remove(readme_path) + logger.debug("Removed %s", readme_path) + except OSError: + logger.debug("Unable to delete %s", readme_path) + # if it's now empty, delete the directory + try: + os.rmdir(directory) # only removes empty directories + logger.debug("Removed %s", directory) + except OSError: + logger.debug("Unable to remove %s; may not be empty.", directory) + + # archive directory + try: + archive_path = _full_archive_path(renewal_config, config, certname) + shutil.rmtree(archive_path) + logger.debug("Removed %s", archive_path) + except OSError: + logger.debug("Unable to remove %s", archive_path) + class RenewableCert(object): # pylint: disable=too-many-instance-attributes,too-many-public-methods @@ -244,7 +344,7 @@ class RenewableCert(object): :param str config_filename: the path to the renewal config file that defines this lineage. - :param .RenewerConfiguration: parsed command line arguments + :param .NamespaceConfig: parsed command line arguments :raises .CertStorageError: if the configuration file's name didn't end in ".conf", or the file is missing or broken. @@ -303,11 +403,8 @@ class RenewableCert(object): @property def archive_dir(self): """Returns the default or specified archive directory""" - if "archive_dir" in self.configuration: - return self.configuration["archive_dir"] - else: - return os.path.join( - self.cli_config.default_archive_dir, self.lineagename) + return _full_archive_path(self.configuration, + self.cli_config, self.lineagename) def relative_archive_dir(self, from_file): """Returns the default or specified archive directory as a relative path @@ -827,7 +924,7 @@ class RenewableCert(object): :param str cert: the initial certificate version in PEM format :param str privkey: the private key in PEM format :param str chain: the certificate chain in PEM format - :param .RenewerConfiguration cli_config: parsed command line + :param .NamespaceConfig cli_config: parsed command line arguments :returns: the newly-created RenewalCert object @@ -843,16 +940,13 @@ class RenewableCert(object): logger.debug("Creating directory %s.", i) config_file, config_filename = util.unique_lineage_name( cli_config.renewal_configs_dir, lineagename) - if not config_filename.endswith(".conf"): - raise errors.CertStorageError( - "renewal config file name must end in .conf") # Determine where on disk everything will go # lineagename will now potentially be modified based on which # renewal configuration file could actually be created - lineagename = os.path.basename(config_filename)[:-len(".conf")] - archive = os.path.join(cli_config.default_archive_dir, lineagename) - live_dir = os.path.join(cli_config.live_dir, lineagename) + lineagename = lineagename_for_filename(config_filename) + archive = _full_archive_path(None, cli_config, lineagename) + live_dir = _full_live_path(cli_config, lineagename) if os.path.exists(archive): raise errors.CertStorageError( "archive directory exists for " + lineagename) @@ -887,7 +981,7 @@ class RenewableCert(object): f.write(cert + chain) # Write a README file to the live directory - readme_path = os.path.join(live_dir, "README") + readme_path = os.path.join(live_dir, README) with open(readme_path, "w") as f: logger.debug("Writing README to %s.", readme_path) f.write("This directory contains your keys and certificates.\n\n" @@ -928,7 +1022,7 @@ class RenewableCert(object): :param str new_privkey: the new private key, in PEM format, or ``None``, if the private key has not changed :param str new_chain: the new chain, in PEM format - :param .RenewerConfiguration cli_config: parsed command line + :param .NamespaceConfig cli_config: parsed command line arguments :returns: the new version number that was created diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index e04e25da8..ae673fba1 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -25,12 +25,12 @@ class BaseCertManagerTest(unittest.TestCase): os.makedirs(os.path.join(self.tempdir, "renewal")) - self.cli_config = mock.MagicMock( + self.cli_config = configuration.NamespaceConfig(mock.MagicMock( config_dir=self.tempdir, work_dir=self.tempdir, logs_dir=self.tempdir, quiet=False, - ) + )) self.domains = { "example.org": None, @@ -47,7 +47,7 @@ class BaseCertManagerTest(unittest.TestCase): junk.close() def _set_up_config(self, domain, custom_archive): - # TODO: maybe provide RenewerConfiguration.make_dirs? + # TODO: maybe provide NamespaceConfig.make_dirs? # TODO: main() should create those dirs, c.f. #902 os.makedirs(os.path.join(self.tempdir, "live", domain)) config = configobj.ConfigObj() @@ -111,6 +111,21 @@ class UpdateLiveSymlinksTest(BaseCertManagerTest): os.chdir(prev_dir) +class DeleteTest(storage_test.BaseRenewableCertTest): + """Tests for certbot.cert_manager.delete + """ + @mock.patch('zope.component.getUtility') + @mock.patch('certbot.cert_manager.lineage_for_certname') + @mock.patch('certbot.storage.delete_files') + def test_delete(self, mock_delete_files, mock_lineage_for_certname, unused_get_utility): + """Test delete""" + mock_lineage_for_certname.return_value = self.test_rc + self.cli_config.certname = "example.org" + from certbot import cert_manager + cert_manager.delete(self.cli_config) + self.assertTrue(mock_delete_files.called) + + class CertificatesTest(BaseCertManagerTest): """Tests for certbot.cert_manager.certificates """ @@ -151,12 +166,12 @@ class CertificatesTest(BaseCertManagerTest): def test_certificates_no_files(self, mock_utility, mock_logger): tempdir = tempfile.mkdtemp() - cli_config = mock.MagicMock( + cli_config = configuration.NamespaceConfig(mock.MagicMock( config_dir=tempdir, work_dir=tempdir, logs_dir=tempdir, quiet=False, - ) + )) os.makedirs(os.path.join(tempdir, "renewal")) self._certificates(cli_config) @@ -202,90 +217,85 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue('INVALID: TEST CERT' in out) -class SearchLineagesTest(unittest.TestCase): +class SearchLineagesTest(BaseCertManagerTest): """Tests for certbot.cert_manager._search_lineages.""" - @mock.patch('certbot.configuration.RenewerConfiguration') @mock.patch('certbot.util.make_or_verify_dir') - @mock.patch('certbot.renewal.renewal_conf_files') + @mock.patch('certbot.storage.renewal_conf_files') @mock.patch('certbot.storage.RenewableCert') def test_cert_storage_error(self, mock_renewable_cert, mock_renewal_conf_files, - mock_make_or_verify_dir, mock_renewer_config): + mock_make_or_verify_dir): mock_renewal_conf_files.return_value = ["badfile"] mock_renewable_cert.side_effect = errors.CertStorageError from certbot import cert_manager # pylint: disable=protected-access - self.assertEqual(cert_manager._search_lineages(None, lambda x: x, "check"), "check") + self.assertEqual(cert_manager._search_lineages(self.cli_config, lambda x: x, "check"), + "check") self.assertTrue(mock_make_or_verify_dir.called) - self.assertTrue(mock_renewer_config) -class LineageForCertnameTest(unittest.TestCase): +class LineageForCertnameTest(BaseCertManagerTest): """Tests for certbot.cert_manager.lineage_for_certname""" - @mock.patch('certbot.configuration.RenewerConfiguration') @mock.patch('certbot.util.make_or_verify_dir') - @mock.patch('certbot.renewal.renewal_conf_files') + @mock.patch('certbot.storage.renewal_conf_files') @mock.patch('certbot.storage.RenewableCert') def test_found_match(self, mock_renewable_cert, mock_renewal_conf_files, - mock_make_or_verify_dir, mock_renewer_config): + mock_make_or_verify_dir): mock_renewal_conf_files.return_value = ["somefile.conf"] mock_match = mock.Mock(lineagename="example.com") mock_renewable_cert.return_value = mock_match from certbot import cert_manager - self.assertEqual(cert_manager.lineage_for_certname(None, "example.com"), mock_match) + self.assertEqual(cert_manager.lineage_for_certname(self.cli_config, "example.com"), + mock_match) self.assertTrue(mock_make_or_verify_dir.called) - self.assertTrue(mock_renewer_config) - @mock.patch('certbot.configuration.RenewerConfiguration') @mock.patch('certbot.util.make_or_verify_dir') - @mock.patch('certbot.renewal.renewal_conf_files') + @mock.patch('certbot.storage.renewal_conf_files') @mock.patch('certbot.storage.RenewableCert') def test_no_match(self, mock_renewable_cert, mock_renewal_conf_files, - mock_make_or_verify_dir, mock_renewer_config): + mock_make_or_verify_dir): mock_renewal_conf_files.return_value = ["somefile.conf"] mock_match = mock.Mock(lineagename="other.com") mock_renewable_cert.return_value = mock_match from certbot import cert_manager - self.assertEqual(cert_manager.lineage_for_certname(None, "example.com"), None) + self.assertEqual(cert_manager.lineage_for_certname(self.cli_config, "example.com"), + None) self.assertTrue(mock_make_or_verify_dir.called) - self.assertTrue(mock_renewer_config) -class DomainsForCertnameTest(unittest.TestCase): +class DomainsForCertnameTest(BaseCertManagerTest): """Tests for certbot.cert_manager.domains_for_certname""" - @mock.patch('certbot.configuration.RenewerConfiguration') @mock.patch('certbot.util.make_or_verify_dir') - @mock.patch('certbot.renewal.renewal_conf_files') + @mock.patch('certbot.storage.renewal_conf_files') @mock.patch('certbot.storage.RenewableCert') def test_found_match(self, mock_renewable_cert, mock_renewal_conf_files, - mock_make_or_verify_dir, mock_renewer_config): + mock_make_or_verify_dir): mock_renewal_conf_files.return_value = ["somefile.conf"] mock_match = mock.Mock(lineagename="example.com") domains = ["example.com", "example.org"] mock_match.names.return_value = domains mock_renewable_cert.return_value = mock_match from certbot import cert_manager - self.assertEqual(cert_manager.domains_for_certname(None, "example.com"), domains) + self.assertEqual(cert_manager.domains_for_certname(self.cli_config, "example.com"), + domains) self.assertTrue(mock_make_or_verify_dir.called) - self.assertTrue(mock_renewer_config) - @mock.patch('certbot.configuration.RenewerConfiguration') @mock.patch('certbot.util.make_or_verify_dir') - @mock.patch('certbot.renewal.renewal_conf_files') + @mock.patch('certbot.storage.renewal_conf_files') @mock.patch('certbot.storage.RenewableCert') def test_no_match(self, mock_renewable_cert, mock_renewal_conf_files, - mock_make_or_verify_dir, mock_renewer_config): + mock_make_or_verify_dir): mock_renewal_conf_files.return_value = ["somefile.conf"] mock_match = mock.Mock(lineagename="example.com") domains = ["example.com", "example.org"] mock_match.names.return_value = domains mock_renewable_cert.return_value = mock_match from certbot import cert_manager - self.assertEqual(cert_manager.domains_for_certname(None, "other.com"), None) + self.assertEqual(cert_manager.domains_for_certname(self.cli_config, "other.com"), + None) self.assertTrue(mock_make_or_verify_dir.called) - self.assertTrue(mock_renewer_config) class RenameLineageTest(BaseCertManagerTest): @@ -293,7 +303,7 @@ class RenameLineageTest(BaseCertManagerTest): def setUp(self): super(RenameLineageTest, self).setUp() - self.mock_config = configuration.RenewerConfiguration( + self.mock_config = configuration.NamespaceConfig( namespace=mock.MagicMock( config_dir=self.tempdir, work_dir=self.tempdir, @@ -307,7 +317,7 @@ class RenameLineageTest(BaseCertManagerTest): from certbot import cert_manager return cert_manager.rename_lineage(*args, **kwargs) - @mock.patch('certbot.renewal.renewal_conf_files') + @mock.patch('certbot.storage.renewal_conf_files') @mock.patch('certbot.main.zope.component.getUtility') def test_no_certname(self, mock_get_utility, mock_renewal_conf_files): mock_config = mock.Mock(certname=None, new_certname="two") diff --git a/certbot/tests/configuration_test.py b/certbot/tests/configuration_test.py index 5e59d0b86..183d6a95c 100644 --- a/certbot/tests/configuration_test.py +++ b/certbot/tests/configuration_test.py @@ -88,31 +88,19 @@ class NamespaceConfigTest(unittest.TestCase): self.assertTrue(os.path.isabs(config.key_dir)) self.assertTrue(os.path.isabs(config.temp_checkpoint_dir)) - -class RenewerConfigurationTest(unittest.TestCase): - """Test for certbot.configuration.RenewerConfiguration.""" - - def setUp(self): - self.namespace = mock.MagicMock(config_dir='/tmp/config') - from certbot.configuration import RenewerConfiguration - self.config = RenewerConfiguration(self.namespace) - @mock.patch('certbot.configuration.constants') - def test_dynamic_dirs(self, constants): + def test_renewal_dynamic_dirs(self, constants): constants.ARCHIVE_DIR = 'a' constants.LIVE_DIR = 'l' constants.RENEWAL_CONFIGS_DIR = 'renewal_configs' - constants.RENEWER_CONFIG_FILENAME = 'r.conf' self.assertEqual(self.config.default_archive_dir, '/tmp/config/a') self.assertEqual(self.config.live_dir, '/tmp/config/l') self.assertEqual( self.config.renewal_configs_dir, '/tmp/config/renewal_configs') - self.assertEqual(self.config.renewer_config_file, '/tmp/config/r.conf') - def test_absolute_paths(self): + def test_renewal_absolute_paths(self): from certbot.configuration import NamespaceConfig - from certbot.configuration import RenewerConfiguration config_base = "foo" work_base = "bar" @@ -125,12 +113,11 @@ class RenewerConfigurationTest(unittest.TestCase): mock_namespace.config_dir = config_base mock_namespace.work_dir = work_base mock_namespace.logs_dir = logs_base - config = RenewerConfiguration(NamespaceConfig(mock_namespace)) + config = NamespaceConfig(mock_namespace) self.assertTrue(os.path.isabs(config.default_archive_dir)) self.assertTrue(os.path.isabs(config.live_dir)) self.assertTrue(os.path.isabs(config.renewal_configs_dir)) - self.assertTrue(os.path.isabs(config.renewer_config_file)) if __name__ == '__main__': diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index a0d6cc418..3665f09bb 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -567,6 +567,11 @@ class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods self._call_no_clientmock(['certificates']) self.assertEqual(1, mock_cert_manager.call_count) + @mock.patch('certbot.cert_manager.delete') + def test_delete(self, mock_cert_manager): + self._call_no_clientmock(['delete']) + self.assertEqual(1, mock_cert_manager.call_count) + def test_plugins(self): flags = ['--init', '--prepare', '--authenticators', '--installers'] for args in itertools.chain( @@ -856,8 +861,7 @@ class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods rc_path = test_util.make_lineage(self, 'sample-renewal-ancient.conf') args = mock.MagicMock(account=None, email=None, webroot_path=None) config = configuration.NamespaceConfig(args) - lineage = storage.RenewableCert(rc_path, - configuration.RenewerConfiguration(config)) + lineage = storage.RenewableCert(rc_path, config) renewalparams = lineage.configuration["renewalparams"] # pylint: disable=protected-access renewal._restore_webroot_config(config, renewalparams) diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index 207b70041..8155595c2 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -21,8 +21,7 @@ class RenewalTest(unittest.TestCase): rc_path = util.make_lineage(self, 'sample-renewal-ancient.conf') args = mock.MagicMock(account=None, email=None, webroot_path=None) config = configuration.NamespaceConfig(args) - lineage = storage.RenewableCert( - rc_path, configuration.RenewerConfiguration(config)) + lineage = storage.RenewableCert(rc_path, config) renewalparams = lineage.configuration["renewalparams"] # pylint: disable=protected-access from certbot import renewal diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index ebe7d2243..a1fda6535 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -49,7 +49,7 @@ class BaseRenewableCertTest(unittest.TestCase): from certbot import storage self.tempdir = tempfile.mkdtemp() - self.cli_config = configuration.RenewerConfiguration( + self.cli_config = configuration.NamespaceConfig( namespace=mock.MagicMock( config_dir=self.tempdir, work_dir=self.tempdir, @@ -57,16 +57,22 @@ class BaseRenewableCertTest(unittest.TestCase): ) ) - # TODO: maybe provide RenewerConfiguration.make_dirs? + # TODO: maybe provide NamespaceConfig.make_dirs? # TODO: main() should create those dirs, c.f. #902 os.makedirs(os.path.join(self.tempdir, "live", "example.org")) - os.makedirs(os.path.join(self.tempdir, "archive", "example.org")) + archive_path = os.path.join(self.tempdir, "archive", "example.org") + os.makedirs(archive_path) os.makedirs(os.path.join(self.tempdir, "renewal")) config = configobj.ConfigObj() for kind in ALL_FOUR: - config[kind] = os.path.join(self.tempdir, "live", "example.org", + kind_path = os.path.join(self.tempdir, "live", "example.org", kind + ".pem") + config[kind] = kind_path + with open(os.path.join(self.tempdir, "live", "example.org", + "README"), 'a'): + pass + config["archive"] = archive_path config.filename = os.path.join(self.tempdir, "renewal", "example.org.conf") config.write() @@ -770,5 +776,95 @@ class RenewableCertTests(BaseRenewableCertTest): storage.RenewableCert(self.config.filename, self.cli_config, update_symlinks=True) +class DeleteFilesTest(BaseRenewableCertTest): + """Tests for certbot.storage.delete_files""" + def setUp(self): + super(DeleteFilesTest, self).setUp() + for kind in ALL_FOUR: + kind_path = os.path.join(self.tempdir, "live", "example.org", + kind + ".pem") + with open(kind_path, 'a'): + pass + self.config.write() + self.assertTrue(os.path.exists(os.path.join( + self.cli_config.renewal_configs_dir, "example.org.conf"))) + self.assertTrue(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertTrue(os.path.exists(os.path.join( + self.tempdir, "archive", "example.org"))) + + def _call(self): + from certbot import storage + with mock.patch("certbot.storage.logger"): + storage.delete_files(self.cli_config, "example.org") + + def test_delete_all_files(self): + self._call() + + self.assertFalse(os.path.exists(os.path.join( + self.cli_config.renewal_configs_dir, "example.org.conf"))) + self.assertFalse(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(os.path.join( + self.tempdir, "archive", "example.org"))) + + def test_bad_renewal_config(self): + with open(self.config.filename, 'a') as config_file: + config_file.write("asdfasfasdfasdf") + + self.assertRaises(errors.CertStorageError, self._call) + self.assertTrue(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(os.path.join( + self.cli_config.renewal_configs_dir, "example.org.conf"))) + + def test_no_renewal_config(self): + os.remove(self.config.filename) + self.assertRaises(errors.CertStorageError, self._call) + self.assertTrue(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(self.config.filename)) + + def test_no_cert_file(self): + os.remove(os.path.join( + self.cli_config.live_dir, "example.org", "cert.pem")) + self._call() + self.assertFalse(os.path.exists(self.config.filename)) + self.assertFalse(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(os.path.join( + self.tempdir, "archive", "example.org"))) + + def test_no_readme_file(self): + os.remove(os.path.join( + self.cli_config.live_dir, "example.org", "README")) + self._call() + self.assertFalse(os.path.exists(self.config.filename)) + self.assertFalse(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(os.path.join( + self.tempdir, "archive", "example.org"))) + + def test_livedir_not_empty(self): + with open(os.path.join( + self.cli_config.live_dir, "example.org", "other_file"), 'a'): + pass + self._call() + self.assertFalse(os.path.exists(self.config.filename)) + self.assertTrue(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(os.path.join( + self.tempdir, "archive", "example.org"))) + + def test_no_archive(self): + archive_dir = os.path.join(self.tempdir, "archive", "example.org") + os.rmdir(archive_dir) + self._call() + self.assertFalse(os.path.exists(self.config.filename)) + self.assertFalse(os.path.exists(os.path.join( + self.cli_config.live_dir, "example.org"))) + self.assertFalse(os.path.exists(archive_dir)) + + if __name__ == "__main__": unittest.main() # pragma: no cover From ae379568b12b28112f6798366002f3265d77217e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 19 Dec 2016 12:45:40 -0800 Subject: [PATCH 09/44] Mitigate problems for people who run without -n (#3916) * CLI flag for forcing interactivity * add --force-interactive * Add force_interactive error checking and tests * Add force_interactive parameter to FileDisplay * add _can_interact * Add _return_default * Add **unused_kwargs to NoninteractiveDisplay * improve _return_default assertion * Change IDisplay calls and write tests * Document force_interactive in interfaces.py * Don't force_interactive with a new prompt * Warn when skipping an interaction for the first time * add specific logger.debug message --- certbot-apache/certbot_apache/configurator.py | 2 +- certbot-apache/certbot_apache/display_ops.py | 6 +- .../certbot_apache/tests/display_ops_test.py | 3 +- certbot-apache/certbot_apache/tests/util.py | 3 +- certbot/cert_manager.py | 5 +- certbot/cli.py | 15 ++ certbot/constants.py | 3 + certbot/display/enhancements.py | 3 +- certbot/display/ops.py | 12 +- certbot/display/util.py | 142 +++++++++++++++--- certbot/interfaces.py | 47 +++++- certbot/main.py | 12 +- certbot/plugins/manual.py | 3 +- certbot/plugins/selection.py | 6 +- certbot/plugins/selection_test.py | 3 +- certbot/plugins/standalone.py | 4 +- certbot/plugins/util.py | 5 +- certbot/plugins/webroot.py | 6 +- certbot/reverter.py | 2 +- certbot/tests/cli_test.py | 6 + certbot/tests/display/ops_test.py | 12 +- certbot/tests/display/util_test.py | 99 +++++++++--- tests/display.py | 2 +- 23 files changed, 320 insertions(+), 81 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 1bb0a1e1a..b200d5eaa 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -463,7 +463,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): zope.component.getUtility(interfaces.IDisplay).notification( "Apache mod_macro seems to be in use in file(s):\n{0}" "\n\nUnfortunately mod_macro is not yet supported".format( - "\n ".join(vhost_macro))) + "\n ".join(vhost_macro)), force_interactive=True) return all_names diff --git a/certbot-apache/certbot_apache/display_ops.py b/certbot-apache/certbot_apache/display_ops.py index 527de1001..22aafc0fe 100644 --- a/certbot-apache/certbot_apache/display_ops.py +++ b/certbot-apache/certbot_apache/display_ops.py @@ -85,7 +85,8 @@ def _vhost_menu(domain, vhosts): "or Address of {0}.{1}Which virtual host would you " "like to choose?\n(note: conf files with multiple " "vhosts are not yet supported)".format(domain, os.linesep), - choices, help_label="More Info", ok_label="Select") + choices, help_label="More Info", + ok_label="Select", force_interactive=True) except errors.MissingCommandlineFlag: msg = ("Encountered vhost ambiguity but unable to ask for user guidance in " "non-interactive mode. Currently Certbot needs each vhost to be " @@ -100,4 +101,5 @@ def _vhost_menu(domain, vhosts): def _more_info_vhost(vhost): zope.component.getUtility(interfaces.IDisplay).notification( "Virtual Host Information:{0}{1}{0}{2}".format( - os.linesep, "-" * (display_util.WIDTH - 4), str(vhost))) + os.linesep, "-" * (display_util.WIDTH - 4), str(vhost)), + force_interactive=True) diff --git a/certbot-apache/certbot_apache/tests/display_ops_test.py b/certbot-apache/certbot_apache/tests/display_ops_test.py index 585661c7f..dea1e4433 100644 --- a/certbot-apache/certbot_apache/tests/display_ops_test.py +++ b/certbot-apache/certbot_apache/tests/display_ops_test.py @@ -17,7 +17,8 @@ class SelectVhostTest(unittest.TestCase): """Tests for certbot_apache.display_ops.select_vhost.""" def setUp(self): - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) self.base_dir = "/example_path" self.vhosts = util.get_vh_truth( self.base_dir, "debian_apache_2_4/multiple_vhosts") diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 6a0a83615..3c33a0e19 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -64,7 +64,8 @@ class ParserTest(ApacheTest): # pytlint: disable=too-few-public-methods vhost_root="debian_apache_2_4/multiple_vhosts/apache2/sites-available"): super(ParserTest, self).setUp(test_dir, config_root, vhost_root) - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) from certbot_apache.parser import ApacheParser self.aug = augeas.Augeas( diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index d8752554a..35b12e1bb 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -47,8 +47,9 @@ def rename_lineage(config): new_certname = config.new_certname if not new_certname: - code, new_certname = disp.input("Enter the new name for certificate {0}" - .format(certname), flag="--updated-cert-name") + code, new_certname = disp.input( + "Enter the new name for certificate {0}".format(certname), + flag="--updated-cert-name", force_interactive=True) if code != display_util.OK or not new_certname: raise errors.Error("User ended interaction.") diff --git a/certbot/cli.py b/certbot/cli.py index 89c171829..0adb7a4b5 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -521,8 +521,17 @@ class HelpfulArgumentParser(object): # Do any post-parsing homework here if self.verb == "renew": + if parsed_args.force_interactive: + raise errors.Error( + "{0} cannot be used with renew".format( + constants.FORCE_INTERACTIVE_FLAG)) parsed_args.noninteractive_mode = True + if parsed_args.force_interactive and parsed_args.noninteractive_mode: + raise errors.Error( + "Flag for non-interactive mode and {0} conflict".format( + constants.FORCE_INTERACTIVE_FLAG)) + if parsed_args.staging or parsed_args.dry_run: self.set_test_server(parsed_args) @@ -808,6 +817,12 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="Run without ever asking for user input. This may require " "additional command line flags; the client will try to explain " "which ones are required if it finds one missing") + helpful.add( + [None, "register", "run", "certonly"], + constants.FORCE_INTERACTIVE_FLAG, action="store_true", + help="Force Certbot to be interactive even if it detects it's not " + "being run in a terminal. This flag cannot be used with the " + "renew subcommand.") helpful.add( [None, "run", "certonly"], "-d", "--domains", "--domain", dest="domains", diff --git a/certbot/constants.py b/certbot/constants.py index 2e74cfd86..7d713d29f 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -92,3 +92,6 @@ TEMP_CHECKPOINT_DIR = "temp_checkpoint" RENEWAL_CONFIGS_DIR = "renewal" """Renewal configs directory, relative to `IConfig.config_dir`.""" + +FORCE_INTERACTIVE_FLAG = "--force-interactive" +"""Flag to disable TTY checking in IDisplay.""" diff --git a/certbot/display/enhancements.py b/certbot/display/enhancements.py index 3b128a874..d2ffe2e0d 100644 --- a/certbot/display/enhancements.py +++ b/certbot/display/enhancements.py @@ -48,7 +48,8 @@ def redirect_by_default(): code, selection = util(interfaces.IDisplay).menu( "Please choose whether HTTPS access is required or optional.", - choices, default=0, cli_flag="--redirect / --no-redirect") + choices, default=0, + cli_flag="--redirect / --no-redirect", force_interactive=True) if code != display_util.OK: return False diff --git a/certbot/display/ops.py b/certbot/display/ops.py index b9cd1e38c..85343fdc3 100644 --- a/certbot/display/ops.py +++ b/certbot/display/ops.py @@ -46,7 +46,8 @@ def get_email(invalid=False, optional=True): while True: try: code, email = z_util(interfaces.IDisplay).input( - invalid_prefix + msg if invalid else msg) + invalid_prefix + msg if invalid else msg, + force_interactive=True) except errors.MissingCommandlineFlag: msg = ("You should register before running non-interactively, " "or provide --agree-tos and --email flags.") @@ -79,7 +80,7 @@ def choose_account(accounts): labels = [acc.slug for acc in accounts] code, index = z_util(interfaces.IDisplay).menu( - "Please choose an account", labels) + "Please choose an account", labels, force_interactive=True) if code == display_util.OK: return accounts[index] else: @@ -157,7 +158,7 @@ def _filter_names(names): code, names = z_util(interfaces.IDisplay).checklist( "Which names would you like to activate HTTPS for?", - tags=sorted_names, cli_flag="--domains") + tags=sorted_names, cli_flag="--domains", force_interactive=True) return code, [str(s) for s in names] @@ -173,7 +174,7 @@ def _choose_names_manually(prompt_prefix=""): code, input_ = z_util(interfaces.IDisplay).input( prompt_prefix + "Please enter in your domain name(s) (comma and/or space separated) ", - cli_flag="--domains") + cli_flag="--domains", force_interactive=True) if code == display_util.OK: invalid_domains = dict() @@ -211,7 +212,8 @@ def _choose_names_manually(prompt_prefix=""): if retry_message: # We had error in input - retry = z_util(interfaces.IDisplay).yesno(retry_message) + retry = z_util(interfaces.IDisplay).yesno(retry_message, + force_interactive=True) if retry: return _choose_names_manually() else: diff --git a/certbot/display/util.py b/certbot/display/util.py index 47bce87b4..0796a0e94 100644 --- a/certbot/display/util.py +++ b/certbot/display/util.py @@ -1,14 +1,18 @@ """Certbot display.""" +import logging import os import textwrap +import sys import six import zope.interface +from certbot import constants from certbot import interfaces from certbot import errors from certbot.display import completer +logger = logging.getLogger(__name__) WIDTH = 72 @@ -50,19 +54,25 @@ def _wrap_lines(msg): @zope.interface.implementer(interfaces.IDisplay) class FileDisplay(object): """File-based display.""" + # pylint: disable=too-many-arguments + # see https://github.com/certbot/certbot/issues/3915 - def __init__(self, outfile): + def __init__(self, outfile, force_interactive): super(FileDisplay, self).__init__() self.outfile = outfile + self.force_interactive = force_interactive + self.skipped_interaction = False - def notification(self, message, pause=True, wrap=True): - # pylint: disable=unused-argument + def notification(self, message, pause=True, + wrap=True, force_interactive=False): """Displays a notification and waits for user acceptance. :param str message: Message to display :param bool pause: Whether or not the program should pause for the user's confirmation :param bool wrap: Whether or not the application should wrap text + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions """ side_frame = "-" * 79 @@ -72,10 +82,14 @@ class FileDisplay(object): "{line}{frame}{line}{msg}{line}{frame}{line}".format( line=os.linesep, frame=side_frame, msg=message)) if pause: - six.moves.input("Press Enter to Continue") + if self._can_interact(force_interactive): + six.moves.input("Press Enter to Continue") + else: + logger.debug("Not pausing for user confirmation") def menu(self, message, choices, ok_label="", cancel_label="", - help_label="", **unused_kwargs): + help_label="", default=None, + cli_flag=None, force_interactive=False, **unused_kwargs): # pylint: disable=unused-argument """Display a menu. @@ -86,7 +100,10 @@ class FileDisplay(object): :param choices: Menu lines, len must be > 0 :type choices: list of tuples (tag, item) or list of descriptions (tags will be enumerated) - :param dict _kwargs: absorbs default / cli_args + :param default: default value to return (if one exists) + :param str cli_flag: option used to set this value with the CLI + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of (`code`, `index`) where `code` - str display exit code @@ -95,18 +112,25 @@ class FileDisplay(object): :rtype: tuple """ + if self._return_default(message, default, cli_flag, force_interactive): + return OK, default + self._print_menu(message, choices) code, selection = self._get_valid_int_ans(len(choices)) return code, selection - 1 - def input(self, message, **unused_kwargs): + def input(self, message, default=None, + cli_flag=None, force_interactive=False, **unused_kwargs): # pylint: disable=no-self-use """Accept input from the user. :param str message: message to display to the user - :param dict _kwargs: absorbs default / cli_args + :param default: default value to return (if one exists) + :param str cli_flag: option used to set this value with the CLI + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of (`code`, `input`) where `code` - str display exit code @@ -114,6 +138,9 @@ class FileDisplay(object): :rtype: tuple """ + if self._return_default(message, default, cli_flag, force_interactive): + return OK, default + ans = six.moves.input( textwrap.fill( "%s (Enter 'c' to cancel): " % message, @@ -126,7 +153,8 @@ class FileDisplay(object): else: return OK, ans - def yesno(self, message, yes_label="Yes", no_label="No", **unused_kwargs): + def yesno(self, message, yes_label="Yes", no_label="No", default=None, + cli_flag=None, force_interactive=False, **unused_kwargs): """Query the user with a yes/no question. Yes and No label must begin with different letters, and must contain at @@ -135,12 +163,18 @@ class FileDisplay(object): :param str message: question for the user :param str yes_label: Label of the "Yes" parameter :param str no_label: Label of the "No" parameter - :param dict _kwargs: absorbs default / cli_args + :param default: default value to return (if one exists) + :param str cli_flag: option used to set this value with the CLI + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: True for "Yes", False for "No" :rtype: bool """ + if self._return_default(message, default, cli_flag, force_interactive): + return default + side_frame = ("-" * 79) + os.linesep message = _wrap_lines(message) @@ -162,14 +196,18 @@ class FileDisplay(object): ans.startswith(no_label[0].upper())): return False - def checklist(self, message, tags, default_status=True, **unused_kwargs): + def checklist(self, message, tags, default_status=True, default=None, + cli_flag=None, force_interactive=False, **unused_kwargs): # pylint: disable=unused-argument """Display a checklist. :param str message: Message to display to user :param list tags: `str` tags to select, len(tags) > 0 :param bool default_status: Not used for FileDisplay - :param dict _kwargs: absorbs default / cli_args + :param default: default value to return (if one exists) + :param str cli_flag: option used to set this value with the CLI + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of (`code`, `tags`) where `code` - str display exit code @@ -177,6 +215,9 @@ class FileDisplay(object): :rtype: tuple """ + if self._return_default(message, default, cli_flag, force_interactive): + return OK, default + while True: self._print_menu(message, tags) @@ -197,10 +238,65 @@ class FileDisplay(object): else: return code, [] - def directory_select(self, message, **unused_kwargs): + def _return_default(self, prompt, default, cli_flag, force_interactive): + """Should we return the default instead of prompting the user? + + :param str prompt: prompt for the user + :param default: default answer to prompt + :param str cli_flag: command line option for setting an answer + to this question + :param bool force_interactive: if interactivity is forced by the + IDisplay call + + :returns: True if we should return the default without prompting + :rtype: bool + + """ + msg = "Invalid IDisplay call for this prompt:\n{0}".format(prompt) + if cli_flag: + msg += ("\nYou can set an answer to " + "this prompt with the {0} flag".format(cli_flag)) + assert default is not None or force_interactive, msg + + if self._can_interact(force_interactive): + return False + else: + logger.debug( + "Falling back to default %s for the prompt:\n%s", + default, prompt) + return True + + def _can_interact(self, force_interactive): + """Can we safely interact with the user? + + :param bool force_interactive: if interactivity is forced by the + IDisplay call + + :returns: True if the display can interact with the user + :rtype: bool + + """ + if (self.force_interactive or force_interactive or + sys.stdin.isatty() and self.outfile.isatty()): + return True + elif not self.skipped_interaction: + logger.warning( + "Skipped user interaction because Certbot doesn't appear to " + "be running in a terminal. You should probably include " + "--non-interactive or %s on the command line.", + constants.FORCE_INTERACTIVE_FLAG) + self.skipped_interaction = True + return False + + def directory_select(self, message, default=None, cli_flag=None, + force_interactive=False, **unused_kwargs): """Display a directory selection screen. :param str message: prompt to give the user + :param default: default value to return (if one exists) + :param str cli_flag: option used to set this value with the CLI + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of the form (`code`, `string`) where `code` - display exit code @@ -208,7 +304,7 @@ class FileDisplay(object): """ with completer.Completer(): - return self.input(message) + return self.input(message, default, cli_flag, force_interactive) def _scrub_checklist_input(self, indices, tags): # pylint: disable=no-self-use @@ -310,7 +406,7 @@ class FileDisplay(object): class NoninteractiveDisplay(object): """An iDisplay implementation that never asks for interactive user input""" - def __init__(self, outfile): + def __init__(self, outfile, *unused_args, **unused_kwargs): super(NoninteractiveDisplay, self).__init__() self.outfile = outfile @@ -324,7 +420,7 @@ class NoninteractiveDisplay(object): msg += "\n\n(You can set this with the {0} flag)".format(cli_flag) raise errors.MissingCommandlineFlag(msg) - def notification(self, message, pause=False, wrap=True): + def notification(self, message, pause=False, wrap=True, **unused_kwargs): # pylint: disable=unused-argument """Displays a notification without waiting for user acceptance. @@ -341,7 +437,7 @@ class NoninteractiveDisplay(object): line=os.linesep, frame=side_frame, msg=message)) def menu(self, message, choices, ok_label=None, cancel_label=None, - help_label=None, default=None, cli_flag=None): + help_label=None, default=None, cli_flag=None, *unused_kwargs): # pylint: disable=unused-argument,too-many-arguments """Avoid displaying a menu. @@ -364,7 +460,7 @@ class NoninteractiveDisplay(object): return OK, default - def input(self, message, default=None, cli_flag=None): + def input(self, message, default=None, cli_flag=None, **unused_kwargs): """Accept input from the user. :param str message: message to display to the user @@ -381,7 +477,8 @@ class NoninteractiveDisplay(object): else: return OK, default - def yesno(self, message, yes_label=None, no_label=None, default=None, cli_flag=None): + def yesno(self, message, yes_label=None, no_label=None, + default=None, cli_flag=None, **unused_kwargs): # pylint: disable=unused-argument """Decide Yes or No, without asking anybody @@ -398,8 +495,8 @@ class NoninteractiveDisplay(object): else: return default - def checklist(self, message, tags, default=None, cli_flag=None, **kwargs): - # pylint: disable=unused-argument + def checklist(self, message, tags, default=None, + cli_flag=None, **unused_kwargs): """Display a checklist. :param str message: Message to display to user @@ -417,7 +514,8 @@ class NoninteractiveDisplay(object): else: return OK, default - def directory_select(self, message, default=None, cli_flag=None): + def directory_select(self, message, default=None, + cli_flag=None, **unused_kwargs): """Simulate prompting the user for a directory. This function returns default if it is not ``None``, otherwise, diff --git a/certbot/interfaces.py b/certbot/interfaces.py index f2b3faf21..46b53129b 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -361,21 +361,29 @@ class IInstaller(IPlugin): class IDisplay(zope.interface.Interface): """Generic display.""" + # pylint: disable=too-many-arguments + # see https://github.com/certbot/certbot/issues/3915 - def notification(message, pause, wrap=True): + def notification(message, pause, wrap=True, force_interactive=False): """Displays a string message :param str message: Message to display :param bool pause: Whether or not the application should pause for confirmation (if available) :param bool wrap: Whether or not the application should wrap text + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions """ - def menu(message, choices, ok_label="OK", # pylint: disable=too-many-arguments - cancel_label="Cancel", help_label="", default=None, cli_flag=None): + def menu(message, choices, ok_label="OK", + cancel_label="Cancel", help_label="", + default=None, cli_flag=None, force_interactive=False): """Displays a generic menu. + When not setting force_interactive=True, you must provide a + default value. + :param str message: message to display :param choices: choices @@ -386,6 +394,8 @@ class IDisplay(zope.interface.Interface): :param str help_label: label for Help button :param int default: default (non-interactive) choice from the menu :param str cli_flag: to automate choice from the menu, eg "--keep" + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of (`code`, `index`) where `code` - str display exit code @@ -396,10 +406,16 @@ class IDisplay(zope.interface.Interface): """ - def input(message, default=None, cli_args=None): + def input(message, default=None, cli_args=None, force_interactive=False): """Accept input from the user. + When not setting force_interactive=True, you must provide a + default value. + :param str message: message to display to the user + :param str default: default (non-interactive) response to prompt + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of (`code`, `input`) where `code` - str display exit code @@ -412,14 +428,19 @@ class IDisplay(zope.interface.Interface): """ def yesno(message, yes_label="Yes", no_label="No", default=None, - cli_args=None): + cli_args=None, force_interactive=False): """Query the user with a yes/no question. Yes and No label must begin with different letters. + When not setting force_interactive=True, you must provide a + default value. + :param str message: question for the user :param str default: default (non-interactive) choice from the menu :param str cli_flag: to automate choice from the menu, eg "--redirect / --no-redirect" + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: True for "Yes", False for "No" :rtype: bool @@ -429,14 +450,20 @@ class IDisplay(zope.interface.Interface): """ - def checklist(message, tags, default_state, default=None, cli_args=None): + def checklist(message, tags, default_state, + default=None, cli_args=None, force_interactive=False): """Allow for multiple selections from a menu. + When not setting force_interactive=True, you must provide a + default value. + :param str message: message to display to the user :param list tags: where each is of type :class:`str` len(tags) > 0 :param bool default_status: If True, items are in a selected state by default. :param str default: default (non-interactive) state of the checklist :param str cli_flag: to automate choice from the menu, eg "--domains" + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of the form (code, list_tags) where `code` - int display exit code @@ -448,15 +475,21 @@ class IDisplay(zope.interface.Interface): """ - def directory_select(self, message, default=None, cli_flag=None): + def directory_select(self, message, default=None, + cli_flag=None, force_interactive=False): """Display a directory selection screen. + When not setting force_interactive=True, you must provide a + default value. + :param str message: prompt to give the user :param default: the default value to return, if one exists, when using the NoninteractiveDisplay :param str cli_flag: option used to set this value with the CLI, if one exists, to be included in error messages given by NoninteractiveDisplay + :param bool force_interactive: True if it's safe to prompt the user + because it won't cause any workflow regressions :returns: tuple of the form (`code`, `string`) where `code` - int display exit code diff --git a/certbot/main.py b/certbot/main.py index 6a1653193..c0e2f5271 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -139,7 +139,8 @@ def _handle_subset_cert_request(config, domains, cert): br=os.linesep) if config.expand or config.renew_by_default or zope.component.getUtility( interfaces.IDisplay).yesno(question, "Expand", "Cancel", - cli_flag="--expand"): + cli_flag="--expand", + force_interactive=True): return "renew", cert else: reporter_util = zope.component.getUtility(interfaces.IReporter) @@ -188,7 +189,8 @@ def _handle_identical_cert_request(config, lineage): "Renew & replace the cert (limit ~5 per 7 days)"] display = zope.component.getUtility(interfaces.IDisplay) - response = display.menu(question, choices, "OK", "Cancel", default=0) + response = display.menu(question, choices, "OK", "Cancel", + default=0, force_interactive=True) if response[0] == display_util.CANCEL: # TODO: Add notification related to command-line options for # skipping the menu for this case. @@ -365,7 +367,8 @@ def _determine_account(config): "server at {1}".format( regr.terms_of_service, config.server)) obj = zope.component.getUtility(interfaces.IDisplay) - return obj.yesno(msg, "Agree", "Cancel", cli_flag="--agree-tos") + return obj.yesno(msg, "Agree", "Cancel", + cli_flag="--agree-tos", force_interactive=True) try: acc, acme = client.register( @@ -788,7 +791,8 @@ def set_displayer(config): elif config.noninteractive_mode: displayer = display_util.NoninteractiveDisplay(sys.stdout) else: - displayer = display_util.FileDisplay(sys.stdout) + displayer = display_util.FileDisplay(sys.stdout, + config.force_interactive) zope.component.provideUtility(displayer) diff --git a/certbot/plugins/manual.py b/certbot/plugins/manual.py index 5f933f8bc..646b1d340 100644 --- a/certbot/plugins/manual.py +++ b/certbot/plugins/manual.py @@ -251,7 +251,8 @@ s.serve_forever()" """ if not (self.conf("test-mode") or self.conf("public-ip-logging-ok")): if not zope.component.getUtility(interfaces.IDisplay).yesno( self.IP_DISCLAIMER, "Yes", "No", - cli_flag="--manual-public-ip-logging-ok"): + cli_flag="--manual-public-ip-logging-ok", + force_interactive=True): raise errors.PluginError("Must agree to IP logging to proceed") else: self.config.namespace.manual_public_ip_logging_ok = True diff --git a/certbot/plugins/selection.py b/certbot/plugins/selection.py index ed0991a89..16932232a 100644 --- a/certbot/plugins/selection.py +++ b/certbot/plugins/selection.py @@ -111,7 +111,8 @@ def choose_plugin(prepared, question): while True: disp = z_util(interfaces.IDisplay) - code, index = disp.menu(question, opts, help_label="More Info") + code, index = disp.menu( + question, opts, help_label="More Info", force_interactive=True) if code == display_util.OK: plugin_ep = prepared[index] @@ -127,7 +128,8 @@ def choose_plugin(prepared, question): msg = "Reported Error: %s" % prepared[index].prepare() else: msg = prepared[index].init().more_info() - z_util(interfaces.IDisplay).notification(msg) + z_util(interfaces.IDisplay).notification(msg, + force_interactive=True) else: return None diff --git a/certbot/plugins/selection_test.py b/certbot/plugins/selection_test.py index 001ca5cff..c0494e565 100644 --- a/certbot/plugins/selection_test.py +++ b/certbot/plugins/selection_test.py @@ -110,7 +110,8 @@ class ChoosePluginTest(unittest.TestCase): """Tests for certbot.plugins.selection.choose_plugin.""" def setUp(self): - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) self.mock_apache = mock.Mock( description_with_name="a", misconfigured=True) self.mock_stand = mock.Mock( diff --git a/certbot/plugins/standalone.py b/certbot/plugins/standalone.py index e8c11a416..4fc52479f 100644 --- a/certbot/plugins/standalone.py +++ b/certbot/plugins/standalone.py @@ -243,13 +243,13 @@ class Authenticator(common.Plugin): "Could not bind TCP port {0} because you don't have " "the appropriate permissions (for example, you " "aren't running this program as " - "root).".format(error.port)) + "root).".format(error.port), force_interactive=True) elif error.socket_error.errno == socket.errno.EADDRINUSE: display.notification( "Could not bind TCP port {0} because it is already in " "use by another process on this system (such as a web " "server). Please stop the program in question and then " - "try again.".format(error.port)) + "try again.".format(error.port), force_interactive=True) else: raise # XXX: How to handle unknown errors in binding? diff --git a/certbot/plugins/util.py b/certbot/plugins/util.py index 8f6a62a7f..1964ae349 100644 --- a/certbot/plugins/util.py +++ b/certbot/plugins/util.py @@ -103,7 +103,7 @@ def already_listening_socket(port, renewer=False): "Port {0} is already in use by another process. This will " "prevent us from binding to that port. Please stop the " "process that is populating the port in question and try " - "again. {1}".format(port, extra)) + "again. {1}".format(port, extra), force_interactive=True) return True finally: testsocket.close() @@ -151,7 +151,8 @@ def already_listening_psutil(port, renewer=False): "The program {0} (process ID {1}) is already listening " "on TCP port {2}. This will prevent us from binding to " "that port. Please stop the {0} program temporarily " - "and then try again.{3}".format(name, pid, port, extra)) + "and then try again.{3}".format(name, pid, port, extra), + force_interactive=True) return True except (psutil.NoSuchProcess, psutil.AccessDenied): # Perhaps the result of a race where the process could have diff --git a/certbot/plugins/webroot.py b/certbot/plugins/webroot.py index e9c3bcdda..09671f989 100644 --- a/certbot/plugins/webroot.py +++ b/certbot/plugins/webroot.py @@ -129,7 +129,8 @@ to serve all files under specified web root ({0}).""" "public_html or webroot directory. The webroot " "plugin works by temporarily saving necessary " "resources in the HTTP server's webroot directory " - "to pass domain validation challenges.") + "to pass domain validation challenges.", + force_interactive=True) else: # code == display_util.OK return None if index == 0 else known_webroots[index - 1] @@ -138,7 +139,8 @@ to serve all files under specified web root ({0}).""" while True: code, webroot = display.directory_select( - "Input the webroot for {0}:".format(domain)) + "Input the webroot for {0}:".format(domain), + force_interactive=True) if code == display_util.HELP: # Displaying help is not currently implemented return None diff --git a/certbot/reverter.py b/certbot/reverter.py index 714a38b8b..32355782e 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -181,7 +181,7 @@ class Reverter(object): if for_logging: return os.linesep.join(output) zope.component.getUtility(interfaces.IDisplay).notification( - os.linesep.join(output)) + os.linesep.join(output), force_interactive=True) def add_to_temp_checkpoint(self, save_files, save_notes): """Add files to temporary checkpoint. diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 2755d992c..e3bd28a5e 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -262,6 +262,12 @@ class ParseTest(unittest.TestCase): self.assertFalse(cli.option_was_set( config_dir_option, cli.flag_default(config_dir_option))) + def test_force_interactive(self): + self.assertRaises( + errors.Error, self.parse, "renew --force-interactive".split()) + self.assertRaises( + errors.Error, self.parse, "-n --force-interactive".split()) + class DefaultTest(unittest.TestCase): """Tests for certbot.cli._Default.""" diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index 1b535bf3a..e2735cbde 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -84,7 +84,8 @@ class GetEmailTest(unittest.TestCase): class ChooseAccountTest(unittest.TestCase): """Tests for certbot.display.ops.choose_account.""" def setUp(self): - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) self.accounts_dir = tempfile.mkdtemp("accounts") self.account_keys_dir = os.path.join(self.accounts_dir, "keys") @@ -127,7 +128,8 @@ class ChooseAccountTest(unittest.TestCase): class GenSSLLabURLs(unittest.TestCase): """Loose test of _gen_ssl_lab_urls. URL can change easily in the future.""" def setUp(self): - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) @classmethod def _call(cls, domains): @@ -146,7 +148,8 @@ class GenSSLLabURLs(unittest.TestCase): class GenHttpsNamesTest(unittest.TestCase): """Test _gen_https_names.""" def setUp(self): - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) @classmethod def _call(cls, domains): @@ -193,7 +196,8 @@ class GenHttpsNamesTest(unittest.TestCase): class ChooseNamesTest(unittest.TestCase): """Test choose names.""" def setUp(self): - zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, + False)) self.mock_install = mock.MagicMock() @classmethod diff --git a/certbot/tests/display/util_test.py b/certbot/tests/display/util_test.py index fa1cb89ba..51fbc2828 100644 --- a/certbot/tests/display/util_test.py +++ b/certbot/tests/display/util_test.py @@ -20,10 +20,11 @@ class FileOutputDisplayTest(unittest.TestCase): functions look to a user, uncomment the test_visual function. """ + # pylint:disable=too-many-public-methods def setUp(self): super(FileOutputDisplayTest, self).setUp() self.mock_stdout = mock.MagicMock() - self.displayer = display_util.FileDisplay(self.mock_stdout) + self.displayer = display_util.FileDisplay(self.mock_stdout, False) def test_notification_no_pause(self): self.displayer.notification("message", False) @@ -33,56 +34,84 @@ class FileOutputDisplayTest(unittest.TestCase): def test_notification_pause(self): with mock.patch("six.moves.input", return_value="enter"): - self.displayer.notification("message") + self.displayer.notification("message", force_interactive=True) self.assertTrue("message" in self.mock_stdout.write.call_args[0][0]) + def test_notification_noninteractive(self): + self._force_noninteractive(self.displayer.notification, "message") + string = self.mock_stdout.write.call_args[0][0] + self.assertTrue("message" in string) + @mock.patch("certbot.display.util." "FileDisplay._get_valid_int_ans") def test_menu(self, mock_ans): mock_ans.return_value = (display_util.OK, 1) - ret = self.displayer.menu("message", CHOICES) + ret = self.displayer.menu("message", CHOICES, force_interactive=True) self.assertEqual(ret, (display_util.OK, 0)) def test_input_cancel(self): with mock.patch("six.moves.input", return_value="c"): - code, _ = self.displayer.input("message") + code, _ = self.displayer.input("message", force_interactive=True) self.assertTrue(code, display_util.CANCEL) def test_input_normal(self): with mock.patch("six.moves.input", return_value="domain.com"): - code, input_ = self.displayer.input("message") + code, input_ = self.displayer.input("message", force_interactive=True) self.assertEqual(code, display_util.OK) self.assertEqual(input_, "domain.com") + def test_input_noninteractive(self): + default = "foo" + code, input_ = self._force_noninteractive( + self.displayer.input, "message", default=default) + + self.assertEqual(code, display_util.OK) + self.assertEqual(input_, default) + + def test_input_assertion_fail(self): + self.assertRaises(AssertionError, self._force_noninteractive, + self.displayer.input, "message", cli_flag="--flag") + def test_yesno(self): with mock.patch("six.moves.input", return_value="Yes"): - self.assertTrue(self.displayer.yesno("message")) + self.assertTrue(self.displayer.yesno( + "message", force_interactive=True)) with mock.patch("six.moves.input", return_value="y"): - self.assertTrue(self.displayer.yesno("message")) + self.assertTrue(self.displayer.yesno( + "message", force_interactive=True)) with mock.patch("six.moves.input", side_effect=["maybe", "y"]): - self.assertTrue(self.displayer.yesno("message")) + self.assertTrue(self.displayer.yesno( + "message", force_interactive=True)) with mock.patch("six.moves.input", return_value="No"): - self.assertFalse(self.displayer.yesno("message")) + self.assertFalse(self.displayer.yesno( + "message", force_interactive=True)) with mock.patch("six.moves.input", side_effect=["cancel", "n"]): - self.assertFalse(self.displayer.yesno("message")) + self.assertFalse(self.displayer.yesno( + "message", force_interactive=True)) with mock.patch("six.moves.input", return_value="a"): - self.assertTrue(self.displayer.yesno("msg", yes_label="Agree")) + self.assertTrue(self.displayer.yesno( + "msg", yes_label="Agree", force_interactive=True)) + + def test_yesno_noninteractive(self): + self.assertTrue(self._force_noninteractive( + self.displayer.yesno, "message", default=True)) @mock.patch("certbot.display.util.FileDisplay.input") def test_checklist_valid(self, mock_input): mock_input.return_value = (display_util.OK, "2 1") - code, tag_list = self.displayer.checklist("msg", TAGS) + code, tag_list = self.displayer.checklist( + "msg", TAGS, force_interactive=True) self.assertEqual( (code, set(tag_list)), (display_util.OK, set(["tag1", "tag2"]))) @mock.patch("certbot.display.util.FileDisplay.input") def test_checklist_empty(self, mock_input): mock_input.return_value = (display_util.OK, "") - code, tag_list = self.displayer.checklist("msg", TAGS) + code, tag_list = self.displayer.checklist("msg", TAGS, force_interactive=True) self.assertEqual( (code, set(tag_list)), (display_util.OK, set(["tag1", "tag2", "tag3"]))) @@ -94,7 +123,7 @@ class FileOutputDisplayTest(unittest.TestCase): (display_util.OK, "1") ] - ret = self.displayer.checklist("msg", TAGS) + ret = self.displayer.checklist("msg", TAGS, force_interactive=True) self.assertEqual(ret, (display_util.OK, ["tag1"])) @mock.patch("certbot.display.util.FileDisplay.input") @@ -103,9 +132,17 @@ class FileOutputDisplayTest(unittest.TestCase): (display_util.OK, "10"), (display_util.CANCEL, "1") ] - ret = self.displayer.checklist("msg", TAGS) + ret = self.displayer.checklist("msg", TAGS, force_interactive=True) self.assertEqual(ret, (display_util.CANCEL, [])) + def test_checklist_noninteractive(self): + default = TAGS + code, input_ = self._force_noninteractive( + self.displayer.checklist, "msg", TAGS, default=default) + + self.assertEqual(code, display_util.OK) + self.assertEqual(input_, default) + def test_scrub_checklist_input_valid(self): # pylint: disable=protected-access indices = [ @@ -125,12 +162,36 @@ class FileOutputDisplayTest(unittest.TestCase): @mock.patch("certbot.display.util.FileDisplay.input") def test_directory_select(self, mock_input): - message = "msg" - result = (display_util.OK, "/var/www/html",) + # pylint: disable=star-args + args = ["msg", "/var/www/html", "--flag", True] + result = (display_util.OK, "/var/www/html") mock_input.return_value = result - self.assertEqual(self.displayer.directory_select(message), result) - mock_input.assert_called_once_with(message) + self.assertEqual(self.displayer.directory_select(*args), result) + mock_input.assert_called_once_with(*args) + + def test_directory_select_noninteractive(self): + default = "/var/www/html" + code, input_ = self._force_noninteractive( + self.displayer.directory_select, "msg", default=default) + + self.assertEqual(code, display_util.OK) + self.assertEqual(input_, default) + + def _force_noninteractive(self, func, *args, **kwargs): + skipped_interaction = self.displayer.skipped_interaction + + with mock.patch("certbot.display.util.sys.stdin") as mock_stdin: + mock_stdin.isatty.return_value = False + with mock.patch("certbot.display.util.logger") as mock_logger: + result = func(*args, **kwargs) + + if skipped_interaction: + self.assertFalse(mock_logger.warning.called) + else: + self.assertEqual(mock_logger.warning.call_count, 1) + + return result def test_scrub_checklist_input_invalid(self): # pylint: disable=protected-access diff --git a/tests/display.py b/tests/display.py index 7400788a3..1f548e33d 100644 --- a/tests/display.py +++ b/tests/display.py @@ -18,5 +18,5 @@ def test_visual(displayer, choices): if __name__ == "__main__": - displayer = util.FileDisplay(sys.stdout): + displayer = util.FileDisplay(sys.stdout, False) test_visual(displayer, util_test.CHOICES) From acc501d3a118e84035e7b3a9fed531ca304f2b9b Mon Sep 17 00:00:00 2001 From: Lior Sabag Date: Mon, 19 Dec 2016 22:49:27 +0200 Subject: [PATCH 10/44] Fix typo (#3932) --- docs/using.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/using.rst b/docs/using.rst index 7764408bf..0aeed58b9 100644 --- a/docs/using.rst +++ b/docs/using.rst @@ -233,7 +233,7 @@ certificate that contains all of the old domains and one or more additional new domains. ``--allow-subset-of-names`` tells Certbot to continue with cert generation if -only some of the specified domain authorazations can be obtained. This may +only some of the specified domain authorizations can be obtained. This may be useful if some domains specified in a certificate no longer point at this system. From 6a933f1de36233b23dec5f7ca20e736288b6b8d4 Mon Sep 17 00:00:00 2001 From: Craig Smith Date: Tue, 20 Dec 2016 12:32:05 +1030 Subject: [PATCH 11/44] Changed plugin interface return types (#3748). (#3780) --- certbot/interfaces.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 46b53129b..2df2abfe8 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -138,15 +138,15 @@ class IAuthenticator(IPlugin): """ def get_chall_pref(domain): - """Return list of challenge preferences. + """Return `collections.Iterable` of challenge preferences. :param str domain: Domain for which challenge preferences are sought. - :returns: List of challenge types (subclasses of + :returns: `collections.Iterable` of challenge types (subclasses of :class:`acme.challenges.Challenge`) with the most preferred challenges first. If a type is not specified, it means the Authenticator cannot perform the challenge. - :rtype: list + :rtype: `collections.Iterable` """ @@ -158,7 +158,7 @@ class IAuthenticator(IPlugin): instances, such that it contains types found within :func:`get_chall_pref` only. - :returns: List of ACME + :returns: `collections.Iterable` of ACME :class:`~acme.challenges.ChallengeResponse` instances or if the :class:`~acme.challenges.Challenge` cannot be fulfilled then: @@ -168,7 +168,7 @@ class IAuthenticator(IPlugin): ``False`` Authenticator will never be able to perform (error). - :rtype: :class:`list` of + :rtype: :class:`collections.Iterable` of :class:`acme.challenges.ChallengeResponse`, where responses are required to be returned in the same order as corresponding input challenges @@ -254,7 +254,7 @@ class IInstaller(IPlugin): def get_all_names(): """Returns all names that may be authenticated. - :rtype: `list` of `str` + :rtype: `collections.Iterable` of `str` """ @@ -289,11 +289,11 @@ class IInstaller(IPlugin): """ def supported_enhancements(): - """Returns a list of supported enhancements. + """Returns a `collections.Iterable` of supported enhancements. :returns: supported enhancements which should be a subset of :const:`~certbot.constants.ENHANCEMENTS` - :rtype: :class:`list` of :class:`str` + :rtype: :class:`collections.Iterable` of :class:`str` """ From f92254769bbf60c3777a2abe337c470a9edb7d5f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 20 Dec 2016 14:34:12 -0800 Subject: [PATCH 12/44] I promise checklists are OK (fixes #3934) (#3940) * TIL checklist calls input * full coverage on certbot/display/util.py * improve no double warning test --- certbot/display/util.py | 3 +- certbot/tests/display/util_test.py | 53 +++++++++++++++++++----------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/certbot/display/util.py b/certbot/display/util.py index 0796a0e94..ebfce78e2 100644 --- a/certbot/display/util.py +++ b/certbot/display/util.py @@ -223,7 +223,8 @@ class FileDisplay(object): code, ans = self.input("Select the appropriate numbers separated " "by commas and/or spaces, or leave input " - "blank to select all options shown") + "blank to select all options shown", + force_interactive=True) if code == OK: if len(ans.strip()) == 0: diff --git a/certbot/tests/display/util_test.py b/certbot/tests/display/util_test.py index 51fbc2828..7b6bd842e 100644 --- a/certbot/tests/display/util_test.py +++ b/certbot/tests/display/util_test.py @@ -43,6 +43,19 @@ class FileOutputDisplayTest(unittest.TestCase): string = self.mock_stdout.write.call_args[0][0] self.assertTrue("message" in string) + def test_notification_noninteractive2(self): + # The main purpose of this test is to make sure we only call + # logger.warning once which _force_noninteractive checks internally + self._force_noninteractive(self.displayer.notification, "message") + string = self.mock_stdout.write.call_args[0][0] + self.assertTrue("message" in string) + + self.assertTrue(self.displayer.skipped_interaction) + + self._force_noninteractive(self.displayer.notification, "message2") + string = self.mock_stdout.write.call_args[0][0] + self.assertTrue("message2" in string) + @mock.patch("certbot.display.util." "FileDisplay._get_valid_int_ans") def test_menu(self, mock_ans): @@ -50,6 +63,12 @@ class FileOutputDisplayTest(unittest.TestCase): ret = self.displayer.menu("message", CHOICES, force_interactive=True) self.assertEqual(ret, (display_util.OK, 0)) + def test_menu_noninteractive(self): + default = 0 + result = self._force_noninteractive( + self.displayer.menu, "msg", CHOICES, default=default) + self.assertEqual(result, (display_util.OK, default)) + def test_input_cancel(self): with mock.patch("six.moves.input", return_value="c"): code, _ = self.displayer.input("message", force_interactive=True) @@ -100,38 +119,32 @@ class FileOutputDisplayTest(unittest.TestCase): self.assertTrue(self._force_noninteractive( self.displayer.yesno, "message", default=True)) - @mock.patch("certbot.display.util.FileDisplay.input") + @mock.patch("certbot.display.util.six.moves.input") def test_checklist_valid(self, mock_input): - mock_input.return_value = (display_util.OK, "2 1") + mock_input.return_value = "2 1" code, tag_list = self.displayer.checklist( "msg", TAGS, force_interactive=True) self.assertEqual( (code, set(tag_list)), (display_util.OK, set(["tag1", "tag2"]))) - @mock.patch("certbot.display.util.FileDisplay.input") + @mock.patch("certbot.display.util.six.moves.input") def test_checklist_empty(self, mock_input): - mock_input.return_value = (display_util.OK, "") + mock_input.return_value = "" code, tag_list = self.displayer.checklist("msg", TAGS, force_interactive=True) self.assertEqual( (code, set(tag_list)), (display_util.OK, set(["tag1", "tag2", "tag3"]))) - @mock.patch("certbot.display.util.FileDisplay.input") + @mock.patch("certbot.display.util.six.moves.input") def test_checklist_miss_valid(self, mock_input): - mock_input.side_effect = [ - (display_util.OK, "10"), - (display_util.OK, "tag1 please"), - (display_util.OK, "1") - ] + mock_input.side_effect = ["10", "tag1 please", "1"] ret = self.displayer.checklist("msg", TAGS, force_interactive=True) self.assertEqual(ret, (display_util.OK, ["tag1"])) - @mock.patch("certbot.display.util.FileDisplay.input") + @mock.patch("certbot.display.util.six.moves.input") def test_checklist_miss_quit(self, mock_input): - mock_input.side_effect = [ - (display_util.OK, "10"), - (display_util.CANCEL, "1") - ] + mock_input.side_effect = ["10", "c"] + ret = self.displayer.checklist("msg", TAGS, force_interactive=True) self.assertEqual(ret, (display_util.CANCEL, [])) @@ -160,15 +173,15 @@ class FileOutputDisplayTest(unittest.TestCase): self.displayer._scrub_checklist_input(list_, TAGS)) self.assertEqual(set_tags, exp[i]) - @mock.patch("certbot.display.util.FileDisplay.input") + @mock.patch("certbot.display.util.six.moves.input") def test_directory_select(self, mock_input): # pylint: disable=star-args args = ["msg", "/var/www/html", "--flag", True] - result = (display_util.OK, "/var/www/html") - mock_input.return_value = result + user_input = "/var/www/html" + mock_input.return_value = user_input - self.assertEqual(self.displayer.directory_select(*args), result) - mock_input.assert_called_once_with(*args) + returned = self.displayer.directory_select(*args) + self.assertEqual(returned, (display_util.OK, user_input)) def test_directory_select_noninteractive(self): default = "/var/www/html" From 28ce10fef5d88fe662bff05f986903685bc16cb1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 20 Dec 2016 15:53:52 -0800 Subject: [PATCH 13/44] Don't add ServerAlias directives when the domain is already covered by a wildcard (#3917) * correctly match * and ? in ServerAlias directives * update Apache wildcard test * Consolidate wildcard matching and remove bad test * Test Apache vhost selection with wildcards * Added few more tests to proof vhost selection --- certbot-apache/certbot_apache/configurator.py | 49 ++++++++++++++----- .../certbot_apache/tests/configurator_test.py | 42 +++++++++++++--- .../sites-available/another_wildcard.conf | 11 +++++ .../apache2/sites-available/wildcard.conf | 11 +++++ 4 files changed, 95 insertions(+), 18 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index b200d5eaa..27e214362 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -1,6 +1,7 @@ """Apache Configuration based off of Augeas Configurator.""" # pylint: disable=too-many-lines import filecmp +import fnmatch import logging import os import re @@ -362,18 +363,24 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return vhost 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("*")] - for wildcard in wildcards: - if len(wildcard) > len(target_name): - continue - for idx, segment in enumerate(wildcard[::-1]): - if segment != target_name[idx]: - break - else: - # https://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops + """Is target_name covered by a wildcard? + + :param names: server aliases + :type names: `collections.Iterable` of `str` + :param str target_name: name to compare with wildcards + + :returns: True if target_name is covered by a wildcard, + otherwise, False + :rtype: bool + + """ + # use lowercase strings because fnmatch can be case sensitive + target_name = target_name.lower() + for name in names: + name = name.lower() + # fnmatch treats "[seq]" specially and [ or ] characters aren't + # valid in Apache but Apache doesn't error out if they are present + if "[" not in name and fnmatch.fnmatch(target_name, name): return True return False @@ -1012,6 +1019,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.parser.find_dir("ServerAlias", target_name, start=vh_path, exclude=False)): return + if self._has_matching_wildcard(vh_path, target_name): + return if not self.parser.find_dir("ServerName", None, start=vh_path, exclude=False): self.parser.add_dir(vh_path, "ServerName", target_name) @@ -1019,6 +1028,22 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.parser.add_dir(vh_path, "ServerAlias", target_name) self._add_servernames(vhost) + def _has_matching_wildcard(self, vh_path, target_name): + """Is target_name already included in a wildcard in the vhost? + + :param str vh_path: Augeas path to the vhost + :param str target_name: name to compare with wildcards + + :returns: True if there is a wildcard covering target_name in + the vhost in vhost_path, otherwise, False + :rtype: bool + + """ + matches = self.parser.find_dir( + "ServerAlias", start=vh_path, exclude=False) + aliases = (self.aug.get(match) for match in matches) + return self.included_in_wildcard(aliases, target_name) + def _add_name_vhost_if_necessary(self, vhost): """Add NameVirtualHost Directives if necessary for new vhost. diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 2f1d01315..1af425824 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -220,10 +220,6 @@ class MultipleVhostsTest(util.ApacheTest): self.assertRaises( errors.PluginError, self.config.choose_vhost, "none.com") - def test_choosevhost_select_vhost_with_wildcard(self): - chosen_vhost = self.config.choose_vhost("blue.purple.com", temp=True) - self.assertEqual(self.vh_truth[6], chosen_vhost) - def test_findbest_continues_on_short_domain(self): # pylint: disable=protected-access chosen_vhost = self.config._find_best_vhost("purple.com") @@ -1255,8 +1251,6 @@ class AugeasVhostsTest(util.ApacheTest): self.config = util.get_apache_configurator( self.config_path, self.vhost_path, self.config_dir, self.work_dir) - self.vh_truth = util.get_vh_truth( - self.temp_dir, "debian_apache_2_4/augeas_vhosts") def tearDown(self): shutil.rmtree(self.temp_dir) @@ -1281,5 +1275,41 @@ class AugeasVhostsTest(util.ApacheTest): vhs = self.config.get_virtual_hosts() self.assertEqual([], vhs) + def test_choose_vhost_with_matching_wildcard(self): + names = ( + "an.example.net", "another.example.net", "an.other.example.net") + for name in names: + self.assertFalse(name in self.config.choose_vhost(name).aliases) + + def test_choose_vhost_without_matching_wildcard(self): + mock_path = "certbot_apache.display_ops.select_vhost" + with mock.patch(mock_path, lambda _, vhosts: vhosts[0]): + for name in ("a.example.net", "other.example.net"): + self.assertTrue(name in self.config.choose_vhost(name).aliases) + + def test_choose_vhost_wildcard_not_found(self): + mock_path = "certbot_apache.display_ops.select_vhost" + names = ( + "abc.example.net", "not.there.tld", "aa.wildcard.tld" + ) + with mock.patch(mock_path) as mock_select: + mock_select.return_value = self.config.vhosts[0] + for name in names: + orig_cc = mock_select.call_count + self.config.choose_vhost(name) + self.assertEqual(mock_select.call_count - orig_cc, 1) + + def test_choose_vhost_wildcard_found(self): + mock_path = "certbot_apache.display_ops.select_vhost" + names = ( + "ab.example.net", "a.wildcard.tld", "yetanother.example.net" + ) + with mock.patch(mock_path) as mock_select: + mock_select.return_value = self.config.vhosts[0] + for name in names: + self.config.choose_vhost(name) + self.assertEqual(mock_select.call_count, 0) + + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf new file mode 100644 index 000000000..1a5b7de47 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf @@ -0,0 +1,11 @@ + + ServerName wildcard.tld + ServerAlias ?.wildcard.tld + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf new file mode 100644 index 000000000..b8046e6c9 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf @@ -0,0 +1,11 @@ + + ServerName example.net + ServerAlias ??.example.net *.other.example.net *another.example.net + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet From 00e143d369dbf5a14497009a2cda37a120c8ac6d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 20 Dec 2016 16:24:33 -0800 Subject: [PATCH 14/44] Serialize coverage tests (#3919) * Serialize coverage tests * add py27_install env * Separate cover from integration tests * Add docker to py27 integration tests --- .travis.yml | 14 ++++++++------ tox.cover.sh | 5 ++--- tox.ini | 5 +++++ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/.travis.yml b/.travis.yml index a42e41352..3a9a994a9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,12 +17,7 @@ env: matrix: include: - python: "2.7" - env: TOXENV=cover BOULDER_INTEGRATION=1 - sudo: required - after_failure: - - sudo cat /var/log/mysql/error.log - - ps aux | grep mysql - services: docker + env: TOXENV=cover - python: "2.7" env: TOXENV=lint - python: "2.7" @@ -46,6 +41,13 @@ matrix: - sudo cat /var/log/mysql/error.log - ps aux | grep mysql services: docker + - python: "2.7" + env: TOXENV=py27_install BOULDER_INTEGRATION=1 + sudo: required + after_failure: + - sudo cat /var/log/mysql/error.log + - ps aux | grep mysql + services: docker - sudo: required env: TOXENV=apache_compat services: docker diff --git a/tox.cover.sh b/tox.cover.sh index d138a98e5..7243c4708 100755 --- a/tox.cover.sh +++ b/tox.cover.sh @@ -36,9 +36,8 @@ cover () { # specific package, positional argument scopes tests only to # specific package directory; --cover-tests makes sure every tests # is run (c.f. #403) - nosetests -c /dev/null --with-cover --cover-tests --cover-package \ - "$1" --cover-min-percentage="$min" "$1" --processes=-1 \ - --process-timeout=100 + nosetests -c /dev/null --with-cover --cover-tests --cover-package \ + "$1" --cover-min-percentage="$min" "$1" } rm -f .coverage # --cover-erase is off, make sure stats are correct diff --git a/tox.ini b/tox.ini index 1d82092f6..959f44a8d 100644 --- a/tox.ini +++ b/tox.ini @@ -63,6 +63,11 @@ commands = pip install -e .[dev] nosetests -v certbot --processes=-1 --process-timeout=100 +[testenv:py27_install] +basepython = python2.7 +commands = + pip install -e acme[dns,dev] -e .[dev] -e certbot-apache -e certbot-nginx -e letshelp-certbot + [testenv:cover] basepython = python2.7 commands = From 8ebca1c0526962058c17744f3564ee4149ea92f6 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Dec 2016 17:17:01 -0800 Subject: [PATCH 15/44] Return domains for _find_domains_or_certname (#3937) * Return domains for _find_domains_or_certname * Revamp find_domains_or_certname --- certbot/main.py | 15 ++++++++++++--- certbot/tests/main_test.py | 18 +++++++++++++++--- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index c0e2f5271..838416822 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -284,17 +284,26 @@ def _find_domains_or_certname(config, installer): """Retrieve domains and certname from config or user input. """ domains = None + certname = config.certname + # first, try to get domains from the config if config.domains: domains = config.domains - elif not config.certname: + # if we can't do that but we have a certname, get the domains + # with that certname + elif certname: + domains = cert_manager.domains_for_certname(config, certname) + + # that certname might not have existed, or there was a problem. + # try to get domains from the user. + if not domains: domains = display_ops.choose_names(installer) - if not domains and not config.certname: + if not domains and not certname: raise errors.Error("Please specify --domains, or --installer that " "will help in domain names autodiscovery, or " "--cert-name for an existing certificate name.") - return domains, config.certname + return domains, certname def _report_new_cert(config, cert_path, fullchain_path): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 3665f09bb..673952b4e 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -159,10 +159,12 @@ class ObtainCertTest(unittest.TestCase): self.assertRaises(errors.ConfigurationError, self._call, ('certonly --webroot -d example.com -d test.com --cert-name example.com').split()) + @mock.patch('certbot.cert_manager.domains_for_certname') + @mock.patch('certbot.display.ops.choose_names') @mock.patch('certbot.cert_manager.lineage_for_certname') @mock.patch('certbot.main._report_new_cert') def test_find_lineage_for_domains_new_certname(self, mock_report_cert, - mock_lineage): + mock_lineage, mock_choose_names, mock_domains_for_certname): mock_lineage.return_value = None # no lineage with this name but we specified domains so create a new cert @@ -172,8 +174,10 @@ class ObtainCertTest(unittest.TestCase): self.assertTrue(mock_report_cert.call_count == 1) # no lineage with this name and we didn't give domains - self.assertRaises(errors.ConfigurationError, self._call, - ('certonly --webroot --cert-name example.com').split()) + mock_choose_names.return_value = ["somename"] + mock_domains_for_certname.return_value = None + self._call(('certonly --webroot --cert-name example.com').split()) + self.assertTrue(mock_choose_names.called) class FindDomainsOrCertnameTest(unittest.TestCase): """Tests for certbot.main._find_domains_or_certname.""" @@ -193,6 +197,14 @@ class FindDomainsOrCertnameTest(unittest.TestCase): # pylint: disable=protected-access self.assertRaises(errors.Error, main._find_domains_or_certname, mock_config, None) + @mock.patch('certbot.cert_manager.domains_for_certname') + def test_grab_domains(self, mock_domains): + mock_config = mock.Mock(domains=None, certname="one.com") + mock_domains.return_value = ["one.com", "two.com"] + # pylint: disable=protected-access + self.assertEqual(main._find_domains_or_certname(mock_config, None), + (["one.com", "two.com"], "one.com")) + class RevokeTest(unittest.TestCase): """Tests for certbot.main.revoke.""" From 44d5886429c37c0dbb76a8ed60e99773e1c75135 Mon Sep 17 00:00:00 2001 From: Tan Jay Jun Date: Thu, 22 Dec 2016 06:21:52 +0800 Subject: [PATCH 16/44] Add missing comma to documentation for 'renew' subcommand (#3945) --- docs/cli-help.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cli-help.txt b/docs/cli-help.txt index cf93daa0e..279b65219 100644 --- a/docs/cli-help.txt +++ b/docs/cli-help.txt @@ -1,4 +1,4 @@ -usage: +usage: certbot [SUBCOMMAND] [options] [-d domain] [-d domain] ... Certbot can obtain and install HTTPS/TLS/SSL certificates. By default, @@ -178,7 +178,7 @@ renew: The 'renew' subcommand will attempt to renew all certificates (or more precisely, certificate lineages) you have previously obtained if they are close to expiry, and print a summary of the results. By default, 'renew' - will reuse the options used to create obtain or most recently successfully + will reuse the options 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. Hooks are available to run commands before and From 15d2a0ffde27f9563ca272d78629b7d23e99d685 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Wed, 21 Dec 2016 14:36:51 -0800 Subject: [PATCH 17/44] Import OCSP code from the historical cert_manager branch (This is pde committing jdkasten's code) --- certbot/ocsp.py | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 certbot/ocsp.py diff --git a/certbot/ocsp.py b/certbot/ocsp.py new file mode 100644 index 000000000..d7fc06e0d --- /dev/null +++ b/certbot/ocsp.py @@ -0,0 +1,59 @@ +import logging + +from letsencrypt import errors +from letsencrypt import le_util + + +logger = logging.getLogger(__name__) + + +REV_LABEL = "**Revoked**" +EXP_LABEL = "**Expired**" + +def revoked_status(cert_path, chain_path): + """Get revoked status for a particular cert version. + + .. todo:: Make this a non-blocking call + + :param str cert_path: Path to certificate + :param str chain_path: Path to chain certificate + + """ + url, _ = le_util.run_script( + ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) + + url = url.rstrip() + host = url.partition("://")[2].rstrip("/") + if not host: + raise errors.Error( + "Unable to get OCSP host from cert, url - %s", url) + + # This was a PITA... + # Thanks to "Bulletproof SSL and TLS - Ivan Ristic" for helping me out + try: + output, _ = le_util.run_script( + ["openssl", "ocsp", + "-no_nonce", "-header", "Host", host, + "-issuer", chain_path, + "-cert", cert_path, + "-url", url, + "-CAfile", chain_path]) + except errors.SubprocessError: + return "(OCSP Failure)" + + return _translate_ocsp_query(cert_path, output) + + +def _translate_ocsp_query(cert_path, ocsp_output): + """Returns a label string out of the query.""" + if not "Response verify OK": + return "Revocation Unknown" + if cert_path + ": good" in ocsp_output: + return "" + elif cert_path + ": revoked" in ocsp_output: + return REV_LABEL + else: + raise errors.Error( + "Unable to properly parse OCSP output: %s", ocsp_output) + + From 40e29bb95f5ee64f55bea25be94a3b3341c99b53 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 14:38:20 -0800 Subject: [PATCH 18/44] begin implementing OCSP checking for "certificates" --- certbot/cert_manager.py | 8 ++++++++ certbot/ocsp.py | 14 ++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 35b12e1bb..1b6d441c7 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -8,6 +8,7 @@ import zope.component from certbot import errors from certbot import interfaces +from certbot import ocsp from certbot import storage from certbot import util @@ -170,11 +171,17 @@ def _report_human_readable(parsed_certs): certinfo = [] for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) + expiration_text = "" if cert.is_test_cert: expiration_text = "INVALID: TEST CERT" elif cert.target_expiry <= now: expiration_text = "INVALID: EXPIRED" else: + revoked = ocsp.revoked_status(cert.cert, cert.chain) + if revoked: + expiration_text = "INVALID: " + revoked + + if not expiration_text: diff = cert.target_expiry - now if diff.days == 1: expiration_text = "VALID: 1 day" @@ -182,6 +189,7 @@ def _report_human_readable(parsed_certs): expiration_text = "VALID: {0} hour(s)".format(diff.seconds // 3600) else: expiration_text = "VALID: {0} days".format(diff.days) + valid_string = "{0} ({1})".format(cert.target_expiry, expiration_text) certinfo.append(" Certificate Name: {0}\n" " Domains: {1}\n" diff --git a/certbot/ocsp.py b/certbot/ocsp.py index d7fc06e0d..cb3dd0610 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -1,8 +1,8 @@ +"""Tools for checking certificate revocation.""" import logging -from letsencrypt import errors -from letsencrypt import le_util - +from certbot import errors +from certbot import util logger = logging.getLogger(__name__) @@ -10,6 +10,9 @@ logger = logging.getLogger(__name__) REV_LABEL = "**Revoked**" EXP_LABEL = "**Expired**" +INSTALL_LABEL = "(Installed)" + + def revoked_status(cert_path, chain_path): """Get revoked status for a particular cert version. @@ -19,7 +22,7 @@ def revoked_status(cert_path, chain_path): :param str chain_path: Path to chain certificate """ - url, _ = le_util.run_script( + url, _ = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) url = url.rstrip() @@ -31,7 +34,7 @@ def revoked_status(cert_path, chain_path): # This was a PITA... # Thanks to "Bulletproof SSL and TLS - Ivan Ristic" for helping me out try: - output, _ = le_util.run_script( + output, _ = util.run_script( ["openssl", "ocsp", "-no_nonce", "-header", "Host", host, "-issuer", chain_path, @@ -56,4 +59,3 @@ def _translate_ocsp_query(cert_path, ocsp_output): raise errors.Error( "Unable to properly parse OCSP output: %s", ocsp_output) - From ac02cd9cb87ad8cd9e8080f13d7ccd36d9f0702b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 17:36:37 -0800 Subject: [PATCH 19/44] ocsp checking needs -verify_other https://community.letsencrypt.org/t/unable-to-verify-ocsp-response/7264 --- certbot/ocsp.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index cb3dd0610..f4c986609 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -40,9 +40,10 @@ def revoked_status(cert_path, chain_path): "-issuer", chain_path, "-cert", cert_path, "-url", url, - "-CAfile", chain_path]) + "-CAfile", chain_path, + "-verify_other", chain_path]) except errors.SubprocessError: - return "(OCSP Failure)" + return "OCSP Failure" return _translate_ocsp_query(cert_path, output) From 245b84ab783488002a11f82c735b1fd6d31614c3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 17:45:21 -0800 Subject: [PATCH 20/44] Format CLI to keep modern openssls happy - This is somewhat ominous --- certbot/ocsp.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index f4c986609..5a66ba48d 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -7,8 +7,7 @@ from certbot import util logger = logging.getLogger(__name__) -REV_LABEL = "**Revoked**" -EXP_LABEL = "**Expired**" +REV_LABEL = "REVOKED" INSTALL_LABEL = "(Installed)" @@ -36,7 +35,7 @@ def revoked_status(cert_path, chain_path): try: output, _ = util.run_script( ["openssl", "ocsp", - "-no_nonce", "-header", "Host", host, + "-no_nonce", "-header", "Host="+host, "-issuer", chain_path, "-cert", cert_path, "-url", url, From fe36e336a8bca509c9657904d7e9972682b028e8 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 18:37:01 -0800 Subject: [PATCH 21/44] Run with both old and new versions of openssl --- certbot/ocsp.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 5a66ba48d..21b5d3be6 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -1,6 +1,8 @@ """Tools for checking certificate revocation.""" import logging +from subprocess import Popen, PIPE + from certbot import errors from certbot import util @@ -21,6 +23,7 @@ def revoked_status(cert_path, chain_path): :param str chain_path: Path to chain certificate """ + url, _ = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) @@ -30,12 +33,21 @@ def revoked_status(cert_path, chain_path): raise errors.Error( "Unable to get OCSP host from cert, url - %s", url) - # This was a PITA... - # Thanks to "Bulletproof SSL and TLS - Ivan Ristic" for helping me out + # New versions of openssl want -header var=val, old ones want -header var val + test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], + stdout=PIPE, stderr=PIPE) + _out, err = test_host_format.communicate() + if "Missing =" in err: + host_arg = ["Host=" + host] + else: + host_arg = ["Host", host] + + # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! try: output, _ = util.run_script( ["openssl", "ocsp", - "-no_nonce", "-header", "Host="+host, + "-no_nonce", + "-header"] + host_arg + [ "-issuer", chain_path, "-cert", cert_path, "-url", url, From 7a18a124cec768f3405ea82667a1bdcec7566c10 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 19 Dec 2016 19:45:39 -0800 Subject: [PATCH 22/44] Better error handling --- certbot/cert_manager.py | 2 +- certbot/ocsp.py | 61 +++++++++++++++++++++++++++-------------- certbot/util.py | 7 +++-- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 1b6d441c7..4b2fc069f 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -172,12 +172,12 @@ def _report_human_readable(parsed_certs): for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) expiration_text = "" + revoked = ocsp.revoked_status(cert.cert, cert.chain) if cert.is_test_cert: expiration_text = "INVALID: TEST CERT" elif cert.target_expiry <= now: expiration_text = "INVALID: EXPIRED" else: - revoked = ocsp.revoked_status(cert.cert, cert.chain) if revoked: expiration_text = "INVALID: " + revoked diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 21b5d3be6..f1b2d7d32 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -24,50 +24,69 @@ def revoked_status(cert_path, chain_path): """ - url, _ = util.run_script( - ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"]) + if revoked_status.broken: + return False + + if not util.exe_exists("openssl"): + logging.info("openssl not installed, can't check revocation") + revoked_status.broken = True + return False + + try: + url, err = util.run_script( + ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], + log=logging.debug) + except errors.SubprocessError: + logger.info("Cannot extract OCSP URI from %s", cert_path) + return False url = url.rstrip() host = url.partition("://")[2].rstrip("/") if not host: - raise errors.Error( - "Unable to get OCSP host from cert, url - %s", url) + logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) + return False # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], stdout=PIPE, stderr=PIPE) _out, err = test_host_format.communicate() if "Missing =" in err: - host_arg = ["Host=" + host] + host_args = ["Host=" + host] else: - host_arg = ["Host", host] + host_args = ["Host", host] # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! try: - output, _ = util.run_script( - ["openssl", "ocsp", - "-no_nonce", - "-header"] + host_arg + [ - "-issuer", chain_path, - "-cert", cert_path, - "-url", url, - "-CAfile", chain_path, - "-verify_other", chain_path]) + cmd = ["openssl", "ocsp", + "-no_nonce", + "-issuer", chain_path, + "-cert", cert_path, + "-url", url, + "-CAfile", chain_path, + "-verify_other", chain_path, + "-header"] + host_args + output, err = util.run_script(cmd, log=logging.debug) except errors.SubprocessError: - return "OCSP Failure" + logger.info("OCSP querying seems to be broken, assuming nothing is revoked...") + logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), err) + revoked_status.broken = True + return False - return _translate_ocsp_query(cert_path, output) + return _translate_ocsp_query(cert_path, output, err) +revoked_status.broken = False -def _translate_ocsp_query(cert_path, ocsp_output): +def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): """Returns a label string out of the query.""" if not "Response verify OK": - return "Revocation Unknown" + logger.info("Revocation status for %s is unknown", cert_path) + logger.debug("Uncertain ouput:\n%s\nstderr:\n%s", ocsp_output, ocsp_errors) + return "" if cert_path + ": good" in ocsp_output: return "" elif cert_path + ": revoked" in ocsp_output: return REV_LABEL else: - raise errors.Error( - "Unable to properly parse OCSP output: %s", ocsp_output) + logger.warn("Unable to properly parse OCSP output: %s", ocsp_output) + return "" diff --git a/certbot/util.py b/certbot/util.py index cbcfa3314..81a9beca1 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -38,10 +38,11 @@ ANSI_SGR_RED = "\033[31m" ANSI_SGR_RESET = "\033[0m" -def run_script(params): +def run_script(params, log=logger.error): """Run the script with the given params. :param list params: List of parameters to pass to Popen + :param logging.Logger log: Logger to use for errors """ try: @@ -51,7 +52,7 @@ def run_script(params): except (OSError, ValueError): msg = "Unable to run the command: %s" % " ".join(params) - logger.error(msg) + log(msg) raise errors.SubprocessError(msg) stdout, stderr = proc.communicate() @@ -60,7 +61,7 @@ def run_script(params): msg = "Error while running %s.\n%s\n%s" % ( " ".join(params), stdout, stderr) # Enter recovery routine... - logger.error(msg) + log(msg) raise errors.SubprocessError(msg) return stdout, stderr From 840c584cbdf8128c3e8715324bf27c59bd7f5bda Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 09:29:05 -0800 Subject: [PATCH 23/44] Make the OCSP checker a class (Since it contains a reasonable amount of system state) --- certbot/cert_manager.py | 7 ++- certbot/cli.py | 4 ++ certbot/ocsp.py | 108 ++++++++++++++++++++++++---------------- 3 files changed, 73 insertions(+), 46 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 4b2fc069f..59b46fe07 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -169,11 +169,14 @@ def _report_lines(msgs): def _report_human_readable(parsed_certs): """Format a results report for a parsed cert""" certinfo = [] + checker = ocsp.RevocationChecker() for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) expiration_text = "" - revoked = ocsp.revoked_status(cert.cert, cert.chain) - if cert.is_test_cert: + revoked = checker.check_ocsp(cert.cert, cert.chain) + if revoked: + expiration_text = "INVALID: " + revoked + elif cert.is_test_cert: expiration_text = "INVALID: TEST CERT" elif cert.target_expiry <= now: expiration_text = "INVALID: EXPIRED" diff --git a/certbot/cli.py b/certbot/cli.py index 0adb7a4b5..fbcc8ff42 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -943,6 +943,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "testing", "--no-verify-ssl", action="store_true", help=config_help("no_verify_ssl"), default=flag_default("no_verify_ssl")) + helpful.add( + ["testing", "certificates"], "--check-ocsp", default="if otherwise valid", + help='Whether to check OCSP for listed certs. Can be set to "never", "always",' + 'or "if otherwise valid".') helpful.add( ["testing", "standalone", "apache", "nginx"], "--tls-sni-01-port", type=int, default=flag_default("tls_sni_01_port"), diff --git a/certbot/ocsp.py b/certbot/ocsp.py index f1b2d7d32..f2fe1188f 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -13,50 +13,43 @@ REV_LABEL = "REVOKED" INSTALL_LABEL = "(Installed)" +class RevocationChecker(object): + "This class figures out OCSP checking on this system, and performs it." -def revoked_status(cert_path, chain_path): - """Get revoked status for a particular cert version. + def __init__(self): + self.broken = False - .. todo:: Make this a non-blocking call + if not util.exe_exists("openssl"): + logging.info("openssl not installed, can't check revocation") + self.broken = True - :param str cert_path: Path to certificate - :param str chain_path: Path to chain certificate + # New versions of openssl want -header var=val, old ones want -header var val + test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], + stdout=PIPE, stderr=PIPE) + _out, err = test_host_format.communicate() + if "Missing =" in err: + self.host_args = lambda host: ["Host=" + host] + else: + self.host_args = lambda host: ["Host", host] - """ - if revoked_status.broken: - return False + def check_ocsp(self, cert_path, chain_path): + """Get revoked status for a particular cert version. - if not util.exe_exists("openssl"): - logging.info("openssl not installed, can't check revocation") - revoked_status.broken = True - return False + .. todo:: Make this a non-blocking call - try: - url, err = util.run_script( - ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], - log=logging.debug) - except errors.SubprocessError: - logger.info("Cannot extract OCSP URI from %s", cert_path) - return False + :param str cert_path: Path to certificate + :param str chain_path: Path to intermediate cert - url = url.rstrip() - host = url.partition("://")[2].rstrip("/") - if not host: - logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) - return False + """ + if self.broken: + return False - # New versions of openssl want -header var=val, old ones want -header var val - test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], - stdout=PIPE, stderr=PIPE) - _out, err = test_host_format.communicate() - if "Missing =" in err: - host_args = ["Host=" + host] - else: - host_args = ["Host", host] - - # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! - try: + logger.debug("Querying OCSP for %s", cert_path) + url, host = self.determine_ocsp_server(cert_path) + if not host: + return False + # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", "-issuer", chain_path, @@ -64,16 +57,43 @@ def revoked_status(cert_path, chain_path): "-url", url, "-CAfile", chain_path, "-verify_other", chain_path, - "-header"] + host_args - output, err = util.run_script(cmd, log=logging.debug) - except errors.SubprocessError: - logger.info("OCSP querying seems to be broken, assuming nothing is revoked...") - logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), err) - revoked_status.broken = True - return False + "-header"] + self.host_args(host) + try: + output, err = util.run_script(cmd, log=logging.debug) + except errors.SubprocessError, e: + logger.info("We're offline, or OCSP querying is broken. " + "Assuming nothing is revoked...") + logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) + self.broken = True + return False - return _translate_ocsp_query(cert_path, output, err) -revoked_status.broken = False + return _translate_ocsp_query(cert_path, output, err) + + + def determine_ocsp_server(self, cert_path): + """Extract the OCSP server host from a certificate. + + :param str cert_path: Path to the cert we're checking OCSP for + :rtype tuple: + :returns: (OCSP server URL or None, OCSP server host or None) + + """ + try: + url, err = util.run_script( + ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], + log=logging.debug) + except errors.SubprocessError: + logger.info("Cannot extract OCSP URI from %s", cert_path) + logger.debug("Error was:\n%s", err) + return None, None + + url = url.rstrip() + host = url.partition("://")[2].rstrip("/") + if host: + return url, host + else: + logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) + return None, None def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): From e5e5db24d72456632d6724ab6693a1e751385329 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:20:15 -0800 Subject: [PATCH 24/44] CLI flag for controlling ocsp checking now works --- certbot/cert_manager.py | 36 +++++++++++++----------------- certbot/cli.py | 4 ++-- certbot/ocsp.py | 27 +++++++++++++++++++++- certbot/tests/cert_manager_test.py | 12 +++++----- 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 59b46fe07..ed8c6e76d 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -81,7 +81,7 @@ def certificates(config): parse_failures.append(renewal_file) # Describe all the certs - _describe_certs(parsed_certs, parse_failures) + _describe_certs(config, parsed_certs, parse_failures) def delete(config): """Delete Certbot files associated with a certificate lineage.""" @@ -166,34 +166,30 @@ def _report_lines(msgs): """Format a results report for a category of single-line renewal outcomes""" return " " + "\n ".join(str(msg) for msg in msgs) -def _report_human_readable(parsed_certs): +def _report_human_readable(config, parsed_certs): """Format a results report for a parsed cert""" certinfo = [] - checker = ocsp.RevocationChecker() + checker = ocsp.RevocationChecker(config) for cert in parsed_certs: now = pytz.UTC.fromutc(datetime.datetime.utcnow()) - expiration_text = "" - revoked = checker.check_ocsp(cert.cert, cert.chain) - if revoked: - expiration_text = "INVALID: " + revoked - elif cert.is_test_cert: - expiration_text = "INVALID: TEST CERT" - elif cert.target_expiry <= now: - expiration_text = "INVALID: EXPIRED" + status = "" + if cert.is_test_cert: + status = "INVALID: TEST_CERT" + if cert.target_expiry <= now: + status = status + ",EXPIRED" if status else "INVALID: EXPIRED" else: - if revoked: - expiration_text = "INVALID: " + revoked + status = checker.ocsp_status(cert.cert, cert.chain, status) - if not expiration_text: + if not status: diff = cert.target_expiry - now if diff.days == 1: - expiration_text = "VALID: 1 day" + status = "VALID: 1 day" elif diff.days < 1: - expiration_text = "VALID: {0} hour(s)".format(diff.seconds // 3600) + status = "VALID: {0} hour(s)".format(diff.seconds // 3600) else: - expiration_text = "VALID: {0} days".format(diff.days) + status = "VALID: {0} days".format(diff.days) - valid_string = "{0} ({1})".format(cert.target_expiry, expiration_text) + valid_string = "{0} ({1})".format(cert.target_expiry, status) certinfo.append(" Certificate Name: {0}\n" " Domains: {1}\n" " Expiry Date: {2}\n" @@ -206,7 +202,7 @@ def _report_human_readable(parsed_certs): cert.privkey)) return "\n".join(certinfo) -def _describe_certs(parsed_certs, parse_failures): +def _describe_certs(config, parsed_certs, parse_failures): """Print information about the certs we know about""" out = [] @@ -217,7 +213,7 @@ def _describe_certs(parsed_certs, parse_failures): else: if parsed_certs: notify("Found the following certs:") - notify(_report_human_readable(parsed_certs)) + notify(_report_human_readable(config, parsed_certs)) if parse_failures: notify("\nThe following renewal configuration files " "were invalid:") diff --git a/certbot/cli.py b/certbot/cli.py index fbcc8ff42..cea8af51b 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -944,9 +944,9 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help=config_help("no_verify_ssl"), default=flag_default("no_verify_ssl")) helpful.add( - ["testing", "certificates"], "--check-ocsp", default="if otherwise valid", + ["certificates", "testing"], "--check-ocsp", default="always", help='Whether to check OCSP for listed certs. Can be set to "never", "always",' - 'or "if otherwise valid".') + ' or "lazy" (ie, only for certs that are otherwise valid).') helpful.add( ["testing", "standalone", "apache", "nginx"], "--tls-sni-01-port", type=int, default=flag_default("tls_sni_01_port"), diff --git a/certbot/ocsp.py b/certbot/ocsp.py index f2fe1188f..c04afcc82 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -16,8 +16,9 @@ INSTALL_LABEL = "(Installed)" class RevocationChecker(object): "This class figures out OCSP checking on this system, and performs it." - def __init__(self): + def __init__(self, config): self.broken = False + self.config = config if not util.exe_exists("openssl"): logging.info("openssl not installed, can't check revocation") @@ -33,6 +34,30 @@ class RevocationChecker(object): self.host_args = lambda host: ["Host", host] + def ocsp_status(self, cert_path, chain_path, status_in): + """Helper function: updates a cert status string with revocation information + + :param str cert_path: path to a cert to check + :param str chain_path: issuing intermediate for the cert + :param str status_in: a string that is either empty, if the cert is otherwise + believed to be valid, or 'INVALID: $REASON'. + + :returns: a new status including revocation, if the cert is revoked.""" + + if self.config.check_ocsp.lower() == "never": + return status_in + elif self.config.check_ocsp.lower() == "lazy" and status_in: + return status_in + + revoked = self.check_ocsp(cert_path, chain_path) + if not revoked: + return status_in + elif status_in: + return status_in + ",REVOKED" + else: + return "INVALID: REVOKED" + + def check_ocsp(self, cert_path, chain_path): """Get revoked status for a particular cert version. diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index ae673fba1..7f4261fd0 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -189,31 +189,33 @@ class CertificatesTest(BaseCertManagerTest): cert.names.return_value = ["nameone", "nametwo"] cert.is_test_cert = False parsed_certs = [cert] + + mock_config = mock.MagicMock() # pylint: disable=protected-access - out = cert_manager._report_human_readable(parsed_certs) + out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue("INVALID: EXPIRED" in out) cert.target_expiry += datetime.timedelta(hours=2) # pylint: disable=protected-access - out = cert_manager._report_human_readable(parsed_certs) + out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue('1 hour(s)' in out) self.assertTrue('VALID' in out and not 'INVALID' in out) cert.target_expiry += datetime.timedelta(days=1) # pylint: disable=protected-access - out = cert_manager._report_human_readable(parsed_certs) + out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue('1 day' in out) self.assertFalse('under' in out) self.assertTrue('VALID' in out and not 'INVALID' in out) cert.target_expiry += datetime.timedelta(days=2) # pylint: disable=protected-access - out = cert_manager._report_human_readable(parsed_certs) + out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue('3 days' in out) self.assertTrue('VALID' in out and not 'INVALID' in out) cert.is_test_cert = True - out = cert_manager._report_human_readable(parsed_certs) + out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue('INVALID: TEST CERT' in out) From 03f312e653e62b41ae639cc2878b5bf6536718c3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:20:35 -0800 Subject: [PATCH 25/44] Allow filtering of "certbot certificates output" with --config-name or -d --- certbot/cert_manager.py | 7 ++++++- certbot/cli.py | 10 ++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index ed8c6e76d..86b5eedd9 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -171,6 +171,10 @@ def _report_human_readable(config, parsed_certs): certinfo = [] checker = ocsp.RevocationChecker(config) for cert in parsed_certs: + if config.certname and cert.lineagename != config.certname: + continue + if config.domains and not set(config.domains).issubset(cert.names()): + continue now = pytz.UTC.fromutc(datetime.datetime.utcnow()) status = "" if cert.is_test_cert: @@ -212,7 +216,8 @@ def _describe_certs(config, parsed_certs, parse_failures): notify("No certs found.") else: if parsed_certs: - notify("Found the following certs:") + match = "matching " if config.certname or config.domains else "" + notify("Found the following {0}certs:".format(match)) notify(_report_human_readable(config, parsed_certs)) if parse_failures: notify("\nThe following renewal configuration files " diff --git a/certbot/cli.py b/certbot/cli.py index cea8af51b..8e6d7910f 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -350,8 +350,10 @@ VERB_HELP = [ "usage": "\n\n certbot renew [--cert-name NAME] [options]\n\n" }), ("certificates", { - "short": "List all certificates managed by Certbot", - "opts": "List all certificates managed by Certbot" + "short": "List certificates managed by Certbot", + "opts": "List certificates managed by Certbot", + "usage": ("\n\n certbot certificates [options] ...\n\n" + "Print information about the status of certificates managed by Certbot.") }), ("delete", { "short": "Clean up all files related to a certificate", @@ -824,14 +826,14 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "being run in a terminal. This flag cannot be used with the " "renew subcommand.") helpful.add( - [None, "run", "certonly"], + [None, "run", "certonly", "certificates"], "-d", "--domains", "--domain", dest="domains", metavar="DOMAIN", action=_DomainsAction, default=[], help="Domain names to apply. For multiple domains you can use " "multiple -d flags or enter a comma separated list of domains " "as a parameter. (default: Ask)") helpful.add( - [None, "run", "certonly", "manage", "rename", "delete"], + [None, "run", "certonly", "manage", "rename", "delete", "certificates"], "--cert-name", dest="certname", metavar="CERTNAME", default=None, help="Certificate name to apply. Only one certificate name can be used " From 15ed372df617caa8342493763b461b144eeb66b8 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:32:39 -0800 Subject: [PATCH 26/44] Fix existing tests --- certbot/tests/cert_manager_test.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 7f4261fd0..4fd66c34a 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -179,7 +179,9 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue(mock_utility.called) shutil.rmtree(tempdir) - def test_report_human_readable(self): + @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_status') + def test_report_human_readable(self, mock_ocsp): + mock_ocsp.side_effect = lambda _cert, _chain, status: status from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) @@ -190,7 +192,7 @@ class CertificatesTest(BaseCertManagerTest): cert.is_test_cert = False parsed_certs = [cert] - mock_config = mock.MagicMock() + mock_config = mock.MagicMock(certname=None, lineagename=None) # pylint: disable=protected-access out = cert_manager._report_human_readable(mock_config, parsed_certs) self.assertTrue("INVALID: EXPIRED" in out) @@ -216,7 +218,7 @@ class CertificatesTest(BaseCertManagerTest): cert.is_test_cert = True out = cert_manager._report_human_readable(mock_config, parsed_certs) - self.assertTrue('INVALID: TEST CERT' in out) + self.assertTrue('INVALID: TEST_CERT' in out) class SearchLineagesTest(BaseCertManagerTest): From bf6084db615afa28c5316c81ea146f869fd32d30 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:41:34 -0800 Subject: [PATCH 27/44] With mixed staging/prod lineages, it might not be correct to stop OCSPing - One lineage might fail, and a later one succeed --- certbot/ocsp.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index c04afcc82..56074fa45 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -86,10 +86,8 @@ class RevocationChecker(object): try: output, err = util.run_script(cmd, log=logging.debug) except errors.SubprocessError, e: - logger.info("We're offline, or OCSP querying is broken. " - "Assuming nothing is revoked...") + logger.info("We're offline, or OCSP querying is broken. ") logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) - self.broken = True return False return _translate_ocsp_query(cert_path, output, err) From 011f6055d4a4a011e166af195af5681be19c395a Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:43:35 -0800 Subject: [PATCH 28/44] Better message --- certbot/ocsp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 56074fa45..c26c8c1f3 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -86,7 +86,7 @@ class RevocationChecker(object): try: output, err = util.run_script(cmd, log=logging.debug) except errors.SubprocessError, e: - logger.info("We're offline, or OCSP querying is broken. ") + logger.info("OCSP check failed for %s (are we offline?)", cert_path) logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) return False From fcf7387c3d901ebe9223147805fab70c533c9805 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 14:54:48 -0800 Subject: [PATCH 29/44] Don't crash if openssl is missing --- certbot/ocsp.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index c26c8c1f3..92dc6f872 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -23,6 +23,7 @@ class RevocationChecker(object): if not util.exe_exists("openssl"): logging.info("openssl not installed, can't check revocation") self.broken = True + return # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], From 7d02b8dbd541e2f35b325a5404b768d29aa36f48 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 15:00:21 -0800 Subject: [PATCH 30/44] py3fix --- certbot/ocsp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 92dc6f872..758fb2e2f 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -86,7 +86,7 @@ class RevocationChecker(object): "-header"] + self.host_args(host) try: output, err = util.run_script(cmd, log=logging.debug) - except errors.SubprocessError, e: + except errors.SubprocessError as e: logger.info("OCSP check failed for %s (are we offline?)", cert_path) logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) return False From 509f4029bb0b8c68a4c7eb26b1a6d3145734ba04 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 15:07:43 -0800 Subject: [PATCH 31/44] more py3 fixes --- certbot/ocsp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 758fb2e2f..9049ddc01 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -27,7 +27,7 @@ class RevocationChecker(object): # New versions of openssl want -header var=val, old ones want -header var val test_host_format = Popen(["openssl", "ocsp", "-header", "var", "val"], - stdout=PIPE, stderr=PIPE) + stdout=PIPE, stderr=PIPE, universal_newlines=True) _out, err = test_host_format.communicate() if "Missing =" in err: self.host_args = lambda host: ["Host=" + host] From f495863da975341474afc2389e5c1022e8cac330 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 15:38:37 -0800 Subject: [PATCH 32/44] Check --check-ocsp flags, and test those checks --- certbot/cli.py | 4 ++++ certbot/tests/cli_test.py | 4 ++++ certbot/util.py | 3 ++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/certbot/cli.py b/certbot/cli.py index 8e6d7910f..19d917739 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -529,6 +529,10 @@ class HelpfulArgumentParser(object): constants.FORCE_INTERACTIVE_FLAG)) parsed_args.noninteractive_mode = True + parsed_args.check_ocsp = parsed_args.check_ocsp.lower() + if parsed_args.check_ocsp not in ("always", "never", "lazy"): + raise errors.Error('--check-ocsp must be "always", "never", or "lazy"') + if parsed_args.force_interactive and parsed_args.noninteractive_mode: raise errors.Error( "Flag for non-interactive mode and {0} conflict".format( diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index e3bd28a5e..1714dcdbb 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -268,6 +268,10 @@ class ParseTest(unittest.TestCase): self.assertRaises( errors.Error, self.parse, "-n --force-interactive".split()) + def test_check_ocsp(self): + self.assertRaises(errors.Error, self.parse, "certificates --check-ocsp bogus".split()) + self.parse("certificates --check-ocsp lazy".split()) + class DefaultTest(unittest.TestCase): """Tests for certbot.cli._Default.""" diff --git a/certbot/util.py b/certbot/util.py index 81a9beca1..cc0a74bd2 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -48,7 +48,8 @@ def run_script(params, log=logger.error): try: proc = subprocess.Popen(params, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, + universal_newlines=True) except (OSError, ValueError): msg = "Unable to run the command: %s" % " ".join(params) From 76b8c53566cb979569b2368a6394e8bc95fd6c09 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 20 Dec 2016 16:22:14 -0800 Subject: [PATCH 33/44] Tests for ocsp.py, and associated fixes --- certbot/ocsp.py | 10 +-- certbot/tests/cli_test.py | 2 +- certbot/tests/ocsp_test.py | 137 +++++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 certbot/tests/ocsp_test.py diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 9049ddc01..f24d94266 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -45,9 +45,9 @@ class RevocationChecker(object): :returns: a new status including revocation, if the cert is revoked.""" - if self.config.check_ocsp.lower() == "never": + if self.config.check_ocsp == "never": return status_in - elif self.config.check_ocsp.lower() == "lazy" and status_in: + elif self.config.check_ocsp == "lazy" and status_in: return status_in revoked = self.check_ocsp(cert_path, chain_path) @@ -106,9 +106,9 @@ class RevocationChecker(object): url, err = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], log=logging.debug) - except errors.SubprocessError: + except errors.SubprocessError as e: logger.info("Cannot extract OCSP URI from %s", cert_path) - logger.debug("Error was:\n%s", err) + logger.debug("Error was:\n%s", e) return None, None url = url.rstrip() @@ -122,7 +122,7 @@ class RevocationChecker(object): def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): """Returns a label string out of the query.""" - if not "Response verify OK": + if not "Response verify OK" in ocsp_errors: logger.info("Revocation status for %s is unknown", cert_path) logger.debug("Uncertain ouput:\n%s\nstderr:\n%s", ocsp_output, ocsp_errors) return "" diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 1714dcdbb..b5c81bee4 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -270,7 +270,7 @@ class ParseTest(unittest.TestCase): def test_check_ocsp(self): self.assertRaises(errors.Error, self.parse, "certificates --check-ocsp bogus".split()) - self.parse("certificates --check-ocsp lazy".split()) + self.parse("certificates --check-ocsp LaZy".split()) class DefaultTest(unittest.TestCase): diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py new file mode 100644 index 000000000..0bc947021 --- /dev/null +++ b/certbot/tests/ocsp_test.py @@ -0,0 +1,137 @@ + +"""Tests for hooks.py""" +# pylint: disable=protected-access + +import os +import unittest + +import mock + +from certbot import errors +from certbot import hooks + +out = """Missing = in header key=value +ocsp: Use -help for summary. +""" + +class OCSPTest(unittest.TestCase): + + _multiprocess_can_split_ = True + + def setUp(self): + from certbot import ocsp + self.config = mock.MagicMock() + self.checker = ocsp.RevocationChecker(self.config) + + def tearDown(self): + pass + + @mock.patch('certbot.ocsp.logging.info') + @mock.patch('certbot.ocsp.Popen') + @mock.patch('certbot.util.exe_exists') + def test_init(self, mock_exists, mock_popen, mock_log): + mock_communicate = mock.MagicMock() + mock_communicate.communicate.return_value = (None, out) + mock_popen.return_value = mock_communicate + mock_exists.return_value = True + + from certbot import ocsp + checker = ocsp.RevocationChecker(self.config) + self.assertEqual(mock_popen.call_count, 1) + self.assertEqual(checker.host_args("x"), ["Host=x"]) + + mock_communicate.communicate.return_value = (None, out.partition("\n")[2]) + checker = ocsp.RevocationChecker(self.config) + self.assertEqual(checker.host_args("x"), ["Host", "x"]) + self.assertEqual(checker.broken, False) + + mock_exists.return_value = False + mock_popen.call_count = 0 + checker = ocsp.RevocationChecker(self.config) + self.assertEqual(mock_popen.call_count, 0) + self.assertEqual(mock_log.call_count, 1) + self.assertEqual(checker.broken, True) + + def test_ocsp_status(self): + from certbot import ocsp + checker = self.checker + checker.check_ocsp = mock.MagicMock() + checker.check_ocsp.return_value = "octopus found in certificate" + + checker.config.check_ocsp = "never" + self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") + self.assertEqual(checker.ocsp_status("a", "a", ""), "") + self.assertEqual(checker.check_ocsp.call_count, 0) + + checker.config.check_ocsp = "lazy" + self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") + self.assertEqual(checker.check_ocsp.call_count, 0) + self.assertEqual(checker.ocsp_status("a", "a", ""), "INVALID: REVOKED") + + checker.config.check_ocsp = "always" + self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz,REVOKED") + checker.check_ocsp.return_value = "" + self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") + + + @mock.patch('certbot.ocsp.logger.debug') + @mock.patch('certbot.ocsp.logger.info') + @mock.patch('certbot.util.run_script') + def test_determine_ocsp_server(self, mock_run, mock_info, mock_debug): + uri = "http://ocsp.stg-int-x1.letsencrypt.org/" + host = "ocsp.stg-int-x1.letsencrypt.org" + mock_run.return_value = uri, "" + self.assertEquals(self.checker.determine_ocsp_server("beep"), (uri, host)) + mock_run.return_value = "ftp:/" + host + "/", "" + self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertEquals(mock_info.call_count, 1) + + c = "confusion" + mock_run.side_effect = errors.SubprocessError(c) + self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertTrue(c in mock_debug.call_args[0][1]) + + @mock.patch('certbot.ocsp.logger') + @mock.patch('certbot.util.run_script') + def test_translate_ocsp(self, mock_run, mock_log): + # pylint: disable=protected-access + mock_run.return_value = openssl_confused + from certbot import ocsp + self.assertEquals(ocsp._translate_ocsp_query(*openssl_happy), "") + self.assertEquals(ocsp._translate_ocsp_query(*openssl_confused), "") + self.assertEquals(mock_log.debug.call_count, 1) + self.assertEquals(mock_log.warn.call_count, 0) + self.assertEquals(ocsp._translate_ocsp_query(*openssl_broken), "") + self.assertEquals(mock_log.warn.call_count, 1) + self.assertEquals(ocsp._translate_ocsp_query(*openssl_revoked), "REVOKED") + + +openssl_confused = ("", """ +/etc/letsencrypt/live/example.org/cert.pem: good + This Update: Dec 17 00:00:00 2016 GMT + Next Update: Dec 24 00:00:00 2016 GMT +""", +""" +Response Verify Failure +139903674214048:error:27069065:OCSP routines:OCSP_basic_verify:certificate verify error:ocsp_vfy.c:138:Verify error:unable to get local issuer certificate +""") + +openssl_happy = ("blah.pem", """ +blah.pem: good + This Update: Dec 20 18:00:00 2016 GMT + Next Update: Dec 27 18:00:00 2016 GMT +""", +"Response verify OK") + +openssl_revoked = ("blah.pem", """ +blah.pem: revoked + This Update: Dec 20 01:00:00 2016 GMT + Next Update: Dec 27 01:00:00 2016 GMT + Revocation Time: Dec 20 01:46:34 2016 GMT +""", +"""Response verify OK""") + +openssl_broken = ("", "tentacles", "Response verify OK") + +if __name__ == '__main__': + unittest.main() # pragma: no cover From 0ed3213989c095455d4c17c91cd1795e794374ea Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 14:25:16 -0800 Subject: [PATCH 34/44] Remove --check-ocsp flag - Might have been occasionally useful, but simplicity - Add some missing tests, remove some obsolete ones --- certbot/cert_manager.py | 21 +++++---- certbot/cli.py | 8 ---- certbot/ocsp.py | 57 ++++++----------------- certbot/tests/cert_manager_test.py | 4 +- certbot/tests/cli_test.py | 4 -- certbot/tests/ocsp_test.py | 74 +++++++++++++++--------------- 6 files changed, 65 insertions(+), 103 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 86b5eedd9..09798e3bc 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -169,22 +169,25 @@ def _report_lines(msgs): def _report_human_readable(config, parsed_certs): """Format a results report for a parsed cert""" certinfo = [] - checker = ocsp.RevocationChecker(config) + checker = ocsp.RevocationChecker() for cert in parsed_certs: if config.certname and cert.lineagename != config.certname: continue if config.domains and not set(config.domains).issubset(cert.names()): continue now = pytz.UTC.fromutc(datetime.datetime.utcnow()) - status = "" - if cert.is_test_cert: - status = "INVALID: TEST_CERT" - if cert.target_expiry <= now: - status = status + ",EXPIRED" if status else "INVALID: EXPIRED" - else: - status = checker.ocsp_status(cert.cert, cert.chain, status) - if not status: + reasons = [] + if cert.is_test_cert: + reasons.append('TEST_CERT') + if cert.target_expiry <= now: + reasons.append('EXPIRED') + if checker.ocsp_revoked(cert.cert, cert.chain): + reasons.append('REVOKED') + + if reasons: + status = "INVALID: " + ", ".join(reasons) + else: diff = cert.target_expiry - now if diff.days == 1: status = "VALID: 1 day" diff --git a/certbot/cli.py b/certbot/cli.py index 19d917739..68eb67b35 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -529,10 +529,6 @@ class HelpfulArgumentParser(object): constants.FORCE_INTERACTIVE_FLAG)) parsed_args.noninteractive_mode = True - parsed_args.check_ocsp = parsed_args.check_ocsp.lower() - if parsed_args.check_ocsp not in ("always", "never", "lazy"): - raise errors.Error('--check-ocsp must be "always", "never", or "lazy"') - if parsed_args.force_interactive and parsed_args.noninteractive_mode: raise errors.Error( "Flag for non-interactive mode and {0} conflict".format( @@ -949,10 +945,6 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "testing", "--no-verify-ssl", action="store_true", help=config_help("no_verify_ssl"), default=flag_default("no_verify_ssl")) - helpful.add( - ["certificates", "testing"], "--check-ocsp", default="always", - help='Whether to check OCSP for listed certs. Can be set to "never", "always",' - ' or "lazy" (ie, only for certs that are otherwise valid).') helpful.add( ["testing", "standalone", "apache", "nginx"], "--tls-sni-01-port", type=int, default=flag_default("tls_sni_01_port"), diff --git a/certbot/ocsp.py b/certbot/ocsp.py index f24d94266..a5df5743d 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -8,17 +8,11 @@ from certbot import util logger = logging.getLogger(__name__) - -REV_LABEL = "REVOKED" - -INSTALL_LABEL = "(Installed)" - class RevocationChecker(object): "This class figures out OCSP checking on this system, and performs it." - def __init__(self, config): + def __init__(self): self.broken = False - self.config = config if not util.exe_exists("openssl"): logging.info("openssl not installed, can't check revocation") @@ -35,46 +29,25 @@ class RevocationChecker(object): self.host_args = lambda host: ["Host", host] - def ocsp_status(self, cert_path, chain_path, status_in): - """Helper function: updates a cert status string with revocation information - - :param str cert_path: path to a cert to check - :param str chain_path: issuing intermediate for the cert - :param str status_in: a string that is either empty, if the cert is otherwise - believed to be valid, or 'INVALID: $REASON'. - - :returns: a new status including revocation, if the cert is revoked.""" - - if self.config.check_ocsp == "never": - return status_in - elif self.config.check_ocsp == "lazy" and status_in: - return status_in - - revoked = self.check_ocsp(cert_path, chain_path) - if not revoked: - return status_in - elif status_in: - return status_in + ",REVOKED" - else: - return "INVALID: REVOKED" - - - def check_ocsp(self, cert_path, chain_path): + def ocsp_revoked(self, cert_path, chain_path): """Get revoked status for a particular cert version. .. todo:: Make this a non-blocking call :param str cert_path: Path to certificate :param str chain_path: Path to intermediate cert + :rtype bool or None: + :returns: False if valid; True if revoked; None if check itself failed """ if self.broken: - return False + return None + logger.debug("Querying OCSP for %s", cert_path) url, host = self.determine_ocsp_server(cert_path) if not host: - return False + return None # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", @@ -89,7 +62,7 @@ class RevocationChecker(object): except errors.SubprocessError as e: logger.info("OCSP check failed for %s (are we offline?)", cert_path) logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) - return False + return None return _translate_ocsp_query(cert_path, output, err) @@ -103,7 +76,7 @@ class RevocationChecker(object): """ try: - url, err = util.run_script( + url, _err = util.run_script( ["openssl", "x509", "-in", cert_path, "-noout", "-ocsp_uri"], log=logging.debug) except errors.SubprocessError as e: @@ -119,18 +92,18 @@ class RevocationChecker(object): logger.info("Cannot process OCSP host from URL (%s) in cert at %s", url, cert_path) return None, None - def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): - """Returns a label string out of the query.""" + """Parse openssl's weird output to work out what it means.""" + if not "Response verify OK" in ocsp_errors: logger.info("Revocation status for %s is unknown", cert_path) logger.debug("Uncertain ouput:\n%s\nstderr:\n%s", ocsp_output, ocsp_errors) - return "" + return None if cert_path + ": good" in ocsp_output: - return "" + return False elif cert_path + ": revoked" in ocsp_output: - return REV_LABEL + return True else: logger.warn("Unable to properly parse OCSP output: %s", ocsp_output) - return "" + return None diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 4fd66c34a..dc83d0952 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -179,9 +179,9 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue(mock_utility.called) shutil.rmtree(tempdir) - @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_status') + @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_revoked') def test_report_human_readable(self, mock_ocsp): - mock_ocsp.side_effect = lambda _cert, _chain, status: status + mock_ocsp.side_effect = lambda _cert, _chain: None from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index b5c81bee4..e3bd28a5e 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -268,10 +268,6 @@ class ParseTest(unittest.TestCase): self.assertRaises( errors.Error, self.parse, "-n --force-interactive".split()) - def test_check_ocsp(self): - self.assertRaises(errors.Error, self.parse, "certificates --check-ocsp bogus".split()) - self.parse("certificates --check-ocsp LaZy".split()) - class DefaultTest(unittest.TestCase): """Tests for certbot.cli._Default.""" diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 0bc947021..ce3ade2e8 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -2,13 +2,11 @@ """Tests for hooks.py""" # pylint: disable=protected-access -import os import unittest import mock from certbot import errors -from certbot import hooks out = """Missing = in header key=value ocsp: Use -help for summary. @@ -20,8 +18,13 @@ class OCSPTest(unittest.TestCase): def setUp(self): from certbot import ocsp - self.config = mock.MagicMock() - self.checker = ocsp.RevocationChecker(self.config) + with mock.patch('certbot.ocsp.Popen') as mock_popen: + with mock.patch('certbot.util.exe_exists') as mock_exists: + mock_communicate = mock.MagicMock() + mock_communicate.communicate.return_value = (None, out) + mock_popen.return_value = mock_communicate + mock_exists.return_value = True + self.checker = ocsp.RevocationChecker() def tearDown(self): pass @@ -36,44 +39,38 @@ class OCSPTest(unittest.TestCase): mock_exists.return_value = True from certbot import ocsp - checker = ocsp.RevocationChecker(self.config) + checker = ocsp.RevocationChecker() self.assertEqual(mock_popen.call_count, 1) self.assertEqual(checker.host_args("x"), ["Host=x"]) mock_communicate.communicate.return_value = (None, out.partition("\n")[2]) - checker = ocsp.RevocationChecker(self.config) + checker = ocsp.RevocationChecker() self.assertEqual(checker.host_args("x"), ["Host", "x"]) self.assertEqual(checker.broken, False) mock_exists.return_value = False mock_popen.call_count = 0 - checker = ocsp.RevocationChecker(self.config) + checker = ocsp.RevocationChecker() self.assertEqual(mock_popen.call_count, 0) self.assertEqual(mock_log.call_count, 1) self.assertEqual(checker.broken, True) - def test_ocsp_status(self): - from certbot import ocsp - checker = self.checker - checker.check_ocsp = mock.MagicMock() - checker.check_ocsp.return_value = "octopus found in certificate" + @mock.patch('certbot.ocsp.RevocationChecker.determine_ocsp_server') + @mock.patch('certbot.util.run_script') + def test_ocsp_revoked(self, mock_run, mock_determine): + self.checker.broken = True + mock_determine.return_value = ("", "") + self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) - checker.config.check_ocsp = "never" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") - self.assertEqual(checker.ocsp_status("a", "a", ""), "") - self.assertEqual(checker.check_ocsp.call_count, 0) + self.checker.broken = False + mock_run.return_value = tuple(openssl_happy[1:]) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(mock_run.call_count, 0) + + mock_determine.return_value = ("http://x.co", "x.co") + self.assertEqual(self.checker.ocsp_revoked("blah.pem", "chain.pem"), False) - checker.config.check_ocsp = "lazy" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") - self.assertEqual(checker.check_ocsp.call_count, 0) - self.assertEqual(checker.ocsp_status("a", "a", ""), "INVALID: REVOKED") - checker.config.check_ocsp = "always" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz,REVOKED") - checker.check_ocsp.return_value = "" - self.assertEqual(checker.ocsp_status("a", "a", "xyz"), "xyz") - - @mock.patch('certbot.ocsp.logger.debug') @mock.patch('certbot.ocsp.logger.info') @mock.patch('certbot.util.run_script') @@ -81,31 +78,32 @@ class OCSPTest(unittest.TestCase): uri = "http://ocsp.stg-int-x1.letsencrypt.org/" host = "ocsp.stg-int-x1.letsencrypt.org" mock_run.return_value = uri, "" - self.assertEquals(self.checker.determine_ocsp_server("beep"), (uri, host)) + self.assertEqual(self.checker.determine_ocsp_server("beep"), (uri, host)) mock_run.return_value = "ftp:/" + host + "/", "" - self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) - self.assertEquals(mock_info.call_count, 1) + self.assertEqual(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertEqual(mock_info.call_count, 1) c = "confusion" mock_run.side_effect = errors.SubprocessError(c) - self.assertEquals(self.checker.determine_ocsp_server("beep"), (None, None)) + self.assertEqual(self.checker.determine_ocsp_server("beep"), (None, None)) self.assertTrue(c in mock_debug.call_args[0][1]) @mock.patch('certbot.ocsp.logger') @mock.patch('certbot.util.run_script') def test_translate_ocsp(self, mock_run, mock_log): - # pylint: disable=protected-access + # pylint: disable=protected-access,star-args mock_run.return_value = openssl_confused from certbot import ocsp - self.assertEquals(ocsp._translate_ocsp_query(*openssl_happy), "") - self.assertEquals(ocsp._translate_ocsp_query(*openssl_confused), "") - self.assertEquals(mock_log.debug.call_count, 1) - self.assertEquals(mock_log.warn.call_count, 0) - self.assertEquals(ocsp._translate_ocsp_query(*openssl_broken), "") - self.assertEquals(mock_log.warn.call_count, 1) - self.assertEquals(ocsp._translate_ocsp_query(*openssl_revoked), "REVOKED") + self.assertEqual(ocsp._translate_ocsp_query(*openssl_happy), False) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), None) + self.assertEqual(mock_log.debug.call_count, 1) + self.assertEqual(mock_log.warn.call_count, 0) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_broken), None) + self.assertEqual(mock_log.warn.call_count, 1) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_revoked), True) +# pylint: disable=line-too-long openssl_confused = ("", """ /etc/letsencrypt/live/example.org/cert.pem: good This Update: Dec 17 00:00:00 2016 GMT From e2d8630f5e3658218ce8fa3256dae841a1257ffe Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 14:42:35 -0800 Subject: [PATCH 35/44] py3fix --- certbot/tests/ocsp_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index ce3ade2e8..285b0379b 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -86,7 +86,7 @@ class OCSPTest(unittest.TestCase): c = "confusion" mock_run.side_effect = errors.SubprocessError(c) self.assertEqual(self.checker.determine_ocsp_server("beep"), (None, None)) - self.assertTrue(c in mock_debug.call_args[0][1]) + self.assertTrue(c in repr(mock_debug.call_args[0][1])) @mock.patch('certbot.ocsp.logger') @mock.patch('certbot.util.run_script') From 61e822a89724a3430777a9840404e4348a5aa1ff Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 21:50:19 -0800 Subject: [PATCH 36/44] Add a few more tests --- certbot/tests/cert_manager_test.py | 35 +++++++++++++++++++++++++----- certbot/tests/ocsp_test.py | 3 +++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index dc83d0952..003eb1470 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -1,6 +1,7 @@ """Tests for certbot.cert_manager.""" # pylint disable=protected-access import os +import re import shutil import tempfile import unittest @@ -181,7 +182,7 @@ class CertificatesTest(BaseCertManagerTest): @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_revoked') def test_report_human_readable(self, mock_ocsp): - mock_ocsp.side_effect = lambda _cert, _chain: None + mock_ocsp.return_value = None from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) @@ -191,35 +192,57 @@ class CertificatesTest(BaseCertManagerTest): cert.names.return_value = ["nameone", "nametwo"] cert.is_test_cert = False parsed_certs = [cert] + get_report = lambda: cert_manager._report_human_readable(mock_config, parsed_certs) mock_config = mock.MagicMock(certname=None, lineagename=None) # pylint: disable=protected-access - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() self.assertTrue("INVALID: EXPIRED" in out) cert.target_expiry += datetime.timedelta(hours=2) # pylint: disable=protected-access - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() self.assertTrue('1 hour(s)' in out) self.assertTrue('VALID' in out and not 'INVALID' in out) cert.target_expiry += datetime.timedelta(days=1) # pylint: disable=protected-access - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() self.assertTrue('1 day' in out) self.assertFalse('under' in out) self.assertTrue('VALID' in out and not 'INVALID' in out) cert.target_expiry += datetime.timedelta(days=2) # pylint: disable=protected-access - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() self.assertTrue('3 days' in out) self.assertTrue('VALID' in out and not 'INVALID' in out) cert.is_test_cert = True - out = cert_manager._report_human_readable(mock_config, parsed_certs) + out = get_report() self.assertTrue('INVALID: TEST_CERT' in out) + cert = mock.MagicMock(lineagename="indescribable") + cert.target_expiry = expiry + cert.names.return_value = ["nameone", "thrice.named"] + cert.is_test_cert = True + parsed_certs.append(cert) + + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 2) + mock_config.domains = ["thrice.named"] + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 1) + mock_config.domains = ["nameone"] + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 2) + mock_config.certname = "indescribable" + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 1) + mock_config.certname = "horror" + out = get_report() + self.assertEqual(len(re.findall("INVALID:", out)), 0) + class SearchLineagesTest(BaseCertManagerTest): """Tests for certbot.cert_manager._search_lineages.""" diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 285b0379b..e136978d8 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -69,6 +69,9 @@ class OCSPTest(unittest.TestCase): mock_determine.return_value = ("http://x.co", "x.co") self.assertEqual(self.checker.ocsp_revoked("blah.pem", "chain.pem"), False) + mock_run.side_effect = errors.SubprocessError("Unable to load certificate launcher") + self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(mock_run.call_count, 2) @mock.patch('certbot.ocsp.logger.debug') From 7014ab5fd0f02530b2319373f5fd45824a4d7a29 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 21 Dec 2016 23:20:19 -0800 Subject: [PATCH 37/44] lint --- certbot/tests/cert_manager_test.py | 4 +++- certbot/tests/ocsp_test.py | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 003eb1470..6caaab878 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -1,5 +1,5 @@ """Tests for certbot.cert_manager.""" -# pylint disable=protected-access +# pylint: disable=protected-access import os import re import shutil @@ -192,6 +192,8 @@ class CertificatesTest(BaseCertManagerTest): cert.names.return_value = ["nameone", "nametwo"] cert.is_test_cert = False parsed_certs = [cert] + + # pylint: disable=protected-access get_report = lambda: cert_manager._report_human_readable(mock_config, parsed_certs) mock_config = mock.MagicMock(certname=None, lineagename=None) diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index e136978d8..c4517174d 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -1,5 +1,4 @@ - -"""Tests for hooks.py""" +"""Tests for ocsp.py""" # pylint: disable=protected-access import unittest From 39f55513054609fbad0d87fbf2d493665b180706 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 22 Dec 2016 08:24:08 -0800 Subject: [PATCH 38/44] Merge the manual and script plugins (#3890) * Start of combined manual/script plugin * Return str from hooks.execute, not bytes * finish manual/script rewrite * delete old manual and script plugins * manually specify we want chall.token * use consistent quotes * specify chall for uri * s/script/hook * fix spacing on instructions * remove unneeded response argument * make achall more helpful * simplify perform * remove old test files * add start of manual_tests * fix ParseTest.test_help * stop using manual_test_mode in cli tests * Revert "make achall more helpful" This reverts commit 54b01cea30167065e3682834a71144b81e96c07f. * use bad response/validation methods on achalls * simplify perform and cleanup environment * finish manual tests * Add HTTP manual hook integration test * add manual http scripts * Add manual DNS script integration test * remove references to the script plugin * they're hooks, not scripts * add --manual-public-ip-logging-ok to integration tests * use --pref-chall for dns integration * does dns work? * validate hooks * test hook validation * Revert "does dns work?" This reverts commit 1224cc2961b35a2b8e9e5d2ca3af7c081161b22a. * busy wait in manual-http-auth * remove DNS script test for now * Fix challenge prefix and add trailing . * Add comment about universal_newlines * Fix typo from 0464ba2c4 * fix nits and typos * Generalize HookCOmmandNotFound error * Add verify_exe_exists * Don't duplicate code in hooks.py * Revert changes to hooks.py * Use consistent hook error messages --- certbot/cli.py | 6 +- certbot/hooks.py | 5 +- certbot/plugins/manual.py | 305 +++++++++++---------------------- certbot/plugins/manual_test.py | 180 +++++++++---------- certbot/plugins/script.py | 161 ----------------- certbot/plugins/script_test.py | 170 ------------------ certbot/plugins/selection.py | 4 +- certbot/tests/acme_util.py | 11 ++ certbot/tests/cli_test.py | 22 +-- setup.py | 1 - tests/boulder-integration.sh | 7 +- tests/integration/_common.sh | 2 +- tests/manual-dns-auth.sh | 4 + tests/manual-http-auth.sh | 12 ++ tests/manual-http-cleanup.sh | 2 + 15 files changed, 237 insertions(+), 655 deletions(-) delete mode 100644 certbot/plugins/script.py delete mode 100644 certbot/plugins/script_test.py create mode 100755 tests/manual-dns-auth.sh create mode 100755 tests/manual-http-auth.sh create mode 100755 tests/manual-http-cleanup.sh diff --git a/certbot/cli.py b/certbot/cli.py index 0adb7a4b5..8b26568c6 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -72,7 +72,7 @@ obtain, install, and renew certificates: --standalone Run a standalone webserver for authentication %s --webroot Place files in a server's webroot folder for authentication - --manual Obtain certs interactively, or using shell script hoooks + --manual Obtain certs interactively, or using shell script hooks -n Run non-interactively --test-cert Obtain a test cert from a staging server @@ -100,7 +100,7 @@ More detailed help: all, automation, commands, paths, security, testing, or any of the subcommands or plugins (certonly, renew, install, register, nginx, - apache, standalone, webroot, script, etc.) + apache, standalone, webroot, etc.) """ @@ -1153,8 +1153,6 @@ def _plugins_parsing(helpful, plugins): "--nginx", action="store_true", help="Obtain and install certs using Nginx") helpful.add(["plugins", "certonly"], "--standalone", action="store_true", help='Obtain certs using a "standalone" webserver.') - helpful.add(["plugins", "certonly"], "--script", action="store_true", - help='Obtain certs using shell script(s)') helpful.add(["plugins", "certonly"], "--manual", action="store_true", help='Provide laborious manual instructions for obtaining a cert') helpful.add(["plugins", "certonly"], "--webroot", action="store_true", diff --git a/certbot/hooks.py b/certbot/hooks.py index 37afee9b0..63afba091 100644 --- a/certbot/hooks.py +++ b/certbot/hooks.py @@ -84,7 +84,10 @@ def execute(shell_cmd): :returns: `tuple` (`str` stderr, `str` stdout)""" - cmd = Popen(shell_cmd, shell=True, stdout=PIPE, stderr=PIPE) + # universal_newlines causes Popen.communicate() + # to return str objects instead of bytes in Python 3 + cmd = Popen(shell_cmd, shell=True, stdout=PIPE, + stderr=PIPE, universal_newlines=True) out, err = cmd.communicate() if cmd.returncode != 0: logger.error('Hook command "%s" returned error code %d', diff --git a/certbot/plugins/manual.py b/certbot/plugins/manual.py index 646b1d340..1163e7e7e 100644 --- a/certbot/plugins/manual.py +++ b/certbot/plugins/manual.py @@ -1,56 +1,49 @@ -"""Manual plugin.""" +"""Manual authenticator plugin""" import os -import logging -import pipes -import shutil -import socket -import subprocess -import sys -import tempfile -import time -import six import zope.component import zope.interface from acme import challenges -from acme import errors as acme_errors -from certbot import errors from certbot import interfaces +from certbot import errors +from certbot import hooks from certbot.plugins import common -logger = logging.getLogger(__name__) - - @zope.interface.implementer(interfaces.IAuthenticator) @zope.interface.provider(interfaces.IPluginFactory) class Authenticator(common.Plugin): - """Manual Authenticator. + """Manual authenticator - This plugin requires user's manual intervention in setting up a HTTP - server for solving http-01 challenges and thus does not need to be - run as a privileged process. Alternatively shows instructions on how - to use Python's built-in HTTP server. - - .. todo:: Support for `~.challenges.TLSSNI01`. + This plugin allows the user to perform the domain validation + challenge(s) themselves. This either be done manually by the user or + through shell scripts provided to Certbot. """ + + description = 'Manual configuration or run your own shell scripts' hidden = True - - description = "Manually configure an HTTP server" - - MESSAGE_TEMPLATE = { - "dns-01": """\ + long_description = ( + 'Authenticate through manual configuration or custom shell scripts. ' + 'When using shell scripts, an authenticator script must be provided. ' + 'The environment variables available to this script are ' + '$CERTBOT_DOMAIN which contains the domain being authenticated, ' + '$CERTBOT_VALIDATION which is the validation string, and ' + '$CERTBOT_TOKEN which is the filename of the resource requested when ' + 'performing an HTTP-01 challenge. An additional cleanup script can ' + 'also be provided and can use the additional variable ' + '$CERTBOT_AUTH_OUTPUT which contains the stdout output from the auth ' + 'script.') + _DNS_INSTRUCTIONS = """\ Please deploy a DNS TXT record under the name {domain} with the following value: {validation} -Once this is deployed, -""", - "http-01": """\ +Once this is deployed,""" + _HTTP_INSTRUCTIONS = """\ Make sure your web server displays the following content at {uri} before continuing: @@ -59,204 +52,114 @@ Make sure your web server displays the following content at If you don't have HTTP server configured, you can run the following command on the target server (as root): -{command} -"""} - - # a disclaimer about your current IP being transmitted to Let's Encrypt's servers. - IP_DISCLAIMER = """\ -NOTE: The IP of this machine will be publicly logged as having requested this certificate. \ -If you're running certbot in manual mode on a machine that is not your server, \ -please ensure you're okay with that. - -Are you OK with your IP being logged? -""" - - # "cd /tmp/certbot" makes sure user doesn't serve /root, - # separate "public_html" ensures that cert.pem/key.pem are not - # served and makes it more obvious that Python command will serve - # anything recursively under the cwd - - CMD_TEMPLATE = """\ -mkdir -p {root}/public_html/{achall.URI_ROOT_PATH} -cd {root}/public_html +mkdir -p /tmp/certbot/public_html/{achall.URI_ROOT_PATH} +cd /tmp/certbot/public_html printf "%s" {validation} > {achall.URI_ROOT_PATH}/{encoded_token} # run only once per server: $(command -v python2 || command -v python2.7 || command -v python2.6) -c \\ "import BaseHTTPServer, SimpleHTTPServer; \\ s = BaseHTTPServer.HTTPServer(('', {port}), SimpleHTTPServer.SimpleHTTPRequestHandler); \\ s.serve_forever()" """ - """Command template.""" def __init__(self, *args, **kwargs): super(Authenticator, self).__init__(*args, **kwargs) - self._root = (tempfile.mkdtemp() if self.conf("test-mode") - else "/tmp/certbot") - self._httpd = None + self.env = dict() @classmethod def add_parser_arguments(cls, add): - add("test-mode", action="store_true", - help="Test mode. Executes the manual command in subprocess.") - add("public-ip-logging-ok", action="store_true", - help="Automatically allows public IP logging. (default: Ask)") + add('auth-hook', + help='Path or command to execute for the authentication script') + add('cleanup-hook', + help='Path or command to execute for the cleanup script') + add('public-ip-logging-ok', action='store_true', + help='Automatically allows public IP logging (default: Ask)') - def prepare(self): # pylint: disable=missing-docstring,no-self-use - if self.config.noninteractive_mode and not self.conf("test-mode"): - raise errors.PluginError("Running manual mode non-interactively is not supported") + def prepare(self): # pylint: disable=missing-docstring + if self.config.noninteractive_mode and not self.conf('auth-hook'): + raise errors.PluginError( + 'An authentication script must be provided with --{0} when ' + 'using the manual plugin non-interactively.'.format( + self.option_name('auth-hook'))) + self._validate_hooks() + + def _validate_hooks(self): + if self.config.validate_hooks: + for name in ('auth-hook', 'cleanup-hook'): + hook = self.conf(name) + if hook is not None: + hook_prefix = self.option_name(name)[:-len('-hook')] + hooks.validate_hook(hook, hook_prefix) def more_info(self): # pylint: disable=missing-docstring,no-self-use - return ("This plugin requires user's manual intervention in setting " - "up challenges to prove control of a domain and does not need " - "to be run as a privileged process. When solving " - "http-01 challenges, the user is responsible for setting up " - "an HTTP server. Alternatively, instructions are shown on how " - "to use Python's built-in HTTP server. The user is " - "responsible for configuration of a domain's DNS when solving " - "dns-01 challenges. The type of challenges used can be " - "controlled through the --preferred-challenges flag.") + return ( + 'This plugin allows the user to customize setup for domain ' + 'validation challenges either through shell scripts provided by ' + 'the user or by performing the setup manually.') def get_chall_pref(self, domain): # pylint: disable=missing-docstring,no-self-use,unused-argument return [challenges.HTTP01, challenges.DNS01] - def perform(self, achalls): - # pylint: disable=missing-docstring - self._get_ip_logging_permission() - mapping = {"http-01": self._perform_http01_challenge, - "dns-01": self._perform_dns01_challenge} + def perform(self, achalls): # pylint: disable=missing-docstring + self._verify_ip_logging_ok() + + if self.conf('auth-hook'): + perform_achall = self._perform_achall_with_script + else: + perform_achall = self._perform_achall_manually + responses = [] - # TODO: group achalls by the same socket.gethostbyname(_ex) - # and prompt only once per server (one "echo -n" per domain) for achall in achalls: - responses.append(mapping[achall.typ](achall)) + perform_achall(achall) + responses.append(achall.response(achall.account_key)) return responses - @classmethod - def _test_mode_busy_wait(cls, port): - while True: - time.sleep(1) - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - try: - sock.connect(("localhost", port)) - except socket.error: # pragma: no cover - pass + def _verify_ip_logging_ok(self): + if not self.conf('public-ip-logging-ok'): + cli_flag = '--{0}'.format(self.option_name('public-ip-logging-ok')) + msg = ('NOTE: The IP of this machine will be publicly logged as ' + "having requested this certificate. If you're running " + 'certbot in manual mode on a machine that is not your ' + "server, please ensure you're okay with that.\n\n" + 'Are you OK with your IP being logged?') + display = zope.component.getUtility(interfaces.IDisplay) + if display.yesno(msg, cli_flag=cli_flag, force_interactive=True): + setattr(self.config, self.dest('public-ip-logging-ok'), True) else: - break - finally: - sock.close() + raise errors.PluginError('Must agree to IP logging to proceed') - def cleanup(self, achalls): - # pylint: disable=missing-docstring - for achall in achalls: - if isinstance(achall.chall, challenges.HTTP01): - self._cleanup_http01_challenge(achall) - - def _perform_http01_challenge(self, achall): - # same path for each challenge response would be easier for - # users, but will not work if multiple domains point at the - # same server: default command doesn't support virtual hosts - response, validation = achall.response_and_validation() - - port = (response.port if self.config.http01_port is None - else int(self.config.http01_port)) - command = self.CMD_TEMPLATE.format( - root=self._root, achall=achall, response=response, - # TODO(kuba): pipes still necessary? - validation=pipes.quote(validation), - encoded_token=achall.chall.encode("token"), - port=port) - if self.conf("test-mode"): - logger.debug("Test mode. Executing the manual command: %s", command) - # sh shipped with OS X does't support echo -n, but supports printf - try: - self._httpd = subprocess.Popen( - command, - # don't care about setting stdout and stderr, - # we're in test mode anyway - shell=True, - executable=None, - # "preexec_fn" is UNIX specific, but so is "command" - preexec_fn=os.setsid) - except OSError as error: # ValueError should not happen! - logger.debug( - "Couldn't execute manual command: %s", error, exc_info=True) - return False - logger.debug("Manual command running as PID %s.", self._httpd.pid) - # give it some time to bootstrap, before we try to verify - # (cert generation in case of simpleHttpS might take time) - self._test_mode_busy_wait(port) - - if self._httpd.poll() is not None: - raise errors.Error("Couldn't execute manual command") + def _perform_achall_with_script(self, achall): + env = dict(CERTBOT_DOMAIN=achall.domain, + CERTBOT_VALIDATION=achall.validation(achall.account_key)) + if isinstance(achall.chall, challenges.HTTP01): + env['CERTBOT_TOKEN'] = achall.chall.encode('token') else: - self._notify_and_wait( - self._get_message(achall).format( - validation=validation, - response=response, - uri=achall.chall.uri(achall.domain), - command=command)) + os.environ.pop('CERTBOT_TOKEN', None) + os.environ.update(env) + _, out = hooks.execute(self.conf('auth-hook')) + env['CERTBOT_AUTH_OUTPUT'] = out.strip() + self.env[achall.domain] = env - if not response.simple_verify( - achall.chall, achall.domain, - achall.account_key.public_key(), self.config.http01_port): - logger.warning("Self-verify of challenge failed.") - - return response - - def _perform_dns01_challenge(self, achall): - response, validation = achall.response_and_validation() - if not self.conf("test-mode"): - self._notify_and_wait( - self._get_message(achall).format( - validation=validation, - domain=achall.validation_domain_name(achall.domain), - response=response)) - - try: - verification_status = response.simple_verify( - achall.chall, achall.domain, - achall.account_key.public_key()) - except acme_errors.DependencyError: - logger.warning("Self verification requires optional " - "dependency `dnspython` to be installed.") + def _perform_achall_manually(self, achall): + validation = achall.validation(achall.account_key) + if isinstance(achall.chall, challenges.HTTP01): + msg = self._HTTP_INSTRUCTIONS.format( + achall=achall, encoded_token=achall.chall.encode('token'), + port=self.config.http01_port, + uri=achall.chall.uri(achall.domain), validation=validation) else: - if not verification_status: - logger.warning("Self-verify of challenge failed.") + assert isinstance(achall.chall, challenges.DNS01) + msg = self._DNS_INSTRUCTIONS.format( + domain=achall.validation_domain_name(achall.domain), + validation=validation) + display = zope.component.getUtility(interfaces.IDisplay) + display.notification(msg, wrap=False, force_interactive=True) - return response - - def _cleanup_http01_challenge(self, achall): - # pylint: disable=missing-docstring,unused-argument - if self.conf("test-mode"): - assert self._httpd is not None, ( - "cleanup() must be called after perform()") - if self._httpd.poll() is None: - logger.debug("Terminating manual command process") - self._httpd.terminate() - else: - logger.debug("Manual command process already terminated " - "with %s code", self._httpd.returncode) - shutil.rmtree(self._root) - - def _notify_and_wait(self, message): - # pylint: disable=no-self-use - # TODO: IDisplay wraps messages, breaking the command - #answer = zope.component.getUtility(interfaces.IDisplay).notification( - # message=message, pause=True) - sys.stdout.write(message) - six.moves.input("Press ENTER to continue") - - def _get_ip_logging_permission(self): - # pylint: disable=missing-docstring - if not (self.conf("test-mode") or self.conf("public-ip-logging-ok")): - if not zope.component.getUtility(interfaces.IDisplay).yesno( - self.IP_DISCLAIMER, "Yes", "No", - cli_flag="--manual-public-ip-logging-ok", - force_interactive=True): - raise errors.PluginError("Must agree to IP logging to proceed") - else: - self.config.namespace.manual_public_ip_logging_ok = True - - def _get_message(self, achall): - # pylint: disable=missing-docstring,no-self-use,unused-argument - return self.MESSAGE_TEMPLATE.get(achall.chall.typ, "") + def cleanup(self, achalls): # pylint: disable=missing-docstring + if self.conf('cleanup-hook'): + for achall in achalls: + env = self.env.pop(achall.domain) + if 'CERTBOT_TOKEN' not in env: + os.environ.pop('CERTBOT_TOKEN', None) + os.environ.update(env) + hooks.execute(self.conf('cleanup-hook')) diff --git a/certbot/plugins/manual_test.py b/certbot/plugins/manual_test.py index 154b0d729..247352256 100644 --- a/certbot/plugins/manual_test.py +++ b/certbot/plugins/manual_test.py @@ -1,134 +1,112 @@ -"""Tests for certbot.plugins.manual.""" +"""Tests for certbot.plugins.manual""" +import os import unittest +import six import mock from acme import challenges -from acme import errors as acme_errors -from acme import jose -from certbot import achallenges from certbot import errors - from certbot.tests import acme_util -from certbot.tests import util as test_util - - -KEY = jose.JWKRSA.load(test_util.load_vector("rsa512_key.pem")) class AuthenticatorTest(unittest.TestCase): """Tests for certbot.plugins.manual.Authenticator.""" def setUp(self): - from certbot.plugins.manual import Authenticator + self.http_achall = acme_util.HTTP01_A + self.dns_achall = acme_util.DNS01_A + self.achalls = [self.http_achall, self.dns_achall] self.config = mock.MagicMock( - http01_port=8080, manual_test_mode=False, - manual_public_ip_logging_ok=False, noninteractive_mode=True) - self.auth = Authenticator(config=self.config, name="manual") + http01_port=0, manual_auth_hook=None, manual_cleanup_hook=None, + manual_public_ip_logging_ok=False, noninteractive_mode=False, + validate_hooks=False) - self.http01 = achallenges.KeyAuthorizationAnnotatedChallenge( - challb=acme_util.HTTP01_P, domain="foo.com", account_key=KEY) - self.dns01 = achallenges.KeyAuthorizationAnnotatedChallenge( - challb=acme_util.DNS01_P, domain="foo.com", account_key=KEY) + from certbot.plugins.manual import Authenticator + self.auth = Authenticator(self.config, name='manual') - self.achalls = [self.http01, self.dns01] - - config_test_mode = mock.MagicMock( - http01_port=8080, manual_test_mode=True, noninteractive_mode=True) - self.auth_test_mode = Authenticator( - config=config_test_mode, name="manual") - - def test_prepare(self): + def test_prepare_no_hook_noninteractive(self): + self.config.noninteractive_mode = True self.assertRaises(errors.PluginError, self.auth.prepare) - self.auth_test_mode.prepare() # error not raised + + def test_prepare_bad_hook(self): + self.config.manual_auth_hook = os.path.abspath(os.sep) # is / on UNIX + self.config.validate_hooks = True + self.assertRaises(errors.HookCommandNotFound, self.auth.prepare) def test_more_info(self): - self.assertTrue(isinstance(self.auth.more_info(), str)) + self.assertTrue(isinstance(self.auth.more_info(), six.string_types)) def test_get_chall_pref(self): - self.assertTrue(all(issubclass(pref, challenges.Challenge) - for pref in self.auth.get_chall_pref("foo.com"))) + self.assertEqual(self.auth.get_chall_pref('example.org'), + [challenges.HTTP01, challenges.DNS01]) - @mock.patch("certbot.plugins.manual.zope.component.getUtility") - def test_perform_empty(self, mock_interaction): - mock_interaction().yesno.return_value = True - self.assertEqual([], self.auth.perform([])) + @mock.patch('certbot.plugins.manual.zope.component.getUtility') + def test_ip_logging_not_ok(self, mock_get_utility): + mock_get_utility().yesno.return_value = False + self.assertRaises(errors.PluginError, self.auth.perform, []) - @mock.patch("certbot.plugins.manual.zope.component.getUtility") - @mock.patch("certbot.plugins.manual.sys.stdout") - @mock.patch("acme.challenges.HTTP01Response.simple_verify") - @mock.patch("six.moves.input") - def test_perform(self, mock_raw_input, mock_verify, mock_stdout, mock_interaction): - mock_verify.return_value = True - mock_interaction().yesno.return_value = True + @mock.patch('certbot.plugins.manual.zope.component.getUtility') + def test_ip_logging_ok(self, mock_get_utility): + mock_get_utility().yesno.return_value = True + self.auth.perform([]) + self.assertTrue(self.config.manual_public_ip_logging_ok) - resp_http = self.http01.response(KEY) - resp_dns = self.dns01.response(KEY) + def test_script_perform(self): + self.config.manual_public_ip_logging_ok = True + self.config.manual_auth_hook = ( + 'echo $CERTBOT_DOMAIN; echo ${CERTBOT_TOKEN:-notoken}; ' + 'echo $CERTBOT_VALIDATION;') + dns_expected = '{0}\n{1}\n{2}'.format( + self.dns_achall.domain, 'notoken', + self.dns_achall.validation(self.dns_achall.account_key)) + http_expected = '{0}\n{1}\n{2}'.format( + self.http_achall.domain, self.http_achall.chall.encode('token'), + self.http_achall.validation(self.http_achall.account_key)) - self.assertEqual([resp_http, resp_dns], self.auth.perform(self.achalls)) - self.assertEqual(2, mock_raw_input.call_count) - mock_verify.assert_called_with( - self.http01.challb.chall, "foo.com", KEY.public_key(), 8080) + self.assertEqual( + self.auth.perform(self.achalls), + [achall.response(achall.account_key) for achall in self.achalls]) + self.assertEqual( + self.auth.env[self.dns_achall.domain]['CERTBOT_AUTH_OUTPUT'], + dns_expected) + self.assertEqual( + self.auth.env[self.http_achall.domain]['CERTBOT_AUTH_OUTPUT'], + http_expected) - message = mock_stdout.write.mock_calls[0][1][0] - self.assertTrue(self.http01.chall.encode("token") in message) + @mock.patch('certbot.plugins.manual.zope.component.getUtility') + def test_manual_perform(self, mock_get_utility): + self.config.manual_public_ip_logging_ok = True + self.assertEqual( + self.auth.perform(self.achalls), + [achall.response(achall.account_key) for achall in self.achalls]) + for i, (args, kwargs) in enumerate(mock_get_utility().notification.call_args_list): + achall = self.achalls[i] + self.assertTrue(achall.validation(achall.account_key) in args[0]) + self.assertFalse(kwargs['wrap']) - mock_verify.return_value = False - with mock.patch("certbot.plugins.manual.logger") as mock_logger: - self.auth.perform(self.achalls) - self.assertEqual(2, mock_logger.warning.call_count) + def test_cleanup(self): + self.config.manual_public_ip_logging_ok = True + self.config.manual_auth_hook = 'echo foo;' + self.config.manual_cleanup_hook = '# cleanup' + self.auth.perform(self.achalls) - @mock.patch("certbot.plugins.manual.zope.component.getUtility") - @mock.patch("acme.challenges.DNS01Response.simple_verify") - @mock.patch("six.moves.input") - def test_perform_missing_dependency(self, mock_raw_input, mock_verify, mock_interaction): - mock_interaction().yesno.return_value = True - mock_verify.side_effect = acme_errors.DependencyError() + for achall in self.achalls: + self.auth.cleanup([achall]) + self.assertEqual(os.environ['CERTBOT_AUTH_OUTPUT'], 'foo') + self.assertEqual(os.environ['CERTBOT_DOMAIN'], achall.domain) + self.assertEqual( + os.environ['CERTBOT_VALIDATION'], + achall.validation(achall.account_key)) - with mock.patch("certbot.plugins.manual.logger") as mock_logger: - self.auth.perform([self.dns01]) - self.assertEqual(1, mock_logger.warning.call_count) - - mock_raw_input.assert_called_once_with("Press ENTER to continue") - - @mock.patch("certbot.plugins.manual.zope.component.getUtility") - @mock.patch("certbot.plugins.manual.Authenticator._notify_and_wait") - def test_disagree_with_ip_logging(self, mock_notify, mock_interaction): - mock_interaction().yesno.return_value = False - mock_notify.side_effect = errors.Error("Exception not raised, \ - continued execution even after disagreeing with IP logging") - - self.assertRaises(errors.PluginError, self.auth.perform, self.achalls) - - @mock.patch("certbot.plugins.manual.subprocess.Popen", autospec=True) - def test_perform_test_command_oserror(self, mock_popen): - mock_popen.side_effect = OSError - self.assertEqual([False], self.auth_test_mode.perform([self.http01])) - - @mock.patch("certbot.plugins.manual.socket.socket") - @mock.patch("certbot.plugins.manual.time.sleep", autospec=True) - @mock.patch("certbot.plugins.manual.subprocess.Popen", autospec=True) - def test_perform_test_command_run_failure( - self, mock_popen, unused_mock_sleep, unused_mock_socket): - mock_popen.poll.return_value = 10 - mock_popen.return_value.pid = 1234 - self.assertRaises( - errors.Error, self.auth_test_mode.perform, self.achalls) - - def test_cleanup_test_mode_already_terminated(self): - # pylint: disable=protected-access - self.auth_test_mode._httpd = httpd = mock.Mock() - httpd.poll.return_value = 0 - self.auth_test_mode.cleanup(self.achalls) - - def test_cleanup_test_mode_kills_still_running(self): - # pylint: disable=protected-access - self.auth_test_mode._httpd = httpd = mock.Mock(pid=1234) - httpd.poll.return_value = None - self.auth_test_mode.cleanup(self.achalls) - httpd.terminate.assert_called_once_with() + if isinstance(achall.chall, challenges.HTTP01): + self.assertEqual( + os.environ['CERTBOT_TOKEN'], + achall.chall.encode('token')) + else: + self.assertFalse('CERTBOT_TOKEN' in os.environ) -if __name__ == "__main__": +if __name__ == '__main__': unittest.main() # pragma: no cover diff --git a/certbot/plugins/script.py b/certbot/plugins/script.py deleted file mode 100644 index 049ee8c96..000000000 --- a/certbot/plugins/script.py +++ /dev/null @@ -1,161 +0,0 @@ -"""Script-based Authenticator.""" -import logging -import os -import sys - -import zope.interface - -from acme import challenges - -from certbot import errors -from certbot import interfaces -from certbot import hooks - -from certbot.plugins import common - -logger = logging.getLogger(__name__) - - -CHALLENGES = ["http-01", "dns-01"] - - -@zope.interface.implementer(interfaces.IAuthenticator) -@zope.interface.provider(interfaces.IPluginFactory) -class Authenticator(common.Plugin): - """Script authenticator - - calls user defined script to perform authentication and - optionally cleanup. - - """ - - description = "Authenticate using user provided script(s)" - - long_description = ("Authenticate using user provided script(s). " + - "Authenticator script has the following environment " + - "variables available for it: " + - "CERTBOT_DOMAIN - The domain being authenticated " + - "CERTBOT_VALIDATION - The validation string " + - "CERTBOT_TOKEN - Resource name part of HTTP-01 " + - "challenge (HTTP-01 only). " + - "Cleanup script has all the above, and additional " + - "var: CERTBOT_AUTH_OUTPUT - stdout output from the " + - "authenticator" - ) - - def __init__(self, *args, **kwargs): - super(Authenticator, self).__init__(*args, **kwargs) - self.cleanup_script = None - self.auth_script = None - self.challenges = [] - - @classmethod - def add_parser_arguments(cls, add): - add("auth", default=None, required=False, - help="path or command for the authentication script") - add("cleanup", default=None, required=False, - help="path or command for the cleanup script") - - @property - def supported_challenges(self): - """Challenges supported by this plugin.""" - return self.challenges - - def more_info(self): # pylint: disable=missing-docstring - return("This authenticator enables user to perform authentication " + - "using shell script(s).") - - def prepare(self): - """Prepare script plugin, check challenge, scripts and register them""" - pref_challenges = self.config.pref_challs - for c in pref_challenges: - if c.typ in CHALLENGES: - self.challenges.append(c) - if not self.challenges and len(pref_challenges): - # Challenges requested, but not supported - raise errors.PluginError( - "Unfortunately script plugin doesn't yet support " + - "the requested challenges") - - # Challenge not defined on cli, set default - if not self.challenges: - self.challenges.append(challenges.Challenge.TYPES["http-01"]) - - if not self.conf("auth"): - raise errors.PluginError("Parameter --script-auth is required " + - "for script plugin") - self._prepare_scripts() - - def _prepare_scripts(self): - """Helper method for prepare, to take care of validating scripts""" - script_path = self.conf("auth") - cleanup_path = self.conf("cleanup") - if self.config.validate_hooks: - hooks.validate_hook(script_path, "script_auth") - self.auth_script = script_path - if cleanup_path: - if self.config.validate_hooks: - hooks.validate_hook(cleanup_path, "script_cleanup") - self.cleanup_script = cleanup_path - - def get_chall_pref(self, domain): - """Return challenge(s) we're answering to """ - # pylint: disable=unused-argument - return self.challenges - - def perform(self, achalls): - """Perform the authentication per challenge""" - mapping = {"http-01": self._setup_env_http, - "dns-01": self._setup_env_dns} - responses = [] - for achall in achalls: - response, validation = achall.response_and_validation() - # Setup env vars - mapping[achall.typ](achall, validation) - output = self.execute(self.auth_script) - if output: - self._write_auth_output(output) - responses.append(response) - return responses - - def _setup_env_http(self, achall, validation): - """Write environment variables for http challenge""" - ev = dict() - ev["CERTBOT_TOKEN"] = achall.chall.encode("token") - ev["CERTBOT_VALIDATION"] = validation - ev["CERTBOT_DOMAIN"] = achall.domain - os.environ.update(ev) - - def _setup_env_dns(self, achall, validation): - """Write environment variables for dns challenge""" - ev = dict() - ev["CERTBOT_VALIDATION"] = validation - ev["CERTBOT_DOMAIN"] = achall.domain - os.environ.update(ev) - - def _write_auth_output(self, out): - """Write output from auth script to env var for - cleanup to act upon""" - os.environ.update({"CERTBOT_AUTH_OUTPUT": out.strip()}) - - def _normalize_string(self, value): - """Return string instead of bytestring for Python3. - Helper function for writing env vars, as os.environ needs str""" - - if isinstance(value, bytes): - value = value.decode(sys.getdefaultencoding()) - return str(value) - - def execute(self, shell_cmd): - """Run a script. - - :param str shell_cmd: Command to run - :returns: `str` stdout output""" - - _, out = hooks.execute(shell_cmd) - return self._normalize_string(out) - - def cleanup(self, achalls): # pylint: disable=unused-argument - """Run cleanup.sh """ - if self.cleanup_script: - self.execute(self.cleanup_script) diff --git a/certbot/plugins/script_test.py b/certbot/plugins/script_test.py deleted file mode 100644 index 1fe57a8dc..000000000 --- a/certbot/plugins/script_test.py +++ /dev/null @@ -1,170 +0,0 @@ -"""Tests for certbot.plugins.manual.""" -import os -import tempfile -import unittest - -import mock - -from acme import challenges -from acme import jose - -from certbot import achallenges -from certbot import errors - -from certbot.tests import acme_util -from certbot.tests import util as test_util - - -KEY = jose.JWKRSA.load(test_util.load_vector("rsa512_key.pem")) - - -class AuthenticatorTest(unittest.TestCase): - """Tests for certbot.plugins.script.Authenticator.""" - - def setUp(self): - from certbot.plugins.script import Authenticator - self.auth_return_value = "return from auth\n" - self.script_nonexec = create_script(b'# empty') - self.script_exec = create_script_exec(b'echo "return from auth\n"') - self.config = mock.MagicMock( - script_auth=self.script_exec, - script_cleanup=self.script_exec, - pref_challs=[challenges.Challenge.TYPES["http-01"], - challenges.Challenge.TYPES["dns-01"], - challenges.Challenge.TYPES["tls-sni-01"]]) - - self.tlssni_config = mock.MagicMock( - script_auth=self.script_exec, - script_cleanup=self.script_exec, - pref_challs=[challenges.Challenge.TYPES["tls-sni-01"]]) - - self.nochall_config = mock.MagicMock( - script_auth=self.script_exec, - script_cleanup=self.script_exec, - ) - - self.default = Authenticator(config=self.config, name="script") - self.onlytlssni = Authenticator(config=self.tlssni_config, - name="script") - self.nochall = Authenticator(config=self.nochall_config, - name="script") - - self.http01 = achallenges.KeyAuthorizationAnnotatedChallenge( - challb=acme_util.HTTP01_P, domain="foo.com", account_key=KEY) - self.dns01 = achallenges.KeyAuthorizationAnnotatedChallenge( - challb=acme_util.DNS01_P, domain="foo.com", account_key=KEY) - - self.achalls = [self.http01, self.dns01] - - def tearDown(self): - os.remove(self.script_exec) - os.remove(self.script_nonexec) - - def test_prepare_normal(self): - """Test prepare with typical configuration""" - from certbot.plugins.script import Authenticator - # Erroring combinations in from of (auth_script, cleanup_script, error) - for v in [("/NONEXISTENT/script.sh", "/NONEXISTENT/script.sh", - errors.HookCommandNotFound), - (self.script_nonexec, "/NONEXISTENT/script.sh", - errors.HookCommandNotFound), - (self.script_exec, "/NONEXISTENT/script.sh", - errors.HookCommandNotFound), - ("/NONEXISTENT/script.sh", self.script_nonexec, - errors.HookCommandNotFound), - ("/NONEXISTENT/script.sh", self.script_exec, - errors.HookCommandNotFound), - (None, self.script_exec, - errors.PluginError)]: - testconf = mock.MagicMock( - script_auth=v[0], - script_cleanup=v[1], - pref_challs=[challenges.Challenge.TYPES["http-01"]]) - testauth = Authenticator(config=testconf, name="script") - self.assertRaises(v[2], testauth.prepare) - - # This should not error - self.default.prepare() - self.assertEqual(len(self.default.challenges), 2) - - def test_prepare_tlssni(self): - """Test for provided, but unsupported challenge type""" - self.assertRaises(errors.PluginError, self.onlytlssni.prepare) - - def test_prepare_nochall(self): - """Test for default challenge""" - self.nochall.prepare() - self.assertEqual(len(self.nochall.challenges), 1) - - def test_more_info(self): - self.assertTrue(isinstance(self.default.more_info(), str)) - - def test_get_chall_pref(self): - self.default.prepare() - self.assertTrue(all(issubclass(pref, challenges.Challenge) - for pref in self.default.get_chall_pref( - "foo.com"))) - - def test_get_supported_challenges(self): - self.default.prepare() - self.assertTrue(all(issubclass(sup, challenges.Challenge) - for sup in self.default.supported_challenges)) - - def test_perform(self): - resp_http = self.http01.response(KEY) - resp_dns = self.dns01.response(KEY) - self.default.prepare() - # Check for the env vars prior to the run - self.assertFalse("CERTBOT_VALIDATION" in os.environ.keys()) - self.assertFalse("CERTBOT_DOMAIN" in os.environ.keys()) - self.assertFalse("CERTBOT_AUTH_OUTPUT" in os.environ.keys()) - - pref_resp = self.default.perform(self.achalls) - self.assertEqual([resp_http, resp_dns], pref_resp) - # Check for the env vars post run - self.assertTrue("CERTBOT_VALIDATION" in os.environ.keys()) - self.assertTrue("CERTBOT_DOMAIN" in os.environ.keys()) - self.assertTrue("CERTBOT_AUTH_OUTPUT" in os.environ.keys()) - self.assertEqual(os.environ["CERTBOT_AUTH_OUTPUT"], - self.auth_return_value.strip()) - - @mock.patch('certbot.plugins.script.Authenticator.execute') - def test_cleanup(self, mock_exec): - mock_exec.return_value = (0, None, None) - self.default.prepare() - self.default.cleanup(self.achalls) - self.assertEqual(mock_exec.call_count, 1) - - @mock.patch('certbot.hooks.Popen') - def test_execute(self, mock_popen): - proc = mock.Mock() - # tuple values: stdout, stderr, errorcode, num_of_logger_calls - for t in [("", "", 0, 0), - (self.auth_return_value, "", 0, 0), - (None, "stderr_output", 0, 1), - ("whatever", "stderr_output", 1, 2), - (b'bytestring outval', "", 0, 0)]: - proc = mock.Mock() - attrs = {'communicate.return_value': (t[0], t[1]), - 'returncode': t[2]} - proc.configure_mock(**attrs) # pylint: disable=star-args - mock_popen.return_value = proc - with mock.patch('certbot.hooks.logger.error') as mock_log: - output = self.default.execute(self.script_exec) - self.assertEqual(mock_log.call_count, t[3]) - self.assertTrue(isinstance(output, str)) - - -def create_script(contents): - """ Helper to create temporary file """ - f = tempfile.NamedTemporaryFile(delete=False, prefix='.sh') - f.write(contents) - f.close() - return f.name - - -def create_script_exec(contents): - """ Helper to create temporary file with exec permissions""" - fname = create_script(contents) - os.chmod(fname, 0o700) - return fname diff --git a/certbot/plugins/selection.py b/certbot/plugins/selection.py index 16932232a..81387c435 100644 --- a/certbot/plugins/selection.py +++ b/certbot/plugins/selection.py @@ -133,7 +133,7 @@ def choose_plugin(prepared, question): else: return None -noninstaller_plugins = ["webroot", "manual", "standalone", "script"] +noninstaller_plugins = ["webroot", "manual", "standalone"] def record_chosen_plugins(config, plugins, auth, inst): "Update the config entries to reflect the plugins we actually selected." @@ -238,8 +238,6 @@ def cli_plugin_requests(config): req_auth = set_configurator(req_auth, "webroot") if config.manual: req_auth = set_configurator(req_auth, "manual") - if config.script: - req_auth = set_configurator(req_auth, "script") logger.debug("Requested authenticator %s and installer %s", req_auth, req_inst) return req_auth, req_inst diff --git a/certbot/tests/acme_util.py b/certbot/tests/acme_util.py index 3168349c9..5e6b190a7 100644 --- a/certbot/tests/acme_util.py +++ b/certbot/tests/acme_util.py @@ -7,9 +7,12 @@ from acme import challenges from acme import jose from acme import messages +from certbot import auth_handler + from certbot.tests import util +JWK = jose.JWK.load(util.load_vector('rsa512_key.pem')) KEY = util.load_rsa_private_key('rsa512_key.pem') # Challenges @@ -50,6 +53,14 @@ DNS01_P = chall_to_challb(DNS01, messages.STATUS_PENDING) CHALLENGES_P = [HTTP01_P, TLSSNI01_P, DNS01_P] +# AnnotatedChallenge objects +HTTP01_A = auth_handler.challb_to_achall(HTTP01_P, JWK, "example.com") +TLSSNI01_A = auth_handler.challb_to_achall(TLSSNI01_P, JWK, "example.net") +DNS01_A = auth_handler.challb_to_achall(DNS01_P, JWK, "example.org") + +ACHALLENGES = [HTTP01_A, TLSSNI01_A, DNS01_A] + + def gen_authzr(authz_status, domain, challs, statuses, combos=True): """Generate an authorization resource. diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index e3bd28a5e..9404a8385 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -81,7 +81,7 @@ class ParseTest(unittest.TestCase): out = self._help_output(['--help', 'all']) self.assertTrue("--configurator" in out) self.assertTrue("how a cert is deployed" in out) - self.assertTrue("--manual-test-mode" in out) + self.assertTrue("--webroot-path" in out) self.assertTrue("--text" not in out) self.assertTrue("--dialog" not in out) self.assertTrue("%s" not in out) @@ -91,7 +91,7 @@ class ParseTest(unittest.TestCase): if "nginx" in self.plugins: # may be false while building distributions without plugins self.assertTrue("--nginx-ctl" in out) - self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--webroot-path" not in out) self.assertTrue("--checkpoints" not in out) out = self._help_output(['-h']) @@ -102,7 +102,7 @@ class ParseTest(unittest.TestCase): self.assertTrue("(the certbot nginx plugin is not" in out) out = self._help_output(['--help', 'plugins']) - self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--webroot-path" not in out) self.assertTrue("--prepare" in out) self.assertTrue('"plugins" subcommand' in out) @@ -305,22 +305,22 @@ class SetByCliTest(unittest.TestCase): 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') + 'manual_auth_hook') + cli.report_config_interaction('manual_auth_hook', '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',)) + ('manual_auth_hook',)) + cli.report_config_interaction(('manual_auth_hook',), ('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 implies --manual-auth-hook which implies --manual-public-ip-logging-ok. These interactions don't actually exist in the client, but are used here for testing purposes. @@ -328,13 +328,13 @@ class SetByCliTest(unittest.TestCase): args = ['--manual'] verb = 'renew' - for v in ('manual', 'manual_test_mode', 'manual_public_ip_logging_ok'): + for v in ('manual', 'manual_auth_hook', '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'): + args = ['--manual-auth-hook', 'command'] + for v in ('manual_auth_hook', 'manual_public_ip_logging_ok'): self.assertTrue(_call_set_by_cli(v, args, verb)) self.assertFalse(_call_set_by_cli('manual', args, verb)) diff --git a/setup.py b/setup.py index 46dbdac81..4227d5d92 100644 --- a/setup.py +++ b/setup.py @@ -131,7 +131,6 @@ setup( 'null = certbot.plugins.null:Installer', 'standalone = certbot.plugins.standalone:Authenticator', 'webroot = certbot.plugins.webroot:Authenticator', - 'script = certbot.plugins.script:Authenticator', ], }, ) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index a70f13f8e..e7975454b 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -44,7 +44,12 @@ python_server_pid=$! common --domains le2.wtf --preferred-challenges http-01 run kill $python_server_pid -common -a manual -d le.wtf auth --rsa-key-size 4096 +common certonly -a manual -d le.wtf --rsa-key-size 4096 \ + --manual-auth-hook ./tests/manual-http-auth.sh \ + --manual-cleanup-hook ./tests/manual-http-cleanup.sh + +common certonly -a manual -d dns.le.wtf --preferred-challenges dns-01 \ + --manual-auth-hook ./tests/manual-dns-auth.sh export CSR_PATH="${root}/csr.der" KEY_PATH="${root}/key.pem" \ OPENSSL_CNF=examples/openssl.cnf diff --git a/tests/integration/_common.sh b/tests/integration/_common.sh index 8d01ad763..12924fe21 100755 --- a/tests/integration/_common.sh +++ b/tests/integration/_common.sh @@ -25,7 +25,7 @@ certbot_test_no_force_renew () { --no-verify-ssl \ --tls-sni-01-port $tls_sni_01_port \ --http-01-port $http_01_port \ - --manual-test-mode \ + --manual-public-ip-logging-ok \ $store_flags \ --non-interactive \ --no-redirect \ diff --git a/tests/manual-dns-auth.sh b/tests/manual-dns-auth.sh new file mode 100755 index 000000000..9b9a1a5eb --- /dev/null +++ b/tests/manual-dns-auth.sh @@ -0,0 +1,4 @@ +#!/bin/sh +curl -X POST 'http://localhost:8055/set-txt' -d \ + "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\", \ + \"value\": \"$CERTBOT_VALIDATION\"}" diff --git a/tests/manual-http-auth.sh b/tests/manual-http-auth.sh new file mode 100755 index 000000000..c4730392b --- /dev/null +++ b/tests/manual-http-auth.sh @@ -0,0 +1,12 @@ +#!/bin/sh +uri_path=".well-known/acme-challenge/$CERTBOT_TOKEN" + +cd $(mktemp -d) +mkdir -p $(dirname $uri_path) +echo $CERTBOT_VALIDATION > $uri_path +python -m SimpleHTTPServer $http_01_port >/dev/null 2>&1 & +server_pid=$! +while ! curl "http://localhost:$http_01_port/$uri_path" >/dev/null 2>&1; do + sleep 1s +done +echo $server_pid diff --git a/tests/manual-http-cleanup.sh b/tests/manual-http-cleanup.sh new file mode 100755 index 000000000..5e437bf08 --- /dev/null +++ b/tests/manual-http-cleanup.sh @@ -0,0 +1,2 @@ +#!/bin/sh +kill $CERTBOT_AUTH_OUTPUT From 19143d83036cdcbfc98c16f202a207c888ce18e3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 22 Dec 2016 13:07:00 -0800 Subject: [PATCH 39/44] Increase test coverage --- certbot/tests/cert_manager_test.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 6caaab878..07f7cedaa 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -181,8 +181,8 @@ class CertificatesTest(BaseCertManagerTest): shutil.rmtree(tempdir) @mock.patch('certbot.cert_manager.ocsp.RevocationChecker.ocsp_revoked') - def test_report_human_readable(self, mock_ocsp): - mock_ocsp.return_value = None + def test_report_human_readable(self, mock_revoked): + mock_revoked.return_value = None from certbot import cert_manager import datetime, pytz expiry = pytz.UTC.fromutc(datetime.datetime.utcnow()) @@ -221,8 +221,9 @@ class CertificatesTest(BaseCertManagerTest): self.assertTrue('VALID' in out and not 'INVALID' in out) cert.is_test_cert = True + mock_revoked.return_value = True out = get_report() - self.assertTrue('INVALID: TEST_CERT' in out) + self.assertTrue('INVALID: TEST_CERT, REVOKED' in out) cert = mock.MagicMock(lineagename="indescribable") cert.target_expiry = expiry From 9aa93c05c1a4b88ab939660737db3d5e70d1b86d Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 22 Dec 2016 15:35:29 -0800 Subject: [PATCH 40/44] Simplify the ocsp_revoked() return type - we weren't reacting to None, so call it False instead --- certbot/ocsp.py | 12 ++++++------ certbot/tests/ocsp_test.py | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/certbot/ocsp.py b/certbot/ocsp.py index a5df5743d..2e0514a44 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -37,17 +37,17 @@ class RevocationChecker(object): :param str cert_path: Path to certificate :param str chain_path: Path to intermediate cert :rtype bool or None: - :returns: False if valid; True if revoked; None if check itself failed + :returns: True if revoked; False if valid or the check failed """ if self.broken: - return None + return False logger.debug("Querying OCSP for %s", cert_path) url, host = self.determine_ocsp_server(cert_path) if not host: - return None + return False # jdkasten thanks "Bulletproof SSL and TLS - Ivan Ristic" for documenting this! cmd = ["openssl", "ocsp", "-no_nonce", @@ -62,7 +62,7 @@ class RevocationChecker(object): except errors.SubprocessError as e: logger.info("OCSP check failed for %s (are we offline?)", cert_path) logger.debug("Command was:\n%s\nError was:\n%s", " ".join(cmd), e) - return None + return False return _translate_ocsp_query(cert_path, output, err) @@ -98,12 +98,12 @@ def _translate_ocsp_query(cert_path, ocsp_output, ocsp_errors): if not "Response verify OK" in ocsp_errors: logger.info("Revocation status for %s is unknown", cert_path) logger.debug("Uncertain ouput:\n%s\nstderr:\n%s", ocsp_output, ocsp_errors) - return None + return False if cert_path + ": good" in ocsp_output: return False elif cert_path + ": revoked" in ocsp_output: return True else: logger.warn("Unable to properly parse OCSP output: %s", ocsp_output) - return None + return False diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index c4517174d..ff79bb01e 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -59,17 +59,17 @@ class OCSPTest(unittest.TestCase): def test_ocsp_revoked(self, mock_run, mock_determine): self.checker.broken = True mock_determine.return_value = ("", "") - self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) self.checker.broken = False mock_run.return_value = tuple(openssl_happy[1:]) - self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) self.assertEqual(mock_run.call_count, 0) mock_determine.return_value = ("http://x.co", "x.co") self.assertEqual(self.checker.ocsp_revoked("blah.pem", "chain.pem"), False) mock_run.side_effect = errors.SubprocessError("Unable to load certificate launcher") - self.assertEqual(self.checker.ocsp_revoked("x", "y"), None) + self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) self.assertEqual(mock_run.call_count, 2) @@ -97,10 +97,10 @@ class OCSPTest(unittest.TestCase): mock_run.return_value = openssl_confused from certbot import ocsp self.assertEqual(ocsp._translate_ocsp_query(*openssl_happy), False) - self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), None) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_confused), False) self.assertEqual(mock_log.debug.call_count, 1) self.assertEqual(mock_log.warn.call_count, 0) - self.assertEqual(ocsp._translate_ocsp_query(*openssl_broken), None) + self.assertEqual(ocsp._translate_ocsp_query(*openssl_broken), False) self.assertEqual(mock_log.warn.call_count, 1) self.assertEqual(ocsp._translate_ocsp_query(*openssl_revoked), True) From 12edbb53dbc28e22eab16649989ce99a0da724c1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 23 Dec 2016 10:49:51 -0800 Subject: [PATCH 41/44] Fixes #3954 and adds a test to prevent regressions (#3957) * fixes #3954 and adds test to prevent regressions * assure pylint I know what I'm doing * Test FileDisplay methods take force_interactive --- certbot/display/util.py | 2 +- certbot/tests/display/util_test.py | 21 ++++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/certbot/display/util.py b/certbot/display/util.py index ebfce78e2..502426626 100644 --- a/certbot/display/util.py +++ b/certbot/display/util.py @@ -438,7 +438,7 @@ class NoninteractiveDisplay(object): line=os.linesep, frame=side_frame, msg=message)) def menu(self, message, choices, ok_label=None, cancel_label=None, - help_label=None, default=None, cli_flag=None, *unused_kwargs): + help_label=None, default=None, cli_flag=None, **unused_kwargs): # pylint: disable=unused-argument,too-many-arguments """Avoid displaying a menu. diff --git a/certbot/tests/display/util_test.py b/certbot/tests/display/util_test.py index 7b6bd842e..10ec463ba 100644 --- a/certbot/tests/display/util_test.py +++ b/certbot/tests/display/util_test.py @@ -1,10 +1,12 @@ """Test :mod:`certbot.display.util`.""" +import inspect import os import unittest import mock -import certbot.errors as errors +from certbot import errors +from certbot import interfaces from certbot.display import util as display_util @@ -259,6 +261,13 @@ class FileOutputDisplayTest(unittest.TestCase): self.displayer._get_valid_int_ans(3), (display_util.CANCEL, -1)) + def test_methods_take_force_interactive(self): + # Every IDisplay method implemented by FileDisplay must take + # force_interactive to prevent workflow regressions. + for name in interfaces.IDisplay.names(): # pylint: disable=no-member + arg_spec = inspect.getargspec(getattr(self.displayer, name)) + self.assertTrue("force_interactive" in arg_spec.args) + class NoninteractiveDisplayTest(unittest.TestCase): """Test non-interactive display. @@ -309,6 +318,16 @@ class NoninteractiveDisplayTest(unittest.TestCase): self.assertRaises( errors.MissingCommandlineFlag, self.displayer.directory_select, "msg") + def test_methods_take_kwargs(self): + # Every IDisplay method implemented by NoninteractiveDisplay + # should take **kwargs because every method of FileDisplay must + # take force_interactive which doesn't apply to + # NoninteractiveDisplay. + for name in interfaces.IDisplay.names(): # pylint: disable=no-member + method = getattr(self.displayer, name) + # asserts method accepts arbitrary keyword arguments + self.assertFalse(inspect.getargspec(method).keywords is None) + class SeparateListInputTest(unittest.TestCase): """Test Module functions.""" From 1946af289f6de89005c8239a2c2c54aab12e82a8 Mon Sep 17 00:00:00 2001 From: Spencer Bliven Date: Tue, 3 Jan 2017 18:09:37 +0100 Subject: [PATCH 42/44] Minor typo fix (#3966) --- docs/using.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/using.rst b/docs/using.rst index 0aeed58b9..349867da4 100644 --- a/docs/using.rst +++ b/docs/using.rst @@ -61,7 +61,7 @@ manual_ Y N | Helps you obtain a cert by giving you instructions to pe Under the hood, plugins use one of several ACME protocol "Challenges_" to prove you control a domain. The options are http-01_ (which uses port 80), tls-sni-01_ (port 443) and dns-01_ (requring configuration of a DNS server on -port 53, thought that's often not the same machine as your webserver). A few +port 53, though that's often not the same machine as your webserver). A few plugins support more than one challenge type, in which case you can choose one with ``--preferred-challenges``. From 8794c4fd4104544a355ed0254d01a57f4f439fda Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 3 Jan 2017 13:49:59 -0800 Subject: [PATCH 43/44] Document some particularities of the revoke subcommand (#3923) * Document some particularities of the revoke subcommand * Add --test-cert to "run" help topic --- certbot/cli.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index d0964e4a2..6c513b4a1 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -361,7 +361,8 @@ VERB_HELP = [ }), ("revoke", { "short": "Revoke a certificate specified with --cert-path", - "opts": "Options for revocation of certs" + "opts": "Options for revocation of certs", + "usage": "\n\n certbot revoke --cert-path /path/to/fullchain.pem [options]\n\n" }), ("rename", { "short": "Change a certificate's name (for management purposes)", @@ -934,8 +935,9 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="Silence all output except errors. Useful for automation via cron." " Implies --non-interactive.") # overwrites server, handled in HelpfulArgumentParser.parse_args() - helpful.add("testing", "--test-cert", "--staging", action='store_true', dest='staging', - help='Use the staging server to obtain test (invalid) certs; equivalent' + helpful.add(["testing", "revoke", "run"], "--test-cert", "--staging", + action='store_true', dest='staging', + help='Use the staging server to obtain or revoke test (invalid) certs; equivalent' ' to --server ' + constants.STAGING_URI) helpful.add( "testing", "--debug", action="store_true", From 3b233df2b176525e52e7855032e76d2776af46f2 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Wed, 4 Jan 2017 11:42:52 -0800 Subject: [PATCH 44/44] Update docs/contributing.rst to match display behavior during release. (#3674) --- docs/contributing.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/contributing.rst b/docs/contributing.rst index a5b9b5688..de520dc0e 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -246,9 +246,8 @@ configuration checkpoints and rollback. Display ~~~~~~~ -We currently offer a pythondialog and "text" mode for displays. Display -plugins implement the `~certbot.interfaces.IDisplay` -interface. +We currently only offer a "text" mode for displays. Display plugins +implement the `~certbot.interfaces.IDisplay` interface. .. _dev-plugin: