Update Nginx redirect enhancement process to modify appropriate blocks (#3546)

* Cache the vhost we find during nginx deployment for OCSP enhancement.

* Refactor to pass domain into enhancement functions

* Add https redirect to most name-matching block listening non-sslishly.

* Redirect enhancement chooses the vhost most closely matching target_name that is listening to port 80 without using ssl.

* Add default listen 80 directive when it is implicitly defined

5 files changed, 259 insertions, 35 deletions

diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py
index 0a23f1f07..bfc7b6a67 100644
--- a/certbot-nginx/certbot_nginx/configurator.py
+++ b/certbot-nginx/certbot_nginx/configurator.py
@@ -58,6 +58,8 @@ class NginxConfigurator(common.Plugin):
 
     hidden = True
 
+    DEFAULT_LISTEN_PORT = '80'
+
     @classmethod
     def add_parser_arguments(cls, add):
         add("server-root", default=constants.CLI_DEFAULTS["server_root"],
@@ -196,19 +198,13 @@ class NginxConfigurator(common.Plugin):
         vhost = None
 
         matches = self._get_ranked_matches(target_name)
-        if not matches:
+        vhost = self._select_best_name_match(matches)
+        if not vhost:
             # No matches. Raise a misconfiguration error.
             raise errors.MisconfigurationError(
                         "Cannot find a VirtualHost matching domain %s." % (target_name))
-        elif matches[0]['rank'] in xrange(2, 6):
-            # Wildcard match - need to find the longest one
-            rank = matches[0]['rank']
-            wildcards = [x for x in matches if x['rank'] == rank]
-            vhost = max(wildcards, key=lambda x: len(x['name']))['vhost']
         else:
-            vhost = matches[0]['vhost']
-
-        if vhost is not None:
+            # Note: if we are enhancing with ocsp, vhost should already be ssl.
             if not vhost.ssl:
                 self._make_server_ssl(vhost)
 
@@ -224,13 +220,48 @@ class NginxConfigurator(common.Plugin):
         :rtype: list
 
         """
+        vhost_list = self.parser.get_vhosts()
+        return self._rank_matches_by_name_and_ssl(vhost_list, target_name)
+
+    def _select_best_name_match(self, matches):
+        """Returns the best name match of a ranked list of vhosts.
+
+        :param list matches: list of dicts containing the vhost, the matching name,
+            and the numerical rank
+        :returns: the most matching vhost
+        :rtype: :class:`~certbot_nginx.obj.VirtualHost`
+
+        """
+        if not matches:
+            return None
+        elif matches[0]['rank'] in xrange(2, 6):
+            # Wildcard match - need to find the longest one
+            rank = matches[0]['rank']
+            wildcards = [x for x in matches if x['rank'] == rank]
+            return max(wildcards, key=lambda x: len(x['name']))['vhost']
+        else:
+            # Exact or regex match
+            return matches[0]['vhost']
+
+
+    def _rank_matches_by_name_and_ssl(self, vhost_list, target_name):
+        """Returns a ranked list of vhosts from vhost_list that match target_name.
+        The ranking gives preference to SSL vhosts.
+
+        :param list vhost_list: list of vhosts to filter and rank
+        :param str target_name: The name to match
+        :returns: list of dicts containing the vhost, the matching name, and
+            the numerical rank
+        :rtype: list
+
+        """
         # Nginx chooses a matching server name for a request with precedence:
         # 1. exact name match
         # 2. longest wildcard name starting with *
         # 3. longest wildcard name ending with *
         # 4. first matching regex in order of appearance in the file
         matches = []
-        for vhost in self.parser.get_vhosts():
+        for vhost in vhost_list:
             name_type, name = parser.get_best_match(target_name, vhost.names)
             if name_type == 'exact':
                 matches.append({'vhost': vhost,
@@ -250,6 +281,73 @@ class NginxConfigurator(common.Plugin):
                                 'rank': 6 if vhost.ssl else 7})
         return sorted(matches, key=lambda x: x['rank'])
 
+
+    def choose_redirect_vhost(self, target_name, port):
+        """Chooses a single virtual host for redirect enhancement.
+
+        Chooses the vhost most closely matching target_name that is
+        listening to port without using ssl.
+
+        .. todo:: This should maybe return list if no obvious answer
+            is presented.
+
+        .. todo:: The special name "$hostname" corresponds to the machine's
+            hostname. Currently we just ignore this.
+
+        :param str target_name: domain name
+        :param str port: port number
+        :returns: vhost associated with name
+        :rtype: :class:`~certbot_nginx.obj.VirtualHost`
+
+        """
+        matches = self._get_redirect_ranked_matches(target_name, port)
+        return self._select_best_name_match(matches)
+
+    def _get_redirect_ranked_matches(self, target_name, port):
+        """Gets a ranked list of plaintextish port-listening vhosts matching target_name
+
+        Filter all hosts for those listening on port without using ssl.
+        Rank by how well these match target_name.
+
+        :param str target_name: The name to match
+        :param str port: port number
+        :returns: list of dicts containing the vhost, the matching name, and
+            the numerical rank
+        :rtype: list
+
+        """
+        all_vhosts = self.parser.get_vhosts()
+        def _port_matches(test_port, matching_port):
+            # test_port is a number, matching is a number or "" or None
+            if matching_port == "" or matching_port is None:
+                # if no port is specified, Nginx defaults to listening on port 80.
+                return test_port == self.DEFAULT_LISTEN_PORT
+            else:
+                return test_port == matching_port
+
+        def _vhost_matches(vhost, port):
+            found_matching_port = False
+            if len(vhost.addrs) == 0:
+                # if there are no listen directives at all, Nginx defaults to
+                # listening on port 80.
+                found_matching_port = (port == self.DEFAULT_LISTEN_PORT)
+            else:
+                for addr in vhost.addrs:
+                    if _port_matches(port, addr.get_port()) and addr.ssl == False:
+                        found_matching_port = True
+
+            if found_matching_port:
+                # make sure we don't have an 'ssl on' directive
+                return not self.parser.has_ssl_on_directive(vhost)
+            else:
+                return False
+
+        matching_vhosts = [vhost for vhost in all_vhosts if _vhost_matches(vhost, port)]
+
+        # We can use this ranking function because sslishness doesn't matter to us, and
+        # there shouldn't be conflicting plaintextish servers listening on 80.
+        return self._rank_matches_by_name_and_ssl(matching_vhosts, target_name)
+
     def get_all_names(self):
         """Returns all names found in the Nginx Configuration.
 
@@ -325,6 +423,12 @@ class NginxConfigurator(common.Plugin):
         :type vhost: :class:`~certbot_nginx.obj.VirtualHost`
 
         """
+        # If the vhost was implicitly listening on the default Nginx port,
+        # have it continue to do so.
+        if len(vhost.addrs) == 0:
+            listen_block = [['\n    ', 'listen', ' ', self.DEFAULT_LISTEN_PORT]]
+            self.parser.add_server_directives(vhost, listen_block, replace=False)
+
         snakeoil_cert, snakeoil_key = self._get_snakeoil_paths()
 
         # the options file doesn't have a newline at the beginning, but there
@@ -370,8 +474,7 @@ class NginxConfigurator(common.Plugin):
 
         """
         try:
-            return self._enhance_func[enhancement](
-                self.choose_vhost(domain), options)
+            return self._enhance_func[enhancement](domain, options)
         except (KeyError, ValueError):
             raise errors.PluginError(
                 "Unsupported enhancement: {0}".format(enhancement))
@@ -379,38 +482,49 @@ class NginxConfigurator(common.Plugin):
             logger.warning("Failed %s for %s", enhancement, domain)
             raise
 
-    def _enable_redirect(self, vhost, unused_options):
+    def _enable_redirect(self, domain, unused_options):
         """Redirect all equivalent HTTP traffic to ssl_vhost.
 
         Add rewrite directive to non https traffic
 
         .. note:: This function saves the configuration
 
-        :param vhost: Destination of traffic, an ssl enabled vhost
-        :type vhost: :class:`~certbot_nginx.obj.VirtualHost`
-
+        :param str domain: domain to enable redirect for
         :param unused_options: Not currently used
         :type unused_options: Not Available
         """
-        redirect_block = [[
-            ['\n    ', 'if', ' ', '($scheme != "https") '],
-            [['\n        ', 'return', ' ', '301 https://$host$request_uri'],
-             '\n    ']
-        ], ['\n']]
-        self.parser.add_server_directives(
-            vhost, redirect_block, replace=False)
-        logger.info("Redirecting all traffic to ssl in %s", vhost.filep)
 
-    def _enable_ocsp_stapling(self, vhost, chain_path):
-        """Include OCSP response in TLS handshake
+        port = self.DEFAULT_LISTEN_PORT
+        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)
 
-        :param vhost: Destination of traffic, an ssl enabled vhost
-        :type vhost: :class:`~certbot_nginx.obj.VirtualHost`
+        if vhost is None:
+            logger.info("No matching insecure server blocks listening on port %s found.",
+                self.DEFAULT_LISTEN_PORT)
+        else:
+            # Redirect plaintextish host to https
+            redirect_block = [[
+                ['\n    ', 'if', ' ', '($scheme != "https") '],
+                [['\n        ', 'return', ' ', '301 https://$host$request_uri'],
+                 '\n    ']
+            ], ['\n']]
+
+            self.parser.add_server_directives(
+                vhost, redirect_block, replace=False)
+            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
 
+        :param str domain: domain to enable OCSP response for
         :param chain_path: chain file path
         :type chain_path: `str` or `None`
 
         """
+        vhost = self.choose_vhost(domain)
         if self.version < (1, 3, 7):
             raise errors.PluginError("Version 1.3.7 or greater of nginx "
                                      "is needed to enable OCSP stapling")
diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py
index 13bb38359..a9ef21f2e 100644
--- a/certbot-nginx/certbot_nginx/parser.py
+++ b/certbot-nginx/certbot_nginx/parser.py
@@ -241,6 +241,24 @@ class NginxParser(object):
             except IOError:
                 logger.error("Could not open file for writing: %s", filename)
 
+    def has_ssl_on_directive(self, vhost):
+        """Does vhost have ssl on for all ports?
+
+        :param :class:`~certbot_nginx.obj.VirtualHost` vhost: The vhost in question
+
+        :returns: True if 'ssl on' directive is included
+        :rtype: bool
+
+        """
+        server = vhost.raw
+        for directive in server:
+            if not directive or len(directive) < 2:
+                continue
+            elif directive[0] == 'ssl' and directive[1] == 'on':
+                return True
+
+        return False
+
     def add_server_directives(self, vhost, directives, replace):
         """Add or replace directives in the server block identified by vhost.
 
diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py
index 9bb8a46d8..10f5e5514 100644
--- a/certbot-nginx/certbot_nginx/tests/configurator_test.py
+++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py
@@ -40,7 +40,7 @@ class NginxConfiguratorTest(util.NginxTest):
 
     def test_prepare(self):
         self.assertEqual((1, 6, 2), self.config.version)
-        self.assertEqual(5, len(self.config.parser.parsed))
+        self.assertEqual(6, len(self.config.parser.parsed))
         # ensure we successfully parsed a file for ssl_options
         self.assertTrue(self.config.parser.loc["ssl_options"])
 
@@ -67,8 +67,8 @@ class NginxConfiguratorTest(util.NginxTest):
         mock_gethostbyaddr.return_value = ('155.225.50.69.nephoscale.net', [], [])
         names = self.config.get_all_names()
         self.assertEqual(names, set(
-            ["155.225.50.69.nephoscale.net",
-             "www.example.org", "another.alias"]))
+            ["155.225.50.69.nephoscale.net", "www.example.org", "another.alias",
+             "migration.com", "summer.com", "geese.com"]))
 
     def test_supported_enhancements(self):
         self.assertEqual(['redirect', 'staple-ocsp'],
@@ -214,9 +214,34 @@ class NginxConfiguratorTest(util.NginxTest):
             ],
             2))
 
+    def test_deploy_cert_add_explicit_listen(self):
+        migration_conf = self.config.parser.abs_path('sites-enabled/migration.com')
+        self.config.deploy_cert(
+            "summer.com",
+            "summer/cert.pem",
+            "summer/key.pem",
+            "summer/chain.pem",
+            "summer/fullchain.pem")
+        self.config.save()
+        self.config.parser.load()
+        parsed_migration_conf = util.filter_comments(self.config.parser.parsed[migration_conf])
+        self.assertEqual([['server'],
+                          [
+                           ['server_name', 'migration.com'],
+                           ['server_name', 'summer.com'],
+
+                           ['listen', '80'],
+                           ['listen', '5001 ssl'],
+                           ['ssl_certificate', 'summer/fullchain.pem'],
+                           ['ssl_certificate_key', 'summer/key.pem']] +
+                           util.filter_comments(self.config.parser.loc["ssl_options"])
+                           ],
+                         parsed_migration_conf[0])
+
     def test_get_all_certs_keys(self):
         nginx_conf = self.config.parser.abs_path('nginx.conf')
         example_conf = self.config.parser.abs_path('sites-enabled/example.com')
+        migration_conf = self.config.parser.abs_path('sites-enabled/migration.com')
 
         # Get the default SSL vhost
         self.config.deploy_cert(
@@ -231,12 +256,19 @@ class NginxConfiguratorTest(util.NginxTest):
             "/etc/nginx/key.pem",
             "/etc/nginx/chain.pem",
             "/etc/nginx/fullchain.pem")
+        self.config.deploy_cert(
+            "migration.com",
+            "migration/cert.pem",
+            "migration/key.pem",
+            "migration/chain.pem",
+            "migration/fullchain.pem")
         self.config.save()
 
         self.config.parser.load()
         self.assertEqual(set([
             ('example/fullchain.pem', 'example/key.pem', example_conf),
             ('/etc/nginx/fullchain.pem', '/etc/nginx/key.pem', nginx_conf),
+            ('migration/fullchain.pem', 'migration/key.pem', migration_conf),
         ]), self.config.get_all_certs_keys())
 
     @mock.patch("certbot_nginx.configurator.tls_sni_01.NginxTlsSni01.perform")
@@ -390,6 +422,8 @@ class NginxConfiguratorTest(util.NginxTest):
                 OpenSSL.crypto.FILETYPE_PEM, key_file.read())
 
     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']]
@@ -401,6 +435,21 @@ class NginxConfiguratorTest(util.NginxTest):
         generated_conf = self.config.parser.parsed[example_conf]
         self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))
 
+        # Test that we successfully add a redirect when there is
+        # no listen directive
+        migration_conf = self.config.parser.abs_path('sites-enabled/migration.com')
+        self.config.enhance("migration.com", "redirect")
+
+        generated_conf = self.config.parser.parsed[migration_conf]
+        self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))
+
+    def test_redirect_dont_enhance(self):
+        # Test that we don't accidentally add redirect to ssl-only block
+        with mock.patch("certbot_nginx.configurator.logger") as mock_logger:
+            self.config.enhance("geese.com", "redirect")
+        self.assertEqual(mock_logger.info.call_args[0][0],
+                'No matching insecure server blocks listening on port %s found.')
+
     def test_staple_ocsp_bad_version(self):
         self.config.version = (1, 3, 1)
         self.assertRaises(errors.PluginError, self.config.enhance,
diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py
index 18de59daf..d148e89aa 100644
--- a/certbot-nginx/certbot_nginx/tests/parser_test.py
+++ b/certbot-nginx/certbot_nginx/tests/parser_test.py
@@ -47,7 +47,8 @@ class NginxParserTest(util.NginxTest):
         self.assertEqual(set([nparser.abs_path(x) for x in
                               ['foo.conf', 'nginx.conf', 'server.conf',
                                'sites-enabled/default',
-                               'sites-enabled/example.com']]),
+                               'sites-enabled/example.com',
+                               'sites-enabled/migration.com']]),
                          set(nparser.parsed.keys()))
         self.assertEqual([['server_name', 'somename  alias  another.alias']],
                          nparser.parsed[nparser.abs_path('server.conf')])
@@ -71,7 +72,7 @@ class NginxParserTest(util.NginxTest):
         parsed = nparser._parse_files(nparser.abs_path(
             'sites-enabled/example.com.test'))
         self.assertEqual(3, len(glob.glob(nparser.abs_path('*.test'))))
-        self.assertEqual(2, len(
+        self.assertEqual(3, len(
             glob.glob(nparser.abs_path('sites-enabled/*.test'))))
         self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'],
                                         ['listen', '127.0.0.1'],
@@ -135,7 +136,7 @@ class NginxParserTest(util.NginxTest):
                                                   '*.www.example.com']),
                                  [], [2, 1, 0])
 
-        self.assertEqual(5, len(vhosts))
+        self.assertEqual(7, len(vhosts))
         example_com = [x for x in vhosts if 'example.com' in x.filep][0]
         self.assertEqual(vhost3, example_com)
         default = [x for x in vhosts if 'default' in x.filep][0]
@@ -147,6 +148,26 @@ class NginxParserTest(util.NginxTest):
         somename = [x for x in vhosts if 'somename' in x.names][0]
         self.assertEqual(vhost2, somename)
 
+    def test_has_ssl_on_directive(self):
+        nparser = parser.NginxParser(self.config_path, self.ssl_options)
+        mock_vhost = obj.VirtualHost(None, None, None, None, None,
+              [['listen', 'myhost default_server'],
+               ['server_name', 'www.example.org'],
+               [['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'],
+                          ['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']]
+        self.assertFalse(nparser.has_ssl_on_directive(mock_vhost))
+        mock_vhost.raw = [['listen', '80'],
+                          ['ssl', 'on'],
+                          ['server_name', '*.www.foo.com *.www.example.com']]
+        self.assertTrue(nparser.has_ssl_on_directive(mock_vhost))
+
     def test_add_server_directives(self):
         nparser = parser.NginxParser(self.config_path, self.ssl_options)
         mock_vhost = obj.VirtualHost(nparser.abs_path('nginx.conf'),
@@ -282,7 +303,10 @@ class NginxParserTest(util.NginxTest):
                                        ['listen', '443 ssl']],
                                       replace=False)
         c_k = nparser.get_all_certs_keys()
-        self.assertEqual(set([('foo.pem', 'bar.key', filep)]), c_k)
+        migration_file = nparser.abs_path('sites-enabled/migration.com')
+        self.assertEqual(set([('foo.pem', 'bar.key', filep),
+                              ('cert.pem', 'cert.key', migration_file)
+                             ]), c_k)
 
     def test_parse_server_ssl(self):
         server = parser.parse_server([
diff --git a/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/migration.com b/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/migration.com
new file mode 100644
index 000000000..17bc6d0c3
--- /dev/null
+++ b/certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/migration.com
@@ -0,0 +1,19 @@
+server {
+    server_name migration.com;
+    server_name summer.com;
+}
+   
+server {
+    listen       443 ssl;
+    server_name  migration.com;
+    server_name  geese.com;
+
+    ssl_certificate      cert.pem;
+    ssl_certificate_key  cert.key;
+
+    ssl_session_cache    shared:SSL:1m;
+    ssl_session_timeout  5m;
+
+    ssl_ciphers  HIGH:!aNULL:!MD5;
+    ssl_prefer_server_ciphers  on;
+}