diff options
author | Peter Eckersley <pde@users.noreply.github.com> | 2015-12-16 23:43:54 +0300 |
---|---|---|
committer | Peter Eckersley <pde@users.noreply.github.com> | 2015-12-16 23:43:54 +0300 |
commit | 55cc72fab7790fccbc55a7dec99380a66dca3cc8 (patch) | |
tree | 90e31b184449f39dec7e0fee836bf756323b3324 /letsencrypt-apache | |
parent | c806b21e771a7f787f55170eaf894050b71c7000 (diff) | |
parent | 06643b35a0a0c8bc9b405099f1d864928e2c434a (diff) |
Merge pull request #1707 from sagi/bugfix_redirect
Fix Redirect Verification
Diffstat (limited to 'letsencrypt-apache')
3 files changed, 163 insertions, 59 deletions
diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 0b40a7e38..f72492ac2 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1,7 +1,6 @@ """Apache Configuration based off of Augeas Configurator.""" # pylint: disable=too-many-lines import filecmp -import itertools import logging import os import re @@ -26,6 +25,7 @@ from letsencrypt_apache import tls_sni_01 from letsencrypt_apache import obj from letsencrypt_apache import parser +from collections import defaultdict logger = logging.getLogger(__name__) @@ -904,15 +904,33 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "redirection") self._create_redirect_vhost(ssl_vhost) else: - # Check if redirection already exists - self._verify_no_redirects(general_vh) + # 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. Finding + # another RewriteRule is likely to be fine in most or all cases, + # but redirect loops are possible in very obscure cases; see #1620 + # for reasoning. + 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() - self.parser.add_dir(general_vh.path, "RewriteEngine", "on") - self.parser.add_dir(general_vh.path, "RewriteRule", + if not self._is_rewrite_engine_on(general_vh): + self.parser.add_dir(general_vh.path, "RewriteEngine", "on") + + 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.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % (general_vh.filep, ssl_vhost.filep)) self.save() @@ -920,40 +938,67 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.info("Redirecting vhost in %s to ssl vhost in %s", general_vh.filep, ssl_vhost.filep) - def _verify_no_redirects(self, vhost): - """Checks to see if existing redirect is in place. + def _verify_no_letsencrypt_redirect(self, vhost): + """Checks to see if a redirect was already installed by letsencrypt. - Checks to see if virtualhost already contains a rewrite or redirect - returns boolean, integer + Checks to see if virtualhost already contains a rewrite rule that is + identical to Letsencrypt's redirection rewrite rule. :param vhost: vhost to check :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost` :raises errors.PluginEnhancementAlreadyPresent: When the exact letsencrypt redirection WriteRule exists in virtual host. + """ + rewrite_path = self.parser.find_dir( + "RewriteRule", None, start=vhost.path) + + # There can be other RewriteRule directive lines in vhost config. + # 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+\]).*' + for match in rewrite_path: + m = re.match(pat, match) + if m: + dir_id = m.group(1) + rewrite_args_dict[dir_id].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: + raise errors.PluginEnhancementAlreadyPresent( + "Let's Encrypt has already enabled redirection") + + def _is_rewrite_exists(self, vhost): + """Checks if there exists a RewriteRule directive in vhost + + :param vhost: vhost to check + :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost` + + :returns: True if a RewriteRule directive exists. + :rtype: bool - errors.PluginError: When there exists directives that may hint - other redirection. (TODO: We should not throw a PluginError, - but that's for an other PR.) """ rewrite_path = self.parser.find_dir( - "RewriteRule", None, start=vhost.path) - redirect_path = self.parser.find_dir("Redirect", None, start=vhost.path) - - if redirect_path: - # "Existing Redirect directive for virtualhost" - raise errors.PluginError("Existing Redirect present on HTTP vhost.") - if rewrite_path: - # "No existing redirection for virtualhost" - if len(rewrite_path) != len(constants.REWRITE_HTTPS_ARGS): - raise errors.PluginError("Unknown Existing RewriteRule") - for match, arg in itertools.izip( - rewrite_path, constants.REWRITE_HTTPS_ARGS): - if self.aug.get(match) != arg: - raise errors.PluginError("Unknown Existing RewriteRule") - - raise errors.PluginEnhancementAlreadyPresent( - "Let's Encrypt has already enabled redirection") + "RewriteRule", None, start=vhost.path) + return bool(rewrite_path) + + def _is_rewrite_engine_on(self, vhost): + """Checks if a RewriteEngine directive is on + + :param vhost: vhost to check + :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost` + + """ + rewrite_engine_path = self.parser.find_dir("RewriteEngine", "on", + start=vhost.path) + if rewrite_engine_path: + return self.parser.get_arg(rewrite_engine_path[0]) + return False def _create_redirect_vhost(self, ssl_vhost): """Creates an http_vhost specifically to redirect for the ssl_vhost. @@ -990,6 +1035,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if ssl_vhost.aliases: serveralias = "ServerAlias " + " ".join(ssl_vhost.aliases) + rewrite_rule_args = [] + if self.get_version() >= (2, 3, 9): + rewrite_rule_args = constants.REWRITE_HTTPS_ARGS_WITH_END + else: + rewrite_rule_args = constants.REWRITE_HTTPS_ARGS + + return ("<VirtualHost %s>\n" "%s \n" "%s \n" @@ -1003,7 +1055,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "</VirtualHost>\n" % (" ".join(str(addr) for addr in self._get_proposed_addrs(ssl_vhost)), servername, serveralias, - " ".join(constants.REWRITE_HTTPS_ARGS))) + " ".join(rewrite_rule_args))) def _write_out_redirect(self, ssl_vhost, text): # This is the default name diff --git a/letsencrypt-apache/letsencrypt_apache/constants.py b/letsencrypt-apache/letsencrypt_apache/constants.py index 202fc3e21..eb004b975 100644 --- a/letsencrypt-apache/letsencrypt_apache/constants.py +++ b/letsencrypt-apache/letsencrypt_apache/constants.py @@ -25,8 +25,12 @@ AUGEAS_LENS_DIR = pkg_resources.resource_filename( REWRITE_HTTPS_ARGS = [ "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[L,QSA,R=permanent]"] -"""Apache rewrite rule arguments used for redirections to https vhost""" +"""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]"] +"""Apache version >= 2.3.9 rewrite rule arguments used for redirections to + https vhost""" HSTS_ARGS = ["always", "set", "Strict-Transport-Security", "\"max-age=31536000; includeSubDomains\""] diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 991704144..f2bf89d2c 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -653,6 +653,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, @@ -741,6 +754,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, 2)) + # This will create an ssl vhost for letsencrypt.demo self.config.enhance("letsencrypt.demo", "redirect") @@ -760,6 +775,55 @@ class TwoVhost80Test(util.ApacheTest): self.assertTrue("rewrite_module" in self.config.parser.modules) + def test_rewrite_rule_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.assertTrue(self.config._is_rewrite_exists(self.vh_truth[3])) # pylint: disable=protected-access + + def test_rewrite_engine_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, "RewriteEngine", "on") + self.assertTrue(self.config._is_rewrite_engine_on(self.vh_truth[3])) # pylint: disable=protected-access + + + @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( @@ -774,43 +838,26 @@ class TwoVhost80Test(util.ApacheTest): def test_redirect_twice(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.enhance("encryption-example.demo", "redirect") self.assertRaises( errors.PluginEnhancementAlreadyPresent, self.config.enhance, "encryption-example.demo", "redirect") - def test_unknown_rewrite(self): - # Skip the enable mod - self.config.parser.modules.add("rewrite_module") - self.config.parser.add_dir( - self.vh_truth[3].path, "RewriteRule", ["Unknown"]) - self.config.save() - self.assertRaises( - errors.PluginError, - self.config.enhance, "letsencrypt.demo", "redirect") - - def test_unknown_rewrite2(self): - # Skip the enable mod + def test_create_own_redirect(self): self.config.parser.modules.add("rewrite_module") - self.config.parser.add_dir( - self.vh_truth[3].path, "RewriteRule", ["Unknown", "2", "3"]) - self.config.save() - self.assertRaises( - errors.PluginError, - self.config.enhance, "letsencrypt.demo", "redirect") + self.config.get_version = mock.Mock(return_value=(2, 3, 9)) + # For full testing... give names... + self.vh_truth[1].name = "default.com" + self.vh_truth[1].aliases = set(["yes.default.com"]) - def test_unknown_redirect(self): - # Skip the enable mod - self.config.parser.modules.add("rewrite_module") - self.config.parser.add_dir( - self.vh_truth[3].path, "Redirect", ["Unknown"]) - self.config.save() - self.assertRaises( - errors.PluginError, - self.config.enhance, "letsencrypt.demo", "redirect") + self.config._enable_redirect(self.vh_truth[1], "") # pylint: disable=protected-access + self.assertEqual(len(self.config.vhosts), 7) - def test_create_own_redirect(self): + 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"]) @@ -818,6 +865,7 @@ class TwoVhost80Test(util.ApacheTest): 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 |