From 917f7aa33e28f429b3798fb386b5b7a35093ca2f Mon Sep 17 00:00:00 2001 From: sagi Date: Tue, 1 Dec 2015 23:38:53 +0000 Subject: [PATCH 01/23] remove check for Redirect header; the existence of a Redirect header does not imply a HTTP->HTTPS redirection --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 ---- .../letsencrypt_apache/tests/configurator_test.py | 10 ---------- 2 files changed, 14 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 5777d204d..0f568db28 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -912,11 +912,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ 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): diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 0b6170e1d..05d97054d 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -771,16 +771,6 @@ class TwoVhost80Test(util.ApacheTest): errors.PluginError, self.config.enhance, "letsencrypt.demo", "redirect") - 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") - def test_create_own_redirect(self): self.config.parser.modules.add("rewrite_module") # For full testing... give names... From bd9ac51fa6b6de29f11389dd632c14aaafaf9d34 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 00:05:15 +0000 Subject: [PATCH 02/23] alter redirect_verification to raise only when an exact Letsencrypt redirction rewrite rule is encountered --- .../letsencrypt_apache/configurator.py | 19 +++++++------------ .../letsencrypt_apache/constants.py | 6 +++++- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 0f568db28..6f3bd7a30 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -878,7 +878,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "redirection") self._create_redirect_vhost(ssl_vhost) else: - # Check if redirection already exists + # Check if LetsEncrypt redirection already exists self._verify_no_redirects(general_vh) # Add directives to server @@ -911,19 +911,14 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): but that's for an other PR.) """ rewrite_path = self.parser.find_dir( - "RewriteRule", None, start=vhost.path) + "RewriteRule", None, start=vhost.path) 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") + if map(self.aug.get, rewrite_path) in [ + constants.REWRITE_HTTPS_ARGS, + constants.REWRITE_HTTPS_ARGS_WITH_END]: + raise errors.PluginEnhancementAlreadyPresent( + "Let's Encrypt has already enabled redirection") def _create_redirect_vhost(self, ssl_vhost): """Creates an http_vhost specifically to redirect for the ssl_vhost. diff --git a/letsencrypt-apache/letsencrypt_apache/constants.py b/letsencrypt-apache/letsencrypt_apache/constants.py index 813eae582..1099262de 100644 --- a/letsencrypt-apache/letsencrypt_apache/constants.py +++ b/letsencrypt-apache/letsencrypt_apache/constants.py @@ -26,8 +26,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}", "[L,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\""] From 005be60d91f2a2b3bb22d5076c831a0cf844877d Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 00:16:13 +0000 Subject: [PATCH 03/23] delete unneeded tests --- .../letsencrypt_apache/configurator.py | 1 - .../letsencrypt_apache/constants.py | 2 +- .../tests/configurator_test.py | 20 ------------------- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 6f3bd7a30..dd99df666 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 diff --git a/letsencrypt-apache/letsencrypt_apache/constants.py b/letsencrypt-apache/letsencrypt_apache/constants.py index 1099262de..448eb6f66 100644 --- a/letsencrypt-apache/letsencrypt_apache/constants.py +++ b/letsencrypt-apache/letsencrypt_apache/constants.py @@ -30,7 +30,7 @@ REWRITE_HTTPS_ARGS = [ REWRITE_HTTPS_ARGS_WITH_END = [ "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[L,QSA,R=permanent]"] -"""Apache version >= 2.3.9 rewrite rule arguments used for redirections to +"""Apache version >= 2.3.9 rewrite rule arguments used for redirections to https vhost""" HSTS_ARGS = ["always", "set", "Strict-Transport-Security", diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 05d97054d..ea282d24e 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -751,26 +751,6 @@ class TwoVhost80Test(util.ApacheTest): 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 - 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") - def test_create_own_redirect(self): self.config.parser.modules.add("rewrite_module") # For full testing... give names... From fdd9cf76101ae0a98526a52d16009877988e88ee Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 00:28:18 +0000 Subject: [PATCH 04/23] change map() to a list comprehension. Long live GvR. --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index dd99df666..1dd9dc84e 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -913,7 +913,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "RewriteRule", None, start=vhost.path) if rewrite_path: - if map(self.aug.get, rewrite_path) in [ + if [self.aug.get(x) for x in rewrite_path] in [ constants.REWRITE_HTTPS_ARGS, constants.REWRITE_HTTPS_ARGS_WITH_END]: raise errors.PluginEnhancementAlreadyPresent( From 5d0337bdf20b1ea675bb571b3a29e254e22c9103 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 00:34:15 +0000 Subject: [PATCH 05/23] change verify_no_redirects to verify_no_letsencrypt_redirect --- .../letsencrypt_apache/configurator.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 1dd9dc84e..16e7dc181 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -878,7 +878,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self._create_redirect_vhost(ssl_vhost) else: # Check if LetsEncrypt redirection already exists - self._verify_no_redirects(general_vh) + self._verify_no_letsencrypt_redirect(general_vh) # Add directives to server # Note: These are not immediately searchable in sites-enabled @@ -893,21 +893,17 @@ 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. - - 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) From 1a9e6b1a8a8ce6fc53d4118c3a85a44e90e5bb9c Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 01:06:48 +0000 Subject: [PATCH 06/23] add _is_rewrite_exists() --- .../letsencrypt_apache/configurator.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 16e7dc181..e4ba19e3d 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -915,6 +915,18 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): raise errors.PluginEnhancementAlreadyPresent( "Let's Encrypt has already enabled redirection") + + def _is_rewrite_exists(self, host): + """Checks if there exists a rewriterule directive + + :param vhost: vhost to check + :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost` + + """ + rewrite_path = self.parser.find_dir( + "RewriteRule", None, start=vhost.path) + return bool(rewrite_path) + def _create_redirect_vhost(self, ssl_vhost): """Creates an http_vhost specifically to redirect for the ssl_vhost. From a7ebeddb7803be0ebaf39721a2de402463ddfded Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 01:37:07 +0000 Subject: [PATCH 07/23] add check for apache 2.3.9, warn of possible conflicting rewrite rules --- .../letsencrypt_apache/configurator.py | 18 ++++++++++++++---- .../letsencrypt_apache/constants.py | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index e4ba19e3d..90f1ed850 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -884,8 +884,19 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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 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) + + if _is_rewrite_exists(vhost): + logger.warn("Preexisting rewrite rules were detected. " + "Please verify that the newly installed " + "redirection rewrite rule doesn't break anything.") + self.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % (general_vh.filep, ssl_vhost.filep)) self.save() @@ -915,9 +926,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): raise errors.PluginEnhancementAlreadyPresent( "Let's Encrypt has already enabled redirection") - - def _is_rewrite_exists(self, host): - """Checks if there exists a rewriterule directive + 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` diff --git a/letsencrypt-apache/letsencrypt_apache/constants.py b/letsencrypt-apache/letsencrypt_apache/constants.py index 448eb6f66..72b4dab24 100644 --- a/letsencrypt-apache/letsencrypt_apache/constants.py +++ b/letsencrypt-apache/letsencrypt_apache/constants.py @@ -29,7 +29,7 @@ REWRITE_HTTPS_ARGS = [ """Apache version<2.3.9 rewrite rule arguments used for redirections to https vhost""" REWRITE_HTTPS_ARGS_WITH_END = [ - "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[L,QSA,R=permanent]"] + "^", "https://%{SERVER_NAME}%{REQUEST_URI}", "[END,QSA,R=permanent]"] """Apache version >= 2.3.9 rewrite rule arguments used for redirections to https vhost""" From f285f3947db54ecb473e15cb152fe402d9b146c6 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 22:00:07 +0000 Subject: [PATCH 08/23] mock get_version in configurator_test --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 +++--- .../letsencrypt_apache/tests/configurator_test.py | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 90f1ed850..8864a5c65 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -885,14 +885,14 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # even with save() and load() self.parser.add_dir(general_vh.path, "RewriteEngine", "on") - if self.get_version >= (2.3.9): + if self.get_version() >= (2, 3, 9): self.parser.add_dir(general_vh.path, "RewriteRule", constants.REWRITE_HTTPS_ARGS_WITH_END) - else: + else: self.parser.add_dir(general_vh.path, "RewriteRule", constants.REWRITE_HTTPS_ARGS) - if _is_rewrite_exists(vhost): + if self._is_rewrite_exists(ssl_vhost): logger.warn("Preexisting rewrite rules were detected. " "Please verify that the newly installed " "redirection rewrite rule doesn't break anything.") diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index ea282d24e..4cce205dc 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -713,6 +713,7 @@ 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)) # This will create an ssl vhost for letsencrypt.demo self.config.enhance("letsencrypt.demo", "redirect") @@ -746,6 +747,8 @@ 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, From 19e191194549089973a1006c96172b127a594981 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 22:48:14 +0000 Subject: [PATCH 09/23] make lint happy; delete trailing whitespaces --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- .../letsencrypt_apache/tests/configurator_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 8864a5c65..5a8dd5ec2 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -896,7 +896,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.warn("Preexisting rewrite rules were detected. " "Please verify that the newly installed " "redirection rewrite rule doesn't break anything.") - + self.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % (general_vh.filep, ssl_vhost.filep)) self.save() diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 4cce205dc..900fd76df 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -713,7 +713,7 @@ 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, 3, 9)) # This will create an ssl vhost for letsencrypt.demo self.config.enhance("letsencrypt.demo", "redirect") From b97fc124e0f40a1ec983907c31e1158791e03009 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 23:05:49 +0000 Subject: [PATCH 10/23] add ver>=2.3.9 check to the case where there is no vhost config --- .../letsencrypt_apache/configurator.py | 18 ++++++++++++++---- .../tests/configurator_test.py | 1 + 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 5a8dd5ec2..f12bbde40 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -892,10 +892,13 @@ 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("Preexisting rewrite rules were detected. " - "Please verify that the newly installed " - "redirection rewrite rule doesn't break anything.") + 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)) @@ -972,6 +975,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 ("\n" "%s \n" "%s \n" @@ -985,7 +995,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "\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/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 900fd76df..7c47a71e6 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -756,6 +756,7 @@ class TwoVhost80Test(util.ApacheTest): def test_create_own_redirect(self): self.config.parser.modules.add("rewrite_module") + 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"]) From 253f2f3768443438de7b021b1819e28dc5563a47 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 2 Dec 2015 23:07:54 +0000 Subject: [PATCH 11/23] make lint happy; delete trailing whitespaces --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index f12bbde40..bf98ddcee 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -897,7 +897,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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 " + "other RewriteRules; you may wish to check for " "overall consistency.") self.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % From 379506739daeec90c35cac5ee8c6b1cc5ff332e0 Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 3 Dec 2015 01:40:12 +0000 Subject: [PATCH 12/23] add tests --- .../letsencrypt_apache/configurator.py | 18 ++--- .../tests/configurator_test.py | 69 ++++++++++++++++++- 2 files changed, 77 insertions(+), 10 deletions(-) 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 From 7c00dba79bc77a2342952666c8d6d66c85cd3c7f Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 3 Dec 2015 22:11:34 +0000 Subject: [PATCH 13/23] fix verification of letsencrypt redirect --- .../letsencrypt_apache/configurator.py | 45 +++++++++++++++---- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 712cbc0d0..855854b6b 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -891,7 +891,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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") + 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", @@ -921,25 +922,53 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ rewrite_path = self.parser.find_dir( "RewriteRule", None, start=vhost.path) - - if rewrite_path: - if [self.aug.get(x) for x in rewrite_path] in [ - constants.REWRITE_HTTPS_ARGS, - constants.REWRITE_HTTPS_ARGS_WITH_END]: - raise errors.PluginEnhancementAlreadyPresent( + + dir_dict = {} + pat = '.*(directive\[\d\]).*' + for match in rewrite_path: + m = re.match(pat, match) + if m: + dir_id = m.group(1) + if dir_id in dir_dict: + dir_dict[dir_id].append(match) + else: + dir_dict[dir_id] = [match] + + if dir_dict: + for dir_id in dir_dict: + if [self.aug.get(x) for x in dir_dict[dir_id]]in [ + constants.REWRITE_HTTPS_ARGS, + constants.REWRITE_HTTPS_ARGS_WITH_END]: + 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 + """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 """ rewrite_path = self.parser.find_dir( "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", None, + start=vhost.path) + if rewrite_engine_path: + return self.aug.get(rewrite_engine_path[0]).lower() == "on" + return False + def _create_redirect_vhost(self, ssl_vhost): """Creates an http_vhost specifically to redirect for the ssl_vhost. From 0348f62ffa410e9a7d43a1a461a95f3bf9a602b1 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 4 Dec 2015 02:00:24 +0000 Subject: [PATCH 14/23] add more tests --- .../letsencrypt_apache/configurator.py | 33 ++++++++++--------- .../tests/configurator_test.py | 13 ++++++-- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index c3d93a057..446bfe9e5 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -884,10 +884,11 @@ 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): + 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.") @@ -895,7 +896,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Add directives to server # Note: These are not immediately searchable in sites-enabled # even with save() and load() - if not self.is_rewrite_engine_on(general_vh): + 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): @@ -926,32 +927,32 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ rewrite_path = self.parser.find_dir( "RewriteRule", None, start=vhost.path) - + dir_dict = {} - pat = '.*(directive\[\d\]).*' + pat = r'.*(directive\[\d+\]).*' for match in rewrite_path: - m = re.match(pat, match) - if m: - dir_id = m.group(1) - if dir_id in dir_dict: - dir_dict[dir_id].append(match) - else: - dir_dict[dir_id] = [match] - + m = re.match(pat, match) + if m: + dir_id = m.group(1) + if dir_id in dir_dict: + dir_dict[dir_id].append(match) + else: + dir_dict[dir_id] = [match] + if dir_dict: for dir_id in dir_dict: - if [self.aug.get(x) for x in dir_dict[dir_id]]in [ + if [self.aug.get(x) for x in dir_dict[dir_id]] in [ constants.REWRITE_HTTPS_ARGS, constants.REWRITE_HTTPS_ARGS_WITH_END]: 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 :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost` - + :returns: True if a RewriteRule directive exists. :rtype: bool @@ -960,7 +961,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "RewriteRule", None, start=vhost.path) return bool(rewrite_path) - def is_rewrite_engine_on(self, vhost): + def _is_rewrite_engine_on(self, vhost): """Checks if a RewriteEngine directive is on :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 2ab582e66..e05d9893f 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -742,14 +742,21 @@ class TwoVhost80Test(util.ApacheTest): self.assertTrue("rewrite_module" in self.config.parser.modules) - def test_rewrite_exists(self): + 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.config.save() - self.assertTrue(self.config.is_rewrite_exists(self.vh_truth[3])) + 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") From b4746e555a0edc8c5450c5e833b09537620a38b0 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 09:18:36 +0000 Subject: [PATCH 15/23] typo: an other -> another --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 446bfe9e5..67b089909 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -887,7 +887,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # 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. + # another 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 " From c594a258feb1333300607308bc898019d4950608 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 09:26:55 +0000 Subject: [PATCH 16/23] Change comment on possibility of redirection loops --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 67b089909..bc12a75fe 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -886,8 +886,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Note: if code flow gets here it means we didn't find the exact - # letsencrypt RewriteRule config for redirection. So if we find - # another RewriteRule it may induce a loop / config mismatch. + # 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 " From 681de292b7878b4d60acd8ffbef59be59bf6520c Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 09:29:02 +0000 Subject: [PATCH 17/23] Switch to using defaultdict(list) --- letsencrypt-apache/letsencrypt_apache/configurator.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index bc12a75fe..6b4a0e46e 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -25,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__) @@ -930,16 +931,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): rewrite_path = self.parser.find_dir( "RewriteRule", None, start=vhost.path) - dir_dict = {} + dir_dict = defaultdict(list) pat = r'.*(directive\[\d+\]).*' for match in rewrite_path: m = re.match(pat, match) if m: dir_id = m.group(1) - if dir_id in dir_dict: - dir_dict[dir_id].append(match) - else: - dir_dict[dir_id] = [match] + dir_dict[dir_id].append(match) if dir_dict: for dir_id in dir_dict: From 4748e1dd1e6134e7bf95e31241c4cb3990f33bca Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 09:57:08 +0000 Subject: [PATCH 18/23] Name the list of two redirect argument lists --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 6b4a0e46e..90921d327 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -940,10 +940,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): dir_dict[dir_id].append(match) if dir_dict: + redirect_args = [constants.REWRITE_HTTPS_ARGS, + constants.REWRITE_HTTPS_ARGS_WITH_END] for dir_id in dir_dict: - if [self.aug.get(x) for x in dir_dict[dir_id]] in [ - constants.REWRITE_HTTPS_ARGS, - constants.REWRITE_HTTPS_ARGS_WITH_END]: + if [self.aug.get(x) for x in dir_dict[dir_id]] in redirect_args: raise errors.PluginEnhancementAlreadyPresent( "Let's Encrypt has already enabled redirection") From ad5817c7a964730e78898a36e306cea9ccf41bbc Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 10:05:09 +0000 Subject: [PATCH 19/23] reason about dir_dict --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 90921d327..4a2c8b6d6 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -930,7 +930,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ rewrite_path = self.parser.find_dir( "RewriteRule", None, start=vhost.path) - + + # There can be other RewriteRule directive lines in vhost config. + # dir_dict keys are directive ids and the corresponding value for each + # is a list of arguments to that directive. dir_dict = defaultdict(list) pat = r'.*(directive\[\d+\]).*' for match in rewrite_path: @@ -942,6 +945,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if dir_dict: redirect_args = [constants.REWRITE_HTTPS_ARGS, constants.REWRITE_HTTPS_ARGS_WITH_END] + for dir_id in dir_dict: if [self.aug.get(x) for x in dir_dict[dir_id]] in redirect_args: raise errors.PluginEnhancementAlreadyPresent( From 23a97b82812d6c9cdb2e59c4a2a255dcefa08eaf Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 10:09:16 +0000 Subject: [PATCH 20/23] Change iteration on dir_dict --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 4a2c8b6d6..79042fe59 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -946,8 +946,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): redirect_args = [constants.REWRITE_HTTPS_ARGS, constants.REWRITE_HTTPS_ARGS_WITH_END] - for dir_id in dir_dict: - if [self.aug.get(x) for x in dir_dict[dir_id]] in redirect_args: + for matches in dir_dict.values(): + if [self.aug.get(x) for x in matches] in redirect_args: raise errors.PluginEnhancementAlreadyPresent( "Let's Encrypt has already enabled redirection") From ab1e75e426f361392c947365e17a0e02b4051946 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 10:17:46 +0000 Subject: [PATCH 21/23] Change dir_dict to rewrite_args_dict --- .../letsencrypt_apache/configurator.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 79042fe59..d60455cb6 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -932,21 +932,21 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "RewriteRule", None, start=vhost.path) # There can be other RewriteRule directive lines in vhost config. - # dir_dict keys are directive ids and the corresponding value for each - # is a list of arguments to that directive. - dir_dict = defaultdict(list) + # 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) - dir_dict[dir_id].append(match) + rewrite_args_dict[dir_id].append(match) - if dir_dict: + if rewrite_args_dict: redirect_args = [constants.REWRITE_HTTPS_ARGS, constants.REWRITE_HTTPS_ARGS_WITH_END] - for matches in dir_dict.values(): + 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") From 5f05c5104e0a38a1cc19d20d12de2c99e8739e2b Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 11:13:32 +0000 Subject: [PATCH 22/23] make lint happy --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d60455cb6..570455bb3 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -887,7 +887,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Note: if code flow gets here it means we didn't find the exact - # letsencrypt RewriteRule config for redirection. Finding + # 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. @@ -930,7 +930,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ 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. From 2edfc1cd59837ccfbea35810f0926837f2cbfb42 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 11 Dec 2015 11:59:26 +0000 Subject: [PATCH 23/23] simplified augeas get, with parsers get_arg func --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 570455bb3..33a9ea9db 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -972,10 +972,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost` """ - rewrite_engine_path = self.parser.find_dir("RewriteEngine", None, + rewrite_engine_path = self.parser.find_dir("RewriteEngine", "on", start=vhost.path) if rewrite_engine_path: - return self.aug.get(rewrite_engine_path[0]).lower() == "on" + return self.parser.get_arg(rewrite_engine_path[0]) return False def _create_redirect_vhost(self, ssl_vhost):