Add tests and comments

letsencrypt-apache/letsencrypt_apache/configurator.py

@@ -710,28 +710,25 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise
 
     def _set_http_header(self, ssl_vhost, header_name):
-        # TODO REWRITE COMMENT
+        """Enables header header_name on ssl_vhost.
-        """Enables the HSTS header on all HTTP responses.
 
-        .. note:: HSTS defends against SSL stripping attacks.
+        If header_name is not already set, a new Header directive is placed in
-
+        ssl_vhost's configuration with arguments from:
-
+        constants.HTTP_HEADER[header_name]
-        Adds the Strict-Transport-Security header with max-age=31536000 (1 year)
-        and includeSubDomains (all subdomains are also set with HSTS).
 
         .. note:: This function saves the configuration
 
         :param ssl_vhost: Destination of traffic, an ssl enabled vhost
         :type ssl_vhost: :class:`~letsencrypt_apache.obj.VirtualHost`
 
-        :param unused_options: Not currently used
+        :param header_name: a header name, e.g: Strict-Transport-Security
-        :type unused_options: Not Available
+        :type str
 
         :returns: Success, general_vhost (HTTP vhost)
         :rtype: (bool, :class:`~letsencrypt_apache.obj.VirtualHost`)
 
         :raises .errors.PluginError: If no viable HTTP host can be created or
-            used for the HSTS.
+            set with header header_name.
 
         """
         if "headers_module" not in self.parser.modules:
@@ -744,7 +741,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self.parser.add_dir(ssl_vhost.path, "Header",
                 constants.HEADER_ARGS[header_name])
 
-        self.save_notes += ("Adding %s header to ssl vhost in %s\n" % 
+        self.save_notes += ("Adding %s header to ssl vhost in %s\n" %
                 (header_name, ssl_vhost.filep))
 
         self.save()
@@ -752,10 +749,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 ssl_vhost.filep)
 
     def _verify_no_http_header(self, ssl_vhost, header_name):
-        # TODO revise comment
+        """Checks to see if existing header_name header is in place.
-        """Checks to see if existing HSTS settings is in place.
 
-        Checks to see if virtualhost already contains a HSTS header
+        Checks to see if virtualhost already contains a header_name header
 
         :param vhost: vhost to check
         :type vhost: :class:`~letsencrypt_apache.obj.VirtualHost`
@@ -763,17 +759,17 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         :returns: boolean
         :rtype: (bool)
 
-        :raises errors.PluginError: When an HSTS header exists
+        :raises errors.PluginError: When header header_name exists
 
         """
         header_path = self.parser.find_dir("Header", None, start=ssl_vhost.path)
         if header_path:
             # "Existing Header directive for virtualhost"
             for match in header_path:
-                if self.aug.get(match) == header_name.lower():
+                if self.aug.get(match).lower() == header_name.lower():
                     raise errors.PluginError("Existing %s header" %
                             (header_name))
-          
+
     def _enable_redirect(self, ssl_vhost, unused_options):
         """Redirect all equivalent HTTP traffic to ssl_vhost.
 

letsencrypt-apache/letsencrypt_apache/constants.py

@@ -34,8 +34,8 @@ HSTS_ARGS = ["always", "set", "Strict-Transport-Security",
 """Apache header arguments for HSTS"""
 
 UIR_ARGS = ["always", "set", "Content-Security-Policy",
-    "upgrade-insecure-requests"] 
+    "upgrade-insecure-requests"]
 
-HEADER_ARGS = {"Strict-Transport-Security" : HSTS_ARGS,
+HEADER_ARGS = {"Strict-Transport-Security": HSTS_ARGS,
-        "Upgrade-Insecure-Requests" : UIR_ARGS}
+        "Upgrade-Insecure-Requests": UIR_ARGS}
 

letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py

@@ -15,7 +15,6 @@ from letsencrypt import errors
 from letsencrypt.tests import acme_util
 
 from letsencrypt_apache import configurator
-from letsencrypt_apache import constants
 from letsencrypt_apache import obj
 
 from letsencrypt_apache.tests import util
@@ -532,29 +531,19 @@ class TwoVhost80Test(util.ApacheTest):
         # four args to HSTS header
         self.assertEqual(len(hsts_header), 4)
 
-    @mock.patch("letsencrypt.le_util.run_script")
+    def test_http_header_hsts_twice(self):
-    @mock.patch("letsencrypt.le_util.exe_exists")
-    def test_http_header_hsts_with_conflict(self, mock_exe, _):
-        mock_exe.return_value = True
-        self.config.parser.update_runtime_variables = mock.Mock()
         self.config.parser.modules.add("mod_ssl.c")
-
+        # skip the enable mod
-        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[3])
+        self.config.parser.modules.add("headers_module")
-        self.config.parser.add_dir(
-            ssl_vhost.path, "Header", constants.HEADER_ARGS[
-                "Strict-Transport-Security"])
 
         # This will create an ssl vhost for letsencrypt.demo
-        self.config.enhance(self.vh_truth[3].name, "http-header",
+        self.config.enhance("encryption-example.demo", "http-header",
                 "Strict-Transport-Security")
 
-        # These are not immediately available in find_dir even with save() and
+        self.assertRaises(
-        # load(). They must be found in sites-available
+            errors.PluginError,
-        hsts_header = self.config.parser.find_dir(
+            self.config.enhance, "encryption-example.demo", "http-header",
-                "Header", None, ssl_vhost.path)
+            "Strict-Transport-Security")
-
-        # four args to HSTS header
-        self.assertEqual(len(hsts_header), 4)
 
 
     @mock.patch("letsencrypt.le_util.run_script")