From a9abc7b39e89ee26c116244e528d49931f922252 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 1 Jul 2016 15:17:37 +0000 Subject: [PATCH 1/4] typo --- certbot-apache/certbot_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 89d602f5f..fdc0f37d8 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -874,7 +874,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if self._sift_line(line): if not sift: new_file.write( - "# Some rewrite rules in this file were " + "# Some rewrite rules in this file " "were disabled on your HTTPS site,\n" "# because they have the potential to " "create redirection loops.\n") From 15ba12ed4647990d8e72f244a682c00327493443 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 1 Jul 2016 21:06:16 +0000 Subject: [PATCH 2/4] Parsing State Machine + some tests --- certbot-apache/certbot_apache/configurator.py | 64 ++++++++++++++++--- .../certbot_apache/tests/configurator_test.py | 11 ++-- 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index fdc0f37d8..23c7a0c29 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -819,7 +819,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): else: return non_ssl_vh_fp + self.conf("le_vhost_ext") - def _sift_line(self, line): + def _sift_rewrite_rule(self, line): """Decides whether a line should be copied to a SSL vhost. A canonical example of when sifting a line is required: @@ -861,7 +861,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): A new file is created on the filesystem. """ - # First register the creation so that it is properly removed if + # First register the creation so thatu it is properly removed if # configuration is rolled back self.reverter.register_file_creation(False, ssl_fp) sift = False @@ -870,18 +870,62 @@ 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") + + comment = ("# Some rewrite rules in this file were " + "disabled on your HTTPS site,\n" + "# because they have the potential to create " + "redirection loops.\n") + for line in orig_file: - if self._sift_line(line): + A = line.lstrip().startswith("RewriteCond") + B = line.lstrip().startswith("RewriteRule") + + if not (A or B): + new_file.write(line) + continue + + # A RewriteRule that doesn't need filtering + if B and not self._sift_rewrite_rule(line): + new_file.write(line) + continue + + # A RewriteRule that does need filtering + if B and self._sift_rewrite_rule(line): if not sift: - new_file.write( - "# Some rewrite rules in this file " - "were disabled on your HTTPS site,\n" - "# because they have the potential to " - "create redirection loops.\n") + new_file.write(comment) sift = True new_file.write("# " + line) - else: - new_file.write(line) + continue + + # We save RewriteCond(s) and their corresponding + # RewriteRule in 'chunk'. + # We then decide whether we comment out the entire + # chunk based on its RewriteRule. + chunk = [] + if A: + chunk.append(line) + line = next(orig_file) + + # RewriteCond(s) must be followed by one RewriteRule + while not line.lstrip().startswith("RewriteRule"): + chunk.append(line) + line = next(orig_file) + + # Now, current line must start with a RewriteRule + chunk.append(line) + + if self._sift_rewrite_rule(line): + if not sift: + new_file.write(comment) + sift = True + + new_file.write(''.join( + ['# ' + l for l in chunk])) + continue + else: + new_file.write(''.join(chunk)) + continue + new_file.write("\n") except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 9a034c3e0..5a8684c9a 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1110,16 +1110,19 @@ class MultipleVhostsTest(util.ApacheTest): self.config._enable_redirect(self.vh_truth[1], "") self.assertEqual(len(self.config.vhosts), 9) - def test_sift_line(self): + def test_sift_rewrite_rule(self): # pylint: disable=protected-access small_quoted_target = "RewriteRule ^ \"http://\"" - self.assertFalse(self.config._sift_line(small_quoted_target)) + self.assertFalse(self.config._sift_rewrite_rule(small_quoted_target)) https_target = "RewriteRule ^ https://satoshi" - self.assertTrue(self.config._sift_line(https_target)) + self.assertTrue(self.config._sift_rewrite_rule(https_target)) normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]" - self.assertFalse(self.config._sift_line(normal_target)) + self.assertFalse(self.config._sift_rewrite_rule(normal_target)) + + not_rewriterule = "NotRewriteRule ^ ..." + self.assertFalse(self.config._sift_rewrite_rule(not_rewriterule)) @mock.patch("certbot_apache.configurator.zope.component.getUtility") def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility): From 74593607803e67818ab23b0e3f7a772ee99bc417 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 1 Jul 2016 22:08:37 +0000 Subject: [PATCH 3/4] Add more test cases --- .../certbot_apache/tests/configurator_test.py | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 5a8684c9a..57c6a8009 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1151,7 +1151,61 @@ class MultipleVhostsTest(util.ApacheTest): "[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) + @mock.patch("certbot_apache.configurator.zope.component.getUtility") + def test_make_vhost_ssl_with_existing_rewrite_conds(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") + + # Add a chunk that should not be commented out. + self.config.parser.add_dir(http_vhost.path, + "RewriteCond", ["%{DOCUMENT_ROOT}/%{REQUEST_FILENAME}", "!-f"]) + self.config.parser.add_dir( + http_vhost.path, "RewriteRule", + ["^(.*)$", "b://u%{REQUEST_URI}", "[P,QSA,L]"]) + + # Add a chunk that should be commented out. + self.config.parser.add_dir(http_vhost.path, + "RewriteCond", ["%{HTTPS}", "!=on"]) + self.config.parser.add_dir(http_vhost.path, + "RewriteCond", ["%{HTTPS}", "!^$"]) + 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]) + + conf_line_set = set(open(ssl_vhost.filep).read().splitlines()) + + not_commented_cond1 = ("RewriteCond " + "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f") + not_commented_rewrite_rule = ("RewriteRule " + "^(.*)$ b://u%{REQUEST_URI} [P,QSA,L]") + + commented_cond1 = "# RewriteCond %{HTTPS} !=on" + commented_cond2 = "# RewriteCond %{HTTPS} !^$" + commented_rewrite_rule = ("# RewriteRule ^ " + "https://%{SERVER_NAME}%{REQUEST_URI} " + "[L,QSA,R=permanent]") + + self.assertTrue(not_commented_cond1 in conf_line_set) + self.assertTrue(not_commented_rewrite_rule in conf_line_set) + + self.assertTrue(commented_cond1 in conf_line_set) + self.assertTrue(commented_cond2 in conf_line_set) + self.assertTrue(commented_rewrite_rule in conf_line_set) + mock_get_utility().add_message.assert_called_once_with(mock.ANY, + mock.ANY) + def get_achalls(self): """Return testing achallenges.""" From 0e9622322a89f8efbeb149d4ccf8cb33ddc19660 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 1 Jul 2016 22:17:41 +0000 Subject: [PATCH 4/4] typo --- certbot-apache/certbot_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 23c7a0c29..0a24759dc 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -861,7 +861,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): A new file is created on the filesystem. """ - # First register the creation so thatu it is properly removed if + # 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