From df10a6431bd98609775cded7f72410dab07bf438 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Mon, 7 Nov 2016 15:48:46 -0800 Subject: [PATCH] Don't re-add redirects if one exists (#3751) * Don't re-add redirects if one exists * coverage * make coverage happy * don't re-add comment, and clean code --- certbot-nginx/certbot_nginx/configurator.py | 72 +++++++++++++++--- certbot-nginx/certbot_nginx/obj.py | 30 ++++++++ certbot-nginx/certbot_nginx/parser.py | 1 - .../certbot_nginx/tests/configurator_test.py | 58 ++++++++++++++ certbot-nginx/certbot_nginx/tests/obj_test.py | 76 ++++++++++++++++++- 5 files changed, 224 insertions(+), 13 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 503599423..94ac7de86 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -29,6 +29,36 @@ from certbot_nginx import parser 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_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'] +] + +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) @@ -483,6 +513,23 @@ class NginxConfigurator(common.Plugin): logger.warning("Failed %s for %s", enhancement, domain) raise + def _has_certbot_redirect(self, vhost): + return vhost.contains_list(TEST_REDIRECT_BLOCK) + + def _has_certbot_redirect_comment(self, vhost): + return vhost.contains_list(TEST_REDIRECT_COMMENT_BLOCK) + + def _add_redirect_block(self, vhost, active=True): + """Add redirect directive to vhost + """ + if active: + redirect_block = REDIRECT_BLOCK + else: + redirect_block = REDIRECT_COMMENT_BLOCK + + self.parser.add_server_directives( + vhost, redirect_block, replace=False) + def _enable_redirect(self, domain, unused_options): """Redirect all equivalent HTTP traffic to ssl_vhost. @@ -505,17 +552,20 @@ class NginxConfigurator(common.Plugin): 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) + 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) def _enable_ocsp_stapling(self, domain, chain_path): """Include OCSP response in TLS handshake diff --git a/certbot-nginx/certbot_nginx/obj.py b/certbot-nginx/certbot_nginx/obj.py index c58a82450..4a3ca865e 100644 --- a/certbot-nginx/certbot_nginx/obj.py +++ b/certbot-nginx/certbot_nginx/obj.py @@ -3,6 +3,7 @@ import re from certbot.plugins import common +REDIRECT_DIRECTIVES = ['return', 'rewrite'] class Addr(common.Addr): r"""Represents an Nginx address, i.e. what comes after the 'listen' @@ -149,3 +150,32 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods self.path == other.path) return False + + def has_redirect(self): + """Determine if this vhost has a redirecting statement + """ + for directive_name in REDIRECT_DIRECTIVES: + found = _find_directive(self.raw, directive_name) + if found is not None: + return True + return False + + def contains_list(self, test): + """Determine if raw server block contains test list at top level + """ + for i in xrange(0, len(self.raw) - len(test)): + if self.raw[i:i + len(test)] == test: + return True + return False + +def _find_directive(directives, directive_name): + """Find a directive of type directive_name in directives + """ + if not directives or isinstance(directives, str) or len(directives) == 0: + return None + + if directives[0] == directive_name: + return directives + + matches = (_find_directive(line, directive_name) for line in directives) + return next((m for m in matches if m is not None), None) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 6203b5f71..d5664ac29 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -496,7 +496,6 @@ def parse_server(server): return parsed_server - def _add_directives(block, directives, replace): """Adds or replaces directives in a config block. diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index d871a5720..f7d6ade2d 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -445,6 +445,64 @@ class NginxConfiguratorTest(util.NginxTest): generated_conf = self.config.parser.parsed[migration_conf] self.assertTrue(util.contains_at_depth(generated_conf, expected, 2)) + @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): + # Test that we add no redirect statement if there is already a + # redirect in the block that is managed by certbot + # Has a certbot redirect + mock_has_redirect.return_value = True + mock_contains_list.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], + "Traffic on port %s already redirecting to ssl in %s") + + @mock.patch('certbot_nginx.obj.VirtualHost.contains_list') + @mock.patch('certbot_nginx.obj.VirtualHost.has_redirect') + def test_non_certbot_redirect_exists(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') + + # 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'], + ['#', ' if ($scheme != "https") {'], + ['#', ' return 301 https://$host$request_uri;'], + ['#', ' } # managed by Certbot'] + ] + 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') + @mock.patch('certbot_nginx.configurator.NginxConfigurator._has_certbot_redirect_comment') + @mock.patch('certbot_nginx.configurator.NginxConfigurator._add_redirect_block') + def test_redirect_comment_exists(self, mock_add_redirect_block, + mock_has_cb_redirect_comment, mock_has_redirect, mock_contains_list): + # Test that we add nothing if there is a non-Certbot redirect and a + # preexisting comment + # Has a non-Certbot redirect and a comment + mock_has_redirect.return_value = True + mock_contains_list.return_value = False # self._has_certbot_redirect(vhost): + mock_has_cb_redirect_comment.return_value = True + + # assert _add_redirect_block not called + with mock.patch("certbot_nginx.configurator.logger") as mock_logger: + self.config.enhance("www.example.com", "redirect") + self.assertFalse(mock_add_redirect_block.called) + self.assertTrue(mock_logger.info.called) + 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: diff --git a/certbot-nginx/certbot_nginx/tests/obj_test.py b/certbot-nginx/certbot_nginx/tests/obj_test.py index 84d0c6bca..b153db8d4 100644 --- a/certbot-nginx/certbot_nginx/tests/obj_test.py +++ b/certbot-nginx/certbot_nginx/tests/obj_test.py @@ -87,10 +87,43 @@ class VirtualHostTest(unittest.TestCase): def setUp(self): from certbot_nginx.obj import VirtualHost from certbot_nginx.obj import Addr + raw1 = [ + ['listen', '69.50.225.155:9000'], + [['if', '($scheme != "https") '], + [['return', '301 https://$host$request_uri']] + ], + ['#', ' managed by Certbot'] + ] self.vhost1 = VirtualHost( "filep", set([Addr.fromstring("localhost")]), False, False, - set(['localhost']), [], []) + set(['localhost']), raw1, []) + raw2 = [ + ['listen', '69.50.225.155:9000'], + [['if', '($scheme != "https") '], + [['return', '301 https://$host$request_uri']] + ] + ] + self.vhost2 = VirtualHost( + "filep", + set([Addr.fromstring("localhost")]), False, False, + set(['localhost']), raw2, []) + raw3 = [ + ['listen', '69.50.225.155:9000'], + ['rewrite', '^(.*)$ $scheme://www.domain.com$1 permanent;'] + ] + self.vhost3 = VirtualHost( + "filep", + set([Addr.fromstring("localhost")]), False, False, + set(['localhost']), raw3, []) + raw4 = [ + ['listen', '69.50.225.155:9000'], + ['server_name', 'return.com'] + ] + self.vhost4 = VirtualHost( + "filp", + set([Addr.fromstring("localhost")]), False, False, + set(['localhost']), raw4, []) def test_eq(self): from certbot_nginx.obj import Addr @@ -110,6 +143,47 @@ class VirtualHostTest(unittest.TestCase): 'enabled: False']) self.assertEqual(stringified, str(self.vhost1)) + def test_has_redirect(self): + self.assertTrue(self.vhost1.has_redirect()) + self.assertTrue(self.vhost2.has_redirect()) + self.assertTrue(self.vhost3.has_redirect()) + self.assertFalse(self.vhost4.has_redirect()) + + 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 + 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'], + ['#', ' managed by Certbot'], + ['ssl_certificate', '/etc/letsencrypt/live/two.functorkitten.xyz/fullchain.pem'], + ['#', ' 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']]], + ['#', ' managed by Certbot'], []] + vhost_haystack = VirtualHost( + "filp", + set([Addr.fromstring("localhost")]), False, False, + set(['localhost']), test_haystack, []) + test_bad_haystack = [['listen', '80'], ['root', '/var/www/html'], + ['index', 'index.html index.htm index.nginx-debian.html'], + ['server_name', 'two.functorkitten.xyz'], ['listen', '443 ssl'], + ['#', ' managed by Certbot'], + ['ssl_certificate', '/etc/letsencrypt/live/two.functorkitten.xyz/fullchain.pem'], + ['#', ' 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']]], + ['#', ' managed by Certbot'], []] + vhost_bad_haystack = VirtualHost( + "filp", + set([Addr.fromstring("localhost")]), False, False, + set(['localhost']), test_bad_haystack, []) + self.assertTrue(vhost_haystack.contains_list(test_needle)) + self.assertFalse(vhost_bad_haystack.contains_list(test_needle)) if __name__ == "__main__": unittest.main() # pragma: no cover