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

8 files changed, 271 insertions, 143 deletions

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))
-
-    location = CharsNotIn("{};," + string.whitespace)
-    # modifier for location uri [ = | ~ | ~* | ^~ ]
-    modifier = Literal("=") | Literal("~*") | Literal("~") | Literal("^~")
-
-    # rules
-    comment = space + Literal('#') + restOfLine
+    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.
 
-    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)
+    token = paren_quote_extend | tokenchars | quoted
 
-    block = Forward()
+    whitespace_token_group = space + token + ZeroOrMore(required_space + token) + space
+    assignment = whitespace_token_group + semicolon
 
-    # key could for instance be "server" or "http", or "location" (in which case
-    # location_statement needs to have a non-empty location)
+    comment = space + Literal('#') + restOfLine
 
-    block_begin = (Group(space + key + location_statement) ^
-                   Group(if_statement) ^
-                   Group(charset_map_statement)).leaveWhitespace()
+    block = Forward()
 
-    block_innards = Group(ZeroOrMore(Group(comment | assignment) | block | map_block)
-                          + space).leaveWhitespace()
+    # order matters! see issue 518, and also http { # server { \n}
+    contents = Group(comment) | Group(block) | Group(assignment)
 
-    block << Group(block_begin + left_bracket + block_innards + right_bracket)
+    block_begin = Group(whitespace_token_group)
+    block_innards = Group(ZeroOrMore(contents) + space).leaveWhitespace()
+    block << 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)