From 211ca2420f454cea580f11d90c58c51818c5d274 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 17:19:26 -0800 Subject: [PATCH 1/8] Make read_file use abspath --- letsencrypt/cli.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 4c798ee5c..7411ada63 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -569,7 +569,7 @@ def plugins_cmd(args, config, plugins): # TODO: Use IDisplay rather than print def read_file(filename, mode="rb"): """Returns the given file's contents. - :param str filename: Filename + :param str filename: filename as an absolute path :param str mode: open mode (see `open`) :returns: A tuple of filename and its contents @@ -579,6 +579,7 @@ def read_file(filename, mode="rb"): """ try: + filename = os.path.abspath(filename) return filename, open(filename, mode).read() except IOError as exc: raise argparse.ArgumentTypeError(exc.strerror) From 707df3d2c61179fe89b99d03deb142e316befa92 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 17:27:56 -0800 Subject: [PATCH 2/8] Add read_file tests --- letsencrypt/tests/cli_test.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 509b7eb34..008d239d3 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -1,4 +1,5 @@ """Tests for letsencrypt.cli.""" +import argparse import itertools import os import shutil @@ -356,6 +357,20 @@ class CLITest(unittest.TestCase): mock_sys.exit.assert_called_with(''.join( traceback.format_exception_only(KeyboardInterrupt, interrupt))) + def test_read_file(self): + from letsencrypt import cli + rel_test_path = os.path.relpath(os.path.join(self.tmp_dir, 'foo')) + self.assertRaises( + argparse.ArgumentTypeError, cli.read_file, rel_test_path) + + test_contents = 'bar\n' + with open(rel_test_path, 'w') as f: + f.write(test_contents) + + path, contents = cli.read_file(rel_test_path) + self.assertEqual(path, os.path.abspath(path)) + self.assertEqual(contents, test_contents) + class DetermineAccountTest(unittest.TestCase): """Tests for letsencrypt.cli._determine_account.""" From dc78f811ce18ca1414e202c2e2a24b2a769e45ed Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 17:33:40 -0800 Subject: [PATCH 3/8] Make cert_path,key_path,chain_path,fullchain_path absolute --- letsencrypt/cli.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 7411ada63..b6181bb64 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -931,26 +931,28 @@ def _paths_parser(helpful): if verb in ("install", "revoke", "certonly"): section = verb if verb == "certonly": - add(section, "--cert-path", default=flag_default("auth_cert_path"), help=cph) + add(section, "--cert-path", type=os.path.abspath, + default=flag_default("auth_cert_path"), help=cph) elif verb == "revoke": add(section, "--cert-path", type=read_file, required=True, help=cph) else: - add(section, "--cert-path", help=cph, required=(verb == "install")) + add(section, "--cert-path", type=os.path.abspath, + help=cph, required=(verb == "install")) 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", type=((verb == "revoke" and read_file) or str), - required=(verb == "install"), + add(section, "--key-path", required=(verb == "install"), + type=((verb == "revoke" and read_file) or os.path.abspath), help="Path to private key for cert creation or revocation (if account key is missing)") default_cp = None if verb == "certonly": default_cp = flag_default("auth_chain_path") - add("paths", "--fullchain-path", default=default_cp, + add("paths", "--fullchain-path", default=default_cp, type=os.path.abspath, help="Accompanying path to a full certificate chain (cert plus chain).") - add("paths", "--chain-path", default=default_cp, + add("paths", "--chain-path", default=default_cp, type=os.path.abspath, help="Accompanying path to a certificate chain.") add("paths", "--config-dir", default=flag_default("config_dir"), help=config_help("config_dir")) From 1dd1523680cadc4d8b8fd59cbf31fdc9c37fcfd0 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 17:47:36 -0800 Subject: [PATCH 4/8] Added cert, key, chain, and fullchain abspath tests --- letsencrypt/tests/cli_test.py | 36 ++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 008d239d3..2f729f71d 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -23,7 +23,7 @@ from letsencrypt.tests import test_util CSR = test_util.vector_path('csr.der') -class CLITest(unittest.TestCase): +class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods """Tests for different commands.""" def setUp(self): @@ -116,6 +116,23 @@ class CLITest(unittest.TestCase): from letsencrypt import cli self.assertTrue(cli.usage_strings(plugins)[0] in out) + def test_install_abspath(self): + cert = 'cert' + key = 'key' + chain = 'chain' + fullchain = 'fullchain' + + with MockedVerb('install') as mock_install: + self._call(['install', '--cert-path', cert, '--key-path', 'key', + '--chain-path', 'chain', + '--fullchain-path', 'fullchain']) + + args = mock_install.call_args[0][0] + self.assertEqual(args.cert_path, os.path.abspath(cert)) + self.assertEqual(args.key_path, os.path.abspath(key)) + self.assertEqual(args.chain_path, os.path.abspath(chain)) + self.assertEqual(args.fullchain_path, os.path.abspath(fullchain)) + @mock.patch('letsencrypt.cli.display_ops') def test_installer_selection(self, mock_display_ops): self._call(['install', '--domain', 'foo.bar', '--cert-path', 'cert', @@ -211,6 +228,23 @@ class CLITest(unittest.TestCase): available = verified.available() stdout.write.called_once_with(str(available)) + def test_certonly_abspath(self): + cert = 'cert' + key = 'key' + chain = 'chain' + fullchain = 'fullchain' + + with MockedVerb('certonly') as mock_obtaincert: + self._call(['certonly', '--cert-path', cert, '--key-path', 'key', + '--chain-path', 'chain', + '--fullchain-path', 'fullchain']) + + args = mock_obtaincert.call_args[0][0] + self.assertEqual(args.cert_path, os.path.abspath(cert)) + self.assertEqual(args.key_path, os.path.abspath(key)) + self.assertEqual(args.chain_path, os.path.abspath(chain)) + self.assertEqual(args.fullchain_path, os.path.abspath(fullchain)) + def test_certonly_bad_args(self): ret, _, _, _ = self._call(['-d', 'foo.bar', 'certonly', '--csr', CSR]) self.assertEqual(ret, '--domain and --csr are mutually exclusive') From 778b8797bb3c215a3f0645d7cd11b375ad974054 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 18:17:10 -0800 Subject: [PATCH 5/8] Ensure config_dir, work_dir, and logs_dir have absolute paths --- letsencrypt/configuration.py | 5 +++++ letsencrypt/tests/renewer_test.py | 3 +++ 2 files changed, 8 insertions(+) diff --git a/letsencrypt/configuration.py b/letsencrypt/configuration.py index e70171675..4955655f3 100644 --- a/letsencrypt/configuration.py +++ b/letsencrypt/configuration.py @@ -37,6 +37,11 @@ class NamespaceConfig(object): def __init__(self, namespace): self.namespace = namespace + + self.namespace.config_dir = os.path.abspath(self.namespace.config_dir) + self.namespace.work_dir = os.path.abspath(self.namespace.work_dir) + self.namespace.logs_dir = os.path.abspath(self.namespace.logs_dir) + # Check command line parameters sanity, and error out in case of problem. check_config_sanity(self) diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py index e76b6eb88..daec9678f 100644 --- a/letsencrypt/tests/renewer_test.py +++ b/letsencrypt/tests/renewer_test.py @@ -692,6 +692,9 @@ class RenewableCertTests(BaseRenewableCertTest): self.test_rc.configfile["renewalparams"]["http01_port"] = "1234" self.test_rc.configfile["renewalparams"]["account"] = "abcde" self.test_rc.configfile["renewalparams"]["domains"] = ["example.com"] + self.test_rc.configfile["renewalparams"]["config_dir"] = "config" + self.test_rc.configfile["renewalparams"]["work_dir"] = "work" + self.test_rc.configfile["renewalparams"]["logs_dir"] = "logs" mock_auth = mock.MagicMock() mock_pd.PluginsRegistry.find_all.return_value = {"apache": mock_auth} # Fails because "fake" != "apache" From b408ec765dd6a0c217783c5f22d41f46efcf730c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 18:20:00 -0800 Subject: [PATCH 6/8] Test absolute paths are used --- letsencrypt/tests/configuration_test.py | 30 +++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/letsencrypt/tests/configuration_test.py b/letsencrypt/tests/configuration_test.py index 3a8bf40cf..be48a7c9c 100644 --- a/letsencrypt/tests/configuration_test.py +++ b/letsencrypt/tests/configuration_test.py @@ -59,6 +59,36 @@ class NamespaceConfigTest(unittest.TestCase): self.namespace.http01_port = None self.assertEqual(80, self.config.http01_port) + def test_absolute_paths(self): + from letsencrypt.configuration import NamespaceConfig + + config_base = "foo" + work_base = "bar" + logs_base = "baz" + + config = NamespaceConfig(mock.MagicMock( + config_dir=config_base, work_dir=work_base, logs_dir=logs_base)) + + self.assertTrue(os.path.isabs(config.config_dir)) + self.assertEqual(config.config_dir, + os.path.join(os.getcwd(), config_base)) + self.assertTrue(os.path.isabs(config.work_dir)) + self.assertEqual(config.work_dir, + os.path.join(os.getcwd(), work_base)) + self.assertTrue(os.path.isabs(config.logs_dir)) + self.assertEqual(config.logs_dir, + os.path.join(os.getcwd(), logs_base)) + self.assertTrue(os.path.isabs(config.cert_path)) + self.assertTrue(os.path.isabs(config.key_path)) + self.assertTrue(os.path.isabs(config.chain_path)) + self.assertTrue(os.path.isabs(config.fullchain_path)) + self.assertTrue(os.path.isabs(config.accounts_dir)) + self.assertTrue(os.path.isabs(config.backup_dir)) + self.assertTrue(os.path.isabs(config.csr_dir)) + self.assertTrue(os.path.isabs(config.in_progress_dir)) + self.assertTrue(os.path.isabs(config.key_dir)) + self.assertTrue(os.path.isabs(config.temp_checkpoint_dir)) + class RenewerConfigurationTest(unittest.TestCase): """Test for letsencrypt.configuration.RenewerConfiguration.""" From 82614df6f0bdd7bf20ea3554780e880f84e80db3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 18:30:37 -0800 Subject: [PATCH 7/8] What do YOU think os.path.isabs(mock.MagicMock()) returns? --- letsencrypt/tests/configuration_test.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/letsencrypt/tests/configuration_test.py b/letsencrypt/tests/configuration_test.py index be48a7c9c..16a4da6e1 100644 --- a/letsencrypt/tests/configuration_test.py +++ b/letsencrypt/tests/configuration_test.py @@ -66,8 +66,14 @@ class NamespaceConfigTest(unittest.TestCase): work_base = "bar" logs_base = "baz" - config = NamespaceConfig(mock.MagicMock( - config_dir=config_base, work_dir=work_base, logs_dir=logs_base)) + mock_namespace = mock.MagicMock(spec=['config_dir', 'work_dir', + 'logs_dir', 'http01_port', + 'tls_sni_01_port', + 'domains', 'server']) + mock_namespace.config_dir = config_base + mock_namespace.work_dir = work_base + mock_namespace.logs_dir = logs_base + config = NamespaceConfig(mock_namespace) self.assertTrue(os.path.isabs(config.config_dir)) self.assertEqual(config.config_dir, @@ -78,10 +84,6 @@ class NamespaceConfigTest(unittest.TestCase): self.assertTrue(os.path.isabs(config.logs_dir)) self.assertEqual(config.logs_dir, os.path.join(os.getcwd(), logs_base)) - self.assertTrue(os.path.isabs(config.cert_path)) - self.assertTrue(os.path.isabs(config.key_path)) - self.assertTrue(os.path.isabs(config.chain_path)) - self.assertTrue(os.path.isabs(config.fullchain_path)) self.assertTrue(os.path.isabs(config.accounts_dir)) self.assertTrue(os.path.isabs(config.backup_dir)) self.assertTrue(os.path.isabs(config.csr_dir)) From 31b984adc520eab2066b41360a9b6da58efe0c25 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 12 Nov 2015 18:34:46 -0800 Subject: [PATCH 8/8] Test RenwerConfiguration --- letsencrypt/tests/configuration_test.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/letsencrypt/tests/configuration_test.py b/letsencrypt/tests/configuration_test.py index 16a4da6e1..c42b99081 100644 --- a/letsencrypt/tests/configuration_test.py +++ b/letsencrypt/tests/configuration_test.py @@ -113,6 +113,28 @@ class RenewerConfigurationTest(unittest.TestCase): self.config.renewal_configs_dir, '/tmp/config/renewal_configs') self.assertEqual(self.config.renewer_config_file, '/tmp/config/r.conf') + def test_absolute_paths(self): + from letsencrypt.configuration import NamespaceConfig + from letsencrypt.configuration import RenewerConfiguration + + config_base = "foo" + work_base = "bar" + logs_base = "baz" + + mock_namespace = mock.MagicMock(spec=['config_dir', 'work_dir', + 'logs_dir', 'http01_port', + 'tls_sni_01_port', + 'domains', 'server']) + mock_namespace.config_dir = config_base + mock_namespace.work_dir = work_base + mock_namespace.logs_dir = logs_base + config = RenewerConfiguration(NamespaceConfig(mock_namespace)) + + self.assertTrue(os.path.isabs(config.archive_dir)) + self.assertTrue(os.path.isabs(config.live_dir)) + self.assertTrue(os.path.isabs(config.renewal_configs_dir)) + self.assertTrue(os.path.isabs(config.renewer_config_file)) + if __name__ == '__main__': unittest.main() # pragma: no cover