Create a new server block when making server block ssl (#5220)

* create_new_vhost_from_default --> duplicate_vhost * add source_path property * set source path for duplicated vhost * change around logic of where making ssl happens * don't add listen 80 to newly created ssl block * cache vhosts list * remove source path * add redirect block if we created a new server block * Remove listen directives when making server block ssl * Reset vhost cache on parser load * flip connected pointer direction for finding newly made server block to match previous redirect search constraints * also test for new redirect block styles * fix contains_list and test redirect blocks * update lint, parser, and obj tests * reset new vhost (fixing previous bug) and move removing default from addrs under if statement * reuse and update newly created ssl server block when appropriate, and update unit tests * append newly created server blocks to file instead of inserting directly after, so we don't have to update other vhosts' paths * add coverage for NO_IF_REDIRECT_COMMENT_BLOCK * add coverage for parser load calls * replace some double quotes with single quotes * replace backslash continuations with parentheses * update docstrings * switch to only creating a new block on redirect enhancement, including removing the get_vhosts cache * update configurator tests * update obj test * switch delete_default default for duplicate_vhost
2026-01-26 07:41:33 +03:00 · 2017-12-07 09:48:54 -08:00
parent d039106b68
commit 8b5d6879cc
7 changed files with 243 additions and 128 deletions
--- a/certbot-nginx/certbot_nginx/configurator.py
+++ b/certbot-nginx/certbot_nginx/configurator.py
@@ -23,43 +23,24 @@ from certbot import util
 from certbot.plugins import common

 from certbot_nginx import constants
-from certbot_nginx import tls_sni_01
+from certbot_nginx import nginxparser
 from certbot_nginx import parser
+from certbot_nginx import tls_sni_01


 logger = logging.getLogger(__name__)

-REDIRECT_BLOCK = [[
-    ['\n    ', 'if', ' ', '($scheme', ' ', '!=', ' ', '"https")'],
-    [['\n        ', 'return', ' ', '301', ' ', 'https://$host$request_uri'],
-     '\n    ']
-], ['\n']]
-
-TEST_REDIRECT_BLOCK = [
-    [
-        ['if', '($scheme', '!=', '"https")'],
-        [
-            ['return', '301', 'https://$host$request_uri']
-        ]
-    ],
-    ['#', ' managed by Certbot']
+REDIRECT_BLOCK = [
+    ['\n    ', 'return', ' ', '301', ' ', 'https://$host$request_uri'],
+    ['\n']
 ]

 REDIRECT_COMMENT_BLOCK = [
    ['\n    ', '#', ' Redirect non-https traffic to https'],
-    ['\n    ', '#', ' if ($scheme != "https") {'],
-    ['\n    ', '#', "     return 301 https://$host$request_uri;"],
-    ['\n    ', '#', " } # managed by Certbot"],
+    ['\n    ', '#', ' return 301 https://$host$request_uri;'],
    ['\n']
 ]

-TEST_REDIRECT_COMMENT_BLOCK = [
-    ['#', ' Redirect non-https traffic to https'],
-    ['#', ' if ($scheme != "https") {'],
-    ['#', "     return 301 https://$host$request_uri;"],
-    ['#', " } # managed by Certbot"],
-]
-
@zope.interface.implementer(interfaces.IAuthenticator, interfaces.IInstaller)
@zope.interface.provider(interfaces.IPluginFactory)
 class NginxConfigurator(common.Installer):
@@ -194,9 +175,7 @@ class NginxConfigurator(common.Installer):
                "The nginx plugin currently requires --fullchain-path to "
                "install a cert.")

-        vhost = self.choose_vhost(domain, raise_if_no_match=False)
-        if vhost is None:
-            vhost = self._vhost_from_duplicated_default(domain)
+        vhost = self.choose_vhost(domain, create_if_no_match=True)
        cert_directives = [['\n    ', 'ssl_certificate', ' ', fullchain_path],
                           ['\n    ', 'ssl_certificate_key', ' ', key_path]]

@@ -214,7 +193,7 @@ class NginxConfigurator(common.Installer):
    #######################
    # Vhost parsing methods
    #######################
-    def choose_vhost(self, target_name, raise_if_no_match=True):
+    def choose_vhost(self, target_name, create_if_no_match=False):
        """Chooses a virtual host based on the given domain name.

        .. note:: This makes the vhost SSL-enabled if it isn't already. Follows
@@ -228,8 +207,8 @@ class NginxConfigurator(common.Installer):
            hostname. Currently we just ignore this.

        :param str target_name: domain name
-        :param bool raise_if_no_match: True iff not finding a match is an error;
-                                       otherwise, return None
+        :param bool create_if_no_match: If we should create a new vhost from default
+            when there is no match found

        :returns: ssl vhost associated with name
        :rtype: :class:`~certbot_nginx.obj.VirtualHost`
@@ -240,7 +219,9 @@ class NginxConfigurator(common.Installer):
        matches = self._get_ranked_matches(target_name)
        vhost = self._select_best_name_match(matches)
        if not vhost:
-            if raise_if_no_match:
+            if create_if_no_match:
+                vhost = self._vhost_from_duplicated_default(target_name)
+            else:
                # No matches. Raise a misconfiguration error.
                raise errors.MisconfigurationError(
                            ("Cannot find a VirtualHost matching domain %s. "
@@ -248,16 +229,12 @@ class NginxConfigurator(common.Installer):
                             "please add a corresponding server_name directive to your "
                             "nginx configuration: "
                             "https://nginx.org/en/docs/http/server_names.html") % (target_name))
-            else:
-                return None
-        else:
-            # Note: if we are enhancing with ocsp, vhost should already be ssl.
-            if not vhost.ssl:
-                self._make_server_ssl(vhost)
+        # Note: if we are enhancing with ocsp, vhost should already be ssl.
+        if not vhost.ssl:
+            self._make_server_ssl(vhost)

        return vhost

-
    def ipv6_info(self, port):
        """Returns tuple of booleans (ipv6_active, ipv6only_present)
        ipv6_active is true if any server block listens ipv6 address in any port
@@ -285,18 +262,19 @@ class NginxConfigurator(common.Installer):
    def _vhost_from_duplicated_default(self, domain):
        if self.new_vhost is None:
            default_vhost = self._get_default_vhost()
-            self.new_vhost = self.parser.create_new_vhost_from_default(default_vhost)
-            if not self.new_vhost.ssl:
-                self._make_server_ssl(self.new_vhost)
+            self.new_vhost = self.parser.duplicate_vhost(default_vhost, delete_default=True)
            self.new_vhost.names = set()

-        self.new_vhost.names.add(domain)
+        self._add_server_name_to_vhost(self.new_vhost, domain)
+        return self.new_vhost
+
+    def _add_server_name_to_vhost(self, vhost, domain):
+        vhost.names.add(domain)
        name_block = [['\n    ', 'server_name']]
-        for name in self.new_vhost.names:
+        for name in vhost.names:
            name_block[0].append(' ')
            name_block[0].append(name)
-        self.parser.add_server_directives(self.new_vhost, name_block, replace=True)
-        return self.new_vhost
+        self.parser.add_server_directives(vhost, name_block, replace=True)

    def _get_default_vhost(self):
        vhost_list = self.parser.get_vhosts()
@@ -505,11 +483,7 @@ class NginxConfigurator(common.Installer):
    def _make_server_ssl(self, vhost):
        """Make a server SSL.

-        Make a server SSL based on server_name and filename by adding a
-        ``listen IConfig.tls_sni_01_port ssl`` directive to the server block.
-
-        .. todo:: Maybe this should create a new block instead of modifying
-            the existing one?
+        Make a server SSL by adding new listen and SSL directives.

        :param vhost: The vhost to add SSL to.
        :type vhost: :class:`~certbot_nginx.obj.VirtualHost`
@@ -529,7 +503,9 @@ class NginxConfigurator(common.Installer):
            ipv6_block = ['\n    ',
                          'listen',
                          ' ',
-                          '[::]:{0} ssl'.format(self.config.tls_sni_01_port)]
+                          '[::]:{0}'.format(self.config.tls_sni_01_port),
+                          ' ',
+                          'ssl']
            if not ipv6info[1]:
                # ipv6only=on is absent in global config
                ipv6_block.append(' ')
@@ -539,8 +515,9 @@ class NginxConfigurator(common.Installer):
            ipv4_block = ['\n    ',
                          'listen',
                          ' ',
-                          '{0} ssl'.format(self.config.tls_sni_01_port)]
-
+                          '{0}'.format(self.config.tls_sni_01_port),
+                          ' ',
+                          'ssl']

        snakeoil_cert, snakeoil_key = self._get_snakeoil_paths()

@@ -584,10 +561,12 @@ class NginxConfigurator(common.Installer):
            raise

    def _has_certbot_redirect(self, vhost):
-        return vhost.contains_list(TEST_REDIRECT_BLOCK)
+        test_redirect_block = _test_block_from_block(REDIRECT_BLOCK)
+        return vhost.contains_list(test_redirect_block)

    def _has_certbot_redirect_comment(self, vhost):
-        return vhost.contains_list(TEST_REDIRECT_COMMENT_BLOCK)
+        test_redirect_comment_block = _test_block_from_block(REDIRECT_COMMENT_BLOCK)
+        return vhost.contains_list(test_redirect_comment_block)

    def _add_redirect_block(self, vhost, active=True):
        """Add redirect directive to vhost
@@ -603,7 +582,8 @@ class NginxConfigurator(common.Installer):
    def _enable_redirect(self, domain, unused_options):
        """Redirect all equivalent HTTP traffic to ssl_vhost.

-        Add rewrite directive to non https traffic
+        If the vhost is listening plaintextishly, separate out the
+        relevant directives into a new server block and add a rewrite directive.

        .. note:: This function saves the configuration

@@ -616,26 +596,46 @@ class NginxConfigurator(common.Installer):
        vhost = None
        # If there are blocks listening plaintextishly on self.DEFAULT_LISTEN_PORT,
        # choose the most name-matching one.
+
        vhost = self.choose_redirect_vhost(domain, port)

        if vhost is None:
            logger.info("No matching insecure server blocks listening on port %s found.",
                self.DEFAULT_LISTEN_PORT)
+            return
+
+        if vhost.ssl:
+            new_vhost = self.parser.duplicate_vhost(vhost,
+                only_directives=['listen', 'server_name'])
+
+            def _ssl_match_func(directive):
+                return 'ssl' in directive
+
+            def _no_ssl_match_func(directive):
+                return 'ssl' not in directive
+
+            # remove all ssl addresses from the new block
+            self.parser.remove_server_directives(new_vhost, 'listen', match_func=_ssl_match_func)
+
+            # remove all non-ssl addresses from the existing block
+            self.parser.remove_server_directives(vhost, 'listen', match_func=_no_ssl_match_func)
+
+            vhost = new_vhost
+
+        if self._has_certbot_redirect(vhost):
+            logger.info("Traffic on port %s already redirecting to ssl in %s",
+                self.DEFAULT_LISTEN_PORT, vhost.filep)
+        elif vhost.has_redirect():
+            if not self._has_certbot_redirect_comment(vhost):
+                self._add_redirect_block(vhost, active=False)
+            logger.info("The appropriate server block is already redirecting "
+                        "traffic. To enable redirect anyway, uncomment the "
+                        "redirect lines in %s.", vhost.filep)
        else:
-            if self._has_certbot_redirect(vhost):
-                logger.info("Traffic on port %s already redirecting to ssl in %s",
-                    self.DEFAULT_LISTEN_PORT, vhost.filep)
-            elif vhost.has_redirect():
-                if not self._has_certbot_redirect_comment(vhost):
-                    self._add_redirect_block(vhost, active=False)
-                logger.info("The appropriate server block is already redirecting "
-                            "traffic. To enable redirect anyway, uncomment the "
-                            "redirect lines in %s.", vhost.filep)
-            else:
-                # Redirect plaintextish host to https
-                self._add_redirect_block(vhost, active=True)
-                logger.info("Redirecting all traffic on port %s to ssl in %s",
-                    self.DEFAULT_LISTEN_PORT, vhost.filep)
+            # Redirect plaintextish host to https
+            self._add_redirect_block(vhost, active=True)
+            logger.info("Redirecting all traffic on port %s to ssl in %s",
+                self.DEFAULT_LISTEN_PORT, vhost.filep)

    def _enable_ocsp_stapling(self, domain, chain_path):
        """Include OCSP response in TLS handshake
@@ -809,6 +809,7 @@ class NginxConfigurator(common.Installer):

        """
        super(NginxConfigurator, self).recovery_routine()
+        self.new_vhost = None
        self.parser.load()

    def revert_challenge_config(self):
@@ -818,6 +819,7 @@ class NginxConfigurator(common.Installer):

        """
        self.revert_temporary_config()
+        self.new_vhost = None
        self.parser.load()

    def rollback_checkpoints(self, rollback=1):
@@ -830,6 +832,7 @@ class NginxConfigurator(common.Installer):

        """
        super(NginxConfigurator, self).rollback_checkpoints(rollback)
+        self.new_vhost = None
        self.parser.load()

    ###########################################################################
@@ -882,6 +885,11 @@ class NginxConfigurator(common.Installer):
            self.restart()


+def _test_block_from_block(block):
+    test_block = nginxparser.UnspacedList(block)
+    parser.comment_directive(test_block, 0)
+    return test_block[:-1]
+
 def nginx_restart(nginx_ctl, nginx_conf):
    """Restarts the Nginx Server.

--- a/certbot-nginx/certbot_nginx/obj.py
+++ b/certbot-nginx/certbot_nginx/obj.py
@@ -205,7 +205,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
    def contains_list(self, test):
        """Determine if raw server block contains test list at top level
        """
-        for i in six.moves.range(0, len(self.raw) - len(test)):
+        for i in six.moves.range(0, len(self.raw) - len(test) + 1):
            if self.raw[i:i + len(test)] == test:
                return True
        return False
@@ -220,6 +220,8 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
    def ipv4_enabled(self):
        """Return true if one or more of the listen directives in vhost are IPv4
        only"""
+        if self.addrs is None or len(self.addrs) == 0:
+            return True
        for a in self.addrs:
            if not a.ipv6:
                return True
--- a/certbot-nginx/certbot_nginx/parser.py
+++ b/certbot-nginx/certbot_nginx/parser.py
@@ -1,5 +1,6 @@
 """NginxParser is a member object of the NginxConfigurator class."""
 import copy
+import functools
 import glob
 import logging
 import os
@@ -294,6 +295,30 @@ class NginxParser(object):
        :param bool replace: Whether to only replace existing directives

        """
+        self._modify_server_directives(vhost,
+            functools.partial(_add_directives, directives, replace))
+
+    def remove_server_directives(self, vhost, directive_name, match_func=None):
+        """Remove all directives of type directive_name.
+
+        :param :class:`~certbot_nginx.obj.VirtualHost` vhost: The vhost
+            to remove directives from
+        :param string directive_name: The directive type to remove
+        :param callable match_func: Function of the directive that returns true for directives
+            to be deleted.
+        """
+        self._modify_server_directives(vhost,
+            functools.partial(_remove_directives, directive_name, match_func))
+
+    def _update_vhost_based_on_new_directives(self, vhost, directives_list):
+        new_server = self._get_included_directives(directives_list)
+        parsed_server = self.parse_server(new_server)
+        vhost.addrs = parsed_server['addrs']
+        vhost.ssl = parsed_server['ssl']
+        vhost.names = parsed_server['names']
+        vhost.raw = new_server
+
+    def _modify_server_directives(self, vhost, block_func):
        filename = vhost.filep
        try:
            result = self.parsed[filename]
@@ -302,42 +327,52 @@ class NginxParser(object):
            if not isinstance(result, list) or len(result) != 2:
                raise errors.MisconfigurationError("Not a server block.")
            result = result[1]
-            _add_directives(result, directives, replace)
+            block_func(result)

-            # update vhost based on new directives
-            new_server = self._get_included_directives(result)
-            parsed_server = self.parse_server(new_server)
-            vhost.addrs = parsed_server['addrs']
-            vhost.ssl = parsed_server['ssl']
-            vhost.names = parsed_server['names']
-            vhost.raw = new_server
+            self._update_vhost_based_on_new_directives(vhost, result)
        except errors.MisconfigurationError as err:
            raise errors.MisconfigurationError("Problem in %s: %s" % (filename, str(err)))

-    def create_new_vhost_from_default(self, vhost_template):
-        """Duplicate the default vhost in the configuration files.
+    def duplicate_vhost(self, vhost_template, delete_default=False, only_directives=None):
+        """Duplicate the vhost in the configuration files.

        :param :class:`~certbot_nginx.obj.VirtualHost` vhost_template: The vhost
            whose information we copy
+        :param bool delete_default: If we should remove default_server
+            from listen directives in the block.
+        :param list only_directives: If it exists, only duplicate the named directives. Only
+            looks at first level of depth; does not expand includes.

        :returns: A vhost object for the newly created vhost
        :rtype: :class:`~certbot_nginx.obj.VirtualHost`
        """
        # TODO: https://github.com/certbot/certbot/issues/5185
        # put it in the same file as the template, at the same level
+        new_vhost = copy.deepcopy(vhost_template)
+
        enclosing_block = self.parsed[vhost_template.filep]
        for index in vhost_template.path[:-1]:
            enclosing_block = enclosing_block[index]
-        new_location = vhost_template.path[-1] + 1
        raw_in_parsed = copy.deepcopy(enclosing_block[vhost_template.path[-1]])
-        enclosing_block.insert(new_location, raw_in_parsed)
-        new_vhost = copy.deepcopy(vhost_template)
-        new_vhost.path[-1] = new_location
-        for addr in new_vhost.addrs:
-            addr.default = False
-        for directive in enclosing_block[new_vhost.path[-1]][1]:
-            if len(directive) > 0 and directive[0] == 'listen' and 'default_server' in directive:
-                del directive[directive.index('default_server')]
+
+        if only_directives is not None:
+            new_directives = nginxparser.UnspacedList([])
+            for directive in raw_in_parsed[1]:
+                if len(directive) > 0 and directive[0] in only_directives:
+                    new_directives.append(directive)
+            raw_in_parsed[1] = new_directives
+
+            self._update_vhost_based_on_new_directives(new_vhost, new_directives)
+
+        enclosing_block.append(raw_in_parsed)
+        new_vhost.path[-1] = len(enclosing_block) - 1
+        if delete_default:
+            for addr in new_vhost.addrs:
+                addr.default = False
+            for directive in enclosing_block[new_vhost.path[-1]][1]:
+                if (len(directive) > 0 and directive[0] == 'listen'
+                    and 'default_server' in directive):
+                    del directive[directive.index('default_server')]
        return new_vhost

 def _parse_ssl_options(ssl_options):
@@ -486,7 +521,7 @@ def _is_ssl_on_directive(entry):
            len(entry) == 2 and entry[0] == 'ssl' and
            entry[1] == 'on')

-def _add_directives(block, directives, replace):
+def _add_directives(directives, replace, block):
    """Adds or replaces directives in a config block.

    When replace=False, it's an error to try and add a directive that already
@@ -498,8 +533,9 @@ def _add_directives(block, directives, replace):

    ..todo :: Find directives that are in included files.

-    :param list block: The block to replace in
    :param list directives: The new directives.
+    :param bool replace: Described above.
+    :param list block: The block to replace in

    """
    for directive in directives:
@@ -513,8 +549,12 @@ REPEATABLE_DIRECTIVES = set(['server_name', 'listen', INCLUDE])
 COMMENT = ' managed by Certbot'
 COMMENT_BLOCK = [' ', '#', COMMENT]

-def _comment_directive(block, location):
-    """Add a comment to the end of the line at location."""
+def comment_directive(block, location):
+    """Add a ``#managed by Certbot`` comment to the end of the line at location.
+
+    :param list block: The block containing the directive to be commented
+    :param int location: The location within ``block`` of the directive to be commented
+    """
    next_entry = block[location + 1] if location + 1 < len(block) else None
    if isinstance(next_entry, list) and next_entry:
        if len(next_entry) >= 2 and next_entry[-2] == "#" and COMMENT in next_entry[-1]:
@@ -551,6 +591,12 @@ def _comment_out_directive(block, location, include_location):

    block[location] = new_dir[0] # set the now-single-line-comment directive back in place

+def _find_location(block, directive_name, match_func=None):
+    """Finds the index of the first instance of directive_name in block.
+       If no line exists, use None."""
+    return next((index for index, line in enumerate(block) \
+        if line and line[0] == directive_name and (match_func is None or match_func(line))), None)
+
 def _add_directive(block, directive, replace):
    """Adds or replaces a single directive in a config block.

@@ -566,19 +612,12 @@ def _add_directive(block, directive, replace):
        block.append(directive)
        return

-    def find_location(direc):
-        """ Find the index of a config line where the name of the directive matches
-        the name of the directive we want to add. If no line exists, use None.
-        """
-        return next((index for index, line in enumerate(block) \
-            if line and line[0] == direc[0]), None)
-
-    location = find_location(directive)
+    location = _find_location(block, directive[0])

    if replace:
        if location is not None:
            block[location] = directive
-            _comment_directive(block, location)
+            comment_directive(block, location)
            return
    # Append directive. Fail if the name is not a repeatable directive name,
    # and there is already a copy of that directive with a different value
@@ -602,7 +641,7 @@ def _add_directive(block, directive, replace):
        included_directives = _parse_ssl_options(directive[1])

        for included_directive in included_directives:
-            included_dir_loc = find_location(included_directive)
+            included_dir_loc = _find_location(block, included_directive[0])
            included_dir_name = included_directive[0]
            if not is_whitespace_or_comment(included_directive) \
                and not can_append(included_dir_loc, included_dir_name):
@@ -614,10 +653,19 @@ def _add_directive(block, directive, replace):

    if can_append(location, directive_name):
        block.append(directive)
-        _comment_directive(block, len(block) - 1)
+        comment_directive(block, len(block) - 1)
    elif block[location] != directive:
        raise errors.MisconfigurationError(err_fmt.format(directive, block[location]))

+def _remove_directives(directive_name, match_func, block):
+    """Removes directives of name directive_name from a config block if match_func matches.
+    """
+    while True:
+        location = _find_location(block, directive_name, match_func=match_func)
+        if location is None:
+            return
+        del block[location]
+
 def _apply_global_addr_ssl(addr_to_ssl, parsed_server):
    """Apply global sslishness information to the parsed server block
    """
--- a/certbot-nginx/certbot_nginx/tests/configurator_test.py
+++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py
@@ -443,10 +443,7 @@ class NginxConfiguratorTest(util.NginxTest):
    def test_redirect_enhance(self):
        # Test that we successfully add a redirect when there is
        # a listen directive
-        expected = [
-            ['if', '($scheme', '!=', '"https")'],
-            [['return', '301', 'https://$host$request_uri']]
-        ]
+        expected = ['return', '301', 'https://$host$request_uri']

        example_conf = self.config.parser.abs_path('sites-enabled/example.com')
        self.config.enhance("www.example.com", "redirect")
@@ -462,6 +459,35 @@ class NginxConfiguratorTest(util.NginxTest):
        generated_conf = self.config.parser.parsed[migration_conf]
        self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))

+    def test_split_for_redirect(self):
+        example_conf = self.config.parser.abs_path('sites-enabled/example.com')
+        self.config.deploy_cert(
+            "example.org",
+            "example/cert.pem",
+            "example/key.pem",
+            "example/chain.pem",
+            "example/fullchain.pem")
+        self.config.enhance("www.example.com", "redirect")
+        generated_conf = self.config.parser.parsed[example_conf]
+        self.assertEqual(
+            [[['server'], [
+               ['server_name', '.example.com'],
+               ['server_name', 'example.*'], [],
+               ['listen', '5001', 'ssl'], ['#', ' managed by Certbot'],
+               ['ssl_certificate', 'example/fullchain.pem'], ['#', ' managed by Certbot'],
+               ['ssl_certificate_key', 'example/key.pem'], ['#', ' managed by Certbot'],
+               ['include', self.config.mod_ssl_conf], ['#', ' managed by Certbot'],
+               ['ssl_dhparam', self.config.ssl_dhparams], ['#', ' managed by Certbot'],
+               [], []]],
+             [['server'], [
+               ['listen', '69.50.225.155:9000'],
+               ['listen', '127.0.0.1'],
+               ['server_name', '.example.com'],
+               ['server_name', 'example.*'],
+               ['return', '301', 'https://$host$request_uri'], ['#', ' managed by Certbot'],
+               [], []]]],
+            generated_conf)
+
    @mock.patch('certbot_nginx.obj.VirtualHost.contains_list')
    @mock.patch('certbot_nginx.obj.VirtualHost.has_redirect')
    def test_certbot_redirect_exists(self, mock_has_redirect, mock_contains_list):
@@ -494,9 +520,38 @@ class NginxConfiguratorTest(util.NginxTest):
            generated_conf = self.config.parser.parsed[example_conf]
            expected = [
                ['#', ' Redirect non-https traffic to https'],
-                ['#', ' if ($scheme != "https") {'],
-                ['#', '     return 301 https://$host$request_uri;'],
-                ['#', ' } # managed by Certbot']
+                ['#', ' return 301 https://$host$request_uri;'],
+            ]
+            for line in expected:
+                self.assertTrue(util.contains_at_depth(generated_conf, line, 2))
+
+    @mock.patch('certbot_nginx.obj.VirtualHost.contains_list')
+    @mock.patch('certbot_nginx.obj.VirtualHost.has_redirect')
+    def test_non_certbot_redirect_exists_has_ssl_copy(self, mock_has_redirect, mock_contains_list):
+        # Test that we add a redirect as a comment if there is already a
+        # redirect-class statement in the block that isn't managed by certbot
+        example_conf = self.config.parser.abs_path('sites-enabled/example.com')
+
+        self.config.deploy_cert(
+            "example.org",
+            "example/cert.pem",
+            "example/key.pem",
+            "example/chain.pem",
+            "example/fullchain.pem")
+
+        # Has a non-Certbot redirect, and has no existing comment
+        mock_contains_list.return_value = False
+        mock_has_redirect.return_value = True
+        with mock.patch("certbot_nginx.configurator.logger") as mock_logger:
+            self.config.enhance("www.example.com", "redirect")
+            self.assertEqual(mock_logger.info.call_args[0][0],
+                "The appropriate server block is already redirecting "
+                "traffic. To enable redirect anyway, uncomment the "
+                "redirect lines in %s.")
+            generated_conf = self.config.parser.parsed[example_conf]
+            expected = [
+                ['#', ' Redirect non-https traffic to https'],
+                ['#', ' return 301 https://$host$request_uri;'],
            ]
            for line in expected:
                self.assertTrue(util.contains_at_depth(generated_conf, line, 2))
@@ -704,14 +759,18 @@ class NginxConfiguratorTest(util.NginxTest):

        self.config.parser.load()

-        expected = [
-            ['if', '($scheme', '!=', '"https")'],
-            [['return', '301', 'https://$host$request_uri']]
-        ]
+        expected = ['return', '301', 'https://$host$request_uri']

        generated_conf = self.config.parser.parsed[default_conf]
        self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))

+    @mock.patch('certbot.reverter.logger')
+    @mock.patch('certbot_nginx.parser.NginxParser.load')
+    def test_parser_reload_after_config_changes(self, mock_parser_load, unused_mock_logger):
+        self.config.recovery_routine()
+        self.config.revert_challenge_config()
+        self.config.rollback_checkpoints()
+        self.assertTrue(mock_parser_load.call_count == 3)

 class InstallSslOptionsConfTest(util.NginxTest):
    """Test that the options-ssl-nginx.conf file is installed and updated properly."""
--- a/certbot-nginx/certbot_nginx/tests/obj_test.py
+++ b/certbot-nginx/certbot_nginx/tests/obj_test.py
@@ -171,8 +171,8 @@ class VirtualHostTest(unittest.TestCase):
    def test_contains_list(self):
        from certbot_nginx.obj import VirtualHost
        from certbot_nginx.obj import Addr
-        from certbot_nginx.configurator import TEST_REDIRECT_BLOCK
-        test_needle = TEST_REDIRECT_BLOCK
+        from certbot_nginx.configurator import REDIRECT_BLOCK, _test_block_from_block
+        test_needle = _test_block_from_block(REDIRECT_BLOCK)
        test_haystack = [['listen', '80'], ['root', '/var/www/html'],
            ['index', 'index.html index.htm index.nginx-debian.html'],
            ['server_name', 'two.functorkitten.xyz'], ['listen', '443 ssl'],
@@ -181,9 +181,7 @@ 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']]
-            ],
+            ['return', '301', 'https://$host$request_uri'],
            ['#', ' managed by Certbot'], []]
        vhost_haystack = VirtualHost(
            "filp",
--- a/certbot-nginx/certbot_nginx/tests/parser_test.py
+++ b/certbot-nginx/certbot_nginx/tests/parser_test.py
@@ -334,9 +334,9 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
            ["\n", "a", " ", "b", "\n"],
            ["c", " ", "d"],
            ["\n", "e", " ", "f"]])
-        from certbot_nginx.parser import _comment_directive, COMMENT_BLOCK
-        _comment_directive(block, 1)
-        _comment_directive(block, 0)
+        from certbot_nginx.parser import comment_directive, COMMENT_BLOCK
+        comment_directive(block, 1)
+        comment_directive(block, 0)
        self.assertEqual(block.spaced, [
            ["\n", "a", " ", "b", "\n"],
            COMMENT_BLOCK,
@@ -406,12 +406,12 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
        ])
        self.assertTrue(server['ssl'])

-    def test_create_new_vhost_from_default(self):
+    def test_duplicate_vhost(self):
        nparser = parser.NginxParser(self.config_path)

        vhosts = nparser.get_vhosts()
        default = [x for x in vhosts if 'default' in x.filep][0]
-        new_vhost = nparser.create_new_vhost_from_default(default)
+        new_vhost = nparser.duplicate_vhost(default, delete_default=True)
        nparser.filedump(ext='')

        # check properties of new vhost
--- a/certbot-nginx/certbot_nginx/tls_sni_01.py
+++ b/certbot-nginx/certbot_nginx/tls_sni_01.py
@@ -55,7 +55,7 @@ class NginxTlsSni01(common.TLSSNI01):
            self.configurator.config.tls_sni_01_port)

        for achall in self.achalls:
-            vhost = self.configurator.choose_vhost(achall.domain, raise_if_no_match=False)
+            vhost = self.configurator.choose_vhost(achall.domain, create_if_no_match=True)

            if vhost is not None and vhost.addrs:
                addresses.append(list(vhost.addrs))