Apache plugin: configure all matching domain names to be able to answer HTTP challenge. (#6729)

Attempts to configure all of the following VirtualHosts for answering the HTTP challenge:

* VirtualHosts that have the requested domain name in either `ServerName` or `ServerAlias` directive.
* VirtualHosts that have a wildcard name that would match the requested domain name.

This also applies to HTTPS VirtualHosts, making Apache plugin able to handle cases where HTTP redirection takes place in reverse proxy or similar, before reaching the Apache HTTPD.

Even though also HTTPS VirtualHosts are selected, Apache plugin tries to ensure that at least one of the selected VirtualHosts listens to HTTP-01 port (configured with `--http-01-port` CLI option). So in a case where only HTTPS VirtualHosts exist, but user wants to configure those, `--http-01-port` parameter needs to be set for the port configured to the HTTPS VirtualHost(s).

Fixes: #6730

* Select all matching VirtualHosts for HTTP-01 challenges instead of just one

* Finalize PR and add tests

* Changelog entry

11 files changed, 112 insertions, 38 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 204b7b328..c529f3bb4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 * Lexicon-based DNS plugins are now fully compatible with Lexicon 3.x (support
   on 2.x branch is maintained).
+* Apache plugin now attempts to configure all VirtualHosts matching requested
+  domain name instead of only a single one when answering the HTTP-01 challenge.
 
 ### Fixed
 
diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py
index 16de3a3d8..efd766e63 100644
--- a/certbot-apache/certbot_apache/configurator.py
+++ b/certbot-apache/certbot_apache/configurator.py
@@ -577,8 +577,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self.assoc[target_name] = vhost
         return vhost
 
-    def included_in_wildcard(self, names, target_name):
-        """Is target_name covered by a wildcard?
+    def domain_in_names(self, names, target_name):
+        """Checks if target domain is covered by one or more of the provided
+        names. The target name is matched by wildcard as well as exact match.
 
         :param names: server aliases
         :type names: `collections.Iterable` of `str`
@@ -649,7 +650,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             names = vhost.get_names()
             if target_name in names:
                 points = 3
-            elif self.included_in_wildcard(names, target_name):
+            elif self.domain_in_names(names, target_name):
                 points = 2
             elif any(addr.get_addr() == target_name for addr in vhost.addrs):
                 points = 1
@@ -1463,7 +1464,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         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)
+        return self.domain_in_names(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/http_01.py b/certbot-apache/certbot_apache/http_01.py
index 22598baca..962433415 100644
--- a/certbot-apache/certbot_apache/http_01.py
+++ b/certbot-apache/certbot_apache/http_01.py
@@ -2,7 +2,7 @@
 import logging
 import os
 
-from acme.magic_typing import Set  # pylint: disable=unused-import, no-name-in-module
+from acme.magic_typing import List, Set  # pylint: disable=unused-import, no-name-in-module
 from certbot import errors
 from certbot.plugins import common
 from certbot_apache.obj import VirtualHost  # pylint: disable=unused-import
@@ -89,15 +89,27 @@ class ApacheHttp01(common.TLSSNI01):
                     self.configurator.enable_mod(mod, temp=True)
 
     def _mod_config(self):
+        selected_vhosts = []  # type: List[VirtualHost]
+        http_port = str(self.configurator.config.http01_port)
         for chall in self.achalls:
-            vh = self.configurator.find_best_http_vhost(
-                chall.domain, filter_defaults=False,
-                port=str(self.configurator.config.http01_port))
-            if vh:
-                self._set_up_include_directives(vh)
-            else:
-                for vh in self._relevant_vhosts():
-                    self._set_up_include_directives(vh)
+            # Search for matching VirtualHosts
+            for vh in self._matching_vhosts(chall.domain):
+                selected_vhosts.append(vh)
+
+        # Ensure that we have one or more VirtualHosts that we can continue
+        # with. (one that listens to port configured with --http-01-port)
+        found = False
+        for vhost in selected_vhosts:
+            if any(a.is_wildcard() or a.get_port() == http_port for a in vhost.addrs):
+                found = True
+
+        if not found:
+            for vh in self._relevant_vhosts():
+                selected_vhosts.append(vh)
+
+        # Add the challenge configuration
+        for vh in selected_vhosts:
+            self._set_up_include_directives(vh)
 
         self.configurator.reverter.register_file_creation(
             True, self.challenge_conf_pre)
@@ -121,6 +133,20 @@ class ApacheHttp01(common.TLSSNI01):
         with open(self.challenge_conf_post, "w") as new_conf:
             new_conf.write(config_text_post)
 
+    def _matching_vhosts(self, domain):
+        """Return all VirtualHost objects that have the requested domain name or
+        a wildcard name that would match the domain in ServerName or ServerAlias
+        directive.
+        """
+        matching_vhosts = []
+        for vhost in self.configurator.vhosts:
+            if self.configurator.domain_in_names(vhost.get_names(), domain):
+                # domain_in_names also matches the exact names, so no need
+                # to check "domain in vhost.get_names()" explicitly here
+                matching_vhosts.append(vhost)
+
+        return matching_vhosts
+
     def _relevant_vhosts(self):
         http01_port = str(self.configurator.config.http01_port)
         relevant_vhosts = []
diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py
index 4aaa23ea4..ff93682b6 100644
--- a/certbot-apache/certbot_apache/tests/configurator_test.py
+++ b/certbot-apache/certbot_apache/tests/configurator_test.py
@@ -139,7 +139,8 @@ class MultipleVhostsTest(util.ApacheTest):
         names = self.config.get_all_names()
         self.assertEqual(names, set(
             ["certbot.demo", "ocspvhost.com", "encryption-example.demo",
-             "nonsym.link", "vhost.in.rootconf", "www.certbot.demo"]
+             "nonsym.link", "vhost.in.rootconf", "www.certbot.demo",
+             "duplicate.example.com"]
         ))
 
     @certbot_util.patch_get_utility()
@@ -158,8 +159,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config.vhosts.append(vhost)
 
         names = self.config.get_all_names()
-        # Names get filtered, only 5 are returned
-        self.assertEqual(len(names), 8)
+        self.assertEqual(len(names), 9)
         self.assertTrue("zombo.com" in names)
         self.assertTrue("google.com" in names)
         self.assertTrue("certbot.demo" in names)
@@ -200,7 +200,7 @@ class MultipleVhostsTest(util.ApacheTest):
     def test_get_virtual_hosts(self):
         """Make sure all vhosts are being properly found."""
         vhs = self.config.get_virtual_hosts()
-        self.assertEqual(len(vhs), 10)
+        self.assertEqual(len(vhs), 12)
         found = 0
 
         for vhost in vhs:
@@ -211,7 +211,7 @@ class MultipleVhostsTest(util.ApacheTest):
             else:
                 raise Exception("Missed: %s" % vhost)  # pragma: no cover
 
-        self.assertEqual(found, 10)
+        self.assertEqual(found, 12)
 
         # Handle case of non-debian layout get_virtual_hosts
         with mock.patch(
@@ -219,7 +219,7 @@ class MultipleVhostsTest(util.ApacheTest):
         ) as mock_conf:
             mock_conf.return_value = False
             vhs = self.config.get_virtual_hosts()
-            self.assertEqual(len(vhs), 10)
+            self.assertEqual(len(vhs), 12)
 
     @mock.patch("certbot_apache.display_ops.select_vhost")
     def test_choose_vhost_none_avail(self, mock_select):
@@ -322,7 +322,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config.vhosts = [
             vh for vh in self.config.vhosts
             if vh.name not in ["certbot.demo", "nonsym.link",
-                "encryption-example.demo",
+                "encryption-example.demo", "duplicate.example.com",
                 "ocspvhost.com", "vhost.in.rootconf"]
             and "*.blue.purple.com" not in vh.aliases
         ]
@@ -333,7 +333,7 @@ class MultipleVhostsTest(util.ApacheTest):
     def test_non_default_vhosts(self):
         # pylint: disable=protected-access
         vhosts = self.config._non_default_vhosts(self.config.vhosts)
-        self.assertEqual(len(vhosts), 8)
+        self.assertEqual(len(vhosts), 10)
 
     def test_deploy_cert_enable_new_vhost(self):
         # Create
@@ -688,7 +688,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertEqual(self.config.is_name_vhost(self.vh_truth[0]),
                          self.config.is_name_vhost(ssl_vhost))
 
-        self.assertEqual(len(self.config.vhosts), 11)
+        self.assertEqual(len(self.config.vhosts), 13)
 
     def test_clean_vhost_ssl(self):
         # pylint: disable=protected-access
@@ -1269,7 +1269,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # pylint: disable=protected-access
         self.config._enable_redirect(self.vh_truth[1], "")
-        self.assertEqual(len(self.config.vhosts), 11)
+        self.assertEqual(len(self.config.vhosts), 13)
 
     def test_create_own_redirect_for_old_apache_version(self):
         self.config.parser.modules.add("rewrite_module")
@@ -1280,7 +1280,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # pylint: disable=protected-access
         self.config._enable_redirect(self.vh_truth[1], "")
-        self.assertEqual(len(self.config.vhosts), 11)
+        self.assertEqual(len(self.config.vhosts), 13)
 
     def test_sift_rewrite_rule(self):
         # pylint: disable=protected-access
diff --git a/certbot-apache/certbot_apache/tests/http_01_test.py b/certbot-apache/certbot_apache/tests/http_01_test.py
index 9c729b08c..bf7a3719a 100644
--- a/certbot-apache/certbot_apache/tests/http_01_test.py
+++ b/certbot-apache/certbot_apache/tests/http_01_test.py
@@ -27,8 +27,8 @@ class ApacheHttp01Test(util.ApacheTest):
         self.achalls = []  # type: List[achallenges.KeyAuthorizationAnnotatedChallenge]
         vh_truth = util.get_vh_truth(
             self.temp_dir, "debian_apache_2_4/multiple_vhosts")
-        # Takes the vhosts for encryption-example.demo, certbot.demo, and
-        # vhost.in.rootconf
+        # Takes the vhosts for encryption-example.demo, certbot.demo
+        # and vhost.in.rootconf
         self.vhosts = [vh_truth[0], vh_truth[3], vh_truth[10]]
 
         for i in range(NUM_ACHALLS):
@@ -39,7 +39,7 @@ class ApacheHttp01Test(util.ApacheTest):
                         "pending"),
                     domain=self.vhosts[i].name, account_key=self.account_key))
 
-        modules = ["rewrite", "authz_core", "authz_host"]
+        modules = ["ssl", "rewrite", "authz_core", "authz_host"]
         for mod in modules:
             self.config.parser.modules.add("mod_{0}.c".format(mod))
             self.config.parser.modules.add(mod + "_module")
@@ -111,6 +111,17 @@ class ApacheHttp01Test(util.ApacheTest):
                 domain="something.nonexistent", account_key=self.account_key)]
         self.common_perform_test(achalls, vhosts)
 
+    def test_configure_multiple_vhosts(self):
+        vhosts = [v for v in self.config.vhosts if "duplicate.example.com" in v.get_names()]
+        self.assertEqual(len(vhosts), 2)
+        achalls = [
+            achallenges.KeyAuthorizationAnnotatedChallenge(
+                challb=acme_util.chall_to_challb(
+                    challenges.HTTP01(token=((b'a' * 16))),
+                    "pending"),
+                domain="duplicate.example.com", account_key=self.account_key)]
+        self.common_perform_test(achalls, vhosts)
+
     def test_no_vhost(self):
         for achall in self.achalls:
             self.http.add_chall(achall)
@@ -176,15 +187,14 @@ class ApacheHttp01Test(util.ApacheTest):
             self._test_challenge_file(achall)
 
         for vhost in vhosts:
-            if not vhost.ssl:
-                matches = self.config.parser.find_dir("Include",
-                                                      self.http.challenge_conf_pre,
-                                                      vhost.path)
-                self.assertEqual(len(matches), 1)
-                matches = self.config.parser.find_dir("Include",
-                                                      self.http.challenge_conf_post,
-                                                      vhost.path)
-                self.assertEqual(len(matches), 1)
+            matches = self.config.parser.find_dir("Include",
+                                                self.http.challenge_conf_pre,
+                                                vhost.path)
+            self.assertEqual(len(matches), 1)
+            matches = self.config.parser.find_dir("Include",
+                                                self.http.challenge_conf_post,
+                                                vhost.path)
+            self.assertEqual(len(matches), 1)
 
         self.assertTrue(os.path.exists(challenge_dir))
 
diff --git a/certbot-apache/certbot_apache/tests/parser_test.py b/certbot-apache/certbot_apache/tests/parser_test.py
index a089ec471..5d692eeac 100644
--- a/certbot-apache/certbot_apache/tests/parser_test.py
+++ b/certbot-apache/certbot_apache/tests/parser_test.py
@@ -52,7 +52,7 @@ class BasicParserTest(util.ParserTest):
         test2 = self.parser.find_dir("documentroot")
 
         self.assertEqual(len(test), 1)
-        self.assertEqual(len(test2), 7)
+        self.assertEqual(len(test2), 8)
 
     def test_add_dir(self):
         aug_default = "/files" + self.parser.loc["default"]
diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttp.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttp.conf
new file mode 100644
index 000000000..5684651fb
--- /dev/null
+++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttp.conf
@@ -0,0 +1,9 @@
+<VirtualHost 10.2.3.4:80>
+	ServerName duplicate.example.com
+
+	ServerAdmin webmaster@certbot.demo
+	DocumentRoot /var/www/html
+
+	ErrorLog ${APACHE_LOG_DIR}/error.log
+	CustomLog ${APACHE_LOG_DIR}/access.log combined
+</VirtualHost>
diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttps.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttps.conf
new file mode 100644
index 000000000..e3ac21fac
--- /dev/null
+++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttps.conf
@@ -0,0 +1,14 @@
+<IfModule mod_ssl.c>
+<VirtualHost 10.2.3.4:443>
+	ServerName duplicate.example.com
+
+	ServerAdmin webmaster@certbot.demo
+	DocumentRoot /var/www/html
+
+	ErrorLog ${APACHE_LOG_DIR}/error.log
+	CustomLog ${APACHE_LOG_DIR}/access.log combined
+
+SSLCertificateFile	/etc/apache2/certs/certbot-cert_5.pem
+SSLCertificateKeyFile /etc/apache2/ssl/key-certbot_15.pem
+</VirtualHost>
+</IfModule>
diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttp.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttp.conf
new file mode 120000
index 000000000..a69ee3c1d
--- /dev/null
+++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttp.conf
@@ -0,0 +1 @@
+../sites-available/duplicatehttp.conf
\ No newline at end of file
diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttps.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttps.conf
new file mode 120000
index 000000000..a52ee1ccb
--- /dev/null
+++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttps.conf
@@ -0,0 +1 @@
+../sites-available/duplicatehttps.conf
\ No newline at end of file
diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py
index 9329ccb20..02de6ada4 100644
--- a/certbot-apache/certbot_apache/tests/util.py
+++ b/certbot-apache/certbot_apache/tests/util.py
@@ -196,7 +196,17 @@ def get_vh_truth(temp_dir, config_name):
                 "/files" + os.path.join(temp_dir, config_name,
                                         "apache2/apache2.conf/VirtualHost"),
                 set([obj.Addr.fromstring("*:80")]), False, True,
-                "vhost.in.rootconf")]
+                "vhost.in.rootconf"),
+            obj.VirtualHost(
+                os.path.join(prefix, "duplicatehttp.conf"),
+                os.path.join(aug_pre, "duplicatehttp.conf/VirtualHost"),
+                set([obj.Addr.fromstring("10.2.3.4:80")]), False, True,
+                "duplicate.example.com"),
+            obj.VirtualHost(
+                os.path.join(prefix, "duplicatehttps.conf"),
+                os.path.join(aug_pre, "duplicatehttps.conf/IfModule/VirtualHost"),
+                set([obj.Addr.fromstring("10.2.3.4:443")]), True, True,
+                "duplicate.example.com")]
         return vh_truth
     if config_name == "debian_apache_2_4/multi_vhosts":
         prefix = os.path.join(