From 18d3df78e8cc7ae498ba411fea155a227cb91642 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Wed, 26 Jul 2017 20:37:13 -0400 Subject: [PATCH 01/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 28 +++++++++++++++++++++++++++- acme/acme/client_test.py | 24 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index fa903f0e6..f781f011b 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -11,6 +11,7 @@ import six from six.moves import http_client # pylint: disable=import-error import OpenSSL +import re import requests import sys @@ -599,6 +600,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes return response def _send_request(self, method, url, *args, **kwargs): + # pylint: disable=too-many-locals """Send HTTP request. Makes sure that `verify_ssl` is respected. Logs request and @@ -624,7 +626,31 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes kwargs.setdefault('headers', {}) kwargs['headers'].setdefault('User-Agent', self.user_agent) kwargs.setdefault('timeout', self._default_timeout) - response = self.session.request(method, url, *args, **kwargs) + try: + response = self.session.request(method, url, *args, **kwargs) + except requests.exceptions.RequestException as e: + # pylint: disable=pointless-string-statement + """Requests response parsing + + The requests library emits exceptions with a lot of extra text. + We parse them with a regexp to raise a more readable exceptions. + + Example: + HTTPSConnectionPool(host='acme-v01.api.letsencrypt.org', + port=443): Max retries exceeded with url: /directory + (Caused by NewConnectionError(' + : Failed to establish a new connection: + [Errno 65] No route to host',))""" + + err_regex = r".*host='(\S*)'.*url\: (\/\w*).*(\[Errno \d+\])([A-Za-z ]*)" + m = re.match(err_regex, str(e)) + if m is None: + raise # pragma: no cover + else: + host, path, err_no, err_msg = m.groups() + raise ValueError("Requesting {0}{1}: {2}{3}".format(host, path, err_no, err_msg)) + # If content is DER, log the base64 of it instead of raw bytes, to keep # binary data out of the logs. if response.headers.get("Content-Type") == DER_CONTENT_TYPE: diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 54652b46c..c800f476c 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -7,6 +7,7 @@ from six.moves import http_client # pylint: disable=import-error import mock import requests +import sys from acme import challenges from acme import errors @@ -621,6 +622,29 @@ class ClientNetworkTest(unittest.TestCase): self.assertRaises(requests.exceptions.RequestException, self.net._send_request, 'GET', 'uri') + def test_urllib_error(self): + # Using a connection error to test a properly formatted error message + try: + # pylint: disable=protected-access + self.net._send_request('GET', "http://localhost:19123/nonexistent.txt") + + # Python 2 + except ValueError as y: + if "linux" in sys.platform: + self.assertEqual("Requesting localhost/nonexistent: " + "[Errno 111] Connection refused", str(y)) + else: #pragma: no cover + self.assertEqual("Requesting localhost/nonexistent: " + "[Errno 61] Connection refused", str(y)) + + # Python 3 + except requests.exceptions.ConnectionError as z: #pragma: no cover + if "linux" in sys.platform: + self.assertEqual("('Connection aborted.', " + "error(111, 'Connection refused'))", str(z)) + else: #pragma: no cover + self.assertEqual("('Connection aborted.', " + "error(61, 'Connection refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From 1e71ff537733d701688117fce0b92d3393018934 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Wed, 2 Aug 2017 22:59:27 -0400 Subject: [PATCH 02/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index f781f011b..67a4079fd 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -643,7 +643,9 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes object at 0x108356c50>: Failed to establish a new connection: [Errno 65] No route to host',))""" - err_regex = r".*host='(\S*)'.*url\: (\/\w*).*(\[Errno \d+\])([A-Za-z ]*)" + print e.message + # pylint: disable=line-too-long + err_regex = r".*host='(\S*)'.*Max retries exceeded with url\: (\/\w*).*(\[Errno \d+\])([A-Za-z ]*)" m = re.match(err_regex, str(e)) if m is None: raise # pragma: no cover From 959d72feb035dd49efc32f8f39f92e79ea32b2c3 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sat, 5 Aug 2017 10:16:47 -0400 Subject: [PATCH 03/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 2 +- acme/acme/client_test.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 67a4079fd..7316066da 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -651,7 +651,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes raise # pragma: no cover else: host, path, err_no, err_msg = m.groups() - raise ValueError("Requesting {0}{1}: {2}{3}".format(host, path, err_no, err_msg)) + raise ValueError("Requesting {0}{1}: {2}".format(host, path, err_msg)) # If content is DER, log the base64 of it instead of raw bytes, to keep # binary data out of the logs. diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index c800f476c..1c6b4a77c 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -632,19 +632,19 @@ class ClientNetworkTest(unittest.TestCase): except ValueError as y: if "linux" in sys.platform: self.assertEqual("Requesting localhost/nonexistent: " - "[Errno 111] Connection refused", str(y)) + "Connection refused", str(y)) else: #pragma: no cover self.assertEqual("Requesting localhost/nonexistent: " - "[Errno 61] Connection refused", str(y)) + "Connection refused", str(y)) # Python 3 except requests.exceptions.ConnectionError as z: #pragma: no cover if "linux" in sys.platform: self.assertEqual("('Connection aborted.', " - "error(111, 'Connection refused'))", str(z)) + "error('Connection refused'))", str(z)) else: #pragma: no cover self.assertEqual("('Connection aborted.', " - "error(61, 'Connection refused'))", str(z)) + "error('Connection refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From 3cc94798b6452f652b898916f5b710293e44f6b4 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sat, 5 Aug 2017 10:26:01 -0400 Subject: [PATCH 04/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 7316066da..dc3080dcf 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -650,7 +650,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes if m is None: raise # pragma: no cover else: - host, path, err_no, err_msg = m.groups() + host, path, _err_no, err_msg = m.groups() raise ValueError("Requesting {0}{1}: {2}".format(host, path, err_msg)) # If content is DER, log the base64 of it instead of raw bytes, to keep From 880c35f3e3f8d1788024d40c6540d9d9ee1f7f01 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sat, 5 Aug 2017 10:39:04 -0400 Subject: [PATCH 05/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index dc3080dcf..ee4cf7985 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -643,7 +643,6 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes object at 0x108356c50>: Failed to establish a new connection: [Errno 65] No route to host',))""" - print e.message # pylint: disable=line-too-long err_regex = r".*host='(\S*)'.*Max retries exceeded with url\: (\/\w*).*(\[Errno \d+\])([A-Za-z ]*)" m = re.match(err_regex, str(e)) From 57e664077fdf006aede83c7934470787e2ee5d90 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sun, 6 Aug 2017 10:51:10 -0400 Subject: [PATCH 06/24] [#4535] - Unwrap max retries exceeded errors - fixing spacing --- acme/acme/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index ee4cf7985..2e07d34d7 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -650,7 +650,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes raise # pragma: no cover else: host, path, _err_no, err_msg = m.groups() - raise ValueError("Requesting {0}{1}: {2}".format(host, path, err_msg)) + raise ValueError("Requesting {0}{1}:{2}".format(host, path, err_msg)) # If content is DER, log the base64 of it instead of raw bytes, to keep # binary data out of the logs. From 5fb1568b6e69866e4b0b7df0fd99a62bf0888b98 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sun, 6 Aug 2017 22:26:17 -0400 Subject: [PATCH 07/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 1c6b4a77c..172e94d74 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -628,7 +628,7 @@ class ClientNetworkTest(unittest.TestCase): # pylint: disable=protected-access self.net._send_request('GET', "http://localhost:19123/nonexistent.txt") - # Python 2 + # Python Exceptions except ValueError as y: if "linux" in sys.platform: self.assertEqual("Requesting localhost/nonexistent: " @@ -637,11 +637,11 @@ class ClientNetworkTest(unittest.TestCase): self.assertEqual("Requesting localhost/nonexistent: " "Connection refused", str(y)) - # Python 3 + # Requests Exceptions except requests.exceptions.ConnectionError as z: #pragma: no cover if "linux" in sys.platform: self.assertEqual("('Connection aborted.', " - "error('Connection refused'))", str(z)) + "error(111, 'Connection refused'))", str(z)) else: #pragma: no cover self.assertEqual("('Connection aborted.', " "error('Connection refused'))", str(z)) From 521f783020e3219bd698bbbe84397426c56e2dce Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sun, 6 Aug 2017 23:02:07 -0400 Subject: [PATCH 08/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 2 +- acme/acme/client_test.py | 18 ++---------------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 2e07d34d7..6fcee2fdc 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -647,7 +647,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes err_regex = r".*host='(\S*)'.*Max retries exceeded with url\: (\/\w*).*(\[Errno \d+\])([A-Za-z ]*)" m = re.match(err_regex, str(e)) if m is None: - raise # pragma: no cover + raise #pragma: no cover else: host, path, _err_no, err_msg = m.groups() raise ValueError("Requesting {0}{1}:{2}".format(host, path, err_msg)) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 172e94d74..ff0ac18ef 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -7,7 +7,6 @@ from six.moves import http_client # pylint: disable=import-error import mock import requests -import sys from acme import challenges from acme import errors @@ -630,21 +629,8 @@ class ClientNetworkTest(unittest.TestCase): # Python Exceptions except ValueError as y: - if "linux" in sys.platform: - self.assertEqual("Requesting localhost/nonexistent: " - "Connection refused", str(y)) - else: #pragma: no cover - self.assertEqual("Requesting localhost/nonexistent: " - "Connection refused", str(y)) - - # Requests Exceptions - except requests.exceptions.ConnectionError as z: #pragma: no cover - if "linux" in sys.platform: - self.assertEqual("('Connection aborted.', " - "error(111, 'Connection refused'))", str(z)) - else: #pragma: no cover - self.assertEqual("('Connection aborted.', " - "error('Connection refused'))", str(z)) + self.assertEqual("Requesting localhost/nonexistent: " + "Connection refused", str(y)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From 8555f4a0bdd83409cddebe831c706e12c056ecdd Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sun, 6 Aug 2017 23:30:11 -0400 Subject: [PATCH 09/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client.py | 2 +- acme/acme/client_test.py | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 6fcee2fdc..2e07d34d7 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -647,7 +647,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes err_regex = r".*host='(\S*)'.*Max retries exceeded with url\: (\/\w*).*(\[Errno \d+\])([A-Za-z ]*)" m = re.match(err_regex, str(e)) if m is None: - raise #pragma: no cover + raise # pragma: no cover else: host, path, _err_no, err_msg = m.groups() raise ValueError("Requesting {0}{1}:{2}".format(host, path, err_msg)) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index ff0ac18ef..6e6a0fecd 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -627,11 +627,15 @@ class ClientNetworkTest(unittest.TestCase): # pylint: disable=protected-access self.net._send_request('GET', "http://localhost:19123/nonexistent.txt") - # Python Exceptions + # Value Error Generated Exceptions except ValueError as y: self.assertEqual("Requesting localhost/nonexistent: " "Connection refused", str(y)) + # Requests Library Exceptions + except requests.exceptions.RequestException as z: + self.assertEqual("('Connection aborted.', error(111, 'Connection refused'))", str(z)) + class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" # pylint: disable=too-many-instance-attributes From 9b8c8f103eaeaa3f3060a253c62fddda90abbf88 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Sun, 6 Aug 2017 23:44:28 -0400 Subject: [PATCH 10/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 6e6a0fecd..213706b71 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -633,7 +633,7 @@ class ClientNetworkTest(unittest.TestCase): "Connection refused", str(y)) # Requests Library Exceptions - except requests.exceptions.RequestException as z: + except requests.exceptions.RequestException as z: #pragma: no cover self.assertEqual("('Connection aborted.', error(111, 'Connection refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): From 9ae987d72bc3709081997b3c7ab383e693bceae0 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Mon, 7 Aug 2017 08:35:00 -0400 Subject: [PATCH 11/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 213706b71..801e34bdf 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -633,8 +633,8 @@ class ClientNetworkTest(unittest.TestCase): "Connection refused", str(y)) # Requests Library Exceptions - except requests.exceptions.RequestException as z: #pragma: no cover - self.assertEqual("('Connection aborted.', error(111, 'Connection refused'))", str(z)) + except requests.exceptions.ConnectionError as z: #pragma: no cover + self.assertEqual("('Connection aborted.', error(99, 'Cannot assign requested address'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From f7dedae388ca338c3f218e680c4774109540f719 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Mon, 7 Aug 2017 08:39:39 -0400 Subject: [PATCH 12/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 801e34bdf..813898d20 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -634,7 +634,7 @@ class ClientNetworkTest(unittest.TestCase): # Requests Library Exceptions except requests.exceptions.ConnectionError as z: #pragma: no cover - self.assertEqual("('Connection aborted.', error(99, 'Cannot assign requested address'))", str(z)) + self.assertEqual("('Connection aborted.', error(111, 'Connection Refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From 2e7ec00e8c5a605c272a72a702f34803463bc5df Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Mon, 7 Aug 2017 08:55:43 -0400 Subject: [PATCH 13/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 813898d20..6509f951b 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -634,7 +634,7 @@ class ClientNetworkTest(unittest.TestCase): # Requests Library Exceptions except requests.exceptions.ConnectionError as z: #pragma: no cover - self.assertEqual("('Connection aborted.', error(111, 'Connection Refused'))", str(z)) + self.assertEqual("('Connection aborted.', error(111, 'Connection refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From 8a78ef9675e1fbed7e5734a5e95c5d121dde0553 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Mon, 7 Aug 2017 15:33:01 -0400 Subject: [PATCH 14/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 6509f951b..801e34bdf 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -634,7 +634,7 @@ class ClientNetworkTest(unittest.TestCase): # Requests Library Exceptions except requests.exceptions.ConnectionError as z: #pragma: no cover - self.assertEqual("('Connection aborted.', error(111, 'Connection refused'))", str(z)) + self.assertEqual("('Connection aborted.', error(99, 'Cannot assign requested address'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From b3216727daffa93391e939cadc1a266978240f22 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Mon, 7 Aug 2017 19:34:20 -0400 Subject: [PATCH 15/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 801e34bdf..d97b4920c 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -634,7 +634,8 @@ class ClientNetworkTest(unittest.TestCase): # Requests Library Exceptions except requests.exceptions.ConnectionError as z: #pragma: no cover - self.assertEqual("('Connection aborted.', error(99, 'Cannot assign requested address'))", str(z)) + self.assertEqual("('Connection aborted.'," + "error(111, 'Connection refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): """Tests for acme.client.ClientNetwork which mock out response.""" From a8e1df6e551e1322fc098d2bcccf5e90bb9d0dd0 Mon Sep 17 00:00:00 2001 From: Bob Strecansky Date: Mon, 7 Aug 2017 20:15:05 -0400 Subject: [PATCH 16/24] [#4535] - Unwrap max retries exceeded errors --- acme/acme/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index d97b4920c..4bd762865 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -634,7 +634,7 @@ class ClientNetworkTest(unittest.TestCase): # Requests Library Exceptions except requests.exceptions.ConnectionError as z: #pragma: no cover - self.assertEqual("('Connection aborted.'," + self.assertEqual("('Connection aborted.', " "error(111, 'Connection refused'))", str(z)) class ClientNetworkWithMockedResponseTest(unittest.TestCase): From 03cbe9dd86291ae006ab212984130ebb727a7c6d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 11 Oct 2017 08:16:48 -0700 Subject: [PATCH 17/24] Document --no-directory-hooks (#5171) --- docs/using.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/using.rst b/docs/using.rst index f2ba93a7e..4fd0b5ec8 100644 --- a/docs/using.rst +++ b/docs/using.rst @@ -503,7 +503,8 @@ run as usual after running all hooks in these directories. One minor exception to this is if a hook specified elsewhere is simply the path to an executable file in the hook directory of the same type (e.g. your pre-hook is the path to an executable in ``/etc/letsencrypt/renewal-hooks/pre``), the file is not run a -second time. +second time. You can stop Certbot from automatically running executables found +in these directories by including ``--no-directory-hooks`` on the command line. More information about hooks can be found by running ``certbot --help renew``. From 1081a2501f8b588d096149f674c40ff564533222 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 11 Oct 2017 08:18:17 -0700 Subject: [PATCH 18/24] integration test to prevent regressions of #5115 (#5172) --- tests/boulder-integration.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 0cfc61dd2..7afba19df 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -326,7 +326,7 @@ CheckDirHooks 5 # test with overlapping directory hooks on the command line common renew --cert-name le2.wtf \ --pre-hook "$renewal_dir_pre_hook" \ - --renew-hook "$renewal_dir_deploy_hook" \ + --deploy-hook "$renewal_dir_deploy_hook" \ --post-hook "$renewal_dir_post_hook" CheckDirHooks 1 From 232f5a92d193b1b4a955d0d5f2d1ea639215331d Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 11 Oct 2017 18:18:41 +0300 Subject: [PATCH 19/24] Fix naming in error message (#5181) --- certbot-apache/certbot_apache/configurator.py | 4 ++-- certbot-nginx/certbot_nginx/tls_sni_01.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index e2069ad95..8b6c578d6 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -1295,13 +1295,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): .. note:: This function saves the configuration :param ssl_vhost: Destination of traffic, an ssl enabled vhost - :type ssl_vhost: :class:`~letsencrypt_apache.obj.VirtualHost` + :type ssl_vhost: :class:`~certbot_apache.obj.VirtualHost` :param unused_options: Not currently used :type unused_options: Not Available :returns: Success, general_vhost (HTTP vhost) - :rtype: (bool, :class:`~letsencrypt_apache.obj.VirtualHost`) + :rtype: (bool, :class:`~certbot_apache.obj.VirtualHost`) """ min_apache_ver = (2, 3, 3) diff --git a/certbot-nginx/certbot_nginx/tls_sni_01.py b/certbot-nginx/certbot_nginx/tls_sni_01.py index 48e117bba..d6faa12be 100644 --- a/certbot-nginx/certbot_nginx/tls_sni_01.py +++ b/certbot-nginx/certbot_nginx/tls_sni_01.py @@ -115,7 +115,7 @@ class NginxTlsSni01(common.TLSSNI01): break if not included: raise errors.MisconfigurationError( - 'LetsEncrypt could not find an HTTP block to include ' + 'Certbot could not find an HTTP block to include ' 'TLS-SNI-01 challenges in %s.' % root) config = [self._make_server_block(pair[0], pair[1]) From 7c1115881012d62c9903c60a0fd57023b641b8d4 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Oct 2017 17:00:13 -0700 Subject: [PATCH 20/24] Retry failures to start boulder (#5176) Occasionally a network error prevents Docker from starting boulder causing Travis tests to fail like it did at https://travis-ci.org/certbot/certbot/jobs/282923098. This works around the problem by using travis_retry to try to start boulder again if it fails. This also moves the logic of waiting for boulder to start into tests/boulder-fetch.sh so people running integration tests locally can benefit. --- .travis.yml | 4 +++- tests/boulder-fetch.sh | 6 ++++++ tests/tox-boulder-integration.sh | 12 ++++++++++++ tests/travis-integration.sh | 14 -------------- 4 files changed, 21 insertions(+), 15 deletions(-) create mode 100755 tests/tox-boulder-integration.sh delete mode 100755 tests/travis-integration.sh diff --git a/.travis.yml b/.travis.yml index b27081c24..48b9b43cb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -161,7 +161,9 @@ addons: - libapache2-mod-macro install: "travis_retry pip install tox coveralls" -script: 'travis_retry tox && ([ "xxx$BOULDER_INTEGRATION" = "xxx" ] || ./tests/travis-integration.sh)' +script: + - travis_retry tox + - '[ -z "${BOULDER_INTEGRATION+x}" ] || (travis_retry tests/boulder-fetch.sh && tests/tox-boulder-integration.sh)' after_success: '[ "$TOXENV" == "cover" ] && coveralls' diff --git a/tests/boulder-fetch.sh b/tests/boulder-fetch.sh index 60538362e..08eb736c2 100755 --- a/tests/boulder-fetch.sh +++ b/tests/boulder-fetch.sh @@ -17,3 +17,9 @@ FAKE_DNS=$(ifconfig docker0 | grep "inet addr:" | cut -d: -f2 | awk '{ print $1} [ -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 + +set +x # reduce verbosity while waiting for boulder +until curl http://localhost:4000/directory 2>/dev/null; do + echo waiting for boulder + sleep 1 +done diff --git a/tests/tox-boulder-integration.sh b/tests/tox-boulder-integration.sh new file mode 100755 index 000000000..8c8a967fd --- /dev/null +++ b/tests/tox-boulder-integration.sh @@ -0,0 +1,12 @@ +#!/bin/bash -e +# A simple wrapper around tests/boulder-integration.sh that activates the tox +# virtual environment defined by the environment variable TOXENV before running +# integration tests. + +if [ -z "${TOXENV+x}" ]; then + echo "The environment variable TOXENV must be set to use this script!" >&2 + exit 1 +fi + +source .tox/$TOXENV/bin/activate +tests/boulder-integration.sh diff --git a/tests/travis-integration.sh b/tests/travis-integration.sh deleted file mode 100755 index b42617400..000000000 --- a/tests/travis-integration.sh +++ /dev/null @@ -1,14 +0,0 @@ -#!/bin/bash - -set -o errexit - -./tests/boulder-fetch.sh - -source .tox/$TOXENV/bin/activate - -until curl http://boulder:4000/directory 2>/dev/null; do - echo waiting for boulder - sleep 1 -done - -./tests/boulder-integration.sh From 99f00d21c414eb244d65e8020579d95eb07a0354 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Fri, 13 Oct 2017 22:25:33 +0300 Subject: [PATCH 21/24] Skip menu in webroot plugin when there's nothing to choose from (#5183) * Skip menu in webroot, when there's nothing to choose from * Added testcase --- certbot/plugins/webroot.py | 21 +++++++++++++++------ certbot/plugins/webroot_test.py | 31 ++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/certbot/plugins/webroot.py b/certbot/plugins/webroot.py index 4662b2aa6..714d83cce 100644 --- a/certbot/plugins/webroot.py +++ b/certbot/plugins/webroot.py @@ -102,10 +102,14 @@ to serve all files under specified web root ({0}).""" webroot = None while webroot is None: - webroot = self._prompt_with_webroot_list(domain, known_webroots) - - if webroot is None: - webroot = self._prompt_for_new_webroot(domain) + if known_webroots: + # Only show the menu if we have options for it + webroot = self._prompt_with_webroot_list(domain, known_webroots) + if webroot is None: + webroot = self._prompt_for_new_webroot(domain) + else: + # Allow prompt to raise PluginError instead of looping forever + webroot = self._prompt_for_new_webroot(domain, True) return webroot @@ -125,13 +129,18 @@ to serve all files under specified web root ({0}).""" else: # code == display_util.OK return None if index == 0 else known_webroots[index - 1] - def _prompt_for_new_webroot(self, domain): + def _prompt_for_new_webroot(self, domain, allowraise=False): code, webroot = ops.validated_directory( _validate_webroot, "Input the webroot for {0}:".format(domain), force_interactive=True) if code == display_util.CANCEL: - return None + if not allowraise: + return None + else: + raise errors.PluginError( + "Every requested domain must have a " + "webroot when using the webroot plugin.") else: # code == display_util.OK return _validate_webroot(webroot) diff --git a/certbot/plugins/webroot_test.py b/certbot/plugins/webroot_test.py index e202a0a6d..5a311716e 100644 --- a/certbot/plugins/webroot_test.py +++ b/certbot/plugins/webroot_test.py @@ -96,7 +96,7 @@ class AuthenticatorTest(unittest.TestCase): @test_util.patch_get_utility() def test_new_webroot(self, mock_get_utility): self.config.webroot_path = [] - self.config.webroot_map = {} + self.config.webroot_map = {"something.com": self.path} mock_display = mock_get_utility() mock_display.menu.return_value = (display_util.OK, 0,) @@ -108,6 +108,19 @@ class AuthenticatorTest(unittest.TestCase): self.assertEqual(self.config.webroot_map[self.achall.domain], self.path) + @test_util.patch_get_utility() + def test_new_webroot_empty_map_cancel(self, mock_get_utility): + self.config.webroot_path = [] + self.config.webroot_map = {} + + mock_display = mock_get_utility() + mock_display.menu.return_value = (display_util.OK, 0,) + with mock.patch('certbot.display.ops.validated_directory') as m: + m.return_value = (display_util.CANCEL, -1) + self.assertRaises(errors.PluginError, + self.auth.perform, + [self.achall]) + def test_perform_missing_root(self): self.config.webroot_path = None self.config.webroot_map = {} @@ -132,6 +145,22 @@ class AuthenticatorTest(unittest.TestCase): mock_chown.side_effect = OSError(errno.EACCES, "msg") self.auth.perform([self.achall]) # exception caught and logged + + @test_util.patch_get_utility() + def test_perform_new_webroot_not_in_map(self, mock_get_utility): + new_webroot = tempfile.mkdtemp() + self.config.webroot_path = [] + self.config.webroot_map = {"whatever.com": self.path} + mock_display = mock_get_utility() + mock_display.menu.side_effect = ((display_util.OK, 0), + (display_util.OK, new_webroot)) + achall = achallenges.KeyAuthorizationAnnotatedChallenge( + challb=acme_util.HTTP01_P, domain="something.com", account_key=KEY) + with mock.patch('certbot.display.ops.validated_directory') as m: + m.return_value = (display_util.OK, new_webroot,) + self.auth.perform([achall]) + self.assertEqual(self.config.webroot_map[achall.domain], new_webroot) + def test_perform_permissions(self): self.auth.prepare() From 95a7d4585619d612ff28ac24dac4faefaee59e72 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Fri, 13 Oct 2017 12:29:02 -0700 Subject: [PATCH 22/24] Nginx creates a vhost block if no matching block is found (#5153) * Allow authentication if there's no appropriate vhost * Update test * add flag to suppress raising error if no match is found * Allow installation if there's no appropriate vhost * remove traceback * make new vhost ssl * Fix existing bugs in nginxparser.py and obj.py * Switch isinstance(x, str) to isinstance(x, six.string_types) in the Nginx plugin * remove unused import * remove unneeded custom copy from Addr * Add docstring for create_new_vhost_from_default * add test for create_new_vhost_from_default * add configurator tests and leave finding the first server block for another PR * don't assume order from a set * address multiple default_server problem * don't add vhosts twice * update unit tests * update docstring * Add logger.info message for using default address in tlssni01 auth --- certbot-nginx/certbot_nginx/configurator.py | 63 +++++++-- certbot-nginx/certbot_nginx/nginxparser.py | 20 +-- certbot-nginx/certbot_nginx/obj.py | 2 +- certbot-nginx/certbot_nginx/parser.py | 33 ++++- .../certbot_nginx/tests/configurator_test.py | 132 ++++++++++++++++++ .../certbot_nginx/tests/parser_test.py | 26 +++- .../testdata/etc_nginx/sites-enabled/default | 1 + .../certbot_nginx/tests/tls_sni_01_test.py | 2 +- certbot-nginx/certbot_nginx/tls_sni_01.py | 11 +- 9 files changed, 258 insertions(+), 32 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 7e55ff0ea..ba6c565c0 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -117,6 +117,9 @@ class NginxConfigurator(common.Installer): # Files to save self.save_notes = "" + # For creating new vhosts if no names match + self.new_vhost = None + # Add number of outstanding challenges self._chall_out = 0 @@ -191,9 +194,11 @@ class NginxConfigurator(common.Installer): "The nginx plugin currently requires --fullchain-path to " "install a cert.") - vhost = self.choose_vhost(domain) - cert_directives = [['\n', 'ssl_certificate', ' ', fullchain_path], - ['\n', 'ssl_certificate_key', ' ', key_path]] + vhost = self.choose_vhost(domain, raise_if_no_match=False) + if vhost is None: + vhost = self._vhost_from_duplicated_default(domain) + cert_directives = [['\n ', 'ssl_certificate', ' ', fullchain_path], + ['\n ', 'ssl_certificate_key', ' ', key_path]] self.parser.add_server_directives(vhost, cert_directives, replace=True) @@ -209,7 +214,7 @@ class NginxConfigurator(common.Installer): ####################### # Vhost parsing methods ####################### - def choose_vhost(self, target_name): + def choose_vhost(self, target_name, raise_if_no_match=True): """Chooses a virtual host based on the given domain name. .. note:: This makes the vhost SSL-enabled if it isn't already. Follows @@ -223,6 +228,8 @@ class NginxConfigurator(common.Installer): hostname. Currently we just ignore this. :param str target_name: domain name + :param bool raise_if_no_match: True iff not finding a match is an error; + otherwise, return None :returns: ssl vhost associated with name :rtype: :class:`~certbot_nginx.obj.VirtualHost` @@ -233,13 +240,16 @@ class NginxConfigurator(common.Installer): matches = self._get_ranked_matches(target_name) vhost = self._select_best_name_match(matches) if not vhost: - # No matches. Raise a misconfiguration error. - raise errors.MisconfigurationError( - ("Cannot find a VirtualHost matching domain %s. " - "In order for Certbot to correctly perform the challenge " - "please add a corresponding server_name directive to your " - "nginx configuration: " - "https://nginx.org/en/docs/http/server_names.html") % (target_name)) + if raise_if_no_match: + # No matches. Raise a misconfiguration error. + raise errors.MisconfigurationError( + ("Cannot find a VirtualHost matching domain %s. " + "In order for Certbot to correctly perform the challenge " + "please add a corresponding server_name directive to your " + "nginx configuration: " + "https://nginx.org/en/docs/http/server_names.html") % (target_name)) + else: + return None else: # Note: if we are enhancing with ocsp, vhost should already be ssl. if not vhost.ssl: @@ -247,6 +257,37 @@ class NginxConfigurator(common.Installer): return vhost + def _vhost_from_duplicated_default(self, domain): + if self.new_vhost is None: + default_vhost = self._get_default_vhost() + self.new_vhost = self.parser.create_new_vhost_from_default(default_vhost) + if not self.new_vhost.ssl: + self._make_server_ssl(self.new_vhost) + self.new_vhost.names = set() + + self.new_vhost.names.add(domain) + name_block = [['\n ', 'server_name', ' ', ' '.join(self.new_vhost.names)]] + self.parser.add_server_directives(self.new_vhost, name_block, replace=True) + return self.new_vhost + + def _get_default_vhost(self): + vhost_list = self.parser.get_vhosts() + # if one has default_server set, return that one + default_vhosts = [] + for vhost in vhost_list: + for addr in vhost.addrs: + if addr.default: + default_vhosts.append(vhost) + break + + if len(default_vhosts) == 1: + return default_vhosts[0] + + # TODO: present a list of vhosts for user to choose from + + raise errors.MisconfigurationError("Could not automatically find a matching server" + " block. Set the `server_name` directive to use the Nginx installer.") + def _get_ranked_matches(self, target_name): """Returns a ranked list of vhosts that match target_name. The ranking gives preference to SSL vhosts. diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 20aeeb554..14481e298 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -7,6 +7,7 @@ from pyparsing import ( Literal, White, Forward, Group, Optional, OneOrMore, QuotedString, Regex, ZeroOrMore, Combine) from pyparsing import stringEnd from pyparsing import restOfLine +import six logger = logging.getLogger(__name__) @@ -71,7 +72,7 @@ class RawNginxDumper(object): """Iterates the dumped nginx content.""" blocks = blocks or self.blocks for b0 in blocks: - if isinstance(b0, str): + if isinstance(b0, six.string_types): yield b0 continue item = copy.deepcopy(b0) @@ -88,7 +89,7 @@ class RawNginxDumper(object): yield '}' else: # not a block - list of strings semicolon = ";" - if isinstance(item[0], str) and item[0].strip() == '#': # comment + if isinstance(item[0], six.string_types) and item[0].strip() == '#': # comment semicolon = "" yield "".join(item) + semicolon @@ -145,7 +146,7 @@ def dump(blocks, _file): return _file.write(dumps(blocks)) -spacey = lambda x: (isinstance(x, str) and x.isspace()) or x == '' +spacey = lambda x: (isinstance(x, six.string_types) and x.isspace()) or x == '' class UnspacedList(list): """Wrap a list [of lists], making any whitespace entries magically invisible""" @@ -189,13 +190,15 @@ class UnspacedList(list): item, spaced_item = self._coerce(x) slicepos = self._spaced_position(i) if i < len(self) else len(self.spaced) self.spaced.insert(slicepos, spaced_item) - list.insert(self, i, item) + if not spacey(item): + list.insert(self, i, item) self.dirty = True def append(self, x): item, spaced_item = self._coerce(x) self.spaced.append(spaced_item) - list.append(self, item) + if not spacey(item): + list.append(self, item) self.dirty = True def extend(self, x): @@ -226,7 +229,8 @@ class UnspacedList(list): raise NotImplementedError("Slice operations on UnspacedLists not yet implemented") item, spaced_item = self._coerce(value) self.spaced.__setitem__(self._spaced_position(i), spaced_item) - list.__setitem__(self, i, item) + if not spacey(item): + list.__setitem__(self, i, item) self.dirty = True def __delitem__(self, i): @@ -235,8 +239,8 @@ class UnspacedList(list): self.dirty = True def __deepcopy__(self, memo): - l = UnspacedList(self[:]) - l.spaced = copy.deepcopy(self.spaced, memo=memo) + new_spaced = copy.deepcopy(self.spaced, memo=memo) + l = UnspacedList(new_spaced) l.dirty = self.dirty return l diff --git a/certbot-nginx/certbot_nginx/obj.py b/certbot-nginx/certbot_nginx/obj.py index 849cefe1f..d7604bdf9 100644 --- a/certbot-nginx/certbot_nginx/obj.py +++ b/certbot-nginx/certbot_nginx/obj.py @@ -198,7 +198,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods def _find_directive(directives, directive_name): """Find a directive of type directive_name in directives """ - if not directives or isinstance(directives, str) or len(directives) == 0: + if not directives or isinstance(directives, six.string_types) or len(directives) == 0: return None if directives[0] == directive_name: diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 158cb9929..3eb6264aa 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -6,6 +6,8 @@ import os import pyparsing import re +import six + from certbot import errors from certbot_nginx import obj @@ -312,6 +314,32 @@ class NginxParser(object): except errors.MisconfigurationError as err: raise errors.MisconfigurationError("Problem in %s: %s" % (filename, str(err))) + def create_new_vhost_from_default(self, vhost_template): + """Duplicate the default vhost in the configuration files. + + :param :class:`~certbot_nginx.obj.VirtualHost` vhost_template: The vhost + whose information we copy + + :returns: A vhost object for the newly created vhost + :rtype: :class:`~certbot_nginx.obj.VirtualHost` + """ + # TODO: https://github.com/certbot/certbot/issues/5185 + # put it in the same file as the template, at the same level + enclosing_block = self.parsed[vhost_template.filep] + for index in vhost_template.path[:-1]: + enclosing_block = enclosing_block[index] + new_location = vhost_template.path[-1] + 1 + raw_in_parsed = copy.deepcopy(enclosing_block[vhost_template.path[-1]]) + enclosing_block.insert(new_location, raw_in_parsed) + new_vhost = copy.deepcopy(vhost_template) + new_vhost.path[-1] = new_location + for addr in new_vhost.addrs: + addr.default = False + for directive in enclosing_block[new_vhost.path[-1]][1]: + if len(directive) > 0 and directive[0] == 'listen' and 'default_server' in directive: + del directive[directive.index('default_server')] + return new_vhost + def _parse_ssl_options(ssl_options): if ssl_options is not None: try: @@ -444,7 +472,7 @@ def _is_include_directive(entry): """ return (isinstance(entry, list) and len(entry) == 2 and entry[0] == 'include' and - isinstance(entry[1], str)) + isinstance(entry[1], six.string_types)) def _is_ssl_on_directive(entry): """Checks if an nginx parsed entry is an 'ssl on' directive. @@ -561,7 +589,8 @@ def _add_directive(block, directive, replace): directive_name = directive[0] def can_append(loc, dir_name): """ Can we append this directive to the block? """ - return loc is None or (isinstance(dir_name, str) and dir_name in REPEATABLE_DIRECTIVES) + return loc is None or (isinstance(dir_name, six.string_types) + and dir_name in REPEATABLE_DIRECTIVES) err_fmt = 'tried to insert directive "{0}" but found conflicting "{1}".' diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 6a917204c..860186693 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -542,6 +542,138 @@ class NginxConfiguratorTest(util.NginxTest): self.assertTrue(util.contains_at_depth( generated_conf, ['ssl_stapling_verify', 'on'], 2)) + def test_deploy_no_match_default_set(self): + default_conf = self.config.parser.abs_path('sites-enabled/default') + foo_conf = self.config.parser.abs_path('foo.conf') + del self.config.parser.parsed[foo_conf][2][1][0][1][0] # remove default_server + self.config.version = (1, 3, 1) + + self.config.deploy_cert( + "www.nomatch.com", + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") + self.config.save() + + self.config.parser.load() + + parsed_default_conf = util.filter_comments(self.config.parser.parsed[default_conf]) + + self.assertEqual([[['server'], + [['listen', 'myhost', 'default_server'], + ['listen', 'otherhost', 'default_server'], + ['server_name', 'www.example.org'], + [['location', '/'], + [['root', 'html'], + ['index', 'index.html', 'index.htm']]]]], + [['server'], + [['listen', 'myhost'], + ['listen', 'otherhost'], + ['server_name', 'www.nomatch.com'], + [['location', '/'], + [['root', 'html'], + ['index', 'index.html', 'index.htm']]], + ['listen', '5001', 'ssl'], + ['ssl_certificate', 'example/fullchain.pem'], + ['ssl_certificate_key', 'example/key.pem'], + ['include', self.config.mod_ssl_conf], + ['ssl_dhparam', self.config.ssl_dhparams]]]], + parsed_default_conf) + + self.config.deploy_cert( + "nomatch.com", + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") + self.config.save() + + self.config.parser.load() + + parsed_default_conf = util.filter_comments(self.config.parser.parsed[default_conf]) + + self.assertTrue(util.contains_at_depth(parsed_default_conf, "nomatch.com", 3)) + + def test_deploy_no_match_default_set_multi_level_path(self): + default_conf = self.config.parser.abs_path('sites-enabled/default') + foo_conf = self.config.parser.abs_path('foo.conf') + del self.config.parser.parsed[default_conf][0][1][0] + del self.config.parser.parsed[default_conf][0][1][0] + self.config.version = (1, 3, 1) + + self.config.deploy_cert( + "www.nomatch.com", + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") + self.config.save() + + self.config.parser.load() + + parsed_foo_conf = util.filter_comments(self.config.parser.parsed[foo_conf]) + + self.assertEqual([['server'], + [['listen', '*:80', 'ssl'], + ['server_name', 'www.nomatch.com'], + ['root', '/home/ubuntu/sites/foo/'], + [['location', '/status'], [[['types'], [['image/jpeg', 'jpg']]]]], + [['location', '~', 'case_sensitive\\.php$'], [['index', 'index.php'], + ['root', '/var/root']]], + [['location', '~*', 'case_insensitive\\.php$'], []], + [['location', '=', 'exact_match\\.php$'], []], + [['location', '^~', 'ignore_regex\\.php$'], []], + ['ssl_certificate', 'example/fullchain.pem'], + ['ssl_certificate_key', 'example/key.pem']]], + parsed_foo_conf[1][1][1]) + + def test_deploy_no_match_no_default_set(self): + default_conf = self.config.parser.abs_path('sites-enabled/default') + foo_conf = self.config.parser.abs_path('foo.conf') + del self.config.parser.parsed[default_conf][0][1][0] + del self.config.parser.parsed[default_conf][0][1][0] + del self.config.parser.parsed[foo_conf][2][1][0][1][0] + self.config.version = (1, 3, 1) + + self.assertRaises(errors.MisconfigurationError, self.config.deploy_cert, + "www.nomatch.com", "example/cert.pem", "example/key.pem", + "example/chain.pem", "example/fullchain.pem") + + def test_deploy_no_match_fail_multiple_defaults(self): + self.config.version = (1, 3, 1) + self.assertRaises(errors.MisconfigurationError, self.config.deploy_cert, + "www.nomatch.com", "example/cert.pem", "example/key.pem", + "example/chain.pem", "example/fullchain.pem") + + def test_deploy_no_match_add_redirect(self): + default_conf = self.config.parser.abs_path('sites-enabled/default') + foo_conf = self.config.parser.abs_path('foo.conf') + del self.config.parser.parsed[foo_conf][2][1][0][1][0] # remove default_server + self.config.version = (1, 3, 1) + + self.config.deploy_cert( + "www.nomatch.com", + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") + + self.config.enhance("www.nomatch.com", "redirect") + + self.config.save() + + self.config.parser.load() + + expected = [ + ['if', '($scheme', '!=', '"https")'], + [['return', '301', 'https://$host$request_uri']] + ] + + generated_conf = self.config.parser.parsed[default_conf] + self.assertTrue(util.contains_at_depth(generated_conf, expected, 2)) + + class InstallSslOptionsConfTest(util.NginxTest): """Test that the options-ssl-nginx.conf file is installed and updated properly.""" diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index e655bc3e3..ec0cfd288 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -139,7 +139,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods False, True, set(['.example.com', 'example.*']), [], [0]) vhost4 = obj.VirtualHost(nparser.abs_path('sites-enabled/default'), - [obj.Addr('myhost', '', False, True)], + [obj.Addr('myhost', '', False, True), + obj.Addr('otherhost', '', False, True)], False, True, set(['www.example.org']), [], [0]) vhost5 = obj.VirtualHost(nparser.abs_path('foo.conf'), @@ -395,6 +396,29 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods ]) self.assertTrue(server['ssl']) + def test_create_new_vhost_from_default(self): + nparser = parser.NginxParser(self.config_path) + + vhosts = nparser.get_vhosts() + default = [x for x in vhosts if 'default' in x.filep][0] + new_vhost = nparser.create_new_vhost_from_default(default) + nparser.filedump(ext='') + + # check properties of new vhost + self.assertFalse(next(iter(new_vhost.addrs)).default) + self.assertNotEqual(new_vhost.path, default.path) + + # check that things are written to file correctly + new_nparser = parser.NginxParser(self.config_path) + new_vhosts = new_nparser.get_vhosts() + new_defaults = [x for x in new_vhosts if 'default' in x.filep] + self.assertEqual(len(new_defaults), 2) + new_vhost_parsed = new_defaults[1] + self.assertFalse(next(iter(new_vhost_parsed.addrs)).default) + self.assertEqual(next(iter(default.names)), next(iter(new_vhost_parsed.names))) + self.assertEqual(len(default.raw), len(new_vhost_parsed.raw)) + self.assertTrue(next(iter(default.addrs)).super_eq(next(iter(new_vhost_parsed.addrs)))) + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/default b/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/default index 26f37020c..4f67fa7d1 100644 --- a/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/default +++ b/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/default @@ -1,5 +1,6 @@ server { listen myhost default_server; + listen otherhost default_server; server_name www.example.org; location / { diff --git a/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py b/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py index 85db584b3..af477c460 100644 --- a/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py +++ b/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py @@ -66,7 +66,7 @@ class TlsSniPerformTest(util.NginxTest): self.sni.add_chall(self.achalls[1]) mock_choose.return_value = None result = self.sni.perform() - self.assertTrue(result is None) + self.assertFalse(result is None) def test_perform0(self): responses = self.sni.perform() diff --git a/certbot-nginx/certbot_nginx/tls_sni_01.py b/certbot-nginx/certbot_nginx/tls_sni_01.py index d6faa12be..bbfecc282 100644 --- a/certbot-nginx/certbot_nginx/tls_sni_01.py +++ b/certbot-nginx/certbot_nginx/tls_sni_01.py @@ -52,18 +52,13 @@ class NginxTlsSni01(common.TLSSNI01): self.configurator.config.tls_sni_01_port) for achall in self.achalls: - vhost = self.configurator.choose_vhost(achall.domain) - if vhost is None: - logger.error( - "No nginx vhost exists with server_name matching: %s. " - "Please specify server_names in the Nginx config.", - achall.domain) - return None + vhost = self.configurator.choose_vhost(achall.domain, raise_if_no_match=False) - if vhost.addrs: + if vhost is not None and vhost.addrs: addresses.append(list(vhost.addrs)) else: addresses.append([obj.Addr.fromstring(default_addr)]) + logger.info("Using default address %s for TLSSNI01 authentication.", default_addr) # Create challenge certs responses = [self._setup_challenge_cert(x) for x in self.achalls] From 5d2f6eb8ed41fd90005ddd15779fa6d4971322ea Mon Sep 17 00:00:00 2001 From: Felix Yan Date: Thu, 19 Oct 2017 13:23:07 -0500 Subject: [PATCH 23/24] Fix typos in certbot_apache/tests/configurator_test.py (#5193) --- certbot-apache/certbot_apache/tests/configurator_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 1d6dd9b05..7c6b071da 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -363,7 +363,7 @@ class MultipleVhostsTest(util.ApacheTest): confvars = {"handle-sites": False} if arg in confvars: return confvars[arg] - inc_path = "/path/to/whereever" + inc_path = "/path/to/wherever" vhost = self.vh_truth[0] with mock.patch(mock_c) as mock_conf: mock_conf.side_effect = conf_side_effect @@ -1367,7 +1367,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertFalse(self.config._check_aug_version()) class AugeasVhostsTest(util.ApacheTest): - """Test vhosts with illegal names dependant on augeas version.""" + """Test vhosts with illegal names dependent on augeas version.""" # pylint: disable=protected-access _multiprocess_can_split_ = True @@ -1451,7 +1451,7 @@ class AugeasVhostsTest(util.ApacheTest): broken_vhost) class MultiVhostsTest(util.ApacheTest): - """Test vhosts with illegal names dependant on augeas version.""" + """Test vhosts with illegal names dependent on augeas version.""" # pylint: disable=protected-access def setUp(self): # pylint: disable=arguments-differ From 3c1dafa9e9ad78ec41e7cb9509ea3f67984bbf7f Mon Sep 17 00:00:00 2001 From: ohemorange Date: Thu, 19 Oct 2017 14:56:53 -0700 Subject: [PATCH 24/24] Correctly test for existing Certbot redirect when adding an Nginx redirect block (#5192) * add test that should fail on completion of this PR * fix double redirect problem * update existing test to match new whitespace --- certbot-nginx/certbot_nginx/configurator.py | 2 +- .../certbot_nginx/tests/configurator_test.py | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index ba6c565c0..4857942a1 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -30,7 +30,7 @@ from certbot_nginx import parser logger = logging.getLogger(__name__) REDIRECT_BLOCK = [[ - ['\n ', 'if', ' ', '($scheme', ' ', '!=', ' ', '"https") '], + ['\n ', 'if', ' ', '($scheme', ' ', '!=', ' ', '"https")'], [['\n ', 'return', ' ', '301', ' ', 'https://$host$request_uri'], '\n '] ], ['\n']] diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 860186693..4f9f685c2 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -429,7 +429,7 @@ class NginxConfiguratorTest(util.NginxTest): # Test that we successfully add a redirect when there is # a listen directive expected = [ - ['if', '($scheme', '!=', '"https") '], + ['if', '($scheme', '!=', '"https")'], [['return', '301', 'https://$host$request_uri']] ] @@ -512,6 +512,23 @@ class NginxConfiguratorTest(util.NginxTest): self.assertEqual(mock_logger.info.call_args[0][0], 'No matching insecure server blocks listening on port %s found.') + def test_no_double_redirect(self): + # Test that we don't also add the commented redirect if we've just added + # a redirect to that vhost this run + example_conf = self.config.parser.abs_path('sites-enabled/example.com') + self.config.enhance("example.com", "redirect") + self.config.enhance("example.org", "redirect") + + unexpected = [ + ['#', ' Redirect non-https traffic to https'], + ['#', ' if ($scheme != "https") {'], + ['#', ' return 301 https://$host$request_uri;'], + ['#', ' } # managed by Certbot'] + ] + generated_conf = self.config.parser.parsed[example_conf] + for line in unexpected: + self.assertFalse(util.contains_at_depth(generated_conf, line, 2)) + def test_staple_ocsp_bad_version(self): self.config.version = (1, 3, 1) self.assertRaises(errors.PluginError, self.config.enhance,