From 31fef196c0ed57a5cc6b1c4d409ff8097afbc716 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 01:15:35 +0000 Subject: [PATCH 01/48] --help is effectively a verb for CLI purposes... --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 7cb4a045..711ac004 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -679,7 +679,7 @@ def create_parser(plugins, args): # For now unfortunately this constant just needs to match the code below; # there isn't an elegant way to autogenerate it in time. VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", - "plugins"] + "plugins", "--help"] def _create_subparsers(helpful): From 001d37f9650d0c5f7521673f9fd075a7bd662b0c Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 02:41:55 +0000 Subject: [PATCH 02/48] "-h" is also a ver. --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index a5968ec9..2c996cd3 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -679,7 +679,7 @@ def create_parser(plugins, args): # For now unfortunately this constant just needs to match the code below; # there isn't an elegant way to autogenerate it in time. VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", - "plugins", "--help"] + "plugins", "--help", "-h"] def _create_subparsers(helpful): From f3c2a096b54950e5368907b698c488a458180961 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 02:48:44 +0000 Subject: [PATCH 03/48] Move the verb/subcommand to the end of the argparse line --- letsencrypt/cli.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 2c996cd3..8afbf023 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -495,13 +495,14 @@ def __init__(self, args, plugins): def preprocess_args(self, args): """Work around some limitations in argparse. - Currently, add the default verb "run" as a default. + Currently: add the default verb "run" as a default, and ensure that the + subcommand / verb comes last. """ - - for token in args: + for i,token in enumerate(args): if token in VERBS: - return args - return ["run"] + args + reordered = args[:i] + args[i+1:] + [args[i]] + return reordered + return args + ["run"] def prescan_for_flag(self, flag, possible_arguments): """Checks cli input for flags. From ddc04c755bf6a3e9da956ecf8782ee32b6464cc0 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 07:56:38 +0000 Subject: [PATCH 04/48] work in progress --- letsencrypt/cli.py | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 6d3aa9d2..53609009 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -730,53 +730,55 @@ def add_subparser(name, func): # pylint: disable=missing-docstring # the order of add_subparser() calls is important: it defines the # order in which subparser names will be displayed in --help add_subparser("run", run) + parser_auth = add_subparser("auth", auth) + helpful.add_group("auth", "Options for modifying how a cert is obtained") parser_install = add_subparser("install", install) + helpful.add_group("install", "Options for modifying how a cert is deployed") parser_revoke = add_subparser("revoke", revoke) + helpful.add_group("revoke", "Options for revocation of certs") parser_rollback = add_subparser("rollback", rollback) + helpful.add_group("rollback", "Options for reverting config changes") add_subparser("config_changes", config_changes) parser_plugins = add_subparser("plugins", plugins_cmd) + helpful.add_group("plugins", "Plugin options") - parser_auth.add_argument( - "--csr", type=read_file, help="Path to a Certificate Signing " - "Request (CSR) in DER format.") - parser_auth.add_argument( + helpful.add("auth", + "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.") + helpful.add("auth", "--cert-path", default=flag_default("auth_cert_path"), help="When using --csr this is where certificate is saved.") - parser_auth.add_argument( + helpful.add("auth", "--chain-path", default=flag_default("auth_chain_path"), help="When using --csr this is where certificate chain is saved.") - parser_install.add_argument( - "--cert-path", required=True, help="Path to a certificate that " - "is going to be installed.") - parser_install.add_argument( + helpful.add("install", + "--cert-path", required=True, help="Path to a certificate that is going to be installed.") + helpful.add("install", "--key-path", required=True, help="Accompanying private key") - parser_install.add_argument( + helpful.add("install", "--chain-path", help="Accompanying path to a certificate chain.") - parser_revoke.add_argument( - "--cert-path", type=read_file, help="Revoke a specific certificate.", - required=True) - parser_revoke.add_argument( + helpful.add("revoke", + "--cert-path", type=read_file, help="Revoke a specific certificate.", required=True) + helpful.add("revoke", "--key-path", type=read_file, - help="Revoke certificate using its accompanying key. Useful if " - "Account Key is lost.") + help="Revoke certificate using its accompanying key. Useful if Account Key is lost.") - parser_rollback.add_argument( + helpful.add("rollback", "--checkpoints", type=int, metavar="N", default=flag_default("rollback_checkpoints"), help="Revert configuration N number of checkpoints.") - parser_plugins.add_argument( + helpful.add("plugins", "--init", action="store_true", help="Initialize plugins.") - parser_plugins.add_argument( + helpful.add("plugins", "--prepare", action="store_true", help="Initialize and prepare plugins.") - parser_plugins.add_argument( + helpful.add("plugins", "--authenticators", action="append_const", dest="ifaces", const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.") - parser_plugins.add_argument( + helpful.add("plugins", "--installers", action="append_const", dest="ifaces", const=interfaces.IInstaller, help="Limit to installer plugins only.") From cbfdae88fcde764b0a60190d12881c9945fe2437 Mon Sep 17 00:00:00 2001 From: Brandon Kreisel Date: Sun, 27 Sep 2015 14:44:00 -0400 Subject: [PATCH 05/48] Add Mac compatibility to boulder-start The version of sort that ships with OS X does not support the -V version flag. Emulate that functionality with some sed-fu --- tests/boulder-start.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/boulder-start.sh b/tests/boulder-start.sh index 7ce7dcba..e8c50633 100755 --- a/tests/boulder-start.sh +++ b/tests/boulder-start.sh @@ -4,11 +4,16 @@ # ugh, go version output is like: # go version go1.4.2 linux/amd64 -GOVER=`go version | cut -d" " -f3 | cut -do -f2` +GOVER=`go version | cut -d" " -f3 | cut -do -f2` # version comparison function verlte { + if [ `uname` == 'Darwin' ]; then + [ "$1" = "`echo -e \"$1\n$2\" | sed 's/\b\([0-9]\)\b/0\1/g' \ + | sort | sed 's/\b0\([0-9]\)/\1/g' | head -n1`" ] + else [ "$1" = "`echo -e "$1\n$2" | sort -V | head -n1`" ] + fi } if ! verlte 1.5 "$GOVER" ; then From 5d8e9a3d68b362634c9fb752e5a0bcb4fb12d021 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Sun, 27 Sep 2015 21:07:40 +0000 Subject: [PATCH 06/48] Fix various doc generation issues --- acme/acme/challenges.py | 2 +- docs/api/display.rst | 6 ------ docs/api/recovery_token.rst | 5 ----- docs/api/revoker.rst | 5 ----- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- 5 files changed, 2 insertions(+), 18 deletions(-) delete mode 100644 docs/api/recovery_token.rst delete mode 100644 docs/api/revoker.rst diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index 13186cc4..81711e60 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -542,7 +542,7 @@ def gen_validation(self, account_key, alg=jose.RS256, **kwargs): def check_validation(self, validation, account_public_key): """Check validation. - :param validation + :param JWS validation: :type account_public_key: `~cryptography.hazmat.primitives.asymmetric.rsa.RSAPublicKey` or diff --git a/docs/api/display.rst b/docs/api/display.rst index b79ef25d..117a9170 100644 --- a/docs/api/display.rst +++ b/docs/api/display.rst @@ -21,9 +21,3 @@ .. automodule:: letsencrypt.display.enhancements :members: - -:mod:`letsencrypt.display.revocation` -===================================== - -.. automodule:: letsencrypt.display.revocation - :members: diff --git a/docs/api/recovery_token.rst b/docs/api/recovery_token.rst deleted file mode 100644 index 774aa4b3..00000000 --- a/docs/api/recovery_token.rst +++ /dev/null @@ -1,5 +0,0 @@ -:mod:`letsencrypt.recovery_token` --------------------------------------------------- - -.. automodule:: letsencrypt.recovery_token - :members: diff --git a/docs/api/revoker.rst b/docs/api/revoker.rst deleted file mode 100644 index a482a138..00000000 --- a/docs/api/revoker.rst +++ /dev/null @@ -1,5 +0,0 @@ -:mod:`letsencrypt.revoker` --------------------------- - -.. automodule:: letsencrypt.revoker - :members: diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index f301de8b..ad3c62d2 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1162,7 +1162,7 @@ def _get_mod_deps(mod_name): changes. .. warning:: If all deps are not included, it may cause incorrect parsing behavior, due to enable_mod's shortcut for updating the parser's - currently defined modules (:method:`.ApacheConfigurator._add_parser_mod`) + currently defined modules (`.ApacheConfigurator._add_parser_mod`) This would only present a major problem in extremely atypical configs that use ifmod for the missing deps. From 96a737bbbaf9aa76accdbd9421b19e38a0703e72 Mon Sep 17 00:00:00 2001 From: David Xia Date: Sun, 27 Sep 2015 16:51:20 -0400 Subject: [PATCH 07/48] Fix CLI --help for OS X OS X's signal module doesn't have SIGPWR. Don't try to use it. Fixes #841 --- letsencrypt/error_handler.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index fedb66c0..99f502ac 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -2,6 +2,7 @@ import logging import os import signal +import sys import traceback @@ -13,9 +14,14 @@ # potentially occur from inside Python. Signals such as SIGILL were not # included as they could be a sign of something devious and we should terminate # immediately. -_SIGNALS = ([signal.SIGTERM] if os.name == "nt" else - [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, - signal.SIGXCPU, signal.SIGXFSZ, signal.SIGPWR]) +if os.name == "nt": + _SIGNALS = [signal.SIGTERM] +elif sys.platform == "darwin": + _SIGNALS = [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, signal.SIGXCPU, + signal.SIGXFSZ] +else: + _SIGNALS = [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, signal.SIGXCPU, + signal.SIGXFSZ, signal.SIGPWR] class ErrorHandler(object): From a7375eb5494df494d2604ee1e903467b093af30b Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Sun, 27 Sep 2015 17:44:31 -0700 Subject: [PATCH 08/48] Emit error when simple_verify fails. When running the manual authenticator, if simple_verify fails, there is no output to indicate what went wrong, just "Incomplete authorizations." --- letsencrypt/plugins/manual.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/letsencrypt/plugins/manual.py b/letsencrypt/plugins/manual.py index 2014c8c0..2fad4ac5 100644 --- a/letsencrypt/plugins/manual.py +++ b/letsencrypt/plugins/manual.py @@ -182,6 +182,8 @@ def _perform_single(self, achall): achall.account_key.public_key(), self.config.simple_http_port): return response else: + logger.error( + "Self-verify of challenge failed, authorization abandoned.\n") if self.conf("test-mode") and self._httpd.poll() is not None: # simply verify cause command failure... return False From 913a0a9e98b2559ab960b58dd533a932cdde8150 Mon Sep 17 00:00:00 2001 From: Jadaw1n Date: Mon, 28 Sep 2015 17:34:43 +0200 Subject: [PATCH 09/48] Dockerfile: option --text doesn't exist --- Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 789e26af..b9ea168d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -62,5 +62,5 @@ RUN virtualenv --no-site-packages -p python2 /opt/letsencrypt/venv && \ # bash" and investigate, apply patches, etc. ENV PATH /opt/letsencrypt/venv/bin:$PATH -# TODO: is --text really necessary? -ENTRYPOINT [ "letsencrypt", "--text" ] + +ENTRYPOINT [ "letsencrypt" ] From 27268afdcc82a34e0d37d39bd6a14af5431ddb8c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 28 Sep 2015 11:58:12 -0700 Subject: [PATCH 10/48] Remove extra newline. --- letsencrypt/plugins/manual.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/plugins/manual.py b/letsencrypt/plugins/manual.py index 2fad4ac5..3f727672 100644 --- a/letsencrypt/plugins/manual.py +++ b/letsencrypt/plugins/manual.py @@ -183,7 +183,7 @@ def _perform_single(self, achall): return response else: logger.error( - "Self-verify of challenge failed, authorization abandoned.\n") + "Self-verify of challenge failed, authorization abandoned.") if self.conf("test-mode") and self._httpd.poll() is not None: # simply verify cause command failure... return False From 315b3577811fba3d3a540c22cc2f6bf772fb98af Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Sun, 27 Sep 2015 21:27:36 +0000 Subject: [PATCH 11/48] Hide null installer (fixes #789). --- letsencrypt/cli.py | 2 +- letsencrypt/display/ops.py | 2 +- letsencrypt/plugins/disco.py | 9 +++++++++ letsencrypt/plugins/null.py | 1 + letsencrypt/tests/display/ops_test.py | 12 +++++++----- setup.py | 1 - 6 files changed, 19 insertions(+), 8 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 3317ae54..8bcbd8f0 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -420,7 +420,7 @@ def plugins_cmd(args, config, plugins): # TODO: Use IDisplay rather than print logger.debug("Expected interfaces: %s", args.ifaces) ifaces = [] if args.ifaces is None else args.ifaces - filtered = plugins.ifaces(ifaces) + filtered = plugins.visible().ifaces(ifaces) logger.debug("Filtered plugins: %r", filtered) if not args.init and not args.prepare: diff --git a/letsencrypt/display/ops.py b/letsencrypt/display/ops.py index 4ab3ec57..43705e30 100644 --- a/letsencrypt/display/ops.py +++ b/letsencrypt/display/ops.py @@ -65,7 +65,7 @@ def pick_plugin(config, default, plugins, question, ifaces): # throw more UX-friendly error if default not in plugins filtered = plugins.filter(lambda p_ep: p_ep.name == default) else: - filtered = plugins.ifaces(ifaces) + filtered = plugins.visible().ifaces(ifaces) filtered.init(config) verified = filtered.verify(ifaces) diff --git a/letsencrypt/plugins/disco.py b/letsencrypt/plugins/disco.py index b6cdb1f9..5a41fda8 100644 --- a/letsencrypt/plugins/disco.py +++ b/letsencrypt/plugins/disco.py @@ -50,6 +50,11 @@ def description_with_name(self): """Description with name. Handy for UI.""" return "{0} ({1})".format(self.description, self.name) + @property + def hidden(self): + """Should this plugin be hidden from UI?""" + return getattr(self.plugin_cls, "hidden", False) + def ifaces(self, *ifaces_groups): """Does plugin implements specified interface groups?""" return not ifaces_groups or any( @@ -183,6 +188,10 @@ def filter(self, pred): return type(self)(dict((name, plugin_ep) for name, plugin_ep in self._plugins.iteritems() if pred(plugin_ep))) + def visible(self): + """Filter plugins based on visibility.""" + return self.filter(lambda plugin_ep: not plugin_ep.hidden) + def ifaces(self, *ifaces_groups): """Filter plugins based on interfaces.""" # pylint: disable=star-args diff --git a/letsencrypt/plugins/null.py b/letsencrypt/plugins/null.py index efe041ca..4ba6c9d6 100644 --- a/letsencrypt/plugins/null.py +++ b/letsencrypt/plugins/null.py @@ -17,6 +17,7 @@ class Installer(common.Plugin): zope.interface.classProvides(interfaces.IPluginFactory) description = "Null Installer" + hidden = True # pylint: disable=missing-docstring,no-self-use diff --git a/letsencrypt/tests/display/ops_test.py b/letsencrypt/tests/display/ops_test.py index 7420a62f..9d4a3a93 100644 --- a/letsencrypt/tests/display/ops_test.py +++ b/letsencrypt/tests/display/ops_test.py @@ -84,7 +84,7 @@ def test_default_provided(self): def test_no_default(self): self._call() - self.assertEqual(1, self.reg.ifaces.call_count) + self.assertEqual(1, self.reg.visible().ifaces.call_count) def test_no_candidate(self): self.assertTrue(self._call() is None) @@ -94,7 +94,8 @@ def test_single(self): plugin_ep.init.return_value = "foo" plugin_ep.misconfigured = False - self.reg.ifaces().verify().available.return_value = {"bar": plugin_ep} + self.reg.visible().ifaces().verify().available.return_value = { + "bar": plugin_ep} self.assertEqual("foo", self._call()) def test_single_misconfigured(self): @@ -102,13 +103,14 @@ def test_single_misconfigured(self): plugin_ep.init.return_value = "foo" plugin_ep.misconfigured = True - self.reg.ifaces().verify().available.return_value = {"bar": plugin_ep} + self.reg.visible().ifaces().verify().available.return_value = { + "bar": plugin_ep} self.assertTrue(self._call() is None) def test_multiple(self): plugin_ep = mock.MagicMock() plugin_ep.init.return_value = "foo" - self.reg.ifaces().verify().available.return_value = { + self.reg.visible().ifaces().verify().available.return_value = { "bar": plugin_ep, "baz": plugin_ep, } @@ -119,7 +121,7 @@ def test_multiple(self): [plugin_ep, plugin_ep], self.question) def test_choose_plugin_none(self): - self.reg.ifaces().verify().available.return_value = { + self.reg.visible().ifaces().verify().available.return_value = { "bar": None, "baz": None, } diff --git a/setup.py b/setup.py index c568d287..8f75aff0 100644 --- a/setup.py +++ b/setup.py @@ -118,7 +118,6 @@ def read_file(filename, encoding='utf8'): ], 'letsencrypt.plugins': [ 'manual = letsencrypt.plugins.manual:Authenticator', - # TODO: null should probably not be presented to the user 'null = letsencrypt.plugins.null:Installer', 'standalone = letsencrypt.plugins.standalone.authenticator' ':StandaloneAuthenticator', From c1012f5f0082dd99d22fb5a49695dfbdfd433f19 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Sep 2015 12:25:37 -0700 Subject: [PATCH 12/48] Removed SIGPWR entirely --- letsencrypt/error_handler.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index 99f502ac..1f979a6d 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -2,7 +2,6 @@ import logging import os import signal -import sys import traceback @@ -14,14 +13,9 @@ # potentially occur from inside Python. Signals such as SIGILL were not # included as they could be a sign of something devious and we should terminate # immediately. -if os.name == "nt": - _SIGNALS = [signal.SIGTERM] -elif sys.platform == "darwin": - _SIGNALS = [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, signal.SIGXCPU, - signal.SIGXFSZ] -else: - _SIGNALS = [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, signal.SIGXCPU, - signal.SIGXFSZ, signal.SIGPWR] +_SIGNALS = ([signal.SIGTERM] if os.name == "nt" else + [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, + signal.SIGXCPU, signal.SIGXFSZ]) class ErrorHandler(object): From ab98d5c39fc19cc90785a87f10cc4b53390e8b20 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Mon, 28 Sep 2015 17:14:33 -0400 Subject: [PATCH 13/48] Ignore unknown challenge types --- acme/acme/messages.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 02ae24c8..002c0876 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -373,7 +373,17 @@ class Authorization(ResourceBody): @challenges.decoder def challenges(value): # pylint: disable=missing-docstring,no-self-argument - return tuple(ChallengeBody.from_json(chall) for chall in value) + # The from_json method raises errors.UnrecognizedTypeError when a + # challenge of unknown type is encountered. We want to ignore this + # case. This forces us to do an explicit iteration, since list + # comprehensions can't handle exceptions. + challenges = [] + for chall in value: + try: + challenges.append(ChallengeBody.from_json(chall)) + except errors.UnknownTypeError: + continue + return tuple(challenges) @property def resolved_combinations(self): From b6bbc9e0a29a7b64ecc03b1ffbbccf67cac37238 Mon Sep 17 00:00:00 2001 From: Brandon Kreisel Date: Mon, 28 Sep 2015 17:39:01 -0400 Subject: [PATCH 14/48] Add inline Mac comment --- tests/boulder-start.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/boulder-start.sh b/tests/boulder-start.sh index e8c50633..530f9c59 100755 --- a/tests/boulder-start.sh +++ b/tests/boulder-start.sh @@ -8,6 +8,7 @@ GOVER=`go version | cut -d" " -f3 | cut -do -f2` # version comparison function verlte { + #OS X doesn't support version sorting; emulate with sed if [ `uname` == 'Darwin' ]; then [ "$1" = "`echo -e \"$1\n$2\" | sed 's/\b\([0-9]\)\b/0\1/g' \ | sort | sed 's/\b0\([0-9]\)/\1/g' | head -n1`" ] From 3279aefefbd409aae2f1bb954cd67d266240e973 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Sep 2015 15:15:44 -0700 Subject: [PATCH 15/48] Made PEP8 happy --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 8bcbd8f0..dccfb928 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -518,7 +518,7 @@ def __init__(self, args, plugins): help2 = self.prescan_for_flag("--help", self.help_topics) assert max(True, "a") == "a", "Gravity changed direction" help_arg = max(help1, help2) - if help_arg == True: + if help_arg is True: # just --help with no topic; avoid argparse altogether print USAGE sys.exit(0) From fa992faf52be93309506ae728eb64340fd388706 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 28 Sep 2015 15:24:51 -0700 Subject: [PATCH 16/48] Fix pylint and add test --- acme/acme/messages.py | 11 ++++++----- acme/acme/messages_test.py | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 002c0876..594b3d5c 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -2,6 +2,7 @@ import collections from acme import challenges +from acme import errors from acme import fields from acme import jose from acme import util @@ -373,17 +374,17 @@ class Authorization(ResourceBody): @challenges.decoder def challenges(value): # pylint: disable=missing-docstring,no-self-argument - # The from_json method raises errors.UnrecognizedTypeError when a + # The from_json method raises errors.UnrecognizedTypeError when a # challenge of unknown type is encountered. We want to ignore this # case. This forces us to do an explicit iteration, since list # comprehensions can't handle exceptions. - challenges = [] + challs = [] for chall in value: try: - challenges.append(ChallengeBody.from_json(chall)) - except errors.UnknownTypeError: + challs.append(ChallengeBody.from_json(chall)) + except jose.UnrecognizedTypeError: continue - return tuple(challenges) + return tuple(challs) @property def resolved_combinations(self): diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index 25f07018..ac722909 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -274,6 +274,9 @@ class AuthorizationTest(unittest.TestCase): def setUp(self): from acme.messages import ChallengeBody from acme.messages import STATUS_VALID + + unknown_chall = mock.MagicMock() + unknown_chall.to_json.side_effect = side_effect=jose.UnrecognizedTypeError self.challbs = ( ChallengeBody( uri='http://challb1', status=STATUS_VALID, @@ -300,6 +303,19 @@ def setUp(self): 'combinations': combinations, } + # For unknown challenge types + self.jmsg_unknown_chall = { + 'resource': 'challenge', + 'uri': 'random_uri', + 'type': 'unknown', + 'tls': True, + } + + self.jobj_from_unknown = { + 'identifier': identifier.to_json(), + 'challenges': [self.jmsg_unknown_chall], + } + def test_from_json(self): from acme.messages import Authorization Authorization.from_json(self.jobj_from) @@ -314,6 +330,11 @@ def test_resolved_combinations(self): (self.challbs[1], self.challbs[2]), )) + def test_unknown_chall_type(self): + """Just make sure an error isn't thrown.""" + from acme.messages import Authorization + Authorization.from_json(self.jobj_from_unknown) + class AuthorizationResourceTest(unittest.TestCase): """Tests for acme.messages.AuthorizationResource.""" From 4da0e17255a15d0e9589795410b25c05a6b87cc2 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Sep 2015 15:45:31 -0700 Subject: [PATCH 17/48] Added message and changed reporter interface --- letsencrypt/account.py | 4 ++-- letsencrypt/auth_handler.py | 2 +- letsencrypt/cli.py | 11 +++++++++++ letsencrypt/client.py | 2 +- letsencrypt/interfaces.py | 2 +- letsencrypt/reporter.py | 2 +- letsencrypt/tests/reporter_test.py | 6 +++--- 7 files changed, 20 insertions(+), 9 deletions(-) diff --git a/letsencrypt/account.py b/letsencrypt/account.py index 8bee2210..c97e4f6f 100644 --- a/letsencrypt/account.py +++ b/letsencrypt/account.py @@ -92,13 +92,13 @@ def report_new_account(acc, config): "contain certificates and private keys obtained by Let's Encrypt " "so making regular backups of this folder is ideal.".format( config.config_dir), - reporter.MEDIUM_PRIORITY, True) + reporter.MEDIUM_PRIORITY) if acc.regr.body.emails: recovery_msg = ("If you lose your account credentials, you can " "recover through e-mails sent to {0}.".format( ", ".join(acc.regr.body.emails))) - reporter.add_message(recovery_msg, reporter.HIGH_PRIORITY, True) + reporter.add_message(recovery_msg, reporter.HIGH_PRIORITY) class AccountMemoryStorage(interfaces.AccountStorage): diff --git a/letsencrypt/auth_handler.py b/letsencrypt/auth_handler.py index 68aed510..b27a569f 100644 --- a/letsencrypt/auth_handler.py +++ b/letsencrypt/auth_handler.py @@ -531,7 +531,7 @@ def _report_failed_challs(failed_achalls): reporter = zope.component.getUtility(interfaces.IReporter) for achalls in problems.itervalues(): reporter.add_message( - _generate_failed_chall_msg(achalls), reporter.MEDIUM_PRIORITY, True) + _generate_failed_chall_msg(achalls), reporter.MEDIUM_PRIORITY) def _generate_failed_chall_msg(failed_achalls): diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index dccfb928..bd49d110 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -267,6 +267,14 @@ def _treat_as_renewal(config, domains): return None +def _report_new_cert(cert_path): + """Reports the creation of a new certificate to the user.""" + reporter_util = zope.component.getUtility(interfaces.IReporter) + reporter_util.add_message("Congratulations! Your certificate has been " + "saved at {0}.".format(cert_path), + reporter.MEDIUM_PRIORITY) + + def _auth_from_domains(le_client, config, domains, plugins): """Authenticate and enroll certificate.""" # Note: This can raise errors... caught above us though. @@ -292,6 +300,8 @@ def _auth_from_domains(le_client, config, domains, plugins): if not lineage: raise errors.Error("Certificate could not be obtained") + _report_new_cert(lineage.cert) + return lineage @@ -365,6 +375,7 @@ def auth(args, config, plugins): file=args.csr[0], data=args.csr[1], form="der")) le_client.save_certificate( certr, chain, args.cert_path, args.chain_path) + _report_new_cert(args.cert_path) else: domains = _find_domains(args, installer) _auth_from_domains(le_client, config, domains, plugins) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index e9decae4..c82131af 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -286,7 +286,7 @@ def _report_renewal_status(self, cert): "configured in the directories under {0}.").format( cert.cli_config.renewal_configs_dir) reporter = zope.component.getUtility(interfaces.IReporter) - reporter.add_message(msg, reporter.LOW_PRIORITY, True) + reporter.add_message(msg, reporter.LOW_PRIORITY) def save_certificate(self, certr, chain_cert, cert_path, chain_path): # pylint: disable=no-self-use diff --git a/letsencrypt/interfaces.py b/letsencrypt/interfaces.py index 1ba8afe4..1f51645a 100644 --- a/letsencrypt/interfaces.py +++ b/letsencrypt/interfaces.py @@ -478,7 +478,7 @@ class IReporter(zope.interface.Interface): LOW_PRIORITY = zope.interface.Attribute( "Used to denote low priority messages") - def add_message(self, msg, priority, on_crash=False): + def add_message(self, msg, priority, on_crash=True): """Adds msg to the list of messages to be printed. :param str msg: Message to be displayed to the user. diff --git a/letsencrypt/reporter.py b/letsencrypt/reporter.py index 48230583..0905dfa5 100644 --- a/letsencrypt/reporter.py +++ b/letsencrypt/reporter.py @@ -36,7 +36,7 @@ class Reporter(object): def __init__(self): self.messages = Queue.PriorityQueue() - def add_message(self, msg, priority, on_crash=False): + def add_message(self, msg, priority, on_crash=True): """Adds msg to the list of messages to be printed. :param str msg: Message to be displayed to the user. diff --git a/letsencrypt/tests/reporter_test.py b/letsencrypt/tests/reporter_test.py index c4351120..89bd9dfc 100644 --- a/letsencrypt/tests/reporter_test.py +++ b/letsencrypt/tests/reporter_test.py @@ -78,13 +78,13 @@ def _unsuccessful_exit_common(self): output = sys.stdout.getvalue() self.assertTrue("IMPORTANT NOTES:" in output) self.assertTrue("High" in output) - self.assertTrue("Med" not in output) + self.assertTrue("Med" in output) self.assertTrue("Low" not in output) def _add_messages(self): - self.reporter.add_message("High", self.reporter.HIGH_PRIORITY, True) + self.reporter.add_message("High", self.reporter.HIGH_PRIORITY) self.reporter.add_message("Med", self.reporter.MEDIUM_PRIORITY) - self.reporter.add_message("Low", self.reporter.LOW_PRIORITY) + self.reporter.add_message("Low", self.reporter.LOW_PRIORITY, False) if __name__ == "__main__": From 243c9e9021cd1183742a516aed0a432a9cc65b73 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Sep 2015 15:52:09 -0700 Subject: [PATCH 18/48] Made cover and lint happy --- letsencrypt/cli.py | 2 +- letsencrypt/tests/reporter_test.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index bd49d110..0b7d1790 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -272,7 +272,7 @@ def _report_new_cert(cert_path): reporter_util = zope.component.getUtility(interfaces.IReporter) reporter_util.add_message("Congratulations! Your certificate has been " "saved at {0}.".format(cert_path), - reporter.MEDIUM_PRIORITY) + reporter_util.MEDIUM_PRIORITY) def _auth_from_domains(le_client, config, domains, plugins): diff --git a/letsencrypt/tests/reporter_test.py b/letsencrypt/tests/reporter_test.py index 89bd9dfc..ddf345c4 100644 --- a/letsencrypt/tests/reporter_test.py +++ b/letsencrypt/tests/reporter_test.py @@ -78,12 +78,12 @@ def _unsuccessful_exit_common(self): output = sys.stdout.getvalue() self.assertTrue("IMPORTANT NOTES:" in output) self.assertTrue("High" in output) - self.assertTrue("Med" in output) + self.assertTrue("Med" not in output) self.assertTrue("Low" not in output) def _add_messages(self): self.reporter.add_message("High", self.reporter.HIGH_PRIORITY) - self.reporter.add_message("Med", self.reporter.MEDIUM_PRIORITY) + self.reporter.add_message("Med", self.reporter.MEDIUM_PRIORITY, False) self.reporter.add_message("Low", self.reporter.LOW_PRIORITY, False) From 67ec4d09eef289b979f18b869c760cc997ef2f44 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 28 Sep 2015 15:53:42 -0700 Subject: [PATCH 19/48] Put in dummy challenge --- acme/acme/challenges.py | 5 +++++ acme/acme/messages.py | 5 +++-- acme/acme/messages_test.py | 2 -- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index 81711e60..1ffc6cc9 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -34,6 +34,11 @@ class DVChallenge(Challenge): # pylint: disable=abstract-method """Domain validation challenges.""" +class UnrecognizedChallenge(DVChallenge): + """Unrecognized challenge.""" + typ = "unknown" + + class ChallengeResponse(jose.TypedJSONObjectWithFields): # _fields_to_partial_json | pylint: disable=abstract-method """ACME challenge response.""" diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 594b3d5c..d6e9952c 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -2,7 +2,6 @@ import collections from acme import challenges -from acme import errors from acme import fields from acme import jose from acme import util @@ -383,7 +382,9 @@ def challenges(value): # pylint: disable=missing-docstring,no-self-argument try: challs.append(ChallengeBody.from_json(chall)) except jose.UnrecognizedTypeError: - continue + challs.append(ChallengeBody( + uri="UNKNOWN", chall=challenges.UnrecognizedChallenge, + status=STATUS_UNKNOWN)) return tuple(challs) @property diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index ac722909..d7bbdb0e 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -275,8 +275,6 @@ def setUp(self): from acme.messages import ChallengeBody from acme.messages import STATUS_VALID - unknown_chall = mock.MagicMock() - unknown_chall.to_json.side_effect = side_effect=jose.UnrecognizedTypeError self.challbs = ( ChallengeBody( uri='http://challb1', status=STATUS_VALID, From 5238f530924de2bf335b958a102b31306cf4a79d Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 28 Sep 2015 16:03:03 -0700 Subject: [PATCH 20/48] DVChallenge -> Challenge --- acme/acme/challenges.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index 1ffc6cc9..fbb2e741 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -34,7 +34,7 @@ class DVChallenge(Challenge): # pylint: disable=abstract-method """Domain validation challenges.""" -class UnrecognizedChallenge(DVChallenge): +class UnrecognizedChallenge(Challenge): """Unrecognized challenge.""" typ = "unknown" From ed7977fb039d74455d088a4bb11cbf2eaf91373b Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Sep 2015 18:45:12 -0700 Subject: [PATCH 21/48] Added cli tests --- letsencrypt/tests/cli_test.py | 140 ++++++++++++++++++++++++++-------- 1 file changed, 107 insertions(+), 33 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 2e9f3330..31cef584 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -16,6 +16,9 @@ from letsencrypt.tests import test_util +CSR = test_util.vector_path('csr.der') + + class CLITest(unittest.TestCase): """Tests for different commands.""" @@ -65,40 +68,111 @@ def test_plugins(self): for r in xrange(len(flags)))): self._call(['plugins'] + list(args)) - @mock.patch("letsencrypt.cli.sys") + def test_auth_bad_args(self): + ret, _, _, _ = self._call(['-d', 'foo.bar', 'auth', '--csr', CSR]) + self.assertEqual(ret, '--domains and --csr are mutually exclusive') + + ret, _, _, _ = self._call(['-a', 'bad_auth', 'auth']) + self.assertEqual(ret, 'Authenticator could not be determined') + + @mock.patch('letsencrypt.cli.zope.component.getUtility') + def test_auth_new_request_success(self, mock_get_utility): + cert_path = '/etc/letsencrypt/live/foo.bar' + mock_lineage = mock.MagicMock(cert=cert_path) + mock_client = mock.MagicMock() + mock_client.obtain_and_enroll_certificate.return_value = mock_lineage + self._auth_new_request_common(mock_client) + self.assertEqual( + mock_client.obtain_and_enroll_certificate.call_count, 1) + self.assertTrue( + cert_path in mock_get_utility().add_message.call_args[0][0]) + + def test_auth_new_request_failure(self): + mock_client = mock.MagicMock() + mock_client.obtain_and_enroll_certificate.return_value = False + self.assertRaises(errors.Error, + self._auth_new_request_common, mock_client) + + def _auth_new_request_common(self, mock_client): + with mock.patch('letsencrypt.cli._treat_as_renewal') as mock_renewal: + mock_renewal.return_value = None + with mock.patch('letsencrypt.cli._init_le_client') as mock_init: + mock_init.return_value = mock_client + self._call(['-d', 'foo.bar', '-a', + 'standalone', '-i', 'bad', 'auth']) + + @mock.patch('letsencrypt.cli.zope.component.getUtility') + @mock.patch('letsencrypt.cli._treat_as_renewal') + @mock.patch('letsencrypt.cli._init_le_client') + def test_auth_renewal(self, mock_init, mock_renewal, mock_get_utility): + cert_path = '/etc/letsencrypt/live/foo.bar' + mock_lineage = mock.MagicMock(cert=cert_path) + mock_cert = mock.MagicMock(body='body') + mock_key = mock.MagicMock(pem='pem_key') + mock_renewal.return_value = mock_lineage + mock_client = mock.MagicMock() + mock_client.obtain_certificate.return_value = (mock_cert, 'chain', + mock_key, 'csr') + mock_init.return_value = mock_client + with mock.patch('letsencrypt.cli.OpenSSL'): + with mock.patch('letsencrypt.cli.crypto_util'): + self._call(['-d', 'foo.bar', '-a', 'standalone', 'auth']) + mock_client.obtain_certificate.assert_called_once_with(['foo.bar']) + self.assertEqual(mock_lineage.save_successor.call_count, 1) + mock_lineage.update_all_links_to.assert_called_once_with( + mock_lineage.latest_common_version()) + self.assertTrue( + cert_path in mock_get_utility().add_message.call_args[0][0]) + + @mock.patch('letsencrypt.cli.zope.component.getUtility') + @mock.patch('letsencrypt.cli._init_le_client') + def test_auth_csr(self, mock_init, mock_get_utility): + cert_path = '/etc/letsencrypt/live/foo.bar' + mock_client = mock.MagicMock() + mock_client.obtain_certificate_from_csr.return_value = ('certr', + 'chain') + mock_init.return_value = mock_client + self._call(['-a', 'standalone', 'auth', '--csr', CSR, + '--cert-path', cert_path, '--chain-path', '/']) + mock_client.save_certificate.assert_called_once_with( + 'certr', 'chain', cert_path, '/') + self.assertTrue( + cert_path in mock_get_utility().add_message.call_args[0][0]) + + @mock.patch('letsencrypt.cli.sys') def test_handle_exception(self, mock_sys): # pylint: disable=protected-access from letsencrypt import cli mock_open = mock.mock_open() - with mock.patch("letsencrypt.cli.open", mock_open, create=True): - exception = Exception("detail") + with mock.patch('letsencrypt.cli.open', mock_open, create=True): + exception = Exception('detail') cli._handle_exception( Exception, exc_value=exception, trace=None, args=None) - mock_open().write.assert_called_once_with("".join( + mock_open().write.assert_called_once_with(''.join( traceback.format_exception_only(Exception, exception))) error_msg = mock_sys.exit.call_args_list[0][0][0] - self.assertTrue("unexpected error" in error_msg) + self.assertTrue('unexpected error' in error_msg) - with mock.patch("letsencrypt.cli.open", mock_open, create=True): + with mock.patch('letsencrypt.cli.open', mock_open, create=True): mock_open.side_effect = [KeyboardInterrupt] - error = errors.Error("detail") + error = errors.Error('detail') cli._handle_exception( errors.Error, exc_value=error, trace=None, args=None) # assert_any_call used because sys.exit doesn't exit in cli.py - mock_sys.exit.assert_any_call("".join( + mock_sys.exit.assert_any_call(''.join( traceback.format_exception_only(errors.Error, error))) args = mock.MagicMock(debug=False) cli._handle_exception( - Exception, exc_value=Exception("detail"), trace=None, args=args) + Exception, exc_value=Exception('detail'), trace=None, args=args) error_msg = mock_sys.exit.call_args_list[-1][0][0] - self.assertTrue("unexpected error" in error_msg) + self.assertTrue('unexpected error' in error_msg) - interrupt = KeyboardInterrupt("detail") + interrupt = KeyboardInterrupt('detail') cli._handle_exception( KeyboardInterrupt, exc_value=interrupt, trace=None, args=None) - mock_sys.exit.assert_called_with("".join( + mock_sys.exit.assert_called_with(''.join( traceback.format_exception_only(KeyboardInterrupt, interrupt))) @@ -108,13 +182,13 @@ class DetermineAccountTest(unittest.TestCase): def setUp(self): self.args = mock.MagicMock(account=None, email=None) self.config = configuration.NamespaceConfig(self.args) - self.accs = [mock.MagicMock(id="x"), mock.MagicMock(id="y")] + self.accs = [mock.MagicMock(id='x'), mock.MagicMock(id='y')] self.account_storage = account.AccountMemoryStorage() def _call(self): # pylint: disable=protected-access from letsencrypt.cli import _determine_account - with mock.patch("letsencrypt.cli.account.AccountFileStorage") as mock_storage: + with mock.patch('letsencrypt.cli.account.AccountFileStorage') as mock_storage: mock_storage.return_value = self.account_storage return _determine_account(self.args, self.config) @@ -131,7 +205,7 @@ def test_single_account(self): self.assertEqual(self.accs[0].id, self.args.account) self.assertTrue(self.args.email is None) - @mock.patch("letsencrypt.client.display_ops.choose_account") + @mock.patch('letsencrypt.client.display_ops.choose_account') def test_multiple_accounts(self, mock_choose_accounts): for acc in self.accs: self.account_storage.save(acc) @@ -142,11 +216,11 @@ def test_multiple_accounts(self, mock_choose_accounts): self.assertEqual(self.accs[1].id, self.args.account) self.assertTrue(self.args.email is None) - @mock.patch("letsencrypt.client.display_ops.get_email") + @mock.patch('letsencrypt.client.display_ops.get_email') def test_no_accounts_no_email(self, mock_get_email): - mock_get_email.return_value = "foo@bar.baz" + mock_get_email.return_value = 'foo@bar.baz' - with mock.patch("letsencrypt.cli.client") as client: + with mock.patch('letsencrypt.cli.client') as client: client.register.return_value = ( self.accs[0], mock.sentinel.acme) self.assertEqual((self.accs[0], mock.sentinel.acme), self._call()) @@ -154,15 +228,15 @@ def test_no_accounts_no_email(self, mock_get_email): self.config, self.account_storage, tos_cb=mock.ANY) self.assertEqual(self.accs[0].id, self.args.account) - self.assertEqual("foo@bar.baz", self.args.email) + self.assertEqual('foo@bar.baz', self.args.email) def test_no_accounts_email(self): - self.args.email = "other email" - with mock.patch("letsencrypt.cli.client") as client: + self.args.email = 'other email' + with mock.patch('letsencrypt.cli.client') as client: client.register.return_value = (self.accs[1], mock.sentinel.acme) self._call() self.assertEqual(self.accs[1].id, self.args.account) - self.assertEqual("other email", self.args.email) + self.assertEqual('other email', self.args.email) class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest): @@ -176,36 +250,36 @@ def setUp(self): def tearDown(self): shutil.rmtree(self.tempdir) - @mock.patch("letsencrypt.le_util.make_or_verify_dir") + @mock.patch('letsencrypt.le_util.make_or_verify_dir') def test_find_duplicative_names(self, unused_makedir): from letsencrypt.cli import _find_duplicative_certs - test_cert = test_util.load_vector("cert-san.pem") - with open(self.test_rc.cert, "w") as f: + test_cert = test_util.load_vector('cert-san.pem') + with open(self.test_rc.cert, 'w') as f: f.write(test_cert) # No overlap at all - result = _find_duplicative_certs(["wow.net", "hooray.org"], + result = _find_duplicative_certs(['wow.net', 'hooray.org'], self.config, self.cli_config) self.assertEqual(result, (None, None)) # Totally identical - result = _find_duplicative_certs(["example.com", "www.example.com"], + result = _find_duplicative_certs(['example.com', 'www.example.com'], self.config, self.cli_config) - self.assertTrue(result[0].configfile.filename.endswith("example.org.conf")) + self.assertTrue(result[0].configfile.filename.endswith('example.org.conf')) self.assertEqual(result[1], None) # Superset - result = _find_duplicative_certs(["example.com", "www.example.com", - "something.new"], self.config, + result = _find_duplicative_certs(['example.com', 'www.example.com', + 'something.new'], self.config, self.cli_config) self.assertEqual(result[0], None) - self.assertTrue(result[1].configfile.filename.endswith("example.org.conf")) + self.assertTrue(result[1].configfile.filename.endswith('example.org.conf')) # Partial overlap doesn't count - result = _find_duplicative_certs(["example.com", "something.new"], + result = _find_duplicative_certs(['example.com', 'something.new'], self.config, self.cli_config) self.assertEqual(result, (None, None)) -if __name__ == "__main__": +if __name__ == '__main__': unittest.main() # pragma: no cover From dc0b26c2781132a1c3f0622c40c93f4e64bf1f53 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 28 Sep 2015 18:47:15 -0700 Subject: [PATCH 22/48] Raised cover percentage --- tox.cover.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.cover.sh b/tox.cover.sh index aa5e3ed8..edfd9b81 100755 --- a/tox.cover.sh +++ b/tox.cover.sh @@ -16,7 +16,7 @@ fi cover () { if [ "$1" = "letsencrypt" ]; then - min=96 + min=97 elif [ "$1" = "acme" ]; then min=100 elif [ "$1" = "letsencrypt_apache" ]; then From ad1fce03f77feddcbf0ef96d1ff63ed40e44576f Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 29 Sep 2015 06:47:15 +0000 Subject: [PATCH 23/48] UnrecognizedChallenge (fixes #855). Overrides quick fix from #856. --- acme/acme/challenges.py | 37 ++++++++++++++++++++++++++++++++----- acme/acme/messages.py | 14 +------------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index fbb2e741..4731c043 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -25,6 +25,14 @@ class Challenge(jose.TypedJSONObjectWithFields): """ACME challenge.""" TYPES = {} + @classmethod + def from_json(cls, jobj): + try: + return super(Challenge, cls).from_json(jobj) + except jose.UnrecognizedTypeError as error: + logger.debug(error) + return UnrecognizedChallenge.from_json(jobj) + class ContinuityChallenge(Challenge): # pylint: disable=abstract-method """Client validation challenges.""" @@ -34,11 +42,6 @@ class DVChallenge(Challenge): # pylint: disable=abstract-method """Domain validation challenges.""" -class UnrecognizedChallenge(Challenge): - """Unrecognized challenge.""" - typ = "unknown" - - class ChallengeResponse(jose.TypedJSONObjectWithFields): # _fields_to_partial_json | pylint: disable=abstract-method """ACME challenge response.""" @@ -47,6 +50,30 @@ class ChallengeResponse(jose.TypedJSONObjectWithFields): resource = fields.Resource(resource_type) +class UnrecognizedChallenge(Challenge): + """Unrecognized challenge. + + ACME specification defines a generic framework for challenges and + defines some standard challenges that are implemented in this + module. However, other implementations (including peers) might + define additional challenge types, which should be ignored if + unrecognized. + + :ivar jobj: Original JSON decoded object. + + """ + + def __init__(self, jobj): + object.__setattr__(self, "jobj", jobj) + + def to_partial_json(self): + return self.jobj + + @classmethod + def from_json(cls, jobj): + return cls(jobj) + + @Challenge.register class SimpleHTTP(DVChallenge): """ACME "simpleHttp" challenge. diff --git a/acme/acme/messages.py b/acme/acme/messages.py index d6e9952c..02ae24c8 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -373,19 +373,7 @@ class Authorization(ResourceBody): @challenges.decoder def challenges(value): # pylint: disable=missing-docstring,no-self-argument - # The from_json method raises errors.UnrecognizedTypeError when a - # challenge of unknown type is encountered. We want to ignore this - # case. This forces us to do an explicit iteration, since list - # comprehensions can't handle exceptions. - challs = [] - for chall in value: - try: - challs.append(ChallengeBody.from_json(chall)) - except jose.UnrecognizedTypeError: - challs.append(ChallengeBody( - uri="UNKNOWN", chall=challenges.UnrecognizedChallenge, - status=STATUS_UNKNOWN)) - return tuple(challs) + return tuple(ChallengeBody.from_json(chall) for chall in value) @property def resolved_combinations(self): From 0ffef20a20522cf060c8c75f84ad6ab9a77470d2 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 29 Sep 2015 07:02:33 +0000 Subject: [PATCH 24/48] UnrecognizedChallenge: fix tests and lint. --- acme/acme/challenges.py | 2 ++ acme/acme/challenges_test.py | 26 ++++++++++++++++++++++++++ acme/acme/messages_test.py | 18 ------------------ 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index 4731c043..d81e77f8 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -64,9 +64,11 @@ class UnrecognizedChallenge(Challenge): """ def __init__(self, jobj): + super(UnrecognizedChallenge, self).__init__() object.__setattr__(self, "jobj", jobj) def to_partial_json(self): + # pylint: disable=no-member return self.jobj @classmethod diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index c82d95e1..ed44d4c4 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -17,6 +17,32 @@ KEY = test_util.load_rsa_private_key('rsa512_key.pem') +class ChallengeTest(unittest.TestCase): + + def test_from_json_unrecognized(self): + from acme.challenges import Challenge + from acme.challenges import UnrecognizedChallenge + chall = UnrecognizedChallenge({"type": "foo"}) + # pylint: disable=no-member + self.assertEqual(chall, Challenge.from_json(chall.jobj)) + + +class UnrecognizedChallengeTest(unittest.TestCase): + + def setUp(self): + from acme.challenges import UnrecognizedChallenge + self.jobj = {"type": "foo"} + self.chall = UnrecognizedChallenge(self.jobj) + + def test_to_partial_json(self): + self.assertEqual(self.jobj, self.chall.to_partial_json()) + + def test_from_json(self): + from acme.challenges import UnrecognizedChallenge + self.assertEqual( + self.chall, UnrecognizedChallenge.from_json(self.jobj)) + + class SimpleHTTPTest(unittest.TestCase): def setUp(self): diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index d7bbdb0e..d2d859bc 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -301,19 +301,6 @@ def setUp(self): 'combinations': combinations, } - # For unknown challenge types - self.jmsg_unknown_chall = { - 'resource': 'challenge', - 'uri': 'random_uri', - 'type': 'unknown', - 'tls': True, - } - - self.jobj_from_unknown = { - 'identifier': identifier.to_json(), - 'challenges': [self.jmsg_unknown_chall], - } - def test_from_json(self): from acme.messages import Authorization Authorization.from_json(self.jobj_from) @@ -328,11 +315,6 @@ def test_resolved_combinations(self): (self.challbs[1], self.challbs[2]), )) - def test_unknown_chall_type(self): - """Just make sure an error isn't thrown.""" - from acme.messages import Authorization - Authorization.from_json(self.jobj_from_unknown) - class AuthorizationResourceTest(unittest.TestCase): """Tests for acme.messages.AuthorizationResource.""" From dcd274ed93182caaf225e33d8efbb50666bb49fa Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 29 Sep 2015 11:06:02 -0700 Subject: [PATCH 25/48] Marked Nginx as Alpha --- letsencrypt-nginx/letsencrypt_nginx/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index 2899e1f7..3f6d6f32 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -56,7 +56,7 @@ class NginxConfigurator(common.Plugin): zope.interface.implements(interfaces.IAuthenticator, interfaces.IInstaller) zope.interface.classProvides(interfaces.IPluginFactory) - description = "Nginx Web Server" + description = "Nginx Web Server - Alpha" @classmethod def add_parser_arguments(cls, add): From 312057b1b817254914256972dc326af3dbdece48 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 29 Sep 2015 12:54:52 -0700 Subject: [PATCH 26/48] changes += kuba_feedback --- letsencrypt/tests/cli_test.py | 13 ++++++++----- letsencrypt/tests/reporter_test.py | 6 ++++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 31cef584..a59bc414 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -98,8 +98,7 @@ def _auth_new_request_common(self, mock_client): mock_renewal.return_value = None with mock.patch('letsencrypt.cli._init_le_client') as mock_init: mock_init.return_value = mock_client - self._call(['-d', 'foo.bar', '-a', - 'standalone', '-i', 'bad', 'auth']) + self._call(['-d', 'foo.bar', '-a', 'standalone', 'auth']) @mock.patch('letsencrypt.cli.zope.component.getUtility') @mock.patch('letsencrypt.cli._treat_as_renewal') @@ -124,16 +123,20 @@ def test_auth_renewal(self, mock_init, mock_renewal, mock_get_utility): self.assertTrue( cert_path in mock_get_utility().add_message.call_args[0][0]) + @mock.patch('letsencrypt.cli.display_ops.pick_installer') @mock.patch('letsencrypt.cli.zope.component.getUtility') @mock.patch('letsencrypt.cli._init_le_client') - def test_auth_csr(self, mock_init, mock_get_utility): + def test_auth_csr(self, mock_init, mock_get_utility, mock_pick_installer): cert_path = '/etc/letsencrypt/live/foo.bar' mock_client = mock.MagicMock() mock_client.obtain_certificate_from_csr.return_value = ('certr', 'chain') mock_init.return_value = mock_client - self._call(['-a', 'standalone', 'auth', '--csr', CSR, - '--cert-path', cert_path, '--chain-path', '/']) + installer = 'installer' + self._call( + ['-a', 'standalone', '-i', installer, 'auth', '--csr', CSR, + '--cert-path', cert_path, '--chain-path', '/']) + self.assertEqual(mock_pick_installer.call_args[0][1], installer) mock_client.save_certificate.assert_called_once_with( 'certr', 'chain', cert_path, '/') self.assertTrue( diff --git a/letsencrypt/tests/reporter_test.py b/letsencrypt/tests/reporter_test.py index ddf345c4..c848b1ca 100644 --- a/letsencrypt/tests/reporter_test.py +++ b/letsencrypt/tests/reporter_test.py @@ -83,8 +83,10 @@ def _unsuccessful_exit_common(self): def _add_messages(self): self.reporter.add_message("High", self.reporter.HIGH_PRIORITY) - self.reporter.add_message("Med", self.reporter.MEDIUM_PRIORITY, False) - self.reporter.add_message("Low", self.reporter.LOW_PRIORITY, False) + self.reporter.add_message( + "Med", self.reporter.MEDIUM_PRIORITY, on_crash=False) + self.reporter.add_message( + "Low", self.reporter.LOW_PRIORITY, on_crash=False) if __name__ == "__main__": From 2e0fd36c2831db4fcdaefdd5c43fac41ee7fbac6 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 21:01:02 +0000 Subject: [PATCH 27/48] Improve flag and help processing * letsencrypt --help $SUBCOMMAND now works. Fixes #787 #819 * subcommand arguments are now actually argument groups, so that all flags can be placed before or after subcommand verbs as the user wishes Fixes: #820 A limitation: * args like --cert-path were previously present for multiple verbs (auth/install/revoke) with separate docs; they are now in the "paths" topic. That's fine, though it would be good to *also* list them when the user types letsencrypt --help install. --- letsencrypt/cli.py | 64 ++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 53609009..ac2c5555 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -489,8 +489,6 @@ def add_argument(self, *args, **kwargs): self.parser.add_argument(*args, **kwargs) -HELP_TOPICS = ["all", "security", "paths", "automation", "testing", "plugins"] - class HelpfulArgumentParser(object): """Argparse Wrapper. @@ -529,12 +527,17 @@ def __init__(self, args, plugins): def preprocess_args(self, args): """Work around some limitations in argparse. - Currently: add the default verb "run" as a default, and ensure that the + Currently: add the default verb "run" as a default, and ensure that the subcommand / verb comes last. """ + + if "-h" in args or "--help" in args: + # all verbs double as help arguments; don't get them confused + return args + for i,token in enumerate(args): if token in VERBS: - reordered = args[:i] + args[i+1:] + [args[i]] + reordered = args[:i] + args[i+1:] + [args[i]] return reordered return args + ["run"] @@ -717,6 +720,9 @@ def create_parser(plugins, args): VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins", "--help", "-h"] +HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + + [v for v in VERBS if "-" not in v]) + def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") @@ -732,53 +738,34 @@ def add_subparser(name, func): # pylint: disable=missing-docstring add_subparser("run", run) parser_auth = add_subparser("auth", auth) - helpful.add_group("auth", "Options for modifying how a cert is obtained") + helpful.add_group("auth", description="Options for modifying how a cert is obtained") parser_install = add_subparser("install", install) - helpful.add_group("install", "Options for modifying how a cert is deployed") + helpful.add_group("install", description="Options for modifying how a cert is deployed") parser_revoke = add_subparser("revoke", revoke) - helpful.add_group("revoke", "Options for revocation of certs") + helpful.add_group("revoke", description="Options for revocation of certs") parser_rollback = add_subparser("rollback", rollback) - helpful.add_group("rollback", "Options for reverting config changes") + helpful.add_group("rollback", description="Options for reverting config changes") add_subparser("config_changes", config_changes) parser_plugins = add_subparser("plugins", plugins_cmd) - helpful.add_group("plugins", "Plugin options") + helpful.add_group("plugins", description="Plugin options") - helpful.add("auth", - "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.") - helpful.add("auth", - "--cert-path", default=flag_default("auth_cert_path"), - help="When using --csr this is where certificate is saved.") helpful.add("auth", - "--chain-path", default=flag_default("auth_chain_path"), - help="When using --csr this is where certificate chain is saved.") - - helpful.add("install", - "--cert-path", required=True, help="Path to a certificate that is going to be installed.") - helpful.add("install", - "--key-path", required=True, help="Accompanying private key") - helpful.add("install", - "--chain-path", help="Accompanying path to a certificate chain.") - helpful.add("revoke", - "--cert-path", type=read_file, help="Revoke a specific certificate.", required=True) - helpful.add("revoke", - "--key-path", type=read_file, - help="Revoke certificate using its accompanying key. Useful if Account Key is lost.") - - helpful.add("rollback", + "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.") + helpful.add("rollback", "--checkpoints", type=int, metavar="N", default=flag_default("rollback_checkpoints"), help="Revert configuration N number of checkpoints.") - helpful.add("plugins", + helpful.add("plugins", "--init", action="store_true", help="Initialize plugins.") - helpful.add("plugins", + helpful.add("plugins", "--prepare", action="store_true", help="Initialize and prepare plugins.") - helpful.add("plugins", + helpful.add("plugins", "--authenticators", action="append_const", dest="ifaces", const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.") - helpful.add("plugins", + helpful.add("plugins", "--installers", action="append_const", dest="ifaces", const=interfaces.IInstaller, help="Limit to installer plugins only.") @@ -787,6 +774,15 @@ def _paths_parser(helpful): add = helpful.add helpful.add_group( "paths", description="Arguments changing execution paths & servers") + helpful.add("paths", + "--cert-path", default=flag_default("auth_cert_path"), + help="Path to where certificate is saved (with auth), " + "installed (with install --csr) or revoked.") + helpful.add("paths", + "--key-path", required=True, + help="Path to private key for cert creation or revocation (if account key is missing)") + helpful.add("paths", + "--chain-path", help="Accompanying path to a certificate chain.") add("paths", "--config-dir", default=flag_default("config_dir"), help=config_help("config_dir")) add("paths", "--work-dir", default=flag_default("work_dir"), From a0af023b1436e27f5c1a7626aeeab374d927cf3b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 14:48:26 -0700 Subject: [PATCH 28/48] --key-path is mandatory for install, optional for revoke --- letsencrypt/cli.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index ac2c5555..8042173e 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -530,7 +530,6 @@ def preprocess_args(self, args): Currently: add the default verb "run" as a default, and ensure that the subcommand / verb comes last. """ - if "-h" in args or "--help" in args: # all verbs double as help arguments; don't get them confused return args @@ -779,7 +778,7 @@ def _paths_parser(helpful): help="Path to where certificate is saved (with auth), " "installed (with install --csr) or revoked.") helpful.add("paths", - "--key-path", required=True, + "--key-path", required=("install" in helpful.args), help="Path to private key for cert creation or revocation (if account key is missing)") helpful.add("paths", "--chain-path", help="Accompanying path to a certificate chain.") From 05d439a33937c4c46d2ee949dc70c9126f463efd Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 14:48:40 -0700 Subject: [PATCH 29/48] Update cli tests We don't expect to error out if called with no args --- letsencrypt/tests/cli_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 2e9f3330..992b254a 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -40,7 +40,9 @@ def _call(self, args): return ret, stdout, stderr, client def test_no_flags(self): - self.assertRaises(SystemExit, self._call, []) + with mock.patch('letsencrypt.cli.run') as mock_run: + self._call([]) + self.assertEqual(1, mock_run.call_count) def test_help(self): self.assertRaises(SystemExit, self._call, ['--help']) From 2297349b95f1451d10caae24297ba3b84dd7d6ce Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 16:56:36 -0700 Subject: [PATCH 30/48] lintian --- letsencrypt/cli.py | 24 ++++++++++++------------ letsencrypt/tests/cli_test.py | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 0d72a3eb..b7efa041 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -500,7 +500,6 @@ def add_argument(self, *args, **kwargs): self.parser.add_argument(*args, **kwargs) - class HelpfulArgumentParser(object): """Argparse Wrapper. @@ -545,7 +544,7 @@ def preprocess_args(self, args): # all verbs double as help arguments; don't get them confused return args - for i,token in enumerate(args): + for i, token in enumerate(args): if token in VERBS: reordered = args[:i] + args[i+1:] + [args[i]] return reordered @@ -745,18 +744,21 @@ def add_subparser(name, func): # pylint: disable=missing-docstring # the order of add_subparser() calls is important: it defines the # order in which subparser names will be displayed in --help - add_subparser("run", run) + # these add_subparser objects return objects to which arguments could be + # attached, but they have annoying arg ordering constrains so we use + # groups instead: https://github.com/letsencrypt/letsencrypt/issues/820 - parser_auth = add_subparser("auth", auth) + add_subparser("run", run) + add_subparser("auth", auth) helpful.add_group("auth", description="Options for modifying how a cert is obtained") - parser_install = add_subparser("install", install) + add_subparser("install", install) helpful.add_group("install", description="Options for modifying how a cert is deployed") - parser_revoke = add_subparser("revoke", revoke) + add_subparser("revoke", revoke) helpful.add_group("revoke", description="Options for revocation of certs") - parser_rollback = add_subparser("rollback", rollback) + add_subparser("rollback", rollback) helpful.add_group("rollback", description="Options for reverting config changes") add_subparser("config_changes", config_changes) - parser_plugins = add_subparser("plugins", plugins_cmd) + add_subparser("plugins", plugins_cmd) helpful.add_group("plugins", description="Plugin options") helpful.add("auth", @@ -769,12 +771,10 @@ def add_subparser(name, func): # pylint: disable=missing-docstring helpful.add("plugins", "--init", action="store_true", help="Initialize plugins.") helpful.add("plugins", - "--prepare", action="store_true", - help="Initialize and prepare plugins.") + "--prepare", action="store_true", help="Initialize and prepare plugins.") helpful.add("plugins", "--authenticators", action="append_const", dest="ifaces", - const=interfaces.IAuthenticator, - help="Limit to authenticator plugins only.") + const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.") helpful.add("plugins", "--installers", action="append_const", dest="ifaces", const=interfaces.IInstaller, help="Limit to installer plugins only.") diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index ce32b8f7..9a99a74c 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -44,8 +44,8 @@ def _call(self, args): def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: - self._call([]) - self.assertEqual(1, mock_run.call_count) + self._call([]) + self.assertEqual(1, mock_run.call_count) def test_help(self): self.assertRaises(SystemExit, self._call, ['--help']) From 6b6bc038827e359173039a5cb229104ef257e127 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 17:12:38 -0700 Subject: [PATCH 31/48] --cert-path was required for install and revoke Oops --- letsencrypt/cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index b7efa041..82bd57ec 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -786,6 +786,7 @@ def _paths_parser(helpful): "paths", description="Arguments changing execution paths & servers") helpful.add("paths", "--cert-path", default=flag_default("auth_cert_path"), + required=("install" in helpful.args or "revoke" in helpful.args), help="Path to where certificate is saved (with auth), " "installed (with install --csr) or revoked.") helpful.add("paths", From 18dacc528df67e703336bd778518a25f4850b345 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 18:08:58 -0700 Subject: [PATCH 32/48] Preserve all argparse parameters Try to restore all variants that applied to the different subcomannds --- letsencrypt/cli.py | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 82bd57ec..ac79ab93 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -521,6 +521,7 @@ def __init__(self, args, plugins): self.parser._add_config_file_help = False # pylint: disable=protected-access self.silent_parser = SilentParser(self.parser) + self.verb = None self.args = self.preprocess_args(args) help1 = self.prescan_for_flag("-h", self.help_topics) help2 = self.prescan_for_flag("--help", self.help_topics) @@ -542,12 +543,16 @@ def preprocess_args(self, args): """ if "-h" in args or "--help" in args: # all verbs double as help arguments; don't get them confused + self.verb = "help" return args for i, token in enumerate(args): if token in VERBS: reordered = args[:i] + args[i+1:] + [args[i]] + self.verb = token return reordered + + self.verb = "run" return args + ["run"] def prescan_for_flag(self, flag, possible_arguments): @@ -782,18 +787,28 @@ def add_subparser(name, func): # pylint: disable=missing-docstring def _paths_parser(helpful): add = helpful.add + verb = helpful.verb helpful.add_group( "paths", description="Arguments changing execution paths & servers") - helpful.add("paths", - "--cert-path", default=flag_default("auth_cert_path"), - required=("install" in helpful.args or "revoke" in helpful.args), - help="Path to where certificate is saved (with auth), " - "installed (with install --csr) or revoked.") - helpful.add("paths", - "--key-path", required=("install" in helpful.args), + + cph = "Path to where cert is saved (with auth), installed (with install --csr) or revoked." + if verb == "auth": + add("paths", "--cert-path", default=flag_default("auth_cert_path"), help=cph) + elif verb == "revoke": + add("paths", "--cert-path", type=read_file, required=True, help=cph) + else: + add("paths", "--cert-path", help=cph, required=(verb == "install")) + + # revoke --key-path reads a file, install --key-path takes a string + add("paths", "--key-path", type=((verb == "revoke" and read_file) or str), + required=(verb == "install"), help="Path to private key for cert creation or revocation (if account key is missing)") - helpful.add("paths", - "--chain-path", help="Accompanying path to a certificate chain.") + + default_cp = None + if verb == "auth": + default_cp = flag_default("auth_chain_path") + add("paths", "--chain-path", default=default_cp, + help="Accompanying path to a certificate chain.") add("paths", "--config-dir", default=flag_default("config_dir"), help=config_help("config_dir")) add("paths", "--work-dir", default=flag_default("work_dir"), From 627fca37b4e45d300fdb3a023943b11c6bbae593 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 18:18:18 -0700 Subject: [PATCH 33/48] We didn't actually need to define --help as a verb --- letsencrypt/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index ac79ab93..efc0f9f7 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -732,10 +732,10 @@ def create_parser(plugins, args): # For now unfortunately this constant just needs to match the code below; # there isn't an elegant way to autogenerate it in time. VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", - "plugins", "--help", "-h"] + "plugins"] HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + - [v for v in VERBS if "-" not in v]) + [v for v in VERBS]) def _create_subparsers(helpful): From 1e3c92c714bb382298343d5c14f14aa896e765ab Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 11:49:46 -0700 Subject: [PATCH 34/48] Cleanup the verb -> subparser mapping --- letsencrypt/cli.py | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index efc0f9f7..fd9d8cbb 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -9,6 +9,7 @@ import pkg_resources import sys import time +import types import traceback import configargparse @@ -731,17 +732,16 @@ def create_parser(plugins, args): # For now unfortunately this constant just needs to match the code below; # there isn't an elegant way to autogenerate it in time. -VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", - "plugins"] - -HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + - [v for v in VERBS]) - +VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins"] +HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + VERBS) def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") - def add_subparser(name, func): # pylint: disable=missing-docstring + def add_subparser(name): # pylint: disable=missing-docstring + # Each subcommand is implemented by a function of the same name + func = eval(name) # pylint: disable=eval-used + assert isinstance(func, types.FunctionType), "squirrels in namespace" subparser = subparsers.add_parser( name, help=func.__doc__.splitlines()[0], description=func.__doc__) subparser.set_defaults(func=func) @@ -752,18 +752,13 @@ def add_subparser(name, func): # pylint: disable=missing-docstring # these add_subparser objects return objects to which arguments could be # attached, but they have annoying arg ordering constrains so we use # groups instead: https://github.com/letsencrypt/letsencrypt/issues/820 + for v in VERBS: + add_subparser(v) - add_subparser("run", run) - add_subparser("auth", auth) helpful.add_group("auth", description="Options for modifying how a cert is obtained") - add_subparser("install", install) helpful.add_group("install", description="Options for modifying how a cert is deployed") - add_subparser("revoke", revoke) helpful.add_group("revoke", description="Options for revocation of certs") - add_subparser("rollback", rollback) helpful.add_group("rollback", description="Options for reverting config changes") - add_subparser("config_changes", config_changes) - add_subparser("plugins", plugins_cmd) helpful.add_group("plugins", description="Plugin options") helpful.add("auth", From 2a3a111d628711e131ea202511a1383c10dfe378 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 12:09:38 -0700 Subject: [PATCH 35/48] Disable pylint invalid-name It's clearly making our code harder to read and write --- .pylintrc | 2 +- acme/acme/challenges.py | 4 ++-- acme/acme/challenges_test.py | 2 +- acme/acme/jose/interfaces_test.py | 4 ++-- acme/acme/jose/json_util_test.py | 6 +++--- acme/acme/jose/jwa_test.py | 2 +- acme/acme/jose/jwk.py | 2 +- acme/acme/jose/util.py | 4 ++-- acme/acme/jose/util_test.py | 4 ++-- acme/acme/messages_test.py | 2 +- letsencrypt/account.py | 2 +- letsencrypt/display/enhancements.py | 2 +- letsencrypt/display/ops.py | 2 +- letsencrypt/log.py | 2 +- letsencrypt/plugins/common.py | 6 +++--- letsencrypt/tests/auth_handler_test.py | 2 +- letsencrypt/tests/log_test.py | 2 +- letsencrypt/tests/reverter_test.py | 8 ++++---- 18 files changed, 29 insertions(+), 29 deletions(-) diff --git a/.pylintrc b/.pylintrc index bf318704..268d61ec 100644 --- a/.pylintrc +++ b/.pylintrc @@ -38,7 +38,7 @@ load-plugins=linter_plugin # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use"--disable=all --enable=classes # --disable=W" -disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use +disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use,invalid-name # abstract-class-not-used cannot be disabled locally (at least in pylint 1.4.1) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index d81e77f8..f5763adc 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -315,7 +315,7 @@ class DVSNIResponse(ChallengeResponse): validation = jose.Field("validation", decoder=jose.JWS.from_json) @property - def z(self): # pylint: disable=invalid-name + def z(self): """The ``z`` parameter. :rtype: bytes @@ -333,7 +333,7 @@ def z_domain(self): :rtype: bytes """ - z = self.z # pylint: disable=invalid-name + z = self.z return z[:32] + b'.' + z[32:] + self.DOMAIN_SUFFIX @property diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index ed44d4c4..b3f48cdf 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -269,7 +269,7 @@ def setUp(self): 'validation': self.validation.to_json(), } - # pylint: disable=invalid-name + label1 = b'e2df3498860637c667fedadc5a8494ec' label2 = b'09dcc75553c9b3bd73662b50e71b1e42' self.z = label1 + label2 diff --git a/acme/acme/jose/interfaces_test.py b/acme/acme/jose/interfaces_test.py index 380c3a2a..a3ee124f 100644 --- a/acme/acme/jose/interfaces_test.py +++ b/acme/acme/jose/interfaces_test.py @@ -8,7 +8,7 @@ class JSONDeSerializableTest(unittest.TestCase): def setUp(self): from acme.jose.interfaces import JSONDeSerializable - # pylint: disable=missing-docstring,invalid-name + # pylint: disable=missing-docstring class Basic(JSONDeSerializable): def __init__(self, v): @@ -53,7 +53,7 @@ def from_json(cls, jobj): self.nested = Basic([[self.basic1]]) self.tuple = Basic(('foo',)) - # pylint: disable=invalid-name + self.Basic = Basic self.Sequence = Sequence self.Mapping = Mapping diff --git a/acme/acme/jose/json_util_test.py b/acme/acme/jose/json_util_test.py index a055f3bf..f751382e 100644 --- a/acme/acme/jose/json_util_test.py +++ b/acme/acme/jose/json_util_test.py @@ -92,7 +92,7 @@ def setUp(self): from acme.jose.json_util import JSONObjectWithFieldsMeta self.field = Field('Baz') self.field2 = Field('Baz2') - # pylint: disable=invalid-name,missing-docstring,too-few-public-methods + # pylint: disable=missing-docstring,too-few-public-methods # pylint: disable=blacklisted-name @six.add_metaclass(JSONObjectWithFieldsMeta) @@ -138,7 +138,7 @@ def setUp(self): from acme.jose.json_util import Field class MockJSONObjectWithFields(JSONObjectWithFields): - # pylint: disable=invalid-name,missing-docstring,no-self-argument + # pylint: disable=missing-docstring,no-self-argument # pylint: disable=too-few-public-methods x = Field('x', omitempty=True, encoder=(lambda x: x * 2), @@ -158,7 +158,7 @@ def y(value): raise errors.DeserializationError() return value - # pylint: disable=invalid-name + self.MockJSONObjectWithFields = MockJSONObjectWithFields self.mock = MockJSONObjectWithFields(x=None, y=2, z=3) diff --git a/acme/acme/jose/jwa_test.py b/acme/acme/jose/jwa_test.py index 3328d083..8ca51204 100644 --- a/acme/acme/jose/jwa_test.py +++ b/acme/acme/jose/jwa_test.py @@ -26,7 +26,7 @@ def sign(self, key, msg): def verify(self, key, msg, sig): raise NotImplementedError() # pragma: no cover - # pylint: disable=invalid-name + self.Sig1 = MockSig('Sig1') self.Sig2 = MockSig('Sig2') diff --git a/acme/acme/jose/jwk.py b/acme/acme/jose/jwk.py index 7a976f18..67f24334 100644 --- a/acme/acme/jose/jwk.py +++ b/acme/acme/jose/jwk.py @@ -186,7 +186,7 @@ def public_key(self): @classmethod def fields_from_json(cls, jobj): - # pylint: disable=invalid-name + n, e = (cls._decode_param(jobj[x]) for x in ('n', 'e')) public_numbers = rsa.RSAPublicNumbers(e=e, n=n) if 'd' not in jobj: # public key diff --git a/acme/acme/jose/util.py b/acme/acme/jose/util.py index ab3606ef..46c43bf3 100644 --- a/acme/acme/jose/util.py +++ b/acme/acme/jose/util.py @@ -7,7 +7,7 @@ class abstractclassmethod(classmethod): - # pylint: disable=invalid-name,too-few-public-methods + # pylint: disable=too-few-public-methods """Descriptor for an abstract classmethod. It augments the :mod:`abc` framework with an abstract @@ -172,7 +172,7 @@ def __repr__(self): class frozendict(collections.Mapping, collections.Hashable): - # pylint: disable=invalid-name,too-few-public-methods + # pylint: disable=too-few-public-methods """Frozen dictionary.""" __slots__ = ('_items', '_keys') diff --git a/acme/acme/jose/util_test.py b/acme/acme/jose/util_test.py index 4cdd9127..295c70fe 100644 --- a/acme/acme/jose/util_test.py +++ b/acme/acme/jose/util_test.py @@ -92,7 +92,7 @@ class ImmutableMapTest(unittest.TestCase): """Tests for acme.jose.util.ImmutableMap.""" def setUp(self): - # pylint: disable=invalid-name,too-few-public-methods + # pylint: disable=too-few-public-methods # pylint: disable=missing-docstring from acme.jose.util import ImmutableMap @@ -156,7 +156,7 @@ def test_repr(self): self.assertEqual("B(x='foo', y='bar')", repr(self.B(x='foo', y='bar'))) -class frozendictTest(unittest.TestCase): # pylint: disable=invalid-name +class frozendictTest(unittest.TestCase): """Tests for acme.jose.util.frozendict.""" def setUp(self): diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index d2d859bc..718a936d 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -64,7 +64,7 @@ def setUp(self): class MockConstant(_Constant): # pylint: disable=missing-docstring POSSIBLE_NAMES = {} - self.MockConstant = MockConstant # pylint: disable=invalid-name + self.MockConstant = MockConstant self.const_a = MockConstant('a') self.const_b = MockConstant('b') diff --git a/letsencrypt/account.py b/letsencrypt/account.py index c97e4f6f..81d31b83 100644 --- a/letsencrypt/account.py +++ b/letsencrypt/account.py @@ -54,7 +54,7 @@ def __init__(self, regr, key, meta=None): tz=pytz.UTC).replace(microsecond=0), creation_host=socket.getfqdn()) if meta is None else meta - self.id = hashlib.md5( # pylint: disable=invalid-name + self.id = hashlib.md5( self.key.key.public_key().public_bytes( encoding=serialization.Encoding.DER, format=serialization.PublicFormat.SubjectPublicKeyInfo) diff --git a/letsencrypt/display/enhancements.py b/letsencrypt/display/enhancements.py index 8edc72ba..c5619816 100644 --- a/letsencrypt/display/enhancements.py +++ b/letsencrypt/display/enhancements.py @@ -11,7 +11,7 @@ logger = logging.getLogger(__name__) # Define a helper function to avoid verbose code -util = zope.component.getUtility # pylint: disable=invalid-name +util = zope.component.getUtility def ask(enhancement): diff --git a/letsencrypt/display/ops.py b/letsencrypt/display/ops.py index 43705e30..cb424a81 100644 --- a/letsencrypt/display/ops.py +++ b/letsencrypt/display/ops.py @@ -12,7 +12,7 @@ logger = logging.getLogger(__name__) # Define a helper function to avoid verbose code -util = zope.component.getUtility # pylint: disable=invalid-name +util = zope.component.getUtility def choose_plugin(prepared, question): diff --git a/letsencrypt/log.py b/letsencrypt/log.py index e800d37c..6436f6fc 100644 --- a/letsencrypt/log.py +++ b/letsencrypt/log.py @@ -25,7 +25,7 @@ def __init__(self, level=logging.NOTSET, height=display_util.HEIGHT, logging.Handler.__init__(self, level) self.height = height self.width = width - # "dialog" collides with module name... pylint: disable=invalid-name + # "dialog" collides with module name... self.d = dialog.Dialog() if d is None else d self.lines = [] diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py index 59598a35..95ad56a0 100644 --- a/letsencrypt/plugins/common.py +++ b/letsencrypt/plugins/common.py @@ -23,10 +23,10 @@ def dest_namespace(name): """ArgumentParser dest namespace (prefix of all destinations).""" return name.replace("-", "_") + "_" -private_ips_regex = re.compile( # pylint: disable=invalid-name +private_ips_regex = re.compile( r"(^127\.0\.0\.1)|(^10\.)|(^172\.1[6-9]\.)|" r"(^172\.2[0-9]\.)|(^172\.3[0-1]\.)|(^192\.168\.)") -hostname_regex = re.compile( # pylint: disable=invalid-name +hostname_regex = re.compile( r"^(([a-z0-9]|[a-z0-9][a-z0-9\-]*[a-z0-9])\.)*[a-z]+$", re.IGNORECASE) @@ -173,7 +173,7 @@ def get_key_path(self, achall): achall.chall.encode("token") + '.pem') def _setup_challenge_cert(self, achall, s=None): - # pylint: disable=invalid-name + """Generate and write out challenge certificate.""" cert_path = self.get_cert_path(achall) key_path = self.get_key_path(achall) diff --git a/letsencrypt/tests/auth_handler_test.py b/letsencrypt/tests/auth_handler_test.py index ed29ead2..18ee5608 100644 --- a/letsencrypt/tests/auth_handler_test.py +++ b/letsencrypt/tests/auth_handler_test.py @@ -355,7 +355,7 @@ def test_not_supported(self): class MutuallyExclusiveTest(unittest.TestCase): """Tests for letsencrypt.auth_handler.mutually_exclusive.""" - # pylint: disable=invalid-name,missing-docstring,too-few-public-methods + # pylint: disable=missing-docstring,too-few-public-methods class A(object): pass diff --git a/letsencrypt/tests/log_test.py b/letsencrypt/tests/log_test.py index 50d0712e..c1afd2c8 100644 --- a/letsencrypt/tests/log_test.py +++ b/letsencrypt/tests/log_test.py @@ -8,7 +8,7 @@ class DialogHandlerTest(unittest.TestCase): def setUp(self): - self.d = mock.MagicMock() # pylint: disable=invalid-name + self.d = mock.MagicMock() from letsencrypt.log import DialogHandler self.handler = DialogHandler(height=2, width=6, d=self.d) diff --git a/letsencrypt/tests/reverter_test.py b/letsencrypt/tests/reverter_test.py index 62c47f8d..d31b6f2c 100644 --- a/letsencrypt/tests/reverter_test.py +++ b/letsencrypt/tests/reverter_test.py @@ -85,7 +85,7 @@ def test_multiple_saves_and_temp_revert(self): self.assertEqual(read_in(self.config1), "directive-dir1") def test_multiple_registration_fail_and_revert(self): - # pylint: disable=invalid-name + config3 = os.path.join(self.dir1, "config3.txt") update_file(config3, "Config3") config4 = os.path.join(self.dir2, "config4.txt") @@ -173,7 +173,7 @@ def test_recovery_routine_in_progress_failure(self): self.assertRaises(errors.ReverterError, self.reverter.recovery_routine) def test_recover_checkpoint_revert_temp_failures(self): - # pylint: disable=invalid-name + mock_recover = mock.MagicMock( side_effect=errors.ReverterError("e")) @@ -291,7 +291,7 @@ def test_rollback_improper_inputs(self): errors.ReverterError, self.reverter.rollback_checkpoints, "one") def test_rollback_finalize_checkpoint_valid_inputs(self): - # pylint: disable=invalid-name + config3 = self._setup_three_checkpoints() # Check resulting backup directory @@ -334,7 +334,7 @@ def test_finalize_checkpoint_cannot_title(self, mock_move): @mock.patch("letsencrypt.reverter.os.rename") def test_finalize_checkpoint_no_rename_directory(self, mock_rename): - # pylint: disable=invalid-name + self.reverter.add_to_checkpoint(self.sets[0], "perm save") mock_rename.side_effect = OSError From 2d578468bde4cbba2138216633de44bfdd46cf04 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 12:32:44 -0700 Subject: [PATCH 36/48] Use a verb -> function table instead of eval() - plugins_cmd() not plugins() broke the more minimalist eval() approach - more wrangling was required to mock out calls via the VERBS table --- letsencrypt/cli.py | 25 +++++++++++++++---------- letsencrypt/tests/cli_test.py | 2 ++ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index fd9d8cbb..66f99106 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -9,7 +9,6 @@ import pkg_resources import sys import time -import types import traceback import configargparse @@ -731,17 +730,23 @@ def create_parser(plugins, args): return helpful.parser, helpful.args # For now unfortunately this constant just needs to match the code below; -# there isn't an elegant way to autogenerate it in time. -VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins"] -HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + VERBS) +# there isn't an elegant way to autogenerate it in time. pylint: disable=bad-whitespace +VERBS = { + "run" : run, + "auth" : auth, + "install" : install, + "revoke" : revoke, + "rollback" : rollback, + "config_changes" : config_changes, + "plugins" : plugins_cmd +} +HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + + VERBS.keys()) def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") - def add_subparser(name): # pylint: disable=missing-docstring - # Each subcommand is implemented by a function of the same name - func = eval(name) # pylint: disable=eval-used - assert isinstance(func, types.FunctionType), "squirrels in namespace" + def add_subparser(name, func): # pylint: disable=missing-docstring subparser = subparsers.add_parser( name, help=func.__doc__.splitlines()[0], description=func.__doc__) subparser.set_defaults(func=func) @@ -752,8 +757,8 @@ def add_subparser(name): # pylint: disable=missing-docstring # these add_subparser objects return objects to which arguments could be # attached, but they have annoying arg ordering constrains so we use # groups instead: https://github.com/letsencrypt/letsencrypt/issues/820 - for v in VERBS: - add_subparser(v) + for v, func in VERBS.items(): + add_subparser(v, func) helpful.add_group("auth", description="Options for modifying how a cert is obtained") helpful.add_group("install", description="Options for modifying how a cert is deployed") diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 9a99a74c..f5613ee5 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -44,6 +44,8 @@ def _call(self, args): def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: + from letsencrypt import cli + cli.VERBS["run"] = mock_run self._call([]) self.assertEqual(1, mock_run.call_count) From bb167743f32f6b1d84a25295505d255aea331d5c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 30 Sep 2015 13:00:10 -0700 Subject: [PATCH 37/48] Don't call_registered() on SystemExit --- letsencrypt/error_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index 1f979a6d..1292f2bc 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -50,7 +50,7 @@ def __enter__(self): self.set_signal_handlers() def __exit__(self, exec_type, exec_value, trace): - if exec_value is not None: + if exec_type not in (None, SystemExit): logger.debug("Encountered exception:\n%s", "".join( traceback.format_exception(exec_type, exec_value, trace))) self.call_registered() From d85f42d71f5aba91cb96af6f9959c778fb047b9f Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 15:29:29 -0700 Subject: [PATCH 38/48] Plugins don't need to be in HELP_TOPICS They're already added as topics automatically, though they do need to be in the hand-written top level help. --- letsencrypt/cli.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 66f99106..1ad57b73 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -80,8 +80,8 @@ -h, --help [topic] print this message, or detailed help on a topic; the available topics are: - all, apache, automation, nginx, paths, security, testing, or any of the - subcommands + all, apache, automation, manual, nginx, paths, security, testing, or any of + the subcommands """ @@ -740,8 +740,7 @@ def create_parser(plugins, args): "config_changes" : config_changes, "plugins" : plugins_cmd } -HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] - + VERBS.keys()) +HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS.keys() def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") From 5ca1a27200fb17ac04104ba65c05d810bb20b906 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 15:31:32 -0700 Subject: [PATCH 39/48] Keep the acme/ subtree compatible with strict pylinting --- acme/acme/challenges.py | 4 ++-- acme/acme/challenges_test.py | 2 +- acme/acme/jose/interfaces_test.py | 4 ++-- acme/acme/jose/json_util_test.py | 6 +++--- acme/acme/jose/jwa_test.py | 2 +- acme/acme/jose/jwk.py | 2 +- acme/acme/jose/util.py | 4 ++-- acme/acme/jose/util_test.py | 4 ++-- acme/acme/messages_test.py | 2 +- 9 files changed, 15 insertions(+), 15 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index f5763adc..d81e77f8 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -315,7 +315,7 @@ class DVSNIResponse(ChallengeResponse): validation = jose.Field("validation", decoder=jose.JWS.from_json) @property - def z(self): + def z(self): # pylint: disable=invalid-name """The ``z`` parameter. :rtype: bytes @@ -333,7 +333,7 @@ def z_domain(self): :rtype: bytes """ - z = self.z + z = self.z # pylint: disable=invalid-name return z[:32] + b'.' + z[32:] + self.DOMAIN_SUFFIX @property diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index b3f48cdf..ed44d4c4 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -269,7 +269,7 @@ def setUp(self): 'validation': self.validation.to_json(), } - + # pylint: disable=invalid-name label1 = b'e2df3498860637c667fedadc5a8494ec' label2 = b'09dcc75553c9b3bd73662b50e71b1e42' self.z = label1 + label2 diff --git a/acme/acme/jose/interfaces_test.py b/acme/acme/jose/interfaces_test.py index a3ee124f..380c3a2a 100644 --- a/acme/acme/jose/interfaces_test.py +++ b/acme/acme/jose/interfaces_test.py @@ -8,7 +8,7 @@ class JSONDeSerializableTest(unittest.TestCase): def setUp(self): from acme.jose.interfaces import JSONDeSerializable - # pylint: disable=missing-docstring + # pylint: disable=missing-docstring,invalid-name class Basic(JSONDeSerializable): def __init__(self, v): @@ -53,7 +53,7 @@ def from_json(cls, jobj): self.nested = Basic([[self.basic1]]) self.tuple = Basic(('foo',)) - + # pylint: disable=invalid-name self.Basic = Basic self.Sequence = Sequence self.Mapping = Mapping diff --git a/acme/acme/jose/json_util_test.py b/acme/acme/jose/json_util_test.py index f751382e..a055f3bf 100644 --- a/acme/acme/jose/json_util_test.py +++ b/acme/acme/jose/json_util_test.py @@ -92,7 +92,7 @@ def setUp(self): from acme.jose.json_util import JSONObjectWithFieldsMeta self.field = Field('Baz') self.field2 = Field('Baz2') - # pylint: disable=missing-docstring,too-few-public-methods + # pylint: disable=invalid-name,missing-docstring,too-few-public-methods # pylint: disable=blacklisted-name @six.add_metaclass(JSONObjectWithFieldsMeta) @@ -138,7 +138,7 @@ def setUp(self): from acme.jose.json_util import Field class MockJSONObjectWithFields(JSONObjectWithFields): - # pylint: disable=missing-docstring,no-self-argument + # pylint: disable=invalid-name,missing-docstring,no-self-argument # pylint: disable=too-few-public-methods x = Field('x', omitempty=True, encoder=(lambda x: x * 2), @@ -158,7 +158,7 @@ def y(value): raise errors.DeserializationError() return value - + # pylint: disable=invalid-name self.MockJSONObjectWithFields = MockJSONObjectWithFields self.mock = MockJSONObjectWithFields(x=None, y=2, z=3) diff --git a/acme/acme/jose/jwa_test.py b/acme/acme/jose/jwa_test.py index 8ca51204..3328d083 100644 --- a/acme/acme/jose/jwa_test.py +++ b/acme/acme/jose/jwa_test.py @@ -26,7 +26,7 @@ def sign(self, key, msg): def verify(self, key, msg, sig): raise NotImplementedError() # pragma: no cover - + # pylint: disable=invalid-name self.Sig1 = MockSig('Sig1') self.Sig2 = MockSig('Sig2') diff --git a/acme/acme/jose/jwk.py b/acme/acme/jose/jwk.py index 67f24334..7a976f18 100644 --- a/acme/acme/jose/jwk.py +++ b/acme/acme/jose/jwk.py @@ -186,7 +186,7 @@ def public_key(self): @classmethod def fields_from_json(cls, jobj): - + # pylint: disable=invalid-name n, e = (cls._decode_param(jobj[x]) for x in ('n', 'e')) public_numbers = rsa.RSAPublicNumbers(e=e, n=n) if 'd' not in jobj: # public key diff --git a/acme/acme/jose/util.py b/acme/acme/jose/util.py index 46c43bf3..ab3606ef 100644 --- a/acme/acme/jose/util.py +++ b/acme/acme/jose/util.py @@ -7,7 +7,7 @@ class abstractclassmethod(classmethod): - # pylint: disable=too-few-public-methods + # pylint: disable=invalid-name,too-few-public-methods """Descriptor for an abstract classmethod. It augments the :mod:`abc` framework with an abstract @@ -172,7 +172,7 @@ def __repr__(self): class frozendict(collections.Mapping, collections.Hashable): - # pylint: disable=too-few-public-methods + # pylint: disable=invalid-name,too-few-public-methods """Frozen dictionary.""" __slots__ = ('_items', '_keys') diff --git a/acme/acme/jose/util_test.py b/acme/acme/jose/util_test.py index 295c70fe..4cdd9127 100644 --- a/acme/acme/jose/util_test.py +++ b/acme/acme/jose/util_test.py @@ -92,7 +92,7 @@ class ImmutableMapTest(unittest.TestCase): """Tests for acme.jose.util.ImmutableMap.""" def setUp(self): - # pylint: disable=too-few-public-methods + # pylint: disable=invalid-name,too-few-public-methods # pylint: disable=missing-docstring from acme.jose.util import ImmutableMap @@ -156,7 +156,7 @@ def test_repr(self): self.assertEqual("B(x='foo', y='bar')", repr(self.B(x='foo', y='bar'))) -class frozendictTest(unittest.TestCase): +class frozendictTest(unittest.TestCase): # pylint: disable=invalid-name """Tests for acme.jose.util.frozendict.""" def setUp(self): diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index 718a936d..d2d859bc 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -64,7 +64,7 @@ def setUp(self): class MockConstant(_Constant): # pylint: disable=missing-docstring POSSIBLE_NAMES = {} - self.MockConstant = MockConstant + self.MockConstant = MockConstant # pylint: disable=invalid-name self.const_a = MockConstant('a') self.const_b = MockConstant('b') From 2406fc0486d8f74ad9979b1973e9e24d9d453df7 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 15:56:58 -0700 Subject: [PATCH 40/48] Go back to VERBS as a list The dictionary was destroying the ordering, which was important. --- letsencrypt/cli.py | 28 ++++++++++++---------------- letsencrypt/tests/cli_test.py | 2 -- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 1ad57b73..73dd24bd 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -730,24 +730,20 @@ def create_parser(plugins, args): return helpful.parser, helpful.args # For now unfortunately this constant just needs to match the code below; -# there isn't an elegant way to autogenerate it in time. pylint: disable=bad-whitespace -VERBS = { - "run" : run, - "auth" : auth, - "install" : install, - "revoke" : revoke, - "rollback" : rollback, - "config_changes" : config_changes, - "plugins" : plugins_cmd -} -HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS.keys() +# there isn't an elegant way to autogenerate it in time. +VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins"] +HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") - def add_subparser(name, func): # pylint: disable=missing-docstring - subparser = subparsers.add_parser( - name, help=func.__doc__.splitlines()[0], description=func.__doc__) + def add_subparser(name): # pylint: disable=missing-docstring + if name == "plugins": + func = plugins_cmd + else: + func = eval(name) # pylint: disable=eval-used + h = func.__doc__.splitlines()[0] + subparser = subparsers.add_parser(name, help=h, description=func.__doc__) subparser.set_defaults(func=func) return subparser @@ -756,8 +752,8 @@ def add_subparser(name, func): # pylint: disable=missing-docstring # these add_subparser objects return objects to which arguments could be # attached, but they have annoying arg ordering constrains so we use # groups instead: https://github.com/letsencrypt/letsencrypt/issues/820 - for v, func in VERBS.items(): - add_subparser(v, func) + for v in VERBS: + add_subparser(v) helpful.add_group("auth", description="Options for modifying how a cert is obtained") helpful.add_group("install", description="Options for modifying how a cert is deployed") diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index f5613ee5..9a99a74c 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -44,8 +44,6 @@ def _call(self, args): def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: - from letsencrypt import cli - cli.VERBS["run"] = mock_run self._call([]) self.assertEqual(1, mock_run.call_count) From 95c4b55da09aee285bd823bf993d43089907456a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 30 Sep 2015 16:49:03 -0700 Subject: [PATCH 41/48] Mark Nginx as non-working. --- letsencrypt-nginx/letsencrypt_nginx/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index 3f6d6f32..a88607e5 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -56,7 +56,7 @@ class NginxConfigurator(common.Plugin): zope.interface.implements(interfaces.IAuthenticator, interfaces.IInstaller) zope.interface.classProvides(interfaces.IPluginFactory) - description = "Nginx Web Server - Alpha" + description = "Nginx Web Server - currently doesn't work" @classmethod def add_parser_arguments(cls, add): From 11ca1108c2536adb0d735e76829f178f06a08715 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 16:53:08 -0700 Subject: [PATCH 42/48] Test cases for command line help --- letsencrypt/tests/cli_test.py | 37 ++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 9a99a74c..75eec197 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -2,6 +2,7 @@ import itertools import os import shutil +import StringIO import traceback import tempfile import unittest @@ -42,6 +43,21 @@ def _call(self, args): ret = cli.main(args) return ret, stdout, stderr, client + def _call_stdout(self, args): + """ + Variant of _call that preserves stdout so that it can be mocked by the + caller. + """ + from letsencrypt import cli + args = ['--text', '--config-dir', self.config_dir, + '--work-dir', self.work_dir, '--logs-dir', self.logs_dir, + '--agree-eula'] + args + with mock.patch('letsencrypt.cli.sys.stderr') as stderr: + with mock.patch('letsencrypt.cli.client') as client: + ret = cli.main(args) + return ret, None, stderr, client + + def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: self._call([]) @@ -49,7 +65,26 @@ def test_no_flags(self): def test_help(self): self.assertRaises(SystemExit, self._call, ['--help']) - self.assertRaises(SystemExit, self._call, ['--help all']) + self.assertRaises(SystemExit, self._call, ['--help', 'all']) + output = StringIO.StringIO() + with mock.patch('letsencrypt.cli.sys.stdout', new=output): + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'all']) + out = output.getvalue() + self.assertTrue("--configurator" in out) + self.assertTrue("how a cert is deployed" in out) + self.assertTrue("--manual-test-mode" in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['-h', 'nginx']) + out = output.getvalue() + self.assertTrue("--nginx-ctl" in out) + self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--checkpoints" not in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'plugins']) + out = output.getvalue() + self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--prepare" in out) + self.assertTrue("Plugin options" in out) def test_rollback(self): _, _, _, client = self._call(['rollback']) From 43cb36807a001562726bceaaaae00d708fcc5ed2 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 17:00:09 -0700 Subject: [PATCH 43/48] Also test top level help --- letsencrypt/tests/cli_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 75eec197..0a92aba6 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -85,6 +85,12 @@ def test_help(self): self.assertTrue("--manual-test-mode" not in out) self.assertTrue("--prepare" in out) self.assertTrue("Plugin options" in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['-h']) + out = output.getvalue() + from letsencrypt import cli + self.assertTrue(cli.USAGE in out) + def test_rollback(self): _, _, _, client = self._call(['rollback']) From 9cf2ea8a5742d8868f1f1c47377626a741464bc7 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 30 Sep 2015 17:16:27 -0700 Subject: [PATCH 44/48] Report Apache correctly when uninstalled --- .../letsencrypt_apache/configurator.py | 6 +++ .../tests/configurator_test.py | 10 +++- .../letsencrypt_apache/tests/util.py | 47 ++++++++++--------- 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index ad3c62d2..f3d2b5f9 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -137,6 +137,12 @@ def prepare(self): :raises .errors.PluginError: If there is any other error """ + # Verify Apache is installed + for exe in (self.conf("ctl"), self.conf("enmod"), + self.conf("dismod"), self.conf("init-script")): + if not le_util.exe_exists(exe): + raise errors.NoInstallationError + # Make sure configuration is valid self.config_test() diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 026594a8..7c2137c4 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -37,8 +37,16 @@ def tearDown(self): shutil.rmtree(self.config_dir) shutil.rmtree(self.work_dir) + @mock.patch("letsencrypt_apache.configurator.le_util.exe_exists") + def test_prepare_no_install(self, mock_exe_exists): + mock_exe_exists.return_value = False + self.assertRaises( + errors.NoInstallationError, self.config.prepare) + @mock.patch("letsencrypt_apache.parser.ApacheParser") - def test_prepare_version(self, _): + @mock.patch("letsencrypt_apache.configurator.le_util.exe_exists") + def test_prepare_version(self, mock_exe_exists, _): + mock_exe_exists.return_value = True self.config.version = None self.config.config_test = mock.Mock() self.config.get_version = mock.Mock(return_value=(1, 1)) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/util.py b/letsencrypt-apache/letsencrypt_apache/tests/util.py index b544e06e..2594ba77 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/util.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/util.py @@ -66,31 +66,34 @@ def get_apache_configurator( """ backups = os.path.join(work_dir, "backups") + mock_le_config = mock.MagicMock( + apache_server_root=config_path, + apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"], + backup_dir=backups, + config_dir=config_dir, + temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"), + in_progress_dir=os.path.join(backups, "IN_PROGRESS"), + work_dir=work_dir) with mock.patch("letsencrypt_apache.configurator." "subprocess.Popen") as mock_popen: - with mock.patch("letsencrypt_apache.parser.ApacheParser." - "update_runtime_variables"): - # This indicates config_test passes - mock_popen().communicate.return_value = ("Fine output", "No problems") - mock_popen().returncode = 0 - - config = configurator.ApacheConfigurator( - config=mock.MagicMock( - apache_server_root=config_path, - apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"], - backup_dir=backups, - config_dir=config_dir, - temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"), - in_progress_dir=os.path.join(backups, "IN_PROGRESS"), - work_dir=work_dir), - name="apache", - version=version) - # This allows testing scripts to set it a bit more quickly - if conf is not None: - config.conf = conf # pragma: no cover - - config.prepare() + # This indicates config_test passes + mock_popen().communicate.return_value = ("Fine output", "No problems") + mock_popen().returncode = 0 + with mock.patch("letsencrypt_apache.configurator.le_util." + "exe_exists") as mock_exe_exists: + mock_exe_exists.return_value = True + with mock.patch("letsencrypt_apache.parser.ApacheParser." + "update_runtime_variables"): + config = configurator.ApacheConfigurator( + config=mock_le_config, + name="apache", + version=version) + # This allows testing scripts to set it a bit more quickly + if conf is not None: + config.conf = conf # pragma: no cover + + config.prepare() return config From 8041b35f9988d1193528df0d36b14eca35babc3a Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 19:06:16 -0700 Subject: [PATCH 45/48] Make sure the LICENSE file is accurate for first pre-relase - In general copyrights remain with their respective authors or authors' organizations, but with license granted by clause 5 of the Apache License. - Presently the plurality of the copyright in the client is held by EFF as a result of work-for-hire by jdkasten, bmw, schoen, pde, rolandshoemaker and jsha; or by Jakub Warmuz or his employer, Google. --- LICENSE.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/LICENSE.txt b/LICENSE.txt index 5a9f6fa5..2ed75252 100644 --- a/LICENSE.txt +++ b/LICENSE.txt @@ -1,5 +1,5 @@ -Let's Encrypt: -Copyright (c) Internet Security Research Group +Let's Encrypt Python Client +Copyright (c) Electronic Frontier Foundation and others Licensed Apache Version 2.0 Incorporating code from nginxparser From 268368b3e928e669420beeefd5d82a8af4de4a1f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 1 Oct 2015 10:12:38 -0700 Subject: [PATCH 46/48] Updated README to reflect state of Nginx plugin --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 23e4dad2..43ecd413 100644 --- a/README.rst +++ b/README.rst @@ -79,7 +79,7 @@ Current Features * web servers supported: - apache/2.x (tested and working on Ubuntu Linux) - - nginx/0.8.48+ (tested and mostly working on Ubuntu Linux) + - nginx/0.8.48+ (under development) - standalone (runs its own webserver to prove you control the domain) * the private key is generated locally on your system From 6bde83c9835b1fba9a935f341e62a48b8393d189 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 1 Oct 2015 11:53:11 -0700 Subject: [PATCH 47/48] Fixed indentation in storage.py --- letsencrypt/storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py index 08dff25a..be270a76 100644 --- a/letsencrypt/storage.py +++ b/letsencrypt/storage.py @@ -520,7 +520,7 @@ def should_autorenew(self): remaining = expiry - now if remaining < autorenew_interval: return True - return False + return False @classmethod def new_lineage(cls, lineagename, cert, privkey, chain, From d7a16ecfcb76d50702375b3dbb66669e59818ddc Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 1 Oct 2015 15:39:55 -0700 Subject: [PATCH 48/48] Added tests and documentation --- letsencrypt/error_handler.py | 5 +++-- letsencrypt/tests/error_handler_test.py | 9 +++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index 1292f2bc..8b0eb7c8 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -22,8 +22,8 @@ class ErrorHandler(object): """Registers functions to be called if an exception or signal occurs. This class allows you to register functions that will be called when - an exception or signal is encountered. The class works best as a - context manager. For example: + an exception (excluding SystemExit) or signal is encountered. The + class works best as a context manager. For example: with ErrorHandler(cleanup_func): do_something() @@ -50,6 +50,7 @@ def __enter__(self): self.set_signal_handlers() def __exit__(self, exec_type, exec_value, trace): + # SystemExit is ignored to properly handle forks that don't exec if exec_type not in (None, SystemExit): logger.debug("Encountered exception:\n%s", "".join( traceback.format_exception(exec_type, exec_value, trace))) diff --git a/letsencrypt/tests/error_handler_test.py b/letsencrypt/tests/error_handler_test.py index 66acac93..c92f1243 100644 --- a/letsencrypt/tests/error_handler_test.py +++ b/letsencrypt/tests/error_handler_test.py @@ -1,5 +1,6 @@ """Tests for letsencrypt.error_handler.""" import signal +import sys import unittest import mock @@ -50,6 +51,14 @@ def test_bad_recovery(self): self.init_func.assert_called_once_with() bad_func.assert_called_once_with() + def test_sysexit_ignored(self): + try: + with self.handler: + sys.exit(0) + except SystemExit: + pass + self.assertFalse(self.init_func.called) + if __name__ == "__main__": unittest.main() # pragma: no cover