diff --git a/CHANGELOG.md b/CHANGELOG.md index 5060a7038..0f44258af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ Certbot adheres to [Semantic Versioning](http://semver.org/). ### Fixed * Match Nginx parser update in allowing variable names to start with `${`. +* Fix ranking of vhosts in Nginx so that all port-matching vhosts come first * Correct OVH integration tests on machines without internet access. * Stop caching the results of ipv6_info in http01.py diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index d526381a2..0f9c43e2d 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -8,7 +8,6 @@ import tempfile import time import OpenSSL -import six import zope.interface from acme import challenges @@ -32,6 +31,12 @@ from certbot_nginx import obj # pylint: disable=unused-import from acme.magic_typing import List, Dict, Set # pylint: disable=unused-import, no-name-in-module +NAME_RANK = 0 +START_WILDCARD_RANK = 1 +END_WILDCARD_RANK = 2 +REGEX_RANK = 3 +NO_SSL_MODIFIER = 4 + logger = logging.getLogger(__name__) @@ -405,7 +410,8 @@ class NginxConfigurator(common.Installer): """ if not matches: return None - elif matches[0]['rank'] in six.moves.range(2, 6): + elif matches[0]['rank'] in [START_WILDCARD_RANK, END_WILDCARD_RANK, + START_WILDCARD_RANK + NO_SSL_MODIFIER, END_WILDCARD_RANK + NO_SSL_MODIFIER]: # Wildcard match - need to find the longest one rank = matches[0]['rank'] wildcards = [x for x in matches if x['rank'] == rank] @@ -414,10 +420,9 @@ class NginxConfigurator(common.Installer): # Exact or regex match return matches[0]['vhost'] - - def _rank_matches_by_name_and_ssl(self, vhost_list, target_name): + def _rank_matches_by_name(self, vhost_list, target_name): """Returns a ranked list of vhosts from vhost_list that match target_name. - The ranking gives preference to SSL vhosts. + This method should always be followed by a call to _select_best_name_match. :param list vhost_list: list of vhosts to filter and rank :param str target_name: The name to match @@ -437,21 +442,37 @@ class NginxConfigurator(common.Installer): if name_type == 'exact': matches.append({'vhost': vhost, 'name': name, - 'rank': 0 if vhost.ssl else 1}) + 'rank': NAME_RANK}) elif name_type == 'wildcard_start': matches.append({'vhost': vhost, 'name': name, - 'rank': 2 if vhost.ssl else 3}) + 'rank': START_WILDCARD_RANK}) elif name_type == 'wildcard_end': matches.append({'vhost': vhost, 'name': name, - 'rank': 4 if vhost.ssl else 5}) + 'rank': END_WILDCARD_RANK}) elif name_type == 'regex': matches.append({'vhost': vhost, 'name': name, - 'rank': 6 if vhost.ssl else 7}) + 'rank': REGEX_RANK}) return sorted(matches, key=lambda x: x['rank']) + def _rank_matches_by_name_and_ssl(self, vhost_list, target_name): + """Returns a ranked list of vhosts from vhost_list that match target_name. + The ranking gives preference to SSLishness before name match level. + + :param list vhost_list: list of vhosts to filter and rank + :param str target_name: The name to match + :returns: list of dicts containing the vhost, the matching name, and + the numerical rank + :rtype: list + + """ + matches = self._rank_matches_by_name(vhost_list, target_name) + for match in matches: + if not match['vhost'].ssl: + match['rank'] += NO_SSL_MODIFIER + return sorted(matches, key=lambda x: x['rank']) def choose_redirect_vhosts(self, target_name, port, create_if_no_match=False): """Chooses a single virtual host for redirect enhancement. @@ -531,9 +552,7 @@ class NginxConfigurator(common.Installer): matching_vhosts = [vhost for vhost in all_vhosts if _vhost_matches(vhost, port)] - # We can use this ranking function because sslishness doesn't matter to us, and - # there shouldn't be conflicting plaintextish servers listening on 80. - return self._rank_matches_by_name_and_ssl(matching_vhosts, target_name) + return self._rank_matches_by_name(matching_vhosts, target_name) def get_all_names(self): """Returns all names found in the Nginx Configuration. @@ -568,6 +587,7 @@ class NginxConfigurator(common.Installer): return util.get_filtered_names(all_names) def _get_snakeoil_paths(self): + """Generate invalid certs that let us create ssl directives for Nginx""" # TODO: generate only once tmp_dir = os.path.join(self.config.work_dir, "snakeoil") le_key = crypto_util.init_save_key( diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 4d23f3518..b180bb930 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -128,22 +128,39 @@ class NginxConfiguratorTest(util.NginxTest): ['#', parser.COMMENT]]]], parsed[0]) - def test_choose_vhosts(self): - localhost_conf = set(['localhost', r'~^(www\.)?(example|bar)\.']) - server_conf = set(['somename', 'another.alias', 'alias']) - example_conf = set(['.example.com', 'example.*']) - foo_conf = set(['*.www.foo.com', '*.www.example.com']) - ipv6_conf = set(['ipv6.com']) + def test_choose_vhosts_alias(self): + self._test_choose_vhosts_common('alias', 'server_conf') - results = {'localhost': localhost_conf, - 'alias': server_conf, - 'example.com': example_conf, - 'example.com.uk.test': example_conf, - 'www.example.com': example_conf, - 'test.www.example.com': foo_conf, - 'abc.www.foo.com': foo_conf, - 'www.bar.co.uk': localhost_conf, - 'ipv6.com': ipv6_conf} + def test_choose_vhosts_example_com(self): + self._test_choose_vhosts_common('example.com', 'example_conf') + + def test_choose_vhosts_localhost(self): + self._test_choose_vhosts_common('localhost', 'localhost_conf') + + def test_choose_vhosts_example_com_uk_test(self): + self._test_choose_vhosts_common('example.com.uk.test', 'example_conf') + + def test_choose_vhosts_www_example_com(self): + self._test_choose_vhosts_common('www.example.com', 'example_conf') + + def test_choose_vhosts_test_www_example_com(self): + self._test_choose_vhosts_common('test.www.example.com', 'foo_conf') + + def test_choose_vhosts_abc_www_foo_com(self): + self._test_choose_vhosts_common('abc.www.foo.com', 'foo_conf') + + def test_choose_vhosts_www_bar_co_uk(self): + self._test_choose_vhosts_common('www.bar.co.uk', 'localhost_conf') + + def test_choose_vhosts_ipv6_com(self): + self._test_choose_vhosts_common('ipv6.com', 'ipv6_conf') + + def _test_choose_vhosts_common(self, name, conf): + conf_names = {'localhost_conf': set(['localhost', r'~^(www\.)?(example|bar)\.']), + 'server_conf': set(['somename', 'another.alias', 'alias']), + 'example_conf': set(['.example.com', 'example.*']), + 'foo_conf': set(['*.www.foo.com', '*.www.example.com']), + 'ipv6_conf': set(['ipv6.com'])} conf_path = {'localhost': "etc_nginx/nginx.conf", 'alias': "etc_nginx/nginx.conf", @@ -155,22 +172,22 @@ class NginxConfiguratorTest(util.NginxTest): 'www.bar.co.uk': "etc_nginx/nginx.conf", 'ipv6.com': "etc_nginx/sites-enabled/ipv6.com"} + vhost = self.config.choose_vhosts(name)[0] + path = os.path.relpath(vhost.filep, self.temp_dir) + + self.assertEqual(conf_names[conf], vhost.names) + self.assertEqual(conf_path[name], path) + # IPv6 specific checks + if name == "ipv6.com": + self.assertTrue(vhost.ipv6_enabled()) + # Make sure that we have SSL enabled also for IPv6 addr + self.assertTrue( + any([True for x in vhost.addrs if x.ssl and x.ipv6])) + + def test_choose_vhosts_bad(self): bad_results = ['www.foo.com', 'example', 't.www.bar.co', '69.255.225.155'] - for name in results: - vhost = self.config.choose_vhosts(name)[0] - path = os.path.relpath(vhost.filep, self.temp_dir) - - self.assertEqual(results[name], vhost.names) - self.assertEqual(conf_path[name], path) - # IPv6 specific checks - if name == "ipv6.com": - self.assertTrue(vhost.ipv6_enabled()) - # Make sure that we have SSL enabled also for IPv6 addr - self.assertTrue( - any([True for x in vhost.addrs if x.ssl and x.ipv6])) - for name in bad_results: self.assertRaises(errors.MisconfigurationError, self.config.choose_vhosts, name)