Search included files for nginx server_names_hash_bucket_size directive (#9198)

* Search all included files for bucket directive

* Add tests for mod_config

* Update changelog

* Move changelog entry to the new release's section

* Break immediately once we've found the `http` block

Co-authored-by: alexzorin <alex@zorin.id.au>

* Add parallel descriptive comment about updating bucket directive

Co-authored-by: alexzorin <alex@zorin.id.au>

* remove github-inserted trailing whitespace

Co-authored-by: alexzorin <alex@zorin.id.au>

3 files changed, 86 insertions, 10 deletions

diff --git a/certbot-nginx/certbot_nginx/_internal/http_01.py b/certbot-nginx/certbot_nginx/_internal/http_01.py
index f9988007e..9b086d429 100644
--- a/certbot-nginx/certbot_nginx/_internal/http_01.py
+++ b/certbot-nginx/certbot_nginx/_internal/http_01.py
@@ -84,23 +84,50 @@ class NginxHttp01(common.ChallengePerformer):
         bucket_directive = ['\n', 'server_names_hash_bucket_size', ' ', '128']
 
         main = self.configurator.parser.parsed[root]
+        # insert include directive
         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
+
+        # insert or update the server_names_hash_bucket_size directive
+        # We have several options here.
+        # 1) Only check nginx.conf
+        # 2) Check included files, assuming they've been included inside http already,
+        #     because if they added it outside an http block their config is broken anyway
+        # 3) Add metadata during parsing to note if an include happened inside the http block
+        #
+        # 1 causes bugs; see https://github.com/certbot/certbot/issues/5199
+        # 3 would require a more extensive rewrite and probably isn't necessary anyway
+        # So this code uses option 2.
+        found_bucket = False
+        for file_contents in self.configurator.parser.parsed.values():
+            body = file_contents # already inside http in an included file
+            for line in file_contents:
+                if line[0] == ['http']:
+                    body = line[1] # enter http because this is nginx.conf
+                    break
+
+            for posn, inner_line in enumerate(body):
+                if inner_line[0] == bucket_directive[1]:
+                    if int(inner_line[1]) < int(bucket_directive[3]):
+                        body[posn] = bucket_directive
+                    found_bucket = True
+                    break
+
+            if found_bucket:
+                break
+
+        if not found_bucket:
+            for line in main:
+                if line[0] == ['http']:
+                    body = line[1]
+                    body.insert(0, bucket_directive)
+                    break
+
         if not included:
             raise errors.MisconfigurationError(
                 'Certbot could not find a block to include '
diff --git a/certbot-nginx/tests/http_01_test.py b/certbot-nginx/tests/http_01_test.py
index f10e44859..b9917af35 100644
--- a/certbot-nginx/tests/http_01_test.py
+++ b/certbot-nginx/tests/http_01_test.py
@@ -146,6 +146,54 @@ class HttpPerformTest(util.NginxTest):
         # 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)
 
+    def test_mod_config_insert_bucket_directive(self):
+        nginx_conf = self.http01.configurator.parser.abs_path('nginx.conf')
+
+        expected = ['server_names_hash_bucket_size', '128']
+        original_conf = self.http01.configurator.parser.parsed[nginx_conf]
+        self.assertFalse(util.contains_at_depth(original_conf, expected, 2))
+
+        self.http01.add_chall(self.achalls[0])
+        self.http01._mod_config()  # pylint: disable=protected-access
+        self.http01.configurator.save()
+        self.http01.configurator.parser.load()
+
+        generated_conf = self.http01.configurator.parser.parsed[nginx_conf]
+        self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))
+
+    def test_mod_config_update_bucket_directive_in_included_file(self):
+        # save old example.com config
+        example_com_loc = self.http01.configurator.parser.abs_path('sites-enabled/example.com')
+        with open(example_com_loc) as f:
+            original_example_com = f.read()
+
+        # modify example.com config
+        modified_example_com = 'server_names_hash_bucket_size 64;\n' + original_example_com
+        with open(example_com_loc, 'w') as f:
+            f.write(modified_example_com)
+        self.http01.configurator.parser.load()
+
+        # run change
+        self.http01.add_chall(self.achalls[0])
+        self.http01._mod_config()  # pylint: disable=protected-access
+        self.http01.configurator.save()
+        self.http01.configurator.parser.load()
+
+        # not in nginx.conf
+        expected = ['server_names_hash_bucket_size', '128']
+        nginx_conf_loc = self.http01.configurator.parser.abs_path('nginx.conf')
+        nginx_conf = self.http01.configurator.parser.parsed[nginx_conf_loc]
+        self.assertFalse(util.contains_at_depth(nginx_conf, expected, 2))
+
+        # is updated in example.com conf
+        generated_conf = self.http01.configurator.parser.parsed[example_com_loc]
+        self.assertTrue(util.contains_at_depth(generated_conf, expected, 0))
+
+        # put back example.com config
+        with open(example_com_loc, 'w') as f:
+            f.write(original_example_com)
+        self.http01.configurator.parser.load()
+
     @mock.patch("certbot_nginx._internal.configurator.NginxConfigurator.ipv6_info")
     def test_default_listen_addresses_no_memoization(self, ipv6_info):
         # pylint: disable=protected-access
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 80b44439e..9794e8426 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -14,6 +14,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
+* Nginx plugin now checks included files for the singleton server_names_hash_bucket_size directive.
 *
 
 More details about these changes can be found on our GitHub repo.