From cc607480ae96661d2038728e44ad542d6555add4 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 10 Sep 2015 20:12:32 +0000 Subject: [PATCH 1/6] acme: fetch_chain for multiple up links --- acme/acme/client.py | 32 +++++++++++++++++++++++--------- acme/acme/client_test.py | 30 ++++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index ef982b093..d2b72a36a 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -417,20 +417,34 @@ class Client(object): # pylint: disable=too-many-instance-attributes # respond with status code 403 (Forbidden) return self.check_cert(certr) - def fetch_chain(self, certr): + def fetch_chain(self, certr, max_length=10): """Fetch chain for certificate. - :param certr: Certificate Resource - :type certr: `.CertificateResource` + :param .CertificateResource certr: Certificate Resource + :param int max_length: Maximum allowed length of the chain. + Note that each element in the certificate requires new + ``HTTP GET`` request, and the length of the chain is + controlled by the ACME CA. - :returns: Certificate chain, or `None` if no "up" Link was provided. - :rtype: `OpenSSL.crypto.X509` wrapped in `.ComparableX509` + :raises errors.Error: if recursion exceeds `max_length` + + :returns: Certificate chain for the Certificate Resource. It is + a list ordered so that the first element is a signer of the + certificate from Certificate Resource. Will be empty if + ``cert_chain_uri`` is ``None``. + :rtype: `list` of `OpenSSL.crypto.X509` wrapped in `.ComparableX509` """ - if certr.cert_chain_uri is not None: - return self._get_cert(certr.cert_chain_uri)[1] - else: - return None + chain = [] + uri = certr.cert_chain_uri + while uri is not None and len(chain) < max_length: + response, cert = self._get_cert(uri) + uri = response.links.get('up', {}).get('url') + chain.append(cert) + if uri is not None: + raise errors.Error( + "Recursion limit reached. Didn't get {0}".format(uri)) + return chain def revoke(self, cert): """Revoke certificate. diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index dcc0832e3..c66c8e0a9 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -335,16 +335,34 @@ class ClientTest(unittest.TestCase): self.assertEqual( self.client.check_cert(self.certr), self.client.refresh(self.certr)) - def test_fetch_chain(self): + def test_fetch_chain_no_up_link(self): + self.assertEqual([], self.client.fetch_chain(self.certr.update( + cert_chain_uri=None))) + + def test_fetch_chain_single(self): # pylint: disable=protected-access self.client._get_cert = mock.MagicMock() - self.client._get_cert.return_value = ("response", "certificate") - self.assertEqual(self.client._get_cert(self.certr.cert_chain_uri)[1], + self.client._get_cert.return_value = ( + mock.MagicMock(links={}), "certificate") + self.assertEqual([self.client._get_cert(self.certr.cert_chain_uri)[1]], self.client.fetch_chain(self.certr)) - def test_fetch_chain_no_up_link(self): - self.assertTrue(self.client.fetch_chain(self.certr.update( - cert_chain_uri=None)) is None) + def test_fetch_chain_max(self): + # pylint: disable=protected-access + up_response = mock.MagicMock(links={'up': {'url': 'http://cert'}}) + noup_response = mock.MagicMock(links={}) + self.client._get_cert = mock.MagicMock() + self.client._get_cert.side_effect = [ + (up_response, "cert")] * 9 + [(noup_response, "last_cert")] + chain = self.client.fetch_chain(self.certr, max_length=10) + self.assertEqual(chain, ["cert"] * 9 + ["last_cert"]) + + def test_fetch_chain_too_many(self): # recursive + # pylint: disable=protected-access + response = mock.MagicMock(links={'up': {'url': 'http://cert'}}) + self.client._get_cert = mock.MagicMock() + self.client._get_cert.return_value = (response, "certificate") + self.assertRaises(errors.Error, self.client.fetch_chain, self.certr) def test_revoke(self): self.client.revoke(self.certr.body) From 0275271ecd36015ec8720e11ec8f83851c7e1963 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 10 Sep 2015 20:13:30 +0000 Subject: [PATCH 2/6] Multi cert chains (fixes #633). --- letsencrypt/client.py | 22 +++++++++++++--------- letsencrypt/storage.py | 2 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index e8dd08c8e..89453d232 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -261,17 +261,22 @@ class Client(object): "Non-standard path(s), might not work with crontab installed " "by your operating system package manager") - # XXX: just to stop RenewableCert from complaining; this is - # probably not a good solution - chain_pem = "" if chain is None else OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_PEM, chain) lineage = storage.RenewableCert.new_lineage( domains[0], OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body), - key.pem, chain_pem, params, config, cli_config) + key.pem, self._dump_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. @@ -306,7 +311,7 @@ class Client(object): :param certr: ACME "certificate" resource. :type certr: :class:`acme.messages.Certificate` - :param chain_cert: + :param list chain_cert: :param str cert_path: Candidate path to a certificate. :param str chain_path: Candidate path to a certificate chain. @@ -333,12 +338,11 @@ class Client(object): logger.info("Server issued certificate; certificate written to %s", act_cert_path) - if chain_cert is not None: + if chain_cert: chain_file, act_chain_path = le_util.unique_file( chain_path, 0o644) # TODO: Except - chain_pem = OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_PEM, chain_cert) + chain_pem = self._dump_chain(chain_cert) try: chain_file.write(chain_pem) finally: diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py index 5b1e90edc..3c703faa2 100644 --- a/letsencrypt/storage.py +++ b/letsencrypt/storage.py @@ -586,6 +586,8 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes with open(target["chain"], "w") as f: f.write(chain) with open(target["fullchain"], "w") as f: + # assumes that OpenSSL.crypto.dump_certificate includes + # ending newline character f.write(cert + chain) # Document what we've done in a new renewal config file From 62a9556bd20d4791c69f10225c567ae10107c3e1 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 10 Sep 2015 21:20:48 +0000 Subject: [PATCH 3/6] Add unittest for save_certificate --- letsencrypt/tests/client_test.py | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index b992089cc..0760b78d3 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -1,4 +1,7 @@ """Tests for letsencrypt.client.""" +import os +import shutil +import tempfile import unittest import configobj @@ -145,6 +148,36 @@ class ClientTest(unittest.TestCase): self.assertTrue("renewal but not automatic deployment" in msg) self.assertTrue(cert.cli_config.renewal_configs_dir in msg) + 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]) + candidate_cert_path = os.path.join(tmp_path, "certs", "cert.pem") + candidate_chain_path = os.path.join(tmp_path, "chains", "chain.pem") + + cert_path, chain_path = self.client.save_certificate( + certr, [cert1, cert2], candidate_cert_path, candidate_chain_path) + + self.assertEqual(os.path.dirname(cert_path), + os.path.dirname(candidate_cert_path)) + self.assertEqual(os.path.dirname(chain_path), + os.path.dirname(candidate_chain_path)) + + with open(cert_path, "r") as cert_file: + cert_contents = cert_file.read() + self.assertEqual(cert_contents, test_util.load_vector(certs[0])) + + with open(chain_path, "r") as chain_file: + chain_contents = chain_file.read() + self.assertEqual(chain_contents, test_util.load_vector(certs[1]) + + test_util.load_vector(certs[2])) + + shutil.rmtree(tmp_path) + class RollbackTest(unittest.TestCase): """Tests for letsencrypt.client.rollback.""" From 491b7a7cde7739f495612febf18da54b915dbc4d Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 10 Sep 2015 21:48:23 +0000 Subject: [PATCH 4/6] 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)) From 3e59ed69391ffed0de718c8984d5afcca2d6b2cf Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 15 Sep 2015 18:25:49 -0700 Subject: [PATCH 5/6] Fix new call to save_successor --- letsencrypt/cli.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index c6f90ea70..59030ff31 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -276,8 +276,7 @@ def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-lo lineage.save_successor( lineage.latest_common_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)) lineage.update_all_links_to(lineage.latest_common_version()) # TODO: Check return value of save_successor From 2945e0657dabd2e22f8d715009d4784c9a9c23c9 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 15 Sep 2015 19:01:55 -0700 Subject: [PATCH 6/6] Don't run tox for temporarily-disabled python versions --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 2b2466c3b..b10558077 100644 --- a/tox.ini +++ b/tox.ini @@ -6,7 +6,7 @@ # acme and letsencrypt are not yet on pypi, so when Tox invokes # "install *.zip", it will not find deps skipsdist = true -envlist = py26,py27,py33,py34,cover,lint +envlist = py27,cover,lint [testenv] commands =