From 0e6d652ce37514b721fba5ce929caeb792930b85 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 13 Oct 2015 18:25:31 -0700 Subject: [PATCH] Move ConfigObj parsing inside of RenewableCert.__init__ --- letsencrypt/cli.py | 17 ++++------- letsencrypt/renewer.py | 30 ++++++------------- letsencrypt/storage.py | 48 ++++++++++++++++--------------- letsencrypt/tests/cli_test.py | 17 ++++++----- letsencrypt/tests/renewer_test.py | 40 +++++++++----------------- 5 files changed, 62 insertions(+), 90 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 07ccd38fd..35113be3a 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -12,7 +12,6 @@ import time import traceback import configargparse -import configobj import OpenSSL import zope.component import zope.interface.exceptions @@ -167,25 +166,21 @@ def _init_le_client(args, config, authenticator, installer): return client.Client(config, acc, authenticator, installer, acme=acme) -def _find_duplicative_certs(domains, config, renew_config): +def _find_duplicative_certs(config, domains): """Find existing certs that duplicate the request.""" identical_names_cert, subset_names_cert = None, None - configs_dir = renew_config.renewal_configs_dir + cli_config = configuration.RenewerConfiguration(config) + configs_dir = cli_config.renewal_configs_dir # Verify the directory is there le_util.make_or_verify_dir(configs_dir, mode=0o755, uid=os.geteuid()) - cli_config = configuration.RenewerConfiguration(config) for renewal_file in os.listdir(configs_dir): try: full_path = os.path.join(configs_dir, renewal_file) - rc_config = configobj.ConfigObj(renew_config.renewer_config_file) - rc_config.merge(configobj.ConfigObj(full_path)) - rc_config.filename = full_path - candidate_lineage = storage.RenewableCert( - rc_config, config_opts=None, cli_config=cli_config) - except (configobj.ConfigObjError, errors.CertStorageError, IOError): + candidate_lineage = storage.RenewableCert(full_path, cli_config) + except (errors.CertStorageError, IOError): logger.warning("Renewal configuration file %s is broken. " "Skipping.", full_path) continue @@ -217,7 +212,7 @@ def _treat_as_renewal(config, domains): # kind of certificate to be obtained with renewal = False.) if not config.duplicate: ident_names_cert, subset_names_cert = _find_duplicative_certs( - domains, config, configuration.RenewerConfiguration(config)) + config, domains) # I am not sure whether that correctly reads the systemwide # configuration file. question = None diff --git a/letsencrypt/renewer.py b/letsencrypt/renewer.py index 8d8540e6f..f91d60833 100644 --- a/letsencrypt/renewer.py +++ b/letsencrypt/renewer.py @@ -12,7 +12,6 @@ import logging import os import sys -import configobj import OpenSSL import zope.component @@ -141,7 +140,7 @@ def _create_parser(): return _paths_parser(parser) -def main(config=None, cli_args=sys.argv[1:]): +def main(cli_args=sys.argv[1:]): """Main function for autorenewer script.""" # TODO: Distinguish automated invocation from manual invocation, # perhaps by looking at sys.argv[0] and inhibiting automated @@ -149,6 +148,11 @@ def main(config=None, cli_args=sys.argv[1:]): # turned it off. (The boolean parameter should probably be # called renewer_enabled.) + # TODO: When we have a more elaborate renewer command line, we will + # presumably also be able to specify a config file on the + # command line, which, if provided, should take precedence over + # te default config files + zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) args = _create_parser().parse_args(cli_args) @@ -159,28 +163,12 @@ def main(config=None, cli_args=sys.argv[1:]): cli_config = configuration.RenewerConfiguration(args) - config = storage.config_with_defaults(config) - # Now attempt to read the renewer config file and augment or replace - # the renewer defaults with any options contained in that file. If - # renewer_config_file is undefined or if the file is nonexistent or - # empty, this .merge() will have no effect. TODO: when we have a more - # elaborate renewer command line, we will presumably also be able to - # specify a config file on the command line, which, if provided, should - # take precedence over this one. - config.merge(configobj.ConfigObj(cli_config.renewer_config_file)) # Ensure that all of the needed folders have been created before continuing le_util.make_or_verify_dir(cli_config.work_dir, constants.CONFIG_DIRS_MODE, uid) - for i in os.listdir(cli_config.renewal_configs_dir): - print "Processing", i - if not i.endswith(".conf"): - continue - rc_config = configobj.ConfigObj(cli_config.renewer_config_file) - rc_config.merge(configobj.ConfigObj( - os.path.join(cli_config.renewal_configs_dir, i))) - # TODO: this is a dirty hack! - rc_config.filename = os.path.join(cli_config.renewal_configs_dir, i) + for renewal_file in os.listdir(cli_config.renewal_configs_dir): + print "Processing", renewal_file try: # TODO: Before trying to initialize the RenewableCert object, # we could check here whether the combination of the config @@ -190,7 +178,7 @@ def main(config=None, cli_args=sys.argv[1:]): # RenewableCert object for this cert at all, which could # dramatically improve performance for large deployments # where autorenewal is widely turned off. - cert = storage.RenewableCert(rc_config, cli_config=cli_config) + cert = storage.RenewableCert(renewal_file, cli_config) except errors.CertStorageError: # This indicates an invalid renewal configuration file, such # as one missing a required parameter (in the future, perhaps diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py index 207099e74..9b3290e2c 100644 --- a/letsencrypt/storage.py +++ b/letsencrypt/storage.py @@ -81,49 +81,49 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes renewal configuration file and/or systemwide defaults. """ - def __init__(self, configfile, config_opts=None, cli_config=None): + def __init__(self, config_filename, cli_config): """Instantiate a RenewableCert object from an existing lineage. - :param configobj.ConfigObj configfile: an already-parsed - ConfigObj object made from reading the renewal config file + :param str config_filename: the path to the renewal config file that defines this lineage. - - :param configobj.ConfigObj config_opts: systemwide defaults for - renewal properties not otherwise specified in the individual - renewal config file. - :param .RenewerConfiguration cli_config: + :param .RenewerConfiguration: parsed command line arguments :raises .CertStorageError: if the configuration file's name didn't end in ".conf", or the file is missing or broken. - :raises TypeError: if the provided renewal configuration isn't a - ConfigObj object. """ self.cli_config = cli_config - if isinstance(configfile, configobj.ConfigObj): - if not os.path.basename(configfile.filename).endswith(".conf"): - raise errors.CertStorageError( - "renewal config file name must end in .conf") - self.lineagename = os.path.basename( - configfile.filename)[:-len(".conf")] - else: - raise TypeError("RenewableCert config must be ConfigObj object") + if not config_filename.endswith(".conf"): + raise errors.CertStorageError( + "renewal config file name must end in .conf") + self.lineagename = os.path.basename( + config_filename[:-len(".conf")]) # self.configuration should be used to read parameters that # may have been chosen based on default values from the # systemwide renewal configuration; self.configfile should be # used to make and save changes. - self.configfile = configfile + try: + self.configfile = configobj.ConfigObj(config_filename) + except configobj.ConfigObjError: + raise errors.CertStorageError( + "error parsing {0}".format(config_filename)) + # TODO: Do we actually use anything from defaults and do we want to # read further defaults from the systemwide renewal configuration # file at this stage? - self.configuration = config_with_defaults(config_opts) - self.configuration.merge(self.configfile) + try: + self.configuration = config_with_defaults( + configobj.ConfigObj(cli_config.renewer_config_file)) + self.configuration.merge(self.configfile) + except: + raise errors.CertStorageError( + "error parsing {0}".format(cli_config.renewer_config_file)) if not all(x in self.configuration for x in ALL_FOUR): raise errors.CertStorageError( "renewal config file {0} is missing a required " - "file reference".format(configfile)) + "file reference".format(self.configfile)) self.cert = self.configuration["cert"] self.privkey = self.configuration["privkey"] @@ -622,6 +622,8 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes :param configobj.ConfigObj config: renewal configuration defaults, affecting, for example, the locations of the directories where the associated files will be saved + :param .RenewerConfiguration cli_config: parsed command line + arguments :returns: the newly-created RenewalCert object :rtype: :class:`storage.renewableCert`""" @@ -691,7 +693,7 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes # TODO: add human-readable comments explaining other available # parameters new_config.write() - return cls(new_config, config, cli_config) + return cls(new_config.filename, cli_config) def save_successor(self, prior_version, new_cert, new_privkey, new_chain): """Save new cert and chain as a successor of a prior version. diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index d0fae370d..bc65d3599 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -302,26 +302,25 @@ class DuplicativeCertsTest(renewer_test.BaseRenewableCertTest): f.write(test_cert) # No overlap at all - result = _find_duplicative_certs(['wow.net', 'hooray.org'], - self.config, self.cli_config) + result = _find_duplicative_certs( + self.cli_config, ['wow.net', 'hooray.org']) self.assertEqual(result, (None, None)) # Totally identical - result = _find_duplicative_certs(['example.com', 'www.example.com'], - self.config, self.cli_config) + result = _find_duplicative_certs( + self.cli_config, ['example.com', 'www.example.com']) self.assertTrue(result[0].configfile.filename.endswith('example.org.conf')) self.assertEqual(result[1], None) # Superset - result = _find_duplicative_certs(['example.com', 'www.example.com', - 'something.new'], self.config, - self.cli_config) + result = _find_duplicative_certs( + self.cli_config, ['example.com', 'www.example.com', 'something.new']) self.assertEqual(result[0], None) self.assertTrue(result[1].configfile.filename.endswith('example.org.conf')) # Partial overlap doesn't count - result = _find_duplicative_certs(['example.com', 'something.new'], - self.config, self.cli_config) + result = _find_duplicative_certs( + self.cli_config, ['example.com', 'something.new']) self.assertEqual(result, (None, None)) diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py index e8ad168e7..652c54577 100644 --- a/letsencrypt/tests/renewer_test.py +++ b/letsencrypt/tests/renewer_test.py @@ -63,11 +63,11 @@ class BaseRenewableCertTest(unittest.TestCase): kind + ".pem") config.filename = os.path.join(self.tempdir, "renewal", "example.org.conf") + config.write() self.config = config self.defaults = configobj.ConfigObj() - self.test_rc = storage.RenewableCert( - self.config, self.defaults, self.cli_config) + self.test_rc = storage.RenewableCert(config.filename, self.cli_config) def tearDown(self): shutil.rmtree(self.tempdir) @@ -99,34 +99,26 @@ class RenewableCertTests(BaseRenewableCertTest): def test_renewal_bad_config(self): """Test that the RenewableCert constructor will complain if - the renewal configuration file doesn't end in ".conf" or if it - isn't a ConfigObj.""" + the renewal configuration file doesn't end in ".conf" + + """ from letsencrypt import storage - defaults = configobj.ConfigObj() - config = configobj.ConfigObj() - # These files don't exist and aren't created here; the point of the test - # is to confirm that the constructor rejects them outright because of - # the configfile's name. - for kind in ALL_FOUR: - config["cert"] = "nonexistent_" + kind + ".pem" - config.filename = "nonexistent_sillyfile" - self.assertRaises( - errors.CertStorageError, storage.RenewableCert, config, defaults) - self.assertRaises(TypeError, storage.RenewableCert, "fun", defaults) + self.assertRaises(errors.CertStorageError, storage.RenewableCert, + "fun", self.cli_config) def test_renewal_incomplete_config(self): """Test that the RenewableCert constructor will complain if the renewal configuration file is missing a required file element.""" from letsencrypt import storage - defaults = configobj.ConfigObj() config = configobj.ConfigObj() config["cert"] = "imaginary_cert.pem" # Here the required privkey is missing. config["chain"] = "imaginary_chain.pem" config["fullchain"] = "imaginary_fullchain.pem" - config.filename = "imaginary_config.conf" - self.assertRaises( - errors.CertStorageError, storage.RenewableCert, config, defaults) + config.filename = os.path.join(self.tempdir, "imaginary_config.conf") + config.write() + self.assertRaises(errors.CertStorageError, storage.RenewableCert, + config.filename, self.cli_config) def test_consistent(self): # pylint: disable=too-many-statements,protected-access @@ -719,10 +711,6 @@ class RenewableCertTests(BaseRenewableCertTest): mock_rc_instance.should_autorenew.return_value = True mock_rc_instance.latest_common_version.return_value = 10 mock_rc.return_value = mock_rc_instance - with open(os.path.join(self.cli_config.renewal_configs_dir, - "README"), "w") as f: - f.write("This is a README file to make sure that the renewer is") - f.write("able to correctly ignore files that don't end in .conf.") with open(os.path.join(self.cli_config.renewal_configs_dir, "example.org.conf"), "w") as f: # This isn't actually parsed in this test; we have a separate @@ -734,7 +722,7 @@ class RenewableCertTests(BaseRenewableCertTest): "example.com.conf"), "w") as f: f.write("cert = cert.pem\nprivkey = privkey.pem\n") f.write("chain = chain.pem\nfullchain = fullchain.pem\n") - renewer.main(self.defaults, cli_args=self._common_cli_args()) + renewer.main(cli_args=self._common_cli_args()) self.assertEqual(mock_rc.call_count, 2) self.assertEqual(mock_rc_instance.update_all_links_to.call_count, 2) self.assertEqual(mock_notify.notify.call_count, 4) @@ -747,7 +735,7 @@ class RenewableCertTests(BaseRenewableCertTest): mock_happy_instance.should_autorenew.return_value = False mock_happy_instance.latest_common_version.return_value = 10 mock_rc.return_value = mock_happy_instance - renewer.main(self.defaults, cli_args=self._common_cli_args()) + renewer.main(cli_args=self._common_cli_args()) self.assertEqual(mock_rc.call_count, 4) self.assertEqual(mock_happy_instance.update_all_links_to.call_count, 0) self.assertEqual(mock_notify.notify.call_count, 4) @@ -758,7 +746,7 @@ class RenewableCertTests(BaseRenewableCertTest): with open(os.path.join(self.cli_config.renewal_configs_dir, "bad.conf"), "w") as f: f.write("incomplete = configfile\n") - renewer.main(self.defaults, cli_args=self._common_cli_args()) + renewer.main(cli_args=self._common_cli_args()) # The errors.CertStorageError is caught inside and nothing happens.