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

Better Nginx error handling.

Browse source 
Raise MisconfigurationError on restart failure, so we don't attempt to continue
with an authorization we know will fail.

Log at debug level the config files that are about to be written out, so it's
easier to debug restart failures.

Fix https://github.com/letsencrypt/letsencrypt/issues/942:
Error out if adding a conflicting directive.

Remove unnecessarily-inserted access_log and error_log directives. These were
added to make integration testing easier but are no longer needed. Incidentally
this makes the plugin work with some configs where it wouldn't have worked
previously.

Change the semantics of add_server_directives with replace=True so only the
first instance of a given directive is replaced, not all of them. This works
fine with the one place in the code that calls add_server_directives with
replace=True, because all of the involved directives aren't allowed to be
duplicated in a given block.

Make add_http_directives do inserts into the structure itself, since its needs
were significantly different than the more general add_server_directives. This
also allows us to narrow the scope of the `block.insert(0, directive)` hack that
we inserted to work around https://trac.nginx.org/nginx/ticket/810, since it's
only necessary for http blocks.
This commit is contained in:
Jacob Hoffman-Andrews 2016-01-01 16:35:57 -08:00
parent ffd0d5aa56
commit 8f984bd24f
5 changed files with 115 additions and 88 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
letsencrypt-nginx/letsencrypt_nginx/configurator.py
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."""

81
letsencrypt-nginx/letsencrypt_nginx/parser.py
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,39 @@ 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.
if location != -1 and directive[0].__str__() not in repeatable_directives:
if block[location][1] == directive[1]:
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)

63
letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py
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):

29
letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py
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)

6
letsencrypt-nginx/tests/boulder-integration.conf.sh
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.