From c178fa8c0be8266a6edd9be5973bc1c135734c38 Mon Sep 17 00:00:00 2001 From: alexzorin Date: Wed, 30 Nov 2022 12:03:51 +1100 Subject: [PATCH] nginx: on encountering lua directives, produce a better warning (#9475) * nginx: capitalise product names in warning message properly * nginx: don't crash on encountering lua directives, warn instead * add tests * undo excess newline * fix oldest tests: use old camelCase function name * add missing newline in new testdata * add tests for _by_lua, which should parse fine --- .../certbot_nginx/_internal/configurator.py | 4 +- .../certbot_nginx/_internal/nginxparser.py | 19 ++++++ .../certbot_nginx/_internal/parser.py | 6 ++ certbot-nginx/tests/nginxparser_test.py | 64 +++++++++++++++++++ certbot-nginx/tests/parser_test.py | 9 +++ .../etc_nginx/unsupported_directives.conf | 11 ++++ 6 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 certbot-nginx/tests/testdata/etc_nginx/unsupported_directives.conf diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py index 508f50ab7..b91b11a53 100644 --- a/certbot-nginx/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/certbot_nginx/_internal/configurator.py @@ -1060,8 +1060,8 @@ class NginxConfigurator(common.Configurator): product_name, product_version = version_matches[0] if product_name != 'nginx': - logger.warning("NGINX derivative %s is not officially supported by" - " certbot", product_name) + logger.warning("nginx derivative %s is not officially supported by " + "Certbot.", product_name) nginx_version = tuple(int(i) for i in product_version.split(".")) diff --git a/certbot-nginx/certbot_nginx/_internal/nginxparser.py b/certbot-nginx/certbot_nginx/_internal/nginxparser.py index 1c74cd367..99955447a 100644 --- a/certbot-nginx/certbot_nginx/_internal/nginxparser.py +++ b/certbot-nginx/certbot_nginx/_internal/nginxparser.py @@ -33,6 +33,18 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) +class UnsupportedDirectiveException(RuntimeError): + """Exception when encountering an nginx directive which is not supported + by this parser.""" + + directive_name: str + line_no: int + + def __init__(self, directive_name: str, line_no: int) -> None: + self.directive_name = directive_name + self.line_no = line_no + + class RawNginxParser: # pylint: disable=pointless-statement """A class that parses nginx configuration with pyparsing.""" @@ -74,6 +86,7 @@ class RawNginxParser: def __init__(self, source: str) -> None: self.source = source + self.whitespace_token_group.addParseAction(self._check_disallowed_directive) def parse(self) -> ParseResults: """Returns the parsed tree.""" @@ -83,6 +96,12 @@ class RawNginxParser: """Returns the parsed tree as a list.""" return self.parse().asList() + def _check_disallowed_directive(self, _source: str, line: int, results: ParseResults) -> None: + # *_by_lua_block might be first or second result, due to optional leading whitespace + toks = [t for t in results[0:2] if isinstance(t, str) and t.endswith("_by_lua_block")] + if toks: + raise UnsupportedDirectiveException(toks[0], line) + class RawNginxDumper: """A class that dumps nginx configuration from the provided tree.""" diff --git a/certbot-nginx/certbot_nginx/_internal/parser.py b/certbot-nginx/certbot_nginx/_internal/parser.py index e60d66eb2..bc1643426 100644 --- a/certbot-nginx/certbot_nginx/_internal/parser.py +++ b/certbot-nginx/certbot_nginx/_internal/parser.py @@ -223,6 +223,12 @@ class NginxParser: "supported.", item) except pyparsing.ParseException as err: logger.warning("Could not parse file: %s due to %s", item, err) + except nginxparser.UnsupportedDirectiveException as e: + logger.warning( + "%s:%d contained the '%s' directive, which is not supported by Certbot. The " + "file has been ignored, which may prevent Certbot from functioning properly. " + "Consider using the --webroot plugin and manually installing the certificate.", + item, e.line_no, e.directive_name) return trees def _find_config_root(self) -> str: diff --git a/certbot-nginx/tests/nginxparser_test.py b/certbot-nginx/tests/nginxparser_test.py index d8f0a5909..3713f16e4 100644 --- a/certbot-nginx/tests/nginxparser_test.py +++ b/certbot-nginx/tests/nginxparser_test.py @@ -12,6 +12,7 @@ from certbot_nginx._internal.nginxparser import load from certbot_nginx._internal.nginxparser import loads from certbot_nginx._internal.nginxparser import RawNginxParser from certbot_nginx._internal.nginxparser import UnspacedList +from certbot_nginx._internal.nginxparser import UnsupportedDirectiveException import test_util as util FIRST = operator.itemgetter(0) @@ -354,6 +355,69 @@ class TestRawNginxParser(unittest.TestCase): parsed = loads("") self.assertEqual(parsed, []) + def test_lua(self): + # https://github.com/certbot/certbot/issues/9066 + self.assertRaises(UnsupportedDirectiveException, loads, """ + location /foo { + content_by_lua_block { + ngx.say('Hello World') + } + } + """) + + # Without leading whitespace + self.assertRaises(UnsupportedDirectiveException, loads, """ + location /foo {content_by_lua_block { + ngx.say('Hello World') + } + } + """) + + # Doesn't trigger if it's commented or not in the right position. + parsed = loads(""" + location /foo {server_name content_by_lua_block; + #content_by_lua_block { + # ngx.say('Hello World') + # } + } + """) + self.assertEqual( + parsed, + [ + [['location', '/foo'], + [['server_name', 'content_by_lua_block'], + ['#', 'content_by_lua_block {'], + ['#', " ngx.say('Hello World')"], + ['#', ' }'] + ]] + ]) + + # *_by_lua should parse successfully. + parsed = loads(""" + location / { + set $a 32; + set $b 56; + set_by_lua $sum + 'return tonumber(ngx.arg[1]) + tonumber(ngx.arg[2])' + $a $b; + content_by_lua ' + ngx.say("foo"); + '; + } + """) + self.assertEqual( + parsed, + [ + [['location', '/'], + [['set', '$a', '32'], + ['set', '$b', '56'], + ['set_by_lua', '$sum', + "'return tonumber(ngx.arg[1]) + tonumber(ngx.arg[2])'", '$a', '$b' + ], + ['content_by_lua', '\'\n ngx.say("foo");\n \''] + ]] + ] + ) class TestUnspacedList(unittest.TestCase): """Test the UnspacedList data structure""" diff --git a/certbot-nginx/tests/parser_test.py b/certbot-nginx/tests/parser_test.py index 7d5b5e669..93a8dcedc 100644 --- a/certbot-nginx/tests/parser_test.py +++ b/certbot-nginx/tests/parser_test.py @@ -4,6 +4,7 @@ import re import shutil import unittest from typing import List +from unittest import mock from certbot import errors from certbot.compat import os @@ -532,6 +533,14 @@ class NginxParserTest(util.NginxTest): for output in log.output )) + @mock.patch('certbot_nginx._internal.parser.logger.warning') + def test_load_unsupported_directive_logged(self, mock_warn): + nparser = parser.NginxParser(self.config_path) + nparser.config_root = nparser.abs_path('unsupported_directives.conf') + nparser.load() + self.assertEqual(mock_warn.call_count, 1) + self.assertIn("which is not supported by Certbot", mock_warn.call_args[0][0]) + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-nginx/tests/testdata/etc_nginx/unsupported_directives.conf b/certbot-nginx/tests/testdata/etc_nginx/unsupported_directives.conf new file mode 100644 index 000000000..d071c68c1 --- /dev/null +++ b/certbot-nginx/tests/testdata/etc_nginx/unsupported_directives.conf @@ -0,0 +1,11 @@ +# This configuration file contains unsupported direcives. + +server { + listen 80; + + location / { + foobar_by_lua_block { + ngx.say("Hello World") + } + } +}