Remove tls sni in nginx plugin (#6857)

* Remove tls-sni from nginx config

* Add a dedicated configuration to define what is the HTTPS port for this certbot instance.

* Correct some tests

* Reestablish default vhost creation

* Clean tls references for nginx integration tests

* Associate https_port only to tests and nginx

certbot-nginx/certbot_nginx/configurator.py

@@ -26,7 +26,6 @@ from certbot_nginx import constants
 from certbot_nginx import display_ops
 from certbot_nginx import nginxparser
 from certbot_nginx import parser
-from certbot_nginx import tls_sni_01
 from certbot_nginx import http_01
 from certbot_nginx import obj # pylint: disable=unused-import
 from acme.magic_typing import List, Dict, Set # pylint: disable=unused-import, no-name-in-module
@@ -149,7 +148,6 @@ class NginxConfigurator(common.Installer):
         # Make sure configuration is valid
         self.config_test()
 
-
         self.parser = parser.NginxParser(self.conf('server-root'))
 
         install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest)
@@ -611,7 +609,8 @@ class NginxConfigurator(common.Installer):
         :type vhost: :class:`~certbot_nginx.obj.VirtualHost`
 
         """
-        ipv6info = self.ipv6_info(self.config.tls_sni_01_port)
+        https_port = self.config.tls_sni_01_port
+        ipv6info = self.ipv6_info(https_port)
         ipv6_block = ['']
         ipv4_block = ['']
 
@@ -625,7 +624,7 @@ class NginxConfigurator(common.Installer):
             ipv6_block = ['\n    ',
                           'listen',
                           ' ',
-                          '[::]:{0}'.format(self.config.tls_sni_01_port),
+                          '[::]:{0}'.format(https_port),
                           ' ',
                           'ssl']
             if not ipv6info[1]:
@@ -637,7 +636,7 @@ class NginxConfigurator(common.Installer):
             ipv4_block = ['\n    ',
                           'listen',
                           ' ',
-                          '{0}'.format(self.config.tls_sni_01_port),
+                          '{0}'.format(https_port),
                           ' ',
                           'ssl']
 
@@ -799,8 +798,6 @@ class NginxConfigurator(common.Installer):
         :param str domain: domain to enable redirect for
         :param `~obj.Vhost` vhost: vhost to enable redirect for
         """
-
-        http_vhost = None
         if vhost.ssl:
             http_vhost, _ = self._split_block(vhost, ['listen', 'server_name'])
 
@@ -1051,19 +1048,14 @@ class NginxConfigurator(common.Installer):
         """
         self._chall_out += len(achalls)
         responses = [None] * len(achalls)
-        sni_doer = tls_sni_01.NginxTlsSni01(self)
         http_doer = http_01.NginxHttp01(self)
 
         for i, achall in enumerate(achalls):
             # Currently also have chall_doer hold associated index of the
             # challenge. This helps to put all of the responses back together
             # when they are all complete.
-            if isinstance(achall.chall, challenges.HTTP01):
             http_doer.add_chall(achall, i)
-            else:  # tls-sni-01
-                sni_doer.add_chall(achall, i)
 
-        sni_response = sni_doer.perform()
         http_response = http_doer.perform()
         # Must restart in order to activate the challenges.
         # Handled here because we may be able to load up other challenge types
@@ -1072,9 +1064,8 @@ class NginxConfigurator(common.Installer):
         # Go through all of the challenges and assign them to the proper place
         # in the responses return value. All responses must be in the same order
         # as the original challenges.
-        for chall_response, chall_doer in ((sni_response, sni_doer), (http_response, http_doer)):
+        for i, resp in enumerate(http_response):
-            for i, resp in enumerate(chall_response):
+            responses[http_doer.indices[i]] = resp
-                responses[chall_doer.indices[i]] = resp
 
         return responses
 
@@ -1152,6 +1143,7 @@ def install_ssl_options_conf(options_ssl, options_ssl_digest):
     return common.install_version_controlled_file(options_ssl, options_ssl_digest,
         constants.MOD_SSL_CONF_SRC, constants.ALL_SSL_OPTIONS_HASHES)
 
+
 def _determine_default_server_root():
     if os.environ.get("CERTBOT_DOCS") == "1":
         default_server_root = "%s or %s" % (constants.LINUX_SERVER_ROOT,

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -318,21 +318,13 @@ class NginxConfiguratorTest(util.NginxTest):
                            ]],
                          parsed_migration_conf[0])
 
-    @mock.patch("certbot_nginx.configurator.tls_sni_01.NginxTlsSni01.perform")
     @mock.patch("certbot_nginx.configurator.http_01.NginxHttp01.perform")
     @mock.patch("certbot_nginx.configurator.NginxConfigurator.restart")
     @mock.patch("certbot_nginx.configurator.NginxConfigurator.revert_challenge_config")
-    def test_perform_and_cleanup(self, mock_revert, mock_restart, mock_http_perform,
+    def test_perform_and_cleanup(self, mock_revert, mock_restart, mock_http_perform):
-        mock_tls_perform):
         # Only tests functionality specific to configurator.perform
         # Note: As more challenges are offered this will have to be expanded
-        achall1 = achallenges.KeyAuthorizationAnnotatedChallenge(
+        achall = achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=messages.ChallengeBody(
-                chall=challenges.TLSSNI01(token=b"kNdwjwOeX0I_A8DXt9Msmg"),
-                uri="https://ca.org/chall0_uri",
-                status=messages.Status("pending"),
-            ), domain="localhost", account_key=self.rsa512jwk)
-        achall2 = achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=messages.ChallengeBody(
                 chall=challenges.HTTP01(token=b"m8TdO1qik4JVFtgPPurJmg"),
                 uri="https://ca.org/chall1_uri",
@@ -340,19 +332,16 @@ class NginxConfiguratorTest(util.NginxTest):
             ), domain="example.com", account_key=self.rsa512jwk)
 
         expected = [
-            achall1.response(self.rsa512jwk),
+            achall.response(self.rsa512jwk),
-            achall2.response(self.rsa512jwk),
         ]
 
-        mock_tls_perform.return_value = expected[:1]
+        mock_http_perform.return_value = expected[:]
-        mock_http_perform.return_value = expected[1:]
+        responses = self.config.perform([achall])
-        responses = self.config.perform([achall1, achall2])
 
-        self.assertEqual(mock_tls_perform.call_count, 1)
         self.assertEqual(mock_http_perform.call_count, 1)
         self.assertEqual(responses, expected)
 
-        self.config.cleanup([achall1, achall2])
+        self.config.cleanup([achall])
         self.assertEqual(0, self.config._chall_out) # pylint: disable=protected-access
         self.assertEqual(mock_revert.call_count, 1)
         self.assertEqual(mock_restart.call_count, 2)

certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py

@@ -1,158 +0,0 @@
-"""Tests for certbot_nginx.tls_sni_01"""
-import unittest
-
-import mock
-import six
-
-from acme import challenges
-
-from certbot import achallenges
-from certbot import errors
-
-from certbot.plugins import common_test
-from certbot.tests import acme_util
-
-from certbot_nginx import obj
-from certbot_nginx.tests import util
-
-
-class TlsSniPerformTest(util.NginxTest):
-    """Test the NginxTlsSni01 challenge."""
-
-    account_key = common_test.AUTH_KEY
-    achalls = [
-        achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.chall_to_challb(
-                challenges.TLSSNI01(token=b"kNdwjwOeX0I_A8DXt9Msmg"), "pending"),
-            domain="www.example.com", account_key=account_key),
-        achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.chall_to_challb(
-                challenges.TLSSNI01(
-                    token=b"\xba\xa9\xda?<m\xaewmx\xea\xad\xadv\xf4\x02\xc9y"
-                          b"\x80\xe2_X\t\xe7\xc7\xa4\t\xca\xf7&\x945"
-                ), "pending"),
-            domain="another.alias", account_key=account_key),
-        achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.chall_to_challb(
-                challenges.TLSSNI01(
-                    token=b"\x8c\x8a\xbf_-f\\cw\xee\xd6\xf8/\xa5\xe3\xfd"
-                          b"\xeb9\xf1\xf5\xb9\xefVM\xc9w\xa4u\x9c\xe1\x87\xb4"
-                ), "pending"),
-            domain="www.example.org", account_key=account_key),
-        achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.chall_to_challb(
-                challenges.TLSSNI01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"),
-            domain="sslon.com", account_key=account_key),
-    ]
-
-    def setUp(self):
-        super(TlsSniPerformTest, self).setUp()
-
-        config = util.get_nginx_configurator(
-            self.config_path, self.config_dir, self.work_dir, self.logs_dir)
-
-        from certbot_nginx import tls_sni_01
-        self.sni = tls_sni_01.NginxTlsSni01(config)
-
-    @mock.patch("certbot_nginx.configurator"
-                ".NginxConfigurator.choose_vhosts")
-    def test_perform(self, mock_choose):
-        self.sni.add_chall(self.achalls[1])
-        mock_choose.return_value = []
-        result = self.sni.perform()
-        self.assertFalse(result is None)
-
-    def test_perform0(self):
-        responses = self.sni.perform()
-        self.assertEqual([], responses)
-
-    @mock.patch("certbot_nginx.configurator.NginxConfigurator.save")
-    def test_perform1(self, mock_save):
-        self.sni.add_chall(self.achalls[0])
-        response = self.achalls[0].response(self.account_key)
-        mock_setup_cert = mock.MagicMock(return_value=response)
-
-        # pylint: disable=protected-access
-        self.sni._setup_challenge_cert = mock_setup_cert
-
-        responses = self.sni.perform()
-
-        mock_setup_cert.assert_called_once_with(self.achalls[0])
-        self.assertEqual([response], responses)
-        self.assertEqual(mock_save.call_count, 1)
-
-        # Make sure challenge config is included in main config
-        http = self.sni.configurator.parser.parsed[
-            self.sni.configurator.parser.config_root][-1]
-        self.assertTrue(
-            util.contains_at_depth(http, ['include', self.sni.challenge_conf], 1))
-
-    def test_perform2(self):
-        acme_responses = []
-        for achall in self.achalls:
-            self.sni.add_chall(achall)
-            acme_responses.append(achall.response(self.account_key))
-
-        mock_setup_cert = mock.MagicMock(side_effect=acme_responses)
-        # pylint: disable=protected-access
-        self.sni._setup_challenge_cert = mock_setup_cert
-
-        sni_responses = self.sni.perform()
-
-        self.assertEqual(mock_setup_cert.call_count, 4)
-
-        for index, achall in enumerate(self.achalls):
-            self.assertEqual(
-                mock_setup_cert.call_args_list[index], mock.call(achall))
-
-        http = self.sni.configurator.parser.parsed[
-            self.sni.configurator.parser.config_root][-1]
-        self.assertTrue(['include', self.sni.challenge_conf] in http[1])
-        self.assertFalse(
-            util.contains_at_depth(http, ['server_name', 'another.alias'], 3))
-
-        self.assertEqual(len(sni_responses), 4)
-        for i in six.moves.range(4):
-            self.assertEqual(sni_responses[i], acme_responses[i])
-
-    def test_mod_config(self):
-        self.sni.add_chall(self.achalls[0])
-        self.sni.add_chall(self.achalls[2])
-
-        v_addr1 = [obj.Addr("69.50.225.155", "9000", True, False, False, False),
-                   obj.Addr("127.0.0.1", "", False, False, False, False)]
-        v_addr2 = [obj.Addr("myhost", "", False, True, False, False)]
-        v_addr2_print = [obj.Addr("myhost", "", False, False, False, False)]
-        ll_addr = [v_addr1, v_addr2]
-        self.sni._mod_config(ll_addr)  # pylint: disable=protected-access
-
-        self.sni.configurator.save()
-
-        self.sni.configurator.parser.load()
-
-        http = self.sni.configurator.parser.parsed[
-            self.sni.configurator.parser.config_root][-1]
-        self.assertTrue(['include', self.sni.challenge_conf] in http[1])
-
-        vhosts = self.sni.configurator.parser.get_vhosts()
-        vhs = [vh for vh in vhosts if vh.filep == self.sni.challenge_conf]
-
-        for vhost in vhs:
-            if vhost.addrs == set(v_addr1):
-                response = self.achalls[0].response(self.account_key)
-            else:
-                response = self.achalls[2].response(self.account_key)
-                self.assertEqual(vhost.addrs, set(v_addr2_print))
-            self.assertEqual(vhost.names, set([response.z_domain.decode('ascii')]))
-
-        self.assertEqual(len(vhs), 2)
-
-    def test_mod_config_fail(self):
-        root = self.sni.configurator.parser.config_root
-        self.sni.configurator.parser.parsed[root] = [['include', 'foo.conf']]
-        # pylint: disable=protected-access
-        self.assertRaises(
-            errors.MisconfigurationError, self.sni._mod_config, [])
-
-if __name__ == "__main__":
-    unittest.main()  # pragma: no cover

certbot-nginx/certbot_nginx/tests/util.py

@@ -81,8 +81,8 @@ def get_nginx_configurator(
                     temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
                     in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
                     server="https://acme-server.org:443/new",
+                    http01_port=80,
                     tls_sni_01_port=5001,
-                    http01_port=80
                 ),
                 name="nginx",
                 version=version)

certbot-nginx/certbot_nginx/tls_sni_01.py

@@ -1,177 +0,0 @@
-"""A class that performs TLS-SNI-01 challenges for Nginx"""
-
-import logging
-import os
-
-import six
-
-from certbot import errors
-from certbot.plugins import common
-
-from certbot_nginx import obj
-from certbot_nginx import nginxparser
-
-
-logger = logging.getLogger(__name__)
-
-
-class NginxTlsSni01(common.TLSSNI01):
-    """TLS-SNI-01 authenticator for Nginx
-
-    :ivar configurator: NginxConfigurator object
-    :type configurator: :class:`~nginx.configurator.NginxConfigurator`
-
-    :ivar list achalls: Annotated
-        class:`~certbot.achallenges.KeyAuthorizationAnnotatedChallenge`
-        challenges
-
-    :param list indices: Meant to hold indices of challenges in a
-        larger array. NginxTlsSni01 is capable of solving many challenges
-        at once which causes an indexing issue within NginxConfigurator
-        who must return all responses in order.  Imagine NginxConfigurator
-        maintaining state about where all of the http-01 Challenges,
-        TLS-SNI-01 Challenges belong in the response array.  This is an
-        optional utility.
-
-    :param str challenge_conf: location of the challenge config file
-
-    """
-
-    def perform(self):
-        """Perform a challenge on Nginx.
-
-        :returns: list of :class:`certbot.acme.challenges.TLSSNI01Response`
-        :rtype: list
-
-        """
-        if not self.achalls:
-            return []
-
-        addresses = []
-        default_addr = "{0} ssl".format(
-            self.configurator.config.tls_sni_01_port)
-
-        for achall in self.achalls:
-            vhosts = self.configurator.choose_vhosts(achall.domain, create_if_no_match=True)
-
-            # len is max 1 because Nginx doesn't authenticate wildcards
-            if vhosts and vhosts[0].addrs:
-                addresses.append(list(vhosts[0].addrs))
-            else:
-                # choose_vhosts might have modified vhosts, so put this after
-                ipv6, ipv6only = self.configurator.ipv6_info(
-                    self.configurator.config.tls_sni_01_port)
-                if ipv6:
-                    # If IPv6 is active in Nginx configuration
-                    ipv6_addr = "[::]:{0} ssl".format(
-                        self.configurator.config.tls_sni_01_port)
-                    if not ipv6only:
-                        # If ipv6only=on is not already present in the config
-                        ipv6_addr = ipv6_addr + " ipv6only=on"
-                    addresses.append([obj.Addr.fromstring(default_addr),
-                                      obj.Addr.fromstring(ipv6_addr)])
-                    logger.info(("Using default addresses %s and %s for " +
-                                 "TLSSNI01 authentication."),
-                                default_addr,
-                                ipv6_addr)
-                else:
-                    addresses.append([obj.Addr.fromstring(default_addr)])
-                    logger.info("Using default address %s for TLSSNI01 authentication.",
-                                default_addr)
-
-        # Create challenge certs
-        responses = [self._setup_challenge_cert(x) for x in self.achalls]
-
-        # Set up the configuration
-        self._mod_config(addresses)
-
-        # Save reversible changes
-        self.configurator.save("SNI Challenge", True)
-
-        return responses
-
-    def _mod_config(self, ll_addrs):
-        """Modifies Nginx config to include challenge server blocks.
-
-        :param list ll_addrs: list of lists of
-            :class:`certbot_nginx.obj.Addr` to apply
-
-        :raises .MisconfigurationError:
-            Unable to find a suitable HTTP block in which to include
-            authenticator hosts.
-
-        """
-        # Add the 'include' statement for the challenges if it doesn't exist
-        # already in the main config
-        included = False
-        include_directive = ['\n', 'include', ' ', self.challenge_conf]
-        root = self.configurator.parser.config_root
-
-        bucket_directive = ['\n', 'server_names_hash_bucket_size', ' ', '128']
-
-        main = self.configurator.parser.parsed[root]
-        for line in main:
-            if line[0] == ['http']:
-                body = line[1]
-                found_bucket = False
-                posn = 0
-                for inner_line in body:
-                    if inner_line[0] == bucket_directive[1]:
-                        if int(inner_line[1]) < int(bucket_directive[3]):
-                            body[posn] = bucket_directive
-                        found_bucket = True
-                    posn += 1
-                if not found_bucket:
-                    body.insert(0, bucket_directive)
-                if include_directive not in body:
-                    body.insert(0, include_directive)
-                included = True
-                break
-        if not included:
-            raise errors.MisconfigurationError(
-                'Certbot could not find an HTTP block to include '
-                'TLS-SNI-01 challenges in %s.' % root)
-        config = [self._make_server_block(pair[0], pair[1])
-                  for pair in six.moves.zip(self.achalls, ll_addrs)]
-        config = nginxparser.UnspacedList(config)
-
-        self.configurator.reverter.register_file_creation(
-            True, self.challenge_conf)
-
-        with open(self.challenge_conf, "w") as new_conf:
-            nginxparser.dump(config, new_conf)
-
-        logger.debug("Generated server block:\n%s", str(config))
-
-    def _make_server_block(self, achall, addrs):
-        """Creates a server block for a challenge.
-
-        :param achall: Annotated TLS-SNI-01 challenge
-        :type achall:
-            :class:`certbot.achallenges.KeyAuthorizationAnnotatedChallenge`
-
-        :param list addrs: addresses of challenged domain
-            :class:`list` of type :class:`~nginx.obj.Addr`
-
-        :returns: server block for the challenge host
-        :rtype: list
-
-        """
-        document_root = os.path.join(
-            self.configurator.config.work_dir, "tls_sni_01_page")
-
-        block = [['listen', ' ', addr.to_string(include_default=False)] for addr in addrs]
-
-        block.extend([['server_name', ' ',
-                       achall.response(achall.account_key).z_domain.decode('ascii')],
-                      # access and error logs necessary for
-                      # integration testing (non-root)
-                      ['access_log', ' ', os.path.join(
-                          self.configurator.config.work_dir, 'access.log')],
-                      ['error_log', ' ', os.path.join(
-                          self.configurator.config.work_dir, 'error.log')],
-                      ['ssl_certificate', ' ', self.get_cert_path(achall)],
-                      ['ssl_certificate_key', ' ', self.get_key_path(achall)],
-                      ['include', ' ', self.configurator.mod_ssl_conf],
-                      [['location', ' ', '/'], [['root', ' ', document_root]]]])
-        return [['server'], block]

certbot-nginx/tests/boulder-integration.conf.sh

@@ -59,7 +59,7 @@ http {
     listen 5002 $default_server;
     # IPv6.
     listen [::]:5002 $default_server;
-    server_name nginx.wtf nginx-tls.wtf nginx2.wtf;
+    server_name nginx.wtf nginx2.wtf;
 
     root $ROOT/webroot;
 

tests/certbot-boulder-integration.sh

@@ -223,7 +223,7 @@ common plugins --init --prepare | grep webroot
 
 # We start a server listening on the port for the
 # unrequested challenge to prevent regressions in #3601.
-python ./tests/run_http_server.py $tls_alpn_01_port &
+python ./tests/run_http_server.py $https_port &
 python_server_pid=$!
 certname="le1.wtf"
 common --domains le1.wtf --preferred-challenges http-01 auth \

tests/integration/_common.sh

@@ -3,10 +3,10 @@
 root=${root:-$(mktemp -d -t leitXXXX)}
 echo "Root integration tests directory: $root"
 config_dir="$root/conf"
-tls_alpn_01_port=5001
+https_port=5001
 http_01_port=5002
 sources="acme/,$(ls -dm certbot*/ | tr -d ' \n')"
-export root config_dir tls_alpn_01_port http_01_port sources
+export root config_dir https_port http_01_port sources
 certbot_path="$(command -v certbot)"
 # Flags that are added here will be added to Certbot calls within
 # certbot_test_no_force_renew.
@@ -60,8 +60,8 @@ certbot_test_no_force_renew () {
         "$certbot_path" \
             --server "${SERVER:-http://localhost:4000/directory}" \
             --no-verify-ssl \
-            --tls-sni-01-port $tls_alpn_01_port \
             --http-01-port $http_01_port \
+            --tls-sni-01-port $https_port \
             --manual-public-ip-logging-ok \
             $other_flags \
             --non-interactive \