From de4540a1c7740605c6c1ced2a8306ee3c497f4e6 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Fri, 17 Jul 2015 14:09:46 -0700 Subject: [PATCH] some cleanup --- .../letsencrypt_apache/configurator.py | 60 +++++++++++++------ .../letsencrypt_apache/parser.py | 27 ++++++--- .../tests/configurator_test.py | 7 +-- .../letsencrypt_apache/tests/parser_test.py | 2 +- .../testdata/complex_parsing/apache2.conf | 53 ++++++++++++++++ .../tests/testdata/complex_parsing/test.conf | 1 + .../letsencrypt_apache/tests/util.py | 9 +-- 7 files changed, 123 insertions(+), 36 deletions(-) create mode 100644 letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/apache2.conf create mode 100644 letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/test.conf diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 7edcb8cc8..69b635bf0 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -59,6 +59,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): .. todo:: Verify permissions on configuration root... it is easier than checking permissions on each of the relative directories and less error prone. + .. todo:: Write a server protocol finder. Listen or + Protocol . This can verify partial setups are correct :ivar config: Configuration. :type config: :class:`~letsencrypt.interfaces.IConfig` @@ -78,6 +80,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): description = "Apache Web Server - Alpha" + # Kept in same function to avoid multiple compilations of the regex + + private_ips_regex = re.compile( + r"(^127\.0\.0\.1)|(^10\.)|(^172\.1[6-9]\.)|" + r"(^172\.2[0-9]\.)|(^172\.3[0-1]\.)|(^192\.168\.)") + @classmethod def add_parser_arguments(cls, add): add("server-root", default=constants.CLI_DEFAULTS["server_root"], @@ -223,7 +231,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if target_name in self.assoc: return self.assoc[target_name] - # Make a new association + # Try to find a reasonable vhost vhost = self._find_best_vhost(target_name) if vhost is not None: if not vhost.ssl: @@ -300,24 +308,35 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ all_names = set() - # Kept in same function to avoid multiple compilations of the regex - priv_ip_regex = (r"(^127\.0\.0\.1)|(^10\.)|(^172\.1[6-9]\.)|" - r"(^172\.2[0-9]\.)|(^172\.3[0-1]\.)|(^192\.168\.)") - private_ips = re.compile(priv_ip_regex) - for vhost in self.vhosts: all_names.update(vhost.names) for addr in vhost.addrs: - # If it isn't a private IP, do a reverse DNS lookup - if not private_ips.match(addr.get_addr()): - try: - socket.inet_aton(addr.get_addr()) - all_names.add(socket.gethostbyaddr(addr.get_addr())[0]) - except (socket.error, socket.herror, socket.timeout): - continue + name = get_name_from_ip(addr) + if name: + all_names.add(name) return all_names + def get_name_from_ip(self, addr): + """Returns a reverse dns name if available. + + :param addr: IP Address + :type addr: ~.common.Addr + + :returns: name + :rtype: str + + """ + # If it isn't a private IP, do a reverse DNS lookup + if not private_ips_regex.match(addr.get_addr()): + try: + socket.inet_aton(addr.get_addr()) + return socket.gethostbyaddr(addr.get_addr())[0] + except (socket.error, socket.herror, socket.timeout): + pass + + return "" + def _add_servernames(self, host): """Helper function for get_virtual_hosts(). @@ -415,7 +434,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ path = self.parser.add_dir_to_ifmodssl( parser.get_aug_path( - self.parser.loc["name"]), "NameVirtualHost", str(addr)) + self.parser.loc["name"]), "NameVirtualHost", [str(addr)]) self.save_notes += "Setting %s to be NameBasedVirtualHost\n" % addr self.save_notes += "\tDirective added to %s\n" % path @@ -438,11 +457,18 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if not self.parser.find_dir(parser.case_i("Listen"), port): logger.debug("No Listen {0} directive found. Setting the " "Apache Server to Listen on port {0}".format(port)) - path = self.parser.add_dir_to_ifmodssl( + + if port == "443": + args = [port] + else: + # Non-standard ports should specify https protocol + args = [port, "https"] + + self.parser.add_dir_to_ifmodssl( parser.get_aug_path( - self.parser.loc["listen"]), "Listen", port) + self.parser.loc["listen"]), "Listen", args) self.save_notes += "Added Listen %s directive to %s\n" % ( - port, path) + port, self.parser.loc["listen"]) def make_addrs_sni_ready( self, addrs, default_addr=obj.Addr(("*", "443"))): diff --git a/letsencrypt-apache/letsencrypt_apache/parser.py b/letsencrypt-apache/letsencrypt_apache/parser.py index 30e97814b..ffbacc066 100644 --- a/letsencrypt-apache/letsencrypt_apache/parser.py +++ b/letsencrypt-apache/letsencrypt_apache/parser.py @@ -81,6 +81,8 @@ class ApacheParser(object): dynamic configuration files. These should not be parsed or interpreted. + .. todo:: Create separate compile time variables... simply for arg_get() + """ stdout = self._get_runtime_cfg(ctl) @@ -157,7 +159,7 @@ class ApacheParser(object): return filtered - def add_dir_to_ifmodssl(self, aug_conf_path, directive, val): + def add_dir_to_ifmodssl(self, aug_conf_path, directive, args): """Adds directive and value to IfMod ssl block. Adds given directive and value along configuration path within @@ -165,8 +167,9 @@ class ApacheParser(object): the file, it is created. :param str aug_conf_path: Desired Augeas config path to add directive - :param str directive: Directive you would like to add - :param str val: Value of directive ie. Listen 443, 443 is the value + :param str directive: Directive you would like to add, e.g. Listen + :param args: Values of the directive; str "443" or list of str + :type args: list """ # TODO: Add error checking code... does the path given even exist? @@ -176,7 +179,12 @@ class ApacheParser(object): self.aug.insert(if_mod_path + "arg", "directive", False) nvh_path = if_mod_path + "directive[1]" self.aug.set(nvh_path, directive) - self.aug.set(nvh_path + "/arg", val) + if len(args) == 1: + self.aug.set(nvh_path + "/arg", args[0]) + else: + for i, arg in enumerate(args): + self.aug.set("%s/arg[%d]" % (nvh_path, i), arg) + def _get_ifmod(self, aug_conf_path, mod): """Returns the path to and creates one if it doesn't exist. @@ -195,7 +203,7 @@ class ApacheParser(object): # Strip off "arg" at end of first ifmod path return if_mods[0][:len(if_mods[0]) - 3] - def add_dir(self, aug_conf_path, directive, arg): + def add_dir(self, aug_conf_path, directive, args): """Appends directive to the end fo the file given by aug_conf_path. .. note:: Not added to AugeasConfigurator because it may depend @@ -203,16 +211,17 @@ class ApacheParser(object): :param str aug_conf_path: Augeas configuration path to add directive :param str directive: Directive to add - :param str arg: Value of the directive. ie. Listen 443, 443 is arg + :param args: Value of the directive. ie. Listen 443, 443 is arg + :type args: list or str """ self.aug.set(aug_conf_path + "/directive[last() + 1]", directive) - if isinstance(arg, list): - for i, value in enumerate(arg, 1): + if isinstance(args, list): + for i, value in enumerate(args, 1): self.aug.set( "%s/directive[last()]/arg[%d]" % (aug_conf_path, i), value) else: - self.aug.set(aug_conf_path + "/directive[last()]/arg", arg) + self.aug.set(aug_conf_path + "/directive[last()]/arg", args) def find_dir(self, directive, arg=None, start=None): """Finds directive in the configuration. diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 9304b634f..c156ebe66 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -28,11 +28,8 @@ class TwoVhost80Test(util.ApacheTest): def setUp(self): super(TwoVhost80Test, self).setUp() - with mock.patch("letsencrypt_apache.configurator.ApacheConfigurator." - "mod_loaded") as mock_load: - mock_load.return_value = True - self.config = util.get_apache_configurator( - self.config_path, self.config_dir, self.work_dir) + self.config = util.get_apache_configurator( + self.config_path, self.config_dir, self.work_dir) self.vh_truth = util.get_vh_truth( self.temp_dir, "debian_apache_2_4/two_vhost_80") diff --git a/letsencrypt-apache/letsencrypt_apache/tests/parser_test.py b/letsencrypt-apache/letsencrypt_apache/tests/parser_test.py index 24aa359ed..05224b56d 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/parser_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/parser_test.py @@ -82,7 +82,7 @@ class ApacheParserTest(util.ApacheTest): from letsencrypt_apache.parser import get_aug_path self.parser.add_dir_to_ifmodssl( get_aug_path(self.parser.loc["default"]), - "FakeDirective", "123") + "FakeDirective", ["123"]) matches = self.parser.find_dir("FakeDirective", "123") diff --git a/letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/apache2.conf b/letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/apache2.conf new file mode 100644 index 000000000..a1f4e05fc --- /dev/null +++ b/letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/apache2.conf @@ -0,0 +1,53 @@ +# Global configuration + +PidFile ${APACHE_PID_FILE} + +# +# Timeout: The number of seconds before receives and sends time out. +# +Timeout 300 + +# +# KeepAlive: Whether or not to allow persistent connections (more than +# one request per connection). Set to "Off" to deactivate. +# +KeepAlive On + +# These need to be set in /etc/apache2/envvars +User ${APACHE_RUN_USER} +Group ${APACHE_RUN_GROUP} + +ErrorLog ${APACHE_LOG_DIR}/error.log + +LogLevel warn + +# Include module configuration: +IncludeOptional mods-enabled/*.load +IncludeOptional mods-enabled/*.conf + + + Options FollowSymLinks + AllowOverride None + Require all denied + + + + Options Indexes FollowSymLinks + AllowOverride None + Require all granted + + +# Include generic snippets of statements +IncludeOptional conf-enabled/*.conf + +# Include the virtual host configurations: +IncludeOptional sites-enabled/*.conf + +Define COMPLEX + +Define tls_port 1234 +Define example_path1 Documents/root + + +Include test.conf +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet diff --git a/letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/test.conf b/letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/test.conf new file mode 100644 index 000000000..f724756d5 --- /dev/null +++ b/letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/test.conf @@ -0,0 +1 @@ +TESTDIRECTIVE ${tls_port} diff --git a/letsencrypt-apache/letsencrypt_apache/tests/util.py b/letsencrypt-apache/letsencrypt_apache/tests/util.py index 81de82742..55349c366 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/util.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/util.py @@ -44,10 +44,11 @@ def get_apache_configurator( """ backups = os.path.join(work_dir, "backups") - with mock.patch("letsencrypt_apache.configurator." - "subprocess.Popen") as mock_popen: - # This just states that the ssl module is already loaded - mock_popen().communicate.return_value = ("ssl_module", "") + with mock.patch("letsencrypt_apache.configurator.subprocess.Popen") as mock_popen: + # This indicates config_test passes + mock_popen().communicate.return_value = ("Fine output", "No problems") + mock_popen.returncode.return_value = 0 + # mock_popen().communicate.return_value = ("ssl_module", "") config = configurator.ApacheConfigurator( config=mock.MagicMock( apache_server_root=config_path,