diff --git a/Cargo.lock b/Cargo.lock index 75b5287dcbf..71eda71280f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9910,8 +9910,8 @@ dependencies = [ "ic-interfaces", "ic-test-utilities", "ic-types", - "maplit", "rand 0.8.5", + "thiserror", ] [[package]] diff --git a/rs/http_endpoints/public/src/read_state.rs b/rs/http_endpoints/public/src/read_state.rs index 8cffcef2a7c..e26beb8ea88 100644 --- a/rs/http_endpoints/public/src/read_state.rs +++ b/rs/http_endpoints/public/src/read_state.rs @@ -562,7 +562,7 @@ mod test { &sre, &user_test_id(1), &[Path::from(Label::from("time"))], - &CanisterIdSet::All, + &CanisterIdSet::all(), canister_test_id(1), ) .await, @@ -584,7 +584,7 @@ mod test { Label::from("reply") ]) ], - &CanisterIdSet::All, + &CanisterIdSet::all(), canister_test_id(1), ) .await, @@ -597,7 +597,7 @@ mod test { Path::new(vec![Label::from("request_status"), [0; 32].into()]), Path::new(vec![Label::from("request_status"), [1; 32].into()]) ], - &CanisterIdSet::All, + &CanisterIdSet::all(), canister_test_id(1), ) .await diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index 4bc77adc3d0..299eb2b439a 100644 --- a/rs/types/types/src/messages/http.rs +++ b/rs/types/types/src/messages/http.rs @@ -517,6 +517,10 @@ impl Delegation { } } } + + pub fn number_of_targets(&self) -> Option { + self.targets.as_ref().map(Vec::len) + } } impl SignedBytesWithoutDomainSeparator for Delegation { diff --git a/rs/validator/BUILD.bazel b/rs/validator/BUILD.bazel index 487a8b3cd6d..20b15c03edd 100644 --- a/rs/validator/BUILD.bazel +++ b/rs/validator/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") +load("@rules_rust//rust:defs.bzl", "rust_doc", "rust_doc_test", "rust_library", "rust_test") package(default_visibility = ["//visibility:public"]) @@ -11,6 +11,7 @@ DEPENDENCIES = [ "//rs/types/types", "@crate_index//:chrono", "@crate_index//:hex", + "@crate_index//:thiserror", ] DEV_DEPENDENCIES = [ @@ -19,7 +20,6 @@ DEV_DEPENDENCIES = [ "//rs/test_utilities", "@crate_index//:assert_matches", "@crate_index//:base64", - "@crate_index//:maplit", "@crate_index//:rand_0_8_4", ] @@ -46,3 +46,13 @@ rust_test( srcs = ["tests/ingress_validation.rs"], deps = DEPENDENCIES + DEV_DEPENDENCIES + [":validator"], ) + +rust_doc( + name = "validator_doc", + crate = ":validator", +) + +rust_doc_test( + name = "validator_doc_test", + crate = ":validator", +) diff --git a/rs/validator/Cargo.toml b/rs/validator/Cargo.toml index f50df7e5c71..a227d675cad 100644 --- a/rs/validator/Cargo.toml +++ b/rs/validator/Cargo.toml @@ -13,6 +13,7 @@ ic-interfaces = { path = "../interfaces" } ic-types = { path = "../types/types" } ic-crypto = { path = "../crypto" } ic-crypto-sha = { path = "../crypto/sha" } +thiserror = "1.0" [dev-dependencies] assert_matches = "1.3.0" @@ -21,7 +22,6 @@ hex = "0.4.2" ic-crypto-sha = { path = "../crypto/sha"} ic-crypto-test-utils-reproducible-rng = { path = "../crypto/test_utils/reproducible_rng" } ic-test-utilities = { path = "../test_utilities" } -maplit = "1.0.2" rand = "0.8" [features] diff --git a/rs/validator/ingress_message/src/internal/tests.rs b/rs/validator/ingress_message/src/internal/tests.rs index b42c9f2266f..f8046ad0ea9 100644 --- a/rs/validator/ingress_message/src/internal/tests.rs +++ b/rs/validator/ingress_message/src/internal/tests.rs @@ -379,6 +379,7 @@ mod validate_request { mod authenticated_requests_delegations { use super::*; use crate::AuthenticationError::DelegationContainsCyclesError; + use crate::AuthenticationError::DelegationTargetError; use crate::AuthenticationError::InvalidBasicSignature; use crate::AuthenticationError::InvalidCanisterSignature; use crate::HttpRequestVerifier; @@ -386,6 +387,7 @@ mod validate_request { use crate::RequestValidationError::InvalidDelegationExpiry; use crate::RequestValidationError::{CanisterNotInDelegationTargets, InvalidSignature}; use ic_crypto_test_utils_reproducible_rng::reproducible_rng; + use ic_types::messages::{HttpRequest, SignedIngressContent}; use ic_types::time::GENESIS; use ic_types::{CanisterId, Time}; use ic_validator_http_request_test_utils::{ @@ -395,6 +397,7 @@ mod validate_request { use std::time::Duration; const MAXIMUM_NUMBER_OF_DELEGATIONS: usize = 20; // !changing this number might be breaking!// + const MAXIMUM_NUMBER_OF_TARGETS: usize = 1_000; // !changing this number might be breaking!// const CURRENT_TIME: Time = GENESIS; #[test] @@ -854,6 +857,50 @@ mod validate_request { if public_key == duplicated_key_pair.public_key_der()) } + #[test] + fn should_fail_when_too_many_distinct_targets_in_delegation() { + let mut targets = Vec::with_capacity(MAXIMUM_NUMBER_OF_TARGETS + 1); + for i in 0..MAXIMUM_NUMBER_OF_TARGETS + 1 { + targets.push(CanisterId::from_u64(i as u64)) + } + let mut rng = reproducible_rng(); + let request = request_authenticated_by_delegation_with_targets(targets, &mut rng); + + let result = verifier_at_time(CURRENT_TIME).validate_request(&request); + + assert_matches!(result, Err(InvalidDelegation(DelegationTargetError(e))) if e.contains("expected at most 1000 targets")) + } + + #[test] + fn should_fail_when_too_many_same_targets_in_delegation() { + let mut targets = Vec::with_capacity(MAXIMUM_NUMBER_OF_TARGETS + 1); + for _ in 0..MAXIMUM_NUMBER_OF_TARGETS + 1 { + targets.push(CanisterId::from_u64(0_u64)) + } + let mut rng = reproducible_rng(); + let request = request_authenticated_by_delegation_with_targets(targets, &mut rng); + + let result = verifier_at_time(CURRENT_TIME).validate_request(&request); + + assert_matches!(result, Err(InvalidDelegation(DelegationTargetError(e))) if e.contains("expected at most 1000 targets")) + } + + fn request_authenticated_by_delegation_with_targets( + targets: Vec, + rng: &mut R, + ) -> HttpRequest { + assert!(!targets.is_empty()); + HttpRequestBuilder::default() + .with_ingress_expiry_at(CURRENT_TIME) + .with_canister_id(Blob(targets[0].get().to_vec())) + .with_authentication(AuthenticationScheme::Delegation( + DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_targets(random_user_key_pair(rng), CURRENT_TIME, targets) + .build(), + )) + .build() + } + fn delegation_chain_of_length( number_of_delegations: usize, delegation_expiration: Time, diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index 8e0bca3f45a..f5fe0eae210 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -14,6 +14,9 @@ use ic_types::{ }; use std::collections::HashSet; use std::{collections::BTreeSet, convert::TryFrom, fmt}; +use thiserror::Error; +use AuthenticationError::*; +use RequestValidationError::*; #[cfg(test)] mod tests; @@ -28,6 +31,13 @@ mod tests; /// see CRP-1961. const MAXIMUM_NUMBER_OF_DELEGATIONS: usize = 20; +/// Maximum number of targets (collection of `CanisterId`s) that can be specified in a +/// single delegation. Requests having a single delegation with more targets will be declared +/// invalid without any further verification. +/// **Note**: this limit part of the [IC specification](https://internetcomputer.org/docs/current/references/ic-interface-spec#authentication) +/// and so changing this value might be breaking or result in a deviation from the specification. +const MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION: usize = 1_000; + /// Validates the `request` and that the sender is authorized to send /// a message to the receiving canister. /// @@ -78,7 +88,7 @@ pub fn get_authorized_canisters( #[cfg(feature = "malicious_code")] { if malicious_flags.maliciously_disable_ingress_validation { - return Ok(CanisterIdSet::All); + return Ok(CanisterIdSet::all()); } } @@ -97,7 +107,7 @@ pub fn get_authorized_canisters( } /// Error in validating an [HttpRequest]. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum RequestValidationError { InvalidIngressExpiry(String), InvalidDelegationExpiry(String), @@ -136,7 +146,7 @@ impl fmt::Display for RequestValidationError { } /// Error in verifying the signature or authentication part of a request. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum AuthenticationError { InvalidBasicSignature(CryptoError), InvalidCanisterSignature(CryptoError), @@ -169,35 +179,138 @@ impl fmt::Display for AuthenticationError { } } -use AuthenticationError::*; -use RequestValidationError::*; +/// Set of canister IDs. +/// +/// It is guaranteed that the set contains at most [`MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION`] +/// elements. +/// Use [`CanisterIdSet::all`] to instantiate a set containing the entire domain of canister IDs +/// or [`CanisterIdSet::try_from_iter`] to instantiate a specific subset. +/// +/// # Examples +/// +/// ``` +/// # use ic_types::CanisterId; +/// # use ic_validator::CanisterIdSet; +/// let all_canister_ids = CanisterIdSet::all(); +/// assert!(all_canister_ids.contains(&CanisterId::from_u64(0))); +/// assert!(all_canister_ids.contains(&CanisterId::from_u64(1))); +/// assert!(all_canister_ids.contains(&CanisterId::from_u64(2))); +/// // ... +/// +/// let subset_canister_ids = CanisterIdSet::try_from_iter(vec![ +/// CanisterId::from_u64(0), +/// CanisterId::from_u64(1), +/// ]).expect("too many elements"); +/// assert!(subset_canister_ids.contains(&CanisterId::from_u64(0))); +/// assert!(subset_canister_ids.contains(&CanisterId::from_u64(1))); +/// assert!(!subset_canister_ids.contains(&CanisterId::from_u64(2))); +/// ``` +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct CanisterIdSet { + ids: internal::CanisterIdSet, +} -/// An enum representing a set of canister IDs. -#[derive(Debug, Eq, PartialEq)] -pub enum CanisterIdSet { - /// The entire domain of canister IDs. - All, - /// A subet of canister IDs. - Some(BTreeSet), +mod internal { + use super::*; + /// An enum representing a mutable set of canister IDs. + /// Contrary to `super::CanisterIdSet`, the number of canister IDs is not restricted. + #[derive(Clone, Debug, PartialEq, Eq, Hash)] + pub(super) enum CanisterIdSet { + /// The entire domain of canister IDs. + All, + /// A subset of canister IDs. + Some(BTreeSet), + } } impl CanisterIdSet { + pub fn all() -> Self { + CanisterIdSet { + ids: internal::CanisterIdSet::All, + } + } + + /// Constructs a specific subset of canister IDs from any collection that + /// can be iterated over. + /// + /// Duplicated elements in the input collection will be ignored. + /// + /// # Errors + /// + /// - [`CanisterIdSetInstantiationError::TooManyElements`] if the given iterator contains too many *distinct* elements + /// + /// # Examples + /// + /// ``` + /// # use ic_types::CanisterId; + /// # use ic_validator::{CanisterIdSet, CanisterIdSetInstantiationError}; + /// let empty_set = CanisterIdSet::try_from_iter(vec![]).expect("too many elements"); + /// let singleton = CanisterIdSet::try_from_iter(vec![CanisterId::from_u64(0)]).expect("too many elements"); + /// + /// let mut duplicated_ids = Vec::with_capacity(1001); + /// let mut distinct_ids = Vec::with_capacity(1001); + /// for i in 0..1001 { + /// duplicated_ids.push(CanisterId::from_u64(0)); + /// distinct_ids.push(CanisterId::from_u64(i)); + /// } + /// + /// assert_eq!(Ok(singleton), CanisterIdSet::try_from_iter(duplicated_ids)); + /// assert_eq!(Err(CanisterIdSetInstantiationError::TooManyElements(1001)), CanisterIdSet::try_from_iter(distinct_ids)); + /// ``` + pub fn try_from_iter>( + iter: I, + ) -> Result { + let ids: BTreeSet = iter.into_iter().collect(); + match ids.len() { + n if n > MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION => { + Err(CanisterIdSetInstantiationError::TooManyElements(n)) + } + _ => Ok(CanisterIdSet { + ids: internal::CanisterIdSet::Some(ids), + }), + } + } + pub fn contains(&self, canister_id: &CanisterId) -> bool { - match self { - Self::All => true, - Self::Some(c) => c.contains(canister_id), + match &self.ids { + internal::CanisterIdSet::All => true, + internal::CanisterIdSet::Some(set) => set.contains(canister_id), } } fn intersect(self, other: Self) -> Self { - match (self, other) { - (Self::All, other) => other, - (me, Self::All) => me, - (Self::Some(c1), Self::Some(c2)) => Self::Some(c1.intersection(&c2).cloned().collect()), + CanisterIdSet { + //the result of set intersection cannot contain + //more elements than the involved sets, + //so controlling the cardinality of the result is not needed + ids: match (self.ids, other.ids) { + (internal::CanisterIdSet::All, other) => other, + (me, internal::CanisterIdSet::All) => me, + (internal::CanisterIdSet::Some(set1), internal::CanisterIdSet::Some(set2)) => { + internal::CanisterIdSet::Some(set1.intersection(&set2).cloned().collect()) + } + }, + } + } + + #[cfg(test)] + fn is_empty(&self) -> bool { + match &self.ids { + internal::CanisterIdSet::All => false, + internal::CanisterIdSet::Some(set) => set.is_empty(), } } } +#[derive(Error, Clone, Debug, PartialEq, Eq, Hash)] +pub enum CanisterIdSetInstantiationError { + #[error( + "Expected at most {} elements but got {0}", + MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION + )] + TooManyElements(usize), +} + // Check if ingress_expiry is within a proper range with respect to the given // time, i.e., it is not expired yet and is not too far in the future. fn validate_ingress_expiry( @@ -372,8 +485,9 @@ fn validate_delegations( registry_version: RegistryVersion, ) -> Result<(Vec, CanisterIdSet), RequestValidationError> { ensure_delegations_does_not_contain_cycles(&pubkey, signed_delegations)?; + ensure_delegations_does_not_contain_too_many_targets(signed_delegations)?; // Initially, assume that the delegations target all possible canister IDs. - let mut targets = CanisterIdSet::All; + let mut targets = CanisterIdSet::all(); for sd in signed_delegations { let delegation = sd.delegation(); @@ -407,6 +521,25 @@ fn ensure_delegations_does_not_contain_cycles( Ok(()) } +fn ensure_delegations_does_not_contain_too_many_targets( + signed_delegations: &[SignedDelegation], +) -> Result<(), RequestValidationError> { + for delegation in signed_delegations { + match delegation.delegation().number_of_targets() { + Some(number_of_targets) + if number_of_targets > MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION => + { + Err(InvalidDelegation(DelegationTargetError(format!( + "expected at most {} targets per delegation, but got {}", + MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION, number_of_targets + )))) + } + _ => Ok(()), + }? + } + Ok(()) +} + fn validate_delegation( validator: &dyn IngressSigVerifier, signature: &[u8], @@ -442,8 +575,9 @@ fn validate_delegation( // Validation succeeded. Return the targets of this delegation. Ok(match delegation.targets().map_err(DelegationTargetError)? { - None => CanisterIdSet::All, - Some(targets) => CanisterIdSet::Some(targets), + None => CanisterIdSet::all(), + Some(targets) => CanisterIdSet::try_from_iter(targets) + .map_err(|e| DelegationTargetError(format!("{e}")))?, }) } @@ -459,7 +593,7 @@ fn validate_user_id_and_signature( match signature { None => { if sender.get().is_anonymous() { - return Ok(CanisterIdSet::All); + return Ok(CanisterIdSet::all()); } Err(MissingSignature(*sender)) } diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index 68c033b9947..2ade7da826d 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -7,7 +7,6 @@ use ic_types::{ messages::{Delegation, SignedDelegation, UserSignature}, time::UNIX_EPOCH, }; -use maplit::btreeset; use std::time::Duration; fn mock_registry_version() -> RegistryVersion { @@ -131,7 +130,7 @@ fn plain_authentication_with_one_delegation() { sender_delegation: Some(vec![signed_delegation]), }; - assert_matches!( + assert_eq!( validate_signature( &sig_verifier, &message_id, @@ -139,7 +138,7 @@ fn plain_authentication_with_one_delegation() { UNIX_EPOCH, mock_registry_version() ), - Ok(CanisterIdSet::All) + Ok(CanisterIdSet::all()) ); // Try verifying the signature in the future. It should fail because the @@ -207,7 +206,7 @@ fn plain_authentication_with_one_scoped_delegation() { UNIX_EPOCH, mock_registry_version() ), - Ok(CanisterIdSet::Some(set)) if set == btreeset! {canister_test_id(1)} + Ok(ids) if ids == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() ); } @@ -308,7 +307,7 @@ fn plain_authentication_with_multiple_delegations() { UNIX_EPOCH, mock_registry_version() ), - Ok(CanisterIdSet::Some(set)) if set == btreeset! {canister_test_id(1)} + Ok(ids) if ids == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() ); assert_matches!( validate_signature( @@ -434,7 +433,7 @@ fn validate_signature_webauthn() { sender_delegation: None, }; - assert_matches!( + assert_eq!( validate_signature( &sig_verifier, &message_id, @@ -442,7 +441,7 @@ fn validate_signature_webauthn() { UNIX_EPOCH, mock_registry_version() ), - Ok(CanisterIdSet::All) + Ok(CanisterIdSet::all()) ); } @@ -471,7 +470,7 @@ fn validate_signature_webauthn_with_delegations() { sender_delegation: Some(vec![SignedDelegation::new(delegation, delegation_sig)]), }; - assert_matches!( + assert_eq!( validate_signature( &sig_verifier, &message_id, @@ -479,7 +478,7 @@ fn validate_signature_webauthn_with_delegations() { UNIX_EPOCH, mock_registry_version() ), - Ok(CanisterIdSet::All) + Ok(CanisterIdSet::all()) ); } @@ -563,27 +562,57 @@ mod canister_id_set { use rand::CryptoRng; use rand::Rng; - // HTTP requests are limited to 2MB. - // A canister ID contains 29 bytes and so there are - // at most 72_315 Canister IDs in an HTTP request. - const MAXIMAL_NUMBER_OF_CANISTER_IDS_PER_HTTP_REQUEST: u32 = 72_315; + #[test] + fn should_contain_expected_elements() { + let number_of_ids = MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION; + let mut ids = Vec::with_capacity(number_of_ids); + for i in 0..number_of_ids { + ids.push(CanisterId::from_u64(i as u64)); + } + + let set = CanisterIdSet::try_from_iter(ids.clone()).expect("too many elements"); + + for id in ids { + assert!(set.contains(&id)) + } + } + + #[test] + fn should_not_store_duplicated_canister_ids() { + let id0 = CanisterId::from_u64(0); + let id1 = CanisterId::from_u64(1); + let duplicated_ids = vec![id0, id1, id0]; + let set = CanisterIdSet::try_from_iter(duplicated_ids).expect("too many elements"); + + assert!(set.contains(&id0)); + assert!(set.contains(&id1)); + assert_matches!(set.ids, internal::CanisterIdSet::Some(subset) if subset.len() == 2); + } - //TODO CRP-1965: limit number of targets per delegation #[test] fn should_efficiently_intersect_large_canister_id_sets() { let mut rng = reproducible_rng(); - let number_of_ids = MAXIMAL_NUMBER_OF_CANISTER_IDS_PER_HTTP_REQUEST; - let mut first_set = BTreeSet::new(); - let mut second_set = BTreeSet::new(); - for _ in 1..=number_of_ids { - assert!(first_set.insert(random_canister_id(&mut rng))); - assert!(second_set.insert(random_canister_id(&mut rng))); - } - - let intersection = - CanisterIdSet::Some(first_set).intersect(CanisterIdSet::Some(second_set)); + let number_of_ids = MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION; + let (first_canister_ids, second_canister_ids) = { + let mut first_set = BTreeSet::new(); + let mut second_set = BTreeSet::new(); + for _ in 1..=number_of_ids { + assert!(first_set.insert(random_canister_id(&mut rng))); + assert!(second_set.insert(random_canister_id(&mut rng))); + } + ( + CanisterIdSet::try_from_iter(first_set).expect("too many elements"), + CanisterIdSet::try_from_iter(second_set).expect("too many elements"), + ) + }; + + let intersection = first_canister_ids.intersect(second_canister_ids); // Probability of collision is negligible (around 10^(-60)). - assert_matches!(intersection, CanisterIdSet::Some(set) if set.is_empty()) + assert!( + intersection.is_empty(), + "expected {:?} to be empty but was not", + intersection + ) } fn random_canister_id(rng: &mut R) -> CanisterId { diff --git a/rs/validator/src/lib.rs b/rs/validator/src/lib.rs index 452ad4ee9bb..39323d3c0f9 100644 --- a/rs/validator/src/lib.rs +++ b/rs/validator/src/lib.rs @@ -9,5 +9,5 @@ mod webauthn; pub use ingress_validation::{ get_authorized_canisters, validate_request, AuthenticationError, CanisterIdSet, - RequestValidationError, + CanisterIdSetInstantiationError, RequestValidationError, };