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/27] 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 5758b1687d5cb79beef8b51d90c655657985f6a4 Mon Sep 17 00:00:00 2001 From: kernelpanek Date: Wed, 15 Mar 2017 00:25:26 -0600 Subject: [PATCH 02/27] 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 03/27] 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 04/27] 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 fd789b4e4b9f58970abf10fed399e4e786ba3bb4 Mon Sep 17 00:00:00 2001 From: Piotr Kasprzyk Date: Fri, 17 Mar 2017 22:11:52 +0100 Subject: [PATCH 05/27] 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 c439057efab57d4b02856839e90d1404eefcb91c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Sat, 18 Mar 2017 13:25:02 -0700 Subject: [PATCH 06/27] 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 07/27] 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 08/27] 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 09/27] 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 10/27] 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 11/27] 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 12/27] 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 13/27] 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 14/27] 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 15/27] 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 16/27] 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 17/27] 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 18/27] 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 19/27] 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 20/27] 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 21/27] 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 22/27] 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 23/27] 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 ece68a1864b72616565711dcab8f3cfd4f388718 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Mon, 27 Mar 2017 15:19:03 -0700 Subject: [PATCH 24/27] 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 25/27] 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 26/27] 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 27/27] 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