diff options
author | ohemorange <ebportnoy@gmail.com> | 2017-07-26 23:57:25 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-07-26 23:57:25 +0300 |
commit | 0321c0cb4cc1515efc1f787678c50874a911d38a (patch) | |
tree | 8e0f16ce200cd991ddc15c08b7608c1c3ae638dd | |
parent | 142ced234b7561c24f2fd089090cf16b64566ae8 (diff) |
Change add_server_directives replace=True behavior to attempt to replace, but append on failure to find. (#4956)
* Change add_server_directives replace=True behavior to attempt to replace, but append on failure to find.
* Remove try/except around add_server_directives
-rw-r--r-- | certbot-nginx/certbot_nginx/configurator.py | 14 | ||||
-rw-r--r-- | certbot-nginx/certbot_nginx/parser.py | 92 | ||||
-rw-r--r-- | certbot-nginx/certbot_nginx/tests/parser_test.py | 15 |
3 files changed, 59 insertions, 62 deletions
diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 63f659453..ec657a9db 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -198,16 +198,10 @@ class NginxConfigurator(common.Plugin): cert_directives = [['\n', 'ssl_certificate', ' ', fullchain_path], ['\n', 'ssl_certificate_key', ' ', key_path]] - try: - self.parser.add_server_directives(vhost, - cert_directives, replace=True) - logger.info("Deployed Certificate to VirtualHost %s for %s", - vhost.filep, vhost.names) - except errors.MisconfigurationError as error: - logger.debug(error) - # Presumably break here so that the virtualhost is not modified - raise errors.PluginError("Cannot find a cert or key directive in {0} for {1}. " - "VirtualHost was not modified.".format(vhost.filep, vhost.names)) + self.parser.add_server_directives(vhost, + cert_directives, replace=True) + logger.info("Deployed Certificate to VirtualHost %s for %s", + vhost.filep, vhost.names) self.save_notes += ("Changed vhost at %s with addresses of %s\n" % (vhost.filep, diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 4e4aa36ca..158cb9929 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -278,8 +278,8 @@ class NginxParser(object): This method modifies vhost to be fully consistent with the new directives. - ..note :: If replace is True, this raises a misconfiguration error - if the directive does not already exist. + ..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. @@ -464,8 +464,9 @@ def _add_directives(block, directives, replace): 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. + 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. @@ -547,49 +548,46 @@ def _add_directive(block, directive, replace): location = find_location(directive) if replace: - if location is None: - raise errors.MisconfigurationError( - 'expected directive for {0} in the Nginx ' - 'config but did not find it.'.format(directive[0])) - block[location] = directive - _comment_directive(block, location) - 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. - - # handle flat include files - - directive_name = directive[0] - def can_append(loc, dir_name): - """ Can we append this directive to the block? """ - return loc is None or (isinstance(dir_name, str) and dir_name in REPEATABLE_DIRECTIVES) - - err_fmt = 'tried to insert directive "{0}" but found conflicting "{1}".' - - # Give a better error message about the specific directive than Nginx's "fail to restart" - if directive_name == INCLUDE: - # in theory, we might want to do this recursively, but in practice, that's really not - # necessary because we know what file we're talking about (and if we don't recurse, we - # just give a worse error message) - included_directives = _parse_ssl_options(directive[1]) - - for included_directive in included_directives: - included_dir_loc = find_location(included_directive) - included_dir_name = included_directive[0] - 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, - block[included_dir_loc])) - else: - _comment_out_directive(block, included_dir_loc, directive[1]) - - if can_append(location, directive_name): - block.append(directive) - _comment_directive(block, len(block) - 1) - elif block[location] != directive: - raise errors.MisconfigurationError(err_fmt.format(directive, block[location])) + if location is not None: + block[location] = directive + _comment_directive(block, location) + return + # 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. + + # handle flat include files + + directive_name = directive[0] + def can_append(loc, dir_name): + """ Can we append this directive to the block? """ + return loc is None or (isinstance(dir_name, str) and dir_name in REPEATABLE_DIRECTIVES) + + err_fmt = 'tried to insert directive "{0}" but found conflicting "{1}".' + + # Give a better error message about the specific directive than Nginx's "fail to restart" + if directive_name == INCLUDE: + # in theory, we might want to do this recursively, but in practice, that's really not + # necessary because we know what file we're talking about (and if we don't recurse, we + # just give a worse error message) + included_directives = _parse_ssl_options(directive[1]) + + for included_directive in included_directives: + included_dir_loc = find_location(included_directive) + included_dir_name = included_directive[0] + 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, + block[included_dir_loc])) + else: + _comment_out_directive(block, included_dir_loc, directive[1]) + + if can_append(location, directive_name): + block.append(directive) + _comment_directive(block, len(block) - 1) + elif block[location] != directive: + raise errors.MisconfigurationError(err_fmt.format(directive, block[location])) def _apply_global_addr_ssl(addr_to_ssl, parsed_server): """Apply global sslishness information to the parsed server block diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 3877bf5d4..e655bc3e3 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -273,11 +273,16 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods ['server_name', 'example.*'], [] ]]]) mock_vhost.names = set(['foobar.com', 'example.*']) - self.assertRaises(errors.MisconfigurationError, - nparser.add_server_directives, - mock_vhost, - [['ssl_certificate', 'cert.pem']], - replace=True) + nparser.add_server_directives( + mock_vhost, [['ssl_certificate', 'cert.pem']], replace=True) + self.assertEqual( + nparser.parsed[filep], + [[['server'], [['listen', '69.50.225.155:9000'], + ['listen', '127.0.0.1'], + ['server_name', 'foobar.com'], ['#', COMMENT], + ['server_name', 'example.*'], [], + ['ssl_certificate', 'cert.pem'], ['#', COMMENT], [], + ]]]) def test_get_best_match(self): target_name = 'www.eff.org' |