From ee4e7c4f71ab181067c878f8fb095611fd19d6c0 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 6 Mar 2017 18:34:14 -0800 Subject: [PATCH 01/40] Improve packaging guide. Correct tagging format. Add request for random offsets for renewal. Make all bulleted lists consistent. Remove obsolete `letsencrypt` package for Fedora. Remove discouraged letshelp-certbot package. --- docs/packaging.rst | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/docs/packaging.rst b/docs/packaging.rst index 1a1b83f50..c3cdfb579 100644 --- a/docs/packaging.rst +++ b/docs/packaging.rst @@ -16,21 +16,19 @@ The following scripts are used in the process: - https://github.com/letsencrypt/letsencrypt/blob/master/tools/release.sh -We currently version with the following scheme: +We use git tags to identify releases, using `Semantic Versioning`_. For +example: `v0.11.1`. -- ``0.1.0`` -- ``0.2.0dev`` for developement in ``master`` -- ``0.2.0`` (only temporarily in ``master``) -- ... +.. _`Semantic Versioning`: http://semver.org/ Notes for package maintainers ============================= -0. Please use our releases, not ``master``! +0. Please use our tagged releases, not ``master``! 1. Do not package ``certbot-compatibility-test`` or ``letshelp-certbot`` - it's only used internally. -2. If you'd like to include automated renewal in your package ``certbot renew -q`` should be added to crontab or systemd timer. +2. If you'd like to include automated renewal in your package ``certbot renew -q`` should be added to crontab or systemd timer. Add a random per-system offset to avoid having a large number of clients hit Let's Encrypt's servers simultaneously. 3. ``jws`` is an internal script for ``acme`` module and it doesn't have to be packaged - it's mostly for debugging: you can use it as ``echo foo | jws sign | jws verify``. @@ -44,34 +42,33 @@ Arch ---- From our official releases: + - https://www.archlinux.org/packages/community/any/python2-acme - https://www.archlinux.org/packages/community/any/certbot - https://www.archlinux.org/packages/community/any/certbot-apache - https://www.archlinux.org/packages/community/any/certbot-nginx -- https://www.archlinux.org/packages/community/any/letshelp-certbot From ``master``: https://aur.archlinux.org/packages/certbot-git Debian (and its derivatives, including Ubuntu) ------ -https://packages.debian.org/sid/certbot -https://packages.debian.org/sid/python-certbot -https://packages.debian.org/sid/python-certbot-apache +- https://packages.debian.org/sid/certbot +- https://packages.debian.org/sid/python-certbot +- https://packages.debian.org/sid/python-certbot-apache Fedora ------ In Fedora 23+. -- https://admin.fedoraproject.org/pkgdb/package/letsencrypt/ - https://admin.fedoraproject.org/pkgdb/package/certbot/ - https://admin.fedoraproject.org/pkgdb/package/python-acme/ FreeBSD ------- -https://svnweb.freebsd.org/ports/head/security/py-certbot/ +- https://svnweb.freebsd.org/ports/head/security/py-certbot/ GNU Guix -------- From da1459df0d3db4339f62456d7dac3abf5df03723 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 14 Mar 2017 16:36:39 -0700 Subject: [PATCH 02/40] use docker hub in install guide --- docs/install.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/install.rst b/docs/install.rst index aa59e44ec..548c8f2a3 100644 --- a/docs/install.rst +++ b/docs/install.rst @@ -120,7 +120,7 @@ to, `install Docker`_, then issue the following command: sudo docker run -it --rm -p 443:443 -p 80:80 --name certbot \ -v "/etc/letsencrypt:/etc/letsencrypt" \ -v "/var/lib/letsencrypt:/var/lib/letsencrypt" \ - quay.io/letsencrypt/letsencrypt:latest certonly + certbot/certbot certonly Running Certbot with the ``certonly`` command will obtain a certificate and place it in the directory ``/etc/letsencrypt/live`` on your system. Because Certonly cannot install the certificate from From 5758b1687d5cb79beef8b51d90c655657985f6a4 Mon Sep 17 00:00:00 2001 From: kernelpanek Date: Wed, 15 Mar 2017 00:25:26 -0600 Subject: [PATCH 03/40] Fixes issue when parsing an Nginx configuration file containing multiline quoted strings --- certbot-nginx/certbot_nginx/nginxparser.py | 6 +++--- .../certbot_nginx/tests/nginxparser_test.py | 15 +++++++++++++++ .../testdata/etc_nginx/multiline_quotes.conf | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/multiline_quotes.conf diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index f6437c589..908a6d6cf 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -6,7 +6,7 @@ import string from pyparsing import ( Literal, White, Word, alphanums, CharsNotIn, Combine, Forward, Group, - Optional, OneOrMore, Regex, ZeroOrMore) + Optional, OneOrMore, QuotedString, Regex, ZeroOrMore) from pyparsing import stringEnd from pyparsing import restOfLine @@ -29,8 +29,8 @@ class RawNginxParser(object): # any chars in single or double quotes # All of these COULD be upgraded to something like # https://stackoverflow.com/a/16130746 - dquoted = Regex(r'(\".*\")') - squoted = Regex(r"(\'.*\')") + dquoted = QuotedString('"', multiline=True) + squoted = QuotedString("'", multiline=True) nonspecial = Regex(r"[^\{\};,]") varsub = Regex(r"(\$\{\w+\})") # nonspecial nibbles one character at a time, but the other objects take diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index e83b414cf..650090cb2 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -109,6 +109,21 @@ class TestRawNginxParser(unittest.TestCase): ['blah', '"hello;world"'], ['try_files', '$uri @rewrites']]]]]]) + def test_parse_from_file3(self): + with open(util.get_data_filename('multiline_quotes.conf')) as handle: + parsed = util.filter_comments(load(handle)) + self.assertEqual( + parsed, + [[['http'], + [[['server'], + [['listen', '*:443'], + [['location', '/'], + [['body_filter_by_lua', + 'ngx.ctx.buffered = (ngx.ctx.buffered or "") .. string.sub(ngx.arg[1], 1, 1000)\n' + ' if ngx.arg[2] then\n' + ' ngx.var.resp_body = ngx.ctx.buffered\n' + ' end']]]]]]]]) + def test_abort_on_parse_failure(self): with open(util.get_data_filename('broken.conf')) as handle: self.assertRaises(ParseException, load, handle) diff --git a/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/multiline_quotes.conf b/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/multiline_quotes.conf new file mode 100644 index 000000000..74cd84bcd --- /dev/null +++ b/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/multiline_quotes.conf @@ -0,0 +1,16 @@ +# Test nginx configuration file with multiline quoted strings. +# Good example of usage for multilined quoted values is when +# using Openresty's Lua directives and you wish to keep the +# inline Lua code readable. +http { + server { + listen *:443; # because there should be no other port open. + + location / { + body_filter_by_lua 'ngx.ctx.buffered = (ngx.ctx.buffered or "") .. string.sub(ngx.arg[1], 1, 1000) + if ngx.arg[2] then + ngx.var.resp_body = ngx.ctx.buffered + end'; + } + } +} From e715b49dd2b826460f7871776e85f46bc2d8f1d2 Mon Sep 17 00:00:00 2001 From: kernelpanek Date: Wed, 15 Mar 2017 01:26:16 -0600 Subject: [PATCH 04/40] Don't unquote the results of the parse --- certbot-nginx/certbot_nginx/nginxparser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 908a6d6cf..67ac7c3a1 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -29,8 +29,8 @@ class RawNginxParser(object): # any chars in single or double quotes # All of these COULD be upgraded to something like # https://stackoverflow.com/a/16130746 - dquoted = QuotedString('"', multiline=True) - squoted = QuotedString("'", multiline=True) + dquoted = QuotedString('"', multiline=True, unquoteResults=False) + squoted = QuotedString("'", multiline=True, unquoteResults=False) nonspecial = Regex(r"[^\{\};,]") varsub = Regex(r"(\$\{\w+\})") # nonspecial nibbles one character at a time, but the other objects take From f791af5afea61359da4576d006f3557236e69966 Mon Sep 17 00:00:00 2001 From: Richard Panek Date: Wed, 15 Mar 2017 02:13:09 -0600 Subject: [PATCH 05/40] New switch for QuotedStrings allows retainer of quotes but my test fails --- certbot-nginx/certbot_nginx/tests/nginxparser_test.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index 650090cb2..44bebb373 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -119,10 +119,13 @@ class TestRawNginxParser(unittest.TestCase): [['listen', '*:443'], [['location', '/'], [['body_filter_by_lua', - 'ngx.ctx.buffered = (ngx.ctx.buffered or "") .. string.sub(ngx.arg[1], 1, 1000)\n' - ' if ngx.arg[2] then\n' - ' ngx.var.resp_body = ngx.ctx.buffered\n' - ' end']]]]]]]]) + '\'ngx.ctx.buffered = (ngx.ctx.buffered or "")' + ' .. string.sub(ngx.arg[1], 1, 1000)\n' + ' ' + 'if ngx.arg[2] then\n' + ' ' + 'ngx.var.resp_body = ngx.ctx.buffered\n' + ' end\'']]]]]]]]) def test_abort_on_parse_failure(self): with open(util.get_data_filename('broken.conf')) as handle: From 5fa20805586213262d5b5848747bfc5ec8241139 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Wed, 15 Mar 2017 17:05:52 -0700 Subject: [PATCH 06/40] If we fail to reload Nginx, write to temporary files instead of piping output (#4333) Due to issues with piping and Nginx on Arch. --- certbot-nginx/certbot_nginx/configurator.py | 27 +++++++++++---------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 7348def2f..46ce18ab2 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -5,6 +5,7 @@ import re import shutil import socket import subprocess +import tempfile import time import OpenSSL @@ -829,22 +830,22 @@ def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"): """ try: - proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - stdout, stderr = proc.communicate() + proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"]) + proc.communicate() if proc.returncode != 0: # Maybe Nginx isn't running - nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - stdout, stderr = nginx_proc.communicate() - - if nginx_proc.returncode != 0: - # Enter recovery routine... - raise errors.MisconfigurationError( - "nginx restart failed:\n%s\n%s" % (stdout, stderr)) + # Write to temporary files instead of piping because of communication issues on Arch + # https://github.com/certbot/certbot/issues/4324 + with tempfile.TemporaryFile() as out: + with tempfile.TemporaryFile() as err: + nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf], + stdout=out, stderr=err) + nginx_proc.communicate() + if nginx_proc.returncode != 0: + # Enter recovery routine... + raise errors.MisconfigurationError( + "nginx restart failed:\n%s\n%s" % (out.read(), err.read())) except (OSError, ValueError): raise errors.MisconfigurationError("nginx restart failed") From edcfc49303d4ddd2e69ba6ac6933af01d00e332c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 17 Mar 2017 13:02:41 -0700 Subject: [PATCH 07/40] Use setattr in NamespaceConfig (#4362) * set setattr in NamespaceConfig * remove unnecessary uses of .namespace * add simple test to ensure it works --- certbot/client.py | 2 +- certbot/configuration.py | 5 ++++- certbot/main.py | 8 ++++---- certbot/plugins/selection.py | 5 ++--- certbot/renewal.py | 10 +++++----- certbot/tests/configuration_test.py | 6 ++++++ certbot/tests/renewal_test.py | 8 ++++---- 7 files changed, 26 insertions(+), 18 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 0f6b35f09..8933ef27e 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -166,7 +166,7 @@ def perform_registration(acme, config): "registration again." % config.email) raise errors.Error(msg) else: - config.namespace.email = display_ops.get_email(invalid=True) + config.email = display_ops.get_email(invalid=True) return perform_registration(acme, config) else: raise diff --git a/certbot/configuration.py b/certbot/configuration.py index d25378922..30c6f0437 100644 --- a/certbot/configuration.py +++ b/certbot/configuration.py @@ -42,7 +42,7 @@ class NamespaceConfig(object): """ def __init__(self, namespace): - self.namespace = namespace + object.__setattr__(self, 'namespace', namespace) self.namespace.config_dir = os.path.abspath(self.namespace.config_dir) self.namespace.work_dir = os.path.abspath(self.namespace.work_dir) @@ -54,6 +54,9 @@ class NamespaceConfig(object): def __getattr__(self, name): return getattr(self.namespace, name) + def __setattr__(self, name, value): + setattr(self.namespace, name, value) + @property def server_path(self): """File path based on ``server``.""" diff --git a/certbot/main.py b/certbot/main.py index 1f247a7d6..b0689faa2 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -359,7 +359,7 @@ def _determine_account(config): acc = accounts[0] else: # no account registered yet if config.email is None and not config.register_unsafely_without_email: - config.namespace.email = display_ops.get_email() + config.email = display_ops.get_email() def _tos_cb(regr): if config.tos: @@ -382,7 +382,7 @@ def _determine_account(config): raise errors.Error( "Unable to register an account with ACME server") - config.namespace.account = acc.id + config.account = acc.id return acc, acme @@ -459,7 +459,7 @@ def register(config, unused_plugins): return ("--register-unsafely-without-email provided, however, a " "new e-mail address must\ncurrently be provided when " "updating a registration.") - config.namespace.email = display_ops.get_email(optional=False) + config.email = display_ops.get_email(optional=False) acc, acme = _determine_account(config) acme_client = client.Client(config, acc, None, None, acme=acme) @@ -565,7 +565,7 @@ def certificates(config, unused_plugins): def revoke(config, unused_plugins): # TODO: coop with renewal config """Revoke a previously obtained certificate.""" # For user-agent construction - config.namespace.installer = config.namespace.authenticator = "None" + config.installer = config.authenticator = "None" if config.key_path is not None: # revocation by cert key logger.debug("Revoking %s using cert key %s", config.cert_path[0], config.key_path[0]) diff --git a/certbot/plugins/selection.py b/certbot/plugins/selection.py index 81387c435..d138001e6 100644 --- a/certbot/plugins/selection.py +++ b/certbot/plugins/selection.py @@ -137,9 +137,8 @@ noninstaller_plugins = ["webroot", "manual", "standalone"] def record_chosen_plugins(config, plugins, auth, inst): "Update the config entries to reflect the plugins we actually selected." - cn = config.namespace - cn.authenticator = plugins.find_init(auth).name if auth else "None" - cn.installer = plugins.find_init(inst).name if inst else "None" + config.authenticator = plugins.find_init(auth).name if auth else "None" + config.installer = plugins.find_init(inst).name if inst else "None" def choose_configurator_plugins(config, plugins, verb): diff --git a/certbot/renewal.py b/certbot/renewal.py index a0cc872a0..6eb171763 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -103,13 +103,13 @@ def _restore_webroot_config(config, renewalparams): """ if "webroot_map" in renewalparams: if not cli.set_by_cli("webroot_map"): - config.namespace.webroot_map = renewalparams["webroot_map"] + config.webroot_map = renewalparams["webroot_map"] elif "webroot_path" in renewalparams: logger.debug("Ancient renewal conf file without webroot-map, restoring webroot-path") wp = renewalparams["webroot_path"] if isinstance(wp, str): # prior to 0.1.0, webroot_path was a string wp = [wp] - config.namespace.webroot_path = wp + config.webroot_path = wp def _restore_plugin_configs(config, renewalparams): @@ -148,10 +148,10 @@ def _restore_plugin_configs(config, renewalparams): if config_value in ("None", "True", "False"): # bool("False") == True # pylint: disable=eval-used - setattr(config.namespace, config_item, eval(config_value)) + setattr(config, config_item, eval(config_value)) else: cast = cli.argparse_type(config_item) - setattr(config.namespace, config_item, cast(config_value)) + setattr(config, config_item, cast(config_value)) def restore_required_config_elements(config, renewalparams): @@ -172,7 +172,7 @@ def restore_required_config_elements(config, renewalparams): for item_name, restore_func in required_items: if item_name in renewalparams and not cli.set_by_cli(item_name): value = restore_func(item_name, renewalparams[item_name]) - setattr(config.namespace, item_name, value) + setattr(config, item_name, value) def _restore_pref_challs(unused_name, value): diff --git a/certbot/tests/configuration_test.py b/certbot/tests/configuration_test.py index 3a2f7d291..66a07dddd 100644 --- a/certbot/tests/configuration_test.py +++ b/certbot/tests/configuration_test.py @@ -120,6 +120,12 @@ class NamespaceConfigTest(unittest.TestCase): self.assertTrue(os.path.isabs(config.live_dir)) self.assertTrue(os.path.isabs(config.renewal_configs_dir)) + def test_get_and_set_attr(self): + self.config.foo = 42 + self.assertEqual(self.config.namespace.foo, 42) + self.config.namespace.bar = 1337 + self.assertEqual(self.config.bar, 1337) + if __name__ == '__main__': unittest.main() # pragma: no cover diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index cd53aa91c..d97e05783 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -52,7 +52,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase): def test_allow_subset_of_names_success(self, mock_set_by_cli): mock_set_by_cli.return_value = False self._call(self.config, {'allow_subset_of_names': 'True'}) - self.assertTrue(self.config.namespace.allow_subset_of_names is True) + self.assertTrue(self.config.allow_subset_of_names is True) @mock.patch('certbot.renewal.cli.set_by_cli') def test_allow_subset_of_names_failure(self, mock_set_by_cli): @@ -68,7 +68,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase): self._call(self.config, renewalparams) expected = [challenges.TLSSNI01.typ, challenges.HTTP01.typ, challenges.DNS01.typ] - self.assertEqual(self.config.namespace.pref_challs, expected) + self.assertEqual(self.config.pref_challs, expected) @mock.patch('certbot.renewal.cli.set_by_cli') def test_pref_challs_str(self, mock_set_by_cli): @@ -76,7 +76,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase): renewalparams = {'pref_challs': 'dns'} self._call(self.config, renewalparams) expected = [challenges.DNS01.typ] - self.assertEqual(self.config.namespace.pref_challs, expected) + self.assertEqual(self.config.pref_challs, expected) @mock.patch('certbot.renewal.cli.set_by_cli') def test_pref_challs_failure(self, mock_set_by_cli): @@ -88,7 +88,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase): def test_must_staple_success(self, mock_set_by_cli): mock_set_by_cli.return_value = False self._call(self.config, {'must_staple': 'True'}) - self.assertTrue(self.config.namespace.must_staple is True) + self.assertTrue(self.config.must_staple is True) @mock.patch('certbot.renewal.cli.set_by_cli') def test_must_staple_failure(self, mock_set_by_cli): From 4cad594b4ba81734c3db3343cc418440de99f06e Mon Sep 17 00:00:00 2001 From: Yen Chi Hsuan Date: Sat, 18 Mar 2017 04:10:02 +0800 Subject: [PATCH 08/40] Python 3 compatibility for all tests (#4358) --- certbot-nginx/certbot_nginx/configurator.py | 3 +- certbot-nginx/certbot_nginx/obj.py | 17 ++++++--- certbot-nginx/certbot_nginx/parser.py | 2 +- .../certbot_nginx/tests/configurator_test.py | 7 ++-- .../certbot_nginx/tests/nginxparser_test.py | 4 +-- certbot-nginx/certbot_nginx/tests/obj_test.py | 2 +- .../certbot_nginx/tests/tls_sni_01_test.py | 19 +++++----- certbot-nginx/certbot_nginx/tests/util.py | 7 ++-- certbot-nginx/certbot_nginx/tls_sni_01.py | 7 ++-- letshelp-certbot/letshelp_certbot/apache.py | 6 ++-- .../letshelp_certbot/apache_test.py | 4 ++- tox.ini | 36 ------------------- 12 files changed, 49 insertions(+), 65 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 46ce18ab2..e62194d4f 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -9,6 +9,7 @@ import tempfile import time import OpenSSL +import six import zope.interface from acme import challenges @@ -263,7 +264,7 @@ class NginxConfigurator(common.Plugin): """ if not matches: return None - elif matches[0]['rank'] in xrange(2, 6): + elif matches[0]['rank'] in six.moves.range(2, 6): # Wildcard match - need to find the longest one rank = matches[0]['rank'] wildcards = [x for x in matches if x['rank'] == rank] diff --git a/certbot-nginx/certbot_nginx/obj.py b/certbot-nginx/certbot_nginx/obj.py index 29fa976f3..849cefe1f 100644 --- a/certbot-nginx/certbot_nginx/obj.py +++ b/certbot-nginx/certbot_nginx/obj.py @@ -1,6 +1,8 @@ """Module contains classes used by the Nginx Configurator.""" import re +import six + from certbot.plugins import common REDIRECT_DIRECTIVES = ['return', 'rewrite'] @@ -97,6 +99,11 @@ class Addr(common.Addr): def __repr__(self): return "Addr(" + self.__str__() + ")" + def __hash__(self): + # Python 3 requires explicit overridden for __hash__ + # See certbot-apache/certbot_apache/obj.py for more information + return super(Addr, self).__hash__() + def super_eq(self, other): """Check ip/port equality, with IPv6 support. """ @@ -147,13 +154,15 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods self.path = path def __str__(self): - addr_str = ", ".join(str(addr) for addr in self.addrs) + addr_str = ", ".join(str(addr) for addr in sorted(self.addrs, key=str)) + # names might be a set, and it has different representations in Python + # 2 and 3. Force it to be a list here for consistent outputs return ("file: %s\n" "addrs: %s\n" "names: %s\n" "ssl: %s\n" "enabled: %s" % (self.filep, addr_str, - self.names, self.ssl, self.enabled)) + list(self.names), self.ssl, self.enabled)) def __repr__(self): return "VirtualHost(" + self.__str__().replace("\n", ", ") + ")\n" @@ -161,7 +170,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods def __eq__(self, other): if isinstance(other, self.__class__): return (self.filep == other.filep and - list(self.addrs) == list(other.addrs) and + sorted(self.addrs, key=str) == sorted(other.addrs, key=str) and self.names == other.names and self.ssl == other.ssl and self.enabled == other.enabled and @@ -181,7 +190,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods def contains_list(self, test): """Determine if raw server block contains test list at top level """ - for i in xrange(0, len(self.raw) - len(test)): + for i in six.moves.range(0, len(self.raw) - len(test)): if self.raw[i:i + len(test)] == test: return True return False diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index eddc7b9b0..fd4ea4f11 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -342,7 +342,7 @@ class NginxParser(object): vhost.names = parsed_server['names'] vhost.raw = new_server except errors.MisconfigurationError as err: - raise errors.MisconfigurationError("Problem in %s: %s" % (filename, err.message)) + raise errors.MisconfigurationError("Problem in %s: %s" % (filename, str(err))) def _do_for_subarray(entry, condition, func, path=None): diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index cc36aa0de..d491d2a15 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -27,12 +27,13 @@ class NginxConfiguratorTest(util.NginxTest): super(NginxConfiguratorTest, self).setUp() self.config = util.get_nginx_configurator( - self.config_path, self.config_dir, self.work_dir) + self.config_path, self.config_dir, self.work_dir, self.logs_dir) def tearDown(self): shutil.rmtree(self.temp_dir) shutil.rmtree(self.config_dir) shutil.rmtree(self.work_dir) + shutil.rmtree(self.logs_dir) @mock.patch("certbot_nginx.configurator.util.exe_exists") def test_prepare_no_install(self, mock_exe_exists): @@ -261,13 +262,13 @@ class NginxConfiguratorTest(util.NginxTest): # Note: As more challenges are offered this will have to be expanded achall1 = achallenges.KeyAuthorizationAnnotatedChallenge( challb=messages.ChallengeBody( - chall=challenges.TLSSNI01(token="kNdwjwOeX0I_A8DXt9Msmg"), + chall=challenges.TLSSNI01(token=b"kNdwjwOeX0I_A8DXt9Msmg"), uri="https://ca.org/chall0_uri", status=messages.Status("pending"), ), domain="localhost", account_key=self.rsa512jwk) achall2 = achallenges.KeyAuthorizationAnnotatedChallenge( challb=messages.ChallengeBody( - chall=challenges.TLSSNI01(token="m8TdO1qik4JVFtgPPurJmg"), + chall=challenges.TLSSNI01(token=b"m8TdO1qik4JVFtgPPurJmg"), uri="https://ca.org/chall1_uri", status=messages.Status("pending"), ), domain="example.com", account_key=self.rsa512jwk) diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index e83b414cf..cf4bbde39 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -128,7 +128,7 @@ class TestRawNginxParser(unittest.TestCase): [['root', ' ', 'html'], ['index', ' ', 'index.html index.htm']]]]])) - with tempfile.TemporaryFile() as f: + with tempfile.TemporaryFile(mode='w+t') as f: dump(parsed, f) f.seek(0) parsed_new = load(f) @@ -138,7 +138,7 @@ class TestRawNginxParser(unittest.TestCase): with open(util.get_data_filename('minimalistic_comments.conf')) as handle: parsed = load(handle) - with tempfile.TemporaryFile() as f: + with tempfile.TemporaryFile(mode='w+t') as f: dump(parsed, f) f.seek(0) parsed_new = load(f) diff --git a/certbot-nginx/certbot_nginx/tests/obj_test.py b/certbot-nginx/certbot_nginx/tests/obj_test.py index b0a2d5ad8..069f860d6 100644 --- a/certbot-nginx/certbot_nginx/tests/obj_test.py +++ b/certbot-nginx/certbot_nginx/tests/obj_test.py @@ -158,7 +158,7 @@ class VirtualHostTest(unittest.TestCase): def test_str(self): stringified = '\n'.join(['file: filep', 'addrs: localhost', - "names: set(['localhost'])", 'ssl: False', + "names: ['localhost']", 'ssl: False', 'enabled: False']) self.assertEqual(stringified, str(self.vhost1)) diff --git a/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py b/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py index e7dacb400..7a2de44a2 100644 --- a/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py +++ b/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py @@ -3,6 +3,7 @@ import unittest import shutil import mock +import six from acme import challenges @@ -23,25 +24,25 @@ class TlsSniPerformTest(util.NginxTest): achalls = [ achallenges.KeyAuthorizationAnnotatedChallenge( challb=acme_util.chall_to_challb( - challenges.TLSSNI01(token="kNdwjwOeX0I_A8DXt9Msmg"), "pending"), + challenges.TLSSNI01(token=b"kNdwjwOeX0I_A8DXt9Msmg"), "pending"), domain="www.example.com", account_key=account_key), achallenges.KeyAuthorizationAnnotatedChallenge( challb=acme_util.chall_to_challb( challenges.TLSSNI01( - token="\xba\xa9\xda? Date: Fri, 17 Mar 2017 13:13:45 -0700 Subject: [PATCH 09/40] Improve plugin-writing docs. (#4329) Move "Writing your own plugin" under Code components and layout, with the other plugin docs. Include instructions on how to install a plugin into a virtualenv and how to check for its presence. Document that users can install third-party plugins systemwide, but not with certbot-auto. Remove obsolete information from Authenticators section and make the section more informative. Remove IDisplay sub-section since it repeats information in the main "Plugin architecture" section. --- docs/contributing.rst | 71 ++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/docs/contributing.rst b/docs/contributing.rst index 5cdf86147..9c7f0636f 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -145,13 +145,15 @@ different webservers, other TLS servers, and operating systems. The interfaces available for plugins to implement are defined in `interfaces.py`_ and `plugins/common.py`_. -The most common kind of plugin is a "Configurator", which is likely to -implement the `~certbot.interfaces.IAuthenticator` and -`~certbot.interfaces.IInstaller` interfaces (though some -Configurators may implement just one of those). +The main two plugin interfaces are `~certbot.interfaces.IAuthenticator`, which +implements various ways of proving domain control to a certificate authority, +and `~certbot.interfaces.IInstaller`, which configures a server to use a +certificate once it is issued. Some plugins, like the built-in Apache and Nginx +plugins, implement both interfaces and perform both tasks. Others, like the +built-in Standalone authenticator, implement just one interface. There are also `~certbot.interfaces.IDisplay` plugins, -which implement bindings to alternative UI libraries. +which can change how prompts are displayed to a user. .. _interfaces.py: https://github.com/certbot/certbot/blob/master/certbot/interfaces.py .. _plugins/common.py: https://github.com/certbot/certbot/blob/master/certbot/plugins/common.py#L34 @@ -160,27 +162,20 @@ which implement bindings to alternative UI libraries. Authenticators -------------- -Authenticators are plugins designed to prove that this client deserves a -certificate for some domain name by solving challenges received from -the ACME server. From the protocol, there are essentially two -different types of challenges. Challenges that must be solved by -individual plugins in order to satisfy domain validation (subclasses -of `~.DVChallenge`, i.e. `~.challenges.TLSSNI01`, -`~.challenges.HTTP01`, `~.challenges.DNS`) and continuity specific -challenges (subclasses of `~.ContinuityChallenge`, -i.e. `~.challenges.RecoveryToken`, `~.challenges.RecoveryContact`, -`~.challenges.ProofOfPossession`). Continuity challenges are -always handled by the `~.ContinuityAuthenticator`, while plugins are -expected to handle `~.DVChallenge` types. -Right now, we have two authenticator plugins, the `~.ApacheConfigurator` -and the `~.StandaloneAuthenticator`. The Standalone and Apache -authenticators only solve the `~.challenges.TLSSNI01` challenge currently. -(You can set which challenges your authenticator can handle through the -:meth:`~.IAuthenticator.get_chall_pref`. +Authenticators are plugins that prove control of a domain name by solving a +challenge provided by the ACME server. ACME currently defines three types of +challenges: HTTP, TLS-SNI, and DNS, represented by classes in `acme.challenges`. +An authenticator plugin should implement support for at least one challenge type. -(FYI: We also have a partial implementation for a `~.DNSAuthenticator` -in a separate branch). +An Authenticator indicates which challenges it supports by implementing +get_chall_pref(domain) to return a sorted list of challenge types in preference +order. +An Authenticator must also implement `perform(achalls)`, which "performs" a list +of challenges by, for instance, provisioning a file on an HTTP server, or +setting a TXT record in DNS. Once all challenges have succeeded or failed, +Certbot will call the plugin's `cleanup(achalls)` method to remove any files or +DNS records that were needed only during authentication. Installer --------- @@ -218,16 +213,10 @@ Augeas may still find the `~.Reverter` class helpful in handling configuration checkpoints and rollback. -Display -~~~~~~~ - -We currently only offer a "text" mode for displays. Display plugins -implement the `~certbot.interfaces.IDisplay` interface. - .. _dev-plugin: Writing your own plugin -======================= +~~~~~~~~~~~~~~~~~~~~~~~ Certbot client supports dynamic discovery of plugins through the `setuptools entry points`_. This way you can, for example, create a @@ -236,6 +225,26 @@ the `~certbot.interfaces.IInstaller` without having to merge it with the core upstream source code. An example is provided in ``examples/plugins/`` directory. +While developing, you can install your plugin into a Certbot development +virtualenv like this: + +.. code-block:: shell + . venv/bin/activate + . tests/integration/_common.sh + pip install -e examples/plugins/ + certbot_test plugins + +Your plugin should show up in the output of the last command. If not, +it was not installed properly. + +Once you've finished your plugin and published it, you can have your +users install it system-wide with `pip install`. Note that this will +only work for users who have Certbot installed from OS packages or via +pip. Users who run `certbot-auto` are currently unable to use third-party +plugins. It's technically possible to install third-party plugins into +the virtualenv used by `certbot-auto`, but they will be wiped away when +`certbot-auto` upgrades. + .. warning:: Please be aware though that as this client is still in a developer-preview stage, the API may undergo a few changes. If you believe the plugin will be beneficial to the community, please From b23a1377e04a7f53269bb36e008a3cf495d2853f Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 17 Mar 2017 13:17:08 -0700 Subject: [PATCH 10/40] Clarify documentation for low-memory machines. (#4305) * Clarify documentation for low-memory machines. * Restore py26/py27 requirement. --- README.rst | 14 +------------- docs/install.rst | 14 +++++++++----- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/README.rst b/README.rst index ab12562df..e44a61021 100644 --- a/README.rst +++ b/README.rst @@ -129,19 +129,7 @@ email to client-dev+subscribe@letsencrypt.org) System Requirements =================== -The Let's Encrypt Client presently only runs on Unix-ish OSes that include -Python 2.6 or 2.7; Python 3.x support will hopefully be added in the future. The -client requires root access in order to write to ``/etc/letsencrypt``, -``/var/log/letsencrypt``, ``/var/lib/letsencrypt``; to bind to ports 80 and 443 -(if you use the ``standalone`` plugin) and to read and modify webserver -configurations (if you use the ``apache`` or ``nginx`` plugins). If none of -these apply to you, it is theoretically possible to run without root privileges, -but for most users who want to avoid running an ACME client as root, either -`letsencrypt-nosudo `_ or -`simp_le `_ are more appropriate choices. - -The Apache plugin currently requires a Debian-based OS with augeas version -1.0; this includes Ubuntu 12.04+ and Debian 7+. +See https://certbot.eff.org/docs/install.html#system-requirements. .. Do not modify this comment unless you know what you're doing. tag:intro-end diff --git a/docs/install.rst b/docs/install.rst index 548c8f2a3..e1ec06f16 100644 --- a/docs/install.rst +++ b/docs/install.rst @@ -22,9 +22,8 @@ your system. System Requirements =================== -The Let's Encrypt Client presently only runs on Unix-ish OSes that include -Python 2.6 or 2.7; Python 3.x support will hopefully be added in the future. The -client requires root access in order to write to ``/etc/letsencrypt``, +Certbot currently requires Python 2.6 or 2.7. By default, it requires root +access in order to write to ``/etc/letsencrypt``, ``/var/log/letsencrypt``, ``/var/lib/letsencrypt``; to bind to ports 80 and 443 (if you use the ``standalone`` plugin) and to read and modify webserver configurations (if you use the ``apache`` or ``nginx`` plugins). If none of @@ -33,11 +32,16 @@ but for most users who want to avoid running an ACME client as root, either `letsencrypt-nosudo `_ or `simp_le `_ are more appropriate choices. -The Apache plugin currently requires OS with augeas version 1.0; currently `it +The Apache plugin currently requires an OS with augeas version 1.0; currently `it supports `_ modern OSes based on Debian, Fedora, SUSE, Gentoo and Darwin. +Installing with ``certbot-auto`` requires 512MB of RAM in order to build some +of the dependencies. Installing from pre-built OS packages avoids this +requirement. You can also temporarily set a swap file. See "Problems with +Python virtual environment" below for details. + Alternate installation methods ================================ @@ -76,7 +80,7 @@ For full command line help, you can type:: Problems with Python virtual environment ---------------------------------------- -On a low memory system such as VPS with only 256MB of RAM, the required dependencies of Certbot will failed to build. +On a low memory system such as VPS with less than 512MB of RAM, the required dependencies of Certbot will failed to build. This can be identified if the pip outputs contains something like ``internal compiler error: Killed (program cc1)``. You can workaround this restriction by creating a temporary swapfile:: From fd789b4e4b9f58970abf10fed399e4e786ba3bb4 Mon Sep 17 00:00:00 2001 From: Piotr Kasprzyk Date: Fri, 17 Mar 2017 22:11:52 +0100 Subject: [PATCH 11/40] Fix choose, remove spaces (#4364) --- docs/using.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/using.rst b/docs/using.rst index 7c17796e7..549a3479c 100644 --- a/docs/using.rst +++ b/docs/using.rst @@ -175,15 +175,15 @@ the UI, you can use the plugin to obtain a cert by specifying to copy and paste commands into another terminal session, which may be on a different computer. -The manual plugin can use either the ``http`` or the ``dns`` challenge. You -can use the ``--preferred-challenges`` option to chose the challenge of your +The manual plugin can use either the ``http`` or the ``dns`` challenge. You +can use the ``--preferred-challenges`` option to choose the challenge of your preference. -The ``http`` challenge will ask you to place a file with a specific name and -specific content in the ``/.well-known/acme-challenge/`` directory directly -in the top-level directory (“web root”) containing the files served by your +The ``http`` challenge will ask you to place a file with a specific name and +specific content in the ``/.well-known/acme-challenge/`` directory directly +in the top-level directory (“web root”) containing the files served by your webserver. In essence it's the same as the webroot_ plugin, but not automated. -When using the ``dns`` plugin, ``certbot`` will ask you to place a TXT DNS -record with specific contents under the domain name consisting of the hostname +When using the ``dns`` plugin, ``certbot`` will ask you to place a TXT DNS +record with specific contents under the domain name consisting of the hostname for which you want a certificate issued, prepended by ``_acme-challenge``. For example, for the domain ``example.com``, a zone file entry would look like: From b81f0296140b4b04bd5e0cda640fff2768596d74 Mon Sep 17 00:00:00 2001 From: Osiris Inferi Date: Sat, 18 Mar 2017 00:51:59 +0100 Subject: [PATCH 12/40] Add Gentoo to list of official packages --- docs/packaging.rst | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/docs/packaging.rst b/docs/packaging.rst index 1a1b83f50..7e4781f65 100644 --- a/docs/packaging.rst +++ b/docs/packaging.rst @@ -44,6 +44,7 @@ Arch ---- From our official releases: + - https://www.archlinux.org/packages/community/any/python2-acme - https://www.archlinux.org/packages/community/any/certbot - https://www.archlinux.org/packages/community/any/certbot-apache @@ -55,9 +56,9 @@ From ``master``: https://aur.archlinux.org/packages/certbot-git Debian (and its derivatives, including Ubuntu) ------ -https://packages.debian.org/sid/certbot -https://packages.debian.org/sid/python-certbot -https://packages.debian.org/sid/python-certbot-apache +- https://packages.debian.org/sid/certbot +- https://packages.debian.org/sid/python-certbot +- https://packages.debian.org/sid/python-certbot-apache Fedora ------ @@ -71,7 +72,17 @@ In Fedora 23+. FreeBSD ------- -https://svnweb.freebsd.org/ports/head/security/py-certbot/ +- https://svnweb.freebsd.org/ports/head/security/py-certbot/ + +Gentoo +------ + +Currently, all ``certbot`` related packages are in the testing branch: + +- https://packages.gentoo.org/packages/app-crypt/certbot +- https://packages.gentoo.org/packages/app-crypt/certbot-apache +- https://packages.gentoo.org/packages/app-crypt/certbot-nginx +- https://packages.gentoo.org/packages/app-crypt/acme GNU Guix -------- From c439057efab57d4b02856839e90d1404eefcb91c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Sat, 18 Mar 2017 13:25:02 -0700 Subject: [PATCH 13/40] install python3-dev for python3 tests in docker (#4381) --- Dockerfile-dev | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile-dev b/Dockerfile-dev index 2a89b2ff5..c09e04ac3 100644 --- a/Dockerfile-dev +++ b/Dockerfile-dev @@ -20,10 +20,10 @@ WORKDIR /opt/certbot/src # If doesn't exist, it is created along with all missing # directories in its path. -# TODO: Install non-default Python versions for tox. # TODO: Install Apache/Nginx for plugin development. COPY letsencrypt-auto-source/letsencrypt-auto /opt/certbot/src/letsencrypt-auto-source/letsencrypt-auto RUN /opt/certbot/src/letsencrypt-auto-source/letsencrypt-auto --os-packages-only && \ + apt-get install python3-dev -y && \ apt-get clean && \ rm -rf /var/lib/apt/lists/* \ /tmp/* \ From 6f979a48084674dc773098709f6f43230277372a Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Sat, 18 Mar 2017 13:40:01 -0700 Subject: [PATCH 14/40] upgrade pip and setuptools before installing packages (#4378) --- Dockerfile-dev | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Dockerfile-dev b/Dockerfile-dev index c09e04ac3..607aa3441 100644 --- a/Dockerfile-dev +++ b/Dockerfile-dev @@ -51,6 +51,8 @@ COPY certbot-compatibility-test /opt/certbot/src/certbot-compatibility-test/ COPY tests /opt/certbot/src/tests/ RUN virtualenv --no-site-packages -p python2 /opt/certbot/venv && \ + /opt/certbot/venv/bin/pip install -U pip && \ + /opt/certbot/venv/bin/pip install -U setuptools && \ /opt/certbot/venv/bin/pip install \ -e /opt/certbot/src/acme \ -e /opt/certbot/src \ @@ -58,8 +60,7 @@ RUN virtualenv --no-site-packages -p python2 /opt/certbot/venv && \ -e /opt/certbot/src/certbot-nginx \ -e /opt/certbot/src/letshelp-certbot \ -e /opt/certbot/src/certbot-compatibility-test \ - -e /opt/certbot/src[dev,docs] && \ - /opt/certbot/venv/bin/pip install -U setuptools + -e /opt/certbot/src[dev,docs] # install in editable mode (-e) to save space: it's not possible to # "rm -rf /opt/certbot/src" (it's stays in the underlaying image); From e034b50363a75d61d1a7e7add2b8aca52684acd1 Mon Sep 17 00:00:00 2001 From: Daniel Huang Date: Sat, 18 Mar 2017 13:42:54 -0700 Subject: [PATCH 15/40] Don't save keys/csr on dry run (#4380) * Don't save keys/csr on dry run (#2495) * Replace assertIsNone for py26 * Fix config defaults for compat tests --- certbot-nginx/certbot_nginx/tests/util.py | 1 + certbot/constants.py | 1 + certbot/crypto_util.py | 30 ++++++++++-------- certbot/tests/crypto_util_test.py | 37 +++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15 deletions(-) diff --git a/certbot-nginx/certbot_nginx/tests/util.py b/certbot-nginx/certbot_nginx/tests/util.py index 259dc1f10..4ab95374e 100644 --- a/certbot-nginx/certbot_nginx/tests/util.py +++ b/certbot-nginx/certbot_nginx/tests/util.py @@ -70,6 +70,7 @@ def get_nginx_configurator( in_progress_dir=os.path.join(backups, "IN_PROGRESS"), server="https://acme-server.org:443/new", tls_sni_01_port=5001, + dry_run=False, ), name="nginx", version=version) diff --git a/certbot/constants.py b/certbot/constants.py index b286ca26a..c85992fc1 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -18,6 +18,7 @@ CLI_DEFAULTS = dict( os.path.join(os.environ.get("XDG_CONFIG_HOME", "~/.config"), "letsencrypt", "cli.ini"), ], + dry_run=False, verbose_count=-int(logging.INFO / 10), server="https://acme-v01.api.letsencrypt.org/directory", rsa_key_size=2048, diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 65e3de345..1ad76d503 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -53,12 +53,15 @@ def init_save_key(key_size, key_dir, keyname="key-certbot.pem"): # Save file util.make_or_verify_dir(key_dir, 0o700, os.geteuid(), config.strict_permissions) - key_f, key_path = util.unique_file( - os.path.join(key_dir, keyname), 0o600, "wb") - with key_f: - key_f.write(key_pem) - - logger.info("Generating key (%d bits): %s", key_size, key_path) + if config.dry_run: + key_path = None + logger.info("Generating key (%d bits), not saving to file", key_size) + else: + key_f, key_path = util.unique_file( + os.path.join(key_dir, keyname), 0o600, "wb") + with key_f: + key_f.write(key_pem) + logger.info("Generating key (%d bits): %s", key_size, key_path) return util.Key(key_path, key_pem) @@ -85,12 +88,15 @@ def init_save_csr(privkey, names, path, csrname="csr-certbot.pem"): # Save CSR util.make_or_verify_dir(path, 0o755, os.geteuid(), config.strict_permissions) - csr_f, csr_filename = util.unique_file( - os.path.join(path, csrname), 0o644, "wb") - csr_f.write(csr_pem) - csr_f.close() - - logger.info("Creating CSR: %s", csr_filename) + if config.dry_run: + csr_filename = None + logger.info("Creating CSR: not saving to file") + else: + csr_f, csr_filename = util.unique_file( + os.path.join(path, csrname), 0o644, "wb") + with csr_f: + csr_f.write(csr_pem) + logger.info("Creating CSR: %s", csr_filename) return util.CSR(csr_filename, csr_der, "der") diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index 946e772c1..a832d0494 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -1,5 +1,6 @@ """Tests for certbot.crypto_util.""" import logging +import os import shutil import tempfile import unittest @@ -26,7 +27,8 @@ class InitSaveKeyTest(unittest.TestCase): def setUp(self): logging.disable(logging.CRITICAL) zope.component.provideUtility( - mock.Mock(strict_permissions=True), interfaces.IConfig) + mock.Mock(strict_permissions=True, dry_run=False), + interfaces.IConfig) self.key_dir = tempfile.mkdtemp('key_dir') def tearDown(self): @@ -44,6 +46,17 @@ class InitSaveKeyTest(unittest.TestCase): key = self._call(1024, self.key_dir) self.assertEqual(key.pem, b'key_pem') self.assertTrue('key-certbot.pem' in key.file) + self.assertTrue(os.path.exists(os.path.join(self.key_dir, key.file))) + + @mock.patch('certbot.crypto_util.make_key') + def test_success_dry_run(self, mock_make): + zope.component.provideUtility( + mock.Mock(strict_permissions=True, dry_run=True), + interfaces.IConfig) + mock_make.return_value = b'key_pem' + key = self._call(1024, self.key_dir) + self.assertEqual(key.pem, b'key_pem') + self.assertTrue(key.file is None) @mock.patch('certbot.crypto_util.make_key') def test_key_failure(self, mock_make): @@ -56,7 +69,8 @@ class InitSaveCSRTest(unittest.TestCase): def setUp(self): zope.component.provideUtility( - mock.Mock(strict_permissions=True), interfaces.IConfig) + mock.Mock(strict_permissions=True, dry_run=False), + interfaces.IConfig) self.csr_dir = tempfile.mkdtemp('csr_dir') def tearDown(self): @@ -64,7 +78,7 @@ class InitSaveCSRTest(unittest.TestCase): @mock.patch('certbot.crypto_util.make_csr') @mock.patch('certbot.crypto_util.util.make_or_verify_dir') - def test_it(self, unused_mock_verify, mock_csr): + def test_success(self, unused_mock_verify, mock_csr): from certbot.crypto_util import init_save_csr mock_csr.return_value = (b'csr_pem', b'csr_der') @@ -76,6 +90,23 @@ class InitSaveCSRTest(unittest.TestCase): self.assertEqual(csr.data, b'csr_der') self.assertTrue('csr-certbot.pem' in csr.file) + @mock.patch('certbot.crypto_util.make_csr') + @mock.patch('certbot.crypto_util.util.make_or_verify_dir') + def test_success_dry_run(self, unused_mock_verify, mock_csr): + from certbot.crypto_util import init_save_csr + + zope.component.provideUtility( + mock.Mock(strict_permissions=True, dry_run=True), + interfaces.IConfig) + mock_csr.return_value = (b'csr_pem', b'csr_der') + + csr = init_save_csr( + mock.Mock(pem='dummy_key'), 'example.com', self.csr_dir, + 'csr-certbot.pem') + + self.assertEqual(csr.data, b'csr_der') + self.assertTrue(csr.file is None) + class MakeCSRTest(unittest.TestCase): """Tests for certbot.crypto_util.make_csr.""" From d54d3eba78cc13bc491507e48388cfeddd14d584 Mon Sep 17 00:00:00 2001 From: Daniel Huang Date: Sat, 18 Mar 2017 17:04:16 -0700 Subject: [PATCH 16/40] Retry fetch chain errors (#4196) (#4383) * Retry fetch chain errors (#4196) * Trying to avoid confusing pylint * Pylint disable * Typo certz->certr * Move pylint disable, log when fetch chain fails --- certbot/client.py | 30 ++++++++++++++++++++++--- certbot/main.py | 2 +- certbot/tests/client_test.py | 43 ++++++++++++++++++++++++++++++++++-- 3 files changed, 69 insertions(+), 6 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 8933ef27e..bd1971371 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -8,6 +8,7 @@ import OpenSSL import zope.component from acme import client as acme_client +from acme import errors as acme_errors from acme import jose from acme import messages @@ -242,7 +243,28 @@ class Client(object): jose.ComparableX509( OpenSSL.crypto.load_certificate_request(typ, csr.data)), authzr) - return certr, self.acme.fetch_chain(certr) + + notify = zope.component.getUtility(interfaces.IDisplay).notification + retries = 0 + chain = None + + while retries <= 1: + if retries: + notify('Failed to fetch chain, please check your network ' + 'and continue', pause=True) + try: + chain = self.acme.fetch_chain(certr) + break + except acme_errors.Error: + logger.debug('Failed to fetch chain', exc_info=True) + retries += 1 + + if chain is None: + raise acme_errors.Error( + 'Failed to fetch chain. You should not deploy the generated ' + 'certificate, please rerun the command for a new one.') + + return certr, chain def obtain_certificate(self, domains): """Obtains a certificate from the ACME server. @@ -269,10 +291,12 @@ class Client(object): key = crypto_util.init_save_key( self.config.rsa_key_size, self.config.key_dir) csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) + certr, chain = self.obtain_certificate_from_csr( + domains, csr, authzr=authzr) - return (self.obtain_certificate_from_csr(domains, csr, authzr=authzr) - + (key, csr)) + return certr, chain, key, csr + # pylint: disable=no-member def obtain_and_enroll_certificate(self, domains, certname): """Obtain and enroll certificate. diff --git a/certbot/main.py b/certbot/main.py index b0689faa2..ab2204428 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -487,7 +487,7 @@ def install(config, plugins): try: installer, _ = plug_sel.choose_configurator_plugins(config, plugins, "install") except errors.PluginSelectionError as e: - return e.message + return str(e) domains, _ = _find_domains_or_certname(config, installer) le_client = _init_le_client(config, authenticator=None, installer=installer) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index cc3bb098d..c61f1fa4e 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -7,6 +7,7 @@ import unittest import OpenSSL import mock +from acme import errors as acme_errors from acme import jose from certbot import account @@ -170,7 +171,9 @@ class ClientTest(ClientTestCommon): self.acme.fetch_chain.assert_called_once_with(mock.sentinel.certr) @mock.patch("certbot.client.logger") - def test_obtain_certificate_from_csr(self, mock_logger): + @test_util.patch_get_utility() + def test_obtain_certificate_from_csr(self, unused_mock_get_utility, + mock_logger): self._mock_obtain_certificate() test_csr = util.CSR(form="der", file=None, data=CSR_SAN) auth_handler = self.client.auth_handler @@ -203,8 +206,44 @@ class ClientTest(ClientTestCommon): test_csr) mock_logger.warning.assert_called_once_with(mock.ANY) + @test_util.patch_get_utility() + def test_obtain_certificate_from_csr_retry_succeeded( + self, mock_get_utility): + self._mock_obtain_certificate() + self.acme.fetch_chain.side_effect = [acme_errors.Error, + mock.sentinel.chain] + test_csr = util.CSR(form="der", file=None, data=CSR_SAN) + auth_handler = self.client.auth_handler + + authzr = auth_handler.get_authorizations(self.eg_domains, False) + self.assertEqual( + (mock.sentinel.certr, mock.sentinel.chain), + self.client.obtain_certificate_from_csr( + self.eg_domains, + test_csr, + authzr=authzr)) + self.assertEqual(1, mock_get_utility().notification.call_count) + + @test_util.patch_get_utility() + def test_obtain_certificate_from_csr_retry_failed(self, mock_get_utility): + self._mock_obtain_certificate() + self.acme.fetch_chain.side_effect = acme_errors.Error + test_csr = util.CSR(form="der", file=None, data=CSR_SAN) + auth_handler = self.client.auth_handler + + authzr = auth_handler.get_authorizations(self.eg_domains, False) + self.assertRaises( + acme_errors.Error, + self.client.obtain_certificate_from_csr, + self.eg_domains, + test_csr, + authzr=authzr) + self.assertEqual(1, mock_get_utility().notification.call_count) + @mock.patch("certbot.client.crypto_util") - def test_obtain_certificate(self, mock_crypto_util): + @test_util.patch_get_utility() + def test_obtain_certificate(self, unused_mock_get_utility, + mock_crypto_util): self._mock_obtain_certificate() csr = util.CSR(form="der", file=None, data=CSR_SAN) From 97db9e646afd17e5e7d15b9afdf895d65510f00c Mon Sep 17 00:00:00 2001 From: Yen Chi Hsuan Date: Sun, 19 Mar 2017 09:06:32 +0800 Subject: [PATCH 17/40] Fix _get_runtime_cfg on Python 3 (#4262) --- certbot-apache/certbot_apache/parser.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index 275a01e7f..67984a26c 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/certbot-apache/certbot_apache/parser.py @@ -136,7 +136,8 @@ class ApacheParser(object): proc = subprocess.Popen( constants.os_constant("define_cmd"), stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, + universal_newlines=True) stdout, stderr = proc.communicate() except (OSError, ValueError): From b9121a8a37456f4504b7bf05a69180973487a885 Mon Sep 17 00:00:00 2001 From: Daniel Huang Date: Sat, 18 Mar 2017 21:14:53 -0400 Subject: [PATCH 18/40] Do not output apache version when deploying cert (#4023) --- certbot-apache/certbot_apache/configurator.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index cdfc01626..9f28eaad0 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -254,9 +254,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): raise errors.PluginError( "Unable to find cert and/or key directives") - logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) - logger.debug("Apache version is %s", - ".".join(str(i) for i in self.version)) + logger.info("Deploying Certificate for %s to VirtualHost %s", domain, vhost.filep) if self.version < (2, 4, 8) or (chain_path and not fullchain_path): # install SSLCertificateFile, SSLCertificateKeyFile, From 679887f6913072ce3280bdf5862d5dabc5c039f8 Mon Sep 17 00:00:00 2001 From: Daniel Huang Date: Sat, 18 Mar 2017 18:33:29 -0700 Subject: [PATCH 19/40] Add --debug-challenges flag (#1684) (#4385) * Add --debug-challenges flag (#1684) * Specify None as topic for --debug-challenges --- certbot/auth_handler.py | 5 +++++ certbot/cli.py | 5 +++++ certbot/constants.py | 1 + certbot/tests/auth_handler_test.py | 22 ++++++++++++++++++++++ 4 files changed, 33 insertions(+) diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 53346a77c..a1f23a895 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -66,11 +66,16 @@ class AuthHandler(object): self.authzr[domain] = self.acme.request_domain_challenges(domain) self._choose_challenges(domains) + config = zope.component.getUtility(interfaces.IConfig) + notify = zope.component.getUtility(interfaces.IDisplay).notification # While there are still challenges remaining... while self.achalls: resp = self._solve_challenges() logger.info("Waiting for verification...") + if config.debug_challenges: + notify('Challenges loaded. Press continue to submit to CA. ' + 'Pass "-v" for more info about challenges.', pause=True) # Send all Responses - this modifies achalls self._respond(resp, best_effort) diff --git a/certbot/cli.py b/certbot/cli.py index c0af490d2..4fabd9a50 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -955,6 +955,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "testing", "--debug", action="store_true", help="Show tracebacks in case of errors, and allow certbot-auto " "execution on experimental platforms") + helpful.add( + [None, "certonly", "renew", "run"], "--debug-challenges", action="store_true", + default=flag_default("debug_challenges"), + help="After setting up challenges, wait for user input before " + "submitting to CA") helpful.add( "testing", "--no-verify-ssl", action="store_true", help=config_help("no_verify_ssl"), diff --git a/certbot/constants.py b/certbot/constants.py index c85992fc1..382b0afb3 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -33,6 +33,7 @@ CLI_DEFAULTS = dict( auth_cert_path="./cert.pem", auth_chain_path="./chain.pem", strict_permissions=False, + debug_challenges=False, ) STAGING_URI = "https://acme-staging.api.letsencrypt.org/directory" diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 9d22843db..32c4c0d3b 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -5,6 +5,7 @@ import unittest import mock import six +import zope.component from acme import challenges from acme import client as acme_client @@ -12,6 +13,7 @@ from acme import messages from certbot import achallenges from certbot import errors +from certbot import interfaces from certbot import util from certbot.tests import acme_util @@ -65,6 +67,12 @@ class GetAuthorizationsTest(unittest.TestCase): def setUp(self): from certbot.auth_handler import AuthHandler + self.mock_display = mock.Mock() + zope.component.provideUtility( + self.mock_display, interfaces.IDisplay) + zope.component.provideUtility( + mock.Mock(debug_challenges=False), interfaces.IConfig) + self.mock_auth = mock.MagicMock(name="ApacheConfigurator") self.mock_auth.get_chall_pref.return_value = [challenges.TLSSNI01] @@ -157,6 +165,20 @@ class GetAuthorizationsTest(unittest.TestCase): self.assertEqual(len(authzr), 3) + @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") + def test_debug_challenges(self, mock_poll): + zope.component.provideUtility( + mock.Mock(debug_challenges=True), interfaces.IConfig) + self.mock_net.request_domain_challenges.side_effect = functools.partial( + gen_dom_authzr, challs=acme_util.CHALLENGES) + + mock_poll.side_effect = self._validate_all + + self.handler.get_authorizations(["0"]) + + self.assertEqual(self.mock_net.answer_challenge.call_count, 1) + self.assertEqual(self.mock_display.notification.call_count, 1) + def test_perform_failure(self): self.mock_net.request_domain_challenges.side_effect = functools.partial( gen_dom_authzr, challs=acme_util.CHALLENGES) From 1e3678398611e7b8c103bbbd96f526007fc684a3 Mon Sep 17 00:00:00 2001 From: Daniel Huang Date: Sat, 18 Mar 2017 21:37:37 -0400 Subject: [PATCH 20/40] Still include apache version in debug logging --- certbot-apache/certbot_apache/configurator.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 9f28eaad0..39d25619d 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -174,6 +174,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Set Version if self.version is None: self.version = self.get_version() + logger.debug('Apache version is %s', + '.'.join(str(i) for i in self.version)) if self.version < (2, 2): raise errors.NotSupportedError( "Apache Version %s not supported.", str(self.version)) From 8011fb2879659008528e8b6ba5c04ac3de1e63a0 Mon Sep 17 00:00:00 2001 From: dokazaki Date: Sat, 18 Mar 2017 19:10:10 -0700 Subject: [PATCH 21/40] Add mypy (#4386) * Initial configuration of mypy in box, correction of base mypy errors. * Move mypy install to toe * Add pylint comments for typing imports. * Remove typing module for Python 2.6 compatibility. --- acme/acme/challenges.py | 6 +++--- acme/acme/client.py | 2 +- acme/acme/crypto_util.py | 2 +- acme/acme/crypto_util_test.py | 2 +- acme/acme/jose/jwa.py | 10 +++++----- acme/acme/jose/jwk.py | 8 ++++---- acme/acme/jose/jws.py | 8 ++++---- acme/acme/jose/util.py | 4 ++-- acme/acme/messages.py | 8 ++++---- acme/acme/standalone.py | 4 ++-- acme/acme/standalone_test.py | 2 +- .../certbot_compatibility_test/interfaces.py | 10 +++++----- certbot/cli.py | 2 +- certbot/display/completer.py | 2 +- certbot/hooks.py | 8 ++++++-- certbot/interfaces.py | 16 ++++++++-------- certbot/plugins/disco.py | 2 +- certbot/reporter.py | 2 +- certbot/tests/util_test.py | 2 +- tox.ini | 7 +++++++ 20 files changed, 59 insertions(+), 48 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index ac4e3d60a..14641af10 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -5,7 +5,7 @@ import hashlib import logging import socket -from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives import hashes # type: ignore import OpenSSL import requests @@ -23,7 +23,7 @@ logger = logging.getLogger(__name__) class Challenge(jose.TypedJSONObjectWithFields): # _fields_to_partial_json | pylint: disable=abstract-method """ACME challenge.""" - TYPES = {} + TYPES = {} # type: dict @classmethod def from_json(cls, jobj): @@ -37,7 +37,7 @@ class Challenge(jose.TypedJSONObjectWithFields): class ChallengeResponse(jose.TypedJSONObjectWithFields): # _fields_to_partial_json | pylint: disable=abstract-method """ACME challenge response.""" - TYPES = {} + TYPES = {} # type: dict resource_type = 'challenge' resource = fields.Resource(resource_type) diff --git a/acme/acme/client.py b/acme/acme/client.py index d6166960d..40291e115 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -28,7 +28,7 @@ logger = logging.getLogger(__name__) # https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning if sys.version_info < (2, 7, 9): # pragma: no cover try: - requests.packages.urllib3.contrib.pyopenssl.inject_into_urllib3() + requests.packages.urllib3.contrib.pyopenssl.inject_into_urllib3() # type: ignore except AttributeError: import urllib3.contrib.pyopenssl # pylint: disable=import-error urllib3.contrib.pyopenssl.inject_into_urllib3() diff --git a/acme/acme/crypto_util.py b/acme/acme/crypto_util.py index 266f2c0c7..6a33b3e52 100644 --- a/acme/acme/crypto_util.py +++ b/acme/acme/crypto_util.py @@ -23,7 +23,7 @@ logger = logging.getLogger(__name__) # https://www.openssl.org/docs/ssl/SSLv23_method.html). _serve_sni # should be changed to use "set_options" to disable SSLv2 and SSLv3, # in case it's used for things other than probing/serving! -_DEFAULT_TLSSNI01_SSL_METHOD = OpenSSL.SSL.SSLv23_METHOD +_DEFAULT_TLSSNI01_SSL_METHOD = OpenSSL.SSL.SSLv23_METHOD # type: ignore class SSLSocket(object): # pylint: disable=too-few-public-methods diff --git a/acme/acme/crypto_util_test.py b/acme/acme/crypto_util_test.py index ebb4010a6..9cf1f7deb 100644 --- a/acme/acme/crypto_util_test.py +++ b/acme/acme/crypto_util_test.py @@ -6,7 +6,7 @@ import time import unittest import six -from six.moves import socketserver # pylint: disable=import-error +from six.moves import socketserver #type: ignore # pylint: disable=import-error import OpenSSL diff --git a/acme/acme/jose/jwa.py b/acme/acme/jose/jwa.py index 1853e0107..9b682ecab 100644 --- a/acme/acme/jose/jwa.py +++ b/acme/acme/jose/jwa.py @@ -9,9 +9,9 @@ import logging import cryptography.exceptions from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import hashes -from cryptography.hazmat.primitives import hmac -from cryptography.hazmat.primitives.asymmetric import padding +from cryptography.hazmat.primitives import hashes # type: ignore +from cryptography.hazmat.primitives import hmac # type: ignore +from cryptography.hazmat.primitives.asymmetric import padding # type: ignore from acme.jose import errors from acme.jose import interfaces @@ -28,9 +28,9 @@ class JWA(interfaces.JSONDeSerializable): # pylint: disable=abstract-method """JSON Web Algorithm.""" -class JWASignature(JWA, collections.Hashable): +class JWASignature(JWA, collections.Hashable): # type: ignore """JSON Web Signature Algorithm.""" - SIGNATURES = {} + SIGNATURES = {} # type: dict def __init__(self, name): self.name = name diff --git a/acme/acme/jose/jwk.py b/acme/acme/jose/jwk.py index 5b6965c4d..54423f670 100644 --- a/acme/acme/jose/jwk.py +++ b/acme/acme/jose/jwk.py @@ -6,9 +6,9 @@ import logging import cryptography.exceptions from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives import hashes # type: ignore from cryptography.hazmat.primitives import serialization -from cryptography.hazmat.primitives.asymmetric import ec +from cryptography.hazmat.primitives.asymmetric import ec # type: ignore from cryptography.hazmat.primitives.asymmetric import rsa import six @@ -25,8 +25,8 @@ class JWK(json_util.TypedJSONObjectWithFields): # pylint: disable=too-few-public-methods """JSON Web Key.""" type_field_name = 'kty' - TYPES = {} - cryptography_key_types = () + TYPES = {} # type: dict + cryptography_key_types = () # type: tuple """Subclasses should override.""" required = NotImplemented diff --git a/acme/acme/jose/jws.py b/acme/acme/jose/jws.py index 9c14cf729..8fa8d7670 100644 --- a/acme/acme/jose/jws.py +++ b/acme/acme/jose/jws.py @@ -121,12 +121,12 @@ class Header(json_util.JSONObjectWithFields): # x5c does NOT use JOSE Base64 (4.1.6) - @x5c.encoder + @x5c.encoder # type: ignore def x5c(value): # pylint: disable=missing-docstring,no-self-argument return [base64.b64encode(OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_ASN1, cert.wrapped)) for cert in value] - @x5c.decoder + @x5c.decoder # type: ignore def x5c(value): # pylint: disable=missing-docstring,no-self-argument try: return tuple(util.ComparableX509(OpenSSL.crypto.load_certificate( @@ -157,12 +157,12 @@ class Signature(json_util.JSONObjectWithFields): 'signature', decoder=json_util.decode_b64jose, encoder=json_util.encode_b64jose) - @protected.encoder + @protected.encoder # type: ignore def protected(value): # pylint: disable=missing-docstring,no-self-argument # wrong type guess (Signature, not bytes) | pylint: disable=no-member return json_util.encode_b64jose(value.encode('utf-8')) - @protected.decoder + @protected.decoder # type: ignore def protected(value): # pylint: disable=missing-docstring,no-self-argument return json_util.decode_b64jose(value).decode('utf-8') diff --git a/acme/acme/jose/util.py b/acme/acme/jose/util.py index 6be9a6602..26b7e0c5a 100644 --- a/acme/acme/jose/util.py +++ b/acme/acme/jose/util.py @@ -134,7 +134,7 @@ class ComparableRSAKey(ComparableKey): # pylint: disable=too-few-public-methods return hash((self.__class__, pub.n, pub.e)) -class ImmutableMap(collections.Mapping, collections.Hashable): +class ImmutableMap(collections.Mapping, collections.Hashable): # type: ignore # pylint: disable=too-few-public-methods """Immutable key to value mapping with attribute access.""" @@ -180,7 +180,7 @@ class ImmutableMap(collections.Mapping, collections.Hashable): for key, value in six.iteritems(self))) -class frozendict(collections.Mapping, collections.Hashable): +class frozendict(collections.Mapping, collections.Hashable): # type: ignore # pylint: disable=invalid-name,too-few-public-methods """Frozen dictionary.""" __slots__ = ('_items', '_keys') diff --git a/acme/acme/messages.py b/acme/acme/messages.py index f7670dd72..4070290ad 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -98,7 +98,7 @@ class Error(jose.JSONObjectWithFields, errors.Error): if part is not None) -class _Constant(jose.JSONDeSerializable, collections.Hashable): +class _Constant(jose.JSONDeSerializable, collections.Hashable): # type: ignore """ACME constant.""" __slots__ = ('name',) POSSIBLE_NAMES = NotImplemented @@ -132,7 +132,7 @@ class _Constant(jose.JSONDeSerializable, collections.Hashable): class Status(_Constant): """ACME "status" field.""" - POSSIBLE_NAMES = {} + POSSIBLE_NAMES = {} # type: dict STATUS_UNKNOWN = Status('unknown') STATUS_PENDING = Status('pending') STATUS_PROCESSING = Status('processing') @@ -143,7 +143,7 @@ STATUS_REVOKED = Status('revoked') class IdentifierType(_Constant): """ACME identifier type.""" - POSSIBLE_NAMES = {} + POSSIBLE_NAMES = {} # type: dict IDENTIFIER_FQDN = IdentifierType('dns') # IdentifierDNS in Boulder @@ -161,7 +161,7 @@ class Identifier(jose.JSONObjectWithFields): class Directory(jose.JSONDeSerializable): """Directory.""" - _REGISTERED_TYPES = {} + _REGISTERED_TYPES = {} # type: dict class Meta(jose.JSONObjectWithFields): """Directory Meta.""" diff --git a/acme/acme/standalone.py b/acme/acme/standalone.py index 02cc2daf5..087240c15 100644 --- a/acme/acme/standalone.py +++ b/acme/acme/standalone.py @@ -6,9 +6,9 @@ import logging import os import sys -from six.moves import BaseHTTPServer # pylint: disable=import-error +from six.moves import BaseHTTPServer # type: ignore # pylint: disable=import-error from six.moves import http_client # pylint: disable=import-error -from six.moves import socketserver # pylint: disable=import-error +from six.moves import socketserver # type: ignore # pylint: disable=import-error import OpenSSL diff --git a/acme/acme/standalone_test.py b/acme/acme/standalone_test.py index 58469d470..613258c97 100644 --- a/acme/acme/standalone_test.py +++ b/acme/acme/standalone_test.py @@ -7,7 +7,7 @@ import time import unittest from six.moves import http_client # pylint: disable=import-error -from six.moves import socketserver # pylint: disable=import-error +from six.moves import socketserver # type: ignore # pylint: disable=import-error import requests diff --git a/certbot-compatibility-test/certbot_compatibility_test/interfaces.py b/certbot-compatibility-test/certbot_compatibility_test/interfaces.py index cd367d9af..7d3daee09 100644 --- a/certbot-compatibility-test/certbot_compatibility_test/interfaces.py +++ b/certbot-compatibility-test/certbot_compatibility_test/interfaces.py @@ -20,20 +20,20 @@ class IPluginProxy(zope.interface.Interface): def __init__(args): """Initializes the plugin with the given command line args""" - def cleanup_from_tests(): + def cleanup_from_tests(): # type: ignore """Performs any necessary cleanup from running plugin tests. This is guaranteed to be called before the program exits. """ - def has_more_configs(): + def has_more_configs(): # type: ignore """Returns True if there are more configs to test""" - def load_config(): + def load_config(): # type: ignore """Loads the next config and returns its name""" - def get_testable_domain_names(): + def get_testable_domain_names(): # type: ignore """Returns the domain names that can be used in testing""" @@ -44,7 +44,7 @@ class IAuthenticatorProxy(IPluginProxy, certbot.interfaces.IAuthenticator): class IInstallerProxy(IPluginProxy, certbot.interfaces.IInstaller): """Wraps a Certbot installer""" - def get_all_names_answer(): + def get_all_names_answer(): # type: ignore """Returns all names that should be found by the installer""" diff --git a/certbot/cli.py b/certbot/cli.py index 4fabd9a50..1d8952d20 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -207,7 +207,7 @@ def set_by_cli(var): return False # static housekeeping var -set_by_cli.detector = None +set_by_cli.detector = None # type: ignore def has_default_value(option, value): diff --git a/certbot/display/completer.py b/certbot/display/completer.py index 37564954a..08b55fdea 100644 --- a/certbot/display/completer.py +++ b/certbot/display/completer.py @@ -4,7 +4,7 @@ import glob try: import readline except ImportError: - import certbot.display.dummy_readline as readline + import certbot.display.dummy_readline as readline # type: ignore class Completer(object): diff --git a/certbot/hooks.py b/certbot/hooks.py index ada3d3aaa..75d7a3b20 100644 --- a/certbot/hooks.py +++ b/certbot/hooks.py @@ -13,12 +13,14 @@ from certbot.plugins import util as plug_util logger = logging.getLogger(__name__) + def validate_hooks(config): """Check hook commands are executable.""" validate_hook(config.pre_hook, "pre") validate_hook(config.post_hook, "post") validate_hook(config.renew_hook, "renew") + def _prog(shell_cmd): """Extract the program run by a shell command. @@ -52,6 +54,7 @@ def validate_hook(shell_cmd, hook_name): raise errors.HookCommandNotFound(msg) + def pre_hook(config): "Run pre-hook if it's defined and hasn't been run." cmd = config.pre_hook @@ -62,7 +65,7 @@ def pre_hook(config): elif cmd: logger.info("Pre-hook command already run, skipping: %s", cmd) -pre_hook.already = set() +pre_hook.already = set() # type: ignore def post_hook(config): @@ -82,7 +85,8 @@ def post_hook(config): logger.info("Running post-hook command: %s", cmd) _run_hook(cmd) -post_hook.eventually = [] +post_hook.eventually = [] # type: ignore + def run_saved_post_hooks(): """Run any post hooks that were saved up in the course of the 'renew' verb""" diff --git a/certbot/interfaces.py b/certbot/interfaces.py index a2767121b..213992993 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -99,7 +99,7 @@ class IPluginFactory(zope.interface.Interface): class IPlugin(zope.interface.Interface): """Certbot plugin.""" - def prepare(): + def prepare(): # type: ignore """Prepare the plugin. Finish up any additional initialization. @@ -118,7 +118,7 @@ class IPlugin(zope.interface.Interface): """ - def more_info(): + def more_info(): # type: ignore """Human-readable string to help the user. Should describe the steps taken and any relevant info to help the user @@ -251,7 +251,7 @@ class IInstaller(IPlugin): """ - def get_all_names(): + def get_all_names(): # type: ignore """Returns all names that may be authenticated. :rtype: `collections.Iterable` of `str` @@ -288,7 +288,7 @@ class IInstaller(IPlugin): """ - def supported_enhancements(): + def supported_enhancements(): # type: ignore """Returns a `collections.Iterable` of supported enhancements. :returns: supported enhancements which should be a subset of @@ -326,7 +326,7 @@ class IInstaller(IPlugin): """ - def recovery_routine(): + def recovery_routine(): # type: ignore """Revert configuration to most recent finalized checkpoint. Remove all changes (temporary and permanent) that have not been @@ -337,21 +337,21 @@ class IInstaller(IPlugin): """ - def view_config_changes(): + def view_config_changes(): # type: ignore """Display all of the LE config changes. :raises .PluginError: when config changes cannot be parsed """ - def config_test(): + def config_test(): # type: ignore """Make sure the configuration is valid. :raises .MisconfigurationError: when the config is not in a usable state """ - def restart(): + def restart(): # type: ignore """Restart or refresh the server content. :raises .PluginError: when server cannot be restarted diff --git a/certbot/plugins/disco.py b/certbot/plugins/disco.py index e567422e2..a17f8d7b3 100644 --- a/certbot/plugins/disco.py +++ b/certbot/plugins/disco.py @@ -27,7 +27,7 @@ class PluginEntryPoint(object): """Distributions for which prefix will be omitted.""" # this object is mutable, don't allow it to be hashed! - __hash__ = None + __hash__ = None # type: ignore def __init__(self, entry_point): self.name = self.entry_point_to_plugin_name(entry_point) diff --git a/certbot/reporter.py b/certbot/reporter.py index 118b13166..f836dbc8c 100644 --- a/certbot/reporter.py +++ b/certbot/reporter.py @@ -7,7 +7,7 @@ import os import sys import textwrap -from six.moves import queue # pylint: disable=import-error +from six.moves import queue # type: ignore # pylint: disable=import-error import zope.interface from certbot import interfaces diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 6dc839025..13a91dfb8 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -193,7 +193,7 @@ try: file_type = file except NameError: import io - file_type = io.TextIOWrapper + file_type = io.TextIOWrapper # type: ignore class UniqueLineageNameTest(unittest.TestCase): diff --git a/tox.ini b/tox.ini index 3e284bf29..d393bb610 100644 --- a/tox.ini +++ b/tox.ini @@ -61,6 +61,13 @@ commands = pip install -q -e acme[dev] -e .[dev] -e certbot-apache -e certbot-nginx -e certbot-compatibility-test -e letshelp-certbot pylint --reports=n --rcfile=.pylintrc acme/acme certbot certbot-apache/certbot_apache certbot-nginx/certbot_nginx certbot-compatibility-test/certbot_compatibility_test letshelp-certbot/letshelp_certbot +[testenv:mypy] +basepython = python3.4 +commands = + pip install mypy + pip install -q -e acme[dev] -e .[dev] -e certbot-apache -e certbot-nginx -e certbot-compatibility-test -e letshelp-certbot + mypy --py2 --ignore-missing-imports acme/acme certbot certbot-apache/certbot_apache certbot-nginx/certbot_nginx certbot-compatibility-test/certbot_compatibility_test letshelp-certbot/letshelp_certbot + [testenv:apacheconftest] #basepython = python2.7 commands = From 32122cfa21fe7ba43189658158949cfe16dc6717 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 20 Mar 2017 15:48:39 -0700 Subject: [PATCH 22/40] Add a global lock file to Certbot (#4369) * add fasteners as a dependency * add LOCK_FILE constant * Add lock file to Certbot * Move code to _run_subcommand * move lock file path into CLI_CONSTANTS * add --lock-path flag * move locking code to separate function * Add TestAcquireFileLock * assert we log * test lock contention * add fasteners to certbot-auto * Use a different lock file for each test in MainTest --- certbot/cli.py | 2 + certbot/constants.py | 1 + certbot/interfaces.py | 3 ++ certbot/main.py | 53 ++++++++++++++++++- certbot/tests/main_test.py | 53 ++++++++++++++++++- letsencrypt-auto-source/letsencrypt-auto | 6 +++ .../pieces/letsencrypt-auto-requirements.txt | 6 +++ setup.py | 1 + 8 files changed, 123 insertions(+), 2 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 1d8952d20..5b8711da6 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1167,6 +1167,8 @@ def _paths_parser(helpful): help="Logs directory.") add("paths", "--server", default=flag_default("server"), help=config_help("server")) + add("paths", "--lock-path", default=flag_default("lock_path"), + help=config_help('lock_path')) def _plugins_parsing(helpful, plugins): diff --git a/certbot/constants.py b/certbot/constants.py index 382b0afb3..24cf89199 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -33,6 +33,7 @@ CLI_DEFAULTS = dict( auth_cert_path="./cert.pem", auth_chain_path="./chain.pem", strict_permissions=False, + lock_path="/tmp/.certbot.lock", debug_challenges=False, ) STAGING_URI = "https://acme-staging.api.letsencrypt.org/directory" diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 213992993..33bde9430 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -222,6 +222,9 @@ class IConfig(zope.interface.Interface): key_dir = zope.interface.Attribute("Keys storage.") temp_checkpoint_dir = zope.interface.Attribute( "Temporary checkpoint directory.") + lock_path = zope.interface.Attribute( + "Path to the lock file used to prevent multiple instances of " + "Certbot from modifying your server's configuration at once.") no_verify_ssl = zope.interface.Attribute( "Disable verification of the ACME server's certificate.") diff --git a/certbot/main.py b/certbot/main.py index ab2204428..c6e61fbf9 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -8,6 +8,7 @@ import sys import time import traceback +import fasteners import zope.component from acme import jose @@ -866,6 +867,56 @@ def _post_logging_setup(config, plugins, cli_args): logger.debug("Discovered plugins: %r", plugins) +def acquire_file_lock(lock_path): + """Obtain a lock on the file at the specified path. + + :param str lock_path: path to the file to be locked + + :returns: lock file object representing the acquired lock + :rtype: fasteners.InterProcessLock + + :raises .Error: if the lock is held by another process + + """ + lock = fasteners.InterProcessLock(lock_path) + logger.debug("Attempting to acquire lock file %s", lock_path) + + try: + lock.acquire(blocking=False) + except IOError as err: + logger.debug(err) + logger.warning( + "Unable to access lock file %s. You should set --lock-file " + "to a writeable path to ensure multiple instances of " + "Certbot don't attempt modify your configuration " + "simultaneously.", lock_path) + else: + if not lock.acquired: + raise errors.Error( + "Another instance of Certbot is already running.") + + return lock + + +def _run_subcommand(config, plugins): + """Executes the Certbot subcommand specified in the configuration. + + :param .IConfig config: parsed configuration object + :param .PluginsRegistry plugins: available plugins + + :returns: return value from the specified subcommand + :rtype: str or int + + """ + lock = acquire_file_lock(config.lock_path) + + try: + return config.func(config, plugins) + finally: + if lock.acquired: + lock.release() + + def main(cli_args=sys.argv[1:]): """Command line argument parsing and main script execution.""" sys.excepthook = functools.partial(_handle_exception, config=None) @@ -893,7 +944,7 @@ def main(cli_args=sys.argv[1:]): zope.component.provideUtility(report) atexit.register(report.atexit_print_messages) - return config.func(config, plugins) + return _run_subcommand(config, plugins) if __name__ == "__main__": diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 170eceb48..5707231b7 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -4,6 +4,7 @@ from __future__ import print_function import itertools import mock +import multiprocessing import os import shutil import tempfile @@ -451,7 +452,8 @@ class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods os.mkdir(self.logs_dir) self.standard_args = ['--config-dir', self.config_dir, '--work-dir', self.work_dir, - '--logs-dir', self.logs_dir, '--text'] + '--logs-dir', self.logs_dir, '--text', + '--lock-path', os.path.join(self.tmp_dir, 'certbot.lock')] def tearDown(self): # Reset globals in cli @@ -1308,5 +1310,54 @@ class TestHandleException(unittest.TestCase): traceback.format_exception_only(KeyboardInterrupt, interrupt))) +class TestAcquireFileLock(unittest.TestCase): + """Test main.acquire_file_lock.""" + + def setUp(self): + self.tempdir = tempfile.mkdtemp() + self.lock_path = os.path.join(self.tempdir, 'certbot.lock') + + def tearDown(self): + shutil.rmtree(self.tempdir) + + @mock.patch('certbot.main.logger') + def test_bad_path(self, mock_logger): + lock = main.acquire_file_lock(os.getcwd()) + self.assertTrue(mock_logger.warning.called) + self.assertFalse(lock.acquired) + + def test_held_lock(self): + # start child and wait for it to grab the lock + cv = multiprocessing.Condition() + cv.acquire() + child_args = (cv, self.lock_path,) + child = multiprocessing.Process(target=_hold_lock, args=child_args) + child.start() + cv.wait() + + # assert we can't grab lock and terminate the child + self.assertRaises(errors.Error, main.acquire_file_lock, self.lock_path) + cv.notify() + cv.release() + child.join() + self.assertEqual(child.exitcode, 0) + + +def _hold_lock(cv, lock_path): + """Acquire a file lock at lock_path and wait to release it. + + :param multiprocessing.Condition cv: condition for syncronization + :param str lock_path: path to the file lock + + """ + import fasteners + lock = fasteners.InterProcessLock(lock_path) + lock.acquire() + cv.acquire() + cv.notify() + cv.wait() + lock.release() + + if __name__ == '__main__': unittest.main() # pragma: no cover diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 35c7a530c..475d57fee 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -727,6 +727,9 @@ cryptography==1.5.3 \ enum34==1.1.2 \ --hash=sha256:2475d7fcddf5951e92ff546972758802de5260bf409319a9f1934e6bbc8b1dc7 \ --hash=sha256:35907defb0f992b75ab7788f65fedc1cf20ffa22688e0e6f6f12afc06b3ea501 +fasteners==0.14.1 \ + --hash=sha256:564a115ff9698767df401efca29620cbb1a1c2146b7095ebd304b79cc5807a7c \ + --hash=sha256:427c76773fe036ddfa41e57d89086ea03111bbac57c55fc55f3006d027107e18 funcsigs==0.4 \ --hash=sha256:ff5ad9e2f8d9e5d1e8bbfbcf47722ab527cf0d51caeeed9da6d0f40799383fde \ --hash=sha256:d83ce6df0b0ea6618700fe1db353526391a8a3ada1b7aba52fed7a61da772033 @@ -739,6 +742,9 @@ ipaddress==1.0.16 \ linecache2==1.0.0 \ --hash=sha256:e78be9c0a0dfcbac712fe04fbf92b96cddae80b1b842f24248214c8496f006ef \ --hash=sha256:4b26ff4e7110db76eeb6f5a7b64a82623839d595c2038eeda662f2a2db78e97c +monotonic==1.3 \ + --hash=sha256:a8c7690953546c6bc8a4f05d347718db50de1225b29f4b9f346c0c6f19bdc286 \ + --hash=sha256:2b469e2d7dd403f7f7f79227fe5ad551ee1e76f8bb300ae935209884b93c7c1b ordereddict==1.1 \ --hash=sha256:1c35b4ac206cef2d24816c89f89cf289dd3d38cf7c449bb3fab7bf6d43f01b1f parsedatetime==2.1 \ diff --git a/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt b/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt index d70d24e2a..fbf416d66 100644 --- a/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt +++ b/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt @@ -65,6 +65,9 @@ cryptography==1.5.3 \ enum34==1.1.2 \ --hash=sha256:2475d7fcddf5951e92ff546972758802de5260bf409319a9f1934e6bbc8b1dc7 \ --hash=sha256:35907defb0f992b75ab7788f65fedc1cf20ffa22688e0e6f6f12afc06b3ea501 +fasteners==0.14.1 \ + --hash=sha256:564a115ff9698767df401efca29620cbb1a1c2146b7095ebd304b79cc5807a7c \ + --hash=sha256:427c76773fe036ddfa41e57d89086ea03111bbac57c55fc55f3006d027107e18 funcsigs==0.4 \ --hash=sha256:ff5ad9e2f8d9e5d1e8bbfbcf47722ab527cf0d51caeeed9da6d0f40799383fde \ --hash=sha256:d83ce6df0b0ea6618700fe1db353526391a8a3ada1b7aba52fed7a61da772033 @@ -77,6 +80,9 @@ ipaddress==1.0.16 \ linecache2==1.0.0 \ --hash=sha256:e78be9c0a0dfcbac712fe04fbf92b96cddae80b1b842f24248214c8496f006ef \ --hash=sha256:4b26ff4e7110db76eeb6f5a7b64a82623839d595c2038eeda662f2a2db78e97c +monotonic==1.3 \ + --hash=sha256:a8c7690953546c6bc8a4f05d347718db50de1225b29f4b9f346c0c6f19bdc286 \ + --hash=sha256:2b469e2d7dd403f7f7f79227fe5ad551ee1e76f8bb300ae935209884b93c7c1b ordereddict==1.1 \ --hash=sha256:1c35b4ac206cef2d24816c89f89cf289dd3d38cf7c449bb3fab7bf6d43f01b1f parsedatetime==2.1 \ diff --git a/setup.py b/setup.py index 0e8d19a22..59f6dbd9e 100644 --- a/setup.py +++ b/setup.py @@ -43,6 +43,7 @@ install_requires = [ 'ConfigArgParse>=0.9.3', 'configobj', 'cryptography>=0.7', # load_pem_x509_certificate + 'fasteners', 'mock', 'parsedatetime>=1.3', # Calendar.parseDT 'PyOpenSSL', From b7d282309d9e28221ab557264966d42f4b383529 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 20 Mar 2017 17:57:09 -0700 Subject: [PATCH 23/40] Save hyphenated plugin params for renewal (#4281) * fix plugin namespace check * Add test to prevent regressions --- certbot/storage.py | 10 ++++++---- certbot/tests/storage_test.py | 9 +++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/certbot/storage.py b/certbot/storage.py index dacc73c4c..a1462b72d 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -19,6 +19,9 @@ from certbot import errors from certbot import error_handler from certbot import util +from certbot.plugins import common as plugins_common +from certbot.plugins import disco as plugins_disco + logger = logging.getLogger(__name__) ALL_FOUR = ("cert", "privkey", "chain", "fullchain") @@ -179,13 +182,12 @@ def _relevant(option): :rtype: bool """ - # The list() here produces a list of the plugin names as strings. from certbot import renewal - from certbot.plugins import disco as plugins_disco - plugins = list(plugins_disco.PluginsRegistry.find_all()) + plugins = plugins_disco.PluginsRegistry.find_all() + namespaces = [plugins_common.dest_namespace(plugin) for plugin in plugins] return (option in renewal.CONFIG_ITEMS or - any(option.startswith(x + "_") for x in plugins)) + any(option.startswith(namespace) for namespace in namespaces)) def relevant_values(all_values): diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index f52f31d3d..f7bde012d 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -581,6 +581,15 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertEqual( self._test_relevant_values_common(values), values) + @mock.patch("certbot.cli.set_by_cli") + @mock.patch("certbot.plugins.disco.PluginsRegistry.find_all") + def test_relevant_values_namespace(self, mock_find_all, mock_set_by_cli): + mock_set_by_cli.return_value = True + mock_find_all.return_value = ["certbot-foo:bar"] + values = {"certbot_foo:bar_baz": 42} + self.assertEqual( + self._test_relevant_values_common(values), values) + @mock.patch("certbot.storage.relevant_values") def test_new_lineage(self, mock_rv): """Test for new_lineage() class method.""" From bf45cea7cddd47e47194904d5f5b47757c622306 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 20 Mar 2017 18:00:50 -0700 Subject: [PATCH 24/40] Ensure a SHA2 hash algorithm is used when signing releases (#4384) * use gpg2 * explictly use sha256 --- tools/release.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/release.sh b/tools/release.sh index 81582cef0..1da11fe2c 100755 --- a/tools/release.sh +++ b/tools/release.sh @@ -109,7 +109,7 @@ do echo "Signing ($pkg_dir)" for x in dist/*.tar.gz dist/*.whl do - gpg -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign $x + gpg2 -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign --digest-algo sha256 $x done cd - @@ -194,7 +194,7 @@ while ! openssl dgst -sha256 -verify $RELEASE_OPENSSL_PUBKEY -signature \ done # This signature is not quite as strong, but easier for people to verify out of band -gpg -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign letsencrypt-auto-source/letsencrypt-auto +gpg2 -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign --digest-algo sha256 letsencrypt-auto-source/letsencrypt-auto # We can't rename the openssl letsencrypt-auto.sig for compatibility reasons, # but we can use the right name for certbot-auto.asc from day one mv letsencrypt-auto-source/letsencrypt-auto.asc letsencrypt-auto-source/certbot-auto.asc @@ -214,7 +214,7 @@ name=${root_without_le%.*} ext="${root_without_le##*.}" rev="$(git rev-parse --short HEAD)" echo tar cJvf $name.$rev.tar.xz $name.$rev -echo gpg -U $RELEASE_GPG_KEY --detach-sign --armor $name.$rev.tar.xz +echo gpg2 -U $RELEASE_GPG_KEY --detach-sign --armor $name.$rev.tar.xz cd ~- echo "New root: $root" From 7be2e790251a9f5b7c8ab48560bfe4e9737c11af Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 24 Mar 2017 19:45:53 -0700 Subject: [PATCH 25/40] Fix nginx parser (#4296) * rewrite nginx parser to allow everything that nginx does * also make changes in tls_sni_01.py * add test case with * allow embedded variables * allow empty ${} variable * fix quotes * un-special case if * update all tests to reflect current parsing * escape in QuotedString after merge * add test cases for variable weirdness that are almost certainly nginx bugs * update regex for correct variable rules * close paren doesn't invoke last_space * Make test file valid Nginx syntax --- .../simplepythonfcgi/weird-spacing.conf | 6 +- certbot-nginx/certbot_nginx/configurator.py | 8 +- certbot-nginx/certbot_nginx/nginxparser.py | 112 ++++------ certbot-nginx/certbot_nginx/parser.py | 30 ++- .../certbot_nginx/tests/configurator_test.py | 18 +- .../certbot_nginx/tests/nginxparser_test.py | 203 ++++++++++++++++-- certbot-nginx/certbot_nginx/tests/obj_test.py | 18 +- .../certbot_nginx/tests/parser_test.py | 14 +- certbot-nginx/certbot_nginx/tls_sni_01.py | 9 +- 9 files changed, 273 insertions(+), 145 deletions(-) diff --git a/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf b/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf index a201fe659..5fbc76676 100644 --- a/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf +++ b/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf @@ -1,16 +1,16 @@ # static files location ~ ^/(images|javascript|js|css|flash|media|static)/ { - root ${PROJECTBASE}/${PROJECTNAME}/static; + root "${PROJECTBASE}/${PROJECTNAME}/static"; } location = /favicon.ico { - root ${PROJECTBASE}/${PROJECTNAME}/static/images; + root "${PROJECTBASE}/${PROJECTNAME}/static/images"; } # pass all requests to FastCGI TG server listening on ${HOST}:${PORT} # location / { - fastcgi_pass ${HOST}:${PORT}; + fastcgi_pass "${HOST}:${PORT}"; fastcgi_index index; fastcgi_param SCRIPT_FILENAME /scripts$fastcgi_script_name; include conf/fastcgi_params; diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index e62194d4f..225702251 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -32,16 +32,16 @@ from certbot_nginx import parser logger = logging.getLogger(__name__) REDIRECT_BLOCK = [[ - ['\n ', 'if', ' ', '($scheme != "https") '], - [['\n ', 'return', ' ', '301 https://$host$request_uri'], + ['\n ', 'if', ' ', '($scheme', ' ', '!=', ' ', '"https") '], + [['\n ', 'return', ' ', '301', ' ', 'https://$host$request_uri'], '\n '] ], ['\n']] TEST_REDIRECT_BLOCK = [ [ - ['if', '($scheme != "https")'], + ['if', '($scheme', '!=', '"https")'], [ - ['return', '301 https://$host$request_uri'] + ['return', '301', 'https://$host$request_uri'] ] ], ['#', ' managed by Certbot'] diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 67ac7c3a1..20aeeb554 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -2,11 +2,9 @@ # Forked from https://github.com/fatiherikli/nginxparser (MIT Licensed) import copy import logging -import string from pyparsing import ( - Literal, White, Word, alphanums, CharsNotIn, Combine, Forward, Group, - Optional, OneOrMore, QuotedString, Regex, ZeroOrMore) + Literal, White, Forward, Group, Optional, OneOrMore, QuotedString, Regex, ZeroOrMore, Combine) from pyparsing import stringEnd from pyparsing import restOfLine @@ -14,73 +12,42 @@ logger = logging.getLogger(__name__) class RawNginxParser(object): # pylint: disable=expression-not-assigned + # pylint: disable=pointless-statement """A class that parses nginx configuration with pyparsing.""" # constants - space = Optional(White()) - nonspace = Regex(r"\S+") + space = Optional(White()).leaveWhitespace() + required_space = White().leaveWhitespace() + left_bracket = Literal("{").suppress() - right_bracket = space.leaveWhitespace() + Literal("}").suppress() + right_bracket = space + Literal("}").suppress() semicolon = Literal(";").suppress() - key = Word(alphanums + "_/+-.") - dollar_var = Combine(Literal('$') + Regex(r"[^\{\};,\s]+")) - condition = Regex(r"\(.+\)") - # Matches anything that is not a special character, and ${SHELL_VARS}, AND - # any chars in single or double quotes - # All of these COULD be upgraded to something like - # https://stackoverflow.com/a/16130746 - dquoted = QuotedString('"', multiline=True, unquoteResults=False) - squoted = QuotedString("'", multiline=True, unquoteResults=False) - nonspecial = Regex(r"[^\{\};,]") - varsub = Regex(r"(\$\{\w+\})") - # nonspecial nibbles one character at a time, but the other objects take - # precedence. We use ZeroOrMore to allow entries like "break ;" to be - # parsed as assignments - value = Combine(ZeroOrMore(dquoted | squoted | varsub | nonspecial)) + dquoted = QuotedString('"', multiline=True, unquoteResults=False, escChar='\\') + squoted = QuotedString("'", multiline=True, unquoteResults=False, escChar='\\') + quoted = dquoted | squoted + head_tokenchars = Regex(r"[^{};\s'\"]") # if (last_space) + tail_tokenchars = Regex(r"(\$\{)|[^{;\s]") # else + tokenchars = Combine(head_tokenchars + ZeroOrMore(tail_tokenchars)) + paren_quote_extend = Combine(quoted + Literal(')') + ZeroOrMore(tail_tokenchars)) + # note: ')' allows extension, but then we fall into else, not last_space. - location = CharsNotIn("{};," + string.whitespace) - # modifier for location uri [ = | ~ | ~* | ^~ ] - modifier = Literal("=") | Literal("~*") | Literal("~") | Literal("^~") + token = paren_quote_extend | tokenchars | quoted + + whitespace_token_group = space + token + ZeroOrMore(required_space + token) + space + assignment = whitespace_token_group + semicolon - # rules comment = space + Literal('#') + restOfLine - assignment = space + key + Optional(space + value, default=None) + semicolon - location_statement = space + Optional(modifier) + Optional(space + location + space) - if_statement = space + Literal("if") + space + condition + space - charset_map_statement = space + Literal("charset_map") + space + value + space + value - - map_statement = space + Literal("map") + space + nonspace + space + dollar_var + space - # This is NOT an accurate way to parse nginx map entries; it's almost - # certainly too permissive and may be wrong in other ways, but it should - # preserve things correctly in mmmmost or all cases. - # - # - I can neither prove nor disprove that it is correct wrt all escaped - # semicolon situations - # Addresses https://github.com/fatiherikli/nginxparser/issues/19 - map_pattern = Regex(r'".*"') | Regex(r"'.*'") | nonspace - map_entry = space + map_pattern + space + value + space + semicolon - map_block = Group( - Group(map_statement).leaveWhitespace() + - left_bracket + - Group(ZeroOrMore(Group(comment | map_entry)) + space).leaveWhitespace() + - right_bracket) - block = Forward() - # key could for instance be "server" or "http", or "location" (in which case - # location_statement needs to have a non-empty location) + # order matters! see issue 518, and also http { # server { \n} + contents = Group(comment) | Group(block) | Group(assignment) - block_begin = (Group(space + key + location_statement) ^ - Group(if_statement) ^ - Group(charset_map_statement)).leaveWhitespace() + block_begin = Group(whitespace_token_group) + block_innards = Group(ZeroOrMore(contents) + space).leaveWhitespace() + block << block_begin + left_bracket + block_innards + right_bracket - block_innards = Group(ZeroOrMore(Group(comment | assignment) | block | map_block) - + space).leaveWhitespace() - - block << Group(block_begin + left_bracket + block_innards + right_bracket) - - script = OneOrMore(Group(comment | assignment) ^ block ^ map_block) + space + stringEnd + script = OneOrMore(contents) + space + stringEnd script.parseWithTabs().leaveWhitespace() def __init__(self, source): @@ -107,30 +74,23 @@ class RawNginxDumper(object): if isinstance(b0, str): yield b0 continue - b = copy.deepcopy(b0) - if spacey(b[0]): - yield b.pop(0) # indentation - if not b: + item = copy.deepcopy(b0) + if spacey(item[0]): + yield item.pop(0) # indentation + if not item: continue - key, values = b.pop(0), b.pop(0) - if isinstance(key, list): - yield "".join(key) + '{' - for parameter in values: + if isinstance(item[0], list): # block + yield "".join(item.pop(0)) + '{' + for parameter in item.pop(0): for line in self.__iter__([parameter]): # negate "for b0 in blocks" yield line yield '}' - else: - if isinstance(key, str) and key.strip() == '#': # comment - yield key + values - else: # assignment - gap = "" - # Sometimes the parser has stuck some gap whitespace in here; - # if so rotate it into gap - if values and spacey(values): - gap = values - values = b.pop(0) - yield key + gap + values + ';' + else: # not a block - list of strings + semicolon = ";" + if isinstance(item[0], str) and item[0].strip() == '#': # comment + semicolon = "" + yield "".join(item) + semicolon def __str__(self): """Return the parsed block as a string.""" diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index fd4ea4f11..6f3f344db 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -298,9 +298,9 @@ class NginxParser(object): """ server = vhost.raw for directive in server: - if not directive or len(directive) < 2: + if not directive: continue - elif directive[0] == 'ssl' and directive[1] == 'on': + elif _is_ssl_on_directive(directive): return True return False @@ -468,17 +468,17 @@ def _is_include_directive(entry): len(entry) == 2 and entry[0] == 'include' and isinstance(entry[1], str)) +def _is_ssl_on_directive(entry): + """Checks if an nginx parsed entry is an 'ssl on' directive. -def _get_servernames(names): - """Turns a server_name string into a list of server names - - :param str names: server names - :rtype: list + :param list entry: the parsed entry + :returns: Whether it's an 'ssl on' directive + :rtype: bool """ - whitespace_re = re.compile(r'\s+') - names = re.sub(whitespace_re, ' ', names) - return names.split(' ') + return (isinstance(entry, list) and + len(entry) == 2 and entry[0] == 'ssl' and + entry[1] == 'on') def _add_directives(block, directives, replace): """Adds or replaces directives in a config block. @@ -550,12 +550,11 @@ def _add_directive(block, directive, replace): # and there is already a copy of that directive with a different value # in the config file. directive_name = directive[0] - directive_value = directive[1] if location is None or (isinstance(directive_name, str) and directive_name in REPEATABLE_DIRECTIVES): block.append(directive) _comment_directive(block, len(block) - 1) - elif block[location][1] != directive_value: + elif block[location] != directive: raise errors.MisconfigurationError( 'tried to insert directive "{0}" but found ' 'conflicting "{1}".'.format(directive, block[location])) @@ -585,15 +584,14 @@ def _parse_server_raw(server): if not directive: continue if directive[0] == 'listen': - addr = obj.Addr.fromstring(directive[1]) + addr = obj.Addr.fromstring(" ".join(directive[1:])) if addr: parsed_server['addrs'].add(addr) if addr.ssl: parsed_server['ssl'] = True elif directive[0] == 'server_name': - parsed_server['names'].update( - _get_servernames(directive[1])) - elif directive[0] == 'ssl' and directive[1] == 'on': + parsed_server['names'].update(directive[1:]) + elif _is_ssl_on_directive(directive): parsed_server['ssl'] = True apply_ssl_to_all_addrs = True diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index d491d2a15..b9e70cd59 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -94,7 +94,7 @@ class NginxConfiguratorTest(util.NginxTest): None, [0]) self.config.parser.add_server_directives( mock_vhost, - [['listen', ' ', '5001 ssl']], + [['listen', ' ', '5001', ' ', 'ssl']], replace=False) self.config.save() @@ -105,7 +105,7 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '127.0.0.1'], ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'], + ['listen', '5001', 'ssl'], ['#', parser.COMMENT]]]], parsed[0]) @@ -205,13 +205,13 @@ class NginxConfiguratorTest(util.NginxTest): ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'], + ['listen', '5001', 'ssl'], ['ssl_certificate', 'example/fullchain.pem'], ['ssl_certificate_key', 'example/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) ]], parsed_example_conf) - self.assertEqual([['server_name', 'somename alias another.alias']], + self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']], parsed_server_conf) self.assertTrue(util.contains_at_depth( parsed_nginx_conf, @@ -222,8 +222,8 @@ class NginxConfiguratorTest(util.NginxTest): ['include', 'server.conf'], [['location', '/'], [['root', 'html'], - ['index', 'index.html index.htm']]], - ['listen', '5001 ssl'], + ['index', 'index.html', 'index.htm']]], + ['listen', '5001', 'ssl'], ['ssl_certificate', '/etc/nginx/fullchain.pem'], ['ssl_certificate_key', '/etc/nginx/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) @@ -247,7 +247,7 @@ class NginxConfiguratorTest(util.NginxTest): ['server_name', 'summer.com'], ['listen', '80'], - ['listen', '5001 ssl'], + ['listen', '5001', 'ssl'], ['ssl_certificate', 'summer/fullchain.pem'], ['ssl_certificate_key', 'summer/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) @@ -408,8 +408,8 @@ class NginxConfiguratorTest(util.NginxTest): # Test that we successfully add a redirect when there is # a listen directive expected = [ - ['if', '($scheme != "https") '], - [['return', '301 https://$host$request_uri']] + ['if', '($scheme', '!=', '"https") '], + [['return', '301', 'https://$host$request_uri']] ] example_conf = self.config.parser.abs_path('sites-enabled/example.com') diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index 653fb5069..0a8dbd100 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -25,15 +25,15 @@ class TestRawNginxParser(unittest.TestCase): def test_blocks(self): parsed = RawNginxParser.block.parseString('foo {}').asList() - self.assertEqual(parsed, [[['foo', ' '], []]]) + self.assertEqual(parsed, [['foo', ' '], []]) parsed = RawNginxParser.block.parseString('location /foo{}').asList() - self.assertEqual(parsed, [[['location', ' ', '/foo'], []]]) + self.assertEqual(parsed, [['location', ' ', '/foo'], []]) parsed = RawNginxParser.block.parseString('foo { bar foo ; }').asList() - self.assertEqual(parsed, [[['foo', ' '], [[' ', 'bar', ' ', 'foo '], ' ']]]) + self.assertEqual(parsed, [['foo', ' '], [[' ', 'bar', ' ', 'foo', ' '], ' ']]) def test_nested_blocks(self): parsed = RawNginxParser.block.parseString('foo { bar {} }').asList() - block, content = FIRST(parsed) + block, content = parsed self.assertEqual(FIRST(content), [[' ', 'bar', ' '], []]) self.assertEqual(FIRST(block), 'foo') @@ -72,8 +72,8 @@ class TestRawNginxParser(unittest.TestCase): [['user', 'www-data'], [['http'], [[['server'], [ - ['listen', '*:80 default_server ssl'], - ['server_name', '*.www.foo.com *.www.example.com'], + ['listen', '*:80', 'default_server', 'ssl'], + ['server_name', '*.www.foo.com', '*.www.example.com'], ['root', '/home/ubuntu/sites/foo/'], [['location', '/status'], [ [['types'], [['image/jpeg', 'jpg']]], @@ -97,17 +97,17 @@ class TestRawNginxParser(unittest.TestCase): [['server'], [['server_name', 'with.if'], [['location', '~', '^/services/.+$'], - [[['if', '($request_filename ~* \\.(ttf|woff)$)'], - [['add_header', 'Access-Control-Allow-Origin "*"']]]]]]], + [[['if', '($request_filename', '~*', '\\.(ttf|woff)$)'], + [['add_header', 'Access-Control-Allow-Origin', '"*"']]]]]]], [['server'], [['server_name', 'with.complicated.headers'], [['location', '~*', '\\.(?:gif|jpe?g|png)$'], - [['add_header', 'Pragma public'], + [['add_header', 'Pragma', 'public'], ['add_header', - 'Cache-Control \'public, must-revalidate, proxy-revalidate\'' - ' "test,;{}" foo'], + 'Cache-Control', '\'public, must-revalidate, proxy-revalidate\'', + '"test,;{}"', 'foo'], ['blah', '"hello;world"'], - ['try_files', '$uri @rewrites']]]]]]) + ['try_files', '$uri', '@rewrites']]]]]]) def test_parse_from_file3(self): with open(util.get_data_filename('multiline_quotes.conf')) as handle: @@ -135,7 +135,7 @@ class TestRawNginxParser(unittest.TestCase): with open(util.get_data_filename('nginx.conf')) as handle: parsed = load(handle) parsed[-1][-1].append(UnspacedList([['server'], - [['listen', ' ', '443 ssl'], + [['listen', ' ', '443', ' ', 'ssl'], ['server_name', ' ', 'localhost'], ['ssl_certificate', ' ', 'cert.pem'], ['ssl_certificate_key', ' ', 'cert.key'], @@ -144,7 +144,7 @@ class TestRawNginxParser(unittest.TestCase): ['ssl_ciphers', ' ', 'HIGH:!aNULL:!MD5'], [['location', ' ', '/'], [['root', ' ', 'html'], - ['index', ' ', 'index.html index.htm']]]]])) + ['index', ' ', 'index.html', ' ', 'index.htm']]]]])) with tempfile.TemporaryFile(mode='w+t') as f: dump(parsed, f) @@ -179,10 +179,175 @@ class TestRawNginxParser(unittest.TestCase): parsed = loads('if ($http_accept ~* "webp") { set $webp "true"; }') self.assertEqual(parsed, [ - [['if', '($http_accept ~* "webp")'], - [['set', '$webp "true"']]] + [['if', '($http_accept', '~*', '"webp")'], + [['set', '$webp', '"true"']]] ]) + def test_comment_in_block(self): + parsed = loads("""http { + # server{ + }""") + + self.assertEqual(parsed, [ + [['http'], + [['#', ' server{']]] + ]) + + def test_access_log(self): + # see issue #3798 + parsed = loads('access_log syslog:server=unix:/dev/log,facility=auth,' + 'tag=nginx_post,severity=info custom;') + + self.assertEqual(parsed, [ + ['access_log', + 'syslog:server=unix:/dev/log,facility=auth,tag=nginx_post,severity=info', + 'custom'] + ]) + + def test_add_header(self): + # see issue #3798 + parsed = loads('add_header Cache-Control no-cache,no-store,must-revalidate,max-age=0;') + + self.assertEqual(parsed, [ + ['add_header', 'Cache-Control', 'no-cache,no-store,must-revalidate,max-age=0'] + ]) + + def test_map_then_assignment_in_block(self): + # see issue #3798 + test_str = """http { + map $http_upgrade $connection_upgrade { + default upgrade; + '' close; + "~Opera Mini" 1; + *.example.com 1; + } + one; + }""" + parsed = loads(test_str) + self.assertEqual(parsed, [ + [['http'], [ + [['map', '$http_upgrade', '$connection_upgrade'], [ + ['default', 'upgrade'], + ["''", 'close'], + ['"~Opera Mini"', '1'], + ['*.example.com', '1'] + ]], + ['one'] + ]] + ]) + + def test_variable_name(self): + parsed = loads('try_files /typo3temp/tx_ncstaticfilecache/' + '$host${request_uri}index.html @nocache;') + + self.assertEqual(parsed, [ + ['try_files', + '/typo3temp/tx_ncstaticfilecache/$host${request_uri}index.html', + '@nocache'] + ]) + + def test_weird_blocks(self): + test = r""" + if ($http_user_agent ~ MSIE) { + rewrite ^(.*)$ /msie/$1 break; + } + + if ($http_cookie ~* "id=([^;]+)(?:;|$)") { + set $id $1; + } + + if ($request_method = POST) { + return 405; + } + + if ($request_method) { + return 403; + } + + if ($args ~ post=140){ + rewrite ^ http://example.com/; + } + + location ~ ^/users/(.+\.(?:gif|jpe?g|png))$ { + alias /data/w3/images/$1; + } + """ + parsed = loads(test) + self.assertEqual(parsed, [[['if', '($http_user_agent', '~', 'MSIE)'], + [['rewrite', '^(.*)$', '/msie/$1', 'break']]], + [['if', '($http_cookie', '~*', '"id=([^;]+)(?:;|$)")'], [['set', '$id', '$1']]], + [['if', '($request_method', '=', 'POST)'], [['return', '405']]], + [['if', '($request_method)'], + [['return', '403']]], [['if', '($args', '~', 'post=140)'], + [['rewrite', '^', 'http://example.com/']]], + [['location', '~', '^/users/(.+\\.(?:gif|jpe?g|png))$'], + [['alias', '/data/w3/images/$1']]]] + ) + + def test_edge_cases(self): + # quotes + parsed = loads(r'"hello\""; # blah "heh heh"') + self.assertEqual(parsed, [['"hello\\""'], ['#', ' blah "heh heh"']]) + + # empty var as block + parsed = loads(r"${}") + self.assertEqual(parsed, [[['$'], []]]) + + # if with comment + parsed = loads("""if ($http_cookie ~* "id=([^;]+)(?:;|$)") { # blah ) + }""") + self.assertEqual(parsed, [[['if', '($http_cookie', '~*', '"id=([^;]+)(?:;|$)")'], + [['#', ' blah )']]]]) + + # end paren + test = """ + one"test"; + ("two"); + "test")red; + "test")"blue"; + "test")"three; + (one"test")one; + one"; + one"test; + one"test"one; + """ + parsed = loads(test) + self.assertEqual(parsed, [ + ['one"test"'], + ['("two")'], + ['"test")red'], + ['"test")"blue"'], + ['"test")"three'], + ['(one"test")one'], + ['one"'], + ['one"test'], + ['one"test"one'] + ]) + self.assertRaises(ParseException, loads, r'"test"one;') # fails + self.assertRaises(ParseException, loads, r'"test;') # fails + + # newlines + test = """ + server_name foo.example.com bar.example.com \ + baz.example.com qux.example.com; + server_name foo.example.com bar.example.com + baz.example.com qux.example.com; + """ + parsed = loads(test) + self.assertEqual(parsed, [ + ['server_name', 'foo.example.com', 'bar.example.com', + 'baz.example.com', 'qux.example.com'], + ['server_name', 'foo.example.com', 'bar.example.com', + 'baz.example.com', 'qux.example.com'] + ]) + + # variable weirdness + parsed = loads("directive $var;") + self.assertEqual(parsed, [['directive', '$var']]) + self.assertRaises(ParseException, loads, "server {server_name test.com};") + self.assertRaises(ParseException, loads, "directive ${var};") + + class TestUnspacedList(unittest.TestCase): """Test the UnspacedList data structure""" def setUp(self): @@ -237,18 +402,18 @@ class TestUnspacedList(unittest.TestCase): ['\n ', 'listen', ' ', '127.0.0.1'], ['\n ', 'server_name', ' ', '.example.com'], ['\n ', 'server_name', ' ', 'example.*'], '\n', - ['listen', ' ', '5001 ssl']]) + ['listen', ' ', '5001', ' ', 'ssl']]) x.insert(5, "FROGZ") self.assertEqual(x, [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'], 'FROGZ']) + ['listen', '5001', 'ssl'], 'FROGZ']) self.assertEqual(x.spaced, [['\n ', 'listen', ' ', '69.50.225.155:9000'], ['\n ', 'listen', ' ', '127.0.0.1'], ['\n ', 'server_name', ' ', '.example.com'], ['\n ', 'server_name', ' ', 'example.*'], '\n', - ['listen', ' ', '5001 ssl'], + ['listen', ' ', '5001', ' ', 'ssl'], 'FROGZ']) def test_rawlists(self): diff --git a/certbot-nginx/certbot_nginx/tests/obj_test.py b/certbot-nginx/certbot_nginx/tests/obj_test.py index 069f860d6..ba136bb78 100644 --- a/certbot-nginx/certbot_nginx/tests/obj_test.py +++ b/certbot-nginx/certbot_nginx/tests/obj_test.py @@ -108,8 +108,8 @@ class VirtualHostTest(unittest.TestCase): from certbot_nginx.obj import Addr raw1 = [ ['listen', '69.50.225.155:9000'], - [['if', '($scheme != "https") '], - [['return', '301 https://$host$request_uri']] + [['if', '($scheme', '!=', '"https") '], + [['return', '301', 'https://$host$request_uri']] ], ['#', ' managed by Certbot'] ] @@ -119,8 +119,8 @@ class VirtualHostTest(unittest.TestCase): set(['localhost']), raw1, []) raw2 = [ ['listen', '69.50.225.155:9000'], - [['if', '($scheme != "https") '], - [['return', '301 https://$host$request_uri']] + [['if', '($scheme', '!=', '"https") '], + [['return', '301', 'https://$host$request_uri']] ] ] self.vhost2 = VirtualHost( @@ -129,7 +129,7 @@ class VirtualHostTest(unittest.TestCase): set(['localhost']), raw2, []) raw3 = [ ['listen', '69.50.225.155:9000'], - ['rewrite', '^(.*)$ $scheme://www.domain.com$1 permanent;'] + ['rewrite', '^(.*)$', '$scheme://www.domain.com$1', 'permanent'] ] self.vhost3 = VirtualHost( "filep", @@ -181,7 +181,9 @@ class VirtualHostTest(unittest.TestCase): ['#', ' managed by Certbot'], ['ssl_certificate_key', '/etc/letsencrypt/live/two.functorkitten.xyz/privkey.pem'], ['#', ' managed by Certbot'], - [['if', '($scheme != "https")'], [['return', '301 https://$host$request_uri']]], + [['if', '($scheme', '!=', '"https")'], + [['return', '301', 'https://$host$request_uri']] + ], ['#', ' managed by Certbot'], []] vhost_haystack = VirtualHost( "filp", @@ -195,7 +197,9 @@ class VirtualHostTest(unittest.TestCase): ['#', ' managed by Certbot'], ['ssl_certificate_key', '/etc/letsencrypt/live/two.functorkitten.xyz/privkey.pem'], ['#', ' managed by Certbot'], - [['if', '($scheme != "https")'], [['return', '302 https://$host$request_uri']]], + [['if', '($scheme', '!=', '"https")'], + [['return', '302', 'https://$host$request_uri']] + ], ['#', ' managed by Certbot'], []] vhost_bad_haystack = VirtualHost( "filp", diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 6a3f2f1de..8d20faa38 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -52,7 +52,7 @@ class NginxParserTest(util.NginxTest): 'sites-enabled/sslon.com', 'sites-enabled/globalssl.com']]), set(nparser.parsed.keys())) - self.assertEqual([['server_name', 'somename alias another.alias']], + self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']], nparser.parsed[nparser.abs_path('server.conf')]) self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], @@ -168,16 +168,16 @@ class NginxParserTest(util.NginxTest): [['location', '/'], [['root', 'html'], ['index', 'index.html index.htm']]] ], None) self.assertFalse(nparser.has_ssl_on_directive(mock_vhost)) - mock_vhost.raw = [['listen', '*:80 default_server ssl'], - ['server_name', '*.www.foo.com *.www.example.com'], + mock_vhost.raw = [['listen', '*:80', 'default_server', 'ssl'], + ['server_name', '*.www.foo.com', '*.www.example.com'], ['root', '/home/ubuntu/sites/foo/']] self.assertFalse(nparser.has_ssl_on_directive(mock_vhost)) mock_vhost.raw = [['listen', '80 ssl'], - ['server_name', '*.www.foo.com *.www.example.com']] + ['server_name', '*.www.foo.com', '*.www.example.com']] self.assertFalse(nparser.has_ssl_on_directive(mock_vhost)) mock_vhost.raw = [['listen', '80'], ['ssl', 'on'], - ['server_name', '*.www.foo.com *.www.example.com']] + ['server_name', '*.www.foo.com', '*.www.example.com']] self.assertTrue(nparser.has_ssl_on_directive(mock_vhost)) def test_add_server_directives(self): @@ -309,7 +309,7 @@ class NginxParserTest(util.NginxTest): self.assertFalse(server['ssl']) server = parser._parse_server_raw([ #pylint: disable=protected-access - ['listen', '443 ssl'] + ['listen', '443', 'ssl'] ]) self.assertTrue(server['ssl']) @@ -341,7 +341,7 @@ class NginxParserTest(util.NginxTest): self.assertEqual(nginxparser.UnspacedList(nparser.loc["ssl_options"]), [['ssl_session_cache', 'shared:le_nginx_SSL:1m'], ['ssl_session_timeout', '1440m'], - ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], + ['ssl_protocols', 'TLSv1', 'TLSv1.1', 'TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-'+ 'AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256'+ diff --git a/certbot-nginx/certbot_nginx/tls_sni_01.py b/certbot-nginx/certbot_nginx/tls_sni_01.py index eedd97c03..2e8125911 100644 --- a/certbot-nginx/certbot_nginx/tls_sni_01.py +++ b/certbot-nginx/certbot_nginx/tls_sni_01.py @@ -96,11 +96,12 @@ class NginxTlsSni01(common.TLSSNI01): bucket_directive = ['\n', 'server_names_hash_bucket_size', ' ', '128'] main = self.configurator.parser.parsed[root] - for key, body in main: - if key == ['http']: + for line in main: + if line[0] == ['http']: + body = line[1] found_bucket = False - for k, _ in body: - if k == bucket_directive[1]: + for inner_line in body: + if inner_line[0] == bucket_directive[1]: found_bucket = True if not found_bucket: body.insert(0, bucket_directive) From 2e102ec9f7523457da30813062f2827608001eec Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Sat, 25 Mar 2017 11:39:19 -0700 Subject: [PATCH 26/40] Review feedback. --- docs/packaging.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/packaging.rst b/docs/packaging.rst index c3cdfb579..780eb313c 100644 --- a/docs/packaging.rst +++ b/docs/packaging.rst @@ -28,7 +28,7 @@ Notes for package maintainers 1. Do not package ``certbot-compatibility-test`` or ``letshelp-certbot`` - it's only used internally. -2. If you'd like to include automated renewal in your package ``certbot renew -q`` should be added to crontab or systemd timer. Add a random per-system offset to avoid having a large number of clients hit Let's Encrypt's servers simultaneously. +2. If you'd like to include automated renewal in your package ``certbot renew -q`` should be added to crontab or systemd timer. Additionally you should include a random per-machine time offset to avoid having a large number of your clients hit Let's Encrypt's servers simultaneously. 3. ``jws`` is an internal script for ``acme`` module and it doesn't have to be packaged - it's mostly for debugging: you can use it as ``echo foo | jws sign | jws verify``. From 5c93ceb67550ca4dd6e34d3edcda580cd4ab04c3 Mon Sep 17 00:00:00 2001 From: Damien Tournoud Date: Mon, 27 Mar 2017 18:24:05 +0200 Subject: [PATCH 27/40] acme: Make the network timeout configurable (#4237) This follows up on https://github.com/certbot/certbot/pull/4217, but allows users to override the default setting. --- acme/acme/client.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 40291e115..0c8886fc6 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -33,6 +33,8 @@ if sys.version_info < (2, 7, 9): # pragma: no cover import urllib3.contrib.pyopenssl # pylint: disable=import-error urllib3.contrib.pyopenssl.inject_into_urllib3() +DEFAULT_NETWORK_TIMEOUT = 45 + DER_CONTENT_TYPE = 'application/pkix-cert' @@ -503,13 +505,14 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes REPLAY_NONCE_HEADER = 'Replay-Nonce' def __init__(self, key, alg=jose.RS256, verify_ssl=True, - user_agent='acme-python'): + user_agent='acme-python', timeout=DEFAULT_NETWORK_TIMEOUT): self.key = key self.alg = alg self.verify_ssl = verify_ssl self._nonces = set() self.user_agent = user_agent self.session = requests.Session() + self._default_timeout = timeout def __del__(self): self.session.close() @@ -608,7 +611,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes kwargs['verify'] = self.verify_ssl kwargs.setdefault('headers', {}) kwargs['headers'].setdefault('User-Agent', self.user_agent) - kwargs.setdefault('timeout', 45) # timeout after 45 seconds + kwargs.setdefault('timeout', self._default_timeout) response = self.session.request(method, url, *args, **kwargs) # If content is DER, log the base64 of it instead of raw bytes, to keep # binary data out of the logs. From 7d57e3104a2d7e514ae19c99d2a41094f3bfc5eb Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Mon, 27 Mar 2017 12:20:51 -0700 Subject: [PATCH 28/40] Ensure --fulchain-path gets put under paths in --help all --- certbot/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/cli.py b/certbot/cli.py index 5b8711da6..0faf4a7c6 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1155,7 +1155,7 @@ def _paths_parser(helpful): default_cp = None if verb == "certonly": default_cp = flag_default("auth_chain_path") - add(["install", "paths"], "--fullchain-path", default=default_cp, type=os.path.abspath, + add(["paths", "install"], "--fullchain-path", default=default_cp, type=os.path.abspath, help="Accompanying path to a full certificate chain (cert plus chain).") add("paths", "--chain-path", default=default_cp, type=os.path.abspath, help="Accompanying path to a certificate chain.") From 1c51ae25887f2dc3168a38d1f0042363cd7ac1e3 Mon Sep 17 00:00:00 2001 From: Zach Shepherd Date: Mon, 27 Mar 2017 13:58:17 -0700 Subject: [PATCH 29/40] Pin python-augeas version to avoid error with 1.0.0 (#4422) When running ./tools/venv.sh with 1.0.0 (now the latest version), I encountered: build/temp.linux-x86_64-2.7/augeas.c:434:35: fatal error: augeas.h: No such file or directory --- certbot-apache/setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-apache/setup.py b/certbot-apache/setup.py index db8cb11db..cb35d2686 100644 --- a/certbot-apache/setup.py +++ b/certbot-apache/setup.py @@ -11,7 +11,7 @@ install_requires = [ 'acme=={0}'.format(version), 'certbot=={0}'.format(version), 'mock', - 'python-augeas', + 'python-augeas<=0.5.0', # For pkg_resources. >=1.0 so pip resolves it to a version cryptography # will tolerate; see #2599: 'setuptools>=1.0', From e9608945c3622540ea0e0dbfb79fd62b7e5d4b47 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Mon, 27 Mar 2017 14:47:14 -0700 Subject: [PATCH 30/40] Change registering unsafely without email logging level to info (#4425) * Change registering unsafely without email logging level to info * update test with new behavior --- certbot/client.py | 2 +- certbot/tests/client_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index bd1971371..f6de26e3d 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -118,7 +118,7 @@ def register(config, account_storage, tos_cb=None): logger.warning(msg) raise errors.Error(msg) if not config.dry_run: - logger.warning("Registering without email!") + logger.info("Registering without email!") # Each new registration shall use a fresh new key key = jose.JWKRSA(key=jose.ComparableRSAKey( diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index c61f1fa4e..8b72e1df7 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -102,7 +102,7 @@ class RegisterTest(unittest.TestCase): self.config.register_unsafely_without_email = True self.config.dry_run = False self._call() - mock_logger.warning.assert_called_once_with(mock.ANY) + mock_logger.info.assert_called_once_with(mock.ANY) self.assertTrue(mock_handle.called) def test_unsupported_error(self): From 07f95e619711af5d6af6d4771e8ba445538ddb0f Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 27 Mar 2017 15:14:07 -0700 Subject: [PATCH 31/40] Improvements to example cli.ini --- examples/cli.ini | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/examples/cli.ini b/examples/cli.ini index 63af3cc49..dbaa9c599 100644 --- a/examples/cli.ini +++ b/examples/cli.ini @@ -1,6 +1,11 @@ # This is an example of the kind of things you can do in a configuration file. # All flags used by the client can be configured here. Run Certbot with # "--help" to learn more about the available options. +# +# Note that these options apply automatically to all use of Certbot for +# obtaining or renewing certificates, so options specific to a single +# certificate on a system with several certificates should not be placed +# here. # Use a 4096 bit RSA key instead of 2048 rsa-key-size = 4096 @@ -8,13 +13,6 @@ rsa-key-size = 4096 # Uncomment and update to register with the specified e-mail address # email = foo@example.com -# Uncomment and update to generate certificates for the specified -# domains. -# domains = example.com, www.example.com - -# Uncomment to use a text interface instead of ncurses -# text = True - # Uncomment to use the standalone authenticator on port 443 # authenticator = standalone # standalone-supported-challenges = tls-sni-01 From ece68a1864b72616565711dcab8f3cfd4f388718 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Mon, 27 Mar 2017 15:19:03 -0700 Subject: [PATCH 32/40] Update Nginx ciphersuites to use Mozilla Intermediate (#4426) * Update Nginx ciphersuites to use Mozilla intermediate * update tests to match new behavior --- .../certbot_nginx/options-ssl-nginx.conf | 2 +- .../certbot_nginx/tests/parser_test.py | 21 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf index 89c920b3e..e1839909d 100644 --- a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf +++ b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf @@ -4,4 +4,4 @@ ssl_session_timeout 1440m; ssl_protocols TLSv1 TLSv1.1 TLSv1.2; ssl_prefer_server_ciphers on; -ssl_ciphers "ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"; +ssl_ciphers "ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS"; diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 8d20faa38..9c2b8656e 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -343,14 +343,19 @@ class NginxParserTest(util.NginxTest): ['ssl_session_timeout', '1440m'], ['ssl_protocols', 'TLSv1', 'TLSv1.1', 'TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], - ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-'+ - 'AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256'+ - '-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384'+ - ' ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384'+ - ' ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-'+ - 'AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM'+ - '-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-'+ - 'AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"'] + ['ssl_ciphers', '"ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-'+ + 'RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:'+ + 'ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-'+ + 'SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-'+ + 'SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-'+ + 'SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:'+ + 'ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-'+ + 'AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:'+ + 'DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-'+ + 'SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-'+ + 'RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:'+ + 'AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:'+ + 'AES256-SHA:DES-CBC3-SHA:!DSS"'] ]) if __name__ == "__main__": From 67e11ae1d895c5b0488872b091d7ef29527f74e3 Mon Sep 17 00:00:00 2001 From: Zach Shepherd Date: Wed, 29 Mar 2017 10:01:16 -0700 Subject: [PATCH 33/40] tests: deduplicate temporary directory code (#4078) (#4297) Introduce a test class to deduplicate temporary directory setup and teardown in testing code and update existing test code to use this new class. --- certbot/tests/account_test.py | 13 +++-- certbot/tests/cert_manager_test.py | 13 ++--- certbot/tests/cli_test.py | 7 +-- certbot/tests/crypto_util_test.py | 30 ++++++----- certbot/tests/display/completer_test.py | 20 +++----- certbot/tests/display/ops_test.py | 10 ++-- certbot/tests/main_test.py | 67 +++++++++++-------------- certbot/tests/renewal_test.py | 10 ++-- certbot/tests/storage_test.py | 10 ++-- certbot/tests/util.py | 11 ++++ certbot/tests/util_test.py | 62 +++++++++-------------- 11 files changed, 113 insertions(+), 140 deletions(-) diff --git a/certbot/tests/account_test.py b/certbot/tests/account_test.py index 7d335b09b..11d14132d 100644 --- a/certbot/tests/account_test.py +++ b/certbot/tests/account_test.py @@ -4,7 +4,6 @@ import json import os import shutil import stat -import tempfile import unittest import mock @@ -17,6 +16,8 @@ from certbot import errors from certbot.tests import util +from certbot.tests.util import TempDirTestCase + KEY = jose.JWKRSA.load(util.load_vector("rsa512_key_2.pem")) @@ -98,13 +99,14 @@ class AccountMemoryStorageTest(unittest.TestCase): self.assertEqual([account], self.storage.find_all()) -class AccountFileStorageTest(unittest.TestCase): +class AccountFileStorageTest(TempDirTestCase): """Tests for certbot.account.AccountFileStorage.""" def setUp(self): - self.tmp = tempfile.mkdtemp() + super(AccountFileStorageTest, self).setUp() + self.config = mock.MagicMock( - accounts_dir=os.path.join(self.tmp, "accounts")) + accounts_dir=os.path.join(self.tempdir, "accounts")) from certbot.account import AccountFileStorage self.storage = AccountFileStorage(self.config) @@ -118,9 +120,6 @@ class AccountFileStorageTest(unittest.TestCase): self.mock_client = mock.MagicMock() self.mock_client.directory.new_authz = new_authzr_uri - def tearDown(self): - shutil.rmtree(self.tmp) - def test_init_creates_dir(self): self.assertTrue(os.path.isdir(self.config.accounts_dir)) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 473970870..b5731fbd6 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -18,11 +18,14 @@ from certbot.storage import ALL_FOUR from certbot.tests import storage_test from certbot.tests import util as test_util -class BaseCertManagerTest(unittest.TestCase): +from certbot.tests.util import TempDirTestCase + + +class BaseCertManagerTest(TempDirTestCase): """Base class for setting up Cert Manager tests. """ def setUp(self): - self.tempdir = tempfile.mkdtemp() + super(BaseCertManagerTest, self).setUp() os.makedirs(os.path.join(self.tempdir, "renewal")) @@ -68,9 +71,6 @@ class BaseCertManagerTest(unittest.TestCase): config.write() return config - def tearDown(self): - shutil.rmtree(self.tempdir) - class UpdateLiveSymlinksTest(BaseCertManagerTest): """Tests for certbot.cert_manager.update_live_symlinks @@ -437,9 +437,6 @@ class DuplicativeCertsTest(storage_test.BaseRenewableCertTest): self.config.write() self._write_out_ex_kinds() - def tearDown(self): - shutil.rmtree(self.tempdir) - @mock.patch('certbot.util.make_or_verify_dir') def test_find_duplicative_names(self, unused_makedir): from certbot.cert_manager import find_duplicative_certs diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 5f4a4e2c7..498bd309d 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -15,17 +15,18 @@ from certbot import constants from certbot import errors from certbot.plugins import disco +from certbot.tests.util import TempDirTestCase + PLUGINS = disco.PluginsRegistry.find_all() -class TestReadFile(unittest.TestCase): +class TestReadFile(TempDirTestCase): '''Test cli.read_file''' _multiprocess_can_split_ = True def test_read_file(self): - tmp_dir = tempfile.mkdtemp() - rel_test_path = os.path.relpath(os.path.join(tmp_dir, 'foo')) + rel_test_path = os.path.relpath(os.path.join(self.tempdir, 'foo')) self.assertRaises( argparse.ArgumentTypeError, cli.read_file, rel_test_path) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index a832d0494..df5d9f0f6 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -1,8 +1,6 @@ """Tests for certbot.crypto_util.""" import logging import os -import shutil -import tempfile import unittest import OpenSSL @@ -22,18 +20,20 @@ CERT = test_util.load_vector('cert.pem') SAN_CERT = test_util.load_vector('cert-san.pem') -class InitSaveKeyTest(unittest.TestCase): +class InitSaveKeyTest(test_util.TempDirTestCase): """Tests for certbot.crypto_util.init_save_key.""" def setUp(self): + super(InitSaveKeyTest, self).setUp() + logging.disable(logging.CRITICAL) zope.component.provideUtility( mock.Mock(strict_permissions=True, dry_run=False), interfaces.IConfig) - self.key_dir = tempfile.mkdtemp('key_dir') def tearDown(self): + super(InitSaveKeyTest, self).tearDown() + logging.disable(logging.NOTSET) - shutil.rmtree(self.key_dir) @classmethod def _call(cls, key_size, key_dir): @@ -43,10 +43,10 @@ class InitSaveKeyTest(unittest.TestCase): @mock.patch('certbot.crypto_util.make_key') def test_success(self, mock_make): mock_make.return_value = b'key_pem' - key = self._call(1024, self.key_dir) + key = self._call(1024, self.tempdir) self.assertEqual(key.pem, b'key_pem') self.assertTrue('key-certbot.pem' in key.file) - self.assertTrue(os.path.exists(os.path.join(self.key_dir, key.file))) + self.assertTrue(os.path.exists(os.path.join(self.tempdir, key.file))) @mock.patch('certbot.crypto_util.make_key') def test_success_dry_run(self, mock_make): @@ -54,27 +54,25 @@ class InitSaveKeyTest(unittest.TestCase): mock.Mock(strict_permissions=True, dry_run=True), interfaces.IConfig) mock_make.return_value = b'key_pem' - key = self._call(1024, self.key_dir) + key = self._call(1024, self.tempdir) self.assertEqual(key.pem, b'key_pem') self.assertTrue(key.file is None) @mock.patch('certbot.crypto_util.make_key') def test_key_failure(self, mock_make): mock_make.side_effect = ValueError - self.assertRaises(ValueError, self._call, 431, self.key_dir) + self.assertRaises(ValueError, self._call, 431, self.tempdir) -class InitSaveCSRTest(unittest.TestCase): +class InitSaveCSRTest(test_util.TempDirTestCase): """Tests for certbot.crypto_util.init_save_csr.""" def setUp(self): + super(InitSaveCSRTest, self).setUp() + zope.component.provideUtility( mock.Mock(strict_permissions=True, dry_run=False), interfaces.IConfig) - self.csr_dir = tempfile.mkdtemp('csr_dir') - - def tearDown(self): - shutil.rmtree(self.csr_dir) @mock.patch('certbot.crypto_util.make_csr') @mock.patch('certbot.crypto_util.util.make_or_verify_dir') @@ -84,7 +82,7 @@ class InitSaveCSRTest(unittest.TestCase): mock_csr.return_value = (b'csr_pem', b'csr_der') csr = init_save_csr( - mock.Mock(pem='dummy_key'), 'example.com', self.csr_dir, + mock.Mock(pem='dummy_key'), 'example.com', self.tempdir, 'csr-certbot.pem') self.assertEqual(csr.data, b'csr_der') @@ -101,7 +99,7 @@ class InitSaveCSRTest(unittest.TestCase): mock_csr.return_value = (b'csr_pem', b'csr_der') csr = init_save_csr( - mock.Mock(pem='dummy_key'), 'example.com', self.csr_dir, + mock.Mock(pem='dummy_key'), 'example.com', self.tempdir, 'csr-certbot.pem') self.assertEqual(csr.data, b'csr_der') diff --git a/certbot/tests/display/completer_test.py b/certbot/tests/display/completer_test.py index 16805314c..333acf2b3 100644 --- a/certbot/tests/display/completer_test.py +++ b/certbot/tests/display/completer_test.py @@ -1,31 +1,30 @@ """Test certbot.display.completer.""" import os import readline -import shutil import string import sys -import tempfile import unittest import mock from six.moves import reload_module # pylint: disable=import-error +from certbot.tests.util import TempDirTestCase -class CompleterTest(unittest.TestCase): +class CompleterTest(TempDirTestCase): """Test certbot.display.completer.Completer.""" def setUp(self): - self.temp_dir = tempfile.mkdtemp() + super(CompleterTest, self).setUp() # directories must end with os.sep for completer to # search inside the directory for possible completions - if self.temp_dir[-1] != os.sep: - self.temp_dir += os.sep + if self.tempdir[-1] != os.sep: + self.tempdir += os.sep self.paths = [] # create some files and directories in temp_dir for c in string.ascii_lowercase: - path = os.path.join(self.temp_dir, c) + path = os.path.join(self.tempdir, c) self.paths.append(path) if ord(c) % 2: os.mkdir(path) @@ -33,21 +32,18 @@ class CompleterTest(unittest.TestCase): with open(path, 'w'): pass - def tearDown(self): - shutil.rmtree(self.temp_dir) - def test_complete(self): from certbot.display import completer my_completer = completer.Completer() num_paths = len(self.paths) for i in range(num_paths): - completion = my_completer.complete(self.temp_dir, i) + completion = my_completer.complete(self.tempdir, i) self.assertTrue(completion in self.paths) self.paths.remove(completion) self.assertFalse(self.paths) - completion = my_completer.complete(self.temp_dir, num_paths) + completion = my_completer.complete(self.tempdir, num_paths) self.assertEqual(completion, None) def test_import_error(self): diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index f2a9b3d07..89cd9e43d 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -2,7 +2,6 @@ """Test certbot.display.ops.""" import os import sys -import tempfile import unittest import mock @@ -87,18 +86,19 @@ class GetEmailTest(unittest.TestCase): self.assertTrue(invalid_txt in mock_input.call_args[0][0]) -class ChooseAccountTest(unittest.TestCase): +class ChooseAccountTest(test_util.TempDirTestCase): """Tests for certbot.display.ops.choose_account.""" def setUp(self): + super(ChooseAccountTest, self).setUp() + zope.component.provideUtility(display_util.FileDisplay(sys.stdout, False)) - self.accounts_dir = tempfile.mkdtemp("accounts") - self.account_keys_dir = os.path.join(self.accounts_dir, "keys") + self.account_keys_dir = os.path.join(self.tempdir, "keys") os.makedirs(self.account_keys_dir, 0o700) self.config = mock.MagicMock( - accounts_dir=self.accounts_dir, + accounts_dir=self.tempdir, account_keys_dir=self.account_keys_dir, server="certbot-demo.org") self.key = KEY diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 5707231b7..02f241255 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -7,7 +7,6 @@ import mock import multiprocessing import os import shutil -import tempfile import traceback import unittest import datetime @@ -220,13 +219,14 @@ class FindDomainsOrCertnameTest(unittest.TestCase): (["one.com", "two.com"], "one.com")) -class RevokeTest(unittest.TestCase): +class RevokeTest(test_util.TempDirTestCase): """Tests for certbot.main.revoke.""" def setUp(self): - self.tempdir_path = tempfile.mkdtemp() - shutil.copy(CERT_PATH, self.tempdir_path) - self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir_path, + super(RevokeTest, self).setUp() + + shutil.copy(CERT_PATH, self.tempdir) + self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir, 'cert.pem')) self.patches = [ @@ -252,7 +252,8 @@ class RevokeTest(unittest.TestCase): self.mock_determine_account.return_value = (self.acc, None) def tearDown(self): - shutil.rmtree(self.tempdir_path) + super(RevokeTest, self).tearDown() + for patch in self.patches: patch.stop() @@ -288,15 +289,14 @@ class RevokeTest(unittest.TestCase): self.mock_success_revoke.assert_not_called() -class SetupLogFileHandlerTest(unittest.TestCase): +class SetupLogFileHandlerTest(test_util.TempDirTestCase): """Tests for certbot.main.setup_log_file_handler.""" def setUp(self): - self.config = mock.Mock(spec_set=['logs_dir'], - logs_dir=tempfile.mkdtemp()) + super(SetupLogFileHandlerTest, self).setUp() - def tearDown(self): - shutil.rmtree(self.config.logs_dir) + self.config = mock.Mock(spec_set=['logs_dir'], + logs_dir=self.tempdir) def _call(self, *args, **kwargs): from certbot.main import setup_log_file_handler @@ -309,18 +309,17 @@ class SetupLogFileHandlerTest(unittest.TestCase): self.config, "test.log", "%s") -class SetupLoggingTest(unittest.TestCase): +class SetupLoggingTest(test_util.TempDirTestCase): """Tests for certbot.main.setup_logging.""" def setUp(self): + super(SetupLoggingTest, self).setUp() + self.config = mock.Mock( - logs_dir=tempfile.mkdtemp(), + logs_dir=self.tempdir, noninteractive_mode=False, quiet=False, verbose_count=constants.CLI_DEFAULTS['verbose_count']) - def tearDown(self): - shutil.rmtree(self.config.logs_dir) - @classmethod def _call(cls, *args, **kwargs): from certbot.main import setup_logging @@ -346,21 +345,15 @@ class SetupLoggingTest(unittest.TestCase): isinstance(cli_handler, colored_logging.StreamHandler)) -class MakeOrVerifyCoreDirTest(unittest.TestCase): +class MakeOrVerifyCoreDirTest(test_util.TempDirTestCase): """Tests for certbot.main.make_or_verify_core_dir.""" - def setUp(self): - self.dir = tempfile.mkdtemp() - - def tearDown(self): - shutil.rmtree(self.dir) - def _call(self, *args, **kwargs): from certbot.main import make_or_verify_core_dir return make_or_verify_core_dir(*args, **kwargs) def test_success(self): - new_dir = os.path.join(self.dir, 'new') + new_dir = os.path.join(self.tempdir, 'new') self._call(new_dir, 0o700, os.geteuid(), False) self.assertTrue(os.path.exists(new_dir)) @@ -368,7 +361,7 @@ class MakeOrVerifyCoreDirTest(unittest.TestCase): def test_failure(self, mock_make_or_verify): mock_make_or_verify.side_effect = OSError self.assertRaises(errors.Error, self._call, - self.dir, 0o700, os.geteuid(), False) + self.tempdir, 0o700, os.geteuid(), False) class DetermineAccountTest(unittest.TestCase): @@ -441,24 +434,26 @@ class DetermineAccountTest(unittest.TestCase): self.assertEqual('other email', self.config.email) -class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods +class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-methods """Tests for different commands.""" def setUp(self): - self.tmp_dir = tempfile.mkdtemp() - self.config_dir = os.path.join(self.tmp_dir, 'config') - self.work_dir = os.path.join(self.tmp_dir, 'work') - self.logs_dir = os.path.join(self.tmp_dir, 'logs') + super(MainTest, self).setUp() + + self.config_dir = os.path.join(self.tempdir, 'config') + self.work_dir = os.path.join(self.tempdir, 'work') + self.logs_dir = os.path.join(self.tempdir, 'logs') os.mkdir(self.logs_dir) self.standard_args = ['--config-dir', self.config_dir, '--work-dir', self.work_dir, '--logs-dir', self.logs_dir, '--text', - '--lock-path', os.path.join(self.tmp_dir, 'certbot.lock')] + '--lock-path', os.path.join(self.tempdir, 'certbot.lock')] def tearDown(self): # Reset globals in cli reload_module(cli) - shutil.rmtree(self.tmp_dir) + + super(MainTest, self).tearDown() def _call(self, args, stdout=None): "Run the cli with output streams and actual client mocked out" @@ -1310,15 +1305,13 @@ class TestHandleException(unittest.TestCase): traceback.format_exception_only(KeyboardInterrupt, interrupt))) -class TestAcquireFileLock(unittest.TestCase): +class TestAcquireFileLock(test_util.TempDirTestCase): """Test main.acquire_file_lock.""" def setUp(self): - self.tempdir = tempfile.mkdtemp() - self.lock_path = os.path.join(self.tempdir, 'certbot.lock') + super(TestAcquireFileLock, self).setUp() - def tearDown(self): - shutil.rmtree(self.tempdir) + self.lock_path = os.path.join(self.tempdir, 'certbot.lock') @mock.patch('certbot.main.logger') def test_bad_path(self, mock_logger): diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index d97e05783..de3efe39c 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -2,8 +2,6 @@ import os import mock import unittest -import shutil -import tempfile from acme import challenges @@ -14,13 +12,11 @@ from certbot import storage from certbot.tests import util -class RenewalTest(unittest.TestCase): +class RenewalTest(util.TempDirTestCase): def setUp(self): - self.tmp_dir = tempfile.mkdtemp() - self.config_dir = os.path.join(self.tmp_dir, 'config') + super(RenewalTest, self).setUp() - def tearDown(self): - shutil.rmtree(self.tmp_dir) + self.config_dir = os.path.join(self.tempdir, 'config') @mock.patch('certbot.cli.set_by_cli') def test_ancient_webroot_renewal_conf(self, mock_set_by_cli): diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index f7bde012d..6635461aa 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -3,7 +3,6 @@ import datetime import os import shutil -import tempfile import unittest import configobj @@ -36,7 +35,7 @@ def fill_with_sample_data(rc_object): f.write(kind) -class BaseRenewableCertTest(unittest.TestCase): +class BaseRenewableCertTest(util.TempDirTestCase): """Base class for setting up Renewable Cert tests. .. note:: It may be required to write out self.config for @@ -47,7 +46,8 @@ class BaseRenewableCertTest(unittest.TestCase): def setUp(self): from certbot import storage - self.tempdir = tempfile.mkdtemp() + + super(BaseRenewableCertTest, self).setUp() self.cli_config = configuration.NamespaceConfig( namespace=mock.MagicMock( @@ -91,9 +91,6 @@ class BaseRenewableCertTest(unittest.TestCase): check.return_value = True self.test_rc = storage.RenewableCert(config.filename, self.cli_config) - def tearDown(self): - shutil.rmtree(self.tempdir) - def _write_out_kind(self, kind, ver, value=None): link = getattr(self.test_rc, kind) if os.path.lexists(link): @@ -798,6 +795,7 @@ class DeleteFilesTest(BaseRenewableCertTest): """Tests for certbot.storage.delete_files""" def setUp(self): super(DeleteFilesTest, self).setUp() + for kind in ALL_FOUR: kind_path = os.path.join(self.tempdir, "live", "example.org", kind + ".pem") diff --git a/certbot/tests/util.py b/certbot/tests/util.py index 092807b56..d58834335 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -6,6 +6,7 @@ import os import pkg_resources import shutil +import tempfile import unittest from cryptography.hazmat.backends import default_backend @@ -230,3 +231,13 @@ def _assert_valid_call(*args, **kwargs): # pylint: disable=star-args display_util.assert_valid_call(*assert_args, **assert_kwargs) + + +class TempDirTestCase(unittest.TestCase): + """Base test class which sets up and tears down a temporary directory""" + + def setUp(self): + self.tempdir = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.tempdir) diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 13a91dfb8..480120772 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -2,9 +2,7 @@ import argparse import errno import os -import shutil import stat -import tempfile import unittest import mock @@ -75,7 +73,7 @@ class ExeExistsTest(unittest.TestCase): self.assertFalse(self._call("exe")) -class MakeOrVerifyDirTest(unittest.TestCase): +class MakeOrVerifyDirTest(test_util.TempDirTestCase): """Tests for certbot.util.make_or_verify_dir. Note that it is not possible to test for a wrong directory owner, @@ -84,21 +82,19 @@ class MakeOrVerifyDirTest(unittest.TestCase): """ def setUp(self): - self.root_path = tempfile.mkdtemp() - self.path = os.path.join(self.root_path, "foo") + super(MakeOrVerifyDirTest, self).setUp() + + self.path = os.path.join(self.tempdir, "foo") os.mkdir(self.path, 0o400) self.uid = os.getuid() - def tearDown(self): - shutil.rmtree(self.root_path, ignore_errors=True) - def _call(self, directory, mode): from certbot.util import make_or_verify_dir return make_or_verify_dir(directory, mode, self.uid, strict=True) def test_creates_dir_when_missing(self): - path = os.path.join(self.root_path, "bar") + path = os.path.join(self.tempdir, "bar") self._call(path, 0o650) self.assertTrue(os.path.isdir(path)) self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o650) @@ -116,7 +112,7 @@ class MakeOrVerifyDirTest(unittest.TestCase): self.assertRaises(OSError, self._call, "bar", 12312312) -class CheckPermissionsTest(unittest.TestCase): +class CheckPermissionsTest(test_util.TempDirTestCase): """Tests for certbot.util.check_permissions. Note that it is not possible to test for a wrong file owner, @@ -125,34 +121,30 @@ class CheckPermissionsTest(unittest.TestCase): """ def setUp(self): - _, self.path = tempfile.mkstemp() - self.uid = os.getuid() + super(CheckPermissionsTest, self).setUp() - def tearDown(self): - os.remove(self.path) + self.uid = os.getuid() def _call(self, mode): from certbot.util import check_permissions - return check_permissions(self.path, mode, self.uid) + return check_permissions(self.tempdir, mode, self.uid) def test_ok_mode(self): - os.chmod(self.path, 0o600) + os.chmod(self.tempdir, 0o600) self.assertTrue(self._call(0o600)) def test_wrong_mode(self): - os.chmod(self.path, 0o400) + os.chmod(self.tempdir, 0o400) self.assertFalse(self._call(0o600)) -class UniqueFileTest(unittest.TestCase): +class UniqueFileTest(test_util.TempDirTestCase): """Tests for certbot.util.unique_file.""" def setUp(self): - self.root_path = tempfile.mkdtemp() - self.default_name = os.path.join(self.root_path, "foo.txt") + super(UniqueFileTest, self).setUp() - def tearDown(self): - shutil.rmtree(self.root_path, ignore_errors=True) + self.default_name = os.path.join(self.tempdir, "foo.txt") def _call(self, mode=0o600): from certbot.util import unique_file @@ -177,9 +169,9 @@ class UniqueFileTest(unittest.TestCase): self.assertNotEqual(name1, name3) self.assertNotEqual(name2, name3) - self.assertEqual(os.path.dirname(name1), self.root_path) - self.assertEqual(os.path.dirname(name2), self.root_path) - self.assertEqual(os.path.dirname(name3), self.root_path) + self.assertEqual(os.path.dirname(name1), self.tempdir) + self.assertEqual(os.path.dirname(name2), self.tempdir) + self.assertEqual(os.path.dirname(name3), self.tempdir) basename1 = os.path.basename(name2) self.assertTrue(basename1.endswith("foo.txt")) @@ -196,23 +188,17 @@ except NameError: file_type = io.TextIOWrapper # type: ignore -class UniqueLineageNameTest(unittest.TestCase): +class UniqueLineageNameTest(test_util.TempDirTestCase): """Tests for certbot.util.unique_lineage_name.""" - def setUp(self): - self.root_path = tempfile.mkdtemp() - - def tearDown(self): - shutil.rmtree(self.root_path, ignore_errors=True) - def _call(self, filename, mode=0o777): from certbot.util import unique_lineage_name - return unique_lineage_name(self.root_path, filename, mode) + return unique_lineage_name(self.tempdir, filename, mode) def test_basic(self): f, path = self._call("wow") self.assertTrue(isinstance(f, file_type)) - self.assertEqual(os.path.join(self.root_path, "wow.conf"), path) + self.assertEqual(os.path.join(self.tempdir, "wow.conf"), path) def test_multiple(self): for _ in six.moves.range(10): @@ -237,15 +223,13 @@ class UniqueLineageNameTest(unittest.TestCase): self.assertRaises(OSError, self._call, "wow") -class SafelyRemoveTest(unittest.TestCase): +class SafelyRemoveTest(test_util.TempDirTestCase): """Tests for certbot.util.safely_remove.""" def setUp(self): - self.tmp = tempfile.mkdtemp() - self.path = os.path.join(self.tmp, "foo") + super(SafelyRemoveTest, self).setUp() - def tearDown(self): - shutil.rmtree(self.tmp) + self.path = os.path.join(self.tempdir, "foo") def _call(self): from certbot.util import safely_remove From 6fb78dab671f723f6b2b4a2d0846fbec0e9634c3 Mon Sep 17 00:00:00 2001 From: Yen Chi Hsuan Date: Thu, 30 Mar 2017 04:34:09 +0800 Subject: [PATCH 34/40] Fix Docker IP detection with different ifconfig output formats (#4376) --- tests/boulder-fetch.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/boulder-fetch.sh b/tests/boulder-fetch.sh index ef61fe3f5..d9a979667 100755 --- a/tests/boulder-fetch.sh +++ b/tests/boulder-fetch.sh @@ -12,5 +12,7 @@ fi cd ${BOULDERPATH} FAKE_DNS=$(ifconfig docker0 | grep "inet addr:" | cut -d: -f2 | awk '{ print $1}') +[ -z "$FAKE_DNS" ] && FAKE_DNS=$(ifconfig docker0 | grep "inet " | xargs | cut -d ' ' -f 2) +[ -z "$FAKE_DNS" ] && echo Unable to find the IP for docker0 && exit 1 sed -i "s/FAKE_DNS: .*/FAKE_DNS: ${FAKE_DNS}/" docker-compose.yml docker-compose up -d From d5f1edf2bb85cba4c0de18722b29e080fb43d4d9 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 29 Mar 2017 16:48:08 -0700 Subject: [PATCH 35/40] Dump Boulder logs on integration test failures. (#4442) Might help debug #4363. Also: make "bash" vs "sh" explicit move the paranoia flags (-ex) from the shebang into the body add -u (fail on unset variables) change _common to work with -u remove some env vars that were no longer used remove shebang from _common.sh because it's meant to be sourced, not run --- tests/boulder-integration.sh | 15 +++++++++++---- tests/integration/_common.sh | 13 ++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index ca6f48e60..6612b2e67 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -1,4 +1,4 @@ -#!/bin/sh -xe +#!/bin/bash # Simple integration test. Make sure to activate virtualenv beforehand # (source venv/bin/activate) and that you are running Boulder test # instance (see ./boulder-fetch.sh). @@ -8,12 +8,11 @@ # # Note: this script is called by Boulder integration test suite! +set -eux + . ./tests/integration/_common.sh export PATH="$PATH:/usr/sbin" # /usr/sbin/nginx -export GOPATH="${GOPATH:-/tmp/go}" -export PATH="$GOPATH/bin:$PATH" - if [ `uname` = "Darwin" ];then readlink="greadlink" else @@ -27,6 +26,14 @@ cleanup_and_exit() { echo Kill server subprocess, left running by abnormal exit kill $SERVER_STILL_RUNNING fi + # Dump boulder logs in case they contain useful debugging information. + : "------------------ ------------------ ------------------" + : "------------------ begin boulder logs ------------------" + : "------------------ ------------------ ------------------" + docker logs boulder_boulder_1 + : "------------------ ------------------ ------------------" + : "------------------ end boulder logs ------------------" + : "------------------ ------------------ ------------------" exit $EXIT_STATUS } diff --git a/tests/integration/_common.sh b/tests/integration/_common.sh index 9b44631d4..8d4baff95 100755 --- a/tests/integration/_common.sh +++ b/tests/integration/_common.sh @@ -1,12 +1,7 @@ -#!/bin/sh - -if [ "xxx$root" = "xxx" ]; -then - # The -t is required on macOS. It provides a template file path for - # the kernel to use. - root="$(mktemp -d -t leitXXXX)" - echo "Root integration tests directory: $root" -fi +# The -t is required on macOS. It provides a template file path for +# the kernel to use. +root=${root:-$(mktemp -d -t leitXXXX)} +echo "Root integration tests directory: $root" store_flags="--config-dir $root/conf --work-dir $root/work" store_flags="$store_flags --logs-dir $root/logs" tls_sni_01_port=5001 From 52e22b22e5120d43f0a3b220d5ba48fca15cafe2 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Thu, 30 Mar 2017 07:47:36 -0700 Subject: [PATCH 36/40] Add additional Nginx parsing test case (#4440) --- certbot-nginx/certbot_nginx/tests/nginxparser_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index 0a8dbd100..dd31ebac8 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -346,6 +346,8 @@ class TestRawNginxParser(unittest.TestCase): self.assertEqual(parsed, [['directive', '$var']]) self.assertRaises(ParseException, loads, "server {server_name test.com};") self.assertRaises(ParseException, loads, "directive ${var};") + self.assertEqual(loads("blag${dfgdfg};"), [['blag${dfgdfg}']]) + self.assertRaises(ParseException, loads, "blag${dfgdf{g};") class TestUnspacedList(unittest.TestCase): From d09bde972ae842c97263414a1c0cf54a721329ce Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Thu, 30 Mar 2017 15:28:24 -0700 Subject: [PATCH 37/40] Remove unused default parameter (#4447) * Remove unnecessary, nonexistent default --- certbot-nginx/certbot_nginx/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 225702251..b36f638ab 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -820,7 +820,7 @@ class NginxConfigurator(common.Plugin): self.restart() -def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"): +def nginx_restart(nginx_ctl, nginx_conf): """Restarts the Nginx Server. .. todo:: Nginx restart is fatal if the configuration references From a542fcd019e1e3eb99dd9872aaa00ca99450800c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 30 Mar 2017 15:47:31 -0700 Subject: [PATCH 38/40] Revert "Add a global lock file to Certbot (#4369)" (#4445) This reverts commit 32122cfa21fe7ba43189658158949cfe16dc6717. --- certbot/cli.py | 2 - certbot/constants.py | 1 - certbot/interfaces.py | 3 -- certbot/main.py | 53 +------------------ certbot/tests/main_test.py | 51 +----------------- letsencrypt-auto-source/letsencrypt-auto | 6 --- .../pieces/letsencrypt-auto-requirements.txt | 6 --- setup.py | 1 - 8 files changed, 2 insertions(+), 121 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 0faf4a7c6..6217baa27 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1167,8 +1167,6 @@ def _paths_parser(helpful): help="Logs directory.") add("paths", "--server", default=flag_default("server"), help=config_help("server")) - add("paths", "--lock-path", default=flag_default("lock_path"), - help=config_help('lock_path')) def _plugins_parsing(helpful, plugins): diff --git a/certbot/constants.py b/certbot/constants.py index 24cf89199..382b0afb3 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -33,7 +33,6 @@ CLI_DEFAULTS = dict( auth_cert_path="./cert.pem", auth_chain_path="./chain.pem", strict_permissions=False, - lock_path="/tmp/.certbot.lock", debug_challenges=False, ) STAGING_URI = "https://acme-staging.api.letsencrypt.org/directory" diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 33bde9430..213992993 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -222,9 +222,6 @@ class IConfig(zope.interface.Interface): key_dir = zope.interface.Attribute("Keys storage.") temp_checkpoint_dir = zope.interface.Attribute( "Temporary checkpoint directory.") - lock_path = zope.interface.Attribute( - "Path to the lock file used to prevent multiple instances of " - "Certbot from modifying your server's configuration at once.") no_verify_ssl = zope.interface.Attribute( "Disable verification of the ACME server's certificate.") diff --git a/certbot/main.py b/certbot/main.py index c6e61fbf9..ab2204428 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -8,7 +8,6 @@ import sys import time import traceback -import fasteners import zope.component from acme import jose @@ -867,56 +866,6 @@ def _post_logging_setup(config, plugins, cli_args): logger.debug("Discovered plugins: %r", plugins) -def acquire_file_lock(lock_path): - """Obtain a lock on the file at the specified path. - - :param str lock_path: path to the file to be locked - - :returns: lock file object representing the acquired lock - :rtype: fasteners.InterProcessLock - - :raises .Error: if the lock is held by another process - - """ - lock = fasteners.InterProcessLock(lock_path) - logger.debug("Attempting to acquire lock file %s", lock_path) - - try: - lock.acquire(blocking=False) - except IOError as err: - logger.debug(err) - logger.warning( - "Unable to access lock file %s. You should set --lock-file " - "to a writeable path to ensure multiple instances of " - "Certbot don't attempt modify your configuration " - "simultaneously.", lock_path) - else: - if not lock.acquired: - raise errors.Error( - "Another instance of Certbot is already running.") - - return lock - - -def _run_subcommand(config, plugins): - """Executes the Certbot subcommand specified in the configuration. - - :param .IConfig config: parsed configuration object - :param .PluginsRegistry plugins: available plugins - - :returns: return value from the specified subcommand - :rtype: str or int - - """ - lock = acquire_file_lock(config.lock_path) - - try: - return config.func(config, plugins) - finally: - if lock.acquired: - lock.release() - - def main(cli_args=sys.argv[1:]): """Command line argument parsing and main script execution.""" sys.excepthook = functools.partial(_handle_exception, config=None) @@ -944,7 +893,7 @@ def main(cli_args=sys.argv[1:]): zope.component.provideUtility(report) atexit.register(report.atexit_print_messages) - return _run_subcommand(config, plugins) + return config.func(config, plugins) if __name__ == "__main__": diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 02f241255..5d456bc3d 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -4,7 +4,6 @@ from __future__ import print_function import itertools import mock -import multiprocessing import os import shutil import traceback @@ -446,8 +445,7 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me os.mkdir(self.logs_dir) self.standard_args = ['--config-dir', self.config_dir, '--work-dir', self.work_dir, - '--logs-dir', self.logs_dir, '--text', - '--lock-path', os.path.join(self.tempdir, 'certbot.lock')] + '--logs-dir', self.logs_dir, '--text'] def tearDown(self): # Reset globals in cli @@ -1305,52 +1303,5 @@ class TestHandleException(unittest.TestCase): traceback.format_exception_only(KeyboardInterrupt, interrupt))) -class TestAcquireFileLock(test_util.TempDirTestCase): - """Test main.acquire_file_lock.""" - - def setUp(self): - super(TestAcquireFileLock, self).setUp() - - self.lock_path = os.path.join(self.tempdir, 'certbot.lock') - - @mock.patch('certbot.main.logger') - def test_bad_path(self, mock_logger): - lock = main.acquire_file_lock(os.getcwd()) - self.assertTrue(mock_logger.warning.called) - self.assertFalse(lock.acquired) - - def test_held_lock(self): - # start child and wait for it to grab the lock - cv = multiprocessing.Condition() - cv.acquire() - child_args = (cv, self.lock_path,) - child = multiprocessing.Process(target=_hold_lock, args=child_args) - child.start() - cv.wait() - - # assert we can't grab lock and terminate the child - self.assertRaises(errors.Error, main.acquire_file_lock, self.lock_path) - cv.notify() - cv.release() - child.join() - self.assertEqual(child.exitcode, 0) - - -def _hold_lock(cv, lock_path): - """Acquire a file lock at lock_path and wait to release it. - - :param multiprocessing.Condition cv: condition for syncronization - :param str lock_path: path to the file lock - - """ - import fasteners - lock = fasteners.InterProcessLock(lock_path) - lock.acquire() - cv.acquire() - cv.notify() - cv.wait() - lock.release() - - if __name__ == '__main__': unittest.main() # pragma: no cover diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 475d57fee..35c7a530c 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -727,9 +727,6 @@ cryptography==1.5.3 \ enum34==1.1.2 \ --hash=sha256:2475d7fcddf5951e92ff546972758802de5260bf409319a9f1934e6bbc8b1dc7 \ --hash=sha256:35907defb0f992b75ab7788f65fedc1cf20ffa22688e0e6f6f12afc06b3ea501 -fasteners==0.14.1 \ - --hash=sha256:564a115ff9698767df401efca29620cbb1a1c2146b7095ebd304b79cc5807a7c \ - --hash=sha256:427c76773fe036ddfa41e57d89086ea03111bbac57c55fc55f3006d027107e18 funcsigs==0.4 \ --hash=sha256:ff5ad9e2f8d9e5d1e8bbfbcf47722ab527cf0d51caeeed9da6d0f40799383fde \ --hash=sha256:d83ce6df0b0ea6618700fe1db353526391a8a3ada1b7aba52fed7a61da772033 @@ -742,9 +739,6 @@ ipaddress==1.0.16 \ linecache2==1.0.0 \ --hash=sha256:e78be9c0a0dfcbac712fe04fbf92b96cddae80b1b842f24248214c8496f006ef \ --hash=sha256:4b26ff4e7110db76eeb6f5a7b64a82623839d595c2038eeda662f2a2db78e97c -monotonic==1.3 \ - --hash=sha256:a8c7690953546c6bc8a4f05d347718db50de1225b29f4b9f346c0c6f19bdc286 \ - --hash=sha256:2b469e2d7dd403f7f7f79227fe5ad551ee1e76f8bb300ae935209884b93c7c1b ordereddict==1.1 \ --hash=sha256:1c35b4ac206cef2d24816c89f89cf289dd3d38cf7c449bb3fab7bf6d43f01b1f parsedatetime==2.1 \ diff --git a/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt b/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt index fbf416d66..d70d24e2a 100644 --- a/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt +++ b/letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt @@ -65,9 +65,6 @@ cryptography==1.5.3 \ enum34==1.1.2 \ --hash=sha256:2475d7fcddf5951e92ff546972758802de5260bf409319a9f1934e6bbc8b1dc7 \ --hash=sha256:35907defb0f992b75ab7788f65fedc1cf20ffa22688e0e6f6f12afc06b3ea501 -fasteners==0.14.1 \ - --hash=sha256:564a115ff9698767df401efca29620cbb1a1c2146b7095ebd304b79cc5807a7c \ - --hash=sha256:427c76773fe036ddfa41e57d89086ea03111bbac57c55fc55f3006d027107e18 funcsigs==0.4 \ --hash=sha256:ff5ad9e2f8d9e5d1e8bbfbcf47722ab527cf0d51caeeed9da6d0f40799383fde \ --hash=sha256:d83ce6df0b0ea6618700fe1db353526391a8a3ada1b7aba52fed7a61da772033 @@ -80,9 +77,6 @@ ipaddress==1.0.16 \ linecache2==1.0.0 \ --hash=sha256:e78be9c0a0dfcbac712fe04fbf92b96cddae80b1b842f24248214c8496f006ef \ --hash=sha256:4b26ff4e7110db76eeb6f5a7b64a82623839d595c2038eeda662f2a2db78e97c -monotonic==1.3 \ - --hash=sha256:a8c7690953546c6bc8a4f05d347718db50de1225b29f4b9f346c0c6f19bdc286 \ - --hash=sha256:2b469e2d7dd403f7f7f79227fe5ad551ee1e76f8bb300ae935209884b93c7c1b ordereddict==1.1 \ --hash=sha256:1c35b4ac206cef2d24816c89f89cf289dd3d38cf7c449bb3fab7bf6d43f01b1f parsedatetime==2.1 \ diff --git a/setup.py b/setup.py index 59f6dbd9e..0e8d19a22 100644 --- a/setup.py +++ b/setup.py @@ -43,7 +43,6 @@ install_requires = [ 'ConfigArgParse>=0.9.3', 'configobj', 'cryptography>=0.7', # load_pem_x509_certificate - 'fasteners', 'mock', 'parsedatetime>=1.3', # Calendar.parseDT 'PyOpenSSL', From e194e0dd5f3fdde78c0342df4b09c7a982249d56 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 30 Mar 2017 16:17:57 -0700 Subject: [PATCH 39/40] Refactoring for better logging (#4444) * Move colored_logging.py to log.py * Add atexit.register code to util * Add tests for atexit_register * Copy except_hook to log * Add pre_arg_setup * move setup_log_file_handler to log.py * Add post_arg_setup * move changes to main * Undo changes to MainTest * s/pre_arg_setup/pre_arg_parse_setup * s/post_arg_setup/post_arg_parse_setup --- certbot/colored_logging.py | 45 ------ certbot/log.py | 180 +++++++++++++++++++++++ certbot/main.py | 163 ++------------------- certbot/reporter.py | 19 --- certbot/tests/colored_logging_test.py | 41 ------ certbot/tests/log_test.py | 199 ++++++++++++++++++++++++++ certbot/tests/main_test.py | 130 ----------------- certbot/tests/reporter_test.py | 12 -- certbot/tests/util_test.py | 51 +++++++ certbot/util.py | 45 ++++++ 10 files changed, 486 insertions(+), 399 deletions(-) delete mode 100644 certbot/colored_logging.py create mode 100644 certbot/log.py delete mode 100644 certbot/tests/colored_logging_test.py create mode 100644 certbot/tests/log_test.py diff --git a/certbot/colored_logging.py b/certbot/colored_logging.py deleted file mode 100644 index 93bf3a55a..000000000 --- a/certbot/colored_logging.py +++ /dev/null @@ -1,45 +0,0 @@ -"""A formatter and StreamHandler for colorizing logging output.""" -import logging -import sys - -from certbot import util - - -class StreamHandler(logging.StreamHandler): - """Sends colored logging output to a stream. - - If the specified stream is not a tty, the class works like the - standard logging.StreamHandler. Default red_level is logging.WARNING. - - :ivar bool colored: True if output should be colored - :ivar bool red_level: The level at which to output - - """ - - def __init__(self, stream=None): - if sys.version_info < (2, 7): - # pragma: no cover - # pylint: disable=non-parent-init-called - logging.StreamHandler.__init__(self, stream) - else: - super(StreamHandler, self).__init__(stream) - self.colored = (sys.stderr.isatty() if stream is None else - stream.isatty()) - self.red_level = logging.WARNING - - def format(self, record): - """Formats the string representation of record. - - :param logging.LogRecord record: Record to be formatted - - :returns: Formatted, string representation of record - :rtype: str - - """ - out = (logging.StreamHandler.format(self, record) - if sys.version_info < (2, 7) - else super(StreamHandler, self).format(record)) - if self.colored and record.levelno >= self.red_level: - return ''.join((util.ANSI_SGR_RED, out, util.ANSI_SGR_RESET)) - else: - return out diff --git a/certbot/log.py b/certbot/log.py new file mode 100644 index 000000000..92b35ed51 --- /dev/null +++ b/certbot/log.py @@ -0,0 +1,180 @@ +"""Logging utilities for Certbot.""" +from __future__ import print_function +import functools +import logging +import os +import sys +import time +import traceback + +from acme import messages + +from certbot import cli +from certbot import constants +from certbot import errors +from certbot import util + +# Logging format +CLI_FMT = "%(message)s" +FILE_FMT = "%(asctime)s:%(levelname)s:%(name)s:%(message)s" + + +logger = logging.getLogger(__name__) + + +def pre_arg_parse_setup(): + """Ensures fatal exceptions are logged and reported to the user.""" + sys.excepthook = functools.partial(except_hook, config=None) + + +def post_arg_parse_setup(config): + """Setup logging after command line arguments are parsed. + + :param certbot.interface.IConfig config: Configuration object + + """ + file_handler, file_path = setup_log_file_handler( + config, 'letsencrypt.log', FILE_FMT) + + if config.quiet: + level = constants.QUIET_LOGGING_LEVEL + else: + level = -config.verbose_count * 10 + stderr_handler = ColoredStreamHandler() + stderr_handler.setFormatter(logging.Formatter(CLI_FMT)) + stderr_handler.setLevel(level) + + root_logger = logging.getLogger() + root_logger.setLevel(logging.DEBUG) # send all records to handlers + root_logger.addHandler(stderr_handler) + root_logger.addHandler(file_handler) + + logger.debug('Root logging level set at %d', level) + logger.info('Saving debug log to %s', file_path) + + sys.excepthook = functools.partial(except_hook, config=config) + + +def setup_log_file_handler(config, logfile, fmt): + """Setup file debug logging. + + :param certbot.interface.IConfig config: Configuration object + :param str logfile: basename for the log file + :param str fmt: logging format string + + :returns: file handler and absolute path to the log file + :rtype: tuple + + """ + # TODO: logs might contain sensitive data such as contents of the + # private key! #525 + util.make_or_verify_core_dir( + config.logs_dir, 0o700, os.geteuid(), config.strict_permissions) + log_file_path = os.path.join(config.logs_dir, logfile) + try: + handler = logging.handlers.RotatingFileHandler( + log_file_path, maxBytes=2 ** 20, backupCount=1000) + except IOError as error: + raise errors.Error(util.PERM_ERR_FMT.format(error)) + # rotate on each invocation, rollover only possible when maxBytes + # is nonzero and backupCount is nonzero, so we set maxBytes as big + # as possible not to overrun in single CLI invocation (1MB). + handler.doRollover() # TODO: creates empty letsencrypt.log.1 file + handler.setLevel(logging.DEBUG) + handler_formatter = logging.Formatter(fmt=fmt) + handler_formatter.converter = time.gmtime # don't use localtime + handler.setFormatter(handler_formatter) + return handler, log_file_path + + +class ColoredStreamHandler(logging.StreamHandler): + """Sends colored logging output to a stream. + + If the specified stream is not a tty, the class works like the + standard logging.StreamHandler. Default red_level is logging.WARNING. + + :ivar bool colored: True if output should be colored + :ivar bool red_level: The level at which to output + + """ + + def __init__(self, stream=None): + if sys.version_info < (2, 7): + # pragma: no cover + # pylint: disable=non-parent-init-called + logging.StreamHandler.__init__(self, stream) + else: + super(ColoredStreamHandler, self).__init__(stream) + self.colored = (sys.stderr.isatty() if stream is None else + stream.isatty()) + self.red_level = logging.WARNING + + def format(self, record): + """Formats the string representation of record. + + :param logging.LogRecord record: Record to be formatted + + :returns: Formatted, string representation of record + :rtype: str + + """ + out = (logging.StreamHandler.format(self, record) + if sys.version_info < (2, 7) + else super(ColoredStreamHandler, self).format(record)) + if self.colored and record.levelno >= self.red_level: + return ''.join((util.ANSI_SGR_RED, out, util.ANSI_SGR_RESET)) + else: + return out + + +def except_hook(exc_type, exc_value, trace, config): + """Logs exceptions and reports them to the user. + + Config is used to determine how to display exceptions to the user. In + general, if config.debug is True, then the full exception and traceback is + shown to the user, otherwise it is suppressed. If config itself is None, + then the traceback and exception is attempted to be written to a logfile. + If this is successful, the traceback is suppressed, otherwise it is shown + to the user. sys.exit is always called with a nonzero status. + + """ + tb_str = "".join(traceback.format_exception(exc_type, exc_value, trace)) + logger.debug("Exiting abnormally:%s%s", os.linesep, tb_str) + + if issubclass(exc_type, Exception) and (config is None or not config.debug): + if config is None: + logfile = "certbot.log" + try: + with open(logfile, "w") as logfd: + traceback.print_exception( + exc_type, exc_value, trace, file=logfd) + assert "--debug" not in sys.argv # config is None if this explodes + except: # pylint: disable=bare-except + sys.exit(tb_str) + if "--debug" in sys.argv: + sys.exit(tb_str) + + if issubclass(exc_type, errors.Error): + sys.exit(exc_value) + else: + # Here we're passing a client or ACME error out to the client at the shell + # Tell the user a bit about what happened, without overwhelming + # them with a full traceback + err = traceback.format_exception_only(exc_type, exc_value)[0] + # Typical error from the ACME module: + # acme.messages.Error: urn:ietf:params:acme:error:malformed :: The + # request message was malformed :: Error creating new registration + # :: Validation of contact mailto:none@longrandomstring.biz failed: + # Server failure at resolver + if (messages.is_acme_error(err) and ":: " in err and + config.verbose_count <= cli.flag_default("verbose_count")): + # prune ACME error code, we have a human description + _code, _sep, err = err.partition(":: ") + msg = "An unexpected error occurred:\n" + err + "Please see the " + if config is None: + msg += "logfile '{0}' for more details.".format(logfile) + else: + msg += "logfiles in {0} for more details.".format(config.logs_dir) + sys.exit(msg) + else: + sys.exit(tb_str) diff --git a/certbot/main.py b/certbot/main.py index ab2204428..090479d20 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -1,47 +1,37 @@ """Certbot main entry point.""" from __future__ import print_function -import atexit -import functools import logging.handlers import os import sys -import time -import traceback import zope.component from acme import jose -from acme import messages from acme import errors as acme_errors import certbot from certbot import account from certbot import cert_manager -from certbot import client from certbot import cli -from certbot import crypto_util -from certbot import colored_logging +from certbot import client from certbot import configuration from certbot import constants +from certbot import crypto_util from certbot import eff from certbot import errors from certbot import hooks from certbot import interfaces -from certbot import util -from certbot import reporter +from certbot import log from certbot import renewal +from certbot import reporter +from certbot import util from certbot.display import util as display_util, ops as display_ops from certbot.plugins import disco as plugins_disco from certbot.plugins import selection as plug_sel -_PERM_ERR_FMT = os.linesep.join(( - "The following error was encountered:", "{0}", - "If running as non-root, set --config-dir, " - "--work-dir, and --logs-dir to writeable paths.")) - USER_CANCELLED = ("User chose to cancel the operation and may " "reinvoke the client.") @@ -704,138 +694,11 @@ def renew(config, unused_plugins): hooks.run_saved_post_hooks() -def setup_log_file_handler(config, logfile, fmt): - """Setup file debug logging.""" - log_file_path = os.path.join(config.logs_dir, logfile) - try: - handler = logging.handlers.RotatingFileHandler( - log_file_path, maxBytes=2 ** 20, backupCount=1000) - except IOError as error: - raise errors.Error(_PERM_ERR_FMT.format(error)) - # rotate on each invocation, rollover only possible when maxBytes - # is nonzero and backupCount is nonzero, so we set maxBytes as big - # as possible not to overrun in single CLI invocation (1MB). - handler.doRollover() # TODO: creates empty letsencrypt.log.1 file - handler.setLevel(logging.DEBUG) - handler_formatter = logging.Formatter(fmt=fmt) - handler_formatter.converter = time.gmtime # don't use localtime - handler.setFormatter(handler_formatter) - return handler, log_file_path - - -def _cli_log_handler(level, fmt): - handler = colored_logging.StreamHandler() - handler.setFormatter(logging.Formatter(fmt)) - handler.setLevel(level) - return handler - - -def setup_logging(config): - """Sets up logging to logfiles and the terminal. - - :param certbot.interface.IConfig config: Configuration object - - """ - cli_fmt = "%(message)s" - file_fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s" - logfile = "letsencrypt.log" - if config.quiet: - level = constants.QUIET_LOGGING_LEVEL - else: - level = -config.verbose_count * 10 - file_handler, log_file_path = setup_log_file_handler( - config, logfile=logfile, fmt=file_fmt) - cli_handler = _cli_log_handler(level, cli_fmt) - - # TODO: use fileConfig? - - root_logger = logging.getLogger() - root_logger.setLevel(logging.DEBUG) # send all records to handlers - root_logger.addHandler(cli_handler) - root_logger.addHandler(file_handler) - - logger.debug("Root logging level set at %d", level) - logger.info("Saving debug log to %s", log_file_path) - - -def _handle_exception(exc_type, exc_value, trace, config): - """Logs exceptions and reports them to the user. - - Config is used to determine how to display exceptions to the user. In - general, if config.debug is True, then the full exception and traceback is - shown to the user, otherwise it is suppressed. If config itself is None, - then the traceback and exception is attempted to be written to a logfile. - If this is successful, the traceback is suppressed, otherwise it is shown - to the user. sys.exit is always called with a nonzero status. - - """ - tb_str = "".join(traceback.format_exception(exc_type, exc_value, trace)) - logger.debug("Exiting abnormally:%s%s", os.linesep, tb_str) - - if issubclass(exc_type, Exception) and (config is None or not config.debug): - if config is None: - logfile = "certbot.log" - try: - with open(logfile, "w") as logfd: - traceback.print_exception( - exc_type, exc_value, trace, file=logfd) - assert "--debug" not in sys.argv # config is None if this explodes - except: # pylint: disable=bare-except - sys.exit(tb_str) - if "--debug" in sys.argv: - sys.exit(tb_str) - - if issubclass(exc_type, errors.Error): - sys.exit(exc_value) - else: - # Here we're passing a client or ACME error out to the client at the shell - # Tell the user a bit about what happened, without overwhelming - # them with a full traceback - err = traceback.format_exception_only(exc_type, exc_value)[0] - # Typical error from the ACME module: - # acme.messages.Error: urn:ietf:params:acme:error:malformed :: The - # request message was malformed :: Error creating new registration - # :: Validation of contact mailto:none@longrandomstring.biz failed: - # Server failure at resolver - if (messages.is_acme_error(err) and ":: " in err and - config.verbose_count <= cli.flag_default("verbose_count")): - # prune ACME error code, we have a human description - _code, _sep, err = err.partition(":: ") - msg = "An unexpected error occurred:\n" + err + "Please see the " - if config is None: - msg += "logfile '{0}' for more details.".format(logfile) - else: - msg += "logfiles in {0} for more details.".format(config.logs_dir) - sys.exit(msg) - else: - sys.exit(tb_str) - - -def make_or_verify_core_dir(directory, mode, uid, strict): - """Make sure directory exists with proper permissions. - - :param str directory: Path to a directory. - :param int mode: Directory mode. - :param int uid: Directory owner. - :param bool strict: require directory to be owned by current user - - :raises .errors.Error: if the directory cannot be made or verified - - """ - try: - util.make_or_verify_dir(directory, mode, uid, strict) - except OSError as error: - raise errors.Error(_PERM_ERR_FMT.format(error)) - def make_or_verify_needed_dirs(config): - """Create or verify existence of config, work, or logs directories""" - make_or_verify_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, + """Create or verify existence of config and work directories""" + util.make_or_verify_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, os.geteuid(), config.strict_permissions) - make_or_verify_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, - os.geteuid(), config.strict_permissions) - # TODO: logs might contain sensitive data such as contents of the - # private key! #525 - make_or_verify_core_dir(config.logs_dir, 0o700, + util.make_or_verify_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, os.geteuid(), config.strict_permissions) @@ -868,7 +731,7 @@ def _post_logging_setup(config, plugins, cli_args): def main(cli_args=sys.argv[1:]): """Command line argument parsing and main script execution.""" - sys.excepthook = functools.partial(_handle_exception, config=None) + log.pre_arg_parse_setup() plugins = plugins_disco.PluginsRegistry.find_all() # note: arg parser internally handles --help (and exits afterwards) @@ -878,20 +741,16 @@ def main(cli_args=sys.argv[1:]): make_or_verify_needed_dirs(config) - # Setup logging ASAP, otherwise "No handlers could be found for - # logger ..." TODO: this should be done before plugins discovery - setup_logging(config) + log.post_arg_parse_setup(config) _post_logging_setup(config, plugins, cli_args) - sys.excepthook = functools.partial(_handle_exception, config=config) - set_displayer(config) # Reporter report = reporter.Reporter(config) zope.component.provideUtility(report) - atexit.register(report.atexit_print_messages) + util.atexit_register(report.print_messages) return config.func(config, plugins) diff --git a/certbot/reporter.py b/certbot/reporter.py index f836dbc8c..e0063d8e5 100644 --- a/certbot/reporter.py +++ b/certbot/reporter.py @@ -3,7 +3,6 @@ from __future__ import print_function import collections import logging -import os import sys import textwrap @@ -16,11 +15,6 @@ from certbot import util logger = logging.getLogger(__name__) -# Store the pid of the process that first imported this module so that -# atexit_print_messages side-effects such as error reporting can be limited to -# this process and not any fork()'d children. -INITIAL_PID = os.getpid() - @zope.interface.implementer(interfaces.IReporter) class Reporter(object): @@ -60,19 +54,6 @@ class Reporter(object): self.messages.put(self._msg_type(priority, msg, on_crash)) logger.debug("Reporting to user: %s", msg) - def atexit_print_messages(self, pid=None): - """Function to be registered with atexit to print messages. - - :param int pid: Process ID - - """ - if pid is None: - pid = INITIAL_PID - # This ensures that messages are only printed from the process that - # created the Reporter. - if pid == os.getpid(): - self.print_messages() - def print_messages(self): """Prints messages to the user and clears the message queue. diff --git a/certbot/tests/colored_logging_test.py b/certbot/tests/colored_logging_test.py deleted file mode 100644 index 0a7929561..000000000 --- a/certbot/tests/colored_logging_test.py +++ /dev/null @@ -1,41 +0,0 @@ -"""Tests for certbot.colored_logging.""" -import logging -import unittest - -import six - -from certbot import util - - -class StreamHandlerTest(unittest.TestCase): - """Tests for certbot.colored_logging.""" - - def setUp(self): - from certbot import colored_logging - - self.stream = six.StringIO() - self.stream.isatty = lambda: True - self.handler = colored_logging.StreamHandler(self.stream) - - self.logger = logging.getLogger() - self.logger.setLevel(logging.DEBUG) - self.logger.addHandler(self.handler) - - def test_format(self): - msg = 'I did a thing' - self.logger.debug(msg) - self.assertEqual(self.stream.getvalue(), '{0}\n'.format(msg)) - - def test_format_and_red_level(self): - msg = 'I did another thing' - self.handler.red_level = logging.DEBUG - self.logger.debug(msg) - - self.assertEqual(self.stream.getvalue(), - '{0}{1}{2}\n'.format(util.ANSI_SGR_RED, - msg, - util.ANSI_SGR_RESET)) - - -if __name__ == "__main__": - unittest.main() # pragma: no cover diff --git a/certbot/tests/log_test.py b/certbot/tests/log_test.py new file mode 100644 index 000000000..745147f0d --- /dev/null +++ b/certbot/tests/log_test.py @@ -0,0 +1,199 @@ +"""Tests for certbot.log.""" +import logging +import traceback +import logging.handlers +import os +import sys +import time +import unittest + +import mock +import six + +from acme import messages + +from certbot import constants +from certbot import errors +from certbot import util +from certbot.tests import util as test_util + + +class PreArgParseSetupTest(unittest.TestCase): + """Tests for certbot.log.pre_arg_parse_setup.""" + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.log import pre_arg_parse_setup + return pre_arg_parse_setup(*args, **kwargs) + + def test_it(self): + with mock.patch('certbot.log.except_hook') as mock_except_hook: + with mock.patch('certbot.log.sys') as mock_sys: + self._call() + + mock_sys.excepthook(1, 2, 3) + mock_except_hook.assert_called_once_with(1, 2, 3, config=None) + + +class PostArgParseSetupTest(test_util.TempDirTestCase): + """Tests for certbot.log.post_arg_parse_setup.""" + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.log import post_arg_parse_setup + return post_arg_parse_setup(*args, **kwargs) + + def setUp(self): + super(PostArgParseSetupTest, self).setUp() + self.config = mock.MagicMock( + logs_dir=self.tempdir, quiet=False, + verbose_count=constants.CLI_DEFAULTS['verbose_count']) + self.root_logger = mock.MagicMock() + + def test_common(self): + with mock.patch('certbot.log.logging.getLogger') as mock_get_logger: + mock_get_logger.return_value = self.root_logger + with mock.patch('certbot.log.except_hook') as mock_except_hook: + with mock.patch('certbot.log.sys') as mock_sys: + mock_sys.version_info = sys.version_info + self._call(self.config) + + self.assertEqual(self.root_logger.addHandler.call_count, 2) + self.assertTrue(os.path.exists(os.path.join( + self.config.logs_dir, 'letsencrypt.log'))) + mock_sys.excepthook(1, 2, 3) + mock_except_hook.assert_called_once_with(1, 2, 3, config=self.config) + + stderr_handler = self.root_logger.addHandler.call_args_list[0][0][0] + level = stderr_handler.level + if self.config.quiet: + self.assertEqual(level, constants.QUIET_LOGGING_LEVEL) + else: + self.assertEqual(level, -self.config.verbose_count * 10) + + def test_quiet(self): + self.config.quiet = True + self.test_common() + + +class SetupLogFileHandlerTest(test_util.TempDirTestCase): + """Tests for certbot.log.setup_log_file_handler.""" + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.log import setup_log_file_handler + return setup_log_file_handler(*args, **kwargs) + + def setUp(self): + super(SetupLogFileHandlerTest, self).setUp() + self.config = mock.MagicMock(logs_dir=self.tempdir) + + @mock.patch('certbot.main.logging.handlers.RotatingFileHandler') + def test_failure(self, mock_handler): + mock_handler.side_effect = IOError + + try: + self._call(self.config, 'test.log', '%(message)s') + except errors.Error as err: + self.assertTrue('--logs-dir' in str(err)) + else: # pragma: no cover + self.fail('Error not raised.') + + def test_success(self): + log_file = 'test.log' + handler, log_path = self._call(self.config, log_file, '%(message)s') + self.assertEqual(handler.level, logging.DEBUG) + self.assertEqual(handler.formatter.converter, time.gmtime) + + expected_path = os.path.join(self.config.logs_dir, log_file) + self.assertEqual(log_path, expected_path) + + +class ColoredStreamHandlerTest(unittest.TestCase): + """Tests for certbot.log.""" + + def setUp(self): + from certbot import log + + self.stream = six.StringIO() + self.stream.isatty = lambda: True + self.handler = log.ColoredStreamHandler(self.stream) + + self.logger = logging.getLogger() + self.logger.setLevel(logging.DEBUG) + self.logger.addHandler(self.handler) + + def test_format(self): + msg = 'I did a thing' + self.logger.debug(msg) + self.assertEqual(self.stream.getvalue(), '{0}\n'.format(msg)) + + def test_format_and_red_level(self): + msg = 'I did another thing' + self.handler.red_level = logging.DEBUG + self.logger.debug(msg) + + self.assertEqual(self.stream.getvalue(), + '{0}{1}{2}\n'.format(util.ANSI_SGR_RED, + msg, + util.ANSI_SGR_RESET)) + + +class ExceptHookTest(unittest.TestCase): + """Tests for certbot.log.except_hook.""" + @classmethod + def _call(cls, *args, **kwargs): + from certbot.log import except_hook + return except_hook(*args, **kwargs) + + @mock.patch('certbot.log.sys') + def test_except_hook(self, mock_sys): + config = mock.MagicMock() + mock_open = mock.mock_open() + + with mock.patch('certbot.log.open', mock_open, create=True): + exception = Exception('detail') + config.verbose_count = 1 + self._call( + Exception, exc_value=exception, trace=None, config=None) + mock_open().write.assert_any_call(''.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) + + with mock.patch('certbot.log.open', mock_open, create=True): + mock_open.side_effect = [KeyboardInterrupt] + error = errors.Error('detail') + self._call( + errors.Error, exc_value=error, trace=None, config=None) + # assert_any_call used because sys.exit doesn't exit in cli.py + mock_sys.exit.assert_any_call(''.join( + traceback.format_exception_only(errors.Error, error))) + + bad_typ = messages.ERROR_PREFIX + 'triffid' + exception = messages.Error(detail='alpha', typ=bad_typ, title='beta') + config = mock.MagicMock(debug=False, verbose_count=-3) + self._call( + messages.Error, exc_value=exception, trace=None, config=config) + error_msg = mock_sys.exit.call_args_list[-1][0][0] + self.assertTrue('unexpected error' in error_msg) + self.assertTrue('acme:error' not in error_msg) + self.assertTrue('alpha' in error_msg) + self.assertTrue('beta' in error_msg) + config = mock.MagicMock(debug=False, verbose_count=1) + self._call( + messages.Error, exc_value=exception, trace=None, config=config) + error_msg = mock_sys.exit.call_args_list[-1][0][0] + self.assertTrue('unexpected error' in error_msg) + self.assertTrue('acme:error' in error_msg) + self.assertTrue('alpha' in error_msg) + + interrupt = KeyboardInterrupt('detail') + self._call( + KeyboardInterrupt, exc_value=interrupt, trace=None, config=None) + mock_sys.exit.assert_called_with(''.join( + traceback.format_exception_only(KeyboardInterrupt, interrupt))) + + +if __name__ == "__main__": + unittest.main() # pragma: no cover diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 5d456bc3d..1afe95924 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -18,7 +18,6 @@ from acme import jose from certbot import account from certbot import cli -from certbot import colored_logging from certbot import constants from certbot import configuration from certbot import crypto_util @@ -288,81 +287,6 @@ class RevokeTest(test_util.TempDirTestCase): self.mock_success_revoke.assert_not_called() -class SetupLogFileHandlerTest(test_util.TempDirTestCase): - """Tests for certbot.main.setup_log_file_handler.""" - - def setUp(self): - super(SetupLogFileHandlerTest, self).setUp() - - self.config = mock.Mock(spec_set=['logs_dir'], - logs_dir=self.tempdir) - - def _call(self, *args, **kwargs): - from certbot.main import setup_log_file_handler - return setup_log_file_handler(*args, **kwargs) - - @mock.patch('certbot.main.logging.handlers.RotatingFileHandler') - def test_ioerror(self, mock_handler): - mock_handler.side_effect = IOError - self.assertRaises(errors.Error, self._call, - self.config, "test.log", "%s") - - -class SetupLoggingTest(test_util.TempDirTestCase): - """Tests for certbot.main.setup_logging.""" - - def setUp(self): - super(SetupLoggingTest, self).setUp() - - self.config = mock.Mock( - logs_dir=self.tempdir, - noninteractive_mode=False, quiet=False, - verbose_count=constants.CLI_DEFAULTS['verbose_count']) - - @classmethod - def _call(cls, *args, **kwargs): - from certbot.main import setup_logging - return setup_logging(*args, **kwargs) - - @mock.patch('certbot.main.logging.getLogger') - def test_defaults(self, mock_get_logger): - self._call(self.config) - - cli_handler = mock_get_logger().addHandler.call_args_list[0][0][0] - self.assertEqual(cli_handler.level, -self.config.verbose_count * 10) - self.assertTrue( - isinstance(cli_handler, colored_logging.StreamHandler)) - - @mock.patch('certbot.main.logging.getLogger') - def test_quiet_mode(self, mock_get_logger): - self.config.quiet = self.config.noninteractive_mode = True - self._call(self.config) - - cli_handler = mock_get_logger().addHandler.call_args_list[0][0][0] - self.assertEqual(cli_handler.level, constants.QUIET_LOGGING_LEVEL) - self.assertTrue( - isinstance(cli_handler, colored_logging.StreamHandler)) - - -class MakeOrVerifyCoreDirTest(test_util.TempDirTestCase): - """Tests for certbot.main.make_or_verify_core_dir.""" - - def _call(self, *args, **kwargs): - from certbot.main import make_or_verify_core_dir - return make_or_verify_core_dir(*args, **kwargs) - - def test_success(self): - new_dir = os.path.join(self.tempdir, 'new') - self._call(new_dir, 0o700, os.geteuid(), False) - self.assertTrue(os.path.exists(new_dir)) - - @mock.patch('certbot.main.util.make_or_verify_dir') - def test_failure(self, mock_make_or_verify): - mock_make_or_verify.side_effect = OSError - self.assertRaises(errors.Error, self._call, - self.tempdir, 0o700, os.geteuid(), False) - - class DetermineAccountTest(unittest.TestCase): """Tests for certbot.main._determine_account.""" @@ -1249,59 +1173,5 @@ class UnregisterTest(unittest.TestCase): self.assertFalse(acme_client.acme.deactivate_registration.called) -class TestHandleException(unittest.TestCase): - """Test main._handle_exception""" - @mock.patch('certbot.main.sys') - def test_handle_exception(self, mock_sys): - # pylint: disable=protected-access - from acme import messages - - config = mock.MagicMock() - mock_open = mock.mock_open() - - with mock.patch('certbot.main.open', mock_open, create=True): - exception = Exception('detail') - config.verbose_count = 1 - main._handle_exception( - Exception, exc_value=exception, trace=None, config=None) - mock_open().write.assert_any_call(''.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) - - with mock.patch('certbot.main.open', mock_open, create=True): - mock_open.side_effect = [KeyboardInterrupt] - error = errors.Error('detail') - main._handle_exception( - errors.Error, exc_value=error, trace=None, config=None) - # assert_any_call used because sys.exit doesn't exit in cli.py - mock_sys.exit.assert_any_call(''.join( - traceback.format_exception_only(errors.Error, error))) - - bad_typ = messages.ERROR_PREFIX + 'triffid' - exception = messages.Error(detail='alpha', typ=bad_typ, title='beta') - config = mock.MagicMock(debug=False, verbose_count=-3) - main._handle_exception( - messages.Error, exc_value=exception, trace=None, config=config) - error_msg = mock_sys.exit.call_args_list[-1][0][0] - self.assertTrue('unexpected error' in error_msg) - self.assertTrue('acme:error' not in error_msg) - self.assertTrue('alpha' in error_msg) - self.assertTrue('beta' in error_msg) - config = mock.MagicMock(debug=False, verbose_count=1) - main._handle_exception( - messages.Error, exc_value=exception, trace=None, config=config) - error_msg = mock_sys.exit.call_args_list[-1][0][0] - self.assertTrue('unexpected error' in error_msg) - self.assertTrue('acme:error' in error_msg) - self.assertTrue('alpha' in error_msg) - - interrupt = KeyboardInterrupt('detail') - main._handle_exception( - KeyboardInterrupt, exc_value=interrupt, trace=None, config=None) - mock_sys.exit.assert_called_with(''.join( - traceback.format_exception_only(KeyboardInterrupt, interrupt))) - - if __name__ == '__main__': unittest.main() # pragma: no cover diff --git a/certbot/tests/reporter_test.py b/certbot/tests/reporter_test.py index 0b06cccd7..9ec8dca28 100644 --- a/certbot/tests/reporter_test.py +++ b/certbot/tests/reporter_test.py @@ -38,18 +38,6 @@ class ReporterTest(unittest.TestCase): self.reporter.print_messages() self.assertEqual(sys.stdout.getvalue(), "") - @mock.patch('certbot.reporter.os.getpid') - def test_atexit_print_messages(self, mock_getpid): - self._add_messages() - mock_getpid.return_value = 42 - with mock.patch('certbot.reporter.INITIAL_PID', 42): - self.reporter.atexit_print_messages() - output = sys.stdout.getvalue() - self.assertTrue("IMPORTANT NOTES:" in output) - self.assertTrue("High" in output) - self.assertTrue("Med" in output) - self.assertTrue("Low" in output) - def test_tty_successful_exit(self): sys.stdout.isatty = lambda: True self._successful_exit_common() diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 480120772..f021c04cf 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -73,6 +73,25 @@ class ExeExistsTest(unittest.TestCase): self.assertFalse(self._call("exe")) +class MakeOrVerifyCoreDirTest(test_util.TempDirTestCase): + """Tests for certbot.util.make_or_verify_core_dir.""" + + def _call(self, *args, **kwargs): + from certbot.util import make_or_verify_core_dir + return make_or_verify_core_dir(*args, **kwargs) + + def test_success(self): + new_dir = os.path.join(self.tempdir, 'new') + self._call(new_dir, 0o700, os.geteuid(), False) + self.assertTrue(os.path.exists(new_dir)) + + @mock.patch('certbot.main.util.make_or_verify_dir') + def test_failure(self, mock_make_or_verify): + mock_make_or_verify.side_effect = OSError + self.assertRaises(errors.Error, self._call, + self.tempdir, 0o700, os.geteuid(), False) + + class MakeOrVerifyDirTest(test_util.TempDirTestCase): """Tests for certbot.util.make_or_verify_dir. @@ -473,5 +492,37 @@ class OsInfoTest(unittest.TestCase): ("windows", "95")) +class AtexitRegisterTest(unittest.TestCase): + """Tests for certbot.util.atexit_register.""" + def setUp(self): + self.func = mock.MagicMock() + self.args = ('hi',) + self.kwargs = {'answer': 42} + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.util import atexit_register + return atexit_register(*args, **kwargs) + + def test_called(self): + self._test_common(os.getpid()) + self.func.assert_called_with(*self.args, **self.kwargs) + + def test_not_called(self): + self._test_common(initial_pid=-1) + self.assertFalse(self.func.called) + + def _test_common(self, initial_pid): + with mock.patch('certbot.util._INITIAL_PID', initial_pid): + with mock.patch('certbot.util.atexit') as mock_atexit: + self._call(self.func, *self.args, **self.kwargs) + + # _INITAL_PID must be mocked when calling atexit_func + self.assertTrue(mock_atexit.register.called) + args, kwargs = mock_atexit.register.call_args + atexit_func = args[0] + atexit_func(*args[1:], **kwargs) # pylint: disable=star-args + + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot/util.py b/certbot/util.py index 95c669d0d..55a75097f 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -1,5 +1,6 @@ """Utilities for all Certbot.""" import argparse +import atexit import collections # distutils.version under virtualenv confuses pylint # For more info, see: https://github.com/PyCQA/pylint/issues/73 @@ -38,6 +39,16 @@ ANSI_SGR_RED = "\033[31m" ANSI_SGR_RESET = "\033[0m" +PERM_ERR_FMT = os.linesep.join(( + "The following error was encountered:", "{0}", + "If running as non-root, set --config-dir, " + "--work-dir, and --logs-dir to writeable paths.")) + + +# Stores importing process ID to be used by atexit_register() +_INITIAL_PID = os.getpid() + + def run_script(params, log=logger.error): """Run the script with the given params. @@ -92,6 +103,23 @@ def exe_exists(exe): return False +def make_or_verify_core_dir(directory, mode, uid, strict): + """Make sure directory exists with proper permissions. + + :param str directory: Path to a directory. + :param int mode: Directory mode. + :param int uid: Directory owner. + :param bool strict: require directory to be owned by current user + + :raises .errors.Error: if the directory cannot be made or verified + + """ + try: + make_or_verify_dir(directory, mode, uid, strict) + except OSError as error: + raise errors.Error(PERM_ERR_FMT.format(error)) + + def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False): """Make sure directory exists with proper permissions. @@ -533,3 +561,20 @@ def is_staging(srv): :rtype bool: """ return srv == constants.STAGING_URI or "staging" in srv + + +def atexit_register(func, *args, **kwargs): + """Sets func to be called before the program exits. + + Special care is taken to ensure func is only called when the process + that first imports this module exits rather than any child processes. + + :param function func: function to be called in case of an error + + """ + atexit.register(_atexit_call, func, *args, **kwargs) + + +def _atexit_call(func, *args, **kwargs): + if _INITIAL_PID == os.getpid(): + func(*args, **kwargs) From 43dccfc67187b0ae3d677fbe0c7fffab20e6179c Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 4 Apr 2017 11:20:58 -0700 Subject: [PATCH 40/40] More thoroughly rename during `certbot rename`. (#4320) * rename more files in rename command * Revert "Hide rename command (#4007)" This reverts commit 8c14de13a5d9d5be34aa13ac49a4f47bfb51a8fe. * Rename files in configuration files * Delete new files if we fail during the renaming process * update tests and error catching * More expressive error message --- certbot/cert_manager.py | 37 +++++++++-- certbot/cli.py | 15 ++++- certbot/main.py | 9 +-- certbot/storage.py | 102 ++++++++++++++++++++++++++--- certbot/tests/cert_manager_test.py | 21 ++++-- 5 files changed, 157 insertions(+), 27 deletions(-) diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 35d539a16..7b1811c99 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -13,6 +13,7 @@ from certbot import storage from certbot import util from certbot.display import util as display_util +from certbot.plugins import disco as plugins_disco logger = logging.getLogger(__name__) @@ -46,6 +47,7 @@ def rename_lineage(config): certname = _get_certname(config, "rename") + # what is the new name we want to use? new_certname = config.new_certname if not new_certname: code, new_certname = disp.input( @@ -54,13 +56,34 @@ def rename_lineage(config): if code != display_util.OK or not new_certname: raise errors.Error("User ended interaction.") - lineage = lineage_for_certname(config, certname) - if not lineage: - raise errors.ConfigurationError("No existing certificate with name " - "{0} found.".format(certname)) - storage.rename_renewal_config(certname, new_certname, config) - disp.notification("Successfully renamed {0} to {1}." - .format(certname, new_certname), pause=False) + try: + # copy files to new name + new_lineage = storage.duplicate_lineage(config, certname, new_certname) + + # install the new name's files + config.certname = new_certname + plugins = plugins_disco.PluginsRegistry.find_all() + from certbot.main import install + install(config, plugins, new_lineage, False) + except (errors.CertStorageError, errors.ConfigurationError, IOError, OSError) as e: + # delete the new files + config.certname = new_certname + # we might not have created anything to delete + try: + storage.delete_files(config, new_certname) + except errors.CertStorageError: + pass + reporter = zope.component.getUtility(interfaces.IReporter) + reporter.add_message("Unable to rename certificate", reporter.HIGH_PRIORITY) + raise e + else: + # delete old files + config.certname = certname + storage.delete_files(config, certname) + disp.notification("Renamed files for {0} to {1}." + .format(certname, new_certname), pause=False) + finally: + config.certname = certname def certificates(config): """Display information about certs configured with Certbot diff --git a/certbot/cli.py b/certbot/cli.py index 6217baa27..0442f481d 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -81,6 +81,7 @@ obtain, install, and renew certificates: manage certificates: certificates Display information about certs you have from Certbot revoke Revoke a certificate (supply --cert-path) + rename Rename a certificate delete Delete a certificate manage your account with Let's Encrypt: @@ -363,6 +364,10 @@ VERB_HELP = [ "opts": "Options for revocation of certs", "usage": "\n\n certbot revoke --cert-path /path/to/fullchain.pem [options]\n\n" }), + ("rename", { + "short": "Change a certificate's name (for management purposes)", + "opts": "Options for changing certificate names" + }), ("register", { "short": "Register for account with Let's Encrypt / other ACME server", "opts": "Options for account registration & modification" @@ -420,6 +425,7 @@ class HelpfulArgumentParser(object): "plugins": main.plugins_cmd, "register": main.register, "unregister": main.unregister, + "rename": main.rename, "renew": main.renew, "revoke": main.revoke, "rollback": main.rollback, @@ -790,7 +796,7 @@ def _add_all_groups(helpful): helpful.add_group("paths", description="Arguments changing execution paths & servers") helpful.add_group("manage", description="Various subcommands and flags are available for managing your certificates:", - verbs=["certificates", "delete", "renew", "revoke", "update_symlinks"]) + verbs=["certificates", "delete", "rename", "renew", "revoke", "update_symlinks"]) # VERBS for verb, docs in VERB_HELP: @@ -843,12 +849,17 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "multiple -d flags or enter a comma separated list of domains " "as a parameter. (default: Ask)") helpful.add( - [None, "run", "certonly", "manage", "delete", "certificates"], + [None, "run", "certonly", "manage", "rename", "delete", "certificates"], "--cert-name", dest="certname", metavar="CERTNAME", default=None, help="Certificate name to apply. Only one certificate name can be used " "per Certbot run. To see certificate names, run 'certbot certificates'. " "When creating a new certificate, specifies the new certificate's name.") + helpful.add( + ["rename", "manage"], + "--updated-cert-name", dest="new_certname", + metavar="NEW_CERTNAME", default=None, + help="New name for the certificate. Must be a valid filename.") helpful.add( [None, "testing", "renew", "certonly"], "--dry-run", action="store_true", dest="dry_run", diff --git a/certbot/main.py b/certbot/main.py index 090479d20..256503d4e 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -460,15 +460,16 @@ def register(config, unused_plugins): eff.handle_subscription(config) add_msg("Your e-mail address was updated to {0}.".format(config.email)) -def _install_cert(config, le_client, domains, lineage=None): +def _install_cert(config, le_client, domains, lineage=None, enhance=True): path_provider = lineage if lineage else config assert path_provider.cert_path is not None le_client.deploy_certificate(domains, path_provider.key_path, path_provider.cert_path, path_provider.chain_path, path_provider.fullchain_path) - le_client.enhance_config(domains, path_provider.chain_path) + if enhance: + le_client.enhance_config(domains, path_provider.chain_path) -def install(config, plugins): +def install(config, plugins, lineage=None, enhance=True): """Install a previously obtained cert in a server.""" # XXX: Update for renewer/RenewableCert # FIXME: be consistent about whether errors are raised or returned from @@ -481,7 +482,7 @@ def install(config, plugins): domains, _ = _find_domains_or_certname(config, installer) le_client = _init_le_client(config, authenticator=None, installer=installer) - _install_cert(config, le_client, domains) + _install_cert(config, le_client, domains, lineage, enhance) def plugins_cmd(config, plugins): # TODO: Use IDisplay rather than print diff --git a/certbot/storage.py b/certbot/storage.py index a1462b72d..34dc57884 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -34,7 +34,9 @@ def renewal_conf_files(config): return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf")) def renewal_file_for_certname(config, certname): - """Return /path/to/certname.conf in the renewal conf directory""" + """Return /path/to/certname.conf in the renewal conf directory + :raises .CertStorageError: if file is missing + """ path = os.path.join(config.renewal_configs_dir, "{0}.conf".format(certname)) if not os.path.exists(path): raise errors.CertStorageError("No certificate found with name {0} (expected " @@ -130,6 +132,8 @@ def rename_renewal_config(prev_name, new_name, cli_config): except OSError: raise errors.ConfigurationError("Please specify a valid filename " "for the new certificate name.") + else: + return new_filename def update_configuration(lineagename, archive_dir, target, cli_config): @@ -146,19 +150,25 @@ def update_configuration(lineagename, archive_dir, target, cli_config): """ config_filename = renewal_filename_for_lineagename(cli_config, lineagename) - temp_filename = config_filename + ".new" + + def _save_renewal_values(unused_config, temp_filename): + # Save only the config items that are relevant to renewal + values = relevant_values(vars(cli_config.namespace)) + write_renewal_config(config_filename, temp_filename, archive_dir, target, values) + _modify_config_with_tempfile(config_filename, _save_renewal_values) + + return configobj.ConfigObj(config_filename) + +def _modify_config_with_tempfile(filename, function): + temp_filename = filename + ".new" # If an existing tempfile exists, delete it if os.path.exists(temp_filename): os.unlink(temp_filename) - # Save only the config items that are relevant to renewal - values = relevant_values(vars(cli_config.namespace)) - write_renewal_config(config_filename, temp_filename, archive_dir, target, values) - os.rename(temp_filename, config_filename) - - return configobj.ConfigObj(config_filename) - + config = configobj.ConfigObj(filename) + function(config, temp_filename) + os.rename(temp_filename, filename) def get_link_target(link): """Get an absolute path to the target of link. @@ -243,6 +253,7 @@ def delete_files(config, certname): """Delete all files related to the certificate. If some files are not found, ignore them and continue. + :raises .CertStorageError: if lineage is missing """ renewal_filename = renewal_file_for_certname(config, certname) # file exists @@ -304,6 +315,79 @@ def delete_files(config, certname): except OSError: logger.debug("Unable to remove %s", archive_path) +def duplicate_lineage(config, certname, new_certname): + """Create a duplicate of certname with name new_certname + + :raises .CertStorageError: for storage errors + :raises .ConfigurationError: for cli and renewal configuration errors + :raises IOError: for filename errors + :raises OSError: for OS errors + """ + + # copy renewal config file + prev_filename = renewal_filename_for_lineagename(config, certname) + new_filename = renewal_filename_for_lineagename(config, new_certname) + if os.path.exists(new_filename): + raise errors.ConfigurationError("The new certificate name " + "is already in use.") + try: + shutil.copy2(prev_filename, new_filename) + except (OSError, IOError): + raise errors.ConfigurationError("Please specify a valid filename " + "for the new certificate name.") + logger.debug("Copied %s to %s", prev_filename, new_filename) + + # load config file + try: + renewal_config = configobj.ConfigObj(new_filename) + except configobj.ConfigObjError: + # config is corrupted + logger.warning("Could not parse %s. Only the certificate has been renamed.", + new_filename) + raise errors.CertStorageError( + "error parsing {0}".format(new_filename)) + + def copy_to_new_dir(prev_dir): + """Replace certname with new_certname in prev_dir""" + new_dir = prev_dir.replace(certname, new_certname) + # make dir iff it doesn't exist + shutil.copytree(prev_dir, new_dir, symlinks=True) + logger.debug("Copied %s to %s", prev_dir, new_dir) + return new_dir + + # archive dir + prev_archive_dir = _full_archive_path(renewal_config, config, certname) + new_archive_dir = prev_archive_dir + if not certname in prev_archive_dir: + raise errors.CertStorageError("Archive directory does not conform to defaults: " + "{0} not in {1}", certname, prev_archive_dir) + else: + new_archive_dir = copy_to_new_dir(prev_archive_dir) + + # live dir + # if things aren't in their default places, don't try to change things. + prev_live_dir = _full_live_path(config, certname) + prev_links = dict((kind, renewal_config.get(kind)) for kind in ALL_FOUR) + if (certname not in prev_live_dir or + len(set(os.path.dirname(renewal_config.get(kind)) for kind in ALL_FOUR)) != 1): + raise errors.CertStorageError("Live directory does not conform to defaults.") + else: + copy_to_new_dir(prev_live_dir) + new_links = dict((k, prev_links[k].replace(certname, new_certname)) for k in prev_links) + + # Update renewal config file + def _update_and_write(renewal_config, temp_filename): + renewal_config["archive_dir"] = new_archive_dir + renewal_config["version"] = certbot.__version__ + for kind in ALL_FOUR: + renewal_config[kind] = new_links[kind] + with open(temp_filename, "wb") as f: + renewal_config.write(outfile=f) + _modify_config_with_tempfile(new_filename, _update_and_write) + + # Update symlinks + return RenewableCert(new_filename, config, update_symlinks=True) + class RenewableCert(object): # pylint: disable=too-many-instance-attributes,too-many-public-methods diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index b5731fbd6..d0e563979 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -384,14 +384,19 @@ class RenameLineageTest(BaseCertManagerTest): @test_util.patch_get_utility() @mock.patch('certbot.cert_manager.lineage_for_certname') def test_no_existing_certname(self, mock_lineage_for_certname, unused_get_utility): - mock_config = mock.Mock(certname="one", new_certname="two") + mock_config = mock.Mock(certname="one", new_certname="two", + renewal_configs_dir="/tmp/etc/letsencrypt/renewal/") mock_lineage_for_certname.return_value = None - self.assertRaises(errors.ConfigurationError, - self._call, mock_config) + self.assertRaises(errors.ConfigurationError, self._call, mock_config) + @mock.patch("certbot.storage.RenewableCert._update_symlinks") @test_util.patch_get_utility() @mock.patch("certbot.storage.RenewableCert._check_symlinks") - def test_rename_cert(self, mock_check, unused_get_utility): + @mock.patch("certbot.storage.relevant_values") + def test_rename_cert(self, mock_rv, mock_check, unused_get_utility, unused_update_symlinks): + # Mock relevant_values() to claim that all values are relevant here + # (to avoid instantiating parser) + mock_rv.side_effect = lambda x: x mock_check.return_value = True mock_config = self.mock_config self._call(mock_config) @@ -400,9 +405,15 @@ class RenameLineageTest(BaseCertManagerTest): self.assertTrue(updated_lineage is not None) self.assertEqual(updated_lineage.lineagename, mock_config.new_certname) + @mock.patch("certbot.storage.RenewableCert._update_symlinks") @test_util.patch_get_utility() @mock.patch("certbot.storage.RenewableCert._check_symlinks") - def test_rename_cert_interactive_certname(self, mock_check, mock_get_utility): + @mock.patch("certbot.storage.relevant_values") + def test_rename_cert_interactive_certname(self, mock_rv, mock_check, mock_get_utility, + unused_update_symlinks): + # python 3.4 and 3.5 order things differently, so remove other.com for this test + os.remove(self.configs["other.com"].filename) + mock_rv.side_effect = lambda x: x mock_check.return_value = True mock_config = self.mock_config mock_config.certname = None