diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index aaaf43c5f..89b1145e7 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -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.""" diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 14db2f8b7..1d424f834 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -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) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 56ad5110c..f6a4f7eee 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -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): diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index 2d6156429..6559a5df6 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -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) diff --git a/letsencrypt-nginx/tests/boulder-integration.conf.sh b/letsencrypt-nginx/tests/boulder-integration.conf.sh index 12610d895..d77669a76 100755 --- a/letsencrypt-nginx/tests/boulder-integration.conf.sh +++ b/letsencrypt-nginx/tests/boulder-integration.conf.sh @@ -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.