From 513190afe0fa27f09e1b7ca783a6d9a0d07dca22 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 25 Sep 2025 12:41:47 -0700 Subject: [PATCH] don't use snakeoil certs in nginx (#10465) this PR finally removes all uses of self-signed certificates from certbot-nginx i plan to create one last PR related to this deprecating `acme.crypto_util.make_self_signed_cert` and moving the function to certbot-compatibility-test which is the only place it's currently used i think we could also do additional refactoring in certbot-nginx by moving the _make_server_ssl call out of choose_or_make_vhost and make deploy_cert responsible for calling it if the returned vhosts aren't ssl. in this case, we could then skip updating cert and key directives a second time as this is duplicate work if we just made the server ssl i considered doing this, but it's a bigger refactor, breaks more tests, and i'm not sure it really buys us much so i skipped it. i could take this on or create an issue for it if you think it's important for us to do for some reason tho ohemorange --- .../certbot_nginx/_internal/configurator.py | 52 +++++-------------- .../_internal/tests/configurator_test.py | 52 +++++++++---------- newsfragments/10465.changed | 1 + 3 files changed, 38 insertions(+), 67 deletions(-) create mode 100644 newsfragments/10465.changed diff --git a/certbot-nginx/src/certbot_nginx/_internal/configurator.py b/certbot-nginx/src/certbot_nginx/_internal/configurator.py index 89e9fb94c..4d0da638e 100644 --- a/certbot-nginx/src/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/src/certbot_nginx/_internal/configurator.py @@ -18,13 +18,8 @@ from typing import Sequence from typing import Union from typing import cast -from cryptography.hazmat.primitives import serialization -from cryptography.hazmat.primitives.asymmetric import rsa - from acme import challenges -from acme import crypto_util as acme_crypto_util from certbot import achallenges -from certbot import crypto_util from certbot import errors from certbot import util from certbot.compat import os @@ -254,7 +249,7 @@ class NginxConfigurator(common.Configurator): "The nginx plugin currently requires --fullchain-path to " "install a certificate.") - vhosts = self.choose_or_make_vhosts(domain) + vhosts = self.choose_or_make_vhosts(domain, key_path, fullchain_path) for vhost in vhosts: self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path) display_util.notify("Successfully deployed certificate for {} to {}" @@ -358,7 +353,8 @@ class NginxConfigurator(common.Configurator): """ return [vhost for vhost in self._choose_vhosts_common(target_name) if vhost.ssl] - def choose_or_make_vhosts(self, target_name: str) -> list[obj.VirtualHost]: + def choose_or_make_vhosts(self, target_name: str, key_path: str, + fullchain_path: str) -> list[obj.VirtualHost]: """Chooses or creates SSL virtual hosts based on the given domain name. If no matching vhost is found, we attempt to create a new one from the @@ -369,6 +365,8 @@ class NginxConfigurator(common.Configurator): already SSL. :param str target_name: domain name + :param str key_path: key to use when creating SSL vhosts + :param str fullchain_path: certificates to use when creating SSL vhosts :returns: ssl vhosts associated with name :rtype: list of :class:`~certbot_nginx._internal.obj.VirtualHost` @@ -381,7 +379,7 @@ class NginxConfigurator(common.Configurator): str(self.config.https_port))] for vhost in vhosts: if not vhost.ssl: - self._make_server_ssl(vhost) + self._make_server_ssl(vhost, key_path, fullchain_path) return vhosts @@ -707,40 +705,16 @@ class NginxConfigurator(common.Configurator): return util.get_filtered_names(all_names) - def _get_snakeoil_paths(self) -> tuple[str, str]: - """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.generate_key( - key_type='rsa', key_size=2048, key_dir=tmp_dir, keyname="key.pem", - strict_permissions=self.config.strict_permissions) - assert le_key.file is not None - cryptography_key = serialization.load_pem_private_key(le_key.pem, password=None) - assert isinstance(cryptography_key, rsa.RSAPrivateKey) - cert = acme_crypto_util.make_self_signed_cert( - cryptography_key, - # we used to use socket.gethostname here, but that sometimes - # resulted in strings over 64 characters long which would error - # on the validation introduced in - # https://github.com/pyca/cryptography/pull/11201. the ".invalid" - # TLD comes from RFC2606 (and was also used in the tls-sni-01 - # challenge in early versions of the ACME spec) - domains=['temp-certbot-nginx.invalid'] - ) - cert_pem = cert.public_bytes(serialization.Encoding.PEM) - cert_file, cert_path = util.unique_file( - os.path.join(tmp_dir, "cert.pem"), mode="wb") - with cert_file: - cert_file.write(cert_pem) - return cert_path, le_key.file - - def _make_server_ssl(self, vhost: obj.VirtualHost) -> None: + def _make_server_ssl(self, vhost: obj.VirtualHost, key_path: str, + fullchain_path: str) -> None: """Make a server SSL. Make a server SSL by adding new listen and SSL directives. :param vhost: The vhost to add SSL to. :type vhost: :class:`~certbot_nginx._internal.obj.VirtualHost` + :param str key_path: key to use for SSL + :param str fullchain_path: certificates to use for SSL """ https_port = self.config.https_port @@ -797,12 +771,10 @@ class NginxConfigurator(common.Configurator): 'ssl'] addr_blocks.append(addr_block) - snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() - ssl_block = ([ *addr_blocks, - ['\n ', 'ssl_certificate', ' ', snakeoil_cert], - ['\n ', 'ssl_certificate_key', ' ', snakeoil_key], + ['\n ', 'ssl_certificate', ' ', fullchain_path], + ['\n ', 'ssl_certificate_key', ' ', key_path], ['\n ', 'include', ' ', self.mod_ssl_conf], ['\n ', 'ssl_dhparam', ' ', self.ssl_dhparams], ]) 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 07a712fc8..afa4a873d 100644 --- a/certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py +++ b/certbot-nginx/src/certbot_nginx/_internal/tests/configurator_test.py @@ -4,9 +4,6 @@ from unittest import mock import pytest -from cryptography import x509 -from cryptography.hazmat.primitives import serialization - from acme import challenges from acme import messages from certbot import achallenges @@ -182,7 +179,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_or_make_vhosts(name)[0] + vhost = self.config.choose_or_make_vhosts(name, 'key.pem', 'fullchain.pem')[0] path = os.path.relpath(vhost.filep, self.temp_dir) assert conf_names[conf] == vhost.names @@ -200,34 +197,51 @@ class NginxConfiguratorTest(util.NginxTest): for name in bad_results: with self.subTest(name=name): with pytest.raises(errors.MisconfigurationError): - self.config.choose_or_make_vhosts(name) + self.config.choose_or_make_vhosts(name, 'key.pem', 'fullchain.pem') def test_choose_or_make_vhosts_keep_ip_address(self): + # let's use a simple helper function to set key and fullchain values + def choose_or_make_vhosts(domain): + return self.config.choose_or_make_vhosts(domain, 'key.pem', 'fullchain.pem') + # no listen on port 80 # listen 69.50.225.155:9000; # listen 127.0.0.1; - vhost = self.config.choose_or_make_vhosts('example.com')[0] + vhost = choose_or_make_vhosts('example.com')[0] assert obj.Addr.fromstring("5001 ssl") in vhost.addrs # no listens at all - vhost = self.config.choose_or_make_vhosts('no-listens.com')[0] + vhost = 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_or_make_vhosts('ipv6.com')[0] + vhost = 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_or_make_vhosts('addr-80.com')[0] + vhost = 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 + def test_choose_or_make_vhost_ssl_directives(self): + conf_path = self.config.parser.abs_path('sites-enabled/example.com') + self.config.choose_or_make_vhosts('example.com', 'my-key.pem', 'my-fullchain.pem') + self.config.save() + self.config.parser.load() + parsed_conf = util.filter_comments(self.config.parser.parsed[conf_path]) + + expected_directives = [ + ['ssl_certificate', 'my-fullchain.pem'], + ['ssl_certificate_key', 'my-key.pem'], + ] + for directive in expected_directives: + assert util.contains_at_depth(parsed_conf, directive, 2) def test_ipv6only(self): # ipv6_info: (ipv6_active, ipv6only_present) @@ -238,14 +252,8 @@ class NginxConfiguratorTest(util.NginxTest): def test_ipv6only_detection(self): self.config.version = (1, 3, 1) - self.config.deploy_cert( - "ipv6.com", - "example/cert.pem", - "example/key.pem", - "example/chain.pem", - "example/fullchain.pem") - - for addr in self.config.choose_or_make_vhosts("ipv6.com")[0].addrs: + vhost = self.config.choose_or_make_vhosts("ipv6.com", "key.pem", "fullchain.pem")[0] + for addr in vhost.addrs: assert not addr.ipv6only def test_more_info(self): @@ -570,16 +578,6 @@ class NginxConfiguratorTest(util.NginxTest): with pytest.raises(errors.PluginError): self.config.save() - def test_get_snakeoil_paths(self): - # pylint: disable=protected-access - cert, key = self.config._get_snakeoil_paths() - assert os.path.exists(cert) - assert os.path.exists(key) - with open(cert, "rb") as cert_file: - x509.load_pem_x509_certificate(cert_file.read()) - with open(key, "rb") as key_file: - serialization.load_pem_private_key(key_file.read(), password=None) - def test_redirect_enhance(self): # Test that we successfully add a redirect when there is # a listen directive diff --git a/newsfragments/10465.changed b/newsfragments/10465.changed new file mode 100644 index 000000000..dc2531d47 --- /dev/null +++ b/newsfragments/10465.changed @@ -0,0 +1 @@ +certbot-nginx no longer creates and uses self-signed certificates as an intermediate step when installing certificates. The certificates the user requested Certbot install are now always used instead.