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.