diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index bf98ddcee..712cbc0d0 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -880,6 +880,14 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Check if LetsEncrypt redirection already exists self._verify_no_letsencrypt_redirect(general_vh) + # Note: if code flow gets here it means we didn't find the exact + # letsencrypt RewriteRule config for redirection. So if we find + # an other RewriteRule it may induce a loop / config mismatch. + if self.is_rewrite_exists(general_vh): + logger.warn("Added an HTTP->HTTPS rewrite in addition to " + "other RewriteRules; you may wish to check for " + "overall consistency.") + # Add directives to server # Note: These are not immediately searchable in sites-enabled # even with save() and load() @@ -892,14 +900,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.parser.add_dir(general_vh.path, "RewriteRule", constants.REWRITE_HTTPS_ARGS) - # Note: if code flow gets here it means we didn't find the exact - # letsencrypt RewriteRule config for redirection. So if we find - # an other RewriteRule it may induce a loop / config mismatch. - if self._is_rewrite_exists(ssl_vhost): - logger.warn("Added an HTTP->HTTPS rewrite in addition to " - "other RewriteRules; you may wish to check for " - "overall consistency.") - self.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % (general_vh.filep, ssl_vhost.filep)) self.save() @@ -929,7 +929,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): raise errors.PluginEnhancementAlreadyPresent( "Let's Encrypt has already enabled redirection") - def _is_rewrite_exists(self, vhost): + def is_rewrite_exists(self, vhost): """Checks if there exists a rewriterule directive in vhost :param vhost: vhost to check diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 7c47a71e6..d291dc539 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -625,6 +625,19 @@ class TwoVhost80Test(util.ApacheTest): def test_supported_enhancements(self): self.assertTrue(isinstance(self.config.supported_enhancements(), list)) + + @mock.patch("letsencrypt.le_util.exe_exists") + def test_enhance_unknown_vhost(self, mock_exe): + self.config.parser.modules.add("rewrite_module") + mock_exe.return_value = True + ssl_vh = obj.VirtualHost( + "fp", "ap", set([obj.Addr(("*", "443")), obj.Addr(("satoshi.com",))]), + True, False) + self.config.vhosts.append(ssl_vh) + self.assertRaises( + errors.PluginError, + self.config.enhance, "satoshi.com", "redirect") + def test_enhance_unknown_enhancement(self): self.assertRaises( errors.PluginError, @@ -713,7 +726,8 @@ class TwoVhost80Test(util.ApacheTest): def test_redirect_well_formed_http(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, 3, 9)) + self.config.get_version = mock.Mock(return_value=(2, 2)) + # This will create an ssl vhost for letsencrypt.demo self.config.enhance("letsencrypt.demo", "redirect") @@ -733,6 +747,48 @@ class TwoVhost80Test(util.ApacheTest): self.assertTrue("rewrite_module" in self.config.parser.modules) + def test_rewrite_exists(self): + # Skip the enable mod + self.config.parser.modules.add("rewrite_module") + self.config.get_version = mock.Mock(return_value=(2, 3, 9)) + self.config.parser.add_dir( + self.vh_truth[3].path, "RewriteRule", ["Unknown"]) + self.config.save() + self.assertTrue(self.config.is_rewrite_exists(self.vh_truth[3])) + + + @mock.patch("letsencrypt.le_util.run_script") + @mock.patch("letsencrypt.le_util.exe_exists") + def test_redirect_with_existing_rewrite(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)) + + # Create a preexisting rewrite rule + self.config.parser.add_dir( + self.vh_truth[3].path, "RewriteRule", ["Unknown"]) + self.config.save() + + # This will create an ssl vhost for letsencrypt.demo + self.config.enhance("letsencrypt.demo", "redirect") + + # These are not immediately available in find_dir even with save() and + # load(). They must be found in sites-available + rw_engine = self.config.parser.find_dir( + "RewriteEngine", "on", self.vh_truth[3].path) + rw_rule = self.config.parser.find_dir( + "RewriteRule", None, self.vh_truth[3].path) + + 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.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path)) + self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path)) + + self.assertTrue("rewrite_module" in self.config.parser.modules) + + def test_redirect_with_conflict(self): self.config.parser.modules.add("rewrite_module") ssl_vh = obj.VirtualHost( @@ -764,6 +820,17 @@ 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_create_own_redirect_for_old_apache_version(self): + self.config.parser.modules.add("rewrite_module") + self.config.get_version = mock.Mock(return_value=(2, 2)) + # For full testing... give names... + self.vh_truth[1].name = "default.com" + self.vh_truth[1].aliases = set(["yes.default.com"]) + + self.config._enable_redirect(self.vh_truth[1], "") # pylint: disable=protected-access + self.assertEqual(len(self.config.vhosts), 7) + + def get_achalls(self): """Return testing achallenges.""" account_key = self.rsa512jwk