diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index cb6029477..a513557af 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -64,7 +64,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): This class can adequately configure most typical configurations but is not ready to handle very complex configurations. - .. todo:: Always use self.parser.aug_get rather than self.aug.get .. todo:: Verify permissions on configuration root... it is easier than checking permissions on each of the relative directories and less error prone. @@ -332,7 +331,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): for name in name_match: args = self.aug.match(name + "/*") for arg in args: - host.add_name(self.aug.get(arg)) + host.add_name(self.parser.get_arg(arg)) def _create_vhost(self, path): """Used by get_virtual_hosts to create vhost objects @@ -346,7 +345,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): addrs = set() args = self.aug.match(path + "/arg") for arg in args: - addrs.add(common.Addr.fromstring(self.aug.get(arg))) + addrs.add(common.Addr.fromstring(self.parser.get_arg(arg))) is_ssl = False if self.parser.find_dir( @@ -514,7 +513,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): for addr in ssl_addr_p: old_addr = common.Addr.fromstring( - str(self.aug.get(addr))) + str(self.parser.get_arg(addr))) ssl_addr = old_addr.get_addr_obj("443") self.aug.set(addr, str(ssl_addr)) ssl_addrs.add(ssl_addr) @@ -868,8 +867,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): errors.MisconfigurationError( "Too many cert/key directives in vhost") - cert = os.path.abspath(self.aug.get(cert_path[0])) - key = os.path.abspath(self.aug.get(key_path[0])) + cert = os.path.abspath(self.parser.get_arg(cert_path[0])) + key = os.path.abspath(self.parser.get_arg(key_path[0])) c_k.add((cert, key, get_file_path(cert_path[0]))) return c_k @@ -942,13 +941,19 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "and try again." % mod_name) self._enable_mod_debian(mod_name) + self.save_notes += "Enabled %s module in Apache" % mod_name + logger.debug("Enabled Apache %s module", mod_name) + + # Modules can enable additional config files. Variables may be defined + # within these new configuration sections. + # Restart is not necessary as DUMP_RUN_CFG uses latest config. + self.parser.update_runtime_variables() self.parser.modules.add(mod_name + "_module") self.parser.modules.add("mod_" + mod_name) def _enable_mod_debian(self, mod_name): """Assumes mods-available, mods-enabled layout.""" - # TODO: This can be further updated to not require all files. if mod_name == "ssl": self._enable_mod_debian_files(["ssl.conf", "ssl.load"]) @@ -957,7 +962,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): else: raise NotImplemented - def _enable_mod_debian_files(self, filenames): + def _enable_mod_debian_files(self, filenames, mod_name): """Move over all required files into mods-enabled.""" mods_available = os.path.join(self.parser.root, "mods-available") mods_enabled = os.path.join(self.parser.root, "mods-enabled") @@ -972,10 +977,14 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Register and symlink files for filename in files: enabled_path = os.path.join(mods_enabled, filename) + if os.path.isfile(enabled_path): + logger.debug( + "Error - enabling module %s, filepath already exists " + "%s", mod_name, enabled_path) + raise errors.PluginError("Error enabling module %s" % mod_name) self.reverter.register_file_creation(False, enabled_path) os.symlink(os.path.join(mods_available, filename), enabled_path) - def mod_loaded(self, module): """Checks to see if mod_ssl is loaded diff --git a/letsencrypt-apache/letsencrypt_apache/parser.py b/letsencrypt-apache/letsencrypt_apache/parser.py index dfd94db98..30e97814b 100644 --- a/letsencrypt-apache/letsencrypt_apache/parser.py +++ b/letsencrypt-apache/letsencrypt_apache/parser.py @@ -1,5 +1,6 @@ """ApacheParser is a member object of the ApacheConfigurator class.""" import collections +import fnmatch import itertools import logging import os @@ -13,6 +14,7 @@ logger = logging.getLogger(__name__) arg_var_interpreter = re.compile(r"\$\{[^ \}]*}") +fnmatch_chars = set(["*", "?", "\\", "[", "]"]) class ApacheParser(object): @@ -29,7 +31,8 @@ class ApacheParser(object): # https://httpd.apache.org/docs/2.4/mod/core.html#define # https://httpd.apache.org/docs/2.4/mod/core.html#ifdefine # This only handles invocation parameters and Define directives! - self.variables = self._init_runtime_variables(ctl) + self.variables = {} + self.update_runtime_variables(ctl) # Find configuration root and make sure augeas can parse it. self.aug = aug @@ -67,11 +70,11 @@ class ApacheParser(object): for match_name, match_filename in itertools.izip( iterator, iterator): - self.modules.add(self.aug.get(match_name)) + self.modules.add(self.get_arg(match_name)) self.modules.add( - os.path.basename(self.aug.get(match_filename))[:-2] + "c") + os.path.basename(self.get_arg(match_filename))[:-2] + "c") - def _init_runtime_variables(self, ctl): + def update_runtime_variables(self, ctl): """" .. note:: Compile time variables (apache2ctl -V) are not used within the @@ -94,7 +97,7 @@ class ApacheParser(object): parts = match.partition("=") variables[parts[0]] = parts[2] - return variables + self.variables = variables def _get_runtime_cfg(self, ctl): """Get runtime configuration info. @@ -269,13 +272,15 @@ class ApacheParser(object): ordered_matches = [] + # TODO: Wildcards should be included in alphabetical order + # https://httpd.apache.org/docs/2.4/mod/core.html#include for match in matches: dir = self.aug.get(match).lower() if dir == "include" or dir == "includeoptional": # start[6:] to strip off /files ordered_matches.extend(self.find_dir( directive, arg, self._get_include_path( - strip_dir(start[6:]), self.aug.get(match + "/arg")))) + self.get_arg(match + "/arg")))) else: ordered_matches.extend(self.aug.match(match + arg_suffix)) @@ -334,7 +339,7 @@ class ApacheParser(object): return True - def _get_include_path(self, cur_dir, arg): + def _get_include_path(self, arg): """Converts an Apache Include directive into Augeas path. Converts an Apache Include directive argument into an Augeas @@ -342,29 +347,12 @@ class ApacheParser(object): .. todo:: convert to use os.path.join() - :param str cur_dir: current working directory - :param str arg: Argument of Include directive :returns: Augeas path string :rtype: str """ - # Sanity check argument - maybe - # Question: what can the attacker do with control over this string - # Effect parse file... maybe exploit unknown errors in Augeas - # If the attacker can Include anything though... and this function - # only operates on Apache real config data... then the attacker has - # already won. - # Perhaps it is better to simply check the permissions on all - # included files? - # check_config to validate apache config doesn't work because it - # would create a race condition between the check and this input - - # TODO: Maybe... although I am convinced we have lost if - # Apache files can't be trusted. The augeas include path - # should be made to be exact. - # Check to make sure only expected characters are used <- maybe remove # validChars = re.compile("[a-zA-Z0-9.*?_-/]*") # matchObj = validChars.match(arg) @@ -374,11 +362,8 @@ class ApacheParser(object): # Standardize the include argument based on server root if not arg.startswith("/"): - arg = cur_dir + arg - # conf/ is a special variable for ServerRoot in Apache - elif arg.startswith("conf/"): - arg = self.root + arg[4:] - # TODO: Test if Apache allows ../ or ~/ for Includes + # Normpath will condense ../ + arg = os.path.normpath(os.path.join(self.root, arg)) # Attempts to add a transform to the file if one does not already exist self._parse_file(arg) @@ -386,46 +371,38 @@ class ApacheParser(object): # Argument represents an fnmatch regular expression, convert it # Split up the path and convert each into an Augeas accepted regex # then reassemble - if "*" in arg or "?" in arg: - split_arg = arg.split("/") - for idx, split in enumerate(split_arg): - # * and ? are the two special fnmatch characters - if "*" in split or "?" in split: - # Turn it into a augeas regex - # TODO: Can this instead be an augeas glob instead of regex - split_arg[idx] = ("* [label()=~regexp('%s')]" % - self.fnmatch_to_re(split)) - # Reassemble the argument - arg = "/".join(split_arg) + split_arg = arg.split("/") + for idx, split in enumerate(split_arg): + if any(char in fnmatch_chars for char in split): + # Turn it into a augeas regex + # TODO: Can this instead be an augeas glob instead of regex + split_arg[idx] = ("* [label()=~regexp('%s')]" % + self.fnmatch_to_re(split)) + # Reassemble the argument + arg = "/".join(split_arg) # If the include is a directory, just return the directory as a file if arg.endswith("/"): - return get_aug_path(arg[:len(arg)-1]) + return get_aug_path(arg[:-1]) return get_aug_path(arg) def fnmatch_to_re(self, clean_fn_match): # pylint: disable=no-self-use """Method converts Apache's basic fnmatch to regular expression. + Assumption - Configs are assumed to be well-formed and only writable by + privileged users. + + https://apr.apache.org/docs/apr/2.0/apr__fnmatch_8h_source.html + http://apache2.sourcearchive.com/documentation/2.2.16-6/apr__fnmatch_8h_source.html + :param str clean_fn_match: Apache style filename match, similar to globs :returns: regex suitable for augeas :rtype: str """ - # Checkout fnmatch.py in venv/local/lib/python2.7/fnmatch.py - regex = "" - for letter in clean_fn_match: - if letter == ".": - regex = regex + r"\." - elif letter == "*": - regex = regex + ".*" - # According to apache.org ? shouldn't appear - # but in case it is valid... - elif letter == "?": - regex = regex + "." - else: - regex = regex + letter - return regex + # This strips off final /Z(?ms) + return fnmatch.translate(clean_fn_match)[:-7] def _parse_file(self, filepath): """Parse file with Augeas