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/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]:
+    def _make_server_ssl(self, vhost: obj.VirtualHost, key_path: str,
-        """Generate invalid certs that let us create ssl directives for Nginx"""
+                         fullchain_path: str) -> None:
-        # 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:
         """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', ' ', fullchain_path],
-            ['\n    ', 'ssl_certificate_key', ' ', snakeoil_key],
+            ['\n    ', 'ssl_certificate_key', ' ', key_path],
             ['\n    ', 'include', ' ', self.mod_ssl_conf],
             ['\n    ', 'ssl_dhparam', ' ', self.ssl_dhparams],
         ])

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(
+        vhost = self.config.choose_or_make_vhosts("ipv6.com", "key.pem", "fullchain.pem")[0]
-            "ipv6.com",
+        for addr in vhost.addrs:
-            "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:
             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

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.