From 3a8a5598a3dca2f44f59443a437aa0d6897921d2 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 19 Sep 2016 14:44:34 -0700 Subject: [PATCH 1/5] update constants.py --- certbot/constants.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/constants.py b/certbot/constants.py index 1ddb9fedf..ae998e15a 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -54,7 +54,7 @@ enhancements. List of expected options parameters: - redirect: None - http-header: TODO -- ocsp-stapling: TODO +- ocsp-stapling: certificate chain file path - spdy: TODO """ From fea079400fb643ca7b7bd94088bd5a1da925cc8b Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 19 Sep 2016 14:55:26 -0700 Subject: [PATCH 2/5] Provide chain_path for enhance config --- certbot/client.py | 8 ++++++-- certbot/main.py | 4 ++-- certbot/tests/client_test.py | 38 ++++++++++++++++++------------------ 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index f92370d30..a4f2248a6 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -382,7 +382,8 @@ class Client(object): 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 @@ class Client(object): 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 @@ class Client(object): 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: diff --git a/certbot/main.py b/certbot/main.py index 511046df0..6c2455a18 100644 --- a/certbot/main.py +++ b/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) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index e5c6b3af9..dd4211367 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -317,15 +317,15 @@ class ClientTest(unittest.TestCase): @mock.patch("certbot.client.enhancements") def test_enhance_config(self, mock_enhancements): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.Error, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.Error, self.client.enhance_config, + ["foo.bar"], config, None) mock_enhancements.ask.return_value = True installer = mock.MagicMock() self.client.installer = installer installer.supported_enhancements.return_value = ["redirect"] - self.client.enhance_config(["foo.bar"], config) + self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_once_with("foo.bar", "redirect", None) self.assertEqual(installer.save.call_count, 1) installer.restart.assert_called_once_with() @@ -333,8 +333,8 @@ class ClientTest(unittest.TestCase): @mock.patch("certbot.client.enhancements") def test_enhance_config_no_ask(self, mock_enhancements): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.Error, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.Error, self.client.enhance_config, + ["foo.bar"], config, None) mock_enhancements.ask.return_value = True installer = mock.MagicMock() @@ -342,16 +342,16 @@ class ClientTest(unittest.TestCase): installer.supported_enhancements.return_value = ["redirect", "ensure-http-header"] config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.client.enhance_config(["foo.bar"], config) + self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_with("foo.bar", "redirect", None) config = ConfigHelper(redirect=False, hsts=True, uir=False) - self.client.enhance_config(["foo.bar"], config) + self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_with("foo.bar", "ensure-http-header", "Strict-Transport-Security") config = ConfigHelper(redirect=False, hsts=False, uir=True) - self.client.enhance_config(["foo.bar"], config) + self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_with("foo.bar", "ensure-http-header", "Upgrade-Insecure-Requests") @@ -365,14 +365,14 @@ class ClientTest(unittest.TestCase): installer.supported_enhancements.return_value = [] config = ConfigHelper(redirect=None, hsts=True, uir=True) - self.client.enhance_config(["foo.bar"], config) + self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_not_called() mock_enhancements.ask.assert_not_called() def test_enhance_config_no_installer(self): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.Error, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.Error, self.client.enhance_config, + ["foo.bar"], config, None) @mock.patch("certbot.client.zope.component.getUtility") @mock.patch("certbot.client.enhancements") @@ -386,8 +386,8 @@ class ClientTest(unittest.TestCase): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.PluginError, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.PluginError, self.client.enhance_config, + ["foo.bar"], config, None) installer.recovery_routine.assert_called_once_with() self.assertEqual(mock_get_utility().add_message.call_count, 1) @@ -403,8 +403,8 @@ class ClientTest(unittest.TestCase): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.PluginError, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.PluginError, self.client.enhance_config, + ["foo.bar"], config, None) installer.recovery_routine.assert_called_once_with() self.assertEqual(mock_get_utility().add_message.call_count, 1) @@ -420,8 +420,8 @@ class ClientTest(unittest.TestCase): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.PluginError, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.PluginError, self.client.enhance_config, + ["foo.bar"], config, None) self.assertEqual(mock_get_utility().add_message.call_count, 1) installer.rollback_checkpoints.assert_called_once_with() @@ -440,8 +440,8 @@ class ClientTest(unittest.TestCase): config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.PluginError, - self.client.enhance_config, ["foo.bar"], config) + self.assertRaises(errors.PluginError, self.client.enhance_config, + ["foo.bar"], config, None) self.assertEqual(mock_get_utility().add_message.call_count, 1) installer.rollback_checkpoints.assert_called_once_with() self.assertEqual(installer.restart.call_count, 1) From dff1368765ae0d4a64681c289e72e8fa162c5d95 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 19 Sep 2016 15:05:16 -0700 Subject: [PATCH 3/5] add staple-ocsp test --- certbot/tests/client_test.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index dd4211367..5e398c2cd 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -332,31 +332,41 @@ class ClientTest(unittest.TestCase): @mock.patch("certbot.client.enhancements") def test_enhance_config_no_ask(self, mock_enhancements): - config = ConfigHelper(redirect=True, hsts=False, uir=False) + config = ConfigHelper(redirect=True, hsts=False, + uir=False, staple=False) self.assertRaises(errors.Error, self.client.enhance_config, ["foo.bar"], config, None) mock_enhancements.ask.return_value = True installer = mock.MagicMock() self.client.installer = installer - installer.supported_enhancements.return_value = ["redirect", "ensure-http-header"] + installer.supported_enhancements.return_value = [ + "redirect", "ensure-http-header", "staple-ocsp"] - config = ConfigHelper(redirect=True, hsts=False, uir=False) + config = ConfigHelper(redirect=True, hsts=False, + uir=False, staple=False) self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_with("foo.bar", "redirect", None) - config = ConfigHelper(redirect=False, hsts=True, uir=False) + config = ConfigHelper(redirect=False, hsts=True, + uir=False, staple=False) self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_with("foo.bar", "ensure-http-header", "Strict-Transport-Security") - config = ConfigHelper(redirect=False, hsts=False, uir=True) + config = ConfigHelper(redirect=False, hsts=False, + uir=True, staple=False) self.client.enhance_config(["foo.bar"], config, None) installer.enhance.assert_called_with("foo.bar", "ensure-http-header", "Upgrade-Insecure-Requests") - self.assertEqual(installer.save.call_count, 3) - self.assertEqual(installer.restart.call_count, 3) + config = ConfigHelper(redirect=False, hsts=False, + uir=False, staple=True) + self.client.enhance_config(["foo.bar"], config, None) + installer.enhance.assert_called_with("foo.bar", "staple-ocsp", None) + + self.assertEqual(installer.save.call_count, 4) + self.assertEqual(installer.restart.call_count, 4) @mock.patch("certbot.client.enhancements") def test_enhance_config_unsupported(self, mock_enhancements): From 8b553fa88f6cb03722695c4be296466ba8b59178 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 21 Sep 2016 15:38:37 -0700 Subject: [PATCH 4/5] tie oscp stapling to enhancements system --- certbot-nginx/certbot_nginx/configurator.py | 71 +++++++++++-------- .../certbot_nginx/tests/configurator_test.py | 64 ++++++++--------- 2 files changed, 73 insertions(+), 62 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 444e48903..cc37e5bfe 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -94,7 +94,8 @@ class NginxConfigurator(common.Plugin): # 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 @@ class NginxConfigurator(common.Plugin): .. 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 @@ class NginxConfigurator(common.Plugin): 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 @@ class NginxConfigurator(common.Plugin): ", ".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 @@ -394,6 +367,7 @@ class NginxConfigurator(common.Plugin): "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 @@ class NginxConfigurator(common.Plugin): 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) ###################################### diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 9e0c0dda5..43226213b 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -141,37 +141,6 @@ class NginxConfiguratorTest(util.NginxTest): 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 +154,6 @@ class NginxConfiguratorTest(util.NginxTest): 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 +396,36 @@ class NginxConfiguratorTest(util.NginxTest): 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 From 93a9e8c836c4c731856e30475358ec7a6b6a56e6 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 21 Sep 2016 15:48:24 -0700 Subject: [PATCH 5/5] list 'staple-ocsp' as supported in nginx --- certbot-nginx/certbot_nginx/configurator.py | 2 +- certbot-nginx/certbot_nginx/tests/configurator_test.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index cc37e5bfe..2b5a10047 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -346,7 +346,7 @@ class NginxConfigurator(common.Plugin): ################################## 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. diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 43226213b..37e120612 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -72,7 +72,8 @@ class NginxConfiguratorTest(util.NginxTest): "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(