From 8b96264576741eee5b66fd5b012edd529ffa545c Mon Sep 17 00:00:00 2001 From: James Kasten Date: Wed, 22 Jul 2015 02:05:01 -0700 Subject: [PATCH] improved redirect, testcases --- .../letsencrypt_apache/configurator.py | 30 ++++++---- .../letsencrypt_apache/display_ops.py | 6 +- letsencrypt-apache/letsencrypt_apache/obj.py | 1 + .../letsencrypt_apache/parser.py | 6 +- .../tests/configurator_test.py | 56 +++++++++++++++++++ .../letsencrypt_apache/tests/dvsni_test.py | 5 +- .../apache2/mods-available/rewrite.load | 1 + 7 files changed, 87 insertions(+), 18 deletions(-) create mode 100644 letsencrypt-apache/letsencrypt_apache/tests/testdata/debian_apache_2_4/two_vhost_80/apache2/mods-available/rewrite.load diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 6ef10baab..a27a5cb95 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1,5 +1,6 @@ """Apache Configuration based off of Augeas Configurator.""" # pylint: disable=too-many-lines +import itertools import logging import os import re @@ -60,6 +61,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): prone. .. todo:: Write a server protocol finder. Listen or Protocol . This can verify partial setups are correct + .. todo:: Add directives to sites-enabled... not sites-available. + sites-available doesn't allow immediate find_dir search even with save() + and load() :ivar config: Configuration. :type config: :class:`~letsencrypt.interfaces.IConfig` @@ -672,11 +676,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): try: return self._enhance_func[enhancement]( self.choose_vhost(domain), options) - except ValueError: + except KeyError: raise errors.PluginError( "Unsupported enhancement: {}".format(enhancement)) except errors.PluginError: logger.warn("Failed %s for %s", enhancement, domain) + raise def _enable_redirect(self, ssl_vhost, unused_options): """Redirect all equivalent HTTP traffic to ssl_vhost. @@ -703,9 +708,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ if "rewrite_module" not in self.parser.modules: self.enable_mod("rewrite") + general_vh = self._get_http_vhost(ssl_vhost) - general_v = self._get_http_vhost(ssl_vhost) - if general_v is None: + if general_vh is None: # Add virtual_server with redirect logger.debug("Did not find http version of ssl virtual host " "attempting to create") @@ -717,20 +722,23 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "Unable to create one as intended addresses conflict; " "Current configuration does not support automated " "redirection") - self.create_redirect_vhost(redirect_addrs) + self._create_redirect_vhost(redirect_addrs) else: # Check if redirection already exists - self._verify_no_redirects(vhost) + self._verify_no_redirects(general_vh) + # Add directives to server - self.parser.add_dir(general_v.path, "RewriteEngine", "on") - self.parser.add_dir(general_v.path, "RewriteRule", + # 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", constants.REWRITE_HTTPS_ARGS) self.save_notes += ("Redirecting host in %s to ssl vhost in %s\n" % - (general_v.filep, ssl_vhost.filep)) + (general_vh.filep, ssl_vhost.filep)) self.save() logger.info("Redirecting vhost in %s to ssl vhost in %s", - general_v.filep, ssl_vhost.filep) + general_vh.filep, ssl_vhost.filep) def _verify_no_redirects(self, vhost): """Checks to see if existing redirect is in place. @@ -775,7 +783,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ text = self._get_redirect_config_str(ssl_vhost) - self._write_out_redirect(ssl_vhost, text) + redirect_filepath = self._write_out_redirect(ssl_vhost, text) self.aug.load() # Make a new vhost data structure and add it to the lists @@ -853,7 +861,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def _get_redirect_addrs(self, ssl_vhost): redirects = set() - for addr in vhost.addrs: + for addr in ssl_vhost.addrs: redirects.add(addr.get_addr_obj("80")) return redirects diff --git a/letsencrypt-apache/letsencrypt_apache/display_ops.py b/letsencrypt-apache/letsencrypt_apache/display_ops.py index 352845376..45c55f49a 100644 --- a/letsencrypt-apache/letsencrypt_apache/display_ops.py +++ b/letsencrypt-apache/letsencrypt_apache/display_ops.py @@ -60,9 +60,9 @@ def _vhost_menu(domain, vhosts): choices = [] for vhost in vhosts: - if len(vhost.names) == 1: - disp_name = next(iter(vhost.names)) - elif len(vhost.names) == 0: + if len(vhost.get_names()) == 1: + disp_name = next(iter(vhost.get_names())) + elif len(vhost.get_names()) == 0: disp_name = "" else: disp_name = "Multiple Names" diff --git a/letsencrypt-apache/letsencrypt_apache/obj.py b/letsencrypt-apache/letsencrypt_apache/obj.py index 6895e9039..d453f34e3 100644 --- a/letsencrypt-apache/letsencrypt_apache/obj.py +++ b/letsencrypt-apache/letsencrypt_apache/obj.py @@ -78,6 +78,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods self.aliases = serveralias def get_names(self): + """Return a set of all names.""" all_names = set(self.aliases) # Strip out any scheme:// and field from servername if self.name is not None: diff --git a/letsencrypt-apache/letsencrypt_apache/parser.py b/letsencrypt-apache/letsencrypt_apache/parser.py index ada2db027..17f6e8f28 100644 --- a/letsencrypt-apache/letsencrypt_apache/parser.py +++ b/letsencrypt-apache/letsencrypt_apache/parser.py @@ -287,9 +287,11 @@ class ApacheParser(object): dir_ = self.aug.get(match).lower() if dir_ == "include" or dir_ == "includeoptional": # start[6:] to strip off /files + #print self._get_include_path(self.get_arg(match +"/arg")), directive, arg ordered_matches.extend(self.find_dir( - directive, arg, self._get_include_path( - self.get_arg(match + "/arg")))) + directive, arg, + self._get_include_path(self.get_arg(match + "/arg")), + exclude)) # This additionally allows Include if dir_ == directive.lower(): ordered_matches.extend(self.aug.match(match + arg_suffix)) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 101a53a97..68b0433f1 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -254,6 +254,62 @@ class TwoVhost80Test(util.ApacheTest): mock_popen.side_effect = OSError("Can't find program") self.assertRaises(errors.PluginError, self.config.get_version) + # TEST ENHANCEMENTS + def test_enhance_unknown_enhancement(self): + self.assertRaises( + errors.PluginError, + self.config.enhance, "letsencrypt.demo", "unknown_enhancement") + + @mock.patch("letsencrypt_apache.parser." + "ApacheParser.update_runtime_variables") + def test_redirect_well_formed_http(self, unused): + # 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 + self.assertEqual(len(rw_rule), 3) + + 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_twice(self): + # Skip the enable mod + self.config.parser.modules.add("rewrite_module") + self.config.enhance("encryption-example.demo", "redirect") + self.assertRaises( + errors.PluginError, + 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_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") + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py b/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py index b0aec4f8a..29b600ec3 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py @@ -109,9 +109,10 @@ class DvsniPerformTest(util.ApacheTest): self.assertEqual(len(vhs), 2) for vhost in vhs: self.assertEqual(vhost.addrs, set([obj.Addr.fromstring("*:443")])) + names = vhost.get_names() self.assertTrue( - vhost.names == set([self.achalls[0].nonce_domain]) or - vhost.names == set([self.achalls[1].nonce_domain])) + names == set([self.achalls[0].nonce_domain]) or + names == set([self.achalls[1].nonce_domain])) if __name__ == "__main__": diff --git a/letsencrypt-apache/letsencrypt_apache/tests/testdata/debian_apache_2_4/two_vhost_80/apache2/mods-available/rewrite.load b/letsencrypt-apache/letsencrypt_apache/tests/testdata/debian_apache_2_4/two_vhost_80/apache2/mods-available/rewrite.load new file mode 100644 index 000000000..b32f16264 --- /dev/null +++ b/letsencrypt-apache/letsencrypt_apache/tests/testdata/debian_apache_2_4/two_vhost_80/apache2/mods-available/rewrite.load @@ -0,0 +1 @@ +LoadModule rewrite_module /usr/lib/apache2/modules/mod_rewrite.so