Merge pull request certbot#3523 from certbot/nginx-ocsp-stapling

Tie Nginx OCSP stapling to enhancements system

certbot-nginx/certbot_nginx/configurator.py

@@ -94,7 +94,8 @@ def __init__(self, *args, **kwargs):
         # These will be set in the prepare function
         self.parser = None
         self.version = version
-        self._enhance_func = {"redirect": self._enable_redirect}
+        self._enhance_func = {"redirect": self._enable_redirect,
+                              "staple-ocsp": self._enable_ocsp_stapling}
 
         # Set up reverter
         self.reverter = reverter.Reverter(self.config)
@@ -137,11 +138,6 @@ def deploy_cert(self, domain, cert_path, key_path,
         .. note:: Aborts if the vhost is missing ssl_certificate or
             ssl_certificate_key.
 
-        .. note:: Nginx doesn't have a cert chain directive.
-            It expects the cert file to have the concatenated chain.
-            However, we use the chain file as input to the
-            ssl_trusted_certificate directive, used for verify OCSP responses.
-
         .. note:: This doesn't save the config files!
 
         :raises errors.PluginError: When unable to deploy certificate due to
@@ -157,26 +153,9 @@ def deploy_cert(self, domain, cert_path, key_path,
         cert_directives = [['\n', 'ssl_certificate', ' ', fullchain_path],
                            ['\n', 'ssl_certificate_key', ' ', key_path]]
 
-        # OCSP stapling was introduced in Nginx 1.3.7. If we have that version
-        # or greater, add config settings for it.
-        stapling_directives = []
-        if self.version >= (1, 3, 7):
-            stapling_directives = [
-                ['\n    ', 'ssl_trusted_certificate', ' ', chain_path],
-                ['\n    ', 'ssl_stapling', ' ', 'on'],
-                ['\n    ', 'ssl_stapling_verify', ' ', 'on'], ['\n']]
-
-        if len(stapling_directives) != 0 and not chain_path:
-            raise errors.PluginError(
-                "--chain-path is required to enable "
-                "Online Certificate Status Protocol (OCSP) stapling "
-                "on nginx >= 1.3.7.")
-
         try:
             self.parser.add_server_directives(vhost.filep, vhost.names,
                                               cert_directives, replace=True)
-            self.parser.add_server_directives(vhost.filep, vhost.names,
-                                              stapling_directives, replace=False)
             logger.info("Deployed Certificate to VirtualHost %s for %s",
                         vhost.filep, vhost.names)
         except errors.MisconfigurationError as error:
@@ -192,12 +171,6 @@ def deploy_cert(self, domain, cert_path, key_path,
                              ", ".join(str(addr) for addr in vhost.addrs)))
         self.save_notes += "\tssl_certificate %s\n" % fullchain_path
         self.save_notes += "\tssl_certificate_key %s\n" % key_path
-        if len(stapling_directives) > 0:
-            self.save_notes += "\tssl_trusted_certificate %s\n" % chain_path
-            self.save_notes += "\tssl_stapling on\n"
-            self.save_notes += "\tssl_stapling_verify on\n"
-
-
 
     #######################
     # Vhost parsing methods
@@ -373,7 +346,7 @@ def get_all_certs_keys(self):
     ##################################
     def supported_enhancements(self):  # pylint: disable=no-self-use
         """Returns currently supported enhancements."""
-        return ['redirect']
+        return ['redirect', 'staple-ocsp']
 
     def enhance(self, domain, enhancement, options=None):
         """Enhance configuration.
@@ -394,6 +367,7 @@ def enhance(self, domain, enhancement, options=None):
                 "Unsupported enhancement: {0}".format(enhancement))
         except errors.PluginError:
             logger.warning("Failed %s for %s", enhancement, domain)
+            raise
 
     def _enable_redirect(self, vhost, unused_options):
         """Redirect all equivalent HTTP traffic to ssl_vhost.
@@ -417,6 +391,45 @@ def _enable_redirect(self, vhost, unused_options):
             vhost.filep, vhost.names, redirect_block, replace=False)
         logger.info("Redirecting all traffic to ssl in %s", vhost.filep)
 
+    def _enable_ocsp_stapling(self, vhost, chain_path):
+        """Include OCSP response in TLS handshake
+
+        :param vhost: Destination of traffic, an ssl enabled vhost
+        :type vhost: :class:`~certbot_nginx.obj.VirtualHost`
+
+        :param chain_path: chain file path
+        :type chain_path: `str` or `None`
+
+        """
+        if self.version < (1, 3, 7):
+            raise errors.PluginError("Version 1.3.7 or greater of nginx "
+                                     "is needed to enable OCSP stapling")
+
+        if chain_path is None:
+            raise errors.PluginError(
+                "--chain-path is required to enable "
+                "Online Certificate Status Protocol (OCSP) stapling "
+                "on nginx >= 1.3.7.")
+
+        stapling_directives = [
+            ['\n    ', 'ssl_trusted_certificate', ' ', chain_path],
+            ['\n    ', 'ssl_stapling', ' ', 'on'],
+            ['\n    ', 'ssl_stapling_verify', ' ', 'on'], ['\n']]
+
+        try:
+            self.parser.add_server_directives(vhost.filep, vhost.names,
+                                              stapling_directives, replace=False)
+        except errors.MisconfigurationError as error:
+            logger.debug(error)
+            raise errors.PluginError("An error occurred while enabling OCSP "
+                                     "stapling for {0}.".format(vhost.names))
+
+        self.save_notes += ("OCSP Stapling was enabled "
+                            "on SSL Vhost: {0}.\n".format(vhost.filep))
+        self.save_notes += "\tssl_trusted_certificate {0}\n".format(chain_path)
+        self.save_notes += "\tssl_stapling on\n"
+        self.save_notes += "\tssl_stapling_verify on\n"
+
     ######################################
     # Nginx server management (IInstaller)
     ######################################

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -72,7 +72,8 @@ def test_get_all_names(self, mock_gethostbyaddr):
              "example.*", "www.example.org", "myhost"]))
 
     def test_supported_enhancements(self):
-        self.assertEqual(['redirect'], self.config.supported_enhancements())
+        self.assertEqual(['redirect', 'staple-ocsp'],
+                         self.config.supported_enhancements())
 
     def test_enhance(self):
         self.assertRaises(
@@ -141,37 +142,6 @@ def test_choose_vhost(self):
     def test_more_info(self):
         self.assertTrue('nginx.conf' in self.config.more_info())
 
-    def test_deploy_cert_stapling(self):
-        # Choose a version of Nginx greater than 1.3.7 so stapling code gets
-        # invoked.
-        self.config.version = (1, 9, 6)
-        example_conf = self.config.parser.abs_path('sites-enabled/example.com')
-        self.config.deploy_cert(
-            "www.example.com",
-            "example/cert.pem",
-            "example/key.pem",
-            "example/chain.pem",
-            "example/fullchain.pem")
-        self.config.save()
-        self.config.parser.load()
-        generated_conf = self.config.parser.parsed[example_conf]
-
-        self.assertTrue(util.contains_at_depth(generated_conf,
-                                               ['ssl_stapling', 'on'], 2))
-        self.assertTrue(util.contains_at_depth(generated_conf,
-                                               ['ssl_stapling_verify', 'on'], 2))
-        self.assertTrue(util.contains_at_depth(generated_conf,
-                                               ['ssl_trusted_certificate', 'example/chain.pem'], 2))
-
-    def test_deploy_cert_stapling_requires_chain_path(self):
-        self.config.version = (1, 3, 7)
-        self.assertRaises(errors.PluginError, self.config.deploy_cert,
-            "www.example.com",
-            "example/cert.pem",
-            "example/key.pem",
-            None,
-            "example/fullchain.pem")
-
     def test_deploy_cert_requires_fullchain_path(self):
         self.config.version = (1, 3, 1)
         self.assertRaises(errors.PluginError, self.config.deploy_cert,
@@ -185,8 +155,6 @@ def test_deploy_cert(self):
         server_conf = self.config.parser.abs_path('server.conf')
         nginx_conf = self.config.parser.abs_path('nginx.conf')
         example_conf = self.config.parser.abs_path('sites-enabled/example.com')
-        # Choose a version of Nginx less than 1.3.7 so stapling code doesn't get
-        # invoked.
         self.config.version = (1, 3, 1)
 
         # Get the default SSL vhost
@@ -429,5 +397,36 @@ def test_redirect_enhance(self):
         generated_conf = self.config.parser.parsed[example_conf]
         self.assertTrue(util.contains_at_depth(generated_conf, expected, 2))
 
+    def test_staple_ocsp_bad_version(self):
+        self.config.version = (1, 3, 1)
+        self.assertRaises(errors.PluginError, self.config.enhance,
+                          "www.example.com", "staple-ocsp", "chain_path")
+
+    def test_staple_ocsp_no_chain_path(self):
+        self.assertRaises(errors.PluginError, self.config.enhance,
+                          "www.example.com", "staple-ocsp", None)
+
+    def test_staple_ocsp_internal_error(self):
+        self.config.enhance("www.example.com", "staple-ocsp", "chain_path")
+        # error is raised because the server block has conflicting directives
+        self.assertRaises(errors.PluginError, self.config.enhance,
+                          "www.example.com", "staple-ocsp", "different_path")
+
+    def test_staple_ocsp(self):
+        chain_path = "example/chain.pem"
+        self.config.enhance("www.example.com", "staple-ocsp", chain_path)
+
+        example_conf = self.config.parser.abs_path('sites-enabled/example.com')
+        generated_conf = self.config.parser.parsed[example_conf]
+
+        self.assertTrue(util.contains_at_depth(
+            generated_conf,
+            ['ssl_trusted_certificate', 'example/chain.pem'], 2))
+        self.assertTrue(util.contains_at_depth(
+            generated_conf, ['ssl_stapling', 'on'], 2))
+        self.assertTrue(util.contains_at_depth(
+            generated_conf, ['ssl_stapling_verify', 'on'], 2))
+
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

certbot/client.py

@@ -382,7 +382,8 @@ def deploy_certificate(self, domains, privkey_path,
         with error_handler.ErrorHandler(self._rollback_and_restart, msg):
             # sites may have been enabled / final cleanup
             self.installer.restart()
-    def enhance_config(self, domains, config):
+
+    def enhance_config(self, domains, config, chain_path):
         """Enhance the configuration.
 
         :param list domains: list of domains to configure
@@ -392,6 +393,9 @@ def enhance_config(self, domains, config):
             it must have the redirect, hsts and uir attributes.
         :type namespace: :class:`argparse.Namespace`
 
+        :param chain_path: chain file path
+        :type chain_path: `str` or `None`
+
         :raises .errors.Error: if no installer is specified in the
             client.
 
@@ -425,7 +429,7 @@ def enhance_config(self, domains, config):
             self.apply_enhancement(domains, "ensure-http-header",
                     "Upgrade-Insecure-Requests")
         if staple:
-            self.apply_enhancement(domains, "staple-ocsp")
+            self.apply_enhancement(domains, "staple-ocsp", chain_path)
 
         msg = ("We were unable to restart web server")
         if redirect or hsts or uir or staple:

certbot/constants.py

@@ -54,7 +54,7 @@
 List of expected options parameters:
 - redirect: None
 - http-header: TODO
-- ocsp-stapling: TODO
+- ocsp-stapling: certificate chain file path
 - spdy: TODO
 
 """

certbot/main.py

@@ -436,7 +436,7 @@ def install(config, plugins):
     le_client.deploy_certificate(
         domains, config.key_path, config.cert_path, config.chain_path,
         config.fullchain_path)
-    le_client.enhance_config(domains, config)
+    le_client.enhance_config(domains, config, config.chain_path)
 
 
 def plugins_cmd(config, plugins):  # TODO: Use IDisplay rather than print
@@ -516,7 +516,7 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
         domains, lineage.privkey, lineage.cert,
         lineage.chain, lineage.fullchain)
 
-    le_client.enhance_config(domains, config)
+    le_client.enhance_config(domains, config, lineage.chain)
 
     if len(lineage.available_versions("cert")) == 1:
         display_ops.success_installation(domains)