From f13566e5ba29edfad9879bc5eacb1e78eb435ec7 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 22 Oct 2015 00:35:52 -0700 Subject: [PATCH] Quick refactor --- letsencrypt/client.py | 64 ++++++++++++++++---------------- letsencrypt/tests/client_test.py | 8 ++-- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index 31597c7d3..3a6d90472 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -258,8 +258,8 @@ class Client(object): params, config, cli_config) return lineage - def save_certificate(self, certr, chain_cert, cert_path, chain_path, fullchain_path): - # pylint: disable=no-self-use,too-many-locals + def save_certificate(self, certr, chain_cert, + cert_path, chain_path, fullchain_path): """Saves the certificate received from the ACME server. :param certr: ACME "certificate" resource. @@ -270,7 +270,8 @@ class Client(object): :param str chain_path: Candidate path to a certificate chain. :param str fullchain_path: Candidate path to a full cert chain. - :returns: cert_path, chain_path, fullchain_path (absolute paths to the actual files) + :returns: cert_path, chain_path, and fullchain_path as absolute + paths to the actual files :rtype: `tuple` of `str` :raises IOError: If unable to find room to write the cert files @@ -281,13 +282,9 @@ class Client(object): os.path.dirname(path), 0o755, os.geteuid(), self.config.strict_permissions) - # try finally close - cert_chain_abspath = None - fullchain_abspath = None - cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644) - # TODO: Except cert_pem = OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body) + cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644) try: cert_file.write(cert_pem) finally: @@ -295,32 +292,13 @@ class Client(object): logger.info("Server issued certificate; certificate written to %s", act_cert_path) - # TODO too long, refactor... either split this into another function - # up one level (though it needs a copy of cert_pem) or find a way to - # reuse machinery from storage.py + cert_chain_abspath = None + fullchain_abspath = None if chain_cert: - chain_file, act_chain_path = le_util.unique_file(chain_path, 0o644) - # TODO: Except chain_pem = crypto_util.dump_pyopenssl_chain(chain_cert) - try: - chain_file.write(chain_pem) - finally: - chain_file.close() - - logger.info("Cert chain written to %s", act_chain_path) - - # This expects a valid chain file - cert_chain_abspath = os.path.abspath(act_chain_path) - - # fullchain is cert + chain - fullchain_file, act_fullchain_path = le_util.unique_file( - fullchain_path, 0o644) - try: - fullchain_file.write(cert_pem + chain_pem) - finally: - fullchain_file.close() - logger.info("Cert chain written to %s", act_fullchain_path) - fullchain_abspath = os.path.abspath(act_fullchain_path) + cert_chain_abspath = _save_chain(chain_pem, chain_path) + fullchain_abspath = _save_chain(cert_pem + chain_pem, + fullchain_path) return os.path.abspath(act_cert_path), cert_chain_abspath, fullchain_abspath @@ -479,3 +457,25 @@ def view_config_changes(config): rev = reverter.Reverter(config) rev.recovery_routine() rev.view_config_changes() + + +def _save_chain(chain_pem, chain_path): + """Saves chain_pem at a unique path based on chain_path. + + :param str chain_pem: certificate chain in PEM format + :param str chain_path: candidate path for the cert chain + + :returns: absolute path to saved cert chain + :rtype: str + + """ + chain_file, act_chain_path = le_util.unique_file(chain_path, 0o644) + try: + chain_file.write(chain_pem) + finally: + chain_file.close() + + logger.info("Cert chain written to %s", act_chain_path) + + # This expects a valid chain file + return os.path.abspath(act_chain_path) diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index 13613374f..2efe11108 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -114,20 +114,20 @@ class ClientTest(unittest.TestCase): mock.sentinel.key, domains, self.config.csr_dir) self._check_obtain_certificate() - def test_save_certificate(self): # pylint: disable=too-many-locals + def test_save_certificate(self): certs = ["matching_cert.pem", "cert.pem", "cert-san.pem"] tmp_path = tempfile.mkdtemp() os.chmod(tmp_path, 0o755) # TODO: really?? certr = mock.MagicMock(body=test_util.load_cert(certs[0])) - cert1 = test_util.load_cert(certs[1]) - cert2 = test_util.load_cert(certs[2]) + chain_cert = [test_util.load_cert(certs[1]), + test_util.load_cert(certs[2])] candidate_cert_path = os.path.join(tmp_path, "certs", "cert.pem") candidate_chain_path = os.path.join(tmp_path, "chains", "chain.pem") candidate_fullchain_path = os.path.join(tmp_path, "chains", "fullchain.pem") cert_path, chain_path, fullchain_path = self.client.save_certificate( - certr, [cert1, cert2], candidate_cert_path, candidate_chain_path, + certr, chain_cert, candidate_cert_path, candidate_chain_path, candidate_fullchain_path) self.assertEqual(os.path.dirname(cert_path),