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

Don't re-add redirects if one exists (#3751)

Browse source 
* Don't re-add redirects if one exists

* coverage

* make coverage happy

* don't re-add comment, and clean code
This commit is contained in:
Erica Portnoy 2016-11-07 15:48:46 -08:00 committed by Peter Eckersley
parent 0bc3e1860b
commit df10a6431b
5 changed files with 224 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -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

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

@ -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)

1
certbot-nginx/certbot_nginx/parser.py
View file

@ -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.

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

@ -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:

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

@ -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