diff --git a/certbot-nginx/src/certbot_nginx/_internal/configurator.py b/certbot-nginx/src/certbot_nginx/_internal/configurator.py index a89351172..0155ff0d1 100644 --- a/certbot-nginx/src/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/src/certbot_nginx/_internal/configurator.py @@ -254,7 +254,7 @@ class NginxConfigurator(common.Configurator): "The nginx plugin currently requires --fullchain-path to " "install a certificate.") - vhosts = self.choose_vhosts(domain, create_if_no_match=True) + vhosts = self.choose_or_make_vhosts(domain, create_if_no_match=True) for vhost in vhosts: self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path) display_util.notify("Successfully deployed certificate for {} to {}" @@ -337,9 +337,30 @@ class NginxConfigurator(common.Configurator): vhosts = [x for x in [self._select_best_name_match(matches)] if x is not None] return vhosts - def choose_vhosts(self, target_name: str, - create_if_no_match: bool = False) -> list[obj.VirtualHost]: - """Chooses a virtual host based on the given domain name. + def _choose_vhosts_common(self, target_name: str) -> list[obj.VirtualHost]: + if util.is_wildcard_domain(target_name): + # Ask user which VHosts to support. + return self._choose_vhosts_wildcard(target_name, prefer_ssl=True) + else: + return self._choose_vhost_single(target_name) + + def choose_vhosts(self, target_name: str) -> list[obj.VirtualHost]: + """Chooses SSL virtual hosts based on the given domain name. + + If no matching SSL vhosts are found, none are created and an empty list + is returned. + + :param str target_name: domain name + + :returns: ssl vhosts associated with name + :rtype: list of :class:`~certbot_nginx._internal.obj.VirtualHost` + + """ + return [vhost for vhost in self._choose_vhosts_common(target_name) if vhost.ssl] + + def choose_or_make_vhosts(self, target_name: str, create_if_no_match: bool + = False) -> list[obj.VirtualHost]: + """Chooses or creates SSL virtual hosts based on the given domain name. .. note:: This makes the vhost SSL-enabled if it isn't already. Follows Nginx's server block selection rules preferring blocks that are @@ -357,11 +378,7 @@ class NginxConfigurator(common.Configurator): :rtype: list of :class:`~certbot_nginx._internal.obj.VirtualHost` """ - if util.is_wildcard_domain(target_name): - # Ask user which VHosts to support. - vhosts = self._choose_vhosts_wildcard(target_name, prefer_ssl=True) - else: - vhosts = self._choose_vhost_single(target_name) + vhosts = self._choose_vhosts_common(target_name) if not vhosts: if create_if_no_match: # result will not be [None] because it errors on failure @@ -375,7 +392,6 @@ class NginxConfigurator(common.Configurator): "please add a corresponding server_name directive to your " "nginx configuration for every domain on your certificate: " "https://nginx.org/en/docs/http/server_names.html") % (target_name)) - # Note: if we are enhancing with ocsp, vhost should already be ssl. for vhost in vhosts: if not vhost.ssl: self._make_server_ssl(vhost) @@ -989,6 +1005,9 @@ class NginxConfigurator(common.Configurator): raise errors.NotSupportedError(f"Invalid chain_path type {type(chain_path)}, " "expected a str or None.") vhosts = self.choose_vhosts(domain) + if not vhosts: + raise errors.PluginError( + "Unable to find corresponding HTTPS host for OCSP stapling.") for vhost in vhosts: self._enable_ocsp_stapling_single(vhost, chain_path) diff --git a/certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py b/certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py index 9368fa64f..07a712fc8 100644 --- a/certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py +++ b/certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py @@ -42,7 +42,7 @@ class NginxConfiguratorTest(util.NginxTest): def test_prepare(self): assert (1, 6, 2) == self.config.version - assert 15 == len(self.config.parser.parsed) + assert 16 == len(self.config.parser.parsed) @mock.patch("certbot_nginx._internal.configurator.util.exe_exists") @mock.patch("certbot_nginx._internal.configurator.subprocess.run") @@ -100,7 +100,8 @@ 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", "ssl.both.com", 'addr-80.com'} + "headers.com", "example.net", "ssl.both.com", "no-listens.com", + "addr-80.com"} def test_supported_enhancements(self): assert ['redirect', 'ensure-http-header', 'staple-ocsp'] == \ @@ -136,34 +137,34 @@ class NginxConfiguratorTest(util.NginxTest): ['#', parser.COMMENT]]]] == \ parsed[filep] - def test_choose_vhosts_alias(self): - self._test_choose_vhosts_common('alias', 'server_conf') + def test_choose_or_make_vhosts_alias(self): + self._test_choose_or_make_vhosts_common('alias', 'server_conf') - def test_choose_vhosts_example_com(self): - self._test_choose_vhosts_common('example.com', 'example_conf') + def test_choose_or_make_vhosts_example_com(self): + self._test_choose_or_make_vhosts_common('example.com', 'example_conf') - def test_choose_vhosts_localhost(self): - self._test_choose_vhosts_common('localhost', 'localhost_conf') + def test_choose_or_make_vhosts_localhost(self): + self._test_choose_or_make_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_or_make_vhosts_example_com_uk_test(self): + self._test_choose_or_make_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_or_make_vhosts_www_example_com(self): + self._test_choose_or_make_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_or_make_vhosts_test_www_example_com(self): + self._test_choose_or_make_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_or_make_vhosts_abc_www_foo_com(self): + self._test_choose_or_make_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_or_make_vhosts_www_bar_co_uk(self): + self._test_choose_or_make_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_or_make_vhosts_ipv6_com(self): + self._test_choose_or_make_vhosts_common('ipv6.com', 'ipv6_conf') - def _test_choose_vhosts_common(self, name, conf): + def _test_choose_or_make_vhosts_common(self, name, conf): conf_names = {'localhost_conf': {'localhost', r'~^(www\.)?(example|bar)\.'}, 'server_conf': {'somename', 'another.alias', 'alias'}, 'example_conf': {'.example.com', 'example.*'}, @@ -181,7 +182,7 @@ class NginxConfiguratorTest(util.NginxTest): 'ipv6.com': "etc_nginx/sites-enabled/ipv6.com"} conf_path = {key: os.path.normpath(value) for key, value in conf_path.items()} - vhost = self.config.choose_vhosts(name)[0] + vhost = self.config.choose_or_make_vhosts(name)[0] path = os.path.relpath(vhost.filep, self.temp_dir) assert conf_names[conf] == vhost.names @@ -192,38 +193,38 @@ class NginxConfiguratorTest(util.NginxTest): # Make sure that we have SSL enabled also for IPv6 addr assert any(True for x in vhost.addrs if x.ssl and x.ipv6) - def test_choose_vhosts_bad(self): + def test_choose_or_make_vhosts_bad(self): bad_results = ['www.foo.com', 'example', 't.www.bar.co', '69.255.225.155'] for name in bad_results: with self.subTest(name=name): with pytest.raises(errors.MisconfigurationError): - self.config.choose_vhosts(name) + self.config.choose_or_make_vhosts(name) - def test_choose_vhosts_keep_ip_address(self): + def test_choose_or_make_vhosts_keep_ip_address(self): # no listen on port 80 # listen 69.50.225.155:9000; # listen 127.0.0.1; - vhost = self.config.choose_vhosts('example.com')[0] + vhost = self.config.choose_or_make_vhosts('example.com')[0] assert obj.Addr.fromstring("5001 ssl") in vhost.addrs # no listens at all - vhost = self.config.choose_vhosts('headers.com')[0] + vhost = self.config.choose_or_make_vhosts('no-listens.com')[0] assert obj.Addr.fromstring("5001 ssl") in vhost.addrs assert obj.Addr.fromstring("80") in vhost.addrs # blank addr listen on 80 should result in blank addr ssl # listen 80; # listen [::]:80; - vhost = self.config.choose_vhosts('ipv6.com')[0] + vhost = self.config.choose_or_make_vhosts('ipv6.com')[0] assert obj.Addr.fromstring("5001 ssl") in vhost.addrs assert obj.Addr.fromstring("[::]:5001 ssl") in vhost.addrs # listen on 80 with ip address should result in copied addr # listen 1.2.3.4:80; # listen [1:20::300]:80; - vhost = self.config.choose_vhosts('addr-80.com')[0] + vhost = self.config.choose_or_make_vhosts('addr-80.com')[0] assert obj.Addr.fromstring("1.2.3.4:5001 ssl") in vhost.addrs assert obj.Addr.fromstring("[1:20::300]:5001 ssl ipv6only=on") in vhost.addrs @@ -244,7 +245,7 @@ class NginxConfiguratorTest(util.NginxTest): "example/chain.pem", "example/fullchain.pem") - for addr in self.config.choose_vhosts("ipv6.com")[0].addrs: + for addr in self.config.choose_or_make_vhosts("ipv6.com")[0].addrs: assert not addr.ipv6only def test_more_info(self): @@ -661,14 +662,16 @@ class NginxConfiguratorTest(util.NginxTest): generated_conf def test_http_header_hsts(self): - example_conf = self.config.parser.abs_path('sites-enabled/example.com') - self.config.enhance("www.example.com", "ensure-http-header", + # we need to select an SSL enabled vhost here for the HSTS enhancement + example_conf = self.config.parser.abs_path('sites-enabled/migration.com') + self.config.enhance("migration.com", "ensure-http-header", "Strict-Transport-Security") expected = ['add_header', 'Strict-Transport-Security', '"max-age=31536000"', 'always'] generated_conf = self.config.parser.parsed[example_conf] assert util.contains_at_depth(generated_conf, expected, 2) is True def test_multiple_headers_hsts(self): + # we need to select an SSL enabled vhost here for the HSTS enhancement headers_conf = self.config.parser.abs_path('sites-enabled/headers.com') self.config.enhance("headers.com", "ensure-http-header", "Strict-Transport-Security") @@ -677,10 +680,11 @@ class NginxConfiguratorTest(util.NginxTest): assert util.contains_at_depth(generated_conf, expected, 2) is True def test_http_header_hsts_twice(self): - self.config.enhance("www.example.com", "ensure-http-header", + # we need to select an SSL enabled vhost here for the HSTS enhancement + self.config.enhance("migration.com", "ensure-http-header", "Strict-Transport-Security") with pytest.raises(errors.PluginEnhancementAlreadyPresent): - self.config.enhance("www.example.com", + self.config.enhance("migration.com", "ensure-http-header", "Strict-Transport-Security") @mock.patch('certbot_nginx._internal.obj.VirtualHost.contains_list') @@ -724,16 +728,20 @@ class NginxConfiguratorTest(util.NginxTest): self.config.enhance("www.example.com", "staple-ocsp", None) def test_staple_ocsp_internal_error(self): - self.config.enhance("www.example.com", "staple-ocsp", "chain_path") + # we need to select an SSL enabled vhost here for the OCSP stapling + # enhancement + self.config.enhance("sslon.com", "staple-ocsp", "chain_path") # error is raised because the server block has conflicting directives with pytest.raises(errors.PluginError): - self.config.enhance("www.example.com", "staple-ocsp", "different_path") + self.config.enhance("sslon.com", "staple-ocsp", "different_path") def test_staple_ocsp(self): chain_path = "example/chain.pem" - self.config.enhance("www.example.com", "staple-ocsp", chain_path) + # we need to select an SSL enabled vhost here for the OCSP stapling + # enhancement + self.config.enhance("sslon.com", "staple-ocsp", chain_path) - example_conf = self.config.parser.abs_path('sites-enabled/example.com') + example_conf = self.config.parser.abs_path('sites-enabled/sslon.com') generated_conf = self.config.parser.parsed[example_conf] assert util.contains_at_depth( @@ -960,8 +968,10 @@ class NginxConfiguratorTest(util.NginxTest): @mock.patch("certbot_nginx._internal.display_ops.select_vhost_multiple") def test_enhance_wildcard_redirect_or_ocsp_no_install(self, mock_dialog): + # we need to select an SSL enabled vhost here for the OCSP stapling + # enhancement vhost = [x for x in self.config.parser.get_vhosts() - if 'summer.com' in x.names][0] + if 'geese.com' in x.names][0] mock_dialog.return_value = [vhost] self.config.enhance("*.com", "staple-ocsp", "example/chain.pem") assert mock_dialog.called is True diff --git a/certbot-nginx/src/certbot_nginx/_internal/tests/parser_test.py b/certbot-nginx/src/certbot_nginx/_internal/tests/parser_test.py index 52bc4456d..1e398717c 100644 --- a/certbot-nginx/src/certbot_nginx/_internal/tests/parser_test.py +++ b/certbot-nginx/src/certbot_nginx/_internal/tests/parser_test.py @@ -62,6 +62,7 @@ class NginxParserTest(util.NginxTest): 'sites-enabled/ipv6.com', 'sites-enabled/ipv6ssl.com', 'sites-enabled/example.net', + 'sites-enabled/no-listens.com', 'sites-enabled/addr-80.com']} == \ set(nparser.parsed.keys()) assert [['server_name', 'somename', 'alias', 'another.alias']] == \ @@ -101,7 +102,7 @@ class NginxParserTest(util.NginxTest): parsed = nparser._parse_files(nparser.abs_path( 'sites-enabled/example.com.test')) assert 4 == len(glob.glob(nparser.abs_path('*.test'))) - assert 11 == len( + assert 12 == len( glob.glob(nparser.abs_path('sites-enabled/*.test'))) assert [[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], @@ -184,7 +185,7 @@ class NginxParserTest(util.NginxTest): '*.www.example.com'}, [], [2, 1, 0]) - assert 20 == len(vhosts) + assert 21 == len(vhosts) example_com = [x for x in vhosts if 'example.com' in x.filep][0] assert vhost3 == example_com default = [x for x in vhosts if 'default' in x.filep][0] diff --git a/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/headers.com b/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/headers.com index 6c032928c..1489a37be 100644 --- a/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/headers.com +++ b/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/headers.com @@ -1,4 +1,7 @@ server { server_name headers.com; add_header X-Content-Type-Options nosniff; + ssl on; + ssl_certificate snakeoil.cert; + ssl_certificate_key snakeoil.key; } diff --git a/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/no-listens.com b/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/no-listens.com new file mode 100644 index 000000000..5081455bd --- /dev/null +++ b/certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/no-listens.com @@ -0,0 +1,3 @@ +server { + server_name no-listens.com; +} diff --git a/newsfragments/10455.fixed b/newsfragments/10455.fixed new file mode 100644 index 000000000..96e1bfa59 --- /dev/null +++ b/newsfragments/10455.fixed @@ -0,0 +1 @@ +Fixed a bug in certbot-nginx that'd leave nginx configured with self-signed certificates if a user ran `certbot enhance` and they didn't have matching SSL server blocks. `certbot enhance` now requires the user to have a matching SSL server block to enable HSTS or OCSP stapling enhancements.