From a38bb418567f2b0710ebd2f0bbc424ed3464a4f5 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 11 Sep 2015 13:49:26 -0700 Subject: [PATCH] PR cleanup --- letsencrypt/cli.py | 35 ++++++++++++++++------------------- letsencrypt/storage.py | 3 +-- letsencrypt/tests/cli_test.py | 11 +++++------ 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 7244ad29e..317e7a541 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1,9 +1,7 @@ """Let's Encrypt CLI.""" # TODO: Sanity check all input. Be sure to avoid shell code etc... - import argparse import atexit -import configobj import functools import logging import logging.handlers @@ -14,6 +12,7 @@ import time import traceback import configargparse +import configobj import OpenSSL import zope.component import zope.interface.exceptions @@ -173,22 +172,22 @@ def _find_duplicative_certs(domains, config, renew_config): identical_names_cert, subset_names_cert = None, None configs_dir = renew_config.renewal_configs_dir + cli_config = configuration.RenewerConfiguration(config) for renewal_file in os.listdir(configs_dir): 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 - cli_config = configuration.RenewerConfiguration(config) - candidate_lineage = storage.RenewableCert(rc_config, None, cli_config) + candidate_lineage = storage.RenewableCert(rc_config, config_opts=None, + cli_config=cli_config) # TODO: Handle these differently depending on whether they are # expired or still valid? candidate_names = set(candidate_lineage.names()) if candidate_names == set(domains): - identical_names_cert = (renewal_file, candidate_lineage) + identical_names_cert = candidate_lineage elif candidate_names.issubset(set(domains)): - subset_names_cert = (renewal_file, candidate_lineage, - candidate_lineage.names()) + subset_names_cert = candidate_lineage return identical_names_cert, subset_names_cert @@ -229,20 +228,21 @@ def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-lo # I am not sure whether that correctly reads the systemwide # configuration file. question = None - if identical_names_cert: + if identical_names_cert is not None: question = ( "You have an existing certificate that contains exactly the " "same domains you requested (ref: {0})\n\nDo you want to " "renew and replace this certificate with a newly-issued one?" - ).format(identical_names_cert[0]) - elif subset_names_cert: + ).format(identical_names_cert.configfile.filename) + elif subset_names_cert is not None: question = ( "You have an existing certificate that contains a portion of " "the domains you requested (ref: {0})\n\nIt contains these " "names: {1}\n\nYou requested these names for the new " "certificate: {2}.\n\nDo you want to replace this existing " "certificate with the new certificate?" - ).format(subset_names_cert[0], ", ".join(subset_names_cert[2]), + ).format(subset_names_cert.configfile.filename, + ", ".join(subset_names_cert.names()), ", ".join(domains)) if question is None: # We aren't in a duplicative-names situation at all, so we don't @@ -257,19 +257,16 @@ def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-lo "To obtain a new certificate that {0} an existing certificate " "in its domain-name coverage, you must use the --duplicate " "option.\n\nFor example:\n\n{1} --duplicate {2}").format( - "duplicates" if identical_names_cert else "overlaps with", - sys.argv[0], " ".join(sys.argv[1:])), - reporter_util.HIGH_PRIORITY, True) - sys.exit(1) + "duplicates" if identical_names_cert is not None else + "overlaps with", sys.argv[0], " ".join(sys.argv[1:])), + reporter_util.HIGH_PRIORITY) + return 1 # Attempting to obtain the certificate # TODO: Handle errors from _init_le_client? le_client = _init_le_client(args, config, authenticator, installer) if treat_as_renewal: - if identical_names_cert: - lineage = identical_names_cert[1] - else: - lineage = subset_names_cert[1] + lineage = identical_names_cert if identical_names_cert is not None else subset_names_cert # TODO: Use existing privkey instead of generating a new one new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains) # TODO: Check whether it worked! diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py index bc9f32af5..504011180 100644 --- a/letsencrypt/storage.py +++ b/letsencrypt/storage.py @@ -437,8 +437,7 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes else: target = self.version("cert", version) with open(target) as f: - sans = crypto_util.get_sans_from_cert(f.read()) - return sans + return crypto_util.get_sans_from_cert(f.read()) def should_autodeploy(self): """Should this lineage now automatically deploy a newer version? diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 3903c824e..9442a3992 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -1,5 +1,4 @@ """Tests for letsencrypt.cli.""" -import configobj import itertools import os import shutil @@ -7,6 +6,7 @@ import traceback import tempfile import unittest +import configobj import mock from letsencrypt import account @@ -14,7 +14,6 @@ from letsencrypt import configuration from letsencrypt import errors from letsencrypt import storage -from letsencrypt.storage import ALL_FOUR from letsencrypt.tests import test_util @@ -178,7 +177,7 @@ class DuplicativeCertsTest(unittest.TestCase): os.makedirs(os.path.join(self.tempdir, "archive", "example.org")) os.makedirs(os.path.join(self.tempdir, "configs")) config = configobj.ConfigObj() - for kind in ALL_FOUR: + for kind in storage.ALL_FOUR: config[kind] = os.path.join(self.tempdir, "live", "example.org", kind + ".pem") config.filename = os.path.join(self.tempdir, "configs", @@ -188,7 +187,7 @@ class DuplicativeCertsTest(unittest.TestCase): self.defaults = configobj.ConfigObj() self.test_rc = storage.RenewableCert( self.config, self.defaults, self.cli_config) - for kind in ALL_FOUR: + for kind in storage.ALL_FOUR: where = getattr(self.test_rc, kind) os.symlink(os.path.join("..", "..", "archive", "example.org", "{0}12.pem".format(kind)), where) @@ -219,15 +218,15 @@ class DuplicativeCertsTest(unittest.TestCase): # Totally identical result = _find_duplicative_certs(["example.com", "www.example.com"], self.config, self.cli_config) - self.assertEqual(result[0][0], "example.org.conf") + 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) - self.assertEqual(result[1][0], "example.org.conf") 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"],