some cleanup

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 <port> <protocol> or
+        Protocol <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"))):

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 <IfMod mod> 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.

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")

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")
 

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
+
+<Directory />
+	Options FollowSymLinks
+	AllowOverride None
+	Require all denied
+</Directory>
+
+<Directory /var/www/>
+	Options Indexes FollowSymLinks
+	AllowOverride None
+	Require all granted
+</Directory>
+
+# 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

letsencrypt-apache/letsencrypt_apache/tests/testdata/complex_parsing/test.conf

@@ -0,0 +1 @@
+TESTDIRECTIVE ${tls_port}

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,