nginx: authenticate all matching vhosts for HTTP01 (#8663)

* nginx: authenticate all matching vhosts for HTTP01

Previously, the nginx authenticator would set up the HTTP-01 challenge
response on a single HTTP vhost which matched the challenge domain.

The nginx authenticator will now set the challenge response on every
vhost which matches the challenge domain, including duplicates and HTTPS
vhosts.

This makes the authenticator usable behind a CDN where all origin
traffic is performed over HTTPS and also makes the authenticator work
more reliably against "invalid" nginx configurations, such as those
where there are duplicate vhosts.

* some typos

* dont authenticate the same vhost twice

One vhost may appear in both the HTTP and HTTPS vhost lists. Use a set()
to avoid trying to mod the same vhost twice.

* fix type annotations

* rewrite changelog entry

certbot-nginx/tests/configurator_test.py

@@ -39,7 +39,7 @@ class NginxConfiguratorTest(util.NginxTest):
 
     def test_prepare(self):
         self.assertEqual((1, 6, 2), self.config.version)
-        self.assertEqual(12, len(self.config.parser.parsed))
+        self.assertEqual(13, len(self.config.parser.parsed))
 
     @mock.patch("certbot_nginx._internal.configurator.util.exe_exists")
     @mock.patch("certbot_nginx._internal.configurator.subprocess.Popen")
@@ -89,7 +89,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"})
+             "headers.com", "example.net", "ssl.both.com"})
 
     def test_supported_enhancements(self):
         self.assertEqual(['redirect', 'ensure-http-header', 'staple-ocsp'],
@@ -935,7 +935,19 @@ class NginxConfiguratorTest(util.NginxTest):
                                                 prefer_ssl=False,
                                                 no_ssl_filter_port='80')
             # Check that the dialog was called with only port 80 vhosts
-            self.assertEqual(len(mock_select_vhs.call_args[0][0]), 6)
+            self.assertEqual(len(mock_select_vhs.call_args[0][0]), 8)
+
+    def test_choose_auth_vhosts(self):
+        """choose_auth_vhosts correctly selects duplicative and HTTP/HTTPS vhosts"""
+        http, https = self.config.choose_auth_vhosts('ssl.both.com')
+        self.assertEqual(len(http), 4)
+        self.assertEqual(len(https), 2)
+        self.assertEqual(http[0].names, {'ssl.both.com'})
+        self.assertEqual(http[1].names, {'ssl.both.com'})
+        self.assertEqual(http[2].names, {'ssl.both.com'})
+        self.assertEqual(http[3].names, {'*.both.com'})
+        self.assertEqual(https[0].names, {'ssl.both.com'})
+        self.assertEqual(https[1].names, {'*.both.com'})
 
 
 class InstallSslOptionsConfTest(util.NginxTest):

certbot-nginx/tests/http_01_test.py

@@ -44,6 +44,10 @@ class HttpPerformTest(util.NginxTest):
             challb=acme_util.chall_to_challb(
                 challenges.HTTP01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"),
             domain="migration.com", account_key=account_key),
+        achallenges.KeyAuthorizationAnnotatedChallenge(
+            challb=acme_util.chall_to_challb(
+                challenges.HTTP01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"),
+            domain="ipv6ssl.com", account_key=account_key),
     ]
 
     def setUp(self):
@@ -77,8 +81,8 @@ class HttpPerformTest(util.NginxTest):
 
         http_responses = self.http01.perform()
 
-        self.assertEqual(len(http_responses), 4)
-        for i in range(4):
+        self.assertEqual(len(http_responses), 5)
+        for i in range(5):
             self.assertEqual(http_responses[i], acme_responses[i])
 
     def test_mod_config(self):
@@ -105,6 +109,43 @@ class HttpPerformTest(util.NginxTest):
             #     self.assertEqual(vhost.addrs, set(v_addr2_print))
             # self.assertEqual(vhost.names, set([response.z_domain.decode('ascii')]))
 
+    @mock.patch('certbot_nginx._internal.parser.NginxParser.add_server_directives')
+    def test_mod_config_http_and_https(self, mock_add_server_directives):
+        """A server_name with both HTTP and HTTPS vhosts should get modded in both vhosts"""
+        self.configuration.https_port = 443
+        self.http01.add_chall(self.achalls[3]) # migration.com
+        self.http01._mod_config()  # pylint: disable=protected-access
+
+        # Domain has an HTTP and HTTPS vhost
+        # 2 * 'rewrite' + 2 * 'return 200 keyauthz' = 4
+        self.assertEqual(mock_add_server_directives.call_count, 4)
+
+    @mock.patch('certbot_nginx._internal.parser.nginxparser.dump')
+    @mock.patch('certbot_nginx._internal.parser.NginxParser.add_server_directives')
+    def test_mod_config_only_https(self, mock_add_server_directives, mock_dump):
+        """A server_name with only an HTTPS vhost should get modded"""
+        self.http01.add_chall(self.achalls[4]) # ipv6ssl.com
+        self.http01._mod_config() # pylint: disable=protected-access
+
+        # It should modify the existing HTTPS vhost
+        self.assertEqual(mock_add_server_directives.call_count, 2)
+        # since there was no suitable HTTP vhost or default HTTP vhost, a non-empty one
+        # should have been created and written to the challenge conf file
+        self.assertNotEqual(mock_dump.call_args[0][0], [])
+
+    @mock.patch('certbot_nginx._internal.parser.NginxParser.add_server_directives')
+    def test_mod_config_deduplicate(self, mock_add_server_directives):
+        """A vhost that appears in both HTTP and HTTPS vhosts only gets modded once"""
+        achall = achallenges.KeyAuthorizationAnnotatedChallenge(
+            challb=acme_util.chall_to_challb(
+                challenges.HTTP01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"),
+            domain="ssl.both.com", account_key=AUTH_KEY)
+        self.http01.add_chall(achall)
+        self.http01._mod_config() # pylint: disable=protected-access
+
+        # Should only get called 5 times, rather than 6, because two vhosts are the same
+        self.assertEqual(mock_add_server_directives.call_count, 5*2)
+
     @mock.patch("certbot_nginx._internal.configurator.NginxConfigurator.ipv6_info")
     def test_default_listen_addresses_no_memoization(self, ipv6_info):
         # pylint: disable=protected-access

certbot-nginx/tests/parser_test.py

@@ -51,6 +51,7 @@ class NginxParserTest(util.NginxTest):
         self.assertEqual({nparser.abs_path(x) for x in
                           ['foo.conf', 'nginx.conf', 'server.conf',
                            'sites-enabled/default',
+                           'sites-enabled/both.com',
                            'sites-enabled/example.com',
                            'sites-enabled/headers.com',
                            'sites-enabled/migration.com',
@@ -88,7 +89,7 @@ class NginxParserTest(util.NginxTest):
         parsed = nparser._parse_files(nparser.abs_path(
             'sites-enabled/example.com.test'))
         self.assertEqual(3, len(glob.glob(nparser.abs_path('*.test'))))
-        self.assertEqual(9, len(
+        self.assertEqual(10, len(
             glob.glob(nparser.abs_path('sites-enabled/*.test'))))
         self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'],
                                         ['listen', '127.0.0.1'],
@@ -171,7 +172,7 @@ class NginxParserTest(util.NginxTest):
                                                   '*.www.example.com'},
                                  [], [2, 1, 0])
 
-        self.assertEqual(14, len(vhosts))
+        self.assertEqual(19, len(vhosts))
         example_com = [x for x in vhosts if 'example.com' in x.filep][0]
         self.assertEqual(vhost3, example_com)
         default = [x for x in vhosts if 'default' in x.filep][0]

certbot-nginx/tests/testdata/etc_nginx/sites-enabled/both.com

@@ -0,0 +1,32 @@
+server {
+  server_name ssl.both.com;
+}
+
+# a duplicate vhost
+server {
+  server_name ssl.both.com;
+}
+
+# a duplicate by means of wildcard
+server {
+  server_name *.both.com;
+}
+
+# combined HTTP and HTTPS
+server {
+  server_name ssl.both.com;
+  listen 80;
+  listen 5001 ssl;
+
+  ssl_certificate      cert.pem;
+  ssl_certificate_key  cert.key;
+}
+
+# HTTPS, duplicate by means of wildcard
+server {
+  server_name *.both.com;
+  listen 5001 ssl;
+
+  ssl_certificate      cert.pem;
+  ssl_certificate_key  cert.key;
+}