From a718cfede0f44cbc0c40ae396911fa3f510c3c7e Mon Sep 17 00:00:00 2001 From: sagi Date: Sun, 3 Jan 2016 22:03:47 +0000 Subject: [PATCH 01/17] Copy only relevant lines from http vhost to ssl vhost skeleton --- .../letsencrypt_apache/configurator.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 836d7713..0d674963 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -712,8 +712,19 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): with open(avail_fp, "r") as orig_file: with open(ssl_fp, "w") as new_file: new_file.write("\n") + + # In some cases we wouldn't want to copy the exact + # directives used in an http vhost to a ssl vhost. + # An example: + # If there's a redirect rewrite rule directive installed in + # the http vhost - copying it to the ssl vhost would cause + # a redirection loop. + blacklist_set = set(['RewriteRule', 'RewriteEngine']) + for line in orig_file: - new_file.write(line) + line_set = set(line.split()) + if not line_set & blacklist_set: # & -> Intersection + new_file.write(line) new_file.write("\n") except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") From e1b4797cbfae87b59d1b3282992a6b605e578a7f Mon Sep 17 00:00:00 2001 From: Wilfried Teiken Date: Tue, 5 Jan 2016 01:12:21 -0500 Subject: [PATCH 02/17] Change the semantics of query_registration and update_registration to set new_authzr_uri from the server if available --- acme/acme/client.py | 12 +++++------- acme/acme/client_test.py | 7 +++++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index c3e28ef4..3d84cb3b 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -66,15 +66,13 @@ def __init__(self, directory, key, alg=jose.RS256, verify_ssl=True, @classmethod def _regr_from_response(cls, response, uri=None, new_authzr_uri=None, terms_of_service=None): - terms_of_service = ( - response.links['terms-of-service']['url'] - if 'terms-of-service' in response.links else terms_of_service) + if 'terms-of-service' in response.links: + terms_of_service = response.links['terms-of-service']['url'] + if 'next' in response.links: + new_authzr_uri = response.links['next']['url'] if new_authzr_uri is None: - try: - new_authzr_uri = response.links['next']['url'] - except KeyError: - raise errors.ClientError('"next" link missing') + raise errors.ClientError(response, 'missing "new_authrz_uri"') return messages.RegistrationResource( body=messages.Registration.from_json(response.json()), diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 58f55b29..9e3ea3d8 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -127,6 +127,13 @@ def test_query_registration(self): self.response.json.return_value = self.regr.body.to_json() self.assertEqual(self.regr, self.client.query_registration(self.regr)) + def test_query_registration_updates_new_authzr_uri(self): + self.response.json.return_value = self.regr.body.to_json() + self.response.links = {'next': {'url': 'UPDATED'}} + self.assertEqual( + 'UPDATED', + self.client.query_registration(self.regr).new_authzr_uri) + def test_agree_to_tos(self): self.client.update_registration = mock.Mock() self.client.agree_to_tos(self.regr) From 7bd7e7ca23420439215e9fe07c9b9ec65f773c54 Mon Sep 17 00:00:00 2001 From: wteiken Date: Tue, 5 Jan 2016 19:51:45 -0500 Subject: [PATCH 03/17] Remove response argument from exception and fix eror messages. --- 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 3d84cb3b..f3c2c605 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -72,7 +72,7 @@ def _regr_from_response(cls, response, uri=None, new_authzr_uri=None, new_authzr_uri = response.links['next']['url'] if new_authzr_uri is None: - raise errors.ClientError(response, 'missing "new_authrz_uri"') + raise errors.ClientError('"next" link missing') return messages.RegistrationResource( body=messages.Registration.from_json(response.json()), From c10bfd6efc6ff27efe94e8a397b809c4faf55986 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Sun, 10 Jan 2016 14:01:34 +0000 Subject: [PATCH 04/17] Fix wrong doc comment: account_public_key is None --- acme/acme/challenges.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index 68bf3fce..13d19d3c 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -236,10 +236,8 @@ def simple_verify(self, chall, domain, account_public_key, port=None): :param challenges.SimpleHTTP chall: Corresponding challenge. :param unicode domain: Domain name being verified. - :param account_public_key: Public key for the key pair - being authorized. If ``None`` key verification is not - performed! - :param JWK account_public_key: + :param JWK account_public_key: Public key for the key pair + being authorized. :param int port: Port used in the validation. :returns: ``True`` iff validation is successful, ``False`` From 0a536d50bea24df181788693ff2751411ff6ed4f Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Sun, 10 Jan 2016 17:31:50 +0000 Subject: [PATCH 05/17] Remove dead code (error in except) --- 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 c3e28ef4..7ff74035 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -539,7 +539,7 @@ def _check_response(cls, response, content_type=None): # TODO: response.json() is called twice, once here, and # once in _get and _post clients jobj = response.json() - except ValueError as error: + except ValueError: jobj = None if not response.ok: From fac2ed41d8eaf689e0a565f89a3b5993705165d6 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Sun, 10 Jan 2016 18:17:35 +0000 Subject: [PATCH 06/17] ACME: pylint to 80 chars --- acme/.pylintrc | 2 +- acme/acme/challenges_test.py | 16 ++++++++++------ acme/acme/client_test.py | 9 ++++++--- acme/acme/messages.py | 5 +++-- acme/acme/standalone_test.py | 15 ++++++++------- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/acme/.pylintrc b/acme/.pylintrc index 33650310..d0d15063 100644 --- a/acme/.pylintrc +++ b/acme/.pylintrc @@ -100,7 +100,7 @@ comment=no [FORMAT] # Maximum number of characters on a single line. -max-line-length=100 +max-line-length=80 # Regexp for a line that is allowed to be longer than the limit. ignore-long-lines=^\s*(# )??$ diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index 4f2d0616..ef78e1eb 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -73,7 +73,8 @@ def test_verify_wrong_thumbprint(self): def test_verify_wrong_form(self): from acme.challenges import KeyAuthorizationChallengeResponse response = KeyAuthorizationChallengeResponse( - key_authorization='.foo.oKGqedy-b-acd5eoybm2f-NVFxvyOoET5CNy3xnv8WY') + key_authorization='.foo.oKGqedy-b-acd5eoybm2f-' + 'NVFxvyOoET5CNy3xnv8WY') self.assertFalse(response.verify(self.chall, KEY.public_key())) @@ -273,10 +274,12 @@ def test_simple_verify_bad_key_authorization(self): @mock.patch('acme.challenges.TLSSNI01Response.verify_cert', autospec=True) def test_simple_verify(self, mock_verify_cert): mock_verify_cert.return_value = mock.sentinel.verification - self.assertEqual(mock.sentinel.verification, self.response.simple_verify( - self.chall, self.domain, KEY.public_key(), - cert=mock.sentinel.cert)) - mock_verify_cert.assert_called_once_with(self.response, mock.sentinel.cert) + self.assertEqual( + mock.sentinel.verification, self.response.simple_verify( + self.chall, self.domain, KEY.public_key(), + cert=mock.sentinel.cert)) + mock_verify_cert.assert_called_once_with( + self.response, mock.sentinel.cert) @mock.patch('acme.challenges.TLSSNI01Response.probe_cert') def test_simple_verify_false_on_probe_error(self, mock_probe_cert): @@ -590,7 +593,8 @@ def test_check_validation_wrong_payload(self): def test_check_validation_wrong_fields(self): bad_validation = jose.JWS.sign( - payload=self.msg.update(token=b'x' * 20).json_dumps().encode('utf-8'), + payload=self.msg.update( + token=b'x' * 20).json_dumps().encode('utf-8'), alg=jose.RS256, key=KEY) self.assertFalse(self.msg.check_validation( bad_validation, KEY.public_key())) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 58f55b29..863fe350 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -34,8 +34,10 @@ def setUp(self): self.net.get.return_value = self.response self.directory = messages.Directory({ - messages.NewRegistration: 'https://www.letsencrypt-demo.org/acme/new-reg', - messages.Revocation: 'https://www.letsencrypt-demo.org/acme/revoke-cert', + messages.NewRegistration: + 'https://www.letsencrypt-demo.org/acme/new-reg', + messages.Revocation: + 'https://www.letsencrypt-demo.org/acme/revoke-cert', }) from acme.client import Client @@ -331,7 +333,8 @@ def request_issuance(csr, authzrs): # pylint: disable=missing-docstring self.assertEqual(clock.dt, datetime.datetime(2015, 3, 27, 0, 1, 7)) # CA sets invalid | TODO: move to a separate test - invalid_authzr = mock.MagicMock(times=[], retries=[messages.STATUS_INVALID]) + invalid_authzr = mock.MagicMock( + times=[], retries=[messages.STATUS_INVALID]) self.assertRaises( errors.PollError, self.client.poll_and_request_issuance, csr, authzrs=(invalid_authzr,), mintime=mintime) diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 9c6a5f7b..06b4492d 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -130,8 +130,9 @@ def _canon_key(cls, key): @classmethod def register(cls, resource_body_cls): """Register resource.""" - assert resource_body_cls.resource_type not in cls._REGISTERED_TYPES - cls._REGISTERED_TYPES[resource_body_cls.resource_type] = resource_body_cls + resource_type = resource_body_cls.resource_type + assert resource_type not in cls._REGISTERED_TYPES + cls._REGISTERED_TYPES[resource_type] = resource_body_cls return resource_body_cls def __init__(self, jobj): diff --git a/acme/acme/standalone_test.py b/acme/acme/standalone_test.py index 2778635f..85cd9d11 100644 --- a/acme/acme/standalone_test.py +++ b/acme/acme/standalone_test.py @@ -32,11 +32,10 @@ class TLSSNI01ServerTest(unittest.TestCase): """Test for acme.standalone.TLSSNI01Server.""" def setUp(self): - self.certs = { - b'localhost': (test_util.load_pyopenssl_private_key('rsa512_key.pem'), - # pylint: disable=protected-access - test_util.load_cert('cert.pem')), - } + self.certs = {b'localhost': ( + test_util.load_pyopenssl_private_key('rsa512_key.pem'), + test_util.load_cert('cert.pem'), + )} from acme.standalone import TLSSNI01Server self.server = TLSSNI01Server(("", 0), certs=self.certs) # pylint: disable=no-member @@ -49,7 +48,8 @@ def tearDown(self): def test_it(self): host, port = self.server.socket.getsockname()[:2] - cert = crypto_util.probe_sni(b'localhost', host=host, port=port, timeout=1) + cert = crypto_util.probe_sni( + b'localhost', host=host, port=port, timeout=1) self.assertEqual(jose.ComparableX509(cert), jose.ComparableX509(self.certs[b'localhost'][1])) @@ -140,7 +140,8 @@ def test_it(self): while max_attempts: max_attempts -= 1 try: - cert = crypto_util.probe_sni(b'localhost', b'0.0.0.0', self.port) + cert = crypto_util.probe_sni( + b'localhost', b'0.0.0.0', self.port) except errors.Error: self.assertTrue(max_attempts > 0, "Timeout!") time.sleep(1) # wait until thread starts From bdd9fa44854b2e25211fd35425337fdc1d73b6cd Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Sun, 10 Jan 2016 18:47:04 +0000 Subject: [PATCH 07/17] Quickfix too-many-instance-attributes. https://github.com/letsencrypt/letsencrypt/pull/2135#issuecomment-170381179 --- 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 7ff74035..b65577c5 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -483,7 +483,7 @@ def revoke(self, cert): 'Successful revocation must return HTTP OK status') -class ClientNetwork(object): +class ClientNetwork(object): # pylint: disable=too-many-instance-attributes """Client network.""" JSON_CONTENT_TYPE = 'application/json' JSON_ERROR_CONTENT_TYPE = 'application/problem+json' From bf74b2cc644c0ffa1e29f0694b25af214b09bf18 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:12:30 +0000 Subject: [PATCH 08/17] Change test RewriteRule so that it conforms with Apaches spec. --- .../letsencrypt_apache/tests/configurator_test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 9838b4f5..599334a2 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -857,7 +857,8 @@ def test_redirect_with_existing_rewrite(self, mock_exe, _): # Create a preexisting rewrite rule self.config.parser.add_dir( - self.vh_truth[3].path, "RewriteRule", ["Unknown"]) + self.vh_truth[3].path, "RewriteRule", ["UnknownPattern", + "UnknownTarget"]) self.config.save() # This will create an ssl vhost for letsencrypt.demo @@ -872,7 +873,7 @@ def test_redirect_with_existing_rewrite(self, mock_exe, _): self.assertEqual(len(rw_engine), 1) # three args to rw_rule + 1 arg for the pre existing rewrite - self.assertEqual(len(rw_rule), 4) + self.assertEqual(len(rw_rule), 5) self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path)) self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path)) From 6c18a7d318c477ed0ffae1aa3e57f5bd197fa1aa Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:15:23 +0000 Subject: [PATCH 09/17] Revise RewriteRule sifting algorithm --- .../letsencrypt_apache/configurator.py | 63 ++++++++++++++++--- 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 948514f9..288ea9dc 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -696,6 +696,42 @@ def _get_ssl_vhost_path(self, non_ssl_vh_fp): return non_ssl_vh_fp[:-(len(".conf"))] + self.conf("le_vhost_ext") else: return non_ssl_vh_fp + self.conf("le_vhost_ext") + + def _sift_line(self, line): + """ Decides whether a line shouldn't be copied from a http vhost to a + SSL vhost. + + A canonical example of when sifting a line is required: + When the http vhost contains a RewriteRule that unconditionally redirects + any request to the https version of the same site. + e.g: RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,QSA,R=permanent] + Copying the above line to the ssl vhost would cause a redirection loop. + + :param str line: a line extracted from the http vhost config file. + + :returns: True - don't copy line from http vhost to SSL vhost. + :rtype: (bool) + """ + + rewrite_rule = "RewriteRule" + if line.lstrip()[:len(rewrite_rule)] != rewrite_rule: + return False + # line starts with RewriteRule. + + # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html + # The syntax of a RewriteRule is: + # RewriteRule pattern target [Flag1,Flag2,Flag3] + # i.e. target is required, so it must exist. + target = line.split()[2].strip() + + https_prefix = "https://" + if len(target) skeleton. @@ -714,20 +750,27 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): with open(avail_fp, "r") as orig_file: with open(ssl_fp, "w") as new_file: new_file.write("\n") - - # In some cases we wouldn't want to copy the exact - # directives used in an http vhost to a ssl vhost. - # An example: - # If there's a redirect rewrite rule directive installed in - # the http vhost - copying it to the ssl vhost would cause - # a redirection loop. - blacklist_set = set(['RewriteRule', 'RewriteEngine']) + sift = False for line in orig_file: - line_set = set(line.split()) - if not line_set & blacklist_set: # & -> Intersection + if self._sift_line(line): + if not sift: + new_file.write("# The following rewrite rules" + "were *not* enabled on your HTTPS site, " + "because they have the potential to create " + "redirection loops:") + sift = True + new_file.write("# " + line) + else: new_file.write(line) + new_file.write("\n") + + if sift: + logger.warn("Some rewrite rules were *not* enabled on " + "your HTTPS site, because they have the " + "potential to create redirection loops.") + except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") raise errors.PluginError("Unable to write/read in make_vhost_ssl") From ae572fe0840bfc6052c3c86acfe8df7f530a26e7 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:20:29 +0000 Subject: [PATCH 10/17] Make lint happy --- .../letsencrypt_apache/configurator.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 288ea9dc..13c7c91a 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -696,11 +696,11 @@ def _get_ssl_vhost_path(self, non_ssl_vh_fp): return non_ssl_vh_fp[:-(len(".conf"))] + self.conf("le_vhost_ext") else: return non_ssl_vh_fp + self.conf("le_vhost_ext") - + def _sift_line(self, line): - """ Decides whether a line shouldn't be copied from a http vhost to a + """ Decides whether a line shouldn't be copied from a http vhost to a SSL vhost. - + A canonical example of when sifting a line is required: When the http vhost contains a RewriteRule that unconditionally redirects any request to the https version of the same site. @@ -709,7 +709,7 @@ def _sift_line(self, line): :param str line: a line extracted from the http vhost config file. - :returns: True - don't copy line from http vhost to SSL vhost. + :returns: True - don't copy line from http vhost to SSL vhost. :rtype: (bool) """ @@ -718,14 +718,14 @@ def _sift_line(self, line): return False # line starts with RewriteRule. - # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html + # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html # The syntax of a RewriteRule is: # RewriteRule pattern target [Flag1,Flag2,Flag3] - # i.e. target is required, so it must exist. + # i.e. target is required, so it must exist. target = line.split()[2].strip() https_prefix = "https://" - if len(target) Date: Mon, 11 Jan 2016 19:48:17 +0000 Subject: [PATCH 11/17] Dequote possible quoted target --- letsencrypt-apache/letsencrypt_apache/configurator.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 13c7c91a..383db3d9 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -716,6 +716,7 @@ def _sift_line(self, line): rewrite_rule = "RewriteRule" if line.lstrip()[:len(rewrite_rule)] != rewrite_rule: return False + # line starts with RewriteRule. # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html @@ -723,6 +724,10 @@ def _sift_line(self, line): # RewriteRule pattern target [Flag1,Flag2,Flag3] # i.e. target is required, so it must exist. target = line.split()[2].strip() + + # target may be surrounded with double quotes + if len(target) > 0 and target[0]==target[-1]=="\"" + target = target[1:-1] https_prefix = "https://" if len(target) < len(https_prefix): From a43e7b11f1d926c4bfe89a402b8597a1fde8fc4c Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:55:15 +0000 Subject: [PATCH 12/17] Add colon --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 383db3d9..af00e752 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -726,7 +726,7 @@ def _sift_line(self, line): target = line.split()[2].strip() # target may be surrounded with double quotes - if len(target) > 0 and target[0]==target[-1]=="\"" + if len(target) > 0 and target[0]==target[-1]=="\"": target = target[1:-1] https_prefix = "https://" From 9c2a0362a763a0aa804748de389650051d351a6c Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:55:55 +0000 Subject: [PATCH 13/17] Add rewrite tests: normal, small, quoted, etc. --- .../letsencrypt_apache/tests/configurator_test.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 599334a2..78714b88 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -922,6 +922,16 @@ def test_create_own_redirect_for_old_apache_version(self): self.config._enable_redirect(self.vh_truth[1], "") # pylint: disable=protected-access self.assertEqual(len(self.config.vhosts), 7) + def test_sift_line(self): + # pylint: disable=protected-access + small_quoted_target = "RewriteRule ^ \"http://\"" + self.assertFalse(self.config._sift_line(small_quoted_target)) + + https_target = "RewriteRule ^ https://satoshi" + self.assertTrue(self.config._sift_line(https_target)) + + normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]" + self.assertFalse(self.config._sift_line(normal_target)) def get_achalls(self): """Return testing achallenges.""" From 4645bf8329f24b59c7be742c5ad4befed5dd3037 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 20:58:52 +0000 Subject: [PATCH 14/17] Make lint happy --- .../letsencrypt_apache/configurator.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index af00e752..16f26474 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -724,9 +724,9 @@ def _sift_line(self, line): # RewriteRule pattern target [Flag1,Flag2,Flag3] # i.e. target is required, so it must exist. target = line.split()[2].strip() - + # target may be surrounded with double quotes - if len(target) > 0 and target[0]==target[-1]=="\"": + if len(target) > 0 and target[0] == target[-1] == "\"": target = target[1:-1] https_prefix = "https://" @@ -760,10 +760,10 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): for line in orig_file: if self._sift_line(line): if not sift: - new_file.write("# The following rewrite rules" - "were *not* enabled on your HTTPS site, " - "because they have the potential to create " - "redirection loops:") + new_file.write("# The following rewrite rules " + "were *not* enabled on your HTTPS site,\n" + "# because they have the potential to create " + "redirection loops:\n") sift = True new_file.write("# " + line) else: From b28b5b08d7f325f7bd089b47250450788240e670 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 20:59:19 +0000 Subject: [PATCH 15/17] More tests; Make Nose happy --- .../tests/configurator_test.py | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 78714b88..954560aa 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -930,9 +930,35 @@ def test_sift_line(self): https_target = "RewriteRule ^ https://satoshi" self.assertTrue(self.config._sift_line(https_target)) - normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]" + normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]" self.assertFalse(self.config._sift_line(normal_target)) + def test_make_vhost_ssl_with_http_vhost_redirect_rewrite_rule(self): + self.config.parser.modules.add("rewrite_module") + + http_vhost = self.vh_truth[0] + + self.config.parser.add_dir( + http_vhost.path, "RewriteEngine", "on") + + self.config.parser.add_dir( + http_vhost.path, "RewriteRule", + ["^", + "https://%{SERVER_NAME}%{REQUEST_URI}", + "[L,QSA,R=permanent]"]) + self.config.save() + + ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) + + #import ipdb; ipdb.set_trace() + self.assertTrue(self.config.parser.find_dir( + "RewriteEngine", "on", ssl_vhost.path, False)) + + conf_text = open(ssl_vhost.filep).read() + commented_rewrite_rule = \ + "# RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,QSA,R=permanent]" + self.assertTrue(commented_rewrite_rule in conf_text) + def get_achalls(self): """Return testing achallenges.""" account_key = self.rsa512jwk From 10df56bab6b5d7a6fb4ee791b1239b149f67ee6a Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 11 Jan 2016 18:21:33 -0800 Subject: [PATCH 16/17] Added revisions --- .../letsencrypt_apache/configurator.py | 63 +++++++++---------- .../tests/configurator_test.py | 6 +- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 16f26474..de179966 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -8,6 +8,7 @@ import socket import time +import zope.component import zope.interface from acme import challenges @@ -698,42 +699,36 @@ def _get_ssl_vhost_path(self, non_ssl_vh_fp): return non_ssl_vh_fp + self.conf("le_vhost_ext") def _sift_line(self, line): - """ Decides whether a line shouldn't be copied from a http vhost to a - SSL vhost. + """Decides whether a line should be copied to a SSL vhost. A canonical example of when sifting a line is required: - When the http vhost contains a RewriteRule that unconditionally redirects - any request to the https version of the same site. - e.g: RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,QSA,R=permanent] - Copying the above line to the ssl vhost would cause a redirection loop. + When the http vhost contains a RewriteRule that unconditionally + redirects any request to the https version of the same site. + e.g: + RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,QSA,R=permanent] + Copying the above line to the ssl vhost would cause a + redirection loop. - :param str line: a line extracted from the http vhost config file. + :param str line: a line extracted from the http vhost. :returns: True - don't copy line from http vhost to SSL vhost. - :rtype: (bool) - """ + :rtype: bool - rewrite_rule = "RewriteRule" - if line.lstrip()[:len(rewrite_rule)] != rewrite_rule: + """ + if not line.lstrip().startswith("RewriteRule"): return False - # line starts with RewriteRule. - # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html # The syntax of a RewriteRule is: # RewriteRule pattern target [Flag1,Flag2,Flag3] # i.e. target is required, so it must exist. target = line.split()[2].strip() - # target may be surrounded with double quotes - if len(target) > 0 and target[0] == target[-1] == "\"": + # target may be surrounded with quotes + if target[0] in ("'", '"') and target[0] == target[-1]: target = target[1:-1] - https_prefix = "https://" - if len(target) < len(https_prefix): - return False - - if target[:len(https_prefix)] == https_prefix: + if target.startswith("https://"): return True return False @@ -750,36 +745,38 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): # First register the creation so that it is properly removed if # configuration is rolled back self.reverter.register_file_creation(False, ssl_fp) + sift = False try: with open(avail_fp, "r") as orig_file: with open(ssl_fp, "w") as new_file: new_file.write("\n") - sift = False - for line in orig_file: if self._sift_line(line): if not sift: - new_file.write("# The following rewrite rules " - "were *not* enabled on your HTTPS site,\n" - "# because they have the potential to create " - "redirection loops:\n") + new_file.write( + "# Some rewrite rules in this file were " + "were disabled on your HTTPS site,\n" + "# because they have the potential to " + "create redirection loops.\n") sift = True new_file.write("# " + line) else: new_file.write(line) - new_file.write("\n") - - if sift: - logger.warn("Some rewrite rules were *not* enabled on " - "your HTTPS site, because they have the " - "potential to create redirection loops.") - except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") raise errors.PluginError("Unable to write/read in make_vhost_ssl") + if sift: + reporter = zope.component.getUtility(interfaces.IReporter) + reporter.add_message( + "Some rewrite rules copied from {0} were disabled in the " + "vhost for your HTTPS site located at {1} because they have " + "the potential to create redirection loops.".format(avail_fp, + ssl_fp), + reporter.MEDIUM_PRIORITY) + def _update_ssl_vhosts_addrs(self, vh_path): ssl_addrs = set() ssl_addr_p = self.aug.match(vh_path + "/arg") diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 954560aa..5905e281 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -933,7 +933,8 @@ def test_sift_line(self): normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]" self.assertFalse(self.config._sift_line(normal_target)) - def test_make_vhost_ssl_with_http_vhost_redirect_rewrite_rule(self): + @mock.patch("letsencrypt_apache.configurator.zope.component.getUtility") + def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility): self.config.parser.modules.add("rewrite_module") http_vhost = self.vh_truth[0] @@ -950,7 +951,6 @@ def test_make_vhost_ssl_with_http_vhost_redirect_rewrite_rule(self): ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) - #import ipdb; ipdb.set_trace() self.assertTrue(self.config.parser.find_dir( "RewriteEngine", "on", ssl_vhost.path, False)) @@ -958,6 +958,8 @@ def test_make_vhost_ssl_with_http_vhost_redirect_rewrite_rule(self): commented_rewrite_rule = \ "# RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,QSA,R=permanent]" self.assertTrue(commented_rewrite_rule in conf_text) + mock_get_utility().add_message.assert_called_once_with(mock.ANY, + mock.ANY) def get_achalls(self): """Return testing achallenges.""" From 4cdf63c55e971929f3c14b2c940c6cfc3c9c19f1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 11 Jan 2016 18:27:01 -0800 Subject: [PATCH 17/17] Fix a couple nits --- letsencrypt-apache/letsencrypt_apache/configurator.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 9b1b3a96..4066d626 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -740,10 +740,8 @@ def _sift_line(self, line): if target[0] in ("'", '"') and target[0] == target[-1]: target = target[1:-1] - if target.startswith("https://"): - return True - - return False + # Sift line if it redirects the request to a HTTPS site + return target.startswith("https://") def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): """Copies over existing Vhost with IfModule mod_ssl.c> skeleton. @@ -771,7 +769,7 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): "were disabled on your HTTPS site,\n" "# because they have the potential to " "create redirection loops.\n") - sift = True + sift = True new_file.write("# " + line) else: new_file.write(line)