From 491b7a7cde7739f495612febf18da54b915dbc4d Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 10 Sep 2015 21:48:23 +0000 Subject: [PATCH] Fix multi-cert chains in renewer --- letsencrypt/client.py | 14 +++----------- letsencrypt/crypto_util.py | 22 ++++++++++++++++++++++ letsencrypt/renewer.py | 5 ++--- letsencrypt/tests/renewer_test.py | 4 ++-- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index 89453d232..b49659e71 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -264,19 +264,11 @@ class Client(object): lineage = storage.RenewableCert.new_lineage( domains[0], OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body), - key.pem, self._dump_chain(chain), params, config, cli_config) + key.pem, crypto_util.dump_pyopenssl_chain(chain), + params, config, cli_config) self._report_renewal_status(lineage) return lineage - @staticmethod - def _dump_chain(chain, filetype=OpenSSL.crypto.FILETYPE_PEM): - # assumes that OpenSSL.crypto.dump_certificate includes ending - # newline character; XXX: returns empty string when no chain - # is available, which shuts up RenewableCert, but might not be - # the best solution... - return "".join(OpenSSL.crypto.dump_certificate( - filetype, cert) for cert in chain) - def _report_renewal_status(self, cert): # pylint: disable=no-self-use """Informs the user about automatic renewal and deployment. @@ -342,7 +334,7 @@ class Client(object): chain_file, act_chain_path = le_util.unique_file( chain_path, 0o644) # TODO: Except - chain_pem = self._dump_chain(chain_cert) + chain_pem = crypto_util.dump_pyopenssl_chain(chain_cert) try: chain_file.write(chain_pem) finally: diff --git a/letsencrypt/crypto_util.py b/letsencrypt/crypto_util.py index 279330f0c..3ef843012 100644 --- a/letsencrypt/crypto_util.py +++ b/letsencrypt/crypto_util.py @@ -11,6 +11,7 @@ import os import OpenSSL from acme import crypto_util as acme_crypto_util +from acme import jose from letsencrypt import errors from letsencrypt import le_util @@ -269,3 +270,24 @@ def asn1_generalizedtime_to_dt(timestamp): def pyopenssl_x509_name_as_text(x509name): """Convert `OpenSSL.crypto.X509Name` to text.""" return "/".join("{0}={1}" for key, value in x509name.get_components()) + + +def dump_pyopenssl_chain(chain, filetype=OpenSSL.crypto.FILETYPE_PEM): + """Dump certificate chain into a bundle. + + :param list chain: List of `OpenSSL.crypto.X509` (or wrapped in + `acme.jose.ComparableX509`). + + """ + # XXX: returns empty string when no chain is available, which + # shuts up RenewableCert, but might not be the best solution... + + def _dump_cert(cert): + if isinstance(cert, jose.ComparableX509): + # pylint: disable=protected-access + cert = cert._wrapped + return OpenSSL.crypto.dump_certificate(filetype, cert) + + # assumes that OpenSSL.crypto.dump_certificate includes ending + # newline character + return "".join(_dump_cert(cert) for cert in chain) diff --git a/letsencrypt/renewer.py b/letsencrypt/renewer.py index e26e8742b..5f73a7dad 100644 --- a/letsencrypt/renewer.py +++ b/letsencrypt/renewer.py @@ -85,7 +85,7 @@ def renew(cert, old_version): with open(cert.version("cert", old_version)) as f: sans = crypto_util.get_sans_from_cert(f.read()) new_certr, new_chain, new_key, _ = le_client.obtain_certificate(sans) - if new_chain is not None: + if new_chain: # XXX: Assumes that there was no key change. We need logic # for figuring out whether there was or not. Probably # best is to have obtain_certificate return None for @@ -94,8 +94,7 @@ def renew(cert, old_version): return cert.save_successor( old_version, OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, new_certr.body), - new_key.pem, OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_PEM, new_chain)) + new_key.pem, crypto_util.dump_pyopenssl_chain(new_chain)) # TODO: Notify results else: # TODO: Notify negative results diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py index 1b58d9e0f..a0078deb2 100644 --- a/letsencrypt/tests/renewer_test.py +++ b/letsencrypt/tests/renewer_test.py @@ -596,7 +596,7 @@ class RenewableCertTests(unittest.TestCase): mock_client = mock.MagicMock() # pylint: disable=star-args mock_client.obtain_certificate.return_value = ( - mock.MagicMock(body=CERT), CERT, mock.Mock(pem="key"), + mock.MagicMock(body=CERT), [CERT], mock.Mock(pem="key"), mock.sentinel.csr) mock_c.return_value = mock_client self.assertEqual(2, renewer.renew(self.test_rc, 1)) @@ -604,7 +604,7 @@ class RenewableCertTests(unittest.TestCase): # have been made to the mock functions here. mock_acc_storage().load.assert_called_once_with(account_id="abcde") mock_client.obtain_certificate.return_value = ( - mock.sentinel.certr, None, mock.sentinel.key, mock.sentinel.csr) + mock.sentinel.certr, [], mock.sentinel.key, mock.sentinel.csr) # This should fail because the renewal itself appears to fail self.assertFalse(renewer.renew(self.test_rc, 1))