From deb3d035e3bc78e0dea8964075752ac38cb4a069 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 17 Sep 2025 14:23:32 -0700 Subject: [PATCH] require ssl vhosts for HSTS and OCSP stapling (#10455) this is the first part of the nginx refactoring work i wanted to do. ohemorange, if this conflicts with your work on updating our nginx ssl config, please feel free to either ignore this for now or point me to your branch after merging this and i can fix up any merge conflicts myself like i previously offered the main thing this PR does is create a new choose_or_make_vhosts function with the current choose_vhosts behavior and makes choose_vhosts only return existing ssl vhosts which i think is the behavior we want when setting up HSTS or OCSP stapling. [this is what we do in apache](https://github.com/certbot/certbot/blob/867b499f9b3934f88beffe53f021c6834350c2f1/certbot-apache/src/certbot_apache/_internal/configurator.py#L1795-L1829), enabling HSTS or OCSP stapling on an HTTP vhost seems wrong/dangerous, and since we don't have cert and key information in these [enhance calls](https://github.com/certbot/certbot/blob/867b499f9b3934f88beffe53f021c6834350c2f1/certbot/src/certbot/interfaces.py#L255), any SSL vhost we create will be left with snakeoil certs which also seems very wrong of course, this simple change to certbot-nginx's prod code required many changes to its tests. the config file for headers.com was introduced [here](https://github.com/certbot/certbot/pull/6068) specifically for testing HSTS so i added the SSL configuration it needs to make work with the choose_vhost changes. that then broke the IP tests for headers.com that were added in https://github.com/certbot/certbot/pull/10145/ so i created a new no-listens.com vhost for testing that if this is merged, my plan in the next PR or two is to: 1. since choose_or_make_vhosts is now always called with create_if_no_match=True in prod code, i plan to remove that variable, make that the default behavior of the function, and fix up tests 2. then, since choose_or_make_vhosts is only called in deploy_cert, i plan to pass the cert and key to it so it can be used in _make_server_ssl instead of the dummy certs currently being used there i could do more of this in this PR if you want ohemorange, but i figured it rarely hurts to break things up especially when the code is kind of tricky like it is (to me) here --- .../certbot_nginx/_internal/configurator.py | 39 +++++--- .../_internal/tests/configurator_test.py | 88 +++++++++++-------- .../_internal/tests/parser_test.py | 5 +- .../etc_nginx/sites-enabled/headers.com | 3 + .../etc_nginx/sites-enabled/no-listens.com | 3 + newsfragments/10455.fixed | 1 + 6 files changed, 88 insertions(+), 51 deletions(-) create mode 100644 certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/no-listens.com create mode 100644 newsfragments/10455.fixed 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.