From a718cfede0f44cbc0c40ae396911fa3f510c3c7e Mon Sep 17 00:00:00 2001 From: sagi Date: Sun, 3 Jan 2016 22:03:47 +0000 Subject: [PATCH 01/11] 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 836d77135..0d6749638 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -712,8 +712,19 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 bf74b2cc644c0ffa1e29f0694b25af214b09bf18 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:12:30 +0000 Subject: [PATCH 02/11] 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 9838b4f52..599334a29 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -857,7 +857,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 @@ -872,7 +873,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)) From 6c18a7d318c477ed0ffae1aa3e57f5bd197fa1aa Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 11 Jan 2016 19:15:23 +0000 Subject: [PATCH 03/11] 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 948514f9b..288ea9dc8 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -696,6 +696,42 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 04/11] 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 288ea9dc8..13c7c91a8 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -696,11 +696,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): :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 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 05/11] 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 13c7c91a8..383db3d98 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -716,6 +716,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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 06/11] 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 383db3d98..af00e7527 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -726,7 +726,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 07/11] 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 599334a29..78714b881 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -922,6 +922,16 @@ 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)) 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 08/11] 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 af00e7527..16f264747 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -724,9 +724,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 09/11] 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 78714b881..954560aa6 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -930,9 +930,35 @@ class TwoVhost80Test(util.ApacheTest): 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 10/11] 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 16f264747..de179966a 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 @@ -698,42 +699,36 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 @@ 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") - 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 954560aa6..5905e281b 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -933,7 +933,8 @@ class TwoVhost80Test(util.ApacheTest): 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 @@ class TwoVhost80Test(util.ApacheTest): 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 @@ class TwoVhost80Test(util.ApacheTest): 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 11/11] 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 9b1b3a961..4066d6264 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -740,10 +740,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "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)