starred/certbot-certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-07-28 00:35:50 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2062 from jsha/nginx-nodupe

Browse Source 
Better Nginx error handling.
This commit is contained in:
Peter Eckersley
2016-01-05 17:42:21 -08:00
parent 4c28fc417c e722a38197
commit 2f195cb9f3
5 changed files with 119 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
letsencrypt-nginx/letsencrypt_nginx/configurator.py
+7 -17
View File
@@ -311,17 +311,11 @@ class NginxConfigurator(common.Plugin):
"""
snakeoil_cert, snakeoil_key = self._get_snakeoil_paths()
ssl_block = [['listen', '{0} ssl'.format(self.config.tls_sni_01_port)],
# access and error logs necessary for integration
# testing (non-root)
['access_log', os.path.join(
self.config.work_dir, 'access.log')],
['error_log', os.path.join(
self.config.work_dir, 'error.log')],
['ssl_certificate', snakeoil_cert],
['ssl_certificate_key', snakeoil_key],
['include', self.parser.loc["ssl_options"]]]
self.parser.add_server_directives(
vhost.filep, vhost.names, ssl_block)
vhost.filep, vhost.names, ssl_block, replace=False)
vhost.ssl = True
vhost.raw.extend(ssl_block)
vhost.addrs.add(obj.Addr(
@@ -384,7 +378,7 @@ class NginxConfigurator(common.Plugin):
[['return', '301 https://$host$request_uri']]
]]
self.parser.add_server_directives(
vhost.filep, vhost.names, redirect_block)
vhost.filep, vhost.names, redirect_block, replace=False)
logger.info("Redirecting all traffic to ssl in %s", vhost.filep)
######################################
@@ -393,11 +387,10 @@ class NginxConfigurator(common.Plugin):
def restart(self):
"""Restarts nginx server.
:returns: Success
:rtype: bool
:raises .errors.MisconfigurationError: If either the reload fails.
"""
return nginx_restart(self.conf('ctl'), self.nginx_conf)
nginx_restart(self.conf('ctl'), self.nginx_conf)
def config_test(self): # pylint: disable=no-self-use
"""Check the configuration of Nginx for errors.
@@ -631,19 +624,16 @@ def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"):
if nginx_proc.returncode != 0:
# Enter recovery routine...
logger.error("Nginx Restart Failed!\n%s\n%s", stdout, stderr)
return False
raise errors.MisconfigurationError(
"nginx restart failed:\n%s\n%s" % (stdout, stderr))
except (OSError, ValueError):
logger.fatal("Nginx Restart Failed - Please Check the Configuration")
sys.exit(1)
raise errors.MisconfigurationError("nginx restart failed")
# Nginx can take a moment to recognize a newly added TLS SNI servername, so sleep
# for a second. TODO: Check for expected servername and loop until it
# appears or return an error if looping too long.
time.sleep(1)
return True
def temp_install(options_ssl):
"""Temporary install for convenience."""
letsencrypt-nginx/letsencrypt_nginx/parser.py
+60 -25
View File
@@ -213,6 +213,7 @@ class NginxParser(object):
if ext:
filename = filename + os.path.extsep + ext
try:
logger.debug('Dumping to %s:\n%s', filename, nginxparser.dumps(tree))
with open(filename, 'w') as _file:
nginxparser.dump(tree, _file)
except IOError:
@@ -252,7 +253,7 @@ class NginxParser(object):
return server_names == names
def add_server_directives(self, filename, names, directives,
replace=False):
replace):
"""Add or replace directives in the first server block with names.
..note :: If replace is True, this raises a misconfiguration error
@@ -269,20 +270,27 @@ class NginxParser(object):
:param bool replace: Whether to only replace existing directives
"""
_do_for_subarray(self.parsed[filename],
lambda x: self._has_server_names(x, names),
lambda x: _add_directives(x, directives, replace))
try:
_do_for_subarray(self.parsed[filename],
lambda x: self._has_server_names(x, names),
lambda x: _add_directives(x, directives, replace))
except errors.MisconfigurationError as err:
raise errors.MisconfigurationError("Problem in %s: %s" % (filename, err.message))
def add_http_directives(self, filename, directives):
"""Adds directives to the first encountered HTTP block in filename.
We insert new directives at the top of the block to work around
https://trac.nginx.org/nginx/ticket/810: If the first server block
doesn't enable OCSP stapling, stapling is broken for all blocks.
:param str filename: The absolute filename of the config file
:param list directives: The directives to add
"""
_do_for_subarray(self.parsed[filename],
lambda x: x[0] == ['http'],
lambda x: _add_directives(x[1], [directives], False))
lambda x: x[1].insert(0, directives))
def get_all_certs_keys(self):
"""Gets all certs and keys in the nginx config.
@@ -467,9 +475,14 @@ def _parse_server(server):
return parsed_server
def _add_directives(block, directives, replace=False):
"""Adds or replaces directives in a block. If the directive doesn't exist in
the entry already, raises a misconfiguration error.
def _add_directives(block, directives, replace):
"""Adds or replaces directives in a config block.
When replace=False, it's an error to try and add a directive that already
exists in the config block with a conflicting value.
When replace=True, a directive with the same name MUST already exist in the
config block, and the first instance will be replaced.
..todo :: Find directives that are in included files.
@@ -478,21 +491,43 @@ def _add_directives(block, directives, replace=False):
"""
for directive in directives:
if not replace:
# We insert new directives at the top of the block, mostly
# to work around https://trac.nginx.org/nginx/ticket/810
# Only add directive if its not already in the block
if directive not in block:
block.insert(0, directive)
else:
changed = False
if len(directive) == 0:
continue
for index, line in enumerate(block):
if len(line) > 0 and line[0] == directive[0]:
block[index] = directive
changed = True
if not changed:
_add_directive(block, directive, replace)
repeatable_directives = set(['server_name', 'listen', 'include'])
def _add_directive(block, directive, replace):
"""Adds or replaces a single directive in a config block.
See _add_directives for more documentation.
"""
location = -1
# Find the index of a config line where the name of the directive matches
# the name of the directive we want to add.
for index, line in enumerate(block):
if len(line) > 0 and line[0] == directive[0]:
location = index
break
if replace:
if location == -1:
raise errors.MisconfigurationError(
'expected directive for %s in the Nginx '
'config but did not find it.' % directive[0])
block[location] = directive
else:
# Append directive. Fail if the name is not a repeatable directive name,
# and there is already a copy of that directive with a different value
# in the config file.
directive_name = directive[0]
directive_value = directive[1]
if location != -1 and directive_name.__str__() not in repeatable_directives:
if block[location][1] == directive_value:
# There's a conflict, but the existing value matches the one we
# want to insert, so it's fine.
pass
else:
raise errors.MisconfigurationError(
'Let\'s Encrypt expected directive for %s in the Nginx '
'config but did not find it.' % directive[0])
'tried to insert directive "%s" but found conflicting "%s".' % (
directive, block[location]))
else:
block.append(directive)
letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py
+32 -31
View File
@@ -65,16 +65,19 @@ class NginxConfiguratorTest(util.NginxTest):
filep = self.config.parser.abs_path('sites-enabled/example.com')
self.config.parser.add_server_directives(
filep, set(['.example.com', 'example.*']),
[['listen', '5001 ssl']])
[['listen', '5001 ssl']],
replace=False)
self.config.save()
# pylint: disable=protected-access
parsed = self.config.parser._parse_files(filep, override=True)
self.assertEqual([[['server'], [['listen', '5001 ssl'],
self.assertEqual([[['server'], [
['listen', '69.50.225.155:9000'],
['listen', '127.0.0.1'],
['server_name', '.example.com'],
['server_name', 'example.*']]]],
['server_name', 'example.*'],
['listen', '5001 ssl']
]]],
parsed[0])
def test_choose_vhost(self):
@@ -154,38 +157,36 @@ class NginxConfiguratorTest(util.NginxTest):
parsed_server_conf = util.filter_comments(self.config.parser.parsed[server_conf])
parsed_nginx_conf = util.filter_comments(self.config.parser.parsed[nginx_conf])
access_log = os.path.join(self.work_dir, "access.log")
error_log = os.path.join(self.work_dir, "error.log")
self.assertEqual([[['server'],
[['include', self.config.parser.loc["ssl_options"]],
['ssl_certificate_key', 'example/key.pem'],
['ssl_certificate', 'example/fullchain.pem'],
['error_log', error_log],
['access_log', access_log],
['listen', '5001 ssl'],
[
['listen', '69.50.225.155:9000'],
['listen', '127.0.0.1'],
['server_name', '.example.com'],
['server_name', 'example.*']]]],
['server_name', 'example.*'],
['listen', '5001 ssl'],
['ssl_certificate', 'example/fullchain.pem'],
['ssl_certificate_key', 'example/key.pem'],
['include', self.config.parser.loc["ssl_options"]]
]]],
parsed_example_conf)
self.assertEqual([['server_name', 'somename alias another.alias']],
parsed_server_conf)
self.assertTrue(util.contains_at_depth(parsed_nginx_conf,
[['server'],
[['include', self.config.parser.loc["ssl_options"]],
['ssl_certificate_key', '/etc/nginx/key.pem'],
['ssl_certificate', '/etc/nginx/fullchain.pem'],
['error_log', error_log],
['access_log', access_log],
['listen', '5001 ssl'],
['listen', '8000'],
['listen', 'somename:8080'],
['include', 'server.conf'],
[['location', '/'],
[['root', 'html'],
['index', 'index.html index.htm']]]]],
2))
self.assertTrue(util.contains_at_depth(
parsed_nginx_conf,
[['server'],
[
['listen', '8000'],
['listen', 'somename:8080'],
['include', 'server.conf'],
[['location', '/'],
[['root', 'html'],
['index', 'index.html index.htm']]],
['listen', '5001 ssl'],
['ssl_certificate', '/etc/nginx/fullchain.pem'],
['ssl_certificate_key', '/etc/nginx/key.pem'],
['include', self.config.parser.loc["ssl_options"]]]],
2))
def test_get_all_certs_keys(self):
nginx_conf = self.config.parser.abs_path('nginx.conf')
@@ -297,19 +298,19 @@ class NginxConfiguratorTest(util.NginxTest):
mocked = mock_popen()
mocked.communicate.return_value = ('', '')
mocked.returncode = 0
self.assertTrue(self.config.restart())
self.config.restart()
@mock.patch("letsencrypt_nginx.configurator.subprocess.Popen")
def test_nginx_restart_fail(self, mock_popen):
mocked = mock_popen()
mocked.communicate.return_value = ('', '')
mocked.returncode = 1
self.assertFalse(self.config.restart())
self.assertRaises(errors.MisconfigurationError, self.config.restart)
@mock.patch("letsencrypt_nginx.configurator.subprocess.Popen")
def test_no_nginx_start(self, mock_popen):
mock_popen.side_effect = OSError("Can't find program")
self.assertRaises(SystemExit, self.config.restart)
self.assertRaises(errors.MisconfigurationError, self.config.restart)
@mock.patch("letsencrypt_nginx.configurator.subprocess.Popen")
def test_config_test(self, mock_popen):
letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py
+18 -11
View File
@@ -127,7 +127,8 @@ class NginxParserTest(util.NginxTest):
set(['localhost',
r'~^(www\.)?(example|bar)\.']),
[['foo', 'bar'], ['ssl_certificate',
'/etc/ssl/cert.pem']])
'/etc/ssl/cert.pem']],
replace=False)
ssl_re = re.compile(r'\n\s+ssl_certificate /etc/ssl/cert.pem')
dump = nginxparser.dumps(nparser.parsed[nparser.abs_path('nginx.conf')])
self.assertEqual(1, len(re.findall(ssl_re, dump)))
@@ -136,12 +137,15 @@ class NginxParserTest(util.NginxTest):
names = set(['alias', 'another.alias', 'somename'])
nparser.add_server_directives(server_conf, names,
[['foo', 'bar'], ['ssl_certificate',
'/etc/ssl/cert2.pem']])
nparser.add_server_directives(server_conf, names, [['foo', 'bar']])
'/etc/ssl/cert2.pem']],
replace=False)
nparser.add_server_directives(server_conf, names, [['foo', 'bar']],
replace=False)
self.assertEqual(nparser.parsed[server_conf],
[['ssl_certificate', '/etc/ssl/cert2.pem'],
[['server_name', 'somename alias another.alias'],
['foo', 'bar'],
['server_name', 'somename alias another.alias']])
['ssl_certificate', '/etc/ssl/cert2.pem']
])
def test_add_http_directives(self):
nparser = parser.NginxParser(self.config_path, self.ssl_options)
@@ -165,17 +169,19 @@ class NginxParserTest(util.NginxTest):
target = set(['.example.com', 'example.*'])
filep = nparser.abs_path('sites-enabled/example.com')
nparser.add_server_directives(
filep, target, [['server_name', 'foo bar']], True)
filep, target, [['server_name', 'foobar.com']], replace=True)
self.assertEqual(
nparser.parsed[filep],
[[['server'], [['listen', '69.50.225.155:9000'],
['listen', '127.0.0.1'],
['server_name', 'foo bar'],
['server_name', 'foo bar']]]])
['server_name', 'foobar.com'],
['server_name', 'example.*'],
]]])
self.assertRaises(errors.MisconfigurationError,
nparser.add_server_directives,
filep, set(['foo', 'bar']),
[['ssl_certificate', 'cert.pem']], True)
filep, set(['foobar.com', 'example.*']),
[['ssl_certificate', 'cert.pem']],
replace=True)
def test_get_best_match(self):
target_name = 'www.eff.org'
@@ -217,7 +223,8 @@ class NginxParserTest(util.NginxTest):
set(['.example.com', 'example.*']),
[['ssl_certificate', 'foo.pem'],
['ssl_certificate_key', 'bar.key'],
['listen', '443 ssl']])
['listen', '443 ssl']],
replace=False)
c_k = nparser.get_all_certs_keys()
self.assertEqual(set([('foo.pem', 'bar.key', filep)]), c_k)
letsencrypt-nginx/tests/boulder-integration.conf.sh
+2 -4
View File
@@ -20,13 +20,14 @@ events {
}
http {
# Set an array of temp and cache file options that will otherwise default to
# Set an array of temp, cache and log file options that will otherwise default to
# restricted locations accessible only to root.
client_body_temp_path $root/client_body;
fastcgi_temp_path $root/fastcgi_temp;
proxy_temp_path $root/proxy_temp;
#scgi_temp_path $root/scgi_temp;
#uwsgi_temp_path $root/uwsgi_temp;
access_log $root/error.log;
# This should be turned off in a Virtualbox VM, as it can cause some
# interesting issues with data corruption in delivered files.
@@ -54,9 +55,6 @@ http {
root $root/webroot;
access_log $root/access.log;
error_log $root/error.log;
location / {
# First attempt to serve request as file, then as directory, then fall
# back to index.html.