From f69337035b29a0eb05010cb9c853908601f1cea2 Mon Sep 17 00:00:00 2001 From: NathanosDev Date: Tue, 19 Dec 2023 13:05:01 +0100 Subject: [PATCH] fix: disallow nested delegations when verifying certificates --- .../src/certificate_verification.rs | 39 ++++- .../ic-certificate-verification/src/error.rs | 4 + .../src/certificate_builder.rs | 135 +++++++++++++----- .../ic-certification-testing/src/error.rs | 3 + 4 files changed, 144 insertions(+), 37 deletions(-) diff --git a/packages/ic-certificate-verification/src/certificate_verification.rs b/packages/ic-certificate-verification/src/certificate_verification.rs index c07b1b53..061eed21 100644 --- a/packages/ic-certificate-verification/src/certificate_verification.rs +++ b/packages/ic-certificate-verification/src/certificate_verification.rs @@ -79,6 +79,9 @@ impl VerifyCertificate> for Delegation { root_public_key: &[u8], ) -> CertificateVerificationResult> { let cert: Certificate = Certificate::from_cbor(&self.certificate)?; + if cert.delegation.is_some() { + return Err(CertificateVerificationError::CertificateHasTooManyDelegations); + } cert.verify(canister_id, root_public_key)?; let canister_range_path = [ @@ -158,11 +161,6 @@ pub fn validate_certificate_time( #[cfg(test)] mod tests { - use std::{ - ops::{Add, Sub}, - time::{Duration, SystemTime}, - }; - use super::*; use ic_cbor::CertificateToCbor; use ic_certification::Certificate; @@ -170,6 +168,10 @@ mod tests { use ic_response_verification_test_utils::{ create_canister_id, get_current_timestamp, get_timestamp, AssetTree, }; + use std::{ + ops::{Add, Sub}, + time::{Duration, SystemTime}, + }; static CANISTER_ID: &str = "r7inp-6aaaa-aaaaa-aaabq-cai"; const MAX_CERT_TIME_OFFSET_NS: u128 = 300_000_000_000; // 5 min @@ -194,6 +196,33 @@ mod tests { certificate.verify(canister_id.as_ref(), &root_key).unwrap(); } + #[test] + fn verify_certificate_with_nested_delegation_should_fail() { + let canister_id = create_canister_id(CANISTER_ID); + let CertificateData { + cbor_encoded_certificate, + certificate: _, + root_key, + } = CertificateBuilder::new( + &canister_id.to_string(), + &AssetTree::new().get_certified_data(), + ) + .unwrap() + .with_delegation(123, vec![(0, 9)]) + .with_nested_delegation(456, vec![(20, 19)]) + .build() + .unwrap(); + + let certificate = Certificate::from_cbor(&cbor_encoded_certificate).unwrap(); + + let result = certificate.verify(canister_id.as_ref(), &root_key); + + assert!(matches!( + result.err(), + Some(CertificateVerificationError::CertificateHasTooManyDelegations), + )) + } + #[test] fn verify_certificate_should_fail() { let canister_id = create_canister_id(CANISTER_ID); diff --git a/packages/ic-certificate-verification/src/error.rs b/packages/ic-certificate-verification/src/error.rs index 4c7b7678..27745eb6 100644 --- a/packages/ic-certificate-verification/src/error.rs +++ b/packages/ic-certificate-verification/src/error.rs @@ -87,4 +87,8 @@ pub enum CertificateVerificationError { /// Failed to decode CBOR #[error("CBOR decoding failed")] CborDecodingFailed(#[from] CborError), + + /// The certificate contained more than one delegation. + #[error("The certificate contained more than one delegation")] + CertificateHasTooManyDelegations, } diff --git a/packages/ic-certification-testing/src/certificate_builder.rs b/packages/ic-certification-testing/src/certificate_builder.rs index 1a80c524..ea4e1cea 100644 --- a/packages/ic-certification-testing/src/certificate_builder.rs +++ b/packages/ic-certification-testing/src/certificate_builder.rs @@ -48,6 +48,7 @@ pub struct CertificateBuilder { time: Option, canister: Option, subnet: Option, + nested_subnet: Option, signature: Option, custom_tree: Option>>, } @@ -67,6 +68,7 @@ impl CertificateBuilder { certified_data: certified_data.to_vec(), }), subnet: None, + nested_subnet: None, signature: None, custom_tree: None, }) @@ -77,6 +79,7 @@ impl CertificateBuilder { time: None, canister: None, subnet: None, + nested_subnet: None, signature: None, custom_tree: Some(custom_tree), } @@ -87,17 +90,17 @@ impl CertificateBuilder { subnet_id: u64, canister_id_ranges: Vec<(u64, u64)>, ) -> &mut Self { - let canister_id_ranges = canister_id_ranges - .into_iter() - .map(|(low, high)| (CanisterId::from_u64(low), CanisterId::from_u64(high))) - .collect(); + self.subnet = Some(create_subnet_data(subnet_id, canister_id_ranges)); - let subnet_id = SubnetId::from(PrincipalId::new_subnet_test_id(subnet_id)); + self + } - self.subnet = Some(SubnetData { - subnet_id, - canister_id_ranges, - }); + pub fn with_nested_delegation( + &mut self, + subnet_id: u64, + canister_id_ranges: Vec<(u64, u64)>, + ) -> &mut Self { + self.nested_subnet = Some(create_subnet_data(subnet_id, canister_id_ranges)); self } @@ -134,10 +137,25 @@ impl CertificateBuilder { })?; let (keypair, tree, signature) = build_certificate(&tree)?; - let delegation = None; - let delegation_data = self.build_delegation(&keypair, &encoded_time)?; let signature = self.signature.as_ref().unwrap_or(&signature); + let nested_delegation_data = self.build_nested_delegation(&keypair, &encoded_time)?; + if let Some((delegation, keypair)) = nested_delegation_data { + let certificate = Certificate { + tree, + signature: signature.clone(), + delegation: Some(delegation), + }; + let certificate_cbor = serialize_to_cbor(&certificate); + + return Ok(CertificateData { + certificate, + root_key: keypair.public_key, + cbor_encoded_certificate: certificate_cbor, + }); + } + + let delegation_data = self.build_delegation(&keypair, &encoded_time)?; if let Some((delegation, keypair)) = delegation_data { let certificate = Certificate { tree, @@ -156,7 +174,7 @@ impl CertificateBuilder { let certificate = Certificate { tree, signature: signature.clone(), - delegation, + delegation: None, }; let certificate_cbor = serialize_to_cbor(&certificate); @@ -173,31 +191,84 @@ impl CertificateBuilder { delegatee_keypair: &KeyPair, encoded_time: &[u8], ) -> CertificationTestResult> { - if let Some(subnet) = &self.subnet { - let tree = create_delegation_tree( - &delegatee_keypair.public_key, - encoded_time, - &subnet.subnet_id, - &subnet.canister_id_ranges, - )?; - let (keypair, tree, signature) = build_certificate(&tree)?; - let certificate = Certificate { - tree, - signature, - delegation: None, - }; + if let Some(subnet_data) = &self.subnet { + let delegation_data = + create_delegation_data(delegatee_keypair, encoded_time, subnet_data, None)?; - return Ok(Some(( - CertificateDelegation { - certificate: Blob(serialize_to_cbor(&certificate)), - subnet_id: Blob(subnet.subnet_id.get().to_vec()), - }, - keypair, - ))); + return Ok(Some(delegation_data)); } Ok(None) } + + fn build_nested_delegation( + &self, + delegatee_keypair: &KeyPair, + encoded_time: &[u8], + ) -> CertificationTestResult> { + match (&self.subnet, &self.nested_subnet) { + (Some(subnet_data), Some(nested_subnet_data)) => { + let (nested_delegation, nested_keypair) = create_delegation_data( + delegatee_keypair, + encoded_time, + nested_subnet_data, + None, + )?; + + let (delegation, _keypair) = create_delegation_data( + &nested_keypair, + encoded_time, + subnet_data, + Some(nested_delegation), + )?; + + return Ok(Some((delegation, nested_keypair))); + } + (_, _) => Err(CertificationTestError::NestedDelegationMissing), + } + } +} + +fn create_delegation_data( + delegatee_keypair: &KeyPair, + encoded_time: &[u8], + subnet_data: &SubnetData, + nested_delegation: Option, +) -> CertificationTestResult<(CertificateDelegation, KeyPair)> { + let tree = create_delegation_tree( + &delegatee_keypair.public_key, + encoded_time, + &subnet_data.subnet_id, + &subnet_data.canister_id_ranges, + )?; + let (keypair, tree, signature) = build_certificate(&tree)?; + let certificate = Certificate { + tree, + signature, + delegation: nested_delegation, + }; + + Ok(( + CertificateDelegation { + certificate: Blob(serialize_to_cbor(&certificate)), + subnet_id: Blob(subnet_data.subnet_id.get().to_vec()), + }, + keypair, + )) +} + +fn create_subnet_data(subnet_id: u64, canister_id_ranges: Vec<(u64, u64)>) -> SubnetData { + let canister_id_ranges = canister_id_ranges + .into_iter() + .map(|(low, high)| (CanisterId::from_u64(low), CanisterId::from_u64(high))) + .collect(); + + let subnet_id = SubnetId::from(PrincipalId::new_subnet_test_id(subnet_id)); + + SubnetData { + subnet_id, + canister_id_ranges, + } } fn build_certificate( diff --git a/packages/ic-certification-testing/src/error.rs b/packages/ic-certification-testing/src/error.rs index 8e8b7a8b..28bf9ca8 100644 --- a/packages/ic-certification-testing/src/error.rs +++ b/packages/ic-certification-testing/src/error.rs @@ -35,6 +35,9 @@ pub enum CertificationTestError { #[error("failed to merge witnesses")] WitnessMergingFailed, + + #[error("a nested delegation was request but either the main delegation is missing, or the nested delegation")] + NestedDelegationMissing } impl From for CertificationTestError {