From 62bdf663f2ec45379c6d15fc92b83536517f33e0 Mon Sep 17 00:00:00 2001 From: Baime Date: Sat, 8 Jul 2017 16:20:12 +0200 Subject: [PATCH 1/5] Check keys if revoke certificate by private key --- certbot/crypto_util.py | 15 ++++++++------- certbot/main.py | 1 + certbot/tests/crypto_util_test.py | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index e22effeb7..2c9dad8bf 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -214,7 +214,7 @@ def verify_renewable_cert(renewable_cert): """ verify_renewable_cert_sig(renewable_cert) verify_fullchain(renewable_cert) - verify_cert_matches_priv_key(renewable_cert) + verify_cert_matches_priv_key(renewable_cert.cert, renewable_cert.privkey) def verify_renewable_cert_sig(renewable_cert): @@ -238,17 +238,18 @@ def verify_renewable_cert_sig(renewable_cert): raise errors.Error(error_str) -def verify_cert_matches_priv_key(renewable_cert): +def verify_cert_matches_priv_key(cert_path, key_path): """ Verifies that the private key and cert match. - :param `.storage.RenewableCert` renewable_cert: cert to verify + :param str cert_path: path to a cert in PEM format + :param str key_path: path to a private key file :raises errors.Error: If they don't match. """ try: - with open(renewable_cert.cert) as cert: + with open(cert_path) as cert: cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM, cert.read()) - with open(renewable_cert.privkey) as privkey: + with open(key_path) as privkey: privkey = OpenSSL.crypto.load_privatekey(OpenSSL.crypto.FILETYPE_PEM, privkey.read()) context = OpenSSL.SSL.Context(OpenSSL.SSL.SSLv23_METHOD) context.use_privatekey(privkey) @@ -257,8 +258,8 @@ def verify_cert_matches_priv_key(renewable_cert): except (IOError, OpenSSL.SSL.Error) as e: error_str = "verifying the cert located at {0} matches the \ private key located at {1} has failed. \ - Details: {2}".format(renewable_cert.cert, - renewable_cert.privkey, e) + Details: {2}".format(cert_path, + key_path, e) logger.exception(error_str) raise errors.Error(error_str) diff --git a/certbot/main.py b/certbot/main.py index f7421d75e..c055b9ba9 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -562,6 +562,7 @@ def revoke(config, unused_plugins): # TODO: coop with renewal config if config.key_path is not None: # revocation by cert key logger.debug("Revoking %s using cert key %s", config.cert_path[0], config.key_path[0]) + crypto_util.verify_cert_matches_priv_key(config.cert_path[0], config.key_path[1]) key = jose.JWK.load(config.key_path[1]) else: # revocation by account key logger.debug("Revoking %s using Account Key", config.cert_path[0]) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index 729b09dc1..cb9077208 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -252,7 +252,7 @@ class VerifyCertMatchesPrivKeyTest(VerifyCertSetup): def _call(self, renewable_cert): from certbot.crypto_util import verify_cert_matches_priv_key - return verify_cert_matches_priv_key(renewable_cert) + return verify_cert_matches_priv_key(renewable_cert.cert, renewable_cert.privkey) def test_cert_priv_key_match(self): self.assertEqual(None, self._call(self.renewable_cert)) From ab286e088728b349b5347c2f39fc8cba57d014d6 Mon Sep 17 00:00:00 2001 From: Baime Date: Sat, 8 Jul 2017 18:00:33 +0200 Subject: [PATCH 2/5] fixed path errors in revoking by key process --- certbot/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/main.py b/certbot/main.py index c055b9ba9..e7ed9bfe3 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -562,7 +562,7 @@ def revoke(config, unused_plugins): # TODO: coop with renewal config if config.key_path is not None: # revocation by cert key logger.debug("Revoking %s using cert key %s", config.cert_path[0], config.key_path[0]) - crypto_util.verify_cert_matches_priv_key(config.cert_path[0], config.key_path[1]) + crypto_util.verify_cert_matches_priv_key(config.cert_path[0], config.key_path[0]) key = jose.JWK.load(config.key_path[1]) else: # revocation by account key logger.debug("Revoking %s using Account Key", config.cert_path[0]) From 3a9150a7bacc3ee67b762c6f42c91ec9bf86711f Mon Sep 17 00:00:00 2001 From: Baime Date: Sat, 8 Jul 2017 19:35:07 +0200 Subject: [PATCH 3/5] Fix for revoke cert by key process --- certbot/crypto_util.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 2c9dad8bf..112ef7c85 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -247,13 +247,9 @@ def verify_cert_matches_priv_key(cert_path, key_path): :raises errors.Error: If they don't match. """ try: - with open(cert_path) as cert: - cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM, cert.read()) - with open(key_path) as privkey: - privkey = OpenSSL.crypto.load_privatekey(OpenSSL.crypto.FILETYPE_PEM, privkey.read()) context = OpenSSL.SSL.Context(OpenSSL.SSL.SSLv23_METHOD) - context.use_privatekey(privkey) - context.use_certificate(cert) + context.use_certificate_file(cert_path) + context.use_privatekey_file(key_path) context.check_privatekey() except (IOError, OpenSSL.SSL.Error) as e: error_str = "verifying the cert located at {0} matches the \ From 368beee8bfd9413771d770df0e1b562e9c9396dd Mon Sep 17 00:00:00 2001 From: Baime Date: Sun, 9 Jul 2017 12:10:35 +0200 Subject: [PATCH 4/5] Use correct key/cert for revoke by key test --- certbot/tests/crypto_util_test.py | 2 ++ certbot/tests/main_test.py | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index cb9077208..bc41ca950 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -255,6 +255,8 @@ class VerifyCertMatchesPrivKeyTest(VerifyCertSetup): return verify_cert_matches_priv_key(renewable_cert.cert, renewable_cert.privkey) def test_cert_priv_key_match(self): + self.renewable_cert.cert = SS_CERT_PATH + self.renewable_cert.privkey = RSA2048_KEY_PATH self.assertEqual(None, self._call(self.renewable_cert)) def test_cert_priv_key_mismatch(self): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 7c2016178..25f8d1631 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -37,6 +37,8 @@ CERT = test_util.vector_path('cert.pem') CSR = test_util.vector_path('csr.der') KEY = test_util.vector_path('rsa256_key.pem') JWK = jose.JWKRSA.load(test_util.load_vector("rsa512_key_2.pem")) +RSA2048_KEY_PATH = test_util.vector_path('rsa2048_key.pem') +SS_CERT_PATH = test_util.vector_path('self_signed_cert.pem') class TestHandleIdenticalCerts(unittest.TestCase): @@ -1012,12 +1014,12 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me @mock.patch('certbot.main.client.acme_client') def test_revoke_with_key(self, mock_acme_client): server = 'foo.bar' - self._call_no_clientmock(['--cert-path', CERT, '--key-path', KEY, + self._call_no_clientmock(['--cert-path', SS_CERT_PATH, '--key-path', RSA2048_KEY_PATH, '--server', server, 'revoke']) - with open(KEY, 'rb') as f: + with open(RSA2048_KEY_PATH, 'rb') as f: mock_acme_client.Client.assert_called_once_with( server, key=jose.JWK.load(f.read()), net=mock.ANY) - with open(CERT, 'rb') as f: + with open(SS_CERT_PATH, 'rb') as f: cert = crypto_util.pyopenssl_load_certificate(f.read())[0] mock_revoke = mock_acme_client.Client().revoke mock_revoke.assert_called_once_with( From 0f30f9e96ff346cc88b7217f9e236f25aa727577 Mon Sep 17 00:00:00 2001 From: Baime Date: Sat, 15 Jul 2017 15:17:25 +0200 Subject: [PATCH 5/5] Added test to check revoke cert by key mismatch --- certbot/tests/main_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 25f8d1631..c86e53834 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1026,6 +1026,12 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me jose.ComparableX509(cert), mock.ANY) + def test_revoke_with_key_mismatch(self): + server = 'foo.bar' + self.assertRaises(errors.Error, self._call_no_clientmock, + ['--cert-path', CERT, '--key-path', KEY, + '--server', server, 'revoke']) + @mock.patch('certbot.main._determine_account') def test_revoke_without_key(self, mock_determine_account): mock_determine_account.return_value = (mock.MagicMock(), None)