From 26a7023b8dd985674dbf9ff2d35d4065ca7cae9d Mon Sep 17 00:00:00 2001 From: Sagi Kedmi Date: Fri, 3 Mar 2017 02:49:34 +0200 Subject: [PATCH] Change QSA to NE in HTTPS redirection (#4204) * Change QSA to NE in HTTPS redirection * Seamless transition to new HTTPS redirection RewriteRule --- certbot-apache/certbot_apache/configurator.py | 41 +++++++++++++----- certbot-apache/certbot_apache/constants.py | 8 +++- .../certbot_apache/tests/configurator_test.py | 43 ++++++++++++++++--- 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 5639dae83..cdfc01626 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -1315,18 +1315,15 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # even with save() and load() if not self._is_rewrite_engine_on(general_vh): self.parser.add_dir(general_vh.path, "RewriteEngine", "on") + names = ssl_vhost.get_names() for idx, name in enumerate(names): args = ["%{SERVER_NAME}", "={0}".format(name), "[OR]"] if idx == len(names) - 1: args.pop() self.parser.add_dir(general_vh.path, "RewriteCond", args) - if self.get_version() >= (2, 3, 9): - self.parser.add_dir(general_vh.path, "RewriteRule", - constants.REWRITE_HTTPS_ARGS_WITH_END) - else: - self.parser.add_dir(general_vh.path, "RewriteRule", - constants.REWRITE_HTTPS_ARGS) + + self._set_https_redirection_rewrite_rule(general_vh) self.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % (general_vh.filep, ssl_vhost.filep)) @@ -1336,12 +1333,24 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.info("Redirecting vhost in %s to ssl vhost in %s", general_vh.filep, ssl_vhost.filep) + def _set_https_redirection_rewrite_rule(self, vhost): + if self.get_version() >= (2, 3, 9): + self.parser.add_dir(vhost.path, "RewriteRule", + constants.REWRITE_HTTPS_ARGS_WITH_END) + else: + self.parser.add_dir(vhost.path, "RewriteRule", + constants.REWRITE_HTTPS_ARGS) + + def _verify_no_certbot_redirect(self, vhost): """Checks to see if a redirect was already installed by certbot. Checks to see if virtualhost already contains a rewrite rule that is identical to Certbot's redirection rewrite rule. + For graceful transition to new rewrite rules for HTTPS redireciton we + delete certbot's old rewrite rules and set the new one instead. + :param vhost: vhost to check :type vhost: :class:`~certbot_apache.obj.VirtualHost` @@ -1355,19 +1364,29 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # rewrite_args_dict keys are directive ids and the corresponding value # for each is a list of arguments to that directive. rewrite_args_dict = defaultdict(list) - pat = r'.*(directive\[\d+\]).*' + pat = r'(.*directive\[\d+\]).*' for match in rewrite_path: m = re.match(pat, match) if m: - dir_id = m.group(1) - rewrite_args_dict[dir_id].append(match) + dir_path = m.group(1) + rewrite_args_dict[dir_path].append(match) if rewrite_args_dict: redirect_args = [constants.REWRITE_HTTPS_ARGS, constants.REWRITE_HTTPS_ARGS_WITH_END] - for matches in rewrite_args_dict.values(): - if [self.aug.get(x) for x in matches] in redirect_args: + for dir_path, args_paths in rewrite_args_dict.items(): + arg_vals = [self.aug.get(x) for x in args_paths] + + # Search for past redirection rule, delete it, set the new one + if arg_vals in constants.OLD_REWRITE_HTTPS_ARGS: + self.aug.remove(dir_path) + self._set_https_redirection_rewrite_rule(vhost) + self.save() + raise errors.PluginEnhancementAlreadyPresent( + "Certbot has already enabled redirection") + + if arg_vals in redirect_args: raise errors.PluginEnhancementAlreadyPresent( "Certbot has already enabled redirection") diff --git a/certbot-apache/certbot_apache/constants.py b/certbot-apache/certbot_apache/constants.py index dcc635c4b..3cfeb4dd6 100644 --- a/certbot-apache/certbot_apache/constants.py +++ b/certbot-apache/certbot_apache/constants.py @@ -136,15 +136,19 @@ AUGEAS_LENS_DIR = pkg_resources.resource_filename( """Path to the Augeas lens directory""" REWRITE_HTTPS_ARGS = [ - "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[L,QSA,R=permanent]"] + "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[L,NE,R=permanent]"] """Apache version<2.3.9 rewrite rule arguments used for redirections to https vhost""" REWRITE_HTTPS_ARGS_WITH_END = [ - "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[END,QSA,R=permanent]"] + "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[END,NE,R=permanent]"] """Apache version >= 2.3.9 rewrite rule arguments used for redirections to https vhost""" +OLD_REWRITE_HTTPS_ARGS = [ + ["^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[L,QSA,R=permanent]"], + ["^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[END,QSA,R=permanent]"]] + HSTS_ARGS = ["always", "set", "Strict-Transport-Security", "\"max-age=31536000\""] """Apache header arguments for HSTS""" diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 937694267..45e701bd5 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -18,6 +18,7 @@ from certbot.tests import acme_util from certbot.tests import util as certbot_util from certbot_apache import configurator +from certbot_apache import constants from certbot_apache import parser from certbot_apache import obj @@ -1047,6 +1048,36 @@ class MultipleVhostsTest(util.ApacheTest): self.assertTrue("rewrite_module" in self.config.parser.modules) + @mock.patch("certbot.util.run_script") + @mock.patch("certbot.util.exe_exists") + def test_redirect_with_old_https_redirection(self, mock_exe, _): + self.config.parser.update_runtime_variables = mock.Mock() + mock_exe.return_value = True + self.config.get_version = mock.Mock(return_value=(2, 2, 0)) + + ssl_vhost = self.config.choose_vhost("certbot.demo") + + # pylint: disable=protected-access + http_vhost = self.config._get_http_vhost(ssl_vhost) + + # Create an old (previously suppoorted) https redirectoin rewrite rule + self.config.parser.add_dir( + http_vhost.path, "RewriteRule", + ["^", + "https://%{SERVER_NAME}%{REQUEST_URI}", + "[L,QSA,R=permanent]"]) + + self.config.save() + + try: + self.config.enhance("certbot.demo", "redirect") + except errors.PluginEnhancementAlreadyPresent: + args_paths = self.config.parser.find_dir( + "RewriteRule", None, http_vhost.path, False) + arg_vals = [self.config.aug.get(x) for x in args_paths] + self.assertEqual(arg_vals, constants.REWRITE_HTTPS_ARGS) + + def test_redirect_with_conflict(self): self.config.parser.modules.add("rewrite_module") ssl_vh = obj.VirtualHost( @@ -1134,7 +1165,7 @@ class MultipleVhostsTest(util.ApacheTest): http_vhost.path, "RewriteRule", ["^", "https://%{SERVER_NAME}%{REQUEST_URI}", - "[L,QSA,R=permanent]"]) + "[L,NE,R=permanent]"]) self.config.save() ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) @@ -1145,7 +1176,7 @@ class MultipleVhostsTest(util.ApacheTest): conf_text = open(ssl_vhost.filep).read() commented_rewrite_rule = ("# RewriteRule ^ " "https://%{SERVER_NAME}%{REQUEST_URI} " - "[L,QSA,R=permanent]") + "[L,NE,R=permanent]") self.assertTrue(commented_rewrite_rule in conf_text) mock_get_utility().add_message.assert_called_once_with(mock.ANY, @@ -1164,7 +1195,7 @@ class MultipleVhostsTest(util.ApacheTest): "RewriteCond", ["%{DOCUMENT_ROOT}/%{REQUEST_FILENAME}", "!-f"]) self.config.parser.add_dir( http_vhost.path, "RewriteRule", - ["^(.*)$", "b://u%{REQUEST_URI}", "[P,QSA,L]"]) + ["^(.*)$", "b://u%{REQUEST_URI}", "[P,NE,L]"]) # Add a chunk that should be commented out. self.config.parser.add_dir(http_vhost.path, @@ -1175,7 +1206,7 @@ class MultipleVhostsTest(util.ApacheTest): http_vhost.path, "RewriteRule", ["^", "https://%{SERVER_NAME}%{REQUEST_URI}", - "[L,QSA,R=permanent]"]) + "[L,NE,R=permanent]"]) self.config.save() @@ -1186,13 +1217,13 @@ class MultipleVhostsTest(util.ApacheTest): not_commented_cond1 = ("RewriteCond " "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f") not_commented_rewrite_rule = ("RewriteRule " - "^(.*)$ b://u%{REQUEST_URI} [P,QSA,L]") + "^(.*)$ b://u%{REQUEST_URI} [P,NE,L]") commented_cond1 = "# RewriteCond %{HTTPS} !=on" commented_cond2 = "# RewriteCond %{HTTPS} !^$" commented_rewrite_rule = ("# RewriteRule ^ " "https://%{SERVER_NAME}%{REQUEST_URI} " - "[L,QSA,R=permanent]") + "[L,NE,R=permanent]") self.assertTrue(not_commented_cond1 in conf_line_set) self.assertTrue(not_commented_rewrite_rule in conf_line_set)