From 7be2e790251a9f5b7c8ab48560bfe4e9737c11af Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 24 Mar 2017 19:45:53 -0700 Subject: [PATCH] Fix nginx parser (#4296) * rewrite nginx parser to allow everything that nginx does * also make changes in tls_sni_01.py * add test case with * allow embedded variables * allow empty ${} variable * fix quotes * un-special case if * update all tests to reflect current parsing * escape in QuotedString after merge * add test cases for variable weirdness that are almost certainly nginx bugs * update regex for correct variable rules * close paren doesn't invoke last_space * Make test file valid Nginx syntax --- .../simplepythonfcgi/weird-spacing.conf | 6 +- certbot-nginx/certbot_nginx/configurator.py | 8 +- certbot-nginx/certbot_nginx/nginxparser.py | 112 ++++------ certbot-nginx/certbot_nginx/parser.py | 30 ++- .../certbot_nginx/tests/configurator_test.py | 18 +- .../certbot_nginx/tests/nginxparser_test.py | 203 ++++++++++++++++-- certbot-nginx/certbot_nginx/tests/obj_test.py | 18 +- .../certbot_nginx/tests/parser_test.py | 14 +- certbot-nginx/certbot_nginx/tls_sni_01.py | 9 +- 9 files changed, 273 insertions(+), 145 deletions(-) diff --git a/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf b/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf index a201fe659..5fbc76676 100644 --- a/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf +++ b/certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf @@ -1,16 +1,16 @@ # static files location ~ ^/(images|javascript|js|css|flash|media|static)/ { - root ${PROJECTBASE}/${PROJECTNAME}/static; + root "${PROJECTBASE}/${PROJECTNAME}/static"; } location = /favicon.ico { - root ${PROJECTBASE}/${PROJECTNAME}/static/images; + root "${PROJECTBASE}/${PROJECTNAME}/static/images"; } # pass all requests to FastCGI TG server listening on ${HOST}:${PORT} # location / { - fastcgi_pass ${HOST}:${PORT}; + fastcgi_pass "${HOST}:${PORT}"; fastcgi_index index; fastcgi_param SCRIPT_FILENAME /scripts$fastcgi_script_name; include conf/fastcgi_params; diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index e62194d4f..225702251 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -32,16 +32,16 @@ from certbot_nginx import parser logger = logging.getLogger(__name__) REDIRECT_BLOCK = [[ - ['\n ', 'if', ' ', '($scheme != "https") '], - [['\n ', 'return', ' ', '301 https://$host$request_uri'], + ['\n ', 'if', ' ', '($scheme', ' ', '!=', ' ', '"https") '], + [['\n ', 'return', ' ', '301', ' ', 'https://$host$request_uri'], '\n '] ], ['\n']] TEST_REDIRECT_BLOCK = [ [ - ['if', '($scheme != "https")'], + ['if', '($scheme', '!=', '"https")'], [ - ['return', '301 https://$host$request_uri'] + ['return', '301', 'https://$host$request_uri'] ] ], ['#', ' managed by Certbot'] diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 67ac7c3a1..20aeeb554 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -2,11 +2,9 @@ # Forked from https://github.com/fatiherikli/nginxparser (MIT Licensed) import copy import logging -import string from pyparsing import ( - Literal, White, Word, alphanums, CharsNotIn, Combine, Forward, Group, - Optional, OneOrMore, QuotedString, Regex, ZeroOrMore) + Literal, White, Forward, Group, Optional, OneOrMore, QuotedString, Regex, ZeroOrMore, Combine) from pyparsing import stringEnd from pyparsing import restOfLine @@ -14,73 +12,42 @@ logger = logging.getLogger(__name__) class RawNginxParser(object): # pylint: disable=expression-not-assigned + # pylint: disable=pointless-statement """A class that parses nginx configuration with pyparsing.""" # constants - space = Optional(White()) - nonspace = Regex(r"\S+") + space = Optional(White()).leaveWhitespace() + required_space = White().leaveWhitespace() + left_bracket = Literal("{").suppress() - right_bracket = space.leaveWhitespace() + Literal("}").suppress() + right_bracket = space + Literal("}").suppress() semicolon = Literal(";").suppress() - key = Word(alphanums + "_/+-.") - dollar_var = Combine(Literal('$') + Regex(r"[^\{\};,\s]+")) - condition = Regex(r"\(.+\)") - # Matches anything that is not a special character, and ${SHELL_VARS}, AND - # any chars in single or double quotes - # All of these COULD be upgraded to something like - # https://stackoverflow.com/a/16130746 - dquoted = QuotedString('"', multiline=True, unquoteResults=False) - squoted = QuotedString("'", multiline=True, unquoteResults=False) - nonspecial = Regex(r"[^\{\};,]") - varsub = Regex(r"(\$\{\w+\})") - # nonspecial nibbles one character at a time, but the other objects take - # precedence. We use ZeroOrMore to allow entries like "break ;" to be - # parsed as assignments - value = Combine(ZeroOrMore(dquoted | squoted | varsub | nonspecial)) + dquoted = QuotedString('"', multiline=True, unquoteResults=False, escChar='\\') + squoted = QuotedString("'", multiline=True, unquoteResults=False, escChar='\\') + quoted = dquoted | squoted + head_tokenchars = Regex(r"[^{};\s'\"]") # if (last_space) + tail_tokenchars = Regex(r"(\$\{)|[^{;\s]") # else + tokenchars = Combine(head_tokenchars + ZeroOrMore(tail_tokenchars)) + paren_quote_extend = Combine(quoted + Literal(')') + ZeroOrMore(tail_tokenchars)) + # note: ')' allows extension, but then we fall into else, not last_space. - location = CharsNotIn("{};," + string.whitespace) - # modifier for location uri [ = | ~ | ~* | ^~ ] - modifier = Literal("=") | Literal("~*") | Literal("~") | Literal("^~") + token = paren_quote_extend | tokenchars | quoted + + whitespace_token_group = space + token + ZeroOrMore(required_space + token) + space + assignment = whitespace_token_group + semicolon - # rules comment = space + Literal('#') + restOfLine - assignment = space + key + Optional(space + value, default=None) + semicolon - location_statement = space + Optional(modifier) + Optional(space + location + space) - if_statement = space + Literal("if") + space + condition + space - charset_map_statement = space + Literal("charset_map") + space + value + space + value - - map_statement = space + Literal("map") + space + nonspace + space + dollar_var + space - # This is NOT an accurate way to parse nginx map entries; it's almost - # certainly too permissive and may be wrong in other ways, but it should - # preserve things correctly in mmmmost or all cases. - # - # - I can neither prove nor disprove that it is correct wrt all escaped - # semicolon situations - # Addresses https://github.com/fatiherikli/nginxparser/issues/19 - map_pattern = Regex(r'".*"') | Regex(r"'.*'") | nonspace - map_entry = space + map_pattern + space + value + space + semicolon - map_block = Group( - Group(map_statement).leaveWhitespace() + - left_bracket + - Group(ZeroOrMore(Group(comment | map_entry)) + space).leaveWhitespace() + - right_bracket) - block = Forward() - # key could for instance be "server" or "http", or "location" (in which case - # location_statement needs to have a non-empty location) + # order matters! see issue 518, and also http { # server { \n} + contents = Group(comment) | Group(block) | Group(assignment) - block_begin = (Group(space + key + location_statement) ^ - Group(if_statement) ^ - Group(charset_map_statement)).leaveWhitespace() + block_begin = Group(whitespace_token_group) + block_innards = Group(ZeroOrMore(contents) + space).leaveWhitespace() + block << block_begin + left_bracket + block_innards + right_bracket - block_innards = Group(ZeroOrMore(Group(comment | assignment) | block | map_block) - + space).leaveWhitespace() - - block << Group(block_begin + left_bracket + block_innards + right_bracket) - - script = OneOrMore(Group(comment | assignment) ^ block ^ map_block) + space + stringEnd + script = OneOrMore(contents) + space + stringEnd script.parseWithTabs().leaveWhitespace() def __init__(self, source): @@ -107,30 +74,23 @@ class RawNginxDumper(object): if isinstance(b0, str): yield b0 continue - b = copy.deepcopy(b0) - if spacey(b[0]): - yield b.pop(0) # indentation - if not b: + item = copy.deepcopy(b0) + if spacey(item[0]): + yield item.pop(0) # indentation + if not item: continue - key, values = b.pop(0), b.pop(0) - if isinstance(key, list): - yield "".join(key) + '{' - for parameter in values: + if isinstance(item[0], list): # block + yield "".join(item.pop(0)) + '{' + for parameter in item.pop(0): for line in self.__iter__([parameter]): # negate "for b0 in blocks" yield line yield '}' - else: - if isinstance(key, str) and key.strip() == '#': # comment - yield key + values - else: # assignment - gap = "" - # Sometimes the parser has stuck some gap whitespace in here; - # if so rotate it into gap - if values and spacey(values): - gap = values - values = b.pop(0) - yield key + gap + values + ';' + else: # not a block - list of strings + semicolon = ";" + if isinstance(item[0], str) and item[0].strip() == '#': # comment + semicolon = "" + yield "".join(item) + semicolon def __str__(self): """Return the parsed block as a string.""" diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index fd4ea4f11..6f3f344db 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -298,9 +298,9 @@ class NginxParser(object): """ server = vhost.raw for directive in server: - if not directive or len(directive) < 2: + if not directive: continue - elif directive[0] == 'ssl' and directive[1] == 'on': + elif _is_ssl_on_directive(directive): return True return False @@ -468,17 +468,17 @@ def _is_include_directive(entry): len(entry) == 2 and entry[0] == 'include' and isinstance(entry[1], str)) +def _is_ssl_on_directive(entry): + """Checks if an nginx parsed entry is an 'ssl on' directive. -def _get_servernames(names): - """Turns a server_name string into a list of server names - - :param str names: server names - :rtype: list + :param list entry: the parsed entry + :returns: Whether it's an 'ssl on' directive + :rtype: bool """ - whitespace_re = re.compile(r'\s+') - names = re.sub(whitespace_re, ' ', names) - return names.split(' ') + return (isinstance(entry, list) and + len(entry) == 2 and entry[0] == 'ssl' and + entry[1] == 'on') def _add_directives(block, directives, replace): """Adds or replaces directives in a config block. @@ -550,12 +550,11 @@ def _add_directive(block, directive, replace): # and there is already a copy of that directive with a different value # in the config file. directive_name = directive[0] - directive_value = directive[1] if location is None or (isinstance(directive_name, str) and directive_name in REPEATABLE_DIRECTIVES): block.append(directive) _comment_directive(block, len(block) - 1) - elif block[location][1] != directive_value: + elif block[location] != directive: raise errors.MisconfigurationError( 'tried to insert directive "{0}" but found ' 'conflicting "{1}".'.format(directive, block[location])) @@ -585,15 +584,14 @@ def _parse_server_raw(server): if not directive: continue if directive[0] == 'listen': - addr = obj.Addr.fromstring(directive[1]) + addr = obj.Addr.fromstring(" ".join(directive[1:])) if addr: parsed_server['addrs'].add(addr) if addr.ssl: parsed_server['ssl'] = True elif directive[0] == 'server_name': - parsed_server['names'].update( - _get_servernames(directive[1])) - elif directive[0] == 'ssl' and directive[1] == 'on': + parsed_server['names'].update(directive[1:]) + elif _is_ssl_on_directive(directive): parsed_server['ssl'] = True apply_ssl_to_all_addrs = True diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index d491d2a15..b9e70cd59 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -94,7 +94,7 @@ class NginxConfiguratorTest(util.NginxTest): None, [0]) self.config.parser.add_server_directives( mock_vhost, - [['listen', ' ', '5001 ssl']], + [['listen', ' ', '5001', ' ', 'ssl']], replace=False) self.config.save() @@ -105,7 +105,7 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '127.0.0.1'], ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'], + ['listen', '5001', 'ssl'], ['#', parser.COMMENT]]]], parsed[0]) @@ -205,13 +205,13 @@ class NginxConfiguratorTest(util.NginxTest): ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'], + ['listen', '5001', 'ssl'], ['ssl_certificate', 'example/fullchain.pem'], ['ssl_certificate_key', 'example/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) ]], parsed_example_conf) - self.assertEqual([['server_name', 'somename alias another.alias']], + self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']], parsed_server_conf) self.assertTrue(util.contains_at_depth( parsed_nginx_conf, @@ -222,8 +222,8 @@ class NginxConfiguratorTest(util.NginxTest): ['include', 'server.conf'], [['location', '/'], [['root', 'html'], - ['index', 'index.html index.htm']]], - ['listen', '5001 ssl'], + ['index', 'index.html', 'index.htm']]], + ['listen', '5001', 'ssl'], ['ssl_certificate', '/etc/nginx/fullchain.pem'], ['ssl_certificate_key', '/etc/nginx/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) @@ -247,7 +247,7 @@ class NginxConfiguratorTest(util.NginxTest): ['server_name', 'summer.com'], ['listen', '80'], - ['listen', '5001 ssl'], + ['listen', '5001', 'ssl'], ['ssl_certificate', 'summer/fullchain.pem'], ['ssl_certificate_key', 'summer/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) @@ -408,8 +408,8 @@ class NginxConfiguratorTest(util.NginxTest): # Test that we successfully add a redirect when there is # a listen directive expected = [ - ['if', '($scheme != "https") '], - [['return', '301 https://$host$request_uri']] + ['if', '($scheme', '!=', '"https") '], + [['return', '301', 'https://$host$request_uri']] ] example_conf = self.config.parser.abs_path('sites-enabled/example.com') diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index 653fb5069..0a8dbd100 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -25,15 +25,15 @@ class TestRawNginxParser(unittest.TestCase): def test_blocks(self): parsed = RawNginxParser.block.parseString('foo {}').asList() - self.assertEqual(parsed, [[['foo', ' '], []]]) + self.assertEqual(parsed, [['foo', ' '], []]) parsed = RawNginxParser.block.parseString('location /foo{}').asList() - self.assertEqual(parsed, [[['location', ' ', '/foo'], []]]) + self.assertEqual(parsed, [['location', ' ', '/foo'], []]) parsed = RawNginxParser.block.parseString('foo { bar foo ; }').asList() - self.assertEqual(parsed, [[['foo', ' '], [[' ', 'bar', ' ', 'foo '], ' ']]]) + self.assertEqual(parsed, [['foo', ' '], [[' ', 'bar', ' ', 'foo', ' '], ' ']]) def test_nested_blocks(self): parsed = RawNginxParser.block.parseString('foo { bar {} }').asList() - block, content = FIRST(parsed) + block, content = parsed self.assertEqual(FIRST(content), [[' ', 'bar', ' '], []]) self.assertEqual(FIRST(block), 'foo') @@ -72,8 +72,8 @@ class TestRawNginxParser(unittest.TestCase): [['user', 'www-data'], [['http'], [[['server'], [ - ['listen', '*:80 default_server ssl'], - ['server_name', '*.www.foo.com *.www.example.com'], + ['listen', '*:80', 'default_server', 'ssl'], + ['server_name', '*.www.foo.com', '*.www.example.com'], ['root', '/home/ubuntu/sites/foo/'], [['location', '/status'], [ [['types'], [['image/jpeg', 'jpg']]], @@ -97,17 +97,17 @@ class TestRawNginxParser(unittest.TestCase): [['server'], [['server_name', 'with.if'], [['location', '~', '^/services/.+$'], - [[['if', '($request_filename ~* \\.(ttf|woff)$)'], - [['add_header', 'Access-Control-Allow-Origin "*"']]]]]]], + [[['if', '($request_filename', '~*', '\\.(ttf|woff)$)'], + [['add_header', 'Access-Control-Allow-Origin', '"*"']]]]]]], [['server'], [['server_name', 'with.complicated.headers'], [['location', '~*', '\\.(?:gif|jpe?g|png)$'], - [['add_header', 'Pragma public'], + [['add_header', 'Pragma', 'public'], ['add_header', - 'Cache-Control \'public, must-revalidate, proxy-revalidate\'' - ' "test,;{}" foo'], + 'Cache-Control', '\'public, must-revalidate, proxy-revalidate\'', + '"test,;{}"', 'foo'], ['blah', '"hello;world"'], - ['try_files', '$uri @rewrites']]]]]]) + ['try_files', '$uri', '@rewrites']]]]]]) def test_parse_from_file3(self): with open(util.get_data_filename('multiline_quotes.conf')) as handle: @@ -135,7 +135,7 @@ class TestRawNginxParser(unittest.TestCase): with open(util.get_data_filename('nginx.conf')) as handle: parsed = load(handle) parsed[-1][-1].append(UnspacedList([['server'], - [['listen', ' ', '443 ssl'], + [['listen', ' ', '443', ' ', 'ssl'], ['server_name', ' ', 'localhost'], ['ssl_certificate', ' ', 'cert.pem'], ['ssl_certificate_key', ' ', 'cert.key'], @@ -144,7 +144,7 @@ class TestRawNginxParser(unittest.TestCase): ['ssl_ciphers', ' ', 'HIGH:!aNULL:!MD5'], [['location', ' ', '/'], [['root', ' ', 'html'], - ['index', ' ', 'index.html index.htm']]]]])) + ['index', ' ', 'index.html', ' ', 'index.htm']]]]])) with tempfile.TemporaryFile(mode='w+t') as f: dump(parsed, f) @@ -179,10 +179,175 @@ class TestRawNginxParser(unittest.TestCase): parsed = loads('if ($http_accept ~* "webp") { set $webp "true"; }') self.assertEqual(parsed, [ - [['if', '($http_accept ~* "webp")'], - [['set', '$webp "true"']]] + [['if', '($http_accept', '~*', '"webp")'], + [['set', '$webp', '"true"']]] ]) + def test_comment_in_block(self): + parsed = loads("""http { + # server{ + }""") + + self.assertEqual(parsed, [ + [['http'], + [['#', ' server{']]] + ]) + + def test_access_log(self): + # see issue #3798 + parsed = loads('access_log syslog:server=unix:/dev/log,facility=auth,' + 'tag=nginx_post,severity=info custom;') + + self.assertEqual(parsed, [ + ['access_log', + 'syslog:server=unix:/dev/log,facility=auth,tag=nginx_post,severity=info', + 'custom'] + ]) + + def test_add_header(self): + # see issue #3798 + parsed = loads('add_header Cache-Control no-cache,no-store,must-revalidate,max-age=0;') + + self.assertEqual(parsed, [ + ['add_header', 'Cache-Control', 'no-cache,no-store,must-revalidate,max-age=0'] + ]) + + def test_map_then_assignment_in_block(self): + # see issue #3798 + test_str = """http { + map $http_upgrade $connection_upgrade { + default upgrade; + '' close; + "~Opera Mini" 1; + *.example.com 1; + } + one; + }""" + parsed = loads(test_str) + self.assertEqual(parsed, [ + [['http'], [ + [['map', '$http_upgrade', '$connection_upgrade'], [ + ['default', 'upgrade'], + ["''", 'close'], + ['"~Opera Mini"', '1'], + ['*.example.com', '1'] + ]], + ['one'] + ]] + ]) + + def test_variable_name(self): + parsed = loads('try_files /typo3temp/tx_ncstaticfilecache/' + '$host${request_uri}index.html @nocache;') + + self.assertEqual(parsed, [ + ['try_files', + '/typo3temp/tx_ncstaticfilecache/$host${request_uri}index.html', + '@nocache'] + ]) + + def test_weird_blocks(self): + test = r""" + if ($http_user_agent ~ MSIE) { + rewrite ^(.*)$ /msie/$1 break; + } + + if ($http_cookie ~* "id=([^;]+)(?:;|$)") { + set $id $1; + } + + if ($request_method = POST) { + return 405; + } + + if ($request_method) { + return 403; + } + + if ($args ~ post=140){ + rewrite ^ http://example.com/; + } + + location ~ ^/users/(.+\.(?:gif|jpe?g|png))$ { + alias /data/w3/images/$1; + } + """ + parsed = loads(test) + self.assertEqual(parsed, [[['if', '($http_user_agent', '~', 'MSIE)'], + [['rewrite', '^(.*)$', '/msie/$1', 'break']]], + [['if', '($http_cookie', '~*', '"id=([^;]+)(?:;|$)")'], [['set', '$id', '$1']]], + [['if', '($request_method', '=', 'POST)'], [['return', '405']]], + [['if', '($request_method)'], + [['return', '403']]], [['if', '($args', '~', 'post=140)'], + [['rewrite', '^', 'http://example.com/']]], + [['location', '~', '^/users/(.+\\.(?:gif|jpe?g|png))$'], + [['alias', '/data/w3/images/$1']]]] + ) + + def test_edge_cases(self): + # quotes + parsed = loads(r'"hello\""; # blah "heh heh"') + self.assertEqual(parsed, [['"hello\\""'], ['#', ' blah "heh heh"']]) + + # empty var as block + parsed = loads(r"${}") + self.assertEqual(parsed, [[['$'], []]]) + + # if with comment + parsed = loads("""if ($http_cookie ~* "id=([^;]+)(?:;|$)") { # blah ) + }""") + self.assertEqual(parsed, [[['if', '($http_cookie', '~*', '"id=([^;]+)(?:;|$)")'], + [['#', ' blah )']]]]) + + # end paren + test = """ + one"test"; + ("two"); + "test")red; + "test")"blue"; + "test")"three; + (one"test")one; + one"; + one"test; + one"test"one; + """ + parsed = loads(test) + self.assertEqual(parsed, [ + ['one"test"'], + ['("two")'], + ['"test")red'], + ['"test")"blue"'], + ['"test")"three'], + ['(one"test")one'], + ['one"'], + ['one"test'], + ['one"test"one'] + ]) + self.assertRaises(ParseException, loads, r'"test"one;') # fails + self.assertRaises(ParseException, loads, r'"test;') # fails + + # newlines + test = """ + server_name foo.example.com bar.example.com \ + baz.example.com qux.example.com; + server_name foo.example.com bar.example.com + baz.example.com qux.example.com; + """ + parsed = loads(test) + self.assertEqual(parsed, [ + ['server_name', 'foo.example.com', 'bar.example.com', + 'baz.example.com', 'qux.example.com'], + ['server_name', 'foo.example.com', 'bar.example.com', + 'baz.example.com', 'qux.example.com'] + ]) + + # variable weirdness + parsed = loads("directive $var;") + self.assertEqual(parsed, [['directive', '$var']]) + self.assertRaises(ParseException, loads, "server {server_name test.com};") + self.assertRaises(ParseException, loads, "directive ${var};") + + class TestUnspacedList(unittest.TestCase): """Test the UnspacedList data structure""" def setUp(self): @@ -237,18 +402,18 @@ class TestUnspacedList(unittest.TestCase): ['\n ', 'listen', ' ', '127.0.0.1'], ['\n ', 'server_name', ' ', '.example.com'], ['\n ', 'server_name', ' ', 'example.*'], '\n', - ['listen', ' ', '5001 ssl']]) + ['listen', ' ', '5001', ' ', 'ssl']]) x.insert(5, "FROGZ") self.assertEqual(x, [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'], 'FROGZ']) + ['listen', '5001', 'ssl'], 'FROGZ']) self.assertEqual(x.spaced, [['\n ', 'listen', ' ', '69.50.225.155:9000'], ['\n ', 'listen', ' ', '127.0.0.1'], ['\n ', 'server_name', ' ', '.example.com'], ['\n ', 'server_name', ' ', 'example.*'], '\n', - ['listen', ' ', '5001 ssl'], + ['listen', ' ', '5001', ' ', 'ssl'], 'FROGZ']) def test_rawlists(self): diff --git a/certbot-nginx/certbot_nginx/tests/obj_test.py b/certbot-nginx/certbot_nginx/tests/obj_test.py index 069f860d6..ba136bb78 100644 --- a/certbot-nginx/certbot_nginx/tests/obj_test.py +++ b/certbot-nginx/certbot_nginx/tests/obj_test.py @@ -108,8 +108,8 @@ class VirtualHostTest(unittest.TestCase): from certbot_nginx.obj import Addr raw1 = [ ['listen', '69.50.225.155:9000'], - [['if', '($scheme != "https") '], - [['return', '301 https://$host$request_uri']] + [['if', '($scheme', '!=', '"https") '], + [['return', '301', 'https://$host$request_uri']] ], ['#', ' managed by Certbot'] ] @@ -119,8 +119,8 @@ class VirtualHostTest(unittest.TestCase): set(['localhost']), raw1, []) raw2 = [ ['listen', '69.50.225.155:9000'], - [['if', '($scheme != "https") '], - [['return', '301 https://$host$request_uri']] + [['if', '($scheme', '!=', '"https") '], + [['return', '301', 'https://$host$request_uri']] ] ] self.vhost2 = VirtualHost( @@ -129,7 +129,7 @@ class VirtualHostTest(unittest.TestCase): set(['localhost']), raw2, []) raw3 = [ ['listen', '69.50.225.155:9000'], - ['rewrite', '^(.*)$ $scheme://www.domain.com$1 permanent;'] + ['rewrite', '^(.*)$', '$scheme://www.domain.com$1', 'permanent'] ] self.vhost3 = VirtualHost( "filep", @@ -181,7 +181,9 @@ class VirtualHostTest(unittest.TestCase): ['#', ' managed by Certbot'], ['ssl_certificate_key', '/etc/letsencrypt/live/two.functorkitten.xyz/privkey.pem'], ['#', ' managed by Certbot'], - [['if', '($scheme != "https")'], [['return', '301 https://$host$request_uri']]], + [['if', '($scheme', '!=', '"https")'], + [['return', '301', 'https://$host$request_uri']] + ], ['#', ' managed by Certbot'], []] vhost_haystack = VirtualHost( "filp", @@ -195,7 +197,9 @@ class VirtualHostTest(unittest.TestCase): ['#', ' managed by Certbot'], ['ssl_certificate_key', '/etc/letsencrypt/live/two.functorkitten.xyz/privkey.pem'], ['#', ' managed by Certbot'], - [['if', '($scheme != "https")'], [['return', '302 https://$host$request_uri']]], + [['if', '($scheme', '!=', '"https")'], + [['return', '302', 'https://$host$request_uri']] + ], ['#', ' managed by Certbot'], []] vhost_bad_haystack = VirtualHost( "filp", diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 6a3f2f1de..8d20faa38 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -52,7 +52,7 @@ class NginxParserTest(util.NginxTest): 'sites-enabled/sslon.com', 'sites-enabled/globalssl.com']]), set(nparser.parsed.keys())) - self.assertEqual([['server_name', 'somename alias another.alias']], + self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']], nparser.parsed[nparser.abs_path('server.conf')]) self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], @@ -168,16 +168,16 @@ class NginxParserTest(util.NginxTest): [['location', '/'], [['root', 'html'], ['index', 'index.html index.htm']]] ], None) self.assertFalse(nparser.has_ssl_on_directive(mock_vhost)) - mock_vhost.raw = [['listen', '*:80 default_server ssl'], - ['server_name', '*.www.foo.com *.www.example.com'], + mock_vhost.raw = [['listen', '*:80', 'default_server', 'ssl'], + ['server_name', '*.www.foo.com', '*.www.example.com'], ['root', '/home/ubuntu/sites/foo/']] self.assertFalse(nparser.has_ssl_on_directive(mock_vhost)) mock_vhost.raw = [['listen', '80 ssl'], - ['server_name', '*.www.foo.com *.www.example.com']] + ['server_name', '*.www.foo.com', '*.www.example.com']] self.assertFalse(nparser.has_ssl_on_directive(mock_vhost)) mock_vhost.raw = [['listen', '80'], ['ssl', 'on'], - ['server_name', '*.www.foo.com *.www.example.com']] + ['server_name', '*.www.foo.com', '*.www.example.com']] self.assertTrue(nparser.has_ssl_on_directive(mock_vhost)) def test_add_server_directives(self): @@ -309,7 +309,7 @@ class NginxParserTest(util.NginxTest): self.assertFalse(server['ssl']) server = parser._parse_server_raw([ #pylint: disable=protected-access - ['listen', '443 ssl'] + ['listen', '443', 'ssl'] ]) self.assertTrue(server['ssl']) @@ -341,7 +341,7 @@ class NginxParserTest(util.NginxTest): self.assertEqual(nginxparser.UnspacedList(nparser.loc["ssl_options"]), [['ssl_session_cache', 'shared:le_nginx_SSL:1m'], ['ssl_session_timeout', '1440m'], - ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], + ['ssl_protocols', 'TLSv1', 'TLSv1.1', 'TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-'+ 'AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256'+ diff --git a/certbot-nginx/certbot_nginx/tls_sni_01.py b/certbot-nginx/certbot_nginx/tls_sni_01.py index eedd97c03..2e8125911 100644 --- a/certbot-nginx/certbot_nginx/tls_sni_01.py +++ b/certbot-nginx/certbot_nginx/tls_sni_01.py @@ -96,11 +96,12 @@ class NginxTlsSni01(common.TLSSNI01): bucket_directive = ['\n', 'server_names_hash_bucket_size', ' ', '128'] main = self.configurator.parser.parsed[root] - for key, body in main: - if key == ['http']: + for line in main: + if line[0] == ['http']: + body = line[1] found_bucket = False - for k, _ in body: - if k == bucket_directive[1]: + for inner_line in body: + if inner_line[0] == bucket_directive[1]: found_bucket = True if not found_bucket: body.insert(0, bucket_directive)