From c9bc0345129f8e927d28a59845f4504974fb01b2 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Thu, 29 Sep 2016 16:16:07 -0700 Subject: [PATCH] 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 --- certbot-nginx/certbot_nginx/configurator.py | 170 +++++++++++++++--- certbot-nginx/certbot_nginx/parser.py | 18 ++ .../certbot_nginx/tests/configurator_test.py | 55 +++++- .../certbot_nginx/tests/parser_test.py | 32 +++- .../etc_nginx/sites-enabled/migration.com | 19 ++ 5 files changed, 259 insertions(+), 35 deletions(-) create mode 100644 certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/sites-enabled/migration.com 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) @@ -223,6 +219,41 @@ class NginxConfigurator(common.Plugin): the numerical rank :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 @@ -230,7 +261,7 @@ class NginxConfigurator(common.Plugin): # 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): + 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) + + 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 vhost: Destination of traffic, an ssl enabled vhost - :type vhost: :class:`~certbot_nginx.obj.VirtualHost` - + :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; +}