From 7e6a1f248866df8b581f372f3e57355ca4ab4b1c Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 6 Feb 2019 20:02:35 +0200 Subject: [PATCH] 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 --- CHANGELOG.md | 2 + certbot-apache/certbot_apache/configurator.py | 9 ++-- certbot-apache/certbot_apache/http_01.py | 44 +++++++++++++++---- .../certbot_apache/tests/configurator_test.py | 22 +++++----- .../certbot_apache/tests/http_01_test.py | 34 +++++++++----- .../certbot_apache/tests/parser_test.py | 2 +- .../sites-available/duplicatehttp.conf | 9 ++++ .../sites-available/duplicatehttps.conf | 14 ++++++ .../apache2/sites-enabled/duplicatehttp.conf | 1 + .../apache2/sites-enabled/duplicatehttps.conf | 1 + certbot-apache/certbot_apache/tests/util.py | 12 ++++- 11 files changed, 112 insertions(+), 38 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttp.conf create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttps.conf create mode 120000 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttp.conf create mode 120000 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-enabled/duplicatehttps.conf diff --git a/CHANGELOG.md b/CHANGELOG.md index 204b7b32890..c529f3bb484 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 16de3a3d86d..efd766e63ec 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -577,8 +577,9 @@ def _choose_vhost_from_list(self, target_name, temp=False): 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 @@ def _find_best_vhost(self, target_name, vhosts=None, filter_defaults=True): 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 @@ def _has_matching_wildcard(self, vh_path, target_name): 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 22598bacaa0..962433415d4 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 @@ def prepare_http01_modules(self): 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 @@ def _mod_config(self): 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 4aaa23ea443..ff93682b668 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -139,7 +139,8 @@ def test_get_all_names(self, mock_getutility): 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 @@ def test_get_all_names_addrs(self, mock_gethost, mock_getutility): 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 @@ def test_add_servernames_alias(self): 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 @@ def test_get_virtual_hosts(self): 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 @@ def test_get_virtual_hosts(self): ) 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 @@ def test_find_best_vhost_default(self): 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 @@ def test_find_best_vhost_default(self): 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 @@ def test_make_vhost_ssl(self): 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 @@ def test_create_own_redirect(self): # 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 @@ def test_create_own_redirect_for_old_apache_version(self): # 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 9c729b08ce0..bf7a3719a41 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 @@ def setUp(self, *args, **kwargs): 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 @@ def setUp(self, *args, **kwargs): "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 @@ def test_anonymous_vhost(self): 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 @@ def common_perform_test(self, achalls, vhosts): 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 a089ec471b9..5d692eeacef 100644 --- a/certbot-apache/certbot_apache/tests/parser_test.py +++ b/certbot-apache/certbot_apache/tests/parser_test.py @@ -52,7 +52,7 @@ def test_find_dir(self): 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 00000000000..5684651fbb8 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttp.conf @@ -0,0 +1,9 @@ + + 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 + 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 00000000000..e3ac21fac80 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multiple_vhosts/apache2/sites-available/duplicatehttps.conf @@ -0,0 +1,14 @@ + + + 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 + + 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 00000000000..a69ee3c1dca --- /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 00000000000..a52ee1ccb97 --- /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 9329ccb2005..02de6ada4f6 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(