diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 4b039417d..1073bff4f 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -192,8 +192,8 @@ class NginxConfigurator(common.Installer): cert_directives = [['\n ', 'ssl_certificate', ' ', fullchain_path], ['\n ', 'ssl_certificate_key', ' ', key_path]] - self.parser.add_server_directives(vhost, - cert_directives, replace=True) + self.parser.update_or_add_server_directives(vhost, + cert_directives) logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) self.save_notes += ("Changed vhost at %s with addresses of %s\n" % @@ -344,7 +344,7 @@ class NginxConfigurator(common.Installer): for name in vhost.names: name_block[0].append(' ') name_block[0].append(name) - self.parser.add_server_directives(vhost, name_block, replace=True) + self.parser.update_or_add_server_directives(vhost, name_block) def _get_default_vhost(self, port): vhost_list = self.parser.get_vhosts() @@ -584,7 +584,7 @@ class NginxConfigurator(common.Installer): # have it continue to do so. if len(vhost.addrs) == 0: listen_block = [['\n ', 'listen', ' ', self.DEFAULT_LISTEN_PORT]] - self.parser.add_server_directives(vhost, listen_block, replace=False) + self.parser.add_server_directives(vhost, listen_block) if vhost.ipv6_enabled(): ipv6_block = ['\n ', @@ -618,7 +618,7 @@ class NginxConfigurator(common.Installer): ]) self.parser.add_server_directives( - vhost, ssl_block, replace=False) + vhost, ssl_block) ################################## # enhancement methods (IInstaller) @@ -683,7 +683,7 @@ class NginxConfigurator(common.Installer): ['\n ', 'add_header', ' ', header_substring, ' '] + constants.HEADER_ARGS[header_substring], ['\n']] - self.parser.add_server_directives(vhost, header_directives, replace=False) + self.parser.add_server_directives(vhost, header_directives) def _add_redirect_block(self, vhost, domain): """Add redirect directive to vhost @@ -691,7 +691,7 @@ class NginxConfigurator(common.Installer): redirect_block = _redirect_block_for_domain(domain) self.parser.add_server_directives( - vhost, redirect_block, replace=False, insert_at_top=True) + vhost, redirect_block, insert_at_top=True) def _split_block(self, vhost, only_directives=None): """Splits this "virtual host" (i.e. this nginx server block) into @@ -771,7 +771,7 @@ class NginxConfigurator(common.Installer): # Add this at the bottom to get the right order of directives return_404_directive = [['\n ', 'return', ' ', '404']] - self.parser.add_server_directives(http_vhost, return_404_directive, replace=False) + self.parser.add_server_directives(http_vhost, return_404_directive) vhost = http_vhost @@ -821,7 +821,7 @@ class NginxConfigurator(common.Installer): try: self.parser.add_server_directives(vhost, - stapling_directives, replace=False) + stapling_directives) except errors.MisconfigurationError as error: logger.debug(error) raise errors.PluginError("An error occurred while enabling OCSP " diff --git a/certbot-nginx/certbot_nginx/http_01.py b/certbot-nginx/certbot_nginx/http_01.py index 0b1b2bfe0..93c7bfc90 100644 --- a/certbot-nginx/certbot_nginx/http_01.py +++ b/certbot-nginx/certbot_nginx/http_01.py @@ -199,9 +199,9 @@ class NginxHttp01(common.ChallengePerformer): ['return', ' ', '200', ' ', validation]]]] self.configurator.parser.add_server_directives(vhost, - location_directive, replace=False) + location_directive) rewrite_directive = [['rewrite', ' ', '^(/.well-known/acme-challenge/.*)', ' ', '$1', ' ', 'break']] self.configurator.parser.add_server_directives(vhost, - rewrite_directive, replace=False, insert_at_top=True) + rewrite_directive, insert_at_top=True) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index e329307c0..3dc70f19b 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -276,15 +276,13 @@ class NginxParser(object): return False - def add_server_directives(self, vhost, directives, replace, insert_at_top=False): - """Add or replace directives in the server block identified by vhost. + def add_server_directives(self, vhost, directives, insert_at_top=False): + """Add directives to the server block identified by vhost. This method modifies vhost to be fully consistent with the new directives. - ..note :: If replace is True and the directive already exists, the first - instance will be replaced. Otherwise, the directive is added. - ..note :: If replace is False nothing gets added if an identical - block exists already. + ..note :: It's an error to try and add a nonrepeatable directive that already + exists in the config block with a conflicting value. ..todo :: Doesn't match server blocks whose server_name directives are split across multiple conf files. @@ -292,13 +290,34 @@ class NginxParser(object): :param :class:`~certbot_nginx.obj.VirtualHost` vhost: The vhost whose information we use to match on :param list directives: The directives to add - :param bool replace: Whether to only replace existing directives :param bool insert_at_top: True if the directives need to be inserted at the top of the server block instead of the bottom """ self._modify_server_directives(vhost, - functools.partial(_add_directives, directives, replace, insert_at_top)) + functools.partial(_add_directives, directives, insert_at_top)) + + def update_or_add_server_directives(self, vhost, directives, insert_at_top=False): + """Add or replace directives in the server block identified by vhost. + + This method modifies vhost to be fully consistent with the new directives. + + ..note :: When a directive with the same name already exists in the + config block, the first instance will be replaced. Otherwise, the directive + will be appended/prepended to the config block as in add_server_directives. + + ..todo :: Doesn't match server blocks whose server_name directives are + split across multiple conf files. + + :param :class:`~certbot_nginx.obj.VirtualHost` vhost: The vhost + whose information we use to match on + :param list directives: The directives to add + :param bool insert_at_top: True if the directives need to be inserted at the top + of the server block instead of the bottom + + """ + self._modify_server_directives(vhost, + functools.partial(_update_or_add_directives, directives, insert_at_top)) def remove_server_directives(self, vhost, directive_name, match_func=None): """Remove all directives of type directive_name. @@ -524,26 +543,17 @@ def _is_ssl_on_directive(entry): len(entry) == 2 and entry[0] == 'ssl' and entry[1] == 'on') -def _add_directives(directives, replace, insert_at_top, block): - """Adds or replaces directives in a config block. - - When replace=False, it's an error to try and add a nonrepeatable directive that already - exists in the config block with a conflicting value. - - When replace=True and a directive with the same name already exists in the - config block, the first instance will be replaced. Otherwise, the directive - will be added to the config block. - - ..todo :: Find directives that are in included files. - - :param list directives: The new directives. - :param bool replace: Described above. - :param bool insert_at_top: Described above. - :param list block: The block to replace in - - """ +def _add_directives(directives, insert_at_top, block): + """Adds directives to a config block.""" for directive in directives: - _add_directive(block, directive, replace, insert_at_top) + _add_directive(block, directive, insert_at_top) + if block and '\n' not in block[-1]: # could be " \n " or ["\n"] ! + block.append(nginxparser.UnspacedList('\n')) + +def _update_or_add_directives(directives, insert_at_top, block): + """Adds or replaces directives in a config block.""" + for directive in directives: + _update_or_add_directive(block, directive, insert_at_top) if block and '\n' not in block[-1]: # could be " \n " or ["\n"] ! block.append(nginxparser.UnspacedList('\n')) @@ -601,28 +611,20 @@ def _find_location(block, directive_name, match_func=None): return next((index for index, line in enumerate(block) \ if line and line[0] == directive_name and (match_func is None or match_func(line))), None) -def _add_directive(block, directive, replace, insert_at_top): - """Adds or replaces a single directive in a config block. +def _is_whitespace_or_comment(directive): + """Is this directive either a whitespace or comment directive?""" + return len(directive) == 0 or directive[0] == '#' - See _add_directives for more documentation. - - """ - directive = nginxparser.UnspacedList(directive) - def is_whitespace_or_comment(directive): - """Is this directive either a whitespace or comment directive?""" - return len(directive) == 0 or directive[0] == '#' - if is_whitespace_or_comment(directive): +def _add_directive(block, directive, insert_at_top): + if not isinstance(directive, nginxparser.UnspacedList): + directive = nginxparser.UnspacedList(directive) + if _is_whitespace_or_comment(directive): # whitespace or comment block.append(directive) return location = _find_location(block, directive[0]) - if replace: - if location is not None: - block[location] = directive - comment_directive(block, location) - return # Append or prepend 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. @@ -647,7 +649,7 @@ def _add_directive(block, directive, replace, insert_at_top): for included_directive in included_directives: included_dir_loc = _find_location(block, included_directive[0]) included_dir_name = included_directive[0] - if not is_whitespace_or_comment(included_directive) \ + if not _is_whitespace_or_comment(included_directive) \ and not can_append(included_dir_loc, included_dir_name): if block[included_dir_loc] != included_directive: raise errors.MisconfigurationError(err_fmt.format(included_directive, @@ -668,6 +670,27 @@ def _add_directive(block, directive, replace, insert_at_top): elif block[location] != directive: raise errors.MisconfigurationError(err_fmt.format(directive, block[location])) +def _update_directive(block, directive, location): + block[location] = directive + comment_directive(block, location) + +def _update_or_add_directive(block, directive, insert_at_top): + if not isinstance(directive, nginxparser.UnspacedList): + directive = nginxparser.UnspacedList(directive) + if _is_whitespace_or_comment(directive): + # whitespace or comment + block.append(directive) + return + + location = _find_location(block, directive[0]) + + # we can update directive + if location is not None: + _update_directive(block, directive, location) + return + + _add_directive(block, directive, insert_at_top) + def _is_certbot_comment(directive): return '#' in directive and COMMENT in directive diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 9489b534a..34abf2f0d 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -113,8 +113,7 @@ class NginxConfiguratorTest(util.NginxTest): None, [0]) self.config.parser.add_server_directives( mock_vhost, - [['listen', ' ', '5001', ' ', 'ssl']], - replace=False) + [['listen', ' ', '5001', ' ', 'ssl']]) self.config.save() # pylint: disable=protected-access @@ -206,9 +205,9 @@ class NginxConfiguratorTest(util.NginxTest): "example/chain.pem", None) - @mock.patch('certbot_nginx.parser.NginxParser.add_server_directives') - def test_deploy_cert_raise_on_add_error(self, mock_add_server_directives): - mock_add_server_directives.side_effect = errors.MisconfigurationError() + @mock.patch('certbot_nginx.parser.NginxParser.update_or_add_server_directives') + def test_deploy_cert_raise_on_add_error(self, mock_update_or_add_server_directives): + mock_update_or_add_server_directives.side_effect = errors.MisconfigurationError() self.assertRaises( errors.PluginError, self.config.deploy_cert, diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 5fce6f25a..9db251d59 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -206,8 +206,7 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods mock_vhost.path = [0] nparser.add_server_directives(mock_vhost, [['foo', 'bar'], ['ssl_certificate', - '/etc/ssl/cert2.pem']], - replace=False) + '/etc/ssl/cert2.pem']]) nparser.remove_server_directives(mock_vhost, 'foo') nparser.remove_server_directives(mock_vhost, 'ssl_certificate') self.assertEqual(nparser.parsed[example_com], @@ -226,8 +225,7 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods None, [10, 1, 9]) nparser.add_server_directives(mock_vhost, [['foo', 'bar'], ['\n ', 'ssl_certificate', ' ', - '/etc/ssl/cert.pem']], - replace=False) + '/etc/ssl/cert.pem']]) 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))) @@ -239,10 +237,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods mock_vhost.path = [0] nparser.add_server_directives(mock_vhost, [['foo', 'bar'], ['ssl_certificate', - '/etc/ssl/cert2.pem']], - replace=False) - nparser.add_server_directives(mock_vhost, [['foo', 'bar']], - replace=False) + '/etc/ssl/cert2.pem']]) + nparser.add_server_directives(mock_vhost, [['foo', 'bar']]) from certbot_nginx.parser import COMMENT self.assertEqual(nparser.parsed[example_com], [[['server'], [['listen', '69.50.225.155:9000'], @@ -264,8 +260,7 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods nparser.add_server_directives, mock_vhost, [['foo', 'bar'], - ['ssl_certificate', '/etc/ssl/cert2.pem']], - replace=False) + ['ssl_certificate', '/etc/ssl/cert2.pem']]) def test_comment_is_repeatable(self): nparser = parser.NginxParser(self.config_path) @@ -275,12 +270,10 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods set(['.example.com', 'example.*']), None, [0]) nparser.add_server_directives(mock_vhost, - [['\n ', '#', ' ', 'what a nice comment']], - replace=False) + [['\n ', '#', ' ', 'what a nice comment']]) nparser.add_server_directives(mock_vhost, [['\n ', 'include', ' ', - nparser.abs_path('comment_in_file.conf')]], - replace=False) + nparser.abs_path('comment_in_file.conf')]]) from certbot_nginx.parser import COMMENT self.assertEqual(nparser.parsed[example_com], [[['server'], [['listen', '69.50.225.155:9000'], @@ -299,8 +292,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods target = set(['.example.com', 'example.*']) filep = nparser.abs_path('sites-enabled/example.com') mock_vhost = obj.VirtualHost(filep, None, None, None, target, None, [0]) - nparser.add_server_directives( - mock_vhost, [['server_name', 'foobar.com']], replace=True) + nparser.update_or_add_server_directives( + mock_vhost, [['server_name', 'foobar.com']]) from certbot_nginx.parser import COMMENT self.assertEqual( nparser.parsed[filep], @@ -310,8 +303,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods ['server_name', 'example.*'], [] ]]]) mock_vhost.names = set(['foobar.com', 'example.*']) - nparser.add_server_directives( - mock_vhost, [['ssl_certificate', 'cert.pem']], replace=True) + nparser.update_or_add_server_directives( + mock_vhost, [['ssl_certificate', 'cert.pem']]) self.assertEqual( nparser.parsed[filep], [[['server'], [['listen', '69.50.225.155:9000'],