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

@@ -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.
 

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

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/another_wildcard.conf

@@ -0,0 +1,11 @@
+<VirtualHost *:80>
+	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
+</VirtualHost>
+
+# vim: syntax=apache ts=4 sw=4 sts=4 sr noet

certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/augeas_vhosts/apache2/sites-available/wildcard.conf

@@ -0,0 +1,11 @@
+<VirtualHost *:80>
+	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
+</VirtualHost>
+
+# vim: syntax=apache ts=4 sw=4 sts=4 sr noet