Preserve IP addresses when adding ssl listen directives in nginx server blocks (#10145)

Fixes #10011

When we take a server block with no ssl addresses in and and enable ssl,
if it has any listens on the http port, use those host addresses when
creating a directive to listen on ssl. Addresses with no port and on
other ports will be ignored.

---------

Co-authored-by: Will Greenberg <willg@eff.org>

certbot-nginx/certbot_nginx/_internal/configurator.py

@@ -21,6 +21,7 @@ from typing import Set
 from typing import Tuple
 from typing import Type
 from typing import Union
+from typing import cast
 
 from cryptography.hazmat.primitives import serialization
 from cryptography.hazmat.primitives.asymmetric import rsa
@@ -371,13 +372,14 @@ class NginxConfigurator(common.Configurator):
 
         return vhosts
 
-    def ipv6_info(self, port: str) -> Tuple[bool, bool]:
+    def ipv6_info(self, host: str, port: str) -> Tuple[bool, bool]:
         """Returns tuple of booleans (ipv6_active, ipv6only_present)
         ipv6_active is true if any server block listens ipv6 address in any port
 
         ipv6only_present is true if ipv6only=on option exists in any server
         block ipv6 listen directive for the specified port.
 
+        :param str host: Host to check ipv6only=on directive for
         :param str port: Port to check ipv6only=on directive for
 
         :returns: Tuple containing information if IPv6 is enabled in the global
@@ -391,7 +393,7 @@ class NginxConfigurator(common.Configurator):
             for addr in vh.addrs:
                 if addr.ipv6:
                     ipv6_active = True
-                if addr.ipv6only and addr.get_port() == port:
+                if addr.ipv6only and addr.get_port() == port and addr.get_addr() == host:
                     ipv6only_present = True
         return ipv6_active, ipv6only_present
 
@@ -723,9 +725,16 @@ class NginxConfigurator(common.Configurator):
 
         """
         https_port = self.config.https_port
-        ipv6info = self.ipv6_info(str(https_port))
+        http_port = self.config.http01_port
-        ipv6_block = ['']
+
-        ipv4_block = ['']
+        # no addresses should have ssl turned on here
+        assert not vhost.ssl
+
+        addrs_to_insert: List[obj.Addr] = [
+            cast(obj.Addr, obj.Addr.fromstring(f'{addr.get_addr()}:{https_port} ssl'))
+            for addr in vhost.addrs
+            if addr.get_port() == str(http_port)
+        ]
 
         # If the vhost was implicitly listening on the default Nginx port,
         # have it continue to do so.
@@ -733,31 +742,46 @@ class NginxConfigurator(common.Configurator):
             listen_block = [['\n    ', 'listen', ' ', self.DEFAULT_LISTEN_PORT]]
             self.parser.add_server_directives(vhost, listen_block)
 
-        if vhost.ipv6_enabled():
+        if not addrs_to_insert:
-            ipv6_block = ['\n    ',
+            # there are no existing addresses listening on 80
-                          'listen',
+            if vhost.ipv6_enabled():
-                          ' ',
+                addrs_to_insert += [cast(obj.Addr, obj.Addr.fromstring(f'[::]:{https_port} ssl'))]
-                          '[::]:{0}'.format(https_port),
+            if vhost.ipv4_enabled():
-                          ' ',
+                addrs_to_insert += [cast(obj.Addr, obj.Addr.fromstring(f'{https_port} ssl'))]
-                          'ssl']
-            if not ipv6info[1]:
-                # ipv6only=on is absent in global config
-                ipv6_block.append(' ')
-                ipv6_block.append('ipv6only=on')
 
-        if vhost.ipv4_enabled():
+        addr_blocks: List[List[str]] = []
-            ipv4_block = ['\n    ',
+        ipv6only_set_here: Set[Tuple[str, str]] = set()
-                          'listen',
+        for addr in addrs_to_insert:
-                          ' ',
+            host = addr.get_addr()
-                          '{0}'.format(https_port),
+            port = addr.get_port()
-                          ' ',
+            if addr.ipv6:
-                          'ssl']
+                addr_block = ['\n    ',
+                              'listen',
+                              ' ',
+                              f'{host}:{port}',
+                              ' ',
+                              'ssl']
+                ipv6only_exists = self.ipv6_info(host, port)[1]
+                if not ipv6only_exists and (host, port) not in ipv6only_set_here:
+                    addr.ipv6only = True # bookkeeping in case we switch output implementation
+                    ipv6only_set_here.add((host, port))
+                    addr_block.append(' ')
+                    addr_block.append('ipv6only=on')
+                addr_blocks.append(addr_block)
+            else:
+                tuple_string = f'{host}:{port}' if host else f'{port}'
+                addr_block = ['\n    ',
+                              'listen',
+                              ' ',
+                              tuple_string,
+                              ' ',
+                              'ssl']
+                addr_blocks.append(addr_block)
 
         snakeoil_cert, snakeoil_key = self._get_snakeoil_paths()
 
         ssl_block = ([
-            ipv6_block,
+            *addr_blocks,
-            ipv4_block,
             ['\n    ', 'ssl_certificate', ' ', snakeoil_cert],
             ['\n    ', 'ssl_certificate_key', ' ', snakeoil_key],
             ['\n    ', 'include', ' ', self.mod_ssl_conf],

certbot-nginx/certbot_nginx/_internal/http_01.py

@@ -152,7 +152,7 @@ class NginxHttp01(common.ChallengePerformer):
             self.configurator.config.http01_port)
         port = self.configurator.config.http01_port
 
-        ipv6, ipv6only = self.configurator.ipv6_info(str(port))
+        ipv6, ipv6only = self.configurator.ipv6_info("[::]", str(port))
 
         if ipv6:
             # If IPv6 is active in Nginx configuration

certbot-nginx/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 14 == len(self.config.parser.parsed)
+        assert 15 == 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,7 @@ 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"}
+             "headers.com", "example.net", "ssl.both.com", 'addr-80.com'}
 
     def test_supported_enhancements(self):
         assert ['redirect', 'ensure-http-header', 'staple-ocsp'] == \
@@ -201,11 +201,38 @@ class NginxConfiguratorTest(util.NginxTest):
                 with pytest.raises(errors.MisconfigurationError):
                     self.config.choose_vhosts(name)
 
+    def test_choose_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]
+        assert obj.Addr.fromstring("5001 ssl") in vhost.addrs
+
+        # no listens at all
+        vhost = self.config.choose_vhosts('headers.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]
+        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]
+        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_ipv6only(self):
         # ipv6_info: (ipv6_active, ipv6only_present)
-        assert (True, False) == self.config.ipv6_info("80")
+        assert (True, False) == self.config.ipv6_info("[::]", "80")
         # Port 443 has ipv6only=on because of ipv6ssl.com vhost
-        assert (True, True) == self.config.ipv6_info("443")
+        assert (True, True) == self.config.ipv6_info("[::]", "443")
 
     def test_ipv6only_detection(self):
         self.config.version = (1, 3, 1)
@@ -585,7 +612,7 @@ class NginxConfiguratorTest(util.NginxTest):
         generated_conf = self.config.parser.parsed[example_conf]
         assert [[['server'], [
                ['server_name', '.example.com'],
-               ['server_name', 'example.*'], [],
+               ['server_name', 'example.*'],
                ['listen', '5001', 'ssl'], ['#', ' managed by Certbot'],
                ['ssl_certificate', 'example/fullchain.pem'], ['#', ' managed by Certbot'],
                ['ssl_certificate_key', 'example/key.pem'], ['#', ' managed by Certbot'],
@@ -615,7 +642,7 @@ class NginxConfiguratorTest(util.NginxTest):
         generated_conf = self.config.parser.parsed[example_conf]
         assert [[['server'], [
                ['server_name', '.example.com'],
-               ['server_name', 'example.*'], [],
+               ['server_name', 'example.*'],
                ['listen', '5001', 'ssl'], ['#', ' managed by Certbot'],
                ['ssl_certificate', 'example/fullchain.pem'], ['#', ' managed by Certbot'],
                ['ssl_certificate_key', 'example/key.pem'], ['#', ' managed by Certbot'],
@@ -630,7 +657,7 @@ class NginxConfiguratorTest(util.NginxTest):
                ['listen', '127.0.0.1'],
                ['server_name', '.example.com'],
                ['server_name', 'example.*'],
-               [], [], []]]] == \
+               [], []]]] == \
             generated_conf
 
     def test_http_header_hsts(self):
@@ -957,7 +984,7 @@ class NginxConfiguratorTest(util.NginxTest):
                                                 prefer_ssl=False,
                                                 no_ssl_filter_port='80')
             # Check that the dialog was called with only port 80 vhosts
-            assert len(mock_select_vhs.call_args[0][0]) == 8
+            assert len(mock_select_vhs.call_args[0][0]) == 9
 
     def test_choose_auth_vhosts(self):
         """choose_auth_vhosts correctly selects duplicative and HTTP/HTTPS vhosts"""

certbot-nginx/certbot_nginx/_internal/tests/parser_test.py

@@ -62,7 +62,8 @@ class NginxParserTest(util.NginxTest):
                            'sites-enabled/globalssl.com',
                            'sites-enabled/ipv6.com',
                            'sites-enabled/ipv6ssl.com',
-                           'sites-enabled/example.net']} == \
+                           'sites-enabled/example.net',
+                           'sites-enabled/addr-80.com']} == \
                          set(nparser.parsed.keys())
         assert [['server_name', 'somename', 'alias', 'another.alias']] == \
                          nparser.parsed[nparser.abs_path('server.conf')]
@@ -92,7 +93,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 10 == len(
+        assert 11 == len(
             glob.glob(nparser.abs_path('sites-enabled/*.test')))
         assert [[['server'], [['listen', '69.50.225.155:9000'],
                                         ['listen', '127.0.0.1'],
@@ -175,7 +176,7 @@ class NginxParserTest(util.NginxTest):
                                                   '*.www.example.com'},
                                  [], [2, 1, 0])
 
-        assert 19 == len(vhosts)
+        assert 20 == 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]

certbot-nginx/certbot_nginx/_internal/tests/testdata/etc_nginx/sites-enabled/addr-80.com

@@ -0,0 +1,5 @@
+server {
+    listen 1.2.3.4:80;
+    listen [1:20::300]:80;
+    server_name addr-80.com;
+}

certbot/CHANGELOG.md

@@ -26,6 +26,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Honor --reuse-key when --allow-subset-of-names is set
 * Fixed regression in symlink parsing on Windows that was introduced in Certbot
   3.1.0.
+* When adding ssl listen directives in nginx server blocks, IP addresses are now
+  preserved.
 
 More details about these changes can be found on our GitHub repo.