From fbace69b5e8fed5ee32cb029478d6b11aea84842 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 14 Feb 2018 19:28:36 +0200 Subject: [PATCH] Fix install verb (#5536) * Fix install verb * Fix error message, tests and remove global pylint change * Fix boulder integration test keypath * Also use chain_path from lineage if not defined on CLI --- certbot/cli.py | 5 +- certbot/main.py | 41 ++++++++++++++-- certbot/tests/main_test.py | 90 +++++++++++++++++++++++++++++++++--- tests/boulder-integration.sh | 2 +- 4 files changed, 125 insertions(+), 13 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index dec7474f9..09dd71d13 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1278,14 +1278,13 @@ def _paths_parser(helpful): elif verb == "revoke": add(section, "--cert-path", type=read_file, required=True, help=cph) else: - add(section, "--cert-path", type=os.path.abspath, - help=cph, required=(verb == "install")) + add(section, "--cert-path", type=os.path.abspath, help=cph) section = "paths" if verb in ("install", "revoke"): section = verb # revoke --key-path reads a file, install --key-path takes a string - add(section, "--key-path", required=(verb == "install"), + add(section, "--key-path", type=((verb == "revoke" and read_file) or os.path.abspath), help="Path to private key for certificate installation " "or revocation (if account key is missing)") diff --git a/certbot/main.py b/certbot/main.py index 32dd69256..13234e0d2 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -1,4 +1,5 @@ """Certbot main entry point.""" +# pylint: disable=too-many-lines from __future__ import print_function import functools import logging.handlers @@ -779,11 +780,45 @@ def install(config, plugins): except errors.PluginSelectionError as e: return str(e) - domains, _ = _find_domains_or_certname(config, installer) - le_client = _init_le_client(config, authenticator=None, installer=installer) - _install_cert(config, le_client, domains) + # If cert-path is defined, populate missing (ie. not overridden) values. + # Unfortunately this can't be done in argument parser, as certificate + # manager needs the access to renewal directory paths + if config.certname: + config = _populate_from_certname(config) + if config.key_path and config.cert_path: + _check_certificate_and_key(config) + domains, _ = _find_domains_or_certname(config, installer) + le_client = _init_le_client(config, authenticator=None, installer=installer) + _install_cert(config, le_client, domains) + else: + raise errors.ConfigurationError("Path to certificate or key was not defined. " + "If your certificate is managed by Certbot, please use --cert-name " + "to define which certificate you would like to install.") +def _populate_from_certname(config): + """Helper function for install to populate missing config values from lineage + defined by --cert-name.""" + lineage = cert_manager.lineage_for_certname(config, config.certname) + if not lineage: + return config + if not config.key_path: + config.namespace.key_path = lineage.key_path + if not config.cert_path: + config.namespace.cert_path = lineage.cert_path + if not config.chain_path: + config.namespace.chain_path = lineage.chain_path + if not config.fullchain_path: + config.namespace.fullchain_path = lineage.fullchain_path + return config + +def _check_certificate_and_key(config): + if not os.path.isfile(os.path.realpath(config.cert_path)): + raise errors.ConfigurationError("Error while reading certificate from path " + "{0}".format(config.cert_path)) + if not os.path.isfile(os.path.realpath(config.key_path)): + raise errors.ConfigurationError("Error while reading private key from path " + "{0}".format(config.key_path)) def plugins_cmd(config, plugins): """List server software plugins. diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index b1d58542f..7368e8513 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -592,11 +592,30 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met super(MainTest, self).tearDown() - def _call(self, args, stdout=None): - "Run the cli with output streams and actual client mocked out" - with mock.patch('certbot.main.client') as client: - ret, stdout, stderr = self._call_no_clientmock(args, stdout) - return ret, stdout, stderr, client + def _call(self, args, stdout=None, mockisfile=False): + """Run the cli with output streams, actual client and optionally + os.path.isfile() mocked out""" + + if mockisfile: + orig_open = os.path.isfile + def mock_isfile(fn, *args, **kwargs): + """Mock os.path.isfile()""" + if (fn.endswith("cert") or + fn.endswith("chain") or + fn.endswith("privkey")): + return True + else: + return orig_open(fn, *args, **kwargs) + + with mock.patch("os.path.isfile") as mock_if: + mock_if.side_effect = mock_isfile + with mock.patch('certbot.main.client') as client: + ret, stdout, stderr = self._call_no_clientmock(args, stdout) + return ret, stdout, stderr, client + else: + with mock.patch('certbot.main.client') as client: + ret, stdout, stderr = self._call_no_clientmock(args, stdout) + return ret, stdout, stderr, client def _call_no_clientmock(self, args, stdout=None): "Run the client with output streams mocked out" @@ -680,9 +699,68 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met @mock.patch('certbot.main.plug_sel.pick_installer') def test_installer_selection(self, mock_pick_installer, _rec): self._call(['install', '--domains', 'foo.bar', '--cert-path', 'cert', - '--key-path', 'key', '--chain-path', 'chain']) + '--key-path', 'privkey', '--chain-path', 'chain'], mockisfile=True) self.assertEqual(mock_pick_installer.call_count, 1) + @mock.patch('certbot.main._install_cert') + @mock.patch('certbot.main.plug_sel.record_chosen_plugins') + @mock.patch('certbot.main.plug_sel.pick_installer') + def test_installer_certname(self, _inst, _rec, mock_install): + mock_lineage = mock.MagicMock(cert_path="/tmp/cert", chain_path="/tmp/chain", + fullchain_path="/tmp/chain", + key_path="/tmp/privkey") + + with mock.patch("certbot.cert_manager.lineage_for_certname") as mock_getlin: + mock_getlin.return_value = mock_lineage + self._call(['install', '--cert-name', 'whatever'], mockisfile=True) + call_config = mock_install.call_args[0][0] + self.assertEqual(call_config.cert_path, "/tmp/cert") + self.assertEqual(call_config.fullchain_path, "/tmp/chain") + self.assertEqual(call_config.key_path, "/tmp/privkey") + + @mock.patch('certbot.main._install_cert') + @mock.patch('certbot.main.plug_sel.record_chosen_plugins') + @mock.patch('certbot.main.plug_sel.pick_installer') + def test_installer_param_override(self, _inst, _rec, mock_install): + mock_lineage = mock.MagicMock(cert_path="/tmp/cert", chain_path="/tmp/chain", + fullchain_path="/tmp/chain", + key_path="/tmp/privkey") + with mock.patch("certbot.cert_manager.lineage_for_certname") as mock_getlin: + mock_getlin.return_value = mock_lineage + self._call(['install', '--cert-name', 'whatever', + '--key-path', '/tmp/overriding_privkey'], mockisfile=True) + call_config = mock_install.call_args[0][0] + self.assertEqual(call_config.cert_path, "/tmp/cert") + self.assertEqual(call_config.fullchain_path, "/tmp/chain") + self.assertEqual(call_config.chain_path, "/tmp/chain") + self.assertEqual(call_config.key_path, "/tmp/overriding_privkey") + + mock_install.reset() + + self._call(['install', '--cert-name', 'whatever', + '--cert-path', '/tmp/overriding_cert'], mockisfile=True) + call_config = mock_install.call_args[0][0] + self.assertEqual(call_config.cert_path, "/tmp/overriding_cert") + self.assertEqual(call_config.fullchain_path, "/tmp/chain") + self.assertEqual(call_config.key_path, "/tmp/privkey") + + @mock.patch('certbot.main.plug_sel.record_chosen_plugins') + @mock.patch('certbot.main.plug_sel.pick_installer') + def test_installer_param_error(self, _inst, _rec): + self.assertRaises(errors.ConfigurationError, + self._call, + ['install', '--key-path', '/tmp/key_path']) + self.assertRaises(errors.ConfigurationError, + self._call, + ['install', '--cert-path', '/tmp/key_path']) + self.assertRaises(errors.ConfigurationError, + self._call, + ['install']) + self.assertRaises(errors.ConfigurationError, + self._call, + ['install', '--cert-name', 'notfound', + '--key-path', 'invalid']) + @mock.patch('certbot.main._report_new_cert') @mock.patch('certbot.util.exe_exists') def test_configurator_selection(self, mock_exe_exists, unused_report): diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index e1aad4336..24d224cb0 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -251,7 +251,7 @@ openssl x509 -in "${root}/csr/chain.pem" -text common --domains le3.wtf install \ --cert-path "${root}/csr/cert.pem" \ - --key-path "${root}/csr/key.pem" + --key-path "${root}/key.pem" CheckCertCount() { CERTCOUNT=`ls "${root}/conf/archive/$1/cert"* | wc -l`