From e1878593d5608f908eebb262fe2c9c7dfcab55a1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 5 Mar 2018 07:27:44 -0800 Subject: [PATCH] Ensure fullchain_pem in the order is unicode/str (#5654) * Decode fullchain_pem in ACMEv1 * Convert back to bytes in Certbot * document bytes are returned --- acme/acme/client.py | 4 ++-- acme/acme/client_test.py | 4 ++-- acme/acme/crypto_util.py | 3 +++ certbot/client.py | 5 +++-- certbot/crypto_util.py | 3 ++- certbot/tests/client_test.py | 22 ++++++++++++---------- certbot/tests/crypto_util_test.py | 4 ++-- 7 files changed, 26 insertions(+), 19 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index c6e897692..d52c82a5c 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -809,8 +809,8 @@ class BackwardsCompatibleClientV2(object): 'certificate, please rerun the command for a new one.') cert = OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped) - chain = crypto_util.dump_pyopenssl_chain(chain) + OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped).decode() + chain = crypto_util.dump_pyopenssl_chain(chain).decode() return orderr.update(fullchain_pem=(cert + chain)) else: diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 060338360..a0c27e74f 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -99,10 +99,10 @@ class BackwardsCompatibleClientV2Test(ClientTestBase): self.chain = [wrapped, wrapped] self.cert_pem = OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_PEM, messages_test.CERT.wrapped) + OpenSSL.crypto.FILETYPE_PEM, messages_test.CERT.wrapped).decode() single_chain = OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_PEM, loaded) + OpenSSL.crypto.FILETYPE_PEM, loaded).decode() self.chain_pem = single_chain + single_chain self.fullchain_pem = self.cert_pem + self.chain_pem diff --git a/acme/acme/crypto_util.py b/acme/acme/crypto_util.py index 07b55ae33..2281196eb 100644 --- a/acme/acme/crypto_util.py +++ b/acme/acme/crypto_util.py @@ -287,6 +287,9 @@ def dump_pyopenssl_chain(chain, filetype=OpenSSL.crypto.FILETYPE_PEM): :param list chain: List of `OpenSSL.crypto.X509` (or wrapped in :class:`josepy.util.ComparableX509`). + :returns: certificate chain bundle + :rtype: bytes + """ # XXX: returns empty string when no chain is available, which # shuts up RenewableCert, but might not be the best solution... diff --git a/certbot/client.py b/certbot/client.py index eddf93e4f..2992c0cec 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -244,7 +244,7 @@ class Client(object): than `authkey`. :param acme.messages.OrderResource orderr: contains authzrs - :returns: certificate and chain as PEM strings + :returns: certificate and chain as PEM byte strings :rtype: tuple """ @@ -263,7 +263,8 @@ class Client(object): deadline = datetime.datetime.now() + datetime.timedelta(seconds=90) orderr = self.acme.finalize_order(orderr, deadline) - return crypto_util.cert_and_chain_from_fullchain(orderr.fullchain_pem) + cert, chain = crypto_util.cert_and_chain_from_fullchain(orderr.fullchain_pem) + return cert.encode(), chain.encode() def obtain_certificate(self, domains): """Obtains a certificate from the ACME server. diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 11721cc10..37118c591 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -441,8 +441,9 @@ def cert_and_chain_from_fullchain(fullchain_pem): :returns: tuple of string cert_pem and chain_pem :rtype: tuple + """ cert = OpenSSL.crypto.dump_certificate(OpenSSL.crypto.FILETYPE_PEM, - OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM, fullchain_pem)) + OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM, fullchain_pem)).decode() chain = fullchain_pem[len(cert):] return (cert, chain) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 5d01b103a..0f2c58161 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -132,7 +132,6 @@ class ClientTest(ClientTestCommon): self.eg_domains = ["example.com", "www.example.com"] self.eg_order = mock.MagicMock( authorizations=[None], - fullchain_pem=mock.sentinel.fullchain_pem, csr_pem=mock.sentinel.csr_pem) def test_init_acme_verify_ssl(self): @@ -165,8 +164,7 @@ class ClientTest(ClientTestCommon): self._mock_obtain_certificate() test_csr = util.CSR(form="pem", file=None, data=CSR_SAN) auth_handler = self.client.auth_handler - mock_crypto_util.cert_and_chain_from_fullchain.return_value = (mock.sentinel.cert, - mock.sentinel.chain) + self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) orderr = self.acme.new_order(test_csr.data) auth_handler.handle_authorizations(orderr, False) @@ -199,8 +197,7 @@ class ClientTest(ClientTestCommon): csr = util.CSR(form="pem", file=None, data=CSR_SAN) mock_crypto_util.init_save_csr.return_value = csr mock_crypto_util.init_save_key.return_value = mock.sentinel.key - mock_crypto_util.cert_and_chain_from_fullchain.return_value = (mock.sentinel.cert, - mock.sentinel.chain) + self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) self._test_obtain_certificate_common(mock.sentinel.key, csr) @@ -209,7 +206,7 @@ class ClientTest(ClientTestCommon): mock_crypto_util.init_save_csr.assert_called_once_with( mock.sentinel.key, self.eg_domains, self.config.csr_dir) mock_crypto_util.cert_and_chain_from_fullchain.assert_called_once_with( - mock.sentinel.fullchain_pem) + self.eg_order.fullchain_pem) @mock.patch("certbot.client.crypto_util") @mock.patch("os.remove") @@ -218,8 +215,7 @@ class ClientTest(ClientTestCommon): key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN) mock_crypto_util.init_save_csr.return_value = csr mock_crypto_util.init_save_key.return_value = key - mock_crypto_util.cert_and_chain_from_fullchain.return_value = (mock.sentinel.cert, - mock.sentinel.chain) + self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) authzr = self._authzr_from_domains(["example.com"]) self.config.allow_subset_of_names = True @@ -237,8 +233,7 @@ class ClientTest(ClientTestCommon): mock_acme_crypto.make_csr.return_value = CSR_SAN mock_crypto.make_key.return_value = mock.sentinel.key_pem key = util.Key(file=None, pem=mock.sentinel.key_pem) - mock_crypto.cert_and_chain_from_fullchain.return_value = (mock.sentinel.cert, - mock.sentinel.chain) + self._set_mock_from_fullchain(mock_crypto.cert_and_chain_from_fullchain) self.client.config.dry_run = True self._test_obtain_certificate_common(key, csr) @@ -250,6 +245,13 @@ class ClientTest(ClientTestCommon): mock_crypto.init_save_csr.assert_not_called() self.assertEqual(mock_crypto.cert_and_chain_from_fullchain.call_count, 1) + def _set_mock_from_fullchain(self, mock_from_fullchain): + mock_cert = mock.Mock() + mock_cert.encode.return_value = mock.sentinel.cert + mock_chain = mock.Mock() + mock_chain.encode.return_value = mock.sentinel.chain + mock_from_fullchain.return_value = (mock_cert, mock_chain) + def _authzr_from_domains(self, domains): authzr = [] diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index 00303fab3..480139378 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -377,8 +377,8 @@ class CertAndChainFromFullchainTest(unittest.TestCase): """Tests for certbot.crypto_util.cert_and_chain_from_fullchain""" def test_cert_and_chain_from_fullchain(self): - cert_pem = CERT - chain_pem = CERT + SS_CERT + cert_pem = CERT.decode() + chain_pem = cert_pem + SS_CERT.decode() fullchain_pem = cert_pem + chain_pem from certbot.crypto_util import cert_and_chain_from_fullchain cert_out, chain_out = cert_and_chain_from_fullchain(fullchain_pem)