upstream/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-05-28 04:34:11 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Fix Nginx redirect issue (#5479) (#5481)

Browse source 
* wrap redirect in if host matches

* return 404 if we've created a new block

* change domain matching to exact match

* insert new redirect directive at the top

* add a redirect block to the top if it doesn't already exist, even if there's an existing redirect

* fix obj tests

* remove active parameter

* update tests

* add back spaces

* move imports

* remove unused code

(cherry picked from commit 8a9f21cdd3)
This commit is contained in:
Brad Warren 2018-01-24 22:49:51 -08:00 committed by GitHub
parent e0262e86df
commit ebc5bb1037
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 44 additions and 151 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

50
certbot-nginx/certbot_nginx/configurator.py
View file

@ -31,16 +31,6 @@ from certbot_nginx import http_01
logger = logging.getLogger(__name__)
REDIRECT_BLOCK = [
['\n ', 'return', ' ', '301', ' ', 'https://$host$request_uri'],
['\n']
]
REDIRECT_COMMENT_BLOCK = [
['\n ', '#', ' Redirect non-https traffic to https'],
['\n ', '#', ' return 301 https://$host$request_uri;'],
['\n']
]
@zope.interface.implementer(interfaces.IAuthenticator, interfaces.IInstaller)
@zope.interface.provider(interfaces.IPluginFactory)
@ -571,24 +561,17 @@ class NginxConfigurator(common.Installer):
logger.warning("Failed %s for %s", enhancement, domain)
raise
def _has_certbot_redirect(self, vhost):
test_redirect_block = _test_block_from_block(REDIRECT_BLOCK)
def _has_certbot_redirect(self, vhost, domain):
test_redirect_block = _test_block_from_block(_redirect_block_for_domain(domain))
return vhost.contains_list(test_redirect_block)
def _has_certbot_redirect_comment(self, vhost):
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):
def _add_redirect_block(self, vhost, domain):
"""Add redirect directive to vhost
"""
if active:
redirect_block = REDIRECT_BLOCK
else:
redirect_block = REDIRECT_COMMENT_BLOCK
redirect_block = _redirect_block_for_domain(domain)
self.parser.add_server_directives(
vhost, redirect_block, replace=False)
vhost, redirect_block, replace=False, insert_at_top=True)
def _enable_redirect(self, domain, unused_options):
"""Redirect all equivalent HTTP traffic to ssl_vhost.
@ -615,6 +598,7 @@ class NginxConfigurator(common.Installer):
self.DEFAULT_LISTEN_PORT)
return
new_vhost = None
if vhost.ssl:
new_vhost = self.parser.duplicate_vhost(vhost,
only_directives=['listen', 'server_name'])
@ -631,20 +615,18 @@ class NginxConfigurator(common.Installer):
# remove all non-ssl addresses from the existing block
self.parser.remove_server_directives(vhost, 'listen', match_func=_no_ssl_match_func)
# Add this at the bottom to get the right order of directives
return_404_directive = [['\n ', 'return', ' ', '404']]
self.parser.add_server_directives(new_vhost, return_404_directive, replace=False)
vhost = new_vhost
if self._has_certbot_redirect(vhost):
if self._has_certbot_redirect(vhost, domain):
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)
self._add_redirect_block(vhost, domain)
logger.info("Redirecting all traffic on port %s to ssl in %s",
self.DEFAULT_LISTEN_PORT, vhost.filep)
@ -907,6 +889,14 @@ def _test_block_from_block(block):
parser.comment_directive(test_block, 0)
return test_block[:-1]
def _redirect_block_for_domain(domain):
redirect_block = [[
['\n ', 'if', ' ', '($host', ' ', '=', ' ', '%s)' % domain, ' '],
[['\n ', 'return', ' ', '301', ' ', 'https://$host$request_uri'],
'\n ']],
['\n']]
return redirect_block
def nginx_restart(nginx_ctl, nginx_conf):
"""Restarts the Nginx Server.

21
certbot-nginx/certbot_nginx/obj.py
View file

@ -193,15 +193,6 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods
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
"""
@ -225,15 +216,3 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods
for a in self.addrs:
if not a.ipv6:
return True
def _find_directive(directives, directive_name):
"""Find a directive of type directive_name in directives
"""
if not directives or isinstance(directives, six.string_types) 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)

110
certbot-nginx/certbot_nginx/tests/configurator_test.py
View file

@ -18,6 +18,8 @@ from certbot.tests import util as certbot_test_util
from certbot_nginx import constants
from certbot_nginx import obj
from certbot_nginx import parser
from certbot_nginx.configurator import _redirect_block_for_domain
from certbot_nginx.nginxparser import UnspacedList
from certbot_nginx.tests import util
@ -447,7 +449,7 @@ class NginxConfiguratorTest(util.NginxTest):
def test_redirect_enhance(self):
# Test that we successfully add a redirect when there is
# a listen directive
expected = ['return', '301', 'https://$host$request_uri']
expected = UnspacedList(_redirect_block_for_domain("www.example.com"))[0]
example_conf = self.config.parser.abs_path('sites-enabled/example.com')
self.config.enhance("www.example.com", "redirect")
@ -460,6 +462,8 @@ class NginxConfiguratorTest(util.NginxTest):
migration_conf = self.config.parser.abs_path('sites-enabled/migration.com')
self.config.enhance("migration.com", "redirect")
expected = UnspacedList(_redirect_block_for_domain("migration.com"))[0]
generated_conf = self.config.parser.parsed[migration_conf]
self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))
@ -484,101 +488,27 @@ class NginxConfiguratorTest(util.NginxTest):
['ssl_dhparam', self.config.ssl_dhparams], ['#', ' managed by Certbot'],
[], []]],
[['server'], [
[['if', '($host', '=', 'www.example.com)'], [
['return', '301', 'https://$host$request_uri']]],
['#', ' managed by Certbot'], [],
['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'],
[], []]]],
['return', '404'], ['#', ' 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):
def test_certbot_redirect_exists(self, 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'],
['#', ' 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))
@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:
@ -586,22 +516,18 @@ class NginxConfiguratorTest(util.NginxTest):
self.assertEqual(mock_logger.info.call_args[0][0],
'No matching insecure server blocks listening on port %s found.')
def test_no_double_redirect(self):
# Test that we don't also add the commented redirect if we've just added
# a redirect to that vhost this run
def test_double_redirect(self):
# Test that we add one redirect for each domain
example_conf = self.config.parser.abs_path('sites-enabled/example.com')
self.config.enhance("example.com", "redirect")
self.config.enhance("example.org", "redirect")
unexpected = [
['#', ' Redirect non-https traffic to https'],
['#', ' if ($scheme != "https") {'],
['#', ' return 301 https://$host$request_uri;'],
['#', ' } # managed by Certbot']
]
expected1 = UnspacedList(_redirect_block_for_domain("example.com"))[0]
expected2 = UnspacedList(_redirect_block_for_domain("example.org"))[0]
generated_conf = self.config.parser.parsed[example_conf]
for line in unexpected:
self.assertFalse(util.contains_at_depth(generated_conf, line, 2))
self.assertTrue(util.contains_at_depth(generated_conf, expected1, 2))
self.assertTrue(util.contains_at_depth(generated_conf, expected2, 2))
def test_staple_ocsp_bad_version(self):
self.config.version = (1, 3, 1)
@ -763,7 +689,7 @@ class NginxConfiguratorTest(util.NginxTest):
self.config.parser.load()
expected = ['return', '301', 'https://$host$request_uri']
expected = UnspacedList(_redirect_block_for_domain("www.nomatch.com"))[0]
generated_conf = self.config.parser.parsed[default_conf]
self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))

14
certbot-nginx/certbot_nginx/tests/obj_test.py
View file

@ -162,17 +162,15 @@ 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 REDIRECT_BLOCK, _test_block_from_block
test_needle = _test_block_from_block(REDIRECT_BLOCK)
from certbot_nginx.configurator import _test_block_from_block
test_block = [
['\n ', 'return', ' ', '301', ' ', 'https://$host$request_uri'],
['\n']
]
test_needle = _test_block_from_block(test_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'],