diff --git a/acme/.pylintrc b/acme/.pylintrc index 33650310d..d0d150631 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.py b/acme/acme/challenges.py index 68bf3fce4..13d19d3c4 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -236,10 +236,8 @@ class HTTP01Response(KeyAuthorizationChallengeResponse): :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`` diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index 4f2d06167..ef78e1eba 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -73,7 +73,8 @@ class KeyAuthorizationChallengeResponseTest(unittest.TestCase): 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 @@ class TLSSNI01ResponseTest(unittest.TestCase): @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 @@ class DNSTest(unittest.TestCase): 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.py b/acme/acme/client.py index c3e28ef47..49c6bcb21 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -66,15 +66,13 @@ class Client(object): # pylint: disable=too-many-instance-attributes @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('"next" link missing') return messages.RegistrationResource( body=messages.Registration.from_json(response.json()), @@ -483,7 +481,7 @@ class Client(object): # pylint: disable=too-many-instance-attributes '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' @@ -539,7 +537,7 @@ class ClientNetwork(object): # 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: diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 58f55b293..449bd695e 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -34,8 +34,10 @@ class ClientTest(unittest.TestCase): 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 @@ -127,6 +129,13 @@ class ClientTest(unittest.TestCase): 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) @@ -331,7 +340,8 @@ class ClientTest(unittest.TestCase): 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 9c6a5f7b9..06b4492d6 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -130,8 +130,9 @@ class Directory(jose.JSONDeSerializable): @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 2778635f5..85cd9d11d 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 @@ class TLSSNI01ServerTest(unittest.TestCase): 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 @@ class TestSimpleTLSSNI01Server(unittest.TestCase): 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 diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 1d4618825..4066d6264 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -8,6 +8,7 @@ import shutil import socket import time +import zope.component import zope.interface from acme import challenges @@ -709,6 +710,39 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): else: return non_ssl_vh_fp + self.conf("le_vhost_ext") + def _sift_line(self, line): + """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. + + :param str line: a line extracted from the http vhost. + + :returns: True - don't copy line from http vhost to SSL vhost. + :rtype: bool + + """ + if not line.lstrip().startswith("RewriteRule"): + return False + + # 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 quotes + if target[0] in ("'", '"') and target[0] == target[-1]: + target = target[1:-1] + + # 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. @@ -721,18 +755,38 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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") for line in orig_file: - new_file.write(line) + if self._sift_line(line): + if not sift: + 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") 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 212f128f2..113b3b2b2 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -847,7 +847,8 @@ class TwoVhost80Test(util.ApacheTest): # 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 @@ -862,7 +863,7 @@ class TwoVhost80Test(util.ApacheTest): 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)) @@ -911,6 +912,44 @@ class TwoVhost80Test(util.ApacheTest): 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)) + + @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] + + 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]) + + 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) + mock_get_utility().add_message.assert_called_once_with(mock.ANY, + mock.ANY) def get_achalls(self): """Return testing achallenges."""