From b7717bbc8e5e524f2972d616ca8df2fca75a2e7d Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 13:06:50 -0800 Subject: [PATCH 01/10] Fixes for comments from PR review --- letsencrypt/cli.py | 47 ++++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 1ab23ea76..e51490379 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -773,27 +773,25 @@ def _restore_plugin_configs(config, renewalparams): # XXX: is it true that an item will end up in _parser._actions even # when no action was explicitly specified? plugin_prefixes = [renewalparams["authenticator"]] - if "installer" in renewalparams and renewalparams["installer"] != None: + if renewalparams.get("installer", None) is not None: plugin_prefixes.append(renewalparams["installer"]) for plugin_prefix in set(renewalparams): - for config_item in renewalparams.keys(): - if renewalparams[config_item] == "None": + for config_item, config_value in renewalparams.iteritems(): + if config_item.startswith(plugin_prefix + "_"): # Avoid confusion when, for example, "csr = None" (avoid # trying to read the file called "None") # Should we omit the item entirely rather than setting # its value to None? - setattr(config.namespace, config_item, None) - continue - if config_item.startswith(plugin_prefix + "_"): + if config_value == "None": + setattr(config.namespace, config_item, None) + continue 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(renewalparams[config_item])) + action.type(config_value)) break else: - setattr(config.namespace, config_item, - str(renewalparams[config_item])) - return True + setattr(config.namespace, config_item, str(config_value)) def _reconstitute(config, full_path): @@ -881,8 +879,6 @@ def renew(config, unused_plugins): "command instead.") renewer_config = configuration.RenewerConfiguration(config) for renewal_file in _renewal_conf_files(renewer_config): - if not renewal_file.endswith(".conf"): - continue print("Processing " + renewal_file) # XXX: does this succeed in making a fully independent config object # each time? @@ -899,20 +895,21 @@ def renew(config, unused_plugins): logger.debug("Traceback was:\n%s", traceback.format_exc()) continue - if renewal_candidate is None: - # reconstitute indicated an error or problem which has - # already been logged. Go on to the next config. - continue - # XXX: ensure that each call here replaces the previous one - zope.component.provideUtility(lineage_config) + if renewal_candidate is not None: + # _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) - 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) def revoke(config, unused_plugins): # TODO: coop with renewal config """Revoke a previously obtained certificate.""" From 5c14c09027aba71ef02d7b93f7f9974498eadb3e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 13:10:00 -0800 Subject: [PATCH 02/10] @bmw noticed we were iterating over the wrong thing! --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index e51490379..88d16be73 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -775,7 +775,7 @@ def _restore_plugin_configs(config, renewalparams): plugin_prefixes = [renewalparams["authenticator"]] if renewalparams.get("installer", None) is not None: plugin_prefixes.append(renewalparams["installer"]) - for plugin_prefix in set(renewalparams): + for plugin_prefix in set(plugin_prefixes): for config_item, config_value in renewalparams.iteritems(): if config_item.startswith(plugin_prefix + "_"): # Avoid confusion when, for example, "csr = None" (avoid From 5c31b000b40566e2a5a552624ab82db96b02278f Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 14:51:14 -0800 Subject: [PATCH 03/10] Error handling around obtain_cert() --- letsencrypt/cli.py | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 88d16be73..abe4ccc0c 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -770,8 +770,10 @@ def _restore_plugin_configs(config, renewalparams): # works as long as plugins don't need to read plugin-specific # variables set by someone else (e.g., assuming Apache # configurator doesn't need to read webroot_ variables). - # XXX: is it true that an item will end up in _parser._actions even - # when no action was explicitly specified? + # Note: if a parameter that used to be defined in the parser is no + # longer defined, stored copies of that parameter will be + # deserialized as strings by this logic even if they were + # originally meant to be some other type. plugin_prefixes = [renewalparams["authenticator"]] if renewalparams.get("installer", None) is not None: plugin_prefixes.append(renewalparams["installer"]) @@ -895,20 +897,26 @@ def renew(config, unused_plugins): logger.debug("Traceback was:\n%s", traceback.format_exc()) continue - if renewal_candidate is not None: - # _reconstitute succeeded in producing a RenewableCert, so we - # have something to work with from this particular config file. + try: + if renewal_candidate is not None: + # _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) + # 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) + 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()) def revoke(config, unused_plugins): # TODO: coop with renewal config From 21fe41c53b6cb8847680715497fef54614076ff2 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 15:37:50 -0800 Subject: [PATCH 04/10] call .restart() on installer after renew if possible --- letsencrypt/cli.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index abe4ccc0c..bc9777ad6 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -704,6 +704,12 @@ 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") _suggest_donation_if_appropriate(config) From 772d424fc791937647912b4988a68810804ed4c1 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 15:38:10 -0800 Subject: [PATCH 05/10] =?UTF-8?q?log=5Fdir=20=E2=86=92=20logs=5Fdir?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index bc9777ad6..d6c4ce930 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -54,7 +54,7 @@ _parser = None # file's renewalparams and actually used in the client configuration # during the renewal process. We have to record their types here because # the renewal configuration process loses this information. -STR_CONFIG_ITEMS = ["config_dir", "log_dir", "work_dir", "user_agent", +STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent", "server", "account", "authenticator", "installer", "standalone_supported_challenges"] INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"] From 6ef0f71e0eb07e063ffdd7ee96dc2d991c6be1a5 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 5 Feb 2016 15:53:54 -0800 Subject: [PATCH 06/10] -n implies -t for logging --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index a4fa409c1..6e35f1a74 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1389,7 +1389,7 @@ def setup_log_file_handler(config, logfile, fmt): def _cli_log_handler(config, level, fmt): - if config.text_mode: + if config.text_mode or config.noninteractive_mode: handler = colored_logging.StreamHandler() handler.setFormatter(logging.Formatter(fmt)) else: From 7eb2bb4d037bd64ce4e31e3de303a968cc45db11 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 5 Feb 2016 16:30:19 -0800 Subject: [PATCH 07/10] Fix renew + noninteractive --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 63c6d96f8..838da4015 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1621,7 +1621,7 @@ def setup_log_file_handler(config, logfile, fmt): def _cli_log_handler(config, level, fmt): - if config.text_mode or config.noninteractive_mode: + if config.text_mode or config.noninteractive_mode or config.verb == "renew": handler = colored_logging.StreamHandler() handler.setFormatter(logging.Formatter(fmt)) else: From 09337517d3cb24ad8606ec3bc37f289e5f7a65e7 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 16:57:41 -0800 Subject: [PATCH 08/10] Try to distinguish renew and non-renew in integration test --- tests/boulder-integration.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 8c5a93e39..294522e05 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -44,12 +44,13 @@ common --domains le3.wtf install \ --cert-path "${root}/csr/cert.pem" \ --key-path "${root}/csr/key.pem" -# This won't renew (because it's not time yet) -common renew +# This won't renew (because it's not time yet) - not using common because +# common forces renewal +letsencrypt_test --authenticator standalone --installer null renew # This will renew sed -i "4arenew_before_expiry = 10 years" "$root/conf/renewal/le1.wtf.conf" -common renew +letsencrypt_test --authenticator standalone --installer null renew ls "$root/conf/archive/le1.wtf" # dir="$root/conf/archive/le1.wtf" From 8b02f485b02e2170a140a5fc07bacf5e1916e658 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 17:13:30 -0800 Subject: [PATCH 09/10] Have a way not to force renewal in integration test --- tests/boulder-integration.sh | 16 +++++++++++----- tests/integration/_common.sh | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 294522e05..b6c76ee22 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -27,6 +27,13 @@ common() { "$@" } +common_no_force_renew() { + letsencrypt_test_no_force_renew \ + --authenticator standalone \ + --installer null \ + "$@" +} + common --domains le1.wtf --standalone-supported-challenges tls-sni-01 auth common --domains le2.wtf --standalone-supported-challenges http-01 run common -a manual -d le.wtf auth @@ -44,13 +51,12 @@ common --domains le3.wtf install \ --cert-path "${root}/csr/cert.pem" \ --key-path "${root}/csr/key.pem" -# This won't renew (because it's not time yet) - not using common because -# common forces renewal -letsencrypt_test --authenticator standalone --installer null renew +# This won't renew (because it's not time yet) +letsencrypt_test_no_force_renew --authenticator standalone --installer null renew -# This will renew +# This will renew because the expiry is less than 10 years from now sed -i "4arenew_before_expiry = 10 years" "$root/conf/renewal/le1.wtf.conf" -letsencrypt_test --authenticator standalone --installer null renew +letsencrypt_test_no_force_renew --authenticator standalone --installer null renew ls "$root/conf/archive/le1.wtf" # dir="$root/conf/archive/le1.wtf" diff --git a/tests/integration/_common.sh b/tests/integration/_common.sh index 4572b0fb3..f133600a0 100755 --- a/tests/integration/_common.sh +++ b/tests/integration/_common.sh @@ -28,3 +28,21 @@ letsencrypt_test () { -vvvvvvv \ "$@" } + +letsencrypt_test_no_force_renew () { + letsencrypt \ + --server "${SERVER:-http://localhost:4000/directory}" \ + --no-verify-ssl \ + --tls-sni-01-port 5001 \ + --http-01-port 5002 \ + --manual-test-mode \ + $store_flags \ + --text \ + --no-redirect \ + --agree-tos \ + --register-unsafely-without-email \ + --renew-by-default \ + --debug \ + -vvvvvvv \ + "$@" +} From fd3d2fa8224b85179f7a278e78fdf1bd90b951df Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 5 Feb 2016 17:19:39 -0800 Subject: [PATCH 10/10] Make _no_force_renew not force renewal --- tests/integration/_common.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/_common.sh b/tests/integration/_common.sh index f133600a0..9230cc682 100755 --- a/tests/integration/_common.sh +++ b/tests/integration/_common.sh @@ -41,7 +41,6 @@ letsencrypt_test_no_force_renew () { --no-redirect \ --agree-tos \ --register-unsafely-without-email \ - --renew-by-default \ --debug \ -vvvvvvv \ "$@"