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

@@ -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",
+            self._set_https_redirection_rewrite_rule(general_vh)
-                                    constants.REWRITE_HTTPS_ARGS_WITH_END)
-            else:
-                self.parser.add_dir(general_vh.path, "RewriteRule",
-                                    constants.REWRITE_HTTPS_ARGS)
 
             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)
+                dir_path = m.group(1)
-                rewrite_args_dict[dir_id].append(match)
+                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():
+            for dir_path, args_paths in rewrite_args_dict.items():
-                if [self.aug.get(x) for x in matches] in redirect_args:
+                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")
 

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"""

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)