diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py index 15fbe61f7..8a3b8078f 100644 --- a/certbot-nginx/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/certbot_nginx/_internal/configurator.py @@ -1,3 +1,4 @@ +# pylint: disable=too-many-lines """Nginx Configuration""" from distutils.version import LooseVersion import logging @@ -15,8 +16,10 @@ from acme import challenges from acme import crypto_util as acme_crypto_util from acme.magic_typing import Dict from acme.magic_typing import List +from acme.magic_typing import Optional from acme.magic_typing import Set from acme.magic_typing import Text +from acme.magic_typing import Tuple from certbot import crypto_util from certbot import errors from certbot import interfaces @@ -105,7 +108,7 @@ class NginxConfigurator(common.Installer): self.save_notes = "" # For creating new vhosts if no names match - self.new_vhost = None + self.new_vhost: Optional[obj.VirtualHost] = None # List of vhosts configured per wildcard domain on this run. # used by deploy_cert() and enhance() @@ -116,7 +119,7 @@ class NginxConfigurator(common.Installer): self._chall_out = 0 # These will be set in the prepare function - self.parser = None + self.parser: Optional[parser.NginxParser] = None self.version = version self.openssl_version = openssl_version self._enhance_func = {"redirect": self._enable_redirect, @@ -377,10 +380,13 @@ class NginxConfigurator(common.Installer): ipv6only_present = True return (ipv6_active, ipv6only_present) - def _vhost_from_duplicated_default(self, domain, allow_port_mismatch, port): + def _vhost_from_duplicated_default(self, domain: str, allow_port_mismatch: bool, port: str + ) -> obj.VirtualHost: """if allow_port_mismatch is False, only server blocks with matching ports will be used as a default server block template. """ + assert self.parser is not None # prepare should already have been called here + if self.new_vhost is None: default_vhost = self._get_default_vhost(domain, allow_port_mismatch, port) self.new_vhost = self.parser.duplicate_vhost(default_vhost, @@ -509,7 +515,7 @@ class NginxConfigurator(common.Installer): 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): + def choose_redirect_vhosts(self, target_name: str, port: str) -> List[obj.VirtualHost]: """Chooses a single virtual host for redirect enhancement. Chooses the vhost most closely matching target_name that is @@ -523,9 +529,6 @@ class NginxConfigurator(common.Installer): :param str target_name: domain name :param str port: port number - :param bool create_if_no_match: If we should create a new vhost from default - when there is no match found. If we can't choose a default, raise a - MisconfigurationError. :returns: vhosts associated with name :rtype: list of :class:`~certbot_nginx._internal.obj.VirtualHost` @@ -538,32 +541,75 @@ class NginxConfigurator(common.Installer): else: matches = self._get_redirect_ranked_matches(target_name, port) vhosts = [x for x in [self._select_best_name_match(matches)]if x is not None] - if not vhosts and create_if_no_match: - vhosts = [self._vhost_from_duplicated_default(target_name, False, port)] return vhosts - def _port_matches(self, test_port, matching_port): + def choose_auth_vhosts(self, target_name: str) -> Tuple[List[obj.VirtualHost], + List[obj.VirtualHost]]: + """Returns a list of HTTP and HTTPS vhosts with a server_name matching target_name. + + If no HTTP vhost exists, one will be cloned from the default vhost. If that fails, no HTTP + vhost will be returned. + + :param str target_name: non-wildcard domain name + + :returns: tuple of HTTP and HTTPS virtualhosts + :rtype: tuple of :class:`~certbot_nginx._internal.obj.VirtualHost` + + """ + vhosts = [m['vhost'] for m in self._get_ranked_matches(target_name) if m and 'vhost' in m] + http_vhosts = [vh for vh in vhosts if + self._vhost_listening(vh, str(self.config.http01_port), False)] + https_vhosts = [vh for vh in vhosts if + self._vhost_listening(vh, str(self.config.https_port), True)] + + # If no HTTP vhost matches, try create one from the default_server on http01_port. + if not http_vhosts: + try: + http_vhosts = [self._vhost_from_duplicated_default(target_name, False, + str(self.config.http01_port))] + except errors.MisconfigurationError: + http_vhosts = [] + + return http_vhosts, https_vhosts + + def _port_matches(self, test_port: str, matching_port: str) -> bool: # test_port is a number, matching is a number or "" or None if matching_port == "" or matching_port is None: # if no port is specified, Nginx defaults to listening on port 80. return test_port == self.DEFAULT_LISTEN_PORT return test_port == matching_port - def _vhost_listening_on_port_no_ssl(self, vhost, port): - found_matching_port = False - if not vhost.addrs: - # if there are no listen directives at all, Nginx defaults to - # listening on port 80. - found_matching_port = (port == self.DEFAULT_LISTEN_PORT) - else: - for addr in vhost.addrs: - if self._port_matches(port, addr.get_port()) and not addr.ssl: - found_matching_port = True + def _vhost_listening(self, vhost: obj.VirtualHost, port: str, ssl: bool) -> bool: + """Tests whether a vhost has an address listening on a port with SSL enabled or disabled. - if found_matching_port: - # make sure we don't have an 'ssl on' directive - return not self.parser.has_ssl_on_directive(vhost) - return False + :param `obj.VirtualHost` vhost: The vhost whose addresses will be tested + :param port str: The port number as a string that the address should be bound to + :param bool ssl: Whether SSL should be enabled or disabled on the address + + :returns: Whether the vhost has an address listening on the port and protocol. + :rtype: bool + + """ + assert self.parser is not None # prepare should already have been called here + + # if the 'ssl on' directive is present on the vhost, all its addresses have SSL enabled + all_addrs_are_ssl = self.parser.has_ssl_on_directive(vhost) + + # if we want ssl vhosts: either 'ssl on' or 'addr.ssl' should be enabled + # if we want plaintext vhosts: neither 'ssl on' nor 'addr.ssl' should be enabled + _ssl_matches = lambda addr: addr.ssl or all_addrs_are_ssl if ssl else \ + not addr.ssl and not all_addrs_are_ssl + + # if there are no listen directives at all, Nginx defaults to + # listening on port 80. + if not vhost.addrs: + return port == self.DEFAULT_LISTEN_PORT and ssl == all_addrs_are_ssl + + return any(self._port_matches(port, addr.get_port()) and _ssl_matches(addr) + for addr in vhost.addrs) + + def _vhost_listening_on_port_no_ssl(self, vhost: obj.VirtualHost, port: str) -> bool: + return self._vhost_listening(vhost, port, False) def _get_redirect_ranked_matches(self, target_name, port): """Gets a ranked list of plaintextish port-listening vhosts matching target_name diff --git a/certbot-nginx/certbot_nginx/_internal/http_01.py b/certbot-nginx/certbot_nginx/_internal/http_01.py index 40f994988..896760fc4 100644 --- a/certbot-nginx/certbot_nginx/_internal/http_01.py +++ b/certbot-nginx/certbot_nginx/_internal/http_01.py @@ -5,6 +5,8 @@ import logging from acme import challenges from acme.magic_typing import List +from acme.magic_typing import Optional +from certbot import achallenges from certbot import errors from certbot.compat import os from certbot.plugins import common @@ -138,13 +140,12 @@ class NginxHttp01(common.ChallengePerformer): def _get_validation_path(self, achall): return os.sep + os.path.join(challenges.HTTP01.URI_ROOT_PATH, achall.chall.encode("token")) - def _make_server_block(self, achall): + def _make_server_block(self, achall: achallenges.KeyAuthorizationAnnotatedChallenge) -> List: """Creates a server block for a challenge. + :param achall: Annotated HTTP-01 challenge - :type achall: - :class:`certbot.achallenges.KeyAuthorizationAnnotatedChallenge` - :param list addrs: addresses of challenged domain - :class:`list` of type :class:`~nginx.obj.Addr` + :type achall: :class:`certbot.achallenges.KeyAuthorizationAnnotatedChallenge` + :returns: server block for the challenge host :rtype: list """ @@ -172,34 +173,35 @@ class NginxHttp01(common.ChallengePerformer): return location_directive - def _make_or_mod_server_block(self, achall): - """Modifies a server block to respond to a challenge. + def _make_or_mod_server_block(self, achall: achallenges.KeyAuthorizationAnnotatedChallenge + ) -> Optional[List]: + """Modifies server blocks to respond to a challenge. Returns a new HTTP server block + to add to the configuration if an existing one can't be found. :param achall: Annotated HTTP-01 challenge - :type achall: - :class:`certbot.achallenges.KeyAuthorizationAnnotatedChallenge` + :type achall: :class:`certbot.achallenges.KeyAuthorizationAnnotatedChallenge` + + :returns: new server block to be added, if any + :rtype: list """ - try: - vhosts = self.configurator.choose_redirect_vhosts(achall.domain, - '%i' % self.configurator.config.http01_port, create_if_no_match=True) - except errors.MisconfigurationError: + http_vhosts, https_vhosts = self.configurator.choose_auth_vhosts(achall.domain) + + new_vhost: Optional[list] = None + if not http_vhosts: # Couldn't find either a matching name+port server block # or a port+default_server block, so create a dummy block - return self._make_server_block(achall) + new_vhost = self._make_server_block(achall) - # len is max 1 because Nginx doesn't authenticate wildcards - # if len were or vhosts None, we would have errored - vhost = vhosts[0] + # Modify any existing server blocks + for vhost in set(http_vhosts + https_vhosts): + location_directive = [self._location_directive_for_achall(achall)] - # Modify existing server block - location_directive = [self._location_directive_for_achall(achall)] + self.configurator.parser.add_server_directives(vhost, location_directive) - self.configurator.parser.add_server_directives(vhost, - location_directive) + rewrite_directive = [['rewrite', ' ', '^(/.well-known/acme-challenge/.*)', + ' ', '$1', ' ', 'break']] + self.configurator.parser.add_server_directives(vhost, + rewrite_directive, insert_at_top=True) - rewrite_directive = [['rewrite', ' ', '^(/.well-known/acme-challenge/.*)', - ' ', '$1', ' ', 'break']] - self.configurator.parser.add_server_directives(vhost, - rewrite_directive, insert_at_top=True) - return None + return new_vhost diff --git a/certbot-nginx/certbot_nginx/_internal/parser.py b/certbot-nginx/certbot_nginx/_internal/parser.py index 17a8c652a..db9530104 100644 --- a/certbot-nginx/certbot_nginx/_internal/parser.py +++ b/certbot-nginx/certbot_nginx/_internal/parser.py @@ -10,6 +10,7 @@ import pyparsing from acme.magic_typing import Dict from acme.magic_typing import List +from acme.magic_typing import Optional from acme.magic_typing import Set from acme.magic_typing import Tuple from acme.magic_typing import Union @@ -361,8 +362,9 @@ class NginxParser: except errors.MisconfigurationError as err: raise errors.MisconfigurationError("Problem in %s: %s" % (filename, str(err))) - def duplicate_vhost(self, vhost_template, remove_singleton_listen_params=False, - only_directives=None): + def duplicate_vhost(self, vhost_template: obj.VirtualHost, + remove_singleton_listen_params: bool = False, + only_directives: Optional[List] = None) -> obj.VirtualHost: """Duplicate the vhost in the configuration files. :param :class:`~certbot_nginx._internal.obj.VirtualHost` vhost_template: The vhost diff --git a/certbot-nginx/tests/configurator_test.py b/certbot-nginx/tests/configurator_test.py index 5e788e394..9ccc3fc9e 100644 --- a/certbot-nginx/tests/configurator_test.py +++ b/certbot-nginx/tests/configurator_test.py @@ -39,7 +39,7 @@ class NginxConfiguratorTest(util.NginxTest): def test_prepare(self): self.assertEqual((1, 6, 2), self.config.version) - self.assertEqual(12, len(self.config.parser.parsed)) + self.assertEqual(13, len(self.config.parser.parsed)) @mock.patch("certbot_nginx._internal.configurator.util.exe_exists") @mock.patch("certbot_nginx._internal.configurator.subprocess.Popen") @@ -89,7 +89,7 @@ class NginxConfiguratorTest(util.NginxTest): "155.225.50.69.nephoscale.net", "www.example.org", "another.alias", "migration.com", "summer.com", "geese.com", "sslon.com", "globalssl.com", "globalsslsetssl.com", "ipv6.com", "ipv6ssl.com", - "headers.com", "example.net"}) + "headers.com", "example.net", "ssl.both.com"}) def test_supported_enhancements(self): self.assertEqual(['redirect', 'ensure-http-header', 'staple-ocsp'], @@ -935,7 +935,19 @@ class NginxConfiguratorTest(util.NginxTest): prefer_ssl=False, no_ssl_filter_port='80') # Check that the dialog was called with only port 80 vhosts - self.assertEqual(len(mock_select_vhs.call_args[0][0]), 6) + self.assertEqual(len(mock_select_vhs.call_args[0][0]), 8) + + def test_choose_auth_vhosts(self): + """choose_auth_vhosts correctly selects duplicative and HTTP/HTTPS vhosts""" + http, https = self.config.choose_auth_vhosts('ssl.both.com') + self.assertEqual(len(http), 4) + self.assertEqual(len(https), 2) + self.assertEqual(http[0].names, {'ssl.both.com'}) + self.assertEqual(http[1].names, {'ssl.both.com'}) + self.assertEqual(http[2].names, {'ssl.both.com'}) + self.assertEqual(http[3].names, {'*.both.com'}) + self.assertEqual(https[0].names, {'ssl.both.com'}) + self.assertEqual(https[1].names, {'*.both.com'}) class InstallSslOptionsConfTest(util.NginxTest): diff --git a/certbot-nginx/tests/http_01_test.py b/certbot-nginx/tests/http_01_test.py index 2947b099d..d0e84fc83 100644 --- a/certbot-nginx/tests/http_01_test.py +++ b/certbot-nginx/tests/http_01_test.py @@ -44,6 +44,10 @@ class HttpPerformTest(util.NginxTest): challb=acme_util.chall_to_challb( challenges.HTTP01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"), domain="migration.com", account_key=account_key), + achallenges.KeyAuthorizationAnnotatedChallenge( + challb=acme_util.chall_to_challb( + challenges.HTTP01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"), + domain="ipv6ssl.com", account_key=account_key), ] def setUp(self): @@ -77,8 +81,8 @@ class HttpPerformTest(util.NginxTest): http_responses = self.http01.perform() - self.assertEqual(len(http_responses), 4) - for i in range(4): + self.assertEqual(len(http_responses), 5) + for i in range(5): self.assertEqual(http_responses[i], acme_responses[i]) def test_mod_config(self): @@ -105,6 +109,43 @@ class HttpPerformTest(util.NginxTest): # self.assertEqual(vhost.addrs, set(v_addr2_print)) # self.assertEqual(vhost.names, set([response.z_domain.decode('ascii')])) + @mock.patch('certbot_nginx._internal.parser.NginxParser.add_server_directives') + def test_mod_config_http_and_https(self, mock_add_server_directives): + """A server_name with both HTTP and HTTPS vhosts should get modded in both vhosts""" + self.configuration.https_port = 443 + self.http01.add_chall(self.achalls[3]) # migration.com + self.http01._mod_config() # pylint: disable=protected-access + + # Domain has an HTTP and HTTPS vhost + # 2 * 'rewrite' + 2 * 'return 200 keyauthz' = 4 + self.assertEqual(mock_add_server_directives.call_count, 4) + + @mock.patch('certbot_nginx._internal.parser.nginxparser.dump') + @mock.patch('certbot_nginx._internal.parser.NginxParser.add_server_directives') + def test_mod_config_only_https(self, mock_add_server_directives, mock_dump): + """A server_name with only an HTTPS vhost should get modded""" + self.http01.add_chall(self.achalls[4]) # ipv6ssl.com + self.http01._mod_config() # pylint: disable=protected-access + + # It should modify the existing HTTPS vhost + self.assertEqual(mock_add_server_directives.call_count, 2) + # since there was no suitable HTTP vhost or default HTTP vhost, a non-empty one + # should have been created and written to the challenge conf file + self.assertNotEqual(mock_dump.call_args[0][0], []) + + @mock.patch('certbot_nginx._internal.parser.NginxParser.add_server_directives') + def test_mod_config_deduplicate(self, mock_add_server_directives): + """A vhost that appears in both HTTP and HTTPS vhosts only gets modded once""" + achall = achallenges.KeyAuthorizationAnnotatedChallenge( + challb=acme_util.chall_to_challb( + challenges.HTTP01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"), + domain="ssl.both.com", account_key=AUTH_KEY) + self.http01.add_chall(achall) + self.http01._mod_config() # pylint: disable=protected-access + + # Should only get called 5 times, rather than 6, because two vhosts are the same + self.assertEqual(mock_add_server_directives.call_count, 5*2) + @mock.patch("certbot_nginx._internal.configurator.NginxConfigurator.ipv6_info") def test_default_listen_addresses_no_memoization(self, ipv6_info): # pylint: disable=protected-access diff --git a/certbot-nginx/tests/parser_test.py b/certbot-nginx/tests/parser_test.py index 0083c2448..23fe390ad 100644 --- a/certbot-nginx/tests/parser_test.py +++ b/certbot-nginx/tests/parser_test.py @@ -51,6 +51,7 @@ class NginxParserTest(util.NginxTest): self.assertEqual({nparser.abs_path(x) for x in ['foo.conf', 'nginx.conf', 'server.conf', 'sites-enabled/default', + 'sites-enabled/both.com', 'sites-enabled/example.com', 'sites-enabled/headers.com', 'sites-enabled/migration.com', @@ -88,7 +89,7 @@ class NginxParserTest(util.NginxTest): parsed = nparser._parse_files(nparser.abs_path( 'sites-enabled/example.com.test')) self.assertEqual(3, len(glob.glob(nparser.abs_path('*.test')))) - self.assertEqual(9, len( + self.assertEqual(10, len( glob.glob(nparser.abs_path('sites-enabled/*.test')))) self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], @@ -171,7 +172,7 @@ class NginxParserTest(util.NginxTest): '*.www.example.com'}, [], [2, 1, 0]) - self.assertEqual(14, len(vhosts)) + self.assertEqual(19, len(vhosts)) example_com = [x for x in vhosts if 'example.com' in x.filep][0] self.assertEqual(vhost3, example_com) default = [x for x in vhosts if 'default' in x.filep][0] diff --git a/certbot-nginx/tests/testdata/etc_nginx/sites-enabled/both.com b/certbot-nginx/tests/testdata/etc_nginx/sites-enabled/both.com new file mode 100644 index 000000000..23a660df3 --- /dev/null +++ b/certbot-nginx/tests/testdata/etc_nginx/sites-enabled/both.com @@ -0,0 +1,32 @@ +server { + server_name ssl.both.com; +} + +# a duplicate vhost +server { + server_name ssl.both.com; +} + +# a duplicate by means of wildcard +server { + server_name *.both.com; +} + +# combined HTTP and HTTPS +server { + server_name ssl.both.com; + listen 80; + listen 5001 ssl; + + ssl_certificate cert.pem; + ssl_certificate_key cert.key; +} + +# HTTPS, duplicate by means of wildcard +server { + server_name *.both.com; + listen 5001 ssl; + + ssl_certificate cert.pem; + ssl_certificate_key cert.key; +} diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 1067c3b92..ea9c35e1d 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -21,6 +21,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). Python 2. * Certbot and all of its components no longer depend on the library `six`. * The update of certbot-auto itself is now disabled on all RHEL-like systems. +* The nginx authenticator now configures all matching HTTP and HTTPS vhosts for the HTTP-01 + challenge. It is now compatible with external HTTPS redirection by a CDN or load balancer. ### Fixed