From 065e923bc9b75f1a7e59164da0541e5f0540e1b4 Mon Sep 17 00:00:00 2001 From: Spencer Eick Date: Wed, 14 Mar 2018 15:59:13 -0400 Subject: [PATCH] Improve "cannot find cert of key directive" error (#5525) (#5679) - Fix code to log separate error messages when either SSLCertificateFile or SSLCertificateKeyFile - directives are not found. - Update the section in install.rst where the relevant error is referenced. - Edit a docstring where 'cert' previously referred to certificate. - Edit test_deploy_cert_invalid_vhost in the test suite to cover changes. Fixes #5525. --- certbot-apache/certbot_apache/configurator.py | 22 ++++++++------ .../certbot_apache/tests/configurator_test.py | 30 +++++++++++++++++-- docs/install.rst | 10 +++---- 3 files changed, 45 insertions(+), 17 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 6377bb114..8b996c675 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -285,8 +285,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): chain_path=None, fullchain_path=None): """Deploys certificate to specified virtual host. - Currently tries to find the last directives to deploy the cert in - the VHost associated with the given domain. If it can't find the + Currently tries to find the last directives to deploy the certificate + in the VHost associated with the given domain. If it can't find the directives, it searches the "included" confs. The function verifies that it has located the three directives and finally modifies them to point to the correct destination. After the certificate is @@ -424,14 +424,20 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): path["chain_path"] = self.parser.find_dir( "SSLCertificateChainFile", None, vhost.path) - if not path["cert_path"] or not path["cert_key"]: - # Throw some can't find all of the directives error" + # Handle errors when certificate/key directives cannot be found + if not path["cert_path"]: logger.warning( - "Cannot find a cert or key directive in %s. " + "Cannot find an SSLCertificateFile directive in %s. " "VirtualHost was not modified", vhost.path) - # Presumably break here so that the virtualhost is not modified raise errors.PluginError( - "Unable to find cert and/or key directives") + "Unable to find an SSLCertificateFile directive") + elif not path["cert_key"]: + logger.warning( + "Cannot find an SSLCertificateKeyFile directive for " + "certificate in %s. VirtualHost was not modified", vhost.path) + raise errors.PluginError( + "Unable to find an SSLCertificateKeyFile directive for " + "certificate") logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) @@ -2117,5 +2123,3 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # to be modified. return common.install_version_controlled_file(options_ssl, options_ssl_digest, self.constant("MOD_SSL_CONF_SRC"), constants.ALL_SSL_OPTIONS_HASHES) - - diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index c9bf9a63f..7b1e4fa86 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -441,13 +441,37 @@ class MultipleVhostsTest(util.ApacheTest): self.vh_truth[1].path)) def test_deploy_cert_invalid_vhost(self): + """For test cases where the `ApacheConfigurator` class' `_deploy_cert` + method is called with an invalid vhost parameter. Currently this tests + that a PluginError is appropriately raised when important directives + are missing in an SSL module.""" self.config.parser.modules.add("ssl_module") - mock_find = mock.MagicMock() - mock_find.return_value = [] - self.config.parser.find_dir = mock_find + self.config.parser.modules.add("mod_ssl.c") + self.config.parser.modules.add("socache_shmcb_module") + + def side_effect(*args): + """Mocks case where an SSLCertificateFile directive can be found + but an SSLCertificateKeyFile directive is missing.""" + if "SSLCertificateFile" in args: + return ["example/cert.pem"] + else: + return [] + + mock_find_dir = mock.MagicMock(return_value=[]) + mock_find_dir.side_effect = side_effect + + self.config.parser.find_dir = mock_find_dir # Get the default 443 vhost self.config.assoc["random.demo"] = self.vh_truth[1] + + self.assertRaises( + errors.PluginError, self.config.deploy_cert, "random.demo", + "example/cert.pem", "example/key.pem", "example/cert_chain.pem") + + # Remove side_effect to mock case where both SSLCertificateFile + # and SSLCertificateKeyFile directives are missing + self.config.parser.find_dir.side_effect = None self.assertRaises( errors.PluginError, self.config.deploy_cert, "random.demo", "example/cert.pem", "example/key.pem", "example/cert_chain.pem") diff --git a/docs/install.rst b/docs/install.rst index 67889d8f7..45b0b7785 100644 --- a/docs/install.rst +++ b/docs/install.rst @@ -204,10 +204,11 @@ want to use the Apache plugin, it has to be installed separately: emerge -av app-crypt/certbot emerge -av app-crypt/certbot-apache -When using the Apache plugin, you will run into a "cannot find a cert or key -directive" error if you're sporting the default Gentoo ``httpd.conf``. -You can fix this by commenting out two lines in ``/etc/apache2/httpd.conf`` -as follows: +When using the Apache plugin, you will run into a "cannot find an +SSLCertificateFile directive" or "cannot find an SSLCertificateKeyFile +directive for certificate" error if you're sporting the default Gentoo +``httpd.conf``. You can fix this by commenting out two lines in +``/etc/apache2/httpd.conf`` as follows: Change @@ -257,4 +258,3 @@ whole process is described in the :doc:`contributing`. e.g. ``sudo python setup.py install``, ``sudo pip install``, ``sudo ./venv/bin/...``. These modes of operation might corrupt your operating system and are **not supported** by the Certbot team! -