From 6e0edf31f415c3b2e96085b1c7d8b1de1d355edc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 20:58:05 +0000 Subject: [PATCH 1/2] 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/2] 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(