Allow http block inclusion at top-level nginx.conf (#10178)

Fixes #9928.

Thanks to @vanviethieuanh for tracking down the source of the issue!

certbot-nginx/certbot_nginx/_internal/http_01.py

@@ -77,11 +77,11 @@ class NginxHttp01(common.ChallengePerformer):
         """
         included = False
         include_directive = ['\n', 'include', ' ', self.challenge_conf]
-        root = self.configurator.parser.config_root
+        http_path = self.configurator.parser.http_path
 
         bucket_directive = ['\n', 'server_names_hash_bucket_size', ' ', '128']
 
-        main = self.configurator.parser.parsed[root]
+        main = self.configurator.parser.parsed[http_path]
         # insert include directive
         for line in main:
             if line[0] == ['http']:
@@ -126,6 +126,7 @@ class NginxHttp01(common.ChallengePerformer):
                     body.insert(0, bucket_directive)
                     break
 
+        root = self.configurator.parser.config_root
         if not included:
             raise errors.MisconfigurationError(
                 'Certbot could not find a block to include '

certbot-nginx/certbot_nginx/_internal/parser.py

@@ -1,4 +1,5 @@
 """NginxParser is a member object of the NginxConfigurator class."""
+from __future__ import annotations
 import copy
 import functools
 import glob
@@ -41,6 +42,7 @@ class NginxParser:
         self.parsed: Dict[str, UnspacedList] = {}
         self.root = os.path.abspath(root)
         self.config_root = self._find_config_root()
+        self._http_path: str | None = None
 
         # Parse nginx.conf and included files.
         # TODO: Check sites-available/ as well. For now, the configurator does
@@ -54,6 +56,14 @@ class NginxParser:
         self.parsed = {}
         self._parse_recursively(self.config_root)
 
+    @property
+    def http_path(self) -> str:
+        """ Filepath of file containing nginx http block. Set in self._parse_recursively
+        """
+        if self._http_path is None:
+            raise errors.MisconfigurationError('No nginx http block found')
+        return self._http_path
+
     def _parse_recursively(self, filepath: str) -> None:
         """Parses nginx config files recursively by looking at 'include'
         directives inside 'http' and 'server' blocks. Note that this only
@@ -64,13 +74,16 @@ class NginxParser:
         """
         # pylint: disable=too-many-nested-blocks
         filepath = self.abs_path(filepath)
-        trees = self._parse_files(filepath)
-        for tree in trees:
+        trees: dict[str, UnspacedList] = self._parse_files(filepath)
+        for filename, tree in trees.items():
             for entry in tree:
                 if _is_include_directive(entry):
                     # Parse the top-level included file
                     self._parse_recursively(entry[1])
                 elif entry[0] == ['http'] or entry[0] == ['server']:
+                    # Note http block location for http_01.py
+                    if entry[0] == ['http']:
+                        self._http_path = filename
                     # Look for includes in the top-level 'http'/'server' context
                     for subentry in entry[1]:
                         if _is_include_directive(subentry):
@@ -193,35 +206,35 @@ class NginxParser:
                         pass
         return result
 
-    def _parse_files(self, filepath: str, override: bool = False) -> List[UnspacedList]:
+    def _parse_files(self, filepath: str, override: bool = False) -> dict[str, UnspacedList]:
         """Parse files from a glob
 
         :param str filepath: Nginx config file path
         :param bool override: Whether to parse a file that has been parsed
-        :returns: list of parsed tree structures
-        :rtype: list
+        :returns: dict of parsed tree structures indexed by filename
+        :rtype: dict[str, UnspacedList]
 
         """
-        files = glob.glob(filepath) # nginx on unix calls glob(3) for this
+        files: list[str] = glob.glob(filepath) # nginx on unix calls glob(3) for this
                                     # XXX Windows nginx uses FindFirstFile, and
                                     # should have a narrower call here
-        trees = []
-        for item in files:
-            if item in self.parsed and not override:
+        trees: dict[str, UnspacedList] = {}
+        for filename in files:
+            if filename in self.parsed and not override:
                 continue
             try:
-                with open(item, "r", encoding="utf-8") as _file:
+                with open(filename, "r", encoding="utf-8") as _file:
                     parsed = nginxparser.load(_file)
-                    self.parsed[item] = parsed
-                    trees.append(parsed)
+                    self.parsed[filename] = parsed
+                    trees[filename] = parsed
             except OSError:
-                logger.warning("Could not open file: %s", item)
+                logger.warning("Could not open file: %s", filename)
             except UnicodeDecodeError:
                 logger.warning("Could not read file: %s due to invalid "
                                "character. Only UTF-8 encoding is "
-                               "supported.", item)
+                               "supported.", filename)
             except pyparsing.ParseException as err:
-                logger.warning("Could not parse file: %s due to %s", item, err)
+                logger.warning("Could not parse file: %s due to %s", filename, err)
         return trees
 
     def _find_config_root(self) -> str:

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

@@ -134,7 +134,7 @@ class NginxConfiguratorTest(util.NginxTest):
                             ['server_name', 'example.*'],
                             ['listen', '5001', 'ssl'],
                             ['#', parser.COMMENT]]]] == \
-                         parsed[0]
+                         parsed[filep]
 
     def test_choose_vhosts_alias(self):
         self._test_choose_vhosts_common('alias', 'server_conf')

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

@@ -74,6 +74,15 @@ class NginxParserTest(util.NginxTest):
                          nparser.parsed[nparser.abs_path(
                              'sites-enabled/example.com')]
 
+    def test_included_load(self):
+        # Test for when the root file doesn't have http in it
+        nparser = parser.NginxParser(self.config_path)
+        nparser.config_root = os.path.join(self.config_path, "nginx-include.conf")
+        nparser.load()
+        assert len(nparser.parsed) > 1
+        assert len(nparser.parsed[nparser.config_root]) == 4
+        assert os.path.join(self.config_path, "nginx.conf") in nparser.parsed
+
     def test_abs_path(self):
         nparser = parser.NginxParser(self.config_path)
         if os.name != 'nt':
@@ -99,7 +108,7 @@ class NginxParserTest(util.NginxTest):
                                         ['listen', '127.0.0.1'],
                                         ['server_name', '.example.com'],
                                         ['server_name', 'example.*']]]] == \
-                         parsed[0]
+                         parsed[nparser.abs_path('sites-enabled/example.com.test')]
 
     def test__do_for_subarray(self):
         # pylint: disable=protected-access
@@ -490,8 +499,8 @@ class NginxParserTest(util.NginxTest):
         nparser = parser.NginxParser(self.config_path)
         path = nparser.abs_path('valid_unicode_comments.conf')
         parsed = nparser._parse_files(path)  # pylint: disable=protected-access
-        assert ['server'] == parsed[0][2][0]
-        assert ['listen', '80'] == parsed[0][2][1][3]
+        assert ['server'] == parsed[path][2][0]
+        assert ['listen', '80'] == parsed[path][2][1][3]
 
     def test_valid_unicode_roundtrip(self):
         """This tests the parser's ability to load and save a config containing Unicode"""
@@ -507,7 +516,7 @@ class NginxParserTest(util.NginxTest):
             path = nparser.abs_path('invalid_unicode_comments.conf')
             parsed = nparser._parse_files(path)  # pylint: disable=protected-access
 
-        assert [] == parsed
+        assert {} == parsed
         assert any(
             ('invalid character' in output) and ('UTF-8' in output)
             for output in log.output

certbot-nginx/certbot_nginx/_internal/tests/testdata/etc_nginx/nginx-include.conf

@@ -0,0 +1,4 @@
+# just a comment on top
+# so we're not at the very top
+include nginx.conf;
+# and another comment for fun

certbot/CHANGELOG.md

@@ -30,6 +30,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   3.1.0.
 * When adding ssl listen directives in nginx server blocks, IP addresses are now
   preserved.
+* Nginx configurations can now have the http block in files other than the root (nginx.conf)
 
 More details about these changes can be found on our GitHub repo.