From 6e0edf31f415c3b2e96085b1c7d8b1de1d355edc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 20:58:05 +0000 Subject: [PATCH 1/9] feat: reject update calls for delegations with queries-only permissions Adds an optional permissions field to request delegations, mirroring the existing targets field. When a delegation in a request's delegation chain carries permissions = "queries", the ingress validator rejects update calls authenticated through that chain with the new UpdateCallNotPermittedByDelegation error, while query and read_state requests remain permitted. Delegations with permissions = "updates" or without the field are unrestricted, and any other value is rejected as unsupported (fail-closed, since no pre-existing delegations carry the field). The field is covered by the delegation signature (it is part of the representation-independent hash), so a client cannot strip it to lift the restriction: removing the field changes the delegation hash and invalidates the signature. For the same reason, replicas that predate this change fail closed on restricted delegations. This enables issuers like Internet Identity to sign read-only delegations that can read on the user's behalf (perform query calls) but cannot change state (perform update calls), enforced by the protocol regardless of the client's cooperation. It is the strictly-enforced counterpart to the sender_info-based attribute approach (#10447). https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/types/types/src/messages/http.rs | 21 +++ rs/types/types/src/messages/http/tests.rs | 23 +++ .../http_request_test_utils/src/lib.rs | 30 ++++ .../ingress_message/src/internal/mod.rs | 6 + rs/validator/ingress_message/src/lib.rs | 16 +++ .../ingress_message/tests/validate_request.rs | 131 ++++++++++++++++++ rs/validator/src/ingress_validation.rs | 105 +++++++++++--- rs/validator/src/ingress_validation/tests.rs | 20 +-- rs/validator/tests/ingress_validation.rs | 34 +++++ 9 files changed, 354 insertions(+), 32 deletions(-) diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index add56b40d4ff..a5fd6fad8a35 100644 --- a/rs/types/types/src/messages/http.rs +++ b/rs/types/types/src/messages/http.rs @@ -544,6 +544,7 @@ pub struct Delegation { pubkey: Blob, expiration: Time, targets: Option>, + permissions: Option, } impl Delegation { @@ -552,6 +553,7 @@ impl Delegation { pubkey: Blob(pubkey), expiration, targets: None, + permissions: None, } } @@ -560,6 +562,16 @@ impl Delegation { pubkey: Blob(pubkey), expiration, targets: Some(targets.iter().map(|c| Blob(c.get().to_vec())).collect()), + permissions: None, + } + } + + pub fn new_with_permissions(pubkey: Vec, expiration: Time, permissions: String) -> Self { + Self { + pubkey: Blob(pubkey), + expiration, + targets: None, + permissions: Some(permissions), } } @@ -590,6 +602,12 @@ impl Delegation { pub fn number_of_targets(&self) -> Option { self.targets.as_ref().map(Vec::len) } + + /// The kinds of calls the delegation permits, if restricted. + /// `None` means the delegation is unrestricted. + pub fn permissions(&self) -> Option<&str> { + self.permissions.as_deref() + } } impl SignedBytesWithoutDomainSeparator for Delegation { @@ -606,6 +624,9 @@ impl SignedBytesWithoutDomainSeparator for Delegation { Array(targets.iter().map(|t| Bytes(t.0.as_slice())).collect()), ); } + if let Some(permissions) = &self.permissions { + map.insert("permissions", String(permissions)); + } bytes.extend_from_slice(&hash_of_map(&map, |key, value| hash_key_val(key, value))); } diff --git a/rs/types/types/src/messages/http/tests.rs b/rs/types/types/src/messages/http/tests.rs index 61ab6f583103..39cb3fce57ea 100644 --- a/rs/types/types/src/messages/http/tests.rs +++ b/rs/types/types/src/messages/http/tests.rs @@ -18,6 +18,7 @@ mod targets { invalid_canister_id, to_blob(CanisterId::from(3)), ]), + permissions: None, }; let targets = delegation.targets(); @@ -33,6 +34,7 @@ mod targets { let delegation = Delegation { pubkey: Blob(vec![]), expiration: CURRENT_TIME, + permissions: None, targets: Some(vec![ to_blob(canister_id_3), to_blob(canister_id_3), @@ -914,11 +916,13 @@ mod cbor_serialization { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: None, + permissions: None, }, Value::Map(btreemap! { text("pubkey") => bytes(&[1, 2, 3]), text("expiration") => int(0), text("targets") => Value::Null, + text("permissions") => Value::Null, }), ); @@ -927,11 +931,28 @@ mod cbor_serialization { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: Some(vec![Blob(vec![4, 5, 6])]), + permissions: None, }, Value::Map(btreemap! { text("pubkey") => bytes(&[1, 2, 3]), text("expiration") => int(0), text("targets") => Value::Array(vec![bytes(&[4, 5, 6])]), + text("permissions") => Value::Null, + }), + ); + + assert_cbor_ser_equal( + &Delegation { + pubkey: Blob(vec![1, 2, 3]), + expiration: UNIX_EPOCH, + targets: None, + permissions: Some("queries".to_string()), + }, + Value::Map(btreemap! { + text("pubkey") => bytes(&[1, 2, 3]), + text("expiration") => int(0), + text("targets") => Value::Null, + text("permissions") => text("queries"), }), ); } @@ -944,6 +965,7 @@ mod cbor_serialization { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: None, + permissions: None, }, signature: Blob(vec![4, 5, 6]), }, @@ -952,6 +974,7 @@ mod cbor_serialization { text("pubkey") => bytes(&[1, 2, 3]), text("expiration") => int(0), text("targets") => Value::Null, + text("permissions") => Value::Null, }), text("signature") => bytes(&[4, 5, 6]), }), diff --git a/rs/validator/http_request_test_utils/src/lib.rs b/rs/validator/http_request_test_utils/src/lib.rs index b4711573498e..226c16238cdd 100644 --- a/rs/validator/http_request_test_utils/src/lib.rs +++ b/rs/validator/http_request_test_utils/src/lib.rs @@ -531,6 +531,23 @@ impl DirectAuthenticationScheme { let signature = self.sign(&delegation); SignedDelegation::new(delegation, signature) } + + /// Creates a delegation that restricts the kinds of calls the delegate + /// may make (e.g., `"queries"`). + fn delegate_to_with_permissions( + &self, + other: &DirectAuthenticationScheme, + expiration: Time, + permissions: &str, + ) -> SignedDelegation { + let delegation = Delegation::new_with_permissions( + other.public_key_der(), + expiration, + permissions.to_string(), + ); + let signature = self.sign(&delegation); + SignedDelegation::new(delegation, signature) + } } #[derive(Clone, Eq, PartialEq, Debug)] @@ -595,6 +612,19 @@ impl DelegationChainBuilder { self } + pub fn delegate_to_with_permissions( + mut self, + new_end: DirectAuthenticationScheme, + expiration: Time, + permissions: &str, + ) -> Self { + let current_end = self.end.unwrap_or_else(|| self.start.clone()); + self.signed_delegations + .push(current_end.delegate_to_with_permissions(&new_end, expiration, permissions)); + self.end = Some(new_end); + self + } + pub fn change_last_delegation SignedDelegationBuilder>( mut self, change: F, diff --git a/rs/validator/ingress_message/src/internal/mod.rs b/rs/validator/ingress_message/src/internal/mod.rs index 5c1286db28e0..fb9ce129e432 100644 --- a/rs/validator/ingress_message/src/internal/mod.rs +++ b/rs/validator/ingress_message/src/internal/mod.rs @@ -208,6 +208,9 @@ fn to_validation_error(error: ic_validator::RequestValidationError) -> RequestVa ic_validator::RequestValidationError::InvalidSenderInfo(msg) => { RequestValidationError::InvalidSenderInfo(msg) } + ic_validator::RequestValidationError::UpdateCallNotPermittedByDelegation => { + RequestValidationError::UpdateCallNotPermittedByDelegation + } } } fn to_authentication_lib_error(error: ic_validator::AuthenticationError) -> AuthenticationError { @@ -233,6 +236,9 @@ fn to_authentication_lib_error(error: ic_validator::AuthenticationError) -> Auth ic_validator::AuthenticationError::DelegationContainsCyclesError { public_key } => { AuthenticationError::DelegationContainsCyclesError { public_key } } + ic_validator::AuthenticationError::UnsupportedDelegationPermissions(permissions) => { + AuthenticationError::UnsupportedDelegationPermissions(permissions) + } } } diff --git a/rs/validator/ingress_message/src/lib.rs b/rs/validator/ingress_message/src/lib.rs index e3080a118bed..1dede333f987 100644 --- a/rs/validator/ingress_message/src/lib.rs +++ b/rs/validator/ingress_message/src/lib.rs @@ -46,6 +46,8 @@ pub use internal::TimeProvider; /// * [`RequestValidationError::CanisterNotInDelegationTargets`]: if the request targets a canister /// that is not authorized in one of the delegations. /// * [`RequestValidationError::InvalidSenderInfo`]: if sender info is provided but invalid. +/// * [`RequestValidationError::UpdateCallNotPermittedByDelegation`]: if the request is an +/// update call but a delegation restricts the sender to query calls. pub trait HttpRequestVerifier { fn validate_request(&self, request: &HttpRequest) -> Result<(), RequestValidationError>; } @@ -65,6 +67,7 @@ pub enum RequestValidationError { PathTooLongError { length: usize, maximum: usize }, NonceTooBigError { num_bytes: usize, maximum: usize }, InvalidSenderInfo(String), + UpdateCallNotPermittedByDelegation, } impl Display for RequestValidationError { @@ -112,6 +115,13 @@ impl Display for RequestValidationError { RequestValidationError::InvalidSenderInfo(msg) => { write!(f, "Invalid sender info: {msg}") } + RequestValidationError::UpdateCallNotPermittedByDelegation => { + write!( + f, + "Update calls are not permitted: a delegation restricts \ + the sender to query calls (permissions = \"queries\")" + ) + } } } } @@ -142,6 +152,9 @@ pub enum AuthenticationError { /// which was already encountered before in the chain of delegations. /// Note that if both keys are equal, then this delegation is self-signed, which is also forbidden. DelegationContainsCyclesError { public_key: Vec }, + + /// A delegation's `permissions` field holds an unsupported value. + UnsupportedDelegationPermissions(String), } impl Display for AuthenticationError { @@ -165,6 +178,9 @@ impl Display for AuthenticationError { "Chain of delegations contains at least one cycle: first repeating public key encountered {}", hex::encode(public_key) ), + AuthenticationError::UnsupportedDelegationPermissions(permissions) => { + write!(f, "Unsupported delegation permissions: {permissions}") + } } } } diff --git a/rs/validator/ingress_message/tests/validate_request.rs b/rs/validator/ingress_message/tests/validate_request.rs index 6c3348be9c01..e5238f34b45b 100644 --- a/rs/validator/ingress_message/tests/validate_request.rs +++ b/rs/validator/ingress_message/tests/validate_request.rs @@ -2175,6 +2175,137 @@ mod sender_info { } } +mod delegation_permissions { + use super::*; + use ic_validator_http_request_test_utils::DelegationChain; + use ic_validator_ingress_message::AuthenticationError::UnsupportedDelegationPermissions; + + #[test] + fn should_reject_update_call_when_delegation_restricts_to_queries() { + let rng = &mut reproducible_rng(); + let verifier = verifier_at_time(CURRENT_TIME).build(); + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .build(); + + let request = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + + assert_matches!( + verifier.validate_request(&request), + Err(RequestValidationError::UpdateCallNotPermittedByDelegation) + ); + } + + #[test] + fn should_accept_query_and_read_state_when_delegation_restricts_to_queries() { + let rng = &mut reproducible_rng(); + let verifier = verifier_at_time(CURRENT_TIME).build(); + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .build(); + + let query = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_eq!(verifier.validate_request(&query), Ok(())); + + let read_state = HttpRequestBuilder::new_read_state() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + assert_eq!(verifier.validate_request(&read_state), Ok(())); + } + + #[test] + fn should_accept_update_call_when_delegation_permissions_are_updates() { + let rng = &mut reproducible_rng(); + let verifier = verifier_at_time(CURRENT_TIME).build(); + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "updates") + .build(); + + let request = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + + assert_eq!(verifier.validate_request(&request), Ok(())); + } + + #[test] + fn should_reject_all_request_types_when_delegation_permissions_unsupported() { + let rng = &mut reproducible_rng(); + let verifier = verifier_at_time(CURRENT_TIME).build(); + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "writes") + .build(); + + let update = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_matches!( + verifier.validate_request(&update), + Err(RequestValidationError::InvalidDelegation( + UnsupportedDelegationPermissions(permissions) + )) if permissions == "writes" + ); + + let query = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_matches!( + verifier.validate_request(&query), + Err(RequestValidationError::InvalidDelegation( + UnsupportedDelegationPermissions(_) + )) + ); + + let read_state = HttpRequestBuilder::new_read_state() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + assert_matches!( + verifier.validate_request(&read_state), + Err(RequestValidationError::InvalidDelegation( + UnsupportedDelegationPermissions(_) + )) + ); + } + + #[test] + fn should_reject_update_call_when_any_delegation_in_chain_restricts_to_queries() { + let rng = &mut reproducible_rng(); + let verifier = verifier_at_time(CURRENT_TIME).build(); + // The restriction sits in the middle of the chain; subsequent + // unrestricted delegations must not lift it. + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .delegate_to(random_user_key_pair(rng), CURRENT_TIME) + .build(); + + let update = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_matches!( + verifier.validate_request(&update), + Err(RequestValidationError::UpdateCallNotPermittedByDelegation) + ); + + let query = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + assert_eq!(verifier.validate_request(&query), Ok(())); + } +} + fn default_verifier() -> IngressMessageVerifierBuilder { IngressMessageVerifier::builder().with_time_provider(TimeProvider::Constant(CURRENT_TIME)) } diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index 337c1672be1b..e759fe6460d8 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -60,6 +60,33 @@ const MAXIMUM_NUMBER_OF_PATHS: usize = 1_000; /// and so changing this value might be breaking or result in a deviation from the specification. const MAXIMUM_NUMBER_OF_LABELS_PER_PATH: usize = 127; +/// Value of a delegation's `permissions` field that restricts the sender +/// to query calls: requests to `/call` endpoints are rejected. +const DELEGATION_PERMISSIONS_QUERIES: &str = "queries"; + +/// Value of a delegation's `permissions` field that explicitly permits all +/// kinds of calls (same as an absent `permissions` field). +const DELEGATION_PERMISSIONS_UPDATES: &str = "updates"; + +/// Restrictions that a chain of delegations places on the sender. +#[derive(Debug)] +struct DelegationRestrictions { + /// The set of canister IDs that are common to all delegations' `targets`. + targets: CanisterIdSet, + /// Whether some delegation in the chain restricts the sender to query + /// calls via the `permissions` field. + queries_only: bool, +} + +impl DelegationRestrictions { + fn unrestricted() -> Self { + Self { + targets: CanisterIdSet::all(), + queries_only: false, + } + } +} + /// A trait for validating an `HttpRequest` with content `C`. pub trait HttpRequestVerifier: Send + Sync { /// Validates the given request. @@ -76,6 +103,9 @@ pub trait HttpRequestVerifier: Send + Sync { /// * The request's signature (if any) is correct. /// * If the request specifies a `CanisterId` (see `HasCanisterId`), /// then it must be among the set of canister IDs that are common to all delegations. + /// * Every delegation's `permissions` field (if any) holds a supported value + /// (`"queries"` or `"updates"`). If the request is an update call, no delegation + /// restricts the sender to query calls (`permissions = "queries"`). /// /// The following signatures (for signing the request or any delegation) are supported /// (see the [IC specification](https://internetcomputer.org/docs/current/references/ic-interface-spec#signatures)): @@ -115,14 +145,17 @@ where root_of_trust_provider: &R, ) -> Result { validate_ingress_expiry(request, current_time)?; - let delegation_targets = validate_request_content( + let restrictions = validate_request_content( request, self.validator.as_ref(), current_time, root_of_trust_provider, )?; - validate_request_target(request, &delegation_targets)?; - Ok(delegation_targets) + if restrictions.queries_only { + return Err(UpdateCallNotPermittedByDelegation); + } + validate_request_target(request, &restrictions.targets)?; + Ok(restrictions.targets) } } @@ -140,14 +173,16 @@ where if !request.sender().get().is_anonymous() { validate_ingress_expiry(request, current_time)?; } - let delegation_targets = validate_request_content( + // A delegation with `permissions = "queries"` does not restrict + // query calls. + let restrictions = validate_request_content( request, self.validator.as_ref(), current_time, root_of_trust_provider, )?; - validate_request_target(request, &delegation_targets)?; - Ok(delegation_targets) + validate_request_target(request, &restrictions.targets)?; + Ok(restrictions.targets) } } @@ -166,12 +201,15 @@ where if !request.sender().get().is_anonymous() { validate_ingress_expiry(request, current_time)?; } + // A delegation with `permissions = "queries"` does not restrict + // read_state requests. validate_request_content( request, self.validator.as_ref(), current_time, root_of_trust_provider, ) + .map(|restrictions| restrictions.targets) } } @@ -198,14 +236,14 @@ fn validate_request_content( ingress_signature_verifier: &dyn IngressSigVerifier, current_time: Time, root_of_trust_provider: &R, -) -> Result +) -> Result where R::Error: std::error::Error, { validate_nonce(request)?; // Validate the envelope signature first (cheap check) before performing // expensive canister signature verification in validate_sender_info. - let targets = validate_user_id_and_signature( + let restrictions = validate_user_id_and_signature( ingress_signature_verifier, &request.sender(), &request.id(), @@ -217,7 +255,7 @@ where root_of_trust_provider, )?; validate_sender_info(request, ingress_signature_verifier, root_of_trust_provider)?; - Ok(targets) + Ok(restrictions) } fn validate_request_target( @@ -266,6 +304,11 @@ pub enum RequestValidationError { NonceTooBig { num_bytes: usize, maximum: usize }, #[error("Invalid sender info: {0}")] InvalidSenderInfo(String), + #[error( + "Update calls are not permitted: a delegation restricts the sender \ + to query calls (permissions = \"queries\")." + )] + UpdateCallNotPermittedByDelegation, } /// Error in verifying the signature or authentication part of a request. @@ -285,6 +328,8 @@ pub enum AuthenticationError { DelegationTooLongError { length: usize, maximum: usize }, #[error("Chain of delegations contains at least one cycle: first repeating public key encountered {}", hex::encode(.public_key))] DelegationContainsCyclesError { public_key: Vec }, + #[error("Unsupported delegation permissions: {0}")] + UnsupportedDelegationPermissions(String), } /// Set of canister IDs. @@ -638,7 +683,7 @@ fn validate_signature( signature: &UserSignature, current_time: Time, root_of_trust_provider: &R, -) -> Result +) -> Result where R::Error: std::error::Error, { @@ -647,7 +692,7 @@ where let empty_vec = Vec::new(); let signed_delegations = signature.sender_delegation.as_ref().unwrap_or(&empty_vec); - let (pubkey, targets) = validate_delegations( + let (pubkey, restrictions) = validate_delegations( validator, signed_delegations.as_slice(), signature.signer_pubkey.clone(), @@ -666,7 +711,7 @@ where validate_webauthn_sig(validator, &webauthn_sig, message_id, &pk) .map_err(WebAuthnError) .map_err(InvalidSignature)?; - Ok(targets) + Ok(restrictions) } KeyBytesContentType::Ed25519PublicKeyDer | KeyBytesContentType::EcdsaP256PublicKeyDer @@ -674,7 +719,7 @@ where let basic_sig = BasicSigOf::from(BasicSig(signature.signature.clone())); validate_signature_plain(validator, message_id, &basic_sig, &pk) .map_err(InvalidSignature)?; - Ok(targets) + Ok(restrictions) } KeyBytesContentType::IcCanisterSignatureAlgPublicKeyDer => { let canister_sig = CanisterSigOf::from(CanisterSig(signature.signature.clone())); @@ -689,7 +734,7 @@ where e.to_string() )) ); - Ok(targets) + Ok(restrictions) } KeyBytesContentType::RsaSha256PublicKeyDer => { Err(RequestValidationError::InvalidSignature( @@ -717,20 +762,23 @@ fn validate_signature_plain( // See https://internetcomputer.org/docs/current/references/ic-interface-spec#authentication // // If the delegations are valid, returns the public key used to sign the -// request as well as the set of canister IDs that the public key is valid for. +// request as well as the restrictions that the delegations place on the +// sender (the set of canister IDs that the public key is valid for, and +// whether the sender is restricted to query calls). fn validate_delegations( validator: &dyn IngressSigVerifier, signed_delegations: &[SignedDelegation], mut pubkey: Vec, root_of_trust_provider: &R, -) -> Result<(Vec, CanisterIdSet), RequestValidationError> +) -> Result<(Vec, DelegationRestrictions), RequestValidationError> where R::Error: std::error::Error, { 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(); + // Initially, assume that the delegations place no restrictions on the + // sender. + let mut restrictions = DelegationRestrictions::unrestricted(); for sd in signed_delegations { let delegation = sd.delegation(); @@ -745,11 +793,24 @@ where ) .map_err(InvalidDelegation)?; // Restrict the canister targets to the ones specified in the delegation. - targets = targets.intersect(new_targets); + restrictions.targets = restrictions.targets.intersect(new_targets); + // Restrict the kinds of calls to the ones permitted by the delegation. + // Unsupported values are rejected (fail-closed): no delegations with a + // `permissions` field exist that predate this validation, so there is + // no backward compatibility concern. + match delegation.permissions() { + None | Some(DELEGATION_PERMISSIONS_UPDATES) => {} + Some(DELEGATION_PERMISSIONS_QUERIES) => restrictions.queries_only = true, + Some(unsupported) => { + return Err(InvalidDelegation(UnsupportedDelegationPermissions( + unsupported.to_string(), + ))); + } + } pubkey = delegation.pubkey().to_vec(); } - Ok((pubkey, targets)) + Ok((pubkey, restrictions)) } fn ensure_delegations_does_not_contain_cycles( @@ -846,14 +907,14 @@ fn validate_user_id_and_signature( signature: Option<&UserSignature>, current_time: Time, root_of_trust_provider: &R, -) -> Result +) -> Result where R::Error: std::error::Error, { match signature { None => { if sender.get().is_anonymous() { - return Ok(CanisterIdSet::all()); + return Ok(DelegationRestrictions::unrestricted()); } Err(MissingSignature(*sender)) } diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index 20371401d30d..badf4773e640 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -131,7 +131,7 @@ fn plain_authentication_with_one_delegation() { sender_delegation: Some(vec![signed_delegation]), }; - assert_eq!( + assert_matches!( validate_signature( &sig_verifier, &message_id, @@ -139,7 +139,7 @@ fn plain_authentication_with_one_delegation() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(CanisterIdSet::all()) + Ok(restrictions) if restrictions.targets == CanisterIdSet::all() && !restrictions.queries_only ); // Try verifying the signature in the future. It should fail because the @@ -207,7 +207,7 @@ fn plain_authentication_with_one_scoped_delegation() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(ids) if ids == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() + Ok(restrictions) if restrictions.targets == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() ); } @@ -308,7 +308,7 @@ fn plain_authentication_with_multiple_delegations() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(ids) if ids == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() + Ok(restrictions) if restrictions.targets == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() ); assert_matches!( validate_signature( @@ -434,7 +434,7 @@ fn validate_signature_webauthn() { sender_delegation: None, }; - assert_eq!( + assert_matches!( validate_signature( &sig_verifier, &message_id, @@ -442,7 +442,7 @@ fn validate_signature_webauthn() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(CanisterIdSet::all()) + Ok(restrictions) if restrictions.targets == CanisterIdSet::all() && !restrictions.queries_only ); } @@ -467,7 +467,7 @@ fn validate_signature_webauthn_ed25519() { sender_delegation: None, }; - assert_eq!( + assert_matches!( validate_signature( &sig_verifier, &message_id, @@ -475,7 +475,7 @@ fn validate_signature_webauthn_ed25519() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(CanisterIdSet::all()) + Ok(restrictions) if restrictions.targets == CanisterIdSet::all() && !restrictions.queries_only ); } @@ -504,7 +504,7 @@ fn validate_signature_webauthn_with_delegations() { sender_delegation: Some(vec![SignedDelegation::new(delegation, delegation_sig)]), }; - assert_eq!( + assert_matches!( validate_signature( &sig_verifier, &message_id, @@ -512,7 +512,7 @@ fn validate_signature_webauthn_with_delegations() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(CanisterIdSet::all()) + Ok(restrictions) if restrictions.targets == CanisterIdSet::all() && !restrictions.queries_only ); } diff --git a/rs/validator/tests/ingress_validation.rs b/rs/validator/tests/ingress_validation.rs index 78fde532c0de..25be133444f9 100644 --- a/rs/validator/tests/ingress_validation.rs +++ b/rs/validator/tests/ingress_validation.rs @@ -70,3 +70,37 @@ fn delegation_with_targets_signed_bytes() { assert_eq!(d.as_signed_bytes(), expected_signed_bytes); } + +#[test] +fn delegation_with_permissions_signed_bytes() { + let d = Delegation::new_with_permissions(vec![1, 2, 3], UNIX_EPOCH, "queries".to_string()); + + let mut expected_signed_bytes = Vec::new(); + expected_signed_bytes.extend_from_slice(b"\x1Aic-request-auth-delegation"); + + // Representation-independent hash of the delegation. + let mut pubkey_hash = Vec::new(); + pubkey_hash.extend_from_slice(&Sha256::hash(b"pubkey")); + pubkey_hash.extend_from_slice(&Sha256::hash(&[1, 2, 3])); + + let mut expiration_hash = Vec::new(); + expiration_hash.extend_from_slice(&Sha256::hash(b"expiration")); + expiration_hash.extend_from_slice(&Sha256::hash(&[0])); + + let mut permissions_hash = Vec::new(); + permissions_hash.extend_from_slice(&Sha256::hash(b"permissions")); + permissions_hash.extend_from_slice(&Sha256::hash(b"queries")); + + let mut hashes: Vec> = vec![pubkey_hash, expiration_hash, permissions_hash]; + hashes.sort(); + + let mut hasher = Sha256::new(); + for hash in hashes { + hasher.write(&hash); + } + + // Concatenate domain with representation-independent hash. + expected_signed_bytes.extend_from_slice(&hasher.finish()); + + assert_eq!(d.as_signed_bytes(), expected_signed_bytes); +} From 89d766cb683c043ef2e45ec52b9b7e817a280453 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 21:05:01 +0000 Subject: [PATCH 2/9] refactor: address review comments Short-circuit the queries-only delegation rejection before sender_info canister signature verification on the call path, make the error message byte-identical across the validator crates, and strengthen test assertions to also check queries_only. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/validator/src/ingress_validation.rs | 18 +++++++++++++++--- rs/validator/src/ingress_validation/tests.rs | 4 ++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index e759fe6460d8..4959f7f89c18 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -145,15 +145,27 @@ where root_of_trust_provider: &R, ) -> Result { validate_ingress_expiry(request, current_time)?; - let restrictions = validate_request_content( - request, + validate_nonce(request)?; + let restrictions = validate_user_id_and_signature( self.validator.as_ref(), + &request.sender(), + &request.id(), + match request.authentication() { + Authentication::Anonymous => None, + Authentication::Authenticated(signature) => Some(signature), + }, current_time, root_of_trust_provider, )?; + // Reject update calls that a delegation restricts to query calls + // before performing potentially expensive canister signature + // verification of the sender info: such calls are rejected either + // way, so the delegation-based rejection takes precedence over an + // invalid sender info. if restrictions.queries_only { return Err(UpdateCallNotPermittedByDelegation); } + validate_sender_info(request, self.validator.as_ref(), root_of_trust_provider)?; validate_request_target(request, &restrictions.targets)?; Ok(restrictions.targets) } @@ -306,7 +318,7 @@ pub enum RequestValidationError { InvalidSenderInfo(String), #[error( "Update calls are not permitted: a delegation restricts the sender \ - to query calls (permissions = \"queries\")." + to query calls (permissions = \"queries\")" )] UpdateCallNotPermittedByDelegation, } diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index badf4773e640..f1d0822a7a9a 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -207,7 +207,7 @@ fn plain_authentication_with_one_scoped_delegation() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(restrictions) if restrictions.targets == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() + Ok(restrictions) if restrictions.targets == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() && !restrictions.queries_only ); } @@ -308,7 +308,7 @@ fn plain_authentication_with_multiple_delegations() { UNIX_EPOCH, &MockRootOfTrustProvider::new() ), - Ok(restrictions) if restrictions.targets == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() + Ok(restrictions) if restrictions.targets == CanisterIdSet::try_from_iter(vec![canister_test_id(1)]).unwrap() && !restrictions.queries_only ); assert_matches!( validate_signature( From 5445d218d4bc207089e1d08cc2ad3aef583c5e71 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 12 Jun 2026 12:40:33 +0000 Subject: [PATCH 3/9] refactor: rename unrestricted delegation permissions value to "all" The supported values of a delegation's permissions field are now: - "queries": the sender can only execute queries; requests to /call endpoints (updates and replicated queries) are rejected. - "all": the sender can execute all functions (queries, replicated queries, and updates); same as an absent permissions field. Any other value, including the previous "updates", is rejected as unsupported. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- .../ingress_message/tests/validate_request.rs | 75 ++++++++++--------- rs/validator/src/ingress_validation.rs | 12 +-- 2 files changed, 47 insertions(+), 40 deletions(-) diff --git a/rs/validator/ingress_message/tests/validate_request.rs b/rs/validator/ingress_message/tests/validate_request.rs index e5238f34b45b..545d724e6b6a 100644 --- a/rs/validator/ingress_message/tests/validate_request.rs +++ b/rs/validator/ingress_message/tests/validate_request.rs @@ -2221,11 +2221,11 @@ mod delegation_permissions { } #[test] - fn should_accept_update_call_when_delegation_permissions_are_updates() { + fn should_accept_update_call_when_delegation_permissions_are_all() { let rng = &mut reproducible_rng(); let verifier = verifier_at_time(CURRENT_TIME).build(); let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "updates") + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "all") .build(); let request = HttpRequestBuilder::new_update_call() @@ -2240,42 +2240,47 @@ mod delegation_permissions { fn should_reject_all_request_types_when_delegation_permissions_unsupported() { let rng = &mut reproducible_rng(); let verifier = verifier_at_time(CURRENT_TIME).build(); - let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "writes") - .build(); + // Note that "updates" is deliberately unsupported: the supported + // vocabulary is "queries" (queries only) and "all" (queries, + // replicated queries, and updates). + for unsupported in ["writes", "updates", ""] { + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, unsupported) + .build(); - let update = HttpRequestBuilder::new_update_call() - .with_ingress_expiry_at(CURRENT_TIME) - .with_authentication(AuthenticationScheme::Delegation(chain.clone())) - .build(); - assert_matches!( - verifier.validate_request(&update), - Err(RequestValidationError::InvalidDelegation( - UnsupportedDelegationPermissions(permissions) - )) if permissions == "writes" - ); + let update = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_matches!( + verifier.validate_request(&update), + Err(RequestValidationError::InvalidDelegation( + UnsupportedDelegationPermissions(permissions) + )) if permissions == unsupported + ); - let query = HttpRequestBuilder::new_query() - .with_ingress_expiry_at(CURRENT_TIME) - .with_authentication(AuthenticationScheme::Delegation(chain.clone())) - .build(); - assert_matches!( - verifier.validate_request(&query), - Err(RequestValidationError::InvalidDelegation( - UnsupportedDelegationPermissions(_) - )) - ); + let query = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_matches!( + verifier.validate_request(&query), + Err(RequestValidationError::InvalidDelegation( + UnsupportedDelegationPermissions(_) + )) + ); - let read_state = HttpRequestBuilder::new_read_state() - .with_ingress_expiry_at(CURRENT_TIME) - .with_authentication(AuthenticationScheme::Delegation(chain)) - .build(); - assert_matches!( - verifier.validate_request(&read_state), - Err(RequestValidationError::InvalidDelegation( - UnsupportedDelegationPermissions(_) - )) - ); + let read_state = HttpRequestBuilder::new_read_state() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + assert_matches!( + verifier.validate_request(&read_state), + Err(RequestValidationError::InvalidDelegation( + UnsupportedDelegationPermissions(_) + )) + ); + } } #[test] diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index 4959f7f89c18..0adb83e6ecb7 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -61,12 +61,14 @@ const MAXIMUM_NUMBER_OF_PATHS: usize = 1_000; const MAXIMUM_NUMBER_OF_LABELS_PER_PATH: usize = 127; /// Value of a delegation's `permissions` field that restricts the sender -/// to query calls: requests to `/call` endpoints are rejected. +/// to query calls: requests to `/call` endpoints (i.e., updates and +/// replicated queries) are rejected. const DELEGATION_PERMISSIONS_QUERIES: &str = "queries"; /// Value of a delegation's `permissions` field that explicitly permits all -/// kinds of calls (same as an absent `permissions` field). -const DELEGATION_PERMISSIONS_UPDATES: &str = "updates"; +/// kinds of calls — queries, replicated queries, and updates (same as an +/// absent `permissions` field). +const DELEGATION_PERMISSIONS_ALL: &str = "all"; /// Restrictions that a chain of delegations places on the sender. #[derive(Debug)] @@ -104,7 +106,7 @@ pub trait HttpRequestVerifier: Send + Sync { /// * If the request specifies a `CanisterId` (see `HasCanisterId`), /// then it must be among the set of canister IDs that are common to all delegations. /// * Every delegation's `permissions` field (if any) holds a supported value - /// (`"queries"` or `"updates"`). If the request is an update call, no delegation + /// (`"queries"` or `"all"`). If the request is an update call, no delegation /// restricts the sender to query calls (`permissions = "queries"`). /// /// The following signatures (for signing the request or any delegation) are supported @@ -811,7 +813,7 @@ where // `permissions` field exist that predate this validation, so there is // no backward compatibility concern. match delegation.permissions() { - None | Some(DELEGATION_PERMISSIONS_UPDATES) => {} + None | Some(DELEGATION_PERMISSIONS_ALL) => {} Some(DELEGATION_PERMISSIONS_QUERIES) => restrictions.queries_only = true, Some(unsupported) => { return Err(InvalidDelegation(UnsupportedDelegationPermissions( From 3bc932ee31acbee5b325858c62b1eec72837fe4f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 10:52:06 +0000 Subject: [PATCH 4/9] test: broaden delegation permissions test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the test coverage identified as missing: 1. System test (rs/tests/crypto/ingress_verification_test.rs): requests_with_delegation_permissions exercises the full HTTP path against a real replica — "queries" rejects update calls (across all /call API versions) while permitting query and read_state, "all" is accepted, and unsupported values (incl. "updates") are rejected for all request kinds; covers the whole-chain semantics. 2. CBOR wire round-trip (ic-types http tests): confirms the permissions field survives serialize/deserialize and that a delegation encoded without the field deserializes to None (backward compatibility). 3. Combined targets + permissions CBOR encoding case. 4. Lower-level validate_delegations unit tests (ic-validator) asserting queries_only is set for "queries", unset for "all"/absent, and that unsupported values (incl. "updates") yield UnsupportedDelegationPermissions. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/tests/crypto/ingress_verification_test.rs | 212 +++++++++++++++++++ rs/types/types/src/messages/http/tests.rs | 51 +++++ rs/validator/src/ingress_validation/tests.rs | 87 ++++++++ 3 files changed, 350 insertions(+) diff --git a/rs/tests/crypto/ingress_verification_test.rs b/rs/tests/crypto/ingress_verification_test.rs index c35c0701c7a0..2dfe2388a8f4 100644 --- a/rs/tests/crypto/ingress_verification_test.rs +++ b/rs/tests/crypto/ingress_verification_test.rs @@ -43,6 +43,7 @@ fn main() -> Result<()> { SystemTestSubGroup::new() .add_test(systest!(requests_with_delegations)) .add_test(systest!(requests_with_delegations_with_targets)) + .add_test(systest!(requests_with_delegation_permissions)) .add_test(systest!(requests_with_delegation_loop)) .add_test(systest!(requests_to_mgmt_canister_with_delegations)) .add_test(systest!(requests_with_sender_info)) @@ -677,6 +678,190 @@ pub fn requests_with_delegations_with_targets(env: TestEnv) { }); } +// Tests for the optional `permissions` field on delegations: `"queries"` +// restricts the sender to query calls and read_state requests (update calls +// are rejected), `"all"` (and an absent field) permits everything, and any +// other value invalidates the delegation for all request kinds. +pub fn requests_with_delegation_permissions(env: TestEnv) { + let logger = env.logger(); + let node = env.get_first_healthy_node_snapshot(); + let agent = node.build_default_agent(); + let rng = &mut reproducible_rng(); + block_on({ + async move { + let node_url = node.get_public_url(); + debug!(logger, "Selected replica"; "url" => format!("{}", node_url)); + + let canister = + UniversalCanister::new_with_retries(&agent, node.effective_canister_id(), &logger) + .await; + let test_info = TestInformation { + url: node_url, + canister_id: canister_id_from_principal(&canister.canister_id()), + }; + + // Expected outcome for a delegation chain carrying some + // `permissions` values. + enum Outcome { + /// Query and read_state succeed; update calls are rejected + /// with an error whose text contains the given substring. + QueriesOnly { update_err: &'static str }, + /// All request kinds succeed. + All, + /// All request kinds are rejected with an error whose text + /// contains the given substring. + Invalid { err: &'static str }, + } + + struct PermissionsTest { + note: &'static str, + // The `permissions` value of each delegation in the chain + // (`None` means the field is omitted for that delegation). + permissions: Vec>, + outcome: Outcome, + } + + let update_not_permitted = "Update calls are not permitted"; + let unsupported = "Unsupported delegation permissions"; + + let scenarios = [ + PermissionsTest { + note: "single delegation restricted to queries", + permissions: vec![Some("queries")], + outcome: Outcome::QueriesOnly { + update_err: update_not_permitted, + }, + }, + PermissionsTest { + note: "single delegation permitting all", + permissions: vec![Some("all")], + outcome: Outcome::All, + }, + PermissionsTest { + note: "queries restriction in the first of two delegations", + permissions: vec![Some("queries"), None], + outcome: Outcome::QueriesOnly { + update_err: update_not_permitted, + }, + }, + PermissionsTest { + note: "queries restriction in the second of two delegations", + permissions: vec![Some("all"), Some("queries")], + outcome: Outcome::QueriesOnly { + update_err: update_not_permitted, + }, + }, + PermissionsTest { + note: "unsupported value \"writes\"", + permissions: vec![Some("writes")], + outcome: Outcome::Invalid { err: unsupported }, + }, + PermissionsTest { + note: "unsupported value \"updates\"", + permissions: vec![Some("updates")], + outcome: Outcome::Invalid { err: unsupported }, + }, + ]; + + for scenario in &scenarios { + let delegation_count = scenario.permissions.len(); + let mut identities = Vec::with_capacity(delegation_count + 1); + for _ in 0..=delegation_count { + let id_type = GenericIdentityType::random_incl_canister(&canister, rng); + identities.push(GenericIdentity::new(id_type, rng)); + } + + let delegations = + create_delegations_with_permissions(&identities, &scenario.permissions); + let sender = &identities[0]; + let signer = &identities[identities.len() - 1]; + + for &api_ver in ALL_QUERY_API_VERSIONS { + let response = perform_query_call_with_delegations( + api_ver, + &test_info, + sender, + signer, + &delegations, + ) + .await; + match scenario.outcome { + Outcome::QueriesOnly { .. } | Outcome::All => { + response.expect_query_ok(api_ver) + } + Outcome::Invalid { err } => { + assert_eq!( + response.status(), + 400, + "Scenario {} (query) using v{api_ver} unexpectedly succeeded", + scenario.note + ); + response.expect_text_error(err); + } + } + } + + for &api_ver in ALL_READ_STATE_API_VERSIONS { + let response = perform_read_state_call_with_delegations( + api_ver, + &test_info, + sender, + signer, + &delegations, + ) + .await; + match scenario.outcome { + Outcome::QueriesOnly { .. } | Outcome::All => { + response.expect_read_state_ok(api_ver) + } + Outcome::Invalid { err } => { + assert_eq!( + response.status(), + 400, + "Scenario {} (read_state) using v{api_ver} unexpectedly succeeded", + scenario.note + ); + response.expect_text_error(err); + } + } + } + + for &api_ver in ALL_UPDATE_API_VERSIONS { + let response = perform_update_call_with_delegations( + api_ver, + &test_info, + sender, + signer, + &delegations, + ) + .await; + match scenario.outcome { + Outcome::All => response.expect_update_ok(api_ver), + Outcome::QueriesOnly { update_err } => { + assert_eq!( + response.status(), + 400, + "Scenario {} (update) using v{api_ver} unexpectedly succeeded", + scenario.note + ); + response.expect_text_error(update_err); + } + Outcome::Invalid { err } => { + assert_eq!( + response.status(), + 400, + "Scenario {} (update) using v{api_ver} unexpectedly succeeded", + scenario.note + ); + response.expect_text_error(err); + } + } + } + } + } + }); +} + // Tests for handling of delegation loops pub fn requests_with_delegation_loop(env: TestEnv) { let logger = env.logger(); @@ -1579,6 +1764,33 @@ fn create_delegations_with_targets( delegations } +fn create_delegations_with_permissions( + identities: &[GenericIdentity], + permissions: &[Option<&str>], +) -> Vec { + let delegation_expiry = Time::from_nanos_since_unix_epoch(expiry_time().as_nanos() as u64); + + let delegation_count = identities.len() - 1; + let mut delegations = Vec::with_capacity(delegation_count); + + for i in 1..=delegation_count { + let delegation = match permissions[i - 1] { + Some(permissions) => Delegation::new_with_permissions( + identities[i].public_key_der(), + delegation_expiry, + permissions.to_string(), + ), + None => Delegation::new(identities[i].public_key_der(), delegation_expiry), + }; + + let signed_delegation = sign_delegation(delegation, &identities[i - 1]); + + delegations.push(signed_delegation); + } + + delegations +} + fn random_canister_id(rng: &mut R) -> CanisterId { CanisterId::from_u64(rng.r#gen::()) } diff --git a/rs/types/types/src/messages/http/tests.rs b/rs/types/types/src/messages/http/tests.rs index 39cb3fce57ea..acad1eb75db8 100644 --- a/rs/types/types/src/messages/http/tests.rs +++ b/rs/types/types/src/messages/http/tests.rs @@ -955,6 +955,57 @@ mod cbor_serialization { text("permissions") => text("queries"), }), ); + + // A delegation may carry both `targets` and `permissions`. + assert_cbor_ser_equal( + &Delegation { + pubkey: Blob(vec![1, 2, 3]), + expiration: UNIX_EPOCH, + targets: Some(vec![Blob(vec![4, 5, 6])]), + permissions: Some("queries".to_string()), + }, + Value::Map(btreemap! { + text("pubkey") => bytes(&[1, 2, 3]), + text("expiration") => int(0), + text("targets") => Value::Array(vec![bytes(&[4, 5, 6])]), + text("permissions") => text("queries"), + }), + ); + } + + /// Verifies that the optional `permissions` field survives a full CBOR + /// wire round-trip (serialize to bytes, deserialize back), since + /// requests carry delegations as CBOR on the wire. An absent field must + /// deserialize to `None` so that delegations predating this field remain + /// parseable. + #[test] + fn delegation_permissions_cbor_round_trip() { + use crate::crypto::Signable; + + for permissions in [None, Some("queries".to_string()), Some("all".to_string())] { + let delegation = Delegation { + pubkey: Blob(vec![1, 2, 3]), + expiration: UNIX_EPOCH, + targets: None, + permissions: permissions.clone(), + }; + let bytes = serde_cbor::to_vec(&delegation).unwrap(); + let decoded: Delegation = serde_cbor::from_slice(&bytes).unwrap(); + assert_eq!(decoded.permissions, permissions); + // The signed bytes (which include the permissions field) must + // also match, i.e. the round-trip preserves what gets signed. + assert_eq!(decoded.as_signed_bytes(), delegation.as_signed_bytes()); + } + + // A delegation encoded without the `permissions` key (as produced by + // implementations predating the field) deserializes to `None`. + let without_permissions = Value::Map(btreemap! { + text("pubkey") => bytes(&[1, 2, 3]), + text("expiration") => int(0), + }); + let decoded: Delegation = serde_cbor::value::from_value(without_permissions).unwrap(); + assert_eq!(decoded.permissions, None); + assert_eq!(decoded.targets, None); } #[test] diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index f1d0822a7a9a..9b3bb4a192f3 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -211,6 +211,93 @@ fn plain_authentication_with_one_scoped_delegation() { ); } +mod delegation_permissions { + use super::*; + use ic_types::crypto::Signable; + + /// Signs `delegation` with `signer` and the message id with `sender_sk`, + /// then validates the single-delegation chain `signer -> sender_sk`. + /// Uses ECDSA secp256r1 identities (supported by the validator and + /// signable in-process, unlike the hard-coded ed25519 signatures used by + /// the surrounding tests). + fn validate_single_delegation( + permissions: Option<&str>, + ) -> Result { + let sig_verifier = temp_crypto_component_with_fake_registry(node_test_id(0)); + let message_id = message_test_id(1); + let rng = &mut ic_crypto_test_utils_reproducible_rng::reproducible_rng(); + + let signer_sk = ic_secp256r1::PrivateKey::generate_using_rng(rng); + let session_sk = ic_secp256r1::PrivateKey::generate_using_rng(rng); + let signer_pk = signer_sk.public_key().serialize_der(); + let session_pk = session_sk.public_key().serialize_der(); + + let delegation = match permissions { + Some(permissions) => { + Delegation::new_with_permissions(session_pk, UNIX_EPOCH, permissions.to_string()) + } + None => Delegation::new(session_pk, UNIX_EPOCH), + }; + let delegation_sig = signer_sk + .sign_message(&delegation.as_signed_bytes()) + .to_vec(); + let signed_delegation = SignedDelegation::new(delegation, delegation_sig); + + let user_signature = UserSignature { + signature: session_sk + .sign_message(&message_id.as_signed_bytes()) + .to_vec(), + signer_pubkey: signer_pk, + sender_delegation: Some(vec![signed_delegation]), + }; + + validate_signature( + &sig_verifier, + &message_id, + &user_signature, + UNIX_EPOCH, + &MockRootOfTrustProvider::new(), + ) + } + + #[test] + fn queries_permission_sets_queries_only() { + assert_matches!( + validate_single_delegation(Some("queries")), + Ok(restrictions) if restrictions.queries_only + ); + } + + #[test] + fn all_permission_does_not_set_queries_only() { + assert_matches!( + validate_single_delegation(Some("all")), + Ok(restrictions) if !restrictions.queries_only + ); + } + + #[test] + fn absent_permission_does_not_set_queries_only() { + assert_matches!( + validate_single_delegation(None), + Ok(restrictions) if !restrictions.queries_only + ); + } + + #[test] + fn unsupported_permission_is_rejected() { + assert_matches!( + validate_single_delegation(Some("writes")), + Err(InvalidDelegation(UnsupportedDelegationPermissions(value))) if value == "writes" + ); + // The previously-used "updates" value is now unsupported. + assert_matches!( + validate_single_delegation(Some("updates")), + Err(InvalidDelegation(UnsupportedDelegationPermissions(value))) if value == "updates" + ); + } +} + #[test] fn plain_authentication_with_multiple_delegations() { let sig_verifier = temp_crypto_component_with_fake_registry(node_test_id(0)); From 5c5d8503baa13c511fae697b343403ae5c54f00b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 13:00:17 +0000 Subject: [PATCH 5/9] refactor: address PR review comments - Replace Delegation::new_with_targets/new_with_permissions with chainable with_targets/with_permissions builder methods, so a delegation can carry both targets and permissions. - Remove the call-path duplication of validate_request_content: the queries-only rejection now happens after the shared validation (the earlier short-circuit-before-sender_info did not actually prevent DoS, since the query/read_state endpoints validate sender_info too). - Clarify docs: an update call or replicated query submitted to /call is rejected by a queries-only delegation. - Tests: add case/whitespace unsupported-permission variants; add an e2e test combining delegate_to_with_targets and delegate_to_with_permissions in one chain; reword the misleading "previously-used" comment. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- .../request_auth_malicious_replica_test.rs | 4 +- rs/tests/crypto/ingress_verification_test.rs | 33 +++------- rs/types/types/src/messages/http.rs | 28 ++++----- .../http_request_test_utils/src/lib.rs | 11 ++-- rs/validator/ingress_message/src/lib.rs | 4 +- .../ingress_message/tests/validate_request.rs | 63 +++++++++++++++++-- rs/validator/src/ingress_validation.rs | 24 +++---- rs/validator/src/ingress_validation/tests.rs | 21 +++---- rs/validator/tests/ingress_validation.rs | 4 +- 9 files changed, 107 insertions(+), 85 deletions(-) diff --git a/rs/tests/consensus/request_auth_malicious_replica_test.rs b/rs/tests/consensus/request_auth_malicious_replica_test.rs index 0eb607ecc8da..42f19d9ff8a3 100644 --- a/rs/tests/consensus/request_auth_malicious_replica_test.rs +++ b/rs/tests/consensus/request_auth_malicious_replica_test.rs @@ -549,9 +549,11 @@ async fn test_request_with_delegation( // A delegation from identity to identity2 for the specific canister ID. let delegation = match delegation_targets { Some(targets) => { - Delegation::new_with_targets( + Delegation::new( signature.public_key.clone().unwrap(), // public key of identity2 Time::from_nanos_since_unix_epoch(delegation_expiry), + ) + .with_targets( targets .into_iter() .map(|principal| CanisterId::try_from(principal.as_slice()).unwrap()) diff --git a/rs/tests/crypto/ingress_verification_test.rs b/rs/tests/crypto/ingress_verification_test.rs index 2dfe2388a8f4..45ab3b55aa2f 100644 --- a/rs/tests/crypto/ingress_verification_test.rs +++ b/rs/tests/crypto/ingress_verification_test.rs @@ -705,7 +705,7 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { enum Outcome { /// Query and read_state succeed; update calls are rejected /// with an error whose text contains the given substring. - QueriesOnly { update_err: &'static str }, + QueriesOnly { err: &'static str }, /// All request kinds succeed. All, /// All request kinds are rejected with an error whose text @@ -729,7 +729,7 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { note: "single delegation restricted to queries", permissions: vec![Some("queries")], outcome: Outcome::QueriesOnly { - update_err: update_not_permitted, + err: update_not_permitted, }, }, PermissionsTest { @@ -741,14 +741,14 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { note: "queries restriction in the first of two delegations", permissions: vec![Some("queries"), None], outcome: Outcome::QueriesOnly { - update_err: update_not_permitted, + err: update_not_permitted, }, }, PermissionsTest { note: "queries restriction in the second of two delegations", permissions: vec![Some("all"), Some("queries")], outcome: Outcome::QueriesOnly { - update_err: update_not_permitted, + err: update_not_permitted, }, }, PermissionsTest { @@ -837,16 +837,7 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { .await; match scenario.outcome { Outcome::All => response.expect_update_ok(api_ver), - Outcome::QueriesOnly { update_err } => { - assert_eq!( - response.status(), - 400, - "Scenario {} (update) using v{api_ver} unexpectedly succeeded", - scenario.note - ); - response.expect_text_error(update_err); - } - Outcome::Invalid { err } => { + Outcome::QueriesOnly { err } | Outcome::Invalid { err } => { assert_eq!( response.status(), 400, @@ -1748,11 +1739,8 @@ fn create_delegations_with_targets( let delegation = if targets[i - 1].is_empty() { Delegation::new(identities[i].public_key_der(), delegation_expiry) } else { - Delegation::new_with_targets( - identities[i].public_key_der(), - delegation_expiry, - targets[i - 1].clone(), - ) + Delegation::new(identities[i].public_key_der(), delegation_expiry) + .with_targets(targets[i - 1].clone()) }; let signed_delegation = sign_delegation(delegation, &identities[i - 1]); @@ -1775,11 +1763,8 @@ fn create_delegations_with_permissions( for i in 1..=delegation_count { let delegation = match permissions[i - 1] { - Some(permissions) => Delegation::new_with_permissions( - identities[i].public_key_der(), - delegation_expiry, - permissions.to_string(), - ), + Some(permissions) => Delegation::new(identities[i].public_key_der(), delegation_expiry) + .with_permissions(permissions.to_string()), None => Delegation::new(identities[i].public_key_der(), delegation_expiry), }; diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index a5fd6fad8a35..0f395ee1eeac 100644 --- a/rs/types/types/src/messages/http.rs +++ b/rs/types/types/src/messages/http.rs @@ -557,22 +557,18 @@ impl Delegation { } } - pub fn new_with_targets(pubkey: Vec, expiration: Time, targets: Vec) -> Self { - Self { - pubkey: Blob(pubkey), - expiration, - targets: Some(targets.iter().map(|c| Blob(c.get().to_vec())).collect()), - permissions: None, - } - } - - pub fn new_with_permissions(pubkey: Vec, expiration: Time, permissions: String) -> Self { - Self { - pubkey: Blob(pubkey), - expiration, - targets: None, - permissions: Some(permissions), - } + /// Restricts the delegation to the given canister targets. Chainable + /// with [`Self::with_permissions`] to build a delegation carrying both. + pub fn with_targets(mut self, targets: Vec) -> Self { + self.targets = Some(targets.iter().map(|c| Blob(c.get().to_vec())).collect()); + self + } + + /// Restricts the kinds of calls the delegation permits. Chainable with + /// [`Self::with_targets`] to build a delegation carrying both. + pub fn with_permissions(mut self, permissions: String) -> Self { + self.permissions = Some(permissions); + self } pub fn pubkey(&self) -> &Vec { diff --git a/rs/validator/http_request_test_utils/src/lib.rs b/rs/validator/http_request_test_utils/src/lib.rs index 226c16238cdd..d4ef36f33569 100644 --- a/rs/validator/http_request_test_utils/src/lib.rs +++ b/rs/validator/http_request_test_utils/src/lib.rs @@ -527,7 +527,7 @@ impl DirectAuthenticationScheme { expiration: Time, targets: Vec, ) -> SignedDelegation { - let delegation = Delegation::new_with_targets(other.public_key_der(), expiration, targets); + let delegation = Delegation::new(other.public_key_der(), expiration).with_targets(targets); let signature = self.sign(&delegation); SignedDelegation::new(delegation, signature) } @@ -540,11 +540,8 @@ impl DirectAuthenticationScheme { expiration: Time, permissions: &str, ) -> SignedDelegation { - let delegation = Delegation::new_with_permissions( - other.public_key_der(), - expiration, - permissions.to_string(), - ); + let delegation = Delegation::new(other.public_key_der(), expiration) + .with_permissions(permissions.to_string()); let signature = self.sign(&delegation); SignedDelegation::new(delegation, signature) } @@ -714,7 +711,7 @@ impl SignedDelegationBuilder { pub fn build(self) -> SignedDelegation { let delegation = match self.targets { Some(canister_ids) => { - Delegation::new_with_targets(self.pubkey, self.expiration, canister_ids) + Delegation::new(self.pubkey, self.expiration).with_targets(canister_ids) } None => Delegation::new(self.pubkey, self.expiration), }; diff --git a/rs/validator/ingress_message/src/lib.rs b/rs/validator/ingress_message/src/lib.rs index 1dede333f987..4dd4e5b7cd9a 100644 --- a/rs/validator/ingress_message/src/lib.rs +++ b/rs/validator/ingress_message/src/lib.rs @@ -46,8 +46,8 @@ pub use internal::TimeProvider; /// * [`RequestValidationError::CanisterNotInDelegationTargets`]: if the request targets a canister /// that is not authorized in one of the delegations. /// * [`RequestValidationError::InvalidSenderInfo`]: if sender info is provided but invalid. -/// * [`RequestValidationError::UpdateCallNotPermittedByDelegation`]: if the request is an -/// update call but a delegation restricts the sender to query calls. +/// * [`RequestValidationError::UpdateCallNotPermittedByDelegation`]: if the request is submitted +/// to `/call` (an update call or replicated query) but a delegation restricts the sender to query calls. pub trait HttpRequestVerifier { fn validate_request(&self, request: &HttpRequest) -> Result<(), RequestValidationError>; } diff --git a/rs/validator/ingress_message/tests/validate_request.rs b/rs/validator/ingress_message/tests/validate_request.rs index 545d724e6b6a..74c32b0dc66c 100644 --- a/rs/validator/ingress_message/tests/validate_request.rs +++ b/rs/validator/ingress_message/tests/validate_request.rs @@ -2240,10 +2240,14 @@ mod delegation_permissions { fn should_reject_all_request_types_when_delegation_permissions_unsupported() { let rng = &mut reproducible_rng(); let verifier = verifier_at_time(CURRENT_TIME).build(); - // Note that "updates" is deliberately unsupported: the supported - // vocabulary is "queries" (queries only) and "all" (queries, - // replicated queries, and updates). - for unsupported in ["writes", "updates", ""] { + // The only supported values are "queries" (queries only) and "all" + // (queries, replicated queries, and updates). Everything else is + // rejected, including "updates" (a plausible-looking value), the + // empty string, and case/whitespace variants of the supported + // values (matching is exact). + for unsupported in [ + "writes", "updates", "", "QUERIES", "queries ", " queries", "All", "all ", + ] { let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, unsupported) .build(); @@ -2309,6 +2313,57 @@ mod delegation_permissions { .build(); assert_eq!(verifier.validate_request(&query), Ok(())); } + + #[test] + fn should_apply_both_targets_and_permissions_restrictions_in_a_chain() { + let rng = &mut reproducible_rng(); + let verifier = verifier_at_time(CURRENT_TIME).build(); + let requested_canister_id = CanisterId::from(42); + // A chain that restricts the targets in one delegation and the + // permissions in another: both restrictions must apply. + let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) + .delegate_to_with_targets( + random_user_key_pair(rng), + CURRENT_TIME, + vec![requested_canister_id, CanisterId::from(43)], + ) + .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .build(); + + // Update call to an in-target canister: rejected by the queries-only + // restriction (the target is satisfied, so it is the permissions + // restriction firing, not the target one). + let update = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_canister_id(Blob(requested_canister_id.get().to_vec())) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_matches!( + verifier.validate_request(&update), + Err(RequestValidationError::UpdateCallNotPermittedByDelegation) + ); + + // Query to an in-target canister: permitted (queries are allowed and + // the target matches). + let query_in_targets = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_canister_id(Blob(requested_canister_id.get().to_vec())) + .with_authentication(AuthenticationScheme::Delegation(chain.clone())) + .build(); + assert_eq!(verifier.validate_request(&query_in_targets), Ok(())); + + // Query to an out-of-target canister: rejected by the targets + // restriction, which applies alongside the permissions restriction. + let query_out_of_targets = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_canister_id(Blob(CanisterId::from(44).get().to_vec())) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + assert_matches!( + verifier.validate_request(&query_out_of_targets), + Err(RequestValidationError::CanisterNotInDelegationTargets(_)) + ); + } } fn default_verifier() -> IngressMessageVerifierBuilder { diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index 0adb83e6ecb7..6b58b985c35b 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -106,8 +106,9 @@ pub trait HttpRequestVerifier: Send + Sync { /// * If the request specifies a `CanisterId` (see `HasCanisterId`), /// then it must be among the set of canister IDs that are common to all delegations. /// * Every delegation's `permissions` field (if any) holds a supported value - /// (`"queries"` or `"all"`). If the request is an update call, no delegation - /// restricts the sender to query calls (`permissions = "queries"`). + /// (`"queries"` or `"all"`). If the request is submitted to `/call` (an update + /// call or replicated query), no delegation restricts the sender to query calls + /// (`permissions = "queries"`). /// /// The following signatures (for signing the request or any delegation) are supported /// (see the [IC specification](https://internetcomputer.org/docs/current/references/ic-interface-spec#signatures)): @@ -147,27 +148,18 @@ where root_of_trust_provider: &R, ) -> Result { validate_ingress_expiry(request, current_time)?; - validate_nonce(request)?; - let restrictions = validate_user_id_and_signature( + let restrictions = validate_request_content( + request, self.validator.as_ref(), - &request.sender(), - &request.id(), - match request.authentication() { - Authentication::Anonymous => None, - Authentication::Authenticated(signature) => Some(signature), - }, current_time, root_of_trust_provider, )?; - // Reject update calls that a delegation restricts to query calls - // before performing potentially expensive canister signature - // verification of the sender info: such calls are rejected either - // way, so the delegation-based rejection takes precedence over an - // invalid sender info. + // A request to a `/call` endpoint (an update call or a replicated + // query) is rejected if a delegation in the chain restricts the + // sender to query calls. if restrictions.queries_only { return Err(UpdateCallNotPermittedByDelegation); } - validate_sender_info(request, self.validator.as_ref(), root_of_trust_provider)?; validate_request_target(request, &restrictions.targets)?; Ok(restrictions.targets) } diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index 9b3bb4a192f3..4f075e9015a5 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -179,7 +179,7 @@ fn plain_authentication_with_one_scoped_delegation() { base64::decode("SyP7C1lwpbsWjwT7ow5CnbiL5JzbyjzQrdDVQQb18yE=").unwrap(), ) .unwrap(); - let delegation = Delegation::new_with_targets(pk2, UNIX_EPOCH, vec![canister_test_id(1)]); + let delegation = Delegation::new(pk2, UNIX_EPOCH).with_targets(vec![canister_test_id(1)]); // Signature of sk1 for the delegation above. let delegation_signature = base64::decode( @@ -234,7 +234,7 @@ mod delegation_permissions { let delegation = match permissions { Some(permissions) => { - Delegation::new_with_permissions(session_pk, UNIX_EPOCH, permissions.to_string()) + Delegation::new(session_pk, UNIX_EPOCH).with_permissions(permissions.to_string()) } None => Delegation::new(session_pk, UNIX_EPOCH), }; @@ -290,7 +290,8 @@ mod delegation_permissions { validate_single_delegation(Some("writes")), Err(InvalidDelegation(UnsupportedDelegationPermissions(value))) if value == "writes" ); - // The previously-used "updates" value is now unsupported. + // "updates" is a plausible-looking value but is nevertheless + // unsupported: the only supported values are "queries" and "all". assert_matches!( validate_single_delegation(Some("updates")), Err(InvalidDelegation(UnsupportedDelegationPermissions(value))) if value == "updates" @@ -336,11 +337,8 @@ fn plain_authentication_with_multiple_delegations() { .unwrap(); // KP1 delegating to KP2. - let delegation = Delegation::new_with_targets( - pk2, - UNIX_EPOCH + Duration::new(4, 0), - vec![canister_test_id(1), canister_test_id(2)], - ); + let delegation = Delegation::new(pk2, UNIX_EPOCH + Duration::new(4, 0)) + .with_targets(vec![canister_test_id(1), canister_test_id(2)]); // Signature of SK1 for `delegation` above. let delegation_signature = base64::decode( @@ -357,11 +355,8 @@ fn plain_authentication_with_multiple_delegations() { .unwrap(); // KP3 delegating to KP4. - let delegation_3 = Delegation::new_with_targets( - pk4, - UNIX_EPOCH + Duration::new(3, 0), - vec![canister_test_id(1)], - ); + let delegation_3 = Delegation::new(pk4, UNIX_EPOCH + Duration::new(3, 0)) + .with_targets(vec![canister_test_id(1)]); // Signature of SK3 for delegation_3 let delegation_3_signature = base64::decode( "a/hTCL8yOijzFIcHdcE0uvt2dj3WQdTiMLPX+xI8mWC0wRt+CYlMoFTc6JlfBopEJDrDwdEBz1n6/S8R2A/CCQ==", diff --git a/rs/validator/tests/ingress_validation.rs b/rs/validator/tests/ingress_validation.rs index 25be133444f9..fdbf8a26381f 100644 --- a/rs/validator/tests/ingress_validation.rs +++ b/rs/validator/tests/ingress_validation.rs @@ -37,7 +37,7 @@ fn delegation_signed_bytes() { #[test] fn delegation_with_targets_signed_bytes() { - let d = Delegation::new_with_targets(vec![1, 2, 3], UNIX_EPOCH, vec![canister_test_id(1)]); + let d = Delegation::new(vec![1, 2, 3], UNIX_EPOCH).with_targets(vec![canister_test_id(1)]); let mut expected_signed_bytes = Vec::new(); expected_signed_bytes.extend_from_slice(b"\x1Aic-request-auth-delegation"); @@ -73,7 +73,7 @@ fn delegation_with_targets_signed_bytes() { #[test] fn delegation_with_permissions_signed_bytes() { - let d = Delegation::new_with_permissions(vec![1, 2, 3], UNIX_EPOCH, "queries".to_string()); + let d = Delegation::new(vec![1, 2, 3], UNIX_EPOCH).with_permissions("queries".to_string()); let mut expected_signed_bytes = Vec::new(); expected_signed_bytes.extend_from_slice(b"\x1Aic-request-auth-delegation"); From 98c7215e6a38630cf52dd00e09892dd40cc2a4c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 13:41:25 +0000 Subject: [PATCH 6/9] refactor: model delegation permissions as a typed enum Per the settled interface-spec model (dfinity/developer-docs#292, which defines the field as the union `Queries | All | Unrestricted`), replace the `Option` `permissions` field with a typed `Option` enum whose `Queries`/`All` variants are encoded on the wire (and in the representation-independent hash) as the text `"queries"`/"all"". Consequences: - Unsupported values are now rejected when the request is decoded (the field is a closed enum), so the runtime AuthenticationError::UnsupportedDelegationPermissions check in validate_delegations is gone, along with the error variant in both the validator and ingress-message crates. - The "unsupported value rejected" coverage moves to a CBOR-decoding test in ic-types (delegation_permissions_rejects_unsupported_value), which also covers case/whitespace variants of the supported values. - The validator/e2e/system tests that previously constructed invalid string permissions are removed or switched to the typed enum. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/tests/crypto/ingress_verification_test.rs | 75 +++++------------ rs/types/types/src/messages.rs | 13 +-- rs/types/types/src/messages/http.rs | 41 +++++++-- rs/types/types/src/messages/http/tests.rs | 37 +++++++-- .../http_request_test_utils/src/lib.rs | 16 ++-- .../ingress_message/src/internal/mod.rs | 3 - rs/validator/ingress_message/src/lib.rs | 6 -- .../ingress_message/tests/validate_request.rs | 83 ++++++------------- rs/validator/src/ingress_validation.rs | 32 ++----- rs/validator/src/ingress_validation/tests.rs | 27 ++---- rs/validator/tests/ingress_validation.rs | 9 +- 11 files changed, 152 insertions(+), 190 deletions(-) diff --git a/rs/tests/crypto/ingress_verification_test.rs b/rs/tests/crypto/ingress_verification_test.rs index 45ab3b55aa2f..cfa5b6b64afc 100644 --- a/rs/tests/crypto/ingress_verification_test.rs +++ b/rs/tests/crypto/ingress_verification_test.rs @@ -19,9 +19,10 @@ use ic_system_test_driver::util::{ }; use ic_types::crypto::Signable; use ic_types::messages::{ - Blob, Certificate, Delegation, HttpCallContent, HttpCanisterUpdate, HttpQueryContent, - HttpReadState, HttpReadStateContent, HttpReadStateResponse, HttpRequestEnvelope, HttpUserQuery, - MessageId, RawSignedSenderInfo, SenderInfoContent, SignedDelegation, + Blob, Certificate, Delegation, DelegationPermissions, HttpCallContent, HttpCanisterUpdate, + HttpQueryContent, HttpReadState, HttpReadStateContent, HttpReadStateResponse, + HttpRequestEnvelope, HttpUserQuery, MessageId, RawSignedSenderInfo, SenderInfoContent, + SignedDelegation, }; use ic_types::{CanisterId, PrincipalId, Time}; use ic_universal_canister::wasm; @@ -708,59 +709,51 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { QueriesOnly { err: &'static str }, /// All request kinds succeed. All, - /// All request kinds are rejected with an error whose text - /// contains the given substring. - Invalid { err: &'static str }, } struct PermissionsTest { note: &'static str, // The `permissions` value of each delegation in the chain // (`None` means the field is omitted for that delegation). - permissions: Vec>, + // Unsupported values cannot be represented (the field is a + // closed enum) and are rejected when the request is decoded; + // that path is covered by ic-types CBOR-decoding tests. + permissions: Vec>, outcome: Outcome, } let update_not_permitted = "Update calls are not permitted"; - let unsupported = "Unsupported delegation permissions"; let scenarios = [ PermissionsTest { note: "single delegation restricted to queries", - permissions: vec![Some("queries")], + permissions: vec![Some(DelegationPermissions::Queries)], outcome: Outcome::QueriesOnly { err: update_not_permitted, }, }, PermissionsTest { note: "single delegation permitting all", - permissions: vec![Some("all")], + permissions: vec![Some(DelegationPermissions::All)], outcome: Outcome::All, }, PermissionsTest { note: "queries restriction in the first of two delegations", - permissions: vec![Some("queries"), None], + permissions: vec![Some(DelegationPermissions::Queries), None], outcome: Outcome::QueriesOnly { err: update_not_permitted, }, }, PermissionsTest { note: "queries restriction in the second of two delegations", - permissions: vec![Some("all"), Some("queries")], + permissions: vec![ + Some(DelegationPermissions::All), + Some(DelegationPermissions::Queries), + ], outcome: Outcome::QueriesOnly { err: update_not_permitted, }, }, - PermissionsTest { - note: "unsupported value \"writes\"", - permissions: vec![Some("writes")], - outcome: Outcome::Invalid { err: unsupported }, - }, - PermissionsTest { - note: "unsupported value \"updates\"", - permissions: vec![Some("updates")], - outcome: Outcome::Invalid { err: unsupported }, - }, ]; for scenario in &scenarios { @@ -785,20 +778,8 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { &delegations, ) .await; - match scenario.outcome { - Outcome::QueriesOnly { .. } | Outcome::All => { - response.expect_query_ok(api_ver) - } - Outcome::Invalid { err } => { - assert_eq!( - response.status(), - 400, - "Scenario {} (query) using v{api_ver} unexpectedly succeeded", - scenario.note - ); - response.expect_text_error(err); - } - } + // Both remaining outcomes permit query calls. + response.expect_query_ok(api_ver); } for &api_ver in ALL_READ_STATE_API_VERSIONS { @@ -810,20 +791,8 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { &delegations, ) .await; - match scenario.outcome { - Outcome::QueriesOnly { .. } | Outcome::All => { - response.expect_read_state_ok(api_ver) - } - Outcome::Invalid { err } => { - assert_eq!( - response.status(), - 400, - "Scenario {} (read_state) using v{api_ver} unexpectedly succeeded", - scenario.note - ); - response.expect_text_error(err); - } - } + // Both remaining outcomes permit read_state requests. + response.expect_read_state_ok(api_ver); } for &api_ver in ALL_UPDATE_API_VERSIONS { @@ -837,7 +806,7 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { .await; match scenario.outcome { Outcome::All => response.expect_update_ok(api_ver), - Outcome::QueriesOnly { err } | Outcome::Invalid { err } => { + Outcome::QueriesOnly { err } => { assert_eq!( response.status(), 400, @@ -1754,7 +1723,7 @@ fn create_delegations_with_targets( fn create_delegations_with_permissions( identities: &[GenericIdentity], - permissions: &[Option<&str>], + permissions: &[Option], ) -> Vec { let delegation_expiry = Time::from_nanos_since_unix_epoch(expiry_time().as_nanos() as u64); @@ -1764,7 +1733,7 @@ fn create_delegations_with_permissions( for i in 1..=delegation_count { let delegation = match permissions[i - 1] { Some(permissions) => Delegation::new(identities[i].public_key_der(), delegation_expiry) - .with_permissions(permissions.to_string()), + .with_permissions(permissions), None => Delegation::new(identities[i].public_key_der(), delegation_expiry), }; diff --git a/rs/types/types/src/messages.rs b/rs/types/types/src/messages.rs index 8a1522aa317d..0496f747b525 100644 --- a/rs/types/types/src/messages.rs +++ b/rs/types/types/src/messages.rs @@ -10,12 +10,13 @@ mod webauthn; pub use self::http::{ Authentication, Certificate, CertificateDelegation, CertificateDelegationFormat, - CertificateDelegationMetadata, Delegation, HasCanisterId, HttpCallContent, HttpCanisterUpdate, - HttpQueryContent, HttpQueryResponse, HttpQueryResponseReply, HttpReadState, - HttpReadStateContent, HttpReadStateResponse, HttpReply, HttpRequest, HttpRequestContent, - HttpRequestEnvelope, HttpRequestError, HttpSignedQueryResponse, HttpStatusResponse, - HttpUserQuery, NodeSignature, QueryResponseHash, RawHttpRequestVal, RawSignedSenderInfo, - ReplicaHealthStatus, SenderInfoContent, SignedDelegation, SignedSenderInfo, + CertificateDelegationMetadata, Delegation, DelegationPermissions, HasCanisterId, + HttpCallContent, HttpCanisterUpdate, HttpQueryContent, HttpQueryResponse, + HttpQueryResponseReply, HttpReadState, HttpReadStateContent, HttpReadStateResponse, HttpReply, + HttpRequest, HttpRequestContent, HttpRequestEnvelope, HttpRequestError, + HttpSignedQueryResponse, HttpStatusResponse, HttpUserQuery, NodeSignature, QueryResponseHash, + RawHttpRequestVal, RawSignedSenderInfo, ReplicaHealthStatus, SenderInfoContent, + SignedDelegation, SignedSenderInfo, }; use crate::methods::Callback; pub use crate::methods::SystemMethod; diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index 0f395ee1eeac..b5a0449f5ba3 100644 --- a/rs/types/types/src/messages/http.rs +++ b/rs/types/types/src/messages/http.rs @@ -536,6 +536,35 @@ impl From for HttpRequestError { } } +/// The kinds of calls a delegation permits, as defined in +/// ``. +/// An absent field on the [`Delegation`] is unrestricted, equivalent to +/// [`DelegationPermissions::All`]. Unsupported values are rejected when +/// decoding the request. +#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, Deserialize, Serialize)] +#[cfg_attr(test, derive(Arbitrary))] +pub enum DelegationPermissions { + /// The sender may only perform query calls and `read_state` requests; + /// requests to `/call` endpoints are rejected. + #[serde(rename = "queries")] + Queries, + /// The sender may perform all kinds of calls (queries, replicated + /// queries, and updates). Same as omitting the field. + #[serde(rename = "all")] + All, +} + +impl DelegationPermissions { + /// The textual value as it appears on the wire and in the + /// representation-independent hash. + fn as_str(&self) -> &'static str { + match self { + DelegationPermissions::Queries => "queries", + DelegationPermissions::All => "all", + } + } +} + /// Describes a delegation map as defined in /// ``. #[derive(Clone, Eq, PartialEq, Hash, Debug, Deserialize, Serialize)] @@ -544,7 +573,7 @@ pub struct Delegation { pubkey: Blob, expiration: Time, targets: Option>, - permissions: Option, + permissions: Option, } impl Delegation { @@ -566,7 +595,7 @@ impl Delegation { /// Restricts the kinds of calls the delegation permits. Chainable with /// [`Self::with_targets`] to build a delegation carrying both. - pub fn with_permissions(mut self, permissions: String) -> Self { + pub fn with_permissions(mut self, permissions: DelegationPermissions) -> Self { self.permissions = Some(permissions); self } @@ -601,8 +630,8 @@ impl Delegation { /// The kinds of calls the delegation permits, if restricted. /// `None` means the delegation is unrestricted. - pub fn permissions(&self) -> Option<&str> { - self.permissions.as_deref() + pub fn permissions(&self) -> Option { + self.permissions } } @@ -620,8 +649,8 @@ impl SignedBytesWithoutDomainSeparator for Delegation { Array(targets.iter().map(|t| Bytes(t.0.as_slice())).collect()), ); } - if let Some(permissions) = &self.permissions { - map.insert("permissions", String(permissions)); + if let Some(permissions) = self.permissions { + map.insert("permissions", String(permissions.as_str())); } bytes.extend_from_slice(&hash_of_map(&map, |key, value| hash_key_val(key, value))); diff --git a/rs/types/types/src/messages/http/tests.rs b/rs/types/types/src/messages/http/tests.rs index acad1eb75db8..08edf9971448 100644 --- a/rs/types/types/src/messages/http/tests.rs +++ b/rs/types/types/src/messages/http/tests.rs @@ -720,8 +720,8 @@ mod cbor_serialization { use crate::{ AmountOf, Time, messages::{ - Blob, Delegation, HttpQueryResponse, HttpQueryResponseReply, HttpStatusResponse, - ReplicaHealthStatus, SignedDelegation, + Blob, Delegation, DelegationPermissions, HttpQueryResponse, HttpQueryResponseReply, + HttpStatusResponse, ReplicaHealthStatus, SignedDelegation, http::{HttpSignedQueryResponse, NodeSignature, btreemap}, }, time::UNIX_EPOCH, @@ -946,7 +946,7 @@ mod cbor_serialization { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: None, - permissions: Some("queries".to_string()), + permissions: Some(DelegationPermissions::Queries), }, Value::Map(btreemap! { text("pubkey") => bytes(&[1, 2, 3]), @@ -962,7 +962,7 @@ mod cbor_serialization { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: Some(vec![Blob(vec![4, 5, 6])]), - permissions: Some("queries".to_string()), + permissions: Some(DelegationPermissions::Queries), }, Value::Map(btreemap! { text("pubkey") => bytes(&[1, 2, 3]), @@ -982,12 +982,16 @@ mod cbor_serialization { fn delegation_permissions_cbor_round_trip() { use crate::crypto::Signable; - for permissions in [None, Some("queries".to_string()), Some("all".to_string())] { + for permissions in [ + None, + Some(DelegationPermissions::Queries), + Some(DelegationPermissions::All), + ] { let delegation = Delegation { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: None, - permissions: permissions.clone(), + permissions, }; let bytes = serde_cbor::to_vec(&delegation).unwrap(); let decoded: Delegation = serde_cbor::from_slice(&bytes).unwrap(); @@ -1008,6 +1012,27 @@ mod cbor_serialization { assert_eq!(decoded.targets, None); } + /// A delegation whose `permissions` field carries an unsupported value + /// (anything other than `"queries"`/`"all"`, including case and + /// whitespace variants of them) must fail to deserialize, so requests + /// carrying such a delegation are rejected when decoded. + #[test] + fn delegation_permissions_rejects_unsupported_value() { + for unsupported in [ + "writes", "updates", "", "QUERIES", "queries ", " queries", "All", "all ", + ] { + let encoded = Value::Map(btreemap! { + text("pubkey") => bytes(&[1, 2, 3]), + text("expiration") => int(0), + text("permissions") => text(unsupported), + }); + assert!( + serde_cbor::value::from_value::(encoded).is_err(), + "permissions value {unsupported:?} unexpectedly deserialized" + ); + } + } + #[test] fn encoding_signed_delegation() { assert_cbor_ser_equal( diff --git a/rs/validator/http_request_test_utils/src/lib.rs b/rs/validator/http_request_test_utils/src/lib.rs index d4ef36f33569..37746eb3fbaf 100644 --- a/rs/validator/http_request_test_utils/src/lib.rs +++ b/rs/validator/http_request_test_utils/src/lib.rs @@ -7,9 +7,9 @@ use ic_crypto_tree_hash::Path; use ic_types::crypto::threshold_sig::ThresholdSigPublicKey; use ic_types::crypto::{CanisterSig, Signable}; use ic_types::messages::{ - Blob, Delegation, HttpCallContent, HttpCanisterUpdate, HttpQueryContent, HttpReadState, - HttpReadStateContent, HttpRequest, HttpRequestEnvelope, HttpUserQuery, MessageId, Query, - RawSignedSenderInfo, ReadState, SignedDelegation, SignedIngressContent, + Blob, Delegation, DelegationPermissions, HttpCallContent, HttpCanisterUpdate, HttpQueryContent, + HttpReadState, HttpReadStateContent, HttpRequest, HttpRequestEnvelope, HttpUserQuery, + MessageId, Query, RawSignedSenderInfo, ReadState, SignedDelegation, SignedIngressContent, }; use ic_types::time::GENESIS; use ic_types::{CanisterId, PrincipalId, Time}; @@ -533,15 +533,15 @@ impl DirectAuthenticationScheme { } /// Creates a delegation that restricts the kinds of calls the delegate - /// may make (e.g., `"queries"`). + /// may make. fn delegate_to_with_permissions( &self, other: &DirectAuthenticationScheme, expiration: Time, - permissions: &str, + permissions: DelegationPermissions, ) -> SignedDelegation { - let delegation = Delegation::new(other.public_key_der(), expiration) - .with_permissions(permissions.to_string()); + let delegation = + Delegation::new(other.public_key_der(), expiration).with_permissions(permissions); let signature = self.sign(&delegation); SignedDelegation::new(delegation, signature) } @@ -613,7 +613,7 @@ impl DelegationChainBuilder { mut self, new_end: DirectAuthenticationScheme, expiration: Time, - permissions: &str, + permissions: DelegationPermissions, ) -> Self { let current_end = self.end.unwrap_or_else(|| self.start.clone()); self.signed_delegations diff --git a/rs/validator/ingress_message/src/internal/mod.rs b/rs/validator/ingress_message/src/internal/mod.rs index fb9ce129e432..8dfdec994b6d 100644 --- a/rs/validator/ingress_message/src/internal/mod.rs +++ b/rs/validator/ingress_message/src/internal/mod.rs @@ -236,9 +236,6 @@ fn to_authentication_lib_error(error: ic_validator::AuthenticationError) -> Auth ic_validator::AuthenticationError::DelegationContainsCyclesError { public_key } => { AuthenticationError::DelegationContainsCyclesError { public_key } } - ic_validator::AuthenticationError::UnsupportedDelegationPermissions(permissions) => { - AuthenticationError::UnsupportedDelegationPermissions(permissions) - } } } diff --git a/rs/validator/ingress_message/src/lib.rs b/rs/validator/ingress_message/src/lib.rs index 4dd4e5b7cd9a..4bf613f25d37 100644 --- a/rs/validator/ingress_message/src/lib.rs +++ b/rs/validator/ingress_message/src/lib.rs @@ -152,9 +152,6 @@ pub enum AuthenticationError { /// which was already encountered before in the chain of delegations. /// Note that if both keys are equal, then this delegation is self-signed, which is also forbidden. DelegationContainsCyclesError { public_key: Vec }, - - /// A delegation's `permissions` field holds an unsupported value. - UnsupportedDelegationPermissions(String), } impl Display for AuthenticationError { @@ -178,9 +175,6 @@ impl Display for AuthenticationError { "Chain of delegations contains at least one cycle: first repeating public key encountered {}", hex::encode(public_key) ), - AuthenticationError::UnsupportedDelegationPermissions(permissions) => { - write!(f, "Unsupported delegation permissions: {permissions}") - } } } } diff --git a/rs/validator/ingress_message/tests/validate_request.rs b/rs/validator/ingress_message/tests/validate_request.rs index 74c32b0dc66c..b0ce57c3fb06 100644 --- a/rs/validator/ingress_message/tests/validate_request.rs +++ b/rs/validator/ingress_message/tests/validate_request.rs @@ -2177,15 +2177,19 @@ mod sender_info { mod delegation_permissions { use super::*; + use ic_types::messages::DelegationPermissions; use ic_validator_http_request_test_utils::DelegationChain; - use ic_validator_ingress_message::AuthenticationError::UnsupportedDelegationPermissions; #[test] fn should_reject_update_call_when_delegation_restricts_to_queries() { let rng = &mut reproducible_rng(); let verifier = verifier_at_time(CURRENT_TIME).build(); let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .delegate_to_with_permissions( + random_user_key_pair(rng), + CURRENT_TIME, + DelegationPermissions::Queries, + ) .build(); let request = HttpRequestBuilder::new_update_call() @@ -2204,7 +2208,11 @@ mod delegation_permissions { let rng = &mut reproducible_rng(); let verifier = verifier_at_time(CURRENT_TIME).build(); let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .delegate_to_with_permissions( + random_user_key_pair(rng), + CURRENT_TIME, + DelegationPermissions::Queries, + ) .build(); let query = HttpRequestBuilder::new_query() @@ -2225,7 +2233,11 @@ mod delegation_permissions { let rng = &mut reproducible_rng(); let verifier = verifier_at_time(CURRENT_TIME).build(); let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "all") + .delegate_to_with_permissions( + random_user_key_pair(rng), + CURRENT_TIME, + DelegationPermissions::All, + ) .build(); let request = HttpRequestBuilder::new_update_call() @@ -2236,57 +2248,6 @@ mod delegation_permissions { assert_eq!(verifier.validate_request(&request), Ok(())); } - #[test] - fn should_reject_all_request_types_when_delegation_permissions_unsupported() { - let rng = &mut reproducible_rng(); - let verifier = verifier_at_time(CURRENT_TIME).build(); - // The only supported values are "queries" (queries only) and "all" - // (queries, replicated queries, and updates). Everything else is - // rejected, including "updates" (a plausible-looking value), the - // empty string, and case/whitespace variants of the supported - // values (matching is exact). - for unsupported in [ - "writes", "updates", "", "QUERIES", "queries ", " queries", "All", "all ", - ] { - let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, unsupported) - .build(); - - let update = HttpRequestBuilder::new_update_call() - .with_ingress_expiry_at(CURRENT_TIME) - .with_authentication(AuthenticationScheme::Delegation(chain.clone())) - .build(); - assert_matches!( - verifier.validate_request(&update), - Err(RequestValidationError::InvalidDelegation( - UnsupportedDelegationPermissions(permissions) - )) if permissions == unsupported - ); - - let query = HttpRequestBuilder::new_query() - .with_ingress_expiry_at(CURRENT_TIME) - .with_authentication(AuthenticationScheme::Delegation(chain.clone())) - .build(); - assert_matches!( - verifier.validate_request(&query), - Err(RequestValidationError::InvalidDelegation( - UnsupportedDelegationPermissions(_) - )) - ); - - let read_state = HttpRequestBuilder::new_read_state() - .with_ingress_expiry_at(CURRENT_TIME) - .with_authentication(AuthenticationScheme::Delegation(chain)) - .build(); - assert_matches!( - verifier.validate_request(&read_state), - Err(RequestValidationError::InvalidDelegation( - UnsupportedDelegationPermissions(_) - )) - ); - } - } - #[test] fn should_reject_update_call_when_any_delegation_in_chain_restricts_to_queries() { let rng = &mut reproducible_rng(); @@ -2294,7 +2255,11 @@ mod delegation_permissions { // The restriction sits in the middle of the chain; subsequent // unrestricted delegations must not lift it. let chain = DelegationChain::rooted_at(random_user_key_pair(rng)) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .delegate_to_with_permissions( + random_user_key_pair(rng), + CURRENT_TIME, + DelegationPermissions::Queries, + ) .delegate_to(random_user_key_pair(rng), CURRENT_TIME) .build(); @@ -2327,7 +2292,11 @@ mod delegation_permissions { CURRENT_TIME, vec![requested_canister_id, CanisterId::from(43)], ) - .delegate_to_with_permissions(random_user_key_pair(rng), CURRENT_TIME, "queries") + .delegate_to_with_permissions( + random_user_key_pair(rng), + CURRENT_TIME, + DelegationPermissions::Queries, + ) .build(); // Update call to an in-target canister: rejected by the queries-only diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index 6b58b985c35b..5df7fefa0417 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -13,9 +13,9 @@ use ic_types::{ threshold_sig::RootOfTrustProvider, }, messages::{ - Authentication, Delegation, HasCanisterId, HttpRequest, HttpRequestContent, MessageId, - Query, ReadState, SenderInfoContent, SignedDelegation, SignedIngressContent, - SignedSenderInfo, UserSignature, WebAuthnSignature, + Authentication, Delegation, DelegationPermissions, HasCanisterId, HttpRequest, + HttpRequestContent, MessageId, Query, ReadState, SenderInfoContent, SignedDelegation, + SignedIngressContent, SignedSenderInfo, UserSignature, WebAuthnSignature, }, }; use std::{ @@ -60,16 +60,6 @@ const MAXIMUM_NUMBER_OF_PATHS: usize = 1_000; /// and so changing this value might be breaking or result in a deviation from the specification. const MAXIMUM_NUMBER_OF_LABELS_PER_PATH: usize = 127; -/// Value of a delegation's `permissions` field that restricts the sender -/// to query calls: requests to `/call` endpoints (i.e., updates and -/// replicated queries) are rejected. -const DELEGATION_PERMISSIONS_QUERIES: &str = "queries"; - -/// Value of a delegation's `permissions` field that explicitly permits all -/// kinds of calls — queries, replicated queries, and updates (same as an -/// absent `permissions` field). -const DELEGATION_PERMISSIONS_ALL: &str = "all"; - /// Restrictions that a chain of delegations places on the sender. #[derive(Debug)] struct DelegationRestrictions { @@ -334,8 +324,6 @@ pub enum AuthenticationError { DelegationTooLongError { length: usize, maximum: usize }, #[error("Chain of delegations contains at least one cycle: first repeating public key encountered {}", hex::encode(.public_key))] DelegationContainsCyclesError { public_key: Vec }, - #[error("Unsupported delegation permissions: {0}")] - UnsupportedDelegationPermissions(String), } /// Set of canister IDs. @@ -801,17 +789,11 @@ where // Restrict the canister targets to the ones specified in the delegation. restrictions.targets = restrictions.targets.intersect(new_targets); // Restrict the kinds of calls to the ones permitted by the delegation. - // Unsupported values are rejected (fail-closed): no delegations with a - // `permissions` field exist that predate this validation, so there is - // no backward compatibility concern. + // Unsupported `permissions` values are rejected earlier, when the + // request is decoded, since the field is a closed enum. match delegation.permissions() { - None | Some(DELEGATION_PERMISSIONS_ALL) => {} - Some(DELEGATION_PERMISSIONS_QUERIES) => restrictions.queries_only = true, - Some(unsupported) => { - return Err(InvalidDelegation(UnsupportedDelegationPermissions( - unsupported.to_string(), - ))); - } + None | Some(DelegationPermissions::All) => {} + Some(DelegationPermissions::Queries) => restrictions.queries_only = true, } pubkey = delegation.pubkey().to_vec(); } diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index 4f075e9015a5..48444893fb37 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -5,7 +5,7 @@ use ic_crypto_temp_crypto::temp_crypto_component_with_fake_registry; use ic_crypto_test_utils_root_of_trust::MockRootOfTrustProvider; use ic_test_utilities_types::ids::{canister_test_id, message_test_id, node_test_id}; use ic_types::{ - messages::{Delegation, SignedDelegation, UserSignature}, + messages::{Delegation, DelegationPermissions, SignedDelegation, UserSignature}, time::UNIX_EPOCH, }; use std::time::Duration; @@ -221,7 +221,7 @@ mod delegation_permissions { /// signable in-process, unlike the hard-coded ed25519 signatures used by /// the surrounding tests). fn validate_single_delegation( - permissions: Option<&str>, + permissions: Option, ) -> Result { let sig_verifier = temp_crypto_component_with_fake_registry(node_test_id(0)); let message_id = message_test_id(1); @@ -234,7 +234,7 @@ mod delegation_permissions { let delegation = match permissions { Some(permissions) => { - Delegation::new(session_pk, UNIX_EPOCH).with_permissions(permissions.to_string()) + Delegation::new(session_pk, UNIX_EPOCH).with_permissions(permissions) } None => Delegation::new(session_pk, UNIX_EPOCH), }; @@ -263,7 +263,7 @@ mod delegation_permissions { #[test] fn queries_permission_sets_queries_only() { assert_matches!( - validate_single_delegation(Some("queries")), + validate_single_delegation(Some(DelegationPermissions::Queries)), Ok(restrictions) if restrictions.queries_only ); } @@ -271,7 +271,7 @@ mod delegation_permissions { #[test] fn all_permission_does_not_set_queries_only() { assert_matches!( - validate_single_delegation(Some("all")), + validate_single_delegation(Some(DelegationPermissions::All)), Ok(restrictions) if !restrictions.queries_only ); } @@ -284,19 +284,10 @@ mod delegation_permissions { ); } - #[test] - fn unsupported_permission_is_rejected() { - assert_matches!( - validate_single_delegation(Some("writes")), - Err(InvalidDelegation(UnsupportedDelegationPermissions(value))) if value == "writes" - ); - // "updates" is a plausible-looking value but is nevertheless - // unsupported: the only supported values are "queries" and "all". - assert_matches!( - validate_single_delegation(Some("updates")), - Err(InvalidDelegation(UnsupportedDelegationPermissions(value))) if value == "updates" - ); - } + // Unsupported `permissions` values cannot reach the validator: the field + // is a closed enum, so unknown values are rejected when the request is + // decoded. That rejection is covered by a CBOR-decoding test in ic-types + // (`delegation_permissions_rejects_unsupported_value`). } #[test] diff --git a/rs/validator/tests/ingress_validation.rs b/rs/validator/tests/ingress_validation.rs index fdbf8a26381f..524754da43e5 100644 --- a/rs/validator/tests/ingress_validation.rs +++ b/rs/validator/tests/ingress_validation.rs @@ -1,6 +1,10 @@ use ic_crypto_sha2::Sha256; use ic_test_utilities_types::ids::canister_test_id; -use ic_types::{crypto::Signable, messages::Delegation, time::UNIX_EPOCH}; +use ic_types::{ + crypto::Signable, + messages::{Delegation, DelegationPermissions}, + time::UNIX_EPOCH, +}; // NOTE: Ideally, this test should be in the types crate where `Delegation` is // defined, but the test is here to avoid circular dependencies between the @@ -73,7 +77,8 @@ fn delegation_with_targets_signed_bytes() { #[test] fn delegation_with_permissions_signed_bytes() { - let d = Delegation::new(vec![1, 2, 3], UNIX_EPOCH).with_permissions("queries".to_string()); + let d = + Delegation::new(vec![1, 2, 3], UNIX_EPOCH).with_permissions(DelegationPermissions::Queries); let mut expected_signed_bytes = Vec::new(); expected_signed_bytes.extend_from_slice(b"\x1Aic-request-auth-delegation"); From b988ba6f7d107ecb81211287841da0c3fc80f557 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 14:09:46 +0000 Subject: [PATCH 7/9] refactor: drop redundant comment in delegation permissions tests https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/validator/src/ingress_validation/tests.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/rs/validator/src/ingress_validation/tests.rs b/rs/validator/src/ingress_validation/tests.rs index 48444893fb37..8006d0dc5c43 100644 --- a/rs/validator/src/ingress_validation/tests.rs +++ b/rs/validator/src/ingress_validation/tests.rs @@ -283,11 +283,6 @@ mod delegation_permissions { Ok(restrictions) if !restrictions.queries_only ); } - - // Unsupported `permissions` values cannot reach the validator: the field - // is a closed enum, so unknown values are rejected when the request is - // decoded. That rejection is covered by a CBOR-decoding test in ic-types - // (`delegation_permissions_rejects_unsupported_value`). } #[test] From 0ecc183f9ef6a02c3908179f59bcb6e2efe387d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 24 Jun 2026 08:41:56 +0000 Subject: [PATCH 8/9] docs: address review comments on delegation permissions doc/test comments Trim verbose doc comments on DelegationPermissions and the Delegation builders, drop the broken interface-spec anchor, and clarify the system-test comments per reviewer feedback. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/tests/crypto/ingress_verification_test.rs | 7 ++----- rs/types/types/src/messages/http.rs | 11 ++++------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/rs/tests/crypto/ingress_verification_test.rs b/rs/tests/crypto/ingress_verification_test.rs index cfa5b6b64afc..9cae1d80a1e1 100644 --- a/rs/tests/crypto/ingress_verification_test.rs +++ b/rs/tests/crypto/ingress_verification_test.rs @@ -715,9 +715,6 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { note: &'static str, // The `permissions` value of each delegation in the chain // (`None` means the field is omitted for that delegation). - // Unsupported values cannot be represented (the field is a - // closed enum) and are rejected when the request is decoded; - // that path is covered by ic-types CBOR-decoding tests. permissions: Vec>, outcome: Outcome, } @@ -778,7 +775,7 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { &delegations, ) .await; - // Both remaining outcomes permit query calls. + // Query calls are permitted regardless of the outcome. response.expect_query_ok(api_ver); } @@ -791,7 +788,7 @@ pub fn requests_with_delegation_permissions(env: TestEnv) { &delegations, ) .await; - // Both remaining outcomes permit read_state requests. + // read_state requests are permitted regardless of the outcome. response.expect_read_state_ok(api_ver); } diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index b5a0449f5ba3..88ae91f08ef5 100644 --- a/rs/types/types/src/messages/http.rs +++ b/rs/types/types/src/messages/http.rs @@ -537,10 +537,9 @@ impl From for HttpRequestError { } /// The kinds of calls a delegation permits, as defined in -/// ``. +/// ``. /// An absent field on the [`Delegation`] is unrestricted, equivalent to -/// [`DelegationPermissions::All`]. Unsupported values are rejected when -/// decoding the request. +/// [`DelegationPermissions::All`]. #[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, Deserialize, Serialize)] #[cfg_attr(test, derive(Arbitrary))] pub enum DelegationPermissions { @@ -586,15 +585,13 @@ impl Delegation { } } - /// Restricts the delegation to the given canister targets. Chainable - /// with [`Self::with_permissions`] to build a delegation carrying both. + /// Restricts the delegation to the given canister targets. pub fn with_targets(mut self, targets: Vec) -> Self { self.targets = Some(targets.iter().map(|c| Blob(c.get().to_vec())).collect()); self } - /// Restricts the kinds of calls the delegation permits. Chainable with - /// [`Self::with_targets`] to build a delegation carrying both. + /// Restricts the kinds of calls the delegation permits. pub fn with_permissions(mut self, permissions: DelegationPermissions) -> Self { self.permissions = Some(permissions); self From 937ddbe33166965f3970a0c151a2febcb23b6e5b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 24 Jun 2026 09:40:55 +0000 Subject: [PATCH 9/9] docs: use correct migrated interface-spec link for DelegationPermissions Point the doc comment at the post-migration URL with a valid `#authentication` anchor, per reviewer feedback. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/types/types/src/messages/http.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index 88ae91f08ef5..b7282931704d 100644 --- a/rs/types/types/src/messages/http.rs +++ b/rs/types/types/src/messages/http.rs @@ -537,7 +537,7 @@ impl From for HttpRequestError { } /// The kinds of calls a delegation permits, as defined in -/// ``. +/// ``. /// An absent field on the [`Delegation`] is unrestricted, equivalent to /// [`DelegationPermissions::All`]. #[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, Deserialize, Serialize)]