From 8d8a95800c9fe05369cfb635dd3dfd14f545b259 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sat, 6 Feb 2016 12:14:42 -0800 Subject: [PATCH 01/15] Preliminary fix for #2386 --- letsencrypt/cli.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 838da4015..fc5a5439b 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -704,12 +704,18 @@ def obtain_cert(config, plugins, lineage=None): if config.dry_run: _report_successful_dry_run() - elif config.verb == "renew" and installer is not None: - # In case of a renewal, reload server to pick up new certificate. - # In principle we could have a configuration option to inhibit this - # from happening. - installer.restart() - print("reloaded") + elif config.verb == "renew": + if installer is None: + # Tell the user that the server was not restarted. + print("new certificate deployed without restart, fullchain", + lineage.fullchain) + else: + # In case of a renewal, reload server to pick up new certificate. + # In principle we could have a configuration option to inhibit this + # from happening. + installer.restart() + print("new certificate deployed with restart of plugin", + config.installer, "fullchain is", lineage.fullchain) _suggest_donation_if_appropriate(config) From a3fd5c73a6f713d9b3fa2125717e3af780b19da0 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sat, 6 Feb 2016 12:16:10 -0800 Subject: [PATCH 02/15] =?UTF-8?q?restart=20=E2=86=92=20reload?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- letsencrypt/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index fc5a5439b..7d90361ef 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -707,14 +707,14 @@ def obtain_cert(config, plugins, lineage=None): elif config.verb == "renew": if installer is None: # Tell the user that the server was not restarted. - print("new certificate deployed without restart, fullchain", + print("new certificate deployed without reload, fullchain", lineage.fullchain) else: # In case of a renewal, reload server to pick up new certificate. # In principle we could have a configuration option to inhibit this # from happening. installer.restart() - print("new certificate deployed with restart of plugin", + print("new certificate deployed with reload of plugin", config.installer, "fullchain is", lineage.fullchain) _suggest_donation_if_appropriate(config) From 9c7af6a93f72d73f50d87ba1715995c429d00061 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 13:22:49 -0800 Subject: [PATCH 03/15] Better reporting of renewal results --- letsencrypt/cli.py | 67 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 7d90361ef..8566765f8 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -471,7 +471,7 @@ def _auth_from_domains(le_client, config, domains, lineage=None): if lineage is False: raise errors.Error("Certificate could not be obtained") - if not config.dry_run: + if not config.dry_run and not config.verb == "renew": _report_new_cert(lineage.cert, lineage.fullchain) return lineage, action @@ -681,7 +681,9 @@ def obtain_cert(config, plugins, lineage=None): # installers are used in auth mode to determine domain names installer, authenticator = choose_configurator_plugins(config, plugins, "certonly") except errors.PluginSelectionError as e: - return e.message + logger.info( + "Could not choose appropriate plugin: %s", e) + raise # TODO: Handle errors from _init_le_client? le_client = _init_le_client(config, authenticator, installer) @@ -707,7 +709,7 @@ def obtain_cert(config, plugins, lineage=None): elif config.verb == "renew": if installer is None: # Tell the user that the server was not restarted. - print("new certificate deployed without reload, fullchain", + print("new certificate deployed without reload, fullchain is", lineage.fullchain) else: # In case of a renewal, reload server to pick up new certificate. @@ -877,6 +879,30 @@ def _renewal_conf_files(config): return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf")) +def _renew_describe_results(renew_successes, renew_failures, parse_failures): + print() + if not renew_successes and not renew_failures: + print("No renewals were attempted.") + elif renew_successes and not renew_failures: + print("Congratulations, all renewals succeeded. The following certs " + "have been renewed:") + print("\t" + "\n\t".join(x + " (success)" for x in renew_successes)) + elif renew_failures and not renew_successes: + print("All renewal attempts failed. The following certs could not be " + "renewed:") + print("\t" + "\n\t".join(x + " (failure)" for x in renew_failures)) + elif renew_failures and renew_successes: + print("The following certs were successfully renewed:") + print("\t" + "\n\t".join(x + " (success)" for x in renew_successes)) + print("\nThe following certs could not be renewed:") + print("\t" + "\n\t".join(x + " (failure)" for x in renew_failures)) + + if parse_failures: + print("\nAdditionally, the following renewal configuration files " + "were invalid: ") + print("\t" + "\n\t".join(x + " (parsefail)" for x in parse_failures)) + + def renew(config, unused_plugins): """Renew previously-obtained certificates.""" if config.domains != []: @@ -892,6 +918,9 @@ def renew(config, unused_plugins): "specifying a CSR file. Please try the certonly " "command instead.") renewer_config = configuration.RenewerConfiguration(config) + renew_successes = [] + renew_failures = [] + parse_failures = [] for renewal_file in _renewal_conf_files(renewer_config): print("Processing " + renewal_file) # XXX: does this succeed in making a fully independent config object @@ -907,28 +936,42 @@ def renew(config, unused_plugins): 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) continue try: - if renewal_candidate is not None: + if renewal_candidate is None: + parse_failures.append(renewal_file) + else: # _reconstitute succeeded in producing a RenewableCert, so we # have something to work with from this particular config file. # XXX: ensure that each call here replaces the previous one zope.component.provideUtility(lineage_config) - print("Trying...") - # Because obtain_cert itself indirectly decides whether to renew - # or not, we couldn't currently make a UI/logging distinction at - # this stage to indicate whether renewal was actually attempted - # (or successful). - obtain_cert(lineage_config, - plugins_disco.PluginsRegistry.find_all(), - renewal_candidate) + # Although obtain_cert itself also indirectly decides + # whether to renew or not, we need to check at this + # stage in order to avoid claiming that renewal + # succeeded when it wasn't even attempted (since + # obtain_cert wouldn't raise an error in that case). + if _should_renew(lineage_config, renewal_candidate): + err = obtain_cert(lineage_config, + plugins_disco.PluginsRegistry.find_all(), + renewal_candidate) + if err is None: + renew_successes.append(renewal_candidate.fullchain) + else: + renew_failures.append(renewal_candidate.fullchain) + else: + print("We skipped this one at the outset!") except Exception as e: # pylint: disable=broad-except # obtain_cert (presumably) encountered an unanticipated problem. logger.warning("Attempting to renew cert from %s produced an " "unexpected error: %s. Skipping.", renewal_file, e) logger.debug("Traceback was:\n%s", traceback.format_exc()) + renew_failures.append(renewal_candidate.fullchain) + + # Describe all the results + _renew_describe_results(renew_successes, renew_failures, parse_failures) def revoke(config, unused_plugins): # TODO: coop with renewal config From 1fd3f8a8dcb9a3a8a3060bf168f7852c4cdde7cd Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 14:21:31 -0800 Subject: [PATCH 04/15] Making tests pass after CLI change --- letsencrypt/tests/cli_test.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index a5757399e..8731b8112 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -222,13 +222,16 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods if "nginx" in real_plugins: # Sending nginx a non-existent conf dir will simulate misconfiguration # (we can only do that if letsencrypt-nginx is actually present) - ret, _, _, _ = self._call(args) - self.assertTrue("The nginx plugin is not working" in ret) - self.assertTrue("MisconfigurationError" in ret) + self._call(args) + # XXX: This probably now raises an exception (when nginx is + # present, but I don't know which one!) + # self.assertTrue("The nginx plugin is not working" in ret) + # self.assertTrue("MisconfigurationError" in ret) args = ["certonly", "--webroot"] - ret, _, _, _ = self._call(args) - self.assertTrue("--webroot-path must be set" in ret) + # ret, _, _, _ = self._call(args) + self.assertRaises(errors.PluginSelectionError, self._call, args) + # self.assertTrue("--webroot-path must be set" in ret) self._cli_missing_flag(["--standalone"], "With the standalone plugin, you probably") @@ -324,10 +327,14 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods def test_certonly_bad_args(self): ret, _, _, _ = self._call(['-d', 'foo.bar', 'certonly', '--csr', CSR]) - self.assertEqual(ret, '--domains and --csr are mutually exclusive') + # self.assertEqual(ret, '--domains and --csr are mutually exclusive') + # self.assertRaises(errors.Error, self._call, + # ['-d', 'foo.bar', 'certonly', '--csr', CSR]) - ret, _, _, _ = self._call(['-a', 'bad_auth', 'certonly']) - self.assertEqual(ret, 'The requested bad_auth plugin does not appear to be installed') + # ret, _, _, _ = self._call(['-a', 'bad_auth', 'certonly']) + self.assertRaises(errors.PluginSelectionError, self._call, + ['-a', 'bad_auth', 'certonly']) + # self.assertEqual(ret, 'The requested bad_auth plugin does not appear to be installed') def test_check_config_sanity_domain(self): # Punycode From d8ea828de6b84cf3393037d6ea07ede2a01abc53 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 16:11:20 -0800 Subject: [PATCH 05/15] Fix lint complaints about cli.py --- letsencrypt/cli.py | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 8566765f8..60b5fdbbc 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -338,6 +338,7 @@ def _handle_identical_cert_request(config, cert): else: assert False, "This is impossible" + def _handle_subset_cert_request(config, domains, cert): """Figure out what to do if a previous cert had a subset of the names now requested @@ -488,7 +489,7 @@ def _avoid_invalidating_lineage(config, lineage, original_server): open(lineage.cert).read()) # all our test certs are from happy hacker fake CA, though maybe one day # we should test more methodically - now_valid = not "fake" in repr(latest_cert.get_issuer()).lower() + now_valid = "fake" not in repr(latest_cert.get_issuer()).lower() if _is_staging(config.server): if not _is_staging(original_server) or now_valid: @@ -547,6 +548,7 @@ def set_configurator(previously, now): raise errors.PluginSelectionError(msg.format(repr(previously), repr(now))) return now + def cli_plugin_requests(config): """ Figure out which plugins the user requested with CLI and config options @@ -575,6 +577,7 @@ def cli_plugin_requests(config): noninstaller_plugins = ["webroot", "manual", "standalone"] + def choose_configurator_plugins(config, plugins, verb): """ Figure out which configurator we're going to use, modifies @@ -597,7 +600,7 @@ def choose_configurator_plugins(config, plugins, verb): '{1} {2} certonly --{0}{1}{1}' '(Alternatively, add a --installer flag. See https://eff.org/letsencrypt-plugins' '{1} and "--help plugins" for more information.)'.format( - req_auth, os.linesep, cli_command)) + req_auth, os.linesep, cli_command)) raise errors.MissingCommandlineFlag(msg) else: @@ -769,6 +772,7 @@ def _restore_required_config_elements(config, renewalparams): raise errors.Error( "Expected a numeric value for {0}".format(config_item)) + def _restore_plugin_configs(config, renewalparams): """Sets plugin specific values in config from renewalparams @@ -801,7 +805,7 @@ def _restore_plugin_configs(config, renewalparams): if config_value == "None": setattr(config.namespace, config_item, None) continue - for action in _parser.parser._actions: # pylint: disable=protected-access + for action in _parser.parser._actions: # pylint: disable=protected-access if action.type is not None and action.dest == config_item: setattr(config.namespace, config_item, action.type(config_value)) @@ -931,7 +935,7 @@ def renew(config, unused_plugins): # elements from within the renewal configuration file). try: renewal_candidate = _reconstitute(lineage_config, renewal_file) - except Exception as e: # pylint: disable=broad-except + except Exception as e: # pylint: disable=broad-except # reconstitute encountered an unanticipated problem. logger.warning("Renewal configuration file %s produced an " "unexpected error: %s. Skipping.", renewal_file, e) @@ -963,7 +967,7 @@ def renew(config, unused_plugins): renew_failures.append(renewal_candidate.fullchain) else: print("We skipped this one at the outset!") - except Exception as e: # pylint: disable=broad-except + except Exception as e: # pylint: disable=broad-except # obtain_cert (presumably) encountered an unanticipated problem. logger.warning("Attempting to renew cert from %s produced an " "unexpected error: %s. Skipping.", renewal_file, e) @@ -1447,7 +1451,7 @@ def prepare_and_parse_args(plugins, args): # parser (--help should display plugin-specific options last) _plugins_parsing(helpful, plugins) - global _parser # pylint: disable=global-statement + global _parser # pylint: disable=global-statement _parser = helpful return helpful.parse_args() @@ -1584,14 +1588,17 @@ def _plugins_parsing(helpful, plugins): "www.example.com -w /var/www/thing -d thing.net -d m.thing.net`") # --webroot-map still has some awkward properties, so it is undocumented helpful.add("webroot", "--webroot-map", default={}, action=WebrootMapProcessor, - help="JSON dictionary mapping domains to webroot paths; this implies -d " - "for each entry. You may need to escape this from your shell. " - """Eg: --webroot-map '{"eg1.is,m.eg1.is":"/www/eg1/", "eg2.is":"/www/eg2"}' """ - "This option is merged with, but takes precedence over, -w / -d entries." - " At present, if you put webroot-map in a config file, it needs to be " - ' on a single line, like: webroot-map = {"example.com":"/var/www"}.') + help="JSON dictionary mapping domains to webroot paths; this " + "implies -d for each entry. You may need to escape this " + "from your shell. " + """E.g.: --webroot-map '{"eg1.is,m.eg1.is":"/www/eg1/", "eg2.is":"/www/eg2"}' """ + "This option is merged with, but takes precedence over, " + "-w / -d entries. At present, if you put webroot-map in " + "a config file, it needs to be on a single line, like: " + 'webroot-map = {"example.com":"/var/www"}.') -class WebrootPathProcessor(argparse.Action): # pylint: disable=missing-docstring + +class WebrootPathProcessor(argparse.Action): # pylint: disable=missing-docstring def __init__(self, *args, **kwargs): self.domain_before_webroot = False argparse.Action.__init__(self, *args, **kwargs) @@ -1640,14 +1647,14 @@ def _process_domain(args_or_config, domain_arg, webroot_path=None): args_or_config.webroot_map.setdefault(domain, webroot_path[-1]) -class WebrootMapProcessor(argparse.Action): # pylint: disable=missing-docstring +class WebrootMapProcessor(argparse.Action): # pylint: disable=missing-docstring def __call__(self, parser, args, webroot_map_arg, option_string=None): webroot_map = json.loads(webroot_map_arg) for domains, webroot_path in webroot_map.iteritems(): _process_domain(args, domains, [webroot_path]) -class DomainFlagProcessor(argparse.Action): # pylint: disable=missing-docstring +class DomainFlagProcessor(argparse.Action): # pylint: disable=missing-docstring def __call__(self, parser, args, domain_arg, option_string=None): """Just wrap _process_domain in argparseese.""" _process_domain(args, domain_arg) @@ -1738,8 +1745,8 @@ def _handle_exception(exc_type, exc_value, trace, config): # acme.messages.Error: urn:acme:error:malformed :: The request message was # malformed :: Error creating new registration :: Validation of contact # mailto:none@longrandomstring.biz failed: Server failure at resolver - if ("urn:acme" in err and ":: " in err - and config.verbose_count <= flag_default("verbose_count")): + if (("urn:acme" in err and ":: " in err and + config.verbose_count <= flag_default("verbose_count"))): # prune ACME error code, we have a human description _code, _sep, err = err.partition(":: ") msg = "An unexpected error occurred:\n" + err + "Please see the " From 4d7ad032ee6d4cd3224d5195c0f7c7bbccbe7618 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 16:34:46 -0800 Subject: [PATCH 06/15] Mention skipped lineages too --- letsencrypt/cli.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index bae124d81..886616dbf 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -719,8 +719,8 @@ def obtain_cert(config, plugins, lineage=None): # In principle we could have a configuration option to inhibit this # from happening. installer.restart() - print("new certificate deployed with reload of plugin", - config.installer, "fullchain is", lineage.fullchain) + print("new certificate deployed with reload of", + config.installer, "server; fullchain is", lineage.fullchain) _suggest_donation_if_appropriate(config) @@ -883,8 +883,12 @@ def _renewal_conf_files(config): return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf")) -def _renew_describe_results(renew_successes, renew_failures, parse_failures): +def _renew_describe_results(renew_successes, renew_failures, renew_skipped, + parse_failures): print() + if renew_skipped: + print("The following certs are not due for renewal yet:") + print("\t" + "\n\t".join(x + " (skipped)" for x in renew_skipped)) if not renew_successes and not renew_failures: print("No renewals were attempted.") elif renew_successes and not renew_failures: @@ -924,11 +928,10 @@ def renew(config, unused_plugins): renewer_config = configuration.RenewerConfiguration(config) renew_successes = [] renew_failures = [] + renew_skipped = [] parse_failures = [] for renewal_file in _renewal_conf_files(renewer_config): print("Processing " + renewal_file) - # XXX: does this succeed in making a fully independent config object - # each time? lineage_config = copy.deepcopy(config) # Note that this modifies config (to add back the configuration @@ -966,7 +969,7 @@ def renew(config, unused_plugins): else: renew_failures.append(renewal_candidate.fullchain) else: - print("We skipped this one at the outset!") + renew_skipped.append(renewal_candidate.fullchain) except Exception as e: # pylint: disable=broad-except # obtain_cert (presumably) encountered an unanticipated problem. logger.warning("Attempting to renew cert from %s produced an " @@ -975,7 +978,8 @@ def renew(config, unused_plugins): renew_failures.append(renewal_candidate.fullchain) # Describe all the results - _renew_describe_results(renew_successes, renew_failures, parse_failures) + _renew_describe_results(renew_successes, renew_failures, renew_skipped, + parse_failures) def revoke(config, unused_plugins): # TODO: coop with renewal config From ad4b8ec147e6560fd3368fbf2274ca6f364b6778 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 16:41:42 -0800 Subject: [PATCH 07/15] lambda to simplify printing lists of success/failure --- letsencrypt/cli.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 886616dbf..27a935ca8 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -885,30 +885,31 @@ def _renewal_conf_files(config): def _renew_describe_results(renew_successes, renew_failures, renew_skipped, parse_failures): + status = lambda x, msg: " " + "\n ".join(i + " (" + msg +")" for i in x) print() if renew_skipped: print("The following certs are not due for renewal yet:") - print("\t" + "\n\t".join(x + " (skipped)" for x in renew_skipped)) + print(status(renew_skipped, "skipped")) if not renew_successes and not renew_failures: print("No renewals were attempted.") elif renew_successes and not renew_failures: print("Congratulations, all renewals succeeded. The following certs " "have been renewed:") - print("\t" + "\n\t".join(x + " (success)" for x in renew_successes)) + print(status(renew_successes, "success")) elif renew_failures and not renew_successes: print("All renewal attempts failed. The following certs could not be " "renewed:") - print("\t" + "\n\t".join(x + " (failure)" for x in renew_failures)) + print(status(renew_failures, "failure")) elif renew_failures and renew_successes: print("The following certs were successfully renewed:") - print("\t" + "\n\t".join(x + " (success)" for x in renew_successes)) + print(status(renew_successes, "success")) print("\nThe following certs could not be renewed:") - print("\t" + "\n\t".join(x + " (failure)" for x in renew_failures)) + print(status(renew_failures, "failure")) if parse_failures: print("\nAdditionally, the following renewal configuration files " "were invalid: ") - print("\t" + "\n\t".join(x + " (parsefail)" for x in parse_failures)) + print(status(parse_failures, "parsefail")) def renew(config, unused_plugins): From de455ac6e06244d747da0c8b219390377a5eae57 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 17:23:06 -0800 Subject: [PATCH 08/15] Don't check _should_renew twice --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 63c672227..19358a8bd 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -440,7 +440,7 @@ def _auth_from_domains(le_client, config, domains, lineage=None): else: # Renewal, where we already know the specific lineage we're # interested in - action = "renew" if _should_renew(config, lineage) else "reinstall" + action = "renew" if action == "reinstall": # The lineage already exists; allow the caller to try installing From 374e4ebb4dc11941f6f271f30872df6bb1bb658e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 17:48:12 -0800 Subject: [PATCH 09/15] Trying to satisfy pylint --- letsencrypt/cli.py | 4 ++-- letsencrypt/tests/cli_test.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 19358a8bd..7dd0a7771 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1589,8 +1589,8 @@ def _plugins_parsing(helpful, plugins): helpful.add("webroot", "--webroot-map", default={}, action=WebrootMapProcessor, help="JSON dictionary mapping domains to webroot paths; this " "implies -d for each entry. You may need to escape this " - "from your shell. " - """E.g.: --webroot-map '{"eg1.is,m.eg1.is":"/www/eg1/", "eg2.is":"/www/eg2"}' """ + "from your shell. E.g.: --webroot-map " + """'{"eg1.is,m.eg1.is":"/www/eg1/", "eg2.is":"/www/eg2"}' """ "This option is merged with, but takes precedence over, " "-w / -d entries. At present, if you put webroot-map in " "a config file, it needs to be on a single line, like: " diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index c263fd8ec..76b7676c3 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -326,7 +326,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods self.assertEqual(config.fullchain_path, os.path.abspath(fullchain)) def test_certonly_bad_args(self): - ret, _, _, _ = self._call(['-d', 'foo.bar', 'certonly', '--csr', CSR]) + _, _, _, _ = self._call(['-d', 'foo.bar', 'certonly', '--csr', CSR]) # self.assertEqual(ret, '--domains and --csr are mutually exclusive') # self.assertRaises(errors.Error, self._call, # ['-d', 'foo.bar', 'certonly', '--csr', CSR]) From a8ba6f7c2c25d279a98c08534da1fc20c051fec7 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 18:54:36 -0800 Subject: [PATCH 10/15] Dry run messages --- letsencrypt/cli.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 96b30d25e..5ebf1ff25 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -419,7 +419,8 @@ def _suggest_donation_if_appropriate(config): def _report_successful_dry_run(): reporter_util = zope.component.getUtility(interfaces.IReporter) - reporter_util.add_message("The dry run was successful.", + reporter_util.add_message("A test certificate requested in dry run was " + "successfully issued.", reporter_util.HIGH_PRIORITY, on_crash=False) @@ -979,8 +980,14 @@ def renew(config, unused_plugins): renew_failures.append(renewal_candidate.fullchain) # Describe all the results + if config.dry_run: + print("** DRY RUN (messages below refer to test certs only!") + print("** The certificates mentioned have not been saved.") _renew_describe_results(renew_successes, renew_failures, renew_skipped, parse_failures) + if config.dry_run: + print("** DRY RUN (messages above refer to test certs only!") + print("** The certificates mentioned have not been saved.") def revoke(config, unused_plugins): # TODO: coop with renewal config From 9bc5523a3b44302c660cc2ddeeedf7138ebbc214 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 8 Feb 2016 19:06:16 -0800 Subject: [PATCH 11/15] Reorganize to make pylint happier --- letsencrypt/cli.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 5ebf1ff25..5e6e1fade 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -884,9 +884,12 @@ def _renewal_conf_files(config): return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf")) -def _renew_describe_results(renew_successes, renew_failures, renew_skipped, - parse_failures): +def _renew_describe_results(config, renew_successes, renew_failures, + renew_skipped, parse_failures): status = lambda x, msg: " " + "\n ".join(i + " (" + msg +")" for i in x) + if config.dry_run: + print("** DRY RUN (messages below refer to test certs only!") + print("** The certificates mentioned have not been saved.") print() if renew_skipped: print("The following certs are not due for renewal yet:") @@ -912,6 +915,10 @@ def _renew_describe_results(renew_successes, renew_failures, renew_skipped, "were invalid: ") print(status(parse_failures, "parsefail")) + if config.dry_run: + print("** DRY RUN (messages above refer to test certs only!") + print("** The certificates mentioned have not been saved.") + def renew(config, unused_plugins): """Renew previously-obtained certificates.""" @@ -980,14 +987,8 @@ def renew(config, unused_plugins): renew_failures.append(renewal_candidate.fullchain) # Describe all the results - if config.dry_run: - print("** DRY RUN (messages below refer to test certs only!") - print("** The certificates mentioned have not been saved.") - _renew_describe_results(renew_successes, renew_failures, renew_skipped, - parse_failures) - if config.dry_run: - print("** DRY RUN (messages above refer to test certs only!") - print("** The certificates mentioned have not been saved.") + _renew_describe_results(config, renew_successes, renew_failures, + renew_skipped, parse_failures) def revoke(config, unused_plugins): # TODO: coop with renewal config From ff9d7a7b802f1e272ad379bdfec9010b9f701246 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Feb 2016 20:21:08 -0800 Subject: [PATCH 12/15] Restore old versions of some tests, port others --- letsencrypt/tests/cli_test.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index ff11b1dde..de64fce04 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -222,16 +222,16 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods if "nginx" in real_plugins: # Sending nginx a non-existent conf dir will simulate misconfiguration # (we can only do that if letsencrypt-nginx is actually present) - self._call(args) - # XXX: This probably now raises an exception (when nginx is - # present, but I don't know which one!) - # self.assertTrue("The nginx plugin is not working" in ret) - # self.assertTrue("MisconfigurationError" in ret) + ret, _, _, _ = self._call(args) + self.assertTrue("The nginx plugin is not working" in ret) + self.assertTrue("MisconfigurationError" in ret) args = ["certonly", "--webroot"] - # ret, _, _, _ = self._call(args) - self.assertRaises(errors.PluginSelectionError, self._call, args) - # self.assertTrue("--webroot-path must be set" in ret) + try: + self._call(args) + assert False, "Exception should have been raised" + except errors.PluginSelectionError as e: + self.assertTrue("--webroot-path must be set" in e.message) self._cli_missing_flag(["--standalone"], "With the standalone plugin, you probably") From b2de2cd1816d1d1646742822eb55925d220fa3e9 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Feb 2016 20:21:40 -0800 Subject: [PATCH 13/15] Better dry run reporting --- letsencrypt/cli.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 5e6e1fade..414c3b0ce 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -417,11 +417,12 @@ def _suggest_donation_if_appropriate(config): reporter_util.add_message(msg, reporter_util.LOW_PRIORITY) -def _report_successful_dry_run(): + +def _report_successful_dry_run(config): reporter_util = zope.component.getUtility(interfaces.IReporter) - reporter_util.add_message("A test certificate requested in dry run was " - "successfully issued.", - reporter_util.HIGH_PRIORITY, on_crash=False) + if config.verb != "renew": + reporter_util.add_message("The dry run was successful.", + reporter_util.HIGH_PRIORITY, on_crash=False) def _auth_from_domains(le_client, config, domains, lineage=None): @@ -709,7 +710,7 @@ def obtain_cert(config, plugins, lineage=None): _auth_from_domains(le_client, config, domains, lineage) if config.dry_run: - _report_successful_dry_run() + _report_successful_dry_run(config) elif config.verb == "renew": if installer is None: # Tell the user that the server was not restarted. From 28c54476533fad62c7bc5d90d123fbb74d90a2be Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Feb 2016 20:30:48 -0800 Subject: [PATCH 14/15] Port another test --- letsencrypt/cli.py | 3 +-- letsencrypt/tests/cli_test.py | 9 +++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 414c3b0ce..064de1b3d 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -686,8 +686,7 @@ def obtain_cert(config, plugins, lineage=None): # installers are used in auth mode to determine domain names installer, authenticator = choose_configurator_plugins(config, plugins, "certonly") except errors.PluginSelectionError as e: - logger.info( - "Could not choose appropriate plugin: %s", e) + logger.info("Could not choose appropriate plugin: %s", e) raise # TODO: Handle errors from _init_le_client? diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index de64fce04..07029ca66 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -329,10 +329,11 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods ret, _, _, _ = self._call(['-d', 'foo.bar', 'certonly', '--csr', CSR]) self.assertEqual(ret, '--domains and --csr are mutually exclusive') - # ret, _, _, _ = self._call(['-a', 'bad_auth', 'certonly']) - self.assertRaises(errors.PluginSelectionError, self._call, - ['-a', 'bad_auth', 'certonly']) - # self.assertEqual(ret, 'The requested bad_auth plugin does not appear to be installed') + try: + self._call(['-a', 'bad_auth', 'certonly']) + assert False, "Exception should have been raised" + except errors.PluginSelectionError as e: + self.assertTrue('The requested bad_auth plugin does not appear' in e.message) def test_check_config_sanity_domain(self): # Punycode From 57ee4f0b4684364f6f9cf62fc8a1589fe5bcd579 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Feb 2016 20:43:13 -0800 Subject: [PATCH 15/15] Nicen dry run renewal messages --- letsencrypt/cli.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 064de1b3d..758c3e7f2 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -888,8 +888,8 @@ def _renew_describe_results(config, renew_successes, renew_failures, renew_skipped, parse_failures): status = lambda x, msg: " " + "\n ".join(i + " (" + msg +")" for i in x) if config.dry_run: - print("** DRY RUN (messages below refer to test certs only!") - print("** The certificates mentioned have not been saved.") + print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry") + print("** (The test certificates below have not been saved.)") print() if renew_skipped: print("The following certs are not due for renewal yet:") @@ -916,8 +916,8 @@ def _renew_describe_results(config, renew_successes, renew_failures, print(status(parse_failures, "parsefail")) if config.dry_run: - print("** DRY RUN (messages above refer to test certs only!") - print("** The certificates mentioned have not been saved.") + print("** DRY RUN: simulating 'letsencrypt renew' close to cert expiry") + print("** (The test certificates above have not been saved.)") def renew(config, unused_plugins):