From 90227c4ed75afc9922edd04c090027d79b2eb825 Mon Sep 17 00:00:00 2001 From: Ariel Mendelzon Date: Mon, 6 Jan 2025 15:30:04 -0300 Subject: [PATCH 1/2] Certificate V2 x509_pem type element validation - Implemented HSMCertificateV2ElementX509's is_valid method - Added root of trust element validation in SGX attestation verification - Added and updated unit tests --- middleware/admin/certificate_v2.py | 36 +++++- middleware/admin/verify_sgx_attestation.py | 3 + .../admin/test_certificate_v2_element_x509.py | 107 ++++++++++++++++-- .../admin/test_verify_sgx_attestation.py | 54 +++++++-- 4 files changed, 182 insertions(+), 18 deletions(-) diff --git a/middleware/admin/certificate_v2.py b/middleware/admin/certificate_v2.py index c61faa3c..54ca1f45 100644 --- a/middleware/admin/certificate_v2.py +++ b/middleware/admin/certificate_v2.py @@ -21,13 +21,14 @@ # SOFTWARE. import re -from pathlib import Path import base64 import ecdsa import hashlib +from datetime import datetime, UTC +from pathlib import Path from cryptography import x509 from cryptography.hazmat.primitives.serialization import PublicFormat, Encoding -from cryptography.hazmat.primitives.asymmetric.ec import SECP256R1 +from cryptography.hazmat.primitives.asymmetric import ec from .certificate_v1 import HSMCertificate from .utils import is_nonempty_hex_string from sgx.envelope import SgxQuote, SgxReportBody @@ -254,13 +255,40 @@ def certificate(self): return self._certificate def is_valid(self, certifier): - return True + try: + # IMPORTANT: for now, we only allow verifying the validity of an + # HSMCertificateV2ElementX509 using another HSMCertificateV2ElementX509 + # instance as certifier. That way, we simplify the validation procedure + # and ensure maximum compatibility wrt the underlying library used + # (cryptography) + if not isinstance(certifier, type(self)): + return False + + subject = self.certificate + issuer = certifier.certificate + now = datetime.now(UTC) + + # 1. Check validity period + if subject.not_valid_before_utc > now or subject.not_valid_after_utc < now: + return False + + # 2. Verify the signature + issuer.public_key().verify( + subject.signature, + subject.tbs_certificate_bytes, + ec.ECDSA(subject.signature_hash_algorithm) + ) + + return True + + except Exception: + return False def get_pubkey(self): try: public_key = self.certificate.public_key() - if not isinstance(public_key.curve, SECP256R1): + if not isinstance(public_key.curve, ec.SECP256R1): raise ValueError("Certificate does not have a NIST P-256 public key") public_bytes = public_key.public_bytes( diff --git a/middleware/admin/verify_sgx_attestation.py b/middleware/admin/verify_sgx_attestation.py index 1e8672fb..bb0f2903 100644 --- a/middleware/admin/verify_sgx_attestation.py +++ b/middleware/admin/verify_sgx_attestation.py @@ -53,6 +53,9 @@ def do_verify_attestation(options): info(f"Attempting to gather root authority from {root_authority}...") try: root_of_trust = get_root_of_trust(root_authority) + info("Attempting to validate self-signed root authority...") + if not root_of_trust.is_valid(root_of_trust): + raise ValueError("Failed to validate self-signed root of trust") except Exception as e: raise AdminError(f"Invalid root authority {root_authority}: {e}") info(f"Using {root_authority} as root authority") diff --git a/middleware/tests/admin/test_certificate_v2_element_x509.py b/middleware/tests/admin/test_certificate_v2_element_x509.py index b5fd0c17..f4f8f4dc 100644 --- a/middleware/tests/admin/test_certificate_v2_element_x509.py +++ b/middleware/tests/admin/test_certificate_v2_element_x509.py @@ -20,6 +20,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. +from datetime import datetime, timedelta, UTC from unittest import TestCase from unittest.mock import Mock, patch from ecdsa import NIST256p @@ -111,7 +112,7 @@ def test_certificate(self, load_pem_x509_certificate): ) self.assertEqual(1, load_pem_x509_certificate.call_count) - def setup_is_valid_mocks(self, load_pem_x509_certificate, VerifyingKey): + def setup_pubkey_mocks(self, load_pem_x509_certificate, VerifyingKey): self.pubkey = Mock() self.pubkey.curve = SECP256R1() self.pubkey.public_bytes.return_value = "the-public-bytes" @@ -123,7 +124,7 @@ def setup_is_valid_mocks(self, load_pem_x509_certificate, VerifyingKey): @patch("admin.certificate_v2.ecdsa.VerifyingKey") @patch("admin.certificate_v2.x509.load_pem_x509_certificate") def test_get_pubkey_ok(self, load_pem_x509_certificate, VerifyingKey): - self.setup_is_valid_mocks(load_pem_x509_certificate, VerifyingKey) + self.setup_pubkey_mocks(load_pem_x509_certificate, VerifyingKey) self.assertEqual("the-expected-pubkey", self.elem.get_pubkey()) self.pubkey.public_bytes.assert_called_with( @@ -133,7 +134,7 @@ def test_get_pubkey_ok(self, load_pem_x509_certificate, VerifyingKey): @patch("admin.certificate_v2.ecdsa.VerifyingKey") @patch("admin.certificate_v2.x509.load_pem_x509_certificate") def test_get_pubkey_err_load_cert(self, load_pem_x509_certificate, VerifyingKey): - self.setup_is_valid_mocks(load_pem_x509_certificate, VerifyingKey) + self.setup_pubkey_mocks(load_pem_x509_certificate, VerifyingKey) load_pem_x509_certificate.side_effect = Exception("blah blah") with self.assertRaises(ValueError) as e: @@ -146,7 +147,7 @@ def test_get_pubkey_err_load_cert(self, load_pem_x509_certificate, VerifyingKey) @patch("admin.certificate_v2.ecdsa.VerifyingKey") @patch("admin.certificate_v2.x509.load_pem_x509_certificate") def test_get_pubkey_err_get_pub(self, load_pem_x509_certificate, VerifyingKey): - self.setup_is_valid_mocks(load_pem_x509_certificate, VerifyingKey) + self.setup_pubkey_mocks(load_pem_x509_certificate, VerifyingKey) self.cert.public_key.side_effect = Exception("blah blah") with self.assertRaises(ValueError) as e: @@ -160,7 +161,7 @@ def test_get_pubkey_err_get_pub(self, load_pem_x509_certificate, VerifyingKey): @patch("admin.certificate_v2.x509.load_pem_x509_certificate") def test_get_pubkey_err_pub_notnistp256(self, load_pem_x509_certificate, VerifyingKey): - self.setup_is_valid_mocks(load_pem_x509_certificate, VerifyingKey) + self.setup_pubkey_mocks(load_pem_x509_certificate, VerifyingKey) self.pubkey.curve = "somethingelse" with self.assertRaises(ValueError) as e: @@ -173,7 +174,7 @@ def test_get_pubkey_err_pub_notnistp256(self, load_pem_x509_certificate, @patch("admin.certificate_v2.ecdsa.VerifyingKey") @patch("admin.certificate_v2.x509.load_pem_x509_certificate") def test_get_pubkey_err_public_bytes(self, load_pem_x509_certificate, VerifyingKey): - self.setup_is_valid_mocks(load_pem_x509_certificate, VerifyingKey) + self.setup_pubkey_mocks(load_pem_x509_certificate, VerifyingKey) self.pubkey.public_bytes.side_effect = Exception("blah blah") with self.assertRaises(ValueError) as e: @@ -188,7 +189,7 @@ def test_get_pubkey_err_public_bytes(self, load_pem_x509_certificate, VerifyingK @patch("admin.certificate_v2.x509.load_pem_x509_certificate") def test_get_pubkey_err_ecdsafromstring(self, load_pem_x509_certificate, VerifyingKey): - self.setup_is_valid_mocks(load_pem_x509_certificate, VerifyingKey) + self.setup_pubkey_mocks(load_pem_x509_certificate, VerifyingKey) VerifyingKey.from_string.side_effect = Exception("blah blah") with self.assertRaises(ValueError) as e: @@ -198,3 +199,95 @@ def test_get_pubkey_err_ecdsafromstring(self, load_pem_x509_certificate, self.pubkey.public_bytes.assert_called_with( Encoding.X962, PublicFormat.CompressedPoint) VerifyingKey.from_string.assert_called_with("the-public-bytes", NIST256p) + + def setup_is_valid_mocks(self, load_pem_x509_certificate, ec): + self.certifier = HSMCertificateV2ElementX509({ + "name": "mock-certifier", + "signed_by": "someone-else", + "message": "Y2VydGlmaWVy" + }) + + self.mock_certifier = Mock() + self.mock_elem = Mock() + + def load_mock(data): + if b"Y2VydGlmaWVy" in data: + return self.mock_certifier + return self.mock_elem + + load_pem_x509_certificate.side_effect = load_mock + + self.now = datetime.now(UTC) + one_week = timedelta(weeks=1) + self.mock_elem.not_valid_before_utc = self.now - one_week + self.mock_elem.not_valid_after_utc = self.now + one_week + self.mock_certifier_pk = Mock() + self.mock_certifier.public_key.return_value = self.mock_certifier_pk + self.mock_elem.signature = "the-signature" + self.mock_elem.tbs_certificate_bytes = "the-fingerprint" + self.mock_elem.signature_hash_algorithm = "the-signature-hash-algo" + ec.ECDSA.return_value = "the-ecdsa-algo" + + @patch("admin.certificate_v2.ec") + @patch("admin.certificate_v2.x509.load_pem_x509_certificate") + def test_is_valid_ok(self, load_pem_x509_certificate, ec): + self.setup_is_valid_mocks(load_pem_x509_certificate, ec) + + self.assertTrue(self.elem.is_valid(self.certifier)) + + self.mock_certifier_pk.verify.assert_called_with( + "the-signature", + "the-fingerprint", + "the-ecdsa-algo" + ) + ec.ECDSA.assert_called_with("the-signature-hash-algo") + + @patch("admin.certificate_v2.ec") + @patch("admin.certificate_v2.x509.load_pem_x509_certificate") + def test_is_valid_before_in_future(self, load_pem_x509_certificate, ec): + self.setup_is_valid_mocks(load_pem_x509_certificate, ec) + self.mock_elem.not_valid_before_utc = self.now + \ + timedelta(minutes=1) + + self.assertFalse(self.elem.is_valid(self.certifier)) + + self.mock_certifier_pk.verify.assert_not_called() + ec.ECDSA.assert_not_called() + + @patch("admin.certificate_v2.ec") + @patch("admin.certificate_v2.x509.load_pem_x509_certificate") + def test_is_valid_after_in_past(self, load_pem_x509_certificate, ec): + self.setup_is_valid_mocks(load_pem_x509_certificate, ec) + self.mock_elem.not_valid_after_utc = self.now - \ + timedelta(minutes=1) + + self.assertFalse(self.elem.is_valid(self.certifier)) + + self.mock_certifier_pk.verify.assert_not_called() + ec.ECDSA.assert_not_called() + + @patch("admin.certificate_v2.ec") + @patch("admin.certificate_v2.x509.load_pem_x509_certificate") + def test_is_valid_signature_invalid(self, load_pem_x509_certificate, ec): + self.setup_is_valid_mocks(load_pem_x509_certificate, ec) + self.mock_certifier_pk.verify.side_effect = RuntimeError("wrong signature") + + self.assertFalse(self.elem.is_valid(self.certifier)) + + self.mock_certifier_pk.verify.assert_called_with( + "the-signature", + "the-fingerprint", + "the-ecdsa-algo" + ) + ec.ECDSA.assert_called_with("the-signature-hash-algo") + + @patch("admin.certificate_v2.ec") + @patch("admin.certificate_v2.x509.load_pem_x509_certificate") + def test_is_valid_x509_error(self, load_pem_x509_certificate, ec): + self.setup_is_valid_mocks(load_pem_x509_certificate, ec) + load_pem_x509_certificate.side_effect = ValueError("a random error") + + self.assertFalse(self.elem.is_valid(self.certifier)) + + self.mock_certifier_pk.verify.assert_not_called() + ec.ECDSA.assert_not_called() diff --git a/middleware/tests/admin/test_verify_sgx_attestation.py b/middleware/tests/admin/test_verify_sgx_attestation.py index 5bacadf5..cf980d9b 100644 --- a/middleware/tests/admin/test_verify_sgx_attestation.py +++ b/middleware/tests/admin/test_verify_sgx_attestation.py @@ -94,7 +94,9 @@ def setUp(self): def configure_mocks(self, get_root_of_trust, load_pubkeys, HSMCertificate, head): - get_root_of_trust.return_value = "the-root-of-trust" + self.root_of_trust = Mock() + self.root_of_trust.is_valid.return_value = True + get_root_of_trust.return_value = self.root_of_trust load_pubkeys.return_value = self.public_keys self.mock_certificate = Mock() self.mock_certificate.validate_and_get_values.return_value = self.validate_result @@ -116,10 +118,11 @@ def test_verify_attestation(self, get_root_of_trust, load_pubkeys, get_root_of_trust.assert_called_with(custom_root) else: get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values \ - .assert_called_with("the-root-of-trust") + .assert_called_with(self.root_of_trust) head.assert_called_with([ "powHSM verified with public keys:" ] + self.expected_pubkeys_output + [ @@ -135,6 +138,36 @@ def test_verify_attestation(self, get_root_of_trust, load_pubkeys, "Timestamp: 205", ], fill="-") + def test_verify_attestation_err_get_root(self, get_root_of_trust, load_pubkeys, + HSMCertificate, head, _): + self.configure_mocks(get_root_of_trust, load_pubkeys, HSMCertificate, head) + get_root_of_trust.side_effect = ValueError("root of trust error") + + with self.assertRaises(AdminError) as e: + do_verify_attestation(self.options) + self.assertIn("root of trust error", str(e.exception)) + + get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_not_called() + load_pubkeys.assert_not_called() + HSMCertificate.from_jsonfile.assert_not_called() + self.mock_certificate.validate_and_get_values.assert_not_called() + + def test_verify_attestation_err_root_invalid(self, get_root_of_trust, load_pubkeys, + HSMCertificate, head, _): + self.configure_mocks(get_root_of_trust, load_pubkeys, HSMCertificate, head) + self.root_of_trust.is_valid.return_value = False + + with self.assertRaises(AdminError) as e: + do_verify_attestation(self.options) + self.assertIn("self-signed root of trust", str(e.exception)) + + get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) + load_pubkeys.assert_not_called() + HSMCertificate.from_jsonfile.assert_not_called() + self.mock_certificate.validate_and_get_values.assert_not_called() + def test_verify_attestation_err_load_pubkeys(self, get_root_of_trust, load_pubkeys, HSMCertificate, head, _): self.configure_mocks(get_root_of_trust, load_pubkeys, HSMCertificate, head) @@ -145,6 +178,7 @@ def test_verify_attestation_err_load_pubkeys(self, get_root_of_trust, load_pubke self.assertIn("pubkeys error", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_not_called() self.mock_certificate.validate_and_get_values.assert_not_called() @@ -159,6 +193,7 @@ def test_verify_attestation_err_load_cert(self, get_root_of_trust, load_pubkeys, self.assertIn("load cert error", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values.assert_not_called() @@ -173,10 +208,11 @@ def test_verify_attestation_validation_noquote(self, get_root_of_trust, load_pub self.assertIn("does not contain", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values \ - .assert_called_with("the-root-of-trust") + .assert_called_with(self.root_of_trust) def test_verify_attestation_validation_failed(self, get_root_of_trust, load_pubkeys, HSMCertificate, head, _): @@ -190,10 +226,11 @@ def test_verify_attestation_validation_failed(self, get_root_of_trust, load_pubk self.assertIn("validation error", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values \ - .assert_called_with("the-root-of-trust") + .assert_called_with(self.root_of_trust) def test_verify_attestation_invalid_header(self, get_root_of_trust, load_pubkeys, HSMCertificate, head, _): @@ -205,10 +242,11 @@ def test_verify_attestation_invalid_header(self, get_root_of_trust, load_pubkeys self.assertIn("message header", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values \ - .assert_called_with("the-root-of-trust") + .assert_called_with(self.root_of_trust) def test_verify_attestation_invalid_message(self, get_root_of_trust, load_pubkeys, HSMCertificate, head, _): @@ -220,10 +258,11 @@ def test_verify_attestation_invalid_message(self, get_root_of_trust, load_pubkey self.assertIn("parsing", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values \ - .assert_called_with("the-root-of-trust") + .assert_called_with(self.root_of_trust) def test_verify_attestation_pkh_mismatch(self, get_root_of_trust, load_pubkeys, HSMCertificate, head, _): @@ -235,7 +274,8 @@ def test_verify_attestation_pkh_mismatch(self, get_root_of_trust, load_pubkeys, self.assertIn("hash mismatch", str(e.exception)) get_root_of_trust.assert_called_with(DEFAULT_ROOT_AUTHORITY) + self.root_of_trust.is_valid.assert_called_with(self.root_of_trust) load_pubkeys.assert_called_with(self.pubkeys_path) HSMCertificate.from_jsonfile.assert_called_with(self.certification_path) self.mock_certificate.validate_and_get_values \ - .assert_called_with("the-root-of-trust") + .assert_called_with(self.root_of_trust) From 0ec005317fc5761b9de120a315ab04f706650d3a Mon Sep 17 00:00:00 2001 From: Ariel Mendelzon Date: Tue, 7 Jan 2025 10:14:19 -0300 Subject: [PATCH 2/2] Changes as per PR review --- middleware/admin/certificate_v2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/middleware/admin/certificate_v2.py b/middleware/admin/certificate_v2.py index 54ca1f45..e0488d54 100644 --- a/middleware/admin/certificate_v2.py +++ b/middleware/admin/certificate_v2.py @@ -259,7 +259,7 @@ def is_valid(self, certifier): # IMPORTANT: for now, we only allow verifying the validity of an # HSMCertificateV2ElementX509 using another HSMCertificateV2ElementX509 # instance as certifier. That way, we simplify the validation procedure - # and ensure maximum compatibility wrt the underlying library used + # and ensure maximum use of the underlying library's capabilities # (cryptography) if not isinstance(certifier, type(self)): return False