Merge remote-tracking branch 'github/letsencrypt/master' into mock-2.6

letsencrypt-apache/letsencrypt_apache/configurator.py

@@ -84,7 +84,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
     description = "Apache Web Server - Alpha"
 
-
     @classmethod
     def add_parser_arguments(cls, add):
         add("ctl", default=constants.CLI_DEFAULTS["ctl"],
@@ -138,6 +137,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :raises .errors.PluginError: If there is any other error
 
         """
+        # Verify Apache is installed
+        for exe in (self.conf("ctl"), self.conf("enmod"),
+                    self.conf("dismod"), self.conf("init-script")):
+            if not le_util.exe_exists(exe):
+                raise errors.NoInstallationError
+
         # Make sure configuration is valid
         self.config_test()
 
@@ -283,7 +288,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self.assoc[target_name] = vhost
         return vhost
 
-
     def _find_best_vhost(self, target_name):
         """Finds the best vhost for a target_name.
 
@@ -492,7 +496,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
         if "ssl_module" not in self.parser.modules:
-            logger.info("Loading mod_ssl into Apache Server")
             self.enable_mod("ssl", temp=temp)
 
         # Check for Listen <port>
@@ -583,7 +586,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         ssl_vhost = self._create_vhost(vh_p)
         self.vhosts.append(ssl_vhost)
 
-
         # NOTE: Searches through Augeas seem to ruin changes to directives
         #       The configuration must also be saved before being searched
         #       for the new directives; For these reasons... this is tacked
@@ -794,7 +796,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise errors.PluginError(
                 "Let's Encrypt has already enabled redirection")
 
-
     def _create_redirect_vhost(self, ssl_vhost):
         """Creates an http_vhost specifically to redirect for the ssl_vhost.
 
@@ -997,22 +998,41 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
         # Support Debian specific setup
-        if (not os.path.isdir(os.path.join(self.parser.root, "mods-available"))
-                or not os.path.isdir(
-                    os.path.join(self.parser.root, "mods-enabled"))):
+        avail_path = os.path.join(self.parser.root, "mods-available")
+        enabled_path = os.path.join(self.parser.root, "mods-enabled")
+        if not os.path.isdir(avail_path) or not os.path.isdir(enabled_path):
             raise errors.NotSupportedError(
                 "Unsupported directory layout. You may try to enable mod %s "
                 "and try again." % mod_name)
 
+        deps = _get_mod_deps(mod_name)
+
+        # Enable all dependencies
+        for dep in deps:
+            if (dep + "_module") not in self.parser.modules:
+                self._enable_mod_debian(dep, temp)
+                self._add_parser_mod(dep)
+
+                note = "Enabled dependency of %s module - %s" % (mod_name, dep)
+                if not temp:
+                    self.save_notes += note + os.linesep
+                logger.debug(note)
+
+        # Enable actual module
         self._enable_mod_debian(mod_name, temp)
-        self.save_notes += "Enabled %s module in Apache" % mod_name
-        logger.debug("Enabled Apache %s module", mod_name)
+        self._add_parser_mod(mod_name)
+
+        if not temp:
+            self.save_notes += "Enabled %s module in Apache\n" % mod_name
+        logger.info("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.conf("ctl"))
 
+    def _add_parser_mod(self, mod_name):
+        """Shortcut for updating parser modules."""
         self.parser.modules.add(mod_name + "_module")
         self.parser.modules.add("mod_" + mod_name + ".c")
 
@@ -1140,6 +1160,25 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             self.parser.init_modules()
 
 
+def _get_mod_deps(mod_name):
+    """Get known module dependencies.
+
+    .. note:: This does not need to be accurate in order for the client to
+        run.  This simply keeps things clean if the user decides to revert
+        changes.
+    .. warning:: If all deps are not included, it may cause incorrect parsing
+        behavior, due to enable_mod's shortcut for updating the parser's
+        currently defined modules (`.ApacheConfigurator._add_parser_mod`)
+        This would only present a major problem in extremely atypical
+        configs that use ifmod for the missing deps.
+
+    """
+    deps = {
+        "ssl": ["setenvif", "mime", "socache_shmcb"]
+    }
+    return deps.get(mod_name, [])
+
+
 def apache_restart(apache_init_script):
     """Restarts the Apache Server.
 

letsencrypt-apache/letsencrypt_apache/obj.py

@@ -14,8 +14,8 @@ class Addr(common.Addr):
         """
         if isinstance(other, self.__class__):
             return ((self.tup == other.tup) or
-                    (self.tup[0] == other.tup[0]
-                     and self.is_wildcard() and other.is_wildcard()))
+                    (self.tup[0] == other.tup[0] and
+                     self.is_wildcard() and other.is_wildcard()))
         return False
 
     def __ne__(self, other):

letsencrypt-apache/letsencrypt_apache/parser.py

@@ -195,8 +195,7 @@ class ApacheParser(object):
             self.aug.set(nvh_path + "/arg", args[0])
         else:
             for i, arg in enumerate(args):
-                self.aug.set("%s/arg[%d]" % (nvh_path, i+1), arg)
-
+                self.aug.set("%s/arg[%d]" % (nvh_path, i + 1), arg)
 
     def _get_ifmod(self, aug_conf_path, mod):
         """Returns the path to <IfMod mod> and creates one if it doesn't exist.
@@ -242,6 +241,10 @@ class ApacheParser(object):
         Directives should be in the form of a case insensitive regex currently
 
         .. todo:: arg should probably be a list
+        .. todo:: arg search currently only supports direct matching. It does
+            not handle the case of variables or quoted arguments. This should
+            be adapted to use a generic search for the directive and then do a
+            case-insensitive self.get_arg filter
 
         Note: Augeas is inherently case sensitive while Apache is case
         insensitive.  Augeas 1.0 allows case insensitive regexes like
@@ -316,6 +319,14 @@ class ApacheParser(object):
 
         """
         value = self.aug.get(match)
+
+        # No need to strip quotes for variables, as apache2ctl already does this
+        # but we do need to strip quotes for all normal arguments.
+
+        # Note: normal argument may be a quoted variable
+        # e.g. strip now, not later
+        value = value.strip("'\"")
+
         variables = ApacheParser.arg_var_interpreter.findall(value)
 
         for var in variables:
@@ -391,10 +402,15 @@ class ApacheParser(object):
         #     logger.error("Error: Invalid regexp characters in %s", arg)
         #     return []
 
+        # Remove beginning and ending quotes
+        arg = arg.strip("'\"")
+
         # Standardize the include argument based on server root
         if not arg.startswith("/"):
             # Normpath will condense ../
             arg = os.path.normpath(os.path.join(self.root, arg))
+        else:
+            arg = os.path.normpath(arg)
 
         # Attempts to add a transform to the file if one does not already exist
         if os.path.isdir(arg):
@@ -568,7 +584,7 @@ def case_i(string):
     :param str string: string to make case i regex
 
     """
-    return "".join(["["+c.upper()+c.lower()+"]"
+    return "".join(["[" + c.upper() + c.lower() + "]"
                     if c.isalpha() else c for c in re.escape(string)])
 
 

letsencrypt-apache/letsencrypt_apache/tests/complex_parsing_test.py

@@ -32,6 +32,7 @@ class ComplexParserTest(util.ParserTest):
                 "COMPLEX": "",
                 "tls_port": "1234",
                 "fnmatch_filename": "test_fnmatch.conf",
+                "tls_port_str": "1234"
             }
         )
 
@@ -49,6 +50,12 @@ class ComplexParserTest(util.ParserTest):
         self.assertEqual(len(matches), 1)
         self.assertEqual(self.parser.get_arg(matches[0]), "1234")
 
+    def test_basic_variable_parsing_quotes(self):
+        matches = self.parser.find_dir("TestVariablePortStr")
+
+        self.assertEqual(len(matches), 1)
+        self.assertEqual(self.parser.get_arg(matches[0]), "1234")
+
     def test_invalid_variable_parsing(self):
         del self.parser.variables["tls_port"]
 
@@ -56,7 +63,6 @@ class ComplexParserTest(util.ParserTest):
         self.assertRaises(
             errors.PluginError, self.parser.get_arg, matches[0])
 
-
     def test_basic_ifdefine(self):
         self.assertEqual(len(self.parser.find_dir("VAR_DIRECTIVE")), 2)
         self.assertEqual(len(self.parser.find_dir("INVALID_VAR_DIRECTIVE")), 0)
@@ -71,7 +77,6 @@ class ComplexParserTest(util.ParserTest):
         self.assertEqual(
             len(self.parser.find_dir("INVALID_NESTED_DIRECTIVE")), 0)
 
-
     def test_load_modules(self):
         """If only first is found, there is bad variable parsing."""
         self.assertTrue("status_module" in self.parser.modules)
@@ -91,6 +96,7 @@ class ComplexParserTest(util.ParserTest):
         else:
             self.assertFalse(self.parser.find_dir("FNMATCH_DIRECTIVE"))
 
+    # NOTE: Only run one test per function otherwise you will have inf recursion
     def test_include(self):
         self.verify_fnmatch("test_fnmatch.?onf")
 
@@ -100,6 +106,15 @@ class ComplexParserTest(util.ParserTest):
     def test_include_fullpath(self):
         self.verify_fnmatch(os.path.join(self.config_path, "test_fnmatch.conf"))
 
+    def test_include_fullpath_trailing_slash(self):
+        self.verify_fnmatch(self.config_path + "//")
+
+    def test_include_single_quotes(self):
+        self.verify_fnmatch("'" + self.config_path + "'")
+
+    def test_include_double_quotes(self):
+        self.verify_fnmatch('"' + self.config_path + '"')
+
     def test_include_variable(self):
         self.verify_fnmatch("../complex_parsing/${fnmatch_filename}")
 

letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py

@@ -37,8 +37,16 @@ class TwoVhost80Test(util.ApacheTest):
         shutil.rmtree(self.config_dir)
         shutil.rmtree(self.work_dir)
 
+    @mock.patch("letsencrypt_apache.configurator.le_util.exe_exists")
+    def test_prepare_no_install(self, mock_exe_exists):
+        mock_exe_exists.return_value = False
+        self.assertRaises(
+            errors.NoInstallationError, self.config.prepare)
+
     @mock.patch("letsencrypt_apache.parser.ApacheParser")
-    def test_prepare_version(self, _):
+    @mock.patch("letsencrypt_apache.configurator.le_util.exe_exists")
+    def test_prepare_version(self, mock_exe_exists, _):
+        mock_exe_exists.return_value = True
         self.config.version = None
         self.config.config_test = mock.Mock()
         self.config.get_version = mock.Mock(return_value=(1, 1))
@@ -551,6 +559,7 @@ class TwoVhost80Test(util.ApacheTest):
         self.assertRaises(
             errors.PluginError,
             self.config.enhance, "letsencrypt.demo", "redirect")
+
     def test_unknown_rewrite2(self):
         # Skip the enable mod
         self.config.parser.modules.add("rewrite_module")

letsencrypt-apache/letsencrypt_apache/tests/parser_test.py

@@ -143,7 +143,7 @@ class BasicParserTest(util.ParserTest):
             'Group: name="www-data" id=33 not_used\n'
         )
         expected_vars = {"TEST": "", "U_MICH": "", "TLS": "443",
-                         "example_path":"Documents/path"}
+                         "example_path": "Documents/path"}
 
         self.parser.update_runtime_variables("ctl")
         self.assertEqual(self.parser.variables, expected_vars)

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

@@ -46,6 +46,8 @@ IncludeOptional sites-enabled/*.conf
 Define COMPLEX
 
 Define tls_port 1234
+Define tls_port_str "1234"
+
 Define fnmatch_filename test_fnmatch.conf
 
 

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

@@ -1,4 +1,5 @@
 TestVariablePort ${tls_port}
+TestVariablePortStr "${tls_port_str}"
 
 LoadModule status_module modules/mod_status.so
 

letsencrypt-apache/letsencrypt_apache/tests/util.py

@@ -66,31 +66,34 @@ def get_apache_configurator(
 
     """
     backups = os.path.join(work_dir, "backups")
+    mock_le_config = mock.MagicMock(
+        apache_server_root=config_path,
+        apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"],
+        backup_dir=backups,
+        config_dir=config_dir,
+        temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
+        in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
+        work_dir=work_dir)
 
     with mock.patch("letsencrypt_apache.configurator."
                     "subprocess.Popen") as mock_popen:
-        with mock.patch("letsencrypt_apache.parser.ApacheParser."
-                        "update_runtime_variables"):
-            # This indicates config_test passes
-            mock_popen().communicate.return_value = ("Fine output", "No problems")
-            mock_popen().returncode = 0
+        # This indicates config_test passes
+        mock_popen().communicate.return_value = ("Fine output", "No problems")
+        mock_popen().returncode = 0
+        with mock.patch("letsencrypt_apache.configurator.le_util."
+                        "exe_exists") as mock_exe_exists:
+            mock_exe_exists.return_value = True
+            with mock.patch("letsencrypt_apache.parser.ApacheParser."
+                            "update_runtime_variables"):
+                config = configurator.ApacheConfigurator(
+                    config=mock_le_config,
+                    name="apache",
+                    version=version)
+                # This allows testing scripts to set it a bit more quickly
+                if conf is not None:
+                    config.conf = conf  # pragma: no cover
 
-            config = configurator.ApacheConfigurator(
-                config=mock.MagicMock(
-                    apache_server_root=config_path,
-                    apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"],
-                    backup_dir=backups,
-                    config_dir=config_dir,
-                    temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
-                    in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
-                    work_dir=work_dir),
-                name="apache",
-                version=version)
-            # This allows testing scripts to set it a bit more quickly
-            if conf is not None:
-                config.conf = conf  # pragma: no cover
-
-            config.prepare()
+                config.prepare()
 
     return config
 

letsencrypt-apache/setup.py

@@ -8,6 +8,7 @@ install_requires = [
     'acme',
     'letsencrypt',
     'python-augeas',
+    'setuptools',  # pkg_resources
     'zope.component',
     'zope.interface',
 ]
@@ -24,7 +25,7 @@ setup(
     entry_points={
         'letsencrypt.plugins': [
             'apache = letsencrypt_apache.configurator:ApacheConfigurator',
-         ],
+        ],
     },
     include_package_data=True,
 )