upstream/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-05-28 04:34:11 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

require ssl vhosts for HSTS and OCSP stapling (#10455)

Browse source 
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](867b499f9b/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](867b499f9b/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
This commit is contained in:
Brad Warren 2025-09-17 14:23:32 -07:00 committed by GitHub
parent 867b499f9b
commit deb3d035e3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 88 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
certbot-nginx/src/certbot_nginx/_internal/configurator.py
View file

@ -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)

88
certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py
View file

@ -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

5
certbot-nginx/src/certbot_nginx/_internal/tests/parser_test.py
View file

@ -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]

3
certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/headers.com vendored
View file

@ -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;
}

3
certbot-nginx/src/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/no-listens.com vendored Normal file
View file

@ -0,0 +1,3 @@
server {
server_name no-listens.com;
}

1
newsfragments/10455.fixed Normal file
View file

@ -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.