From 1a860e0ac1e1b7186dd4545d78b82c35072699d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 12 Jun 2026 09:59:02 +0000 Subject: [PATCH 1/2] feat: delegations can require calls to carry a signed sender_info Adds an optional sender_info_required field to request delegations, covered by the delegation signature. When a delegation in a request's delegation chain sets it to true, update calls authenticated through that chain must carry a (signed and verified) sender_info; calls without one are rejected with the new SenderInfoRequiredByDelegation error. Query and read_state requests are unaffected (read_state cannot even carry a sender_info, and query responses cannot change state). Since the field is part of the delegation's representation-independent hash, the bearer of the delegation cannot lift the requirement by stripping the field: doing so invalidates the delegation signature. For the same reason, replicas that predate this change fail closed on such delegations. Combined with sender_info attribute checking (#10447), this lets issuers like Internet Identity enforce read-only sessions end to end: the delegation forces the certified session attributes to be present on every update call, and the attributes' implicit:permissions = "queries" then causes the call to be rejected. Without this bit, a dapp holding the session key could lift the restriction by omitting the sender_info. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/types/types/src/messages/http.rs | 28 ++++ rs/types/types/src/messages/http/tests.rs | 23 +++ .../http_request_test_utils/src/lib.rs | 33 +++++ .../ingress_message/src/internal/mod.rs | 3 + rs/validator/ingress_message/src/lib.rs | 10 ++ .../ingress_message/tests/validate_request.rs | 135 ++++++++++++++++++ rs/validator/src/ingress_validation.rs | 99 ++++++++++--- rs/validator/src/ingress_validation/tests.rs | 20 +-- rs/validator/tests/ingress_validation.rs | 34 +++++ 9 files changed, 352 insertions(+), 33 deletions(-) diff --git a/rs/types/types/src/messages/http.rs b/rs/types/types/src/messages/http.rs index add56b40d4ff..a1b07200ce88 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>, + sender_info_required: Option, } impl Delegation { @@ -552,6 +553,7 @@ impl Delegation { pubkey: Blob(pubkey), expiration, targets: None, + sender_info_required: None, } } @@ -560,6 +562,20 @@ impl Delegation { pubkey: Blob(pubkey), expiration, targets: Some(targets.iter().map(|c| Blob(c.get().to_vec())).collect()), + sender_info_required: None, + } + } + + pub fn new_with_sender_info_required( + pubkey: Vec, + expiration: Time, + sender_info_required: bool, + ) -> Self { + Self { + pubkey: Blob(pubkey), + expiration, + targets: None, + sender_info_required: Some(sender_info_required), } } @@ -590,6 +606,12 @@ impl Delegation { pub fn number_of_targets(&self) -> Option { self.targets.as_ref().map(Vec::len) } + + /// Whether the delegation requires requests authenticated through it to + /// carry a signed `sender_info`. + pub fn sender_info_required(&self) -> bool { + self.sender_info_required.unwrap_or(false) + } } impl SignedBytesWithoutDomainSeparator for Delegation { @@ -606,6 +628,12 @@ impl SignedBytesWithoutDomainSeparator for Delegation { Array(targets.iter().map(|t| Bytes(t.0.as_slice())).collect()), ); } + if let Some(sender_info_required) = self.sender_info_required { + map.insert( + "sender_info_required", + U64(if sender_info_required { 1 } else { 0 }), + ); + } 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..45b9037768a5 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)), ]), + sender_info_required: None, }; let targets = delegation.targets(); @@ -33,6 +34,7 @@ mod targets { let delegation = Delegation { pubkey: Blob(vec![]), expiration: CURRENT_TIME, + sender_info_required: 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, + sender_info_required: None, }, Value::Map(btreemap! { text("pubkey") => bytes(&[1, 2, 3]), text("expiration") => int(0), text("targets") => Value::Null, + text("sender_info_required") => 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])]), + sender_info_required: None, }, Value::Map(btreemap! { text("pubkey") => bytes(&[1, 2, 3]), text("expiration") => int(0), text("targets") => Value::Array(vec![bytes(&[4, 5, 6])]), + text("sender_info_required") => Value::Null, + }), + ); + + assert_cbor_ser_equal( + &Delegation { + pubkey: Blob(vec![1, 2, 3]), + expiration: UNIX_EPOCH, + targets: None, + sender_info_required: Some(true), + }, + Value::Map(btreemap! { + text("pubkey") => bytes(&[1, 2, 3]), + text("expiration") => int(0), + text("targets") => Value::Null, + text("sender_info_required") => Value::Bool(true), }), ); } @@ -944,6 +965,7 @@ mod cbor_serialization { pubkey: Blob(vec![1, 2, 3]), expiration: UNIX_EPOCH, targets: None, + sender_info_required: 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("sender_info_required") => 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..e1157ff7f258 100644 --- a/rs/validator/http_request_test_utils/src/lib.rs +++ b/rs/validator/http_request_test_utils/src/lib.rs @@ -531,6 +531,22 @@ impl DirectAuthenticationScheme { let signature = self.sign(&delegation); SignedDelegation::new(delegation, signature) } + + /// Creates a delegation that requires calls to carry a signed `sender_info`. + fn delegate_to_with_sender_info_required( + &self, + other: &DirectAuthenticationScheme, + expiration: Time, + sender_info_required: bool, + ) -> SignedDelegation { + let delegation = Delegation::new_with_sender_info_required( + other.public_key_der(), + expiration, + sender_info_required, + ); + let signature = self.sign(&delegation); + SignedDelegation::new(delegation, signature) + } } #[derive(Clone, Eq, PartialEq, Debug)] @@ -595,6 +611,23 @@ impl DelegationChainBuilder { self } + pub fn delegate_to_with_sender_info_required( + mut self, + new_end: DirectAuthenticationScheme, + expiration: Time, + sender_info_required: bool, + ) -> Self { + let current_end = self.end.unwrap_or_else(|| self.start.clone()); + self.signed_delegations + .push(current_end.delegate_to_with_sender_info_required( + &new_end, + expiration, + sender_info_required, + )); + 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..bd5bc0b401a2 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::SenderInfoRequiredByDelegation => { + RequestValidationError::SenderInfoRequiredByDelegation + } } } fn to_authentication_lib_error(error: ic_validator::AuthenticationError) -> AuthenticationError { diff --git a/rs/validator/ingress_message/src/lib.rs b/rs/validator/ingress_message/src/lib.rs index e3080a118bed..775c346368ae 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::SenderInfoRequiredByDelegation`]: if the request is an update +/// call without a `sender_info` but a delegation requires one to be present. 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), + SenderInfoRequiredByDelegation, } impl Display for RequestValidationError { @@ -112,6 +115,13 @@ impl Display for RequestValidationError { RequestValidationError::InvalidSenderInfo(msg) => { write!(f, "Invalid sender info: {msg}") } + RequestValidationError::SenderInfoRequiredByDelegation => { + write!( + f, + "A delegation requires the call to carry a signed sender info, \ + but no sender_info was provided" + ) + } } } } diff --git a/rs/validator/ingress_message/tests/validate_request.rs b/rs/validator/ingress_message/tests/validate_request.rs index 6c3348be9c01..64054ee3da13 100644 --- a/rs/validator/ingress_message/tests/validate_request.rs +++ b/rs/validator/ingress_message/tests/validate_request.rs @@ -2175,6 +2175,141 @@ mod sender_info { } } +mod delegation_sender_info_required { + use super::*; + use ic_types::messages::{RawSignedSenderInfo, SenderInfoContent}; + use ic_validator_http_request_test_utils::DelegationChain; + use ic_validator_http_request_test_utils::DirectAuthenticationScheme::CanisterSignature; + + /// Creates a canister signer (acting as the delegation issuer, e.g. + /// Internet Identity), a delegation chain rooted at it that requires a + /// signed `sender_info`, and a valid `RawSignedSenderInfo` signed by the + /// same canister. + fn setup( + rng: &mut R, + sender_info_required: bool, + ) -> ( + IngressMessageVerifierBuilder, + DelegationChain, + RawSignedSenderInfo, + ) { + let root_of_trust = RootOfTrust::new_random(rng); + let builder = default_verifier().with_root_of_trust(root_of_trust.public_key); + let signer = CanisterSigner { + seed: CANISTER_SIGNATURE_SEED.to_vec(), + canister_id: CANISTER_ID_SIGNER, + root_public_key: root_of_trust.public_key, + root_secret_key: root_of_trust.secret_key, + }; + let info_bytes = b"certified session attributes".to_vec(); + let sig = signer.sign(&SenderInfoContent(&info_bytes)); + let sender_info = RawSignedSenderInfo { + info: Blob(info_bytes), + signer: Blob(CANISTER_ID_SIGNER.get().into_vec()), + sig: Blob(sig.0), + }; + let chain = DelegationChain::rooted_at(CanisterSignature(signer)) + .delegate_to_with_sender_info_required( + random_user_key_pair(rng), + CURRENT_TIME, + sender_info_required, + ) + .build(); + (builder, chain, sender_info) + } + + #[test] + fn should_reject_update_call_without_sender_info_when_delegation_requires_it() { + let rng = &mut reproducible_rng(); + let (builder, chain, _sender_info) = setup(rng, true); + let verifier = builder.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::SenderInfoRequiredByDelegation) + ); + } + + #[test] + fn should_accept_update_call_with_sender_info_when_delegation_requires_it() { + let rng = &mut reproducible_rng(); + let (builder, chain, sender_info) = setup(rng, true); + let verifier = builder.build(); + + let request = HttpRequestBuilder::new_update_call() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .with_sender_info(sender_info) + .build(); + + assert_eq!(verifier.validate_request(&request), Ok(())); + } + + #[test] + fn should_accept_query_without_sender_info_when_delegation_requires_it() { + let rng = &mut reproducible_rng(); + let (builder, chain, _sender_info) = setup(rng, true); + let verifier = builder.build(); + + let request = HttpRequestBuilder::new_query() + .with_ingress_expiry_at(CURRENT_TIME) + .with_authentication(AuthenticationScheme::Delegation(chain)) + .build(); + + assert_eq!(verifier.validate_request(&request), Ok(())); + } + + #[test] + fn should_accept_update_call_without_sender_info_when_not_required() { + let rng = &mut reproducible_rng(); + let (builder, chain, _sender_info) = setup(rng, false); + let verifier = builder.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_update_call_when_any_delegation_in_chain_requires_sender_info() { + let rng = &mut reproducible_rng(); + let root_of_trust = RootOfTrust::new_random(rng); + let verifier = default_verifier() + .with_root_of_trust(root_of_trust.public_key) + .build(); + let signer = CanisterSigner { + seed: CANISTER_SIGNATURE_SEED.to_vec(), + canister_id: CANISTER_ID_SIGNER, + root_public_key: root_of_trust.public_key, + root_secret_key: root_of_trust.secret_key, + }; + // The requirement sits in the middle of the chain; subsequent + // delegations must not lift it. + let chain = DelegationChain::rooted_at(CanisterSignature(signer)) + .delegate_to_with_sender_info_required(random_user_key_pair(rng), CURRENT_TIME, true) + .delegate_to(random_user_key_pair(rng), CURRENT_TIME) + .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::SenderInfoRequiredByDelegation) + ); + } +} + 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..b352ffbda0e0 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -60,6 +60,25 @@ 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; +/// 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 requires calls to carry a signed + /// `sender_info`. + sender_info_required: bool, +} + +impl DelegationRestrictions { + fn unrestricted() -> Self { + Self { + targets: CanisterIdSet::all(), + sender_info_required: false, + } + } +} + /// A trait for validating an `HttpRequest` with content `C`. pub trait HttpRequestVerifier: Send + Sync { /// Validates the given request. @@ -76,6 +95,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. + /// * If the request is an update call and some delegation requires the sender to + /// carry a signed `sender_info` (`sender_info_required = true`), then the request + /// must contain a `sender_info`. /// /// 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 +137,29 @@ where root_of_trust_provider: &R, ) -> Result { validate_ingress_expiry(request, current_time)?; - let delegation_targets = 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, )?; - validate_request_target(request, &delegation_targets)?; - Ok(delegation_targets) + // A delegation may require update calls to carry a signed + // `sender_info` (e.g. certified session attributes issued by the + // delegation's signer). Since the requirement is part of the signed + // delegation, the bearer of the delegation cannot lift it by simply + // omitting the sender info. + if restrictions.sender_info_required && request.sender_info().is_none() { + return Err(SenderInfoRequiredByDelegation); + } + validate_sender_info(request, self.validator.as_ref(), root_of_trust_provider)?; + validate_request_target(request, &restrictions.targets)?; + Ok(restrictions.targets) } } @@ -140,14 +177,16 @@ where if !request.sender().get().is_anonymous() { validate_ingress_expiry(request, current_time)?; } - let delegation_targets = validate_request_content( + // `sender_info_required` only applies to update calls: query + // responses cannot change state, so there is nothing to restrict. + 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 +205,15 @@ where if !request.sender().get().is_anonymous() { validate_ingress_expiry(request, current_time)?; } + // `sender_info_required` only applies to update calls; read_state + // requests cannot even carry a `sender_info`. validate_request_content( request, self.validator.as_ref(), current_time, root_of_trust_provider, ) + .map(|restrictions| restrictions.targets) } } @@ -198,14 +240,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 +259,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 +308,11 @@ pub enum RequestValidationError { NonceTooBig { num_bytes: usize, maximum: usize }, #[error("Invalid sender info: {0}")] InvalidSenderInfo(String), + #[error( + "A delegation requires the call to carry a signed sender info, \ + but no sender_info was provided" + )] + SenderInfoRequiredByDelegation, } /// Error in verifying the signature or authentication part of a request. @@ -638,7 +685,7 @@ fn validate_signature( signature: &UserSignature, current_time: Time, root_of_trust_provider: &R, -) -> Result +) -> Result where R::Error: std::error::Error, { @@ -647,7 +694,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 +713,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 +721,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 +736,7 @@ where e.to_string() )) ); - Ok(targets) + Ok(restrictions) } KeyBytesContentType::RsaSha256PublicKeyDer => { Err(RequestValidationError::InvalidSignature( @@ -717,20 +764,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 calls must carry a signed `sender_info`). 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 +795,14 @@ 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); + // Since the field is covered by the delegation signature, the bearer + // of the delegation cannot lift the requirement by stripping it. + restrictions.sender_info_required |= delegation.sender_info_required(); pubkey = delegation.pubkey().to_vec(); } - Ok((pubkey, targets)) + Ok((pubkey, restrictions)) } fn ensure_delegations_does_not_contain_cycles( @@ -846,14 +899,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..0702ccfeed07 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.sender_info_required ); // 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() && !restrictions.sender_info_required ); } @@ -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() && !restrictions.sender_info_required ); 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.sender_info_required ); } @@ -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.sender_info_required ); } @@ -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.sender_info_required ); } diff --git a/rs/validator/tests/ingress_validation.rs b/rs/validator/tests/ingress_validation.rs index 78fde532c0de..d819d3266a63 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_sender_info_required_signed_bytes() { + let d = Delegation::new_with_sender_info_required(vec![1, 2, 3], UNIX_EPOCH, true); + + 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 sender_info_required_hash = Vec::new(); + sender_info_required_hash.extend_from_slice(&Sha256::hash(b"sender_info_required")); + sender_info_required_hash.extend_from_slice(&Sha256::hash(&[1])); + + let mut hashes: Vec> = vec![pubkey_hash, expiration_hash, sender_info_required_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 4aa50fb6301527d61cb8e37c4cc77a5fc60af671 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 12 Jun 2026 10:05:37 +0000 Subject: [PATCH 2/2] refactor: address review comments Avoid duplicating the shared request-content validation on the call path by enforcing the sender_info_required restriction inside validate_request_content behind a parameter, and fix a comment to say query requests rather than query responses. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --- rs/validator/src/ingress_validation.rs | 47 ++++++++++++++++---------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/rs/validator/src/ingress_validation.rs b/rs/validator/src/ingress_validation.rs index b352ffbda0e0..6bd94232a315 100644 --- a/rs/validator/src/ingress_validation.rs +++ b/rs/validator/src/ingress_validation.rs @@ -137,27 +137,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, + // A delegation may require update calls to carry a signed + // `sender_info` (e.g. certified session attributes issued by the + // delegation's signer). Since the requirement is part of the + // signed delegation, the bearer of the delegation cannot lift it + // by simply omitting the sender info. + SenderInfoRequirement::EnforceIfRequiredByDelegation, )?; - // A delegation may require update calls to carry a signed - // `sender_info` (e.g. certified session attributes issued by the - // delegation's signer). Since the requirement is part of the signed - // delegation, the bearer of the delegation cannot lift it by simply - // omitting the sender info. - if restrictions.sender_info_required && request.sender_info().is_none() { - return Err(SenderInfoRequiredByDelegation); - } - validate_sender_info(request, self.validator.as_ref(), root_of_trust_provider)?; validate_request_target(request, &restrictions.targets)?; Ok(restrictions.targets) } @@ -178,12 +169,13 @@ where validate_ingress_expiry(request, current_time)?; } // `sender_info_required` only applies to update calls: query - // responses cannot change state, so there is nothing to restrict. + // requests cannot change state, so there is nothing to restrict. let restrictions = validate_request_content( request, self.validator.as_ref(), current_time, root_of_trust_provider, + SenderInfoRequirement::Ignore, )?; validate_request_target(request, &restrictions.targets)?; Ok(restrictions.targets) @@ -212,6 +204,7 @@ where self.validator.as_ref(), current_time, root_of_trust_provider, + SenderInfoRequirement::Ignore, ) .map(|restrictions| restrictions.targets) } @@ -235,11 +228,21 @@ fn validate_paths_width_and_depth(paths: &[Path]) -> Result<(), RequestValidatio Ok(()) } +/// Whether `validate_request_content` should enforce the delegations' +/// `sender_info_required` restriction. Only update calls enforce it: query +/// requests cannot change state, so there is nothing to restrict, and +/// read_state requests cannot even carry a `sender_info`. +enum SenderInfoRequirement { + EnforceIfRequiredByDelegation, + Ignore, +} + fn validate_request_content( request: &HttpRequest, ingress_signature_verifier: &dyn IngressSigVerifier, current_time: Time, root_of_trust_provider: &R, + sender_info_requirement: SenderInfoRequirement, ) -> Result where R::Error: std::error::Error, @@ -258,6 +261,14 @@ where current_time, root_of_trust_provider, )?; + if matches!( + sender_info_requirement, + SenderInfoRequirement::EnforceIfRequiredByDelegation + ) && restrictions.sender_info_required + && request.sender_info().is_none() + { + return Err(SenderInfoRequiredByDelegation); + } validate_sender_info(request, ingress_signature_verifier, root_of_trust_provider)?; Ok(restrictions) }