From 28ce10fef5d88fe662bff05f986903685bc16cb1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 20 Dec 2016 15:53:52 -0800 Subject: [PATCH] Don't add ServerAlias directives when the domain is already covered by a wildcard (#3917) * correctly match * and ? in ServerAlias directives * update Apache wildcard test * Consolidate wildcard matching and remove bad test * Test Apache vhost selection with wildcards * Added few more tests to proof vhost selection --- certbot-apache/certbot_apache/configurator.py | 49 ++++++++++++++----- .../certbot_apache/tests/configurator_test.py | 42 +++++++++++++--- .../sites-available/another_wildcard.conf | 11 +++++ .../apache2/sites-available/wildcard.conf | 11 +++++ 4 files changed, 95 insertions(+), 18 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index b200d5eaa..27e214362 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -1,6 +1,7 @@ """Apache Configuration based off of Augeas Configurator.""" # pylint: disable=too-many-lines import filecmp +import fnmatch import logging import os import re @@ -362,18 +363,24 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return vhost def included_in_wildcard(self, names, target_name): - """Helper function to see if alias is covered by wildcard""" - target_name = target_name.split(".")[::-1] - wildcards = [domain.split(".")[1:] for domain in - names if domain.startswith("*")] - for wildcard in wildcards: - if len(wildcard) > len(target_name): - continue - for idx, segment in enumerate(wildcard[::-1]): - if segment != target_name[idx]: - break - else: - # https://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops + """Is target_name covered by a wildcard? + + :param names: server aliases + :type names: `collections.Iterable` of `str` + :param str target_name: name to compare with wildcards + + :returns: True if target_name is covered by a wildcard, + otherwise, False + :rtype: bool + + """ + # use lowercase strings because fnmatch can be case sensitive + target_name = target_name.lower() + for name in names: + name = name.lower() + # fnmatch treats "[seq]" specially and [ or ] characters aren't + # valid in Apache but Apache doesn't error out if they are present + if "[" not in name and fnmatch.fnmatch(target_name, name): return True return False @@ -1012,6 +1019,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.parser.find_dir("ServerAlias", target_name, start=vh_path, exclude=False)): return + if self._has_matching_wildcard(vh_path, target_name): + return if not self.parser.find_dir("ServerName", None, start=vh_path, exclude=False): self.parser.add_dir(vh_path, "ServerName", target_name) @@ -1019,6 +1028,22 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.parser.add_dir(vh_path, "ServerAlias", target_name) self._add_servernames(vhost) + def _has_matching_wildcard(self, vh_path, target_name): + """Is target_name already included in a wildcard in the vhost? + + :param str vh_path: Augeas path to the vhost + :param str target_name: name to compare with wildcards + + :returns: True if there is a wildcard covering target_name in + the vhost in vhost_path, otherwise, False + :rtype: bool + + """ + matches = self.parser.find_dir( + "ServerAlias", start=vh_path, exclude=False) + aliases = (self.aug.get(match) for match in matches) + return self.included_in_wildcard(aliases, target_name) + def _add_name_vhost_if_necessary(self, vhost): """Add NameVirtualHost Directives if necessary for new vhost. diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 2f1d01315..1af425824 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -220,10 +220,6 @@ class MultipleVhostsTest(util.ApacheTest): self.assertRaises( errors.PluginError, self.config.choose_vhost, "none.com") - def test_choosevhost_select_vhost_with_wildcard(self): - chosen_vhost = self.config.choose_vhost("blue.purple.com", temp=True) - self.assertEqual(self.vh_truth[6], chosen_vhost) - def test_findbest_continues_on_short_domain(self): # pylint: disable=protected-access chosen_vhost = self.config._find_best_vhost("purple.com") @@ -1255,8 +1251,6 @@ class AugeasVhostsTest(util.ApacheTest): self.config = util.get_apache_configurator( self.config_path, self.vhost_path, self.config_dir, self.work_dir) - self.vh_truth = util.get_vh_truth( - self.temp_dir, "debian_apache_2_4/augeas_vhosts") def tearDown(self): shutil.rmtree(self.temp_dir) @@ -1281,5 +1275,41 @@ class AugeasVhostsTest(util.ApacheTest): vhs = self.config.get_virtual_hosts() self.assertEqual([], vhs) + def test_choose_vhost_with_matching_wildcard(self): + names = ( + "an.example.net", "another.example.net", "an.other.example.net") + for name in names: + self.assertFalse(name in self.config.choose_vhost(name).aliases) + + def test_choose_vhost_without_matching_wildcard(self): + mock_path = "certbot_apache.display_ops.select_vhost" + with mock.patch(mock_path, lambda _, vhosts: vhosts[0]): + for name in ("a.example.net", "other.example.net"): + self.assertTrue(name in self.config.choose_vhost(name).aliases) + + def test_choose_vhost_wildcard_not_found(self): + mock_path = "certbot_apache.display_ops.select_vhost" + names = ( + "abc.example.net", "not.there.tld", "aa.wildcard.tld" + ) + with mock.patch(mock_path) as mock_select: + mock_select.return_value = self.config.vhosts[0] + for name in names: + orig_cc = mock_select.call_count + self.config.choose_vhost(name) + self.assertEqual(mock_select.call_count - orig_cc, 1) + + def test_choose_vhost_wildcard_found(self): + mock_path = "certbot_apache.display_ops.select_vhost" + names = ( + "ab.example.net", "a.wildcard.tld", "yetanother.example.net" + ) + with mock.patch(mock_path) as mock_select: + mock_select.return_value = self.config.vhosts[0] + for name in names: + self.config.choose_vhost(name) + self.assertEqual(mock_select.call_count, 0) + + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf new file mode 100644 index 000000000..1a5b7de47 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf @@ -0,0 +1,11 @@ + + ServerName wildcard.tld + ServerAlias ?.wildcard.tld + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf new file mode 100644 index 000000000..b8046e6c9 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf @@ -0,0 +1,11 @@ + + ServerName example.net + ServerAlias ??.example.net *.other.example.net *another.example.net + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet