Authz/token validator #11
Conversation
MrWoo034
requested review from
ncpenke and
jeffgrunewald
and removed request for
jeffgrunewald
| /// invalid without any further verification. | ||
| /// **Note**: this limit is part of the [IC specification](https://internetcomputer.org/docs/current/references/ic-interface-spec#authentication) | ||
| /// and so changing this value might be breaking or result in a deviation from the specification. | ||
| const MAXIMUM_NUMBER_OF_TARGETS_PER_DELEGATION: usize = 1_000; |
There was a problem hiding this comment.
if we're worried about this deviating is there any function or constant exported in any of the IC crates we import that we can draw this value from so we can just implicitly continue to follow the spec even if it changes?
| pub enum RequestValidationError { | ||
| InvalidIngressExpiry(String), | ||
| InvalidDelegationExpiry(String), | ||
| UserIdDoesNotMatchPublicKey(UserId, Vec<u8>), | ||
| InvalidSignature(AuthenticationError), | ||
| InvalidDelegation(AuthenticationError), | ||
| MissingSignature(UserId), | ||
| AnonymousSignatureNotAllowed, | ||
| CanisterNotInDelegationTargets(CanisterId), | ||
| TooManyPathsError { length: usize, maximum: usize }, | ||
| PathTooLongError { length: usize, maximum: usize }, | ||
| NonceTooBigError { num_bytes: usize, maximum: usize }, |
There was a problem hiding this comment.
can we use the thiserror #[error()] macro to define the display impls for these?
| pub enum RequestValidationError { | |
| InvalidIngressExpiry(String), | |
| InvalidDelegationExpiry(String), | |
| UserIdDoesNotMatchPublicKey(UserId, Vec<u8>), | |
| InvalidSignature(AuthenticationError), | |
| InvalidDelegation(AuthenticationError), | |
| MissingSignature(UserId), | |
| AnonymousSignatureNotAllowed, | |
| CanisterNotInDelegationTargets(CanisterId), | |
| TooManyPathsError { length: usize, maximum: usize }, | |
| PathTooLongError { length: usize, maximum: usize }, | |
| NonceTooBigError { num_bytes: usize, maximum: usize }, | |
| pub enum RequestValidationError { | |
| #[error("invalid ingress expiry {0}")] | |
| InvalidIngressExpiry(String), | |
| #[error("invalid delegation expiry {0}")] | |
| InvalidDelegationExpiry(String), | |
| ... |
|
|
||
| /// Error in verifying the signature or authentication part of a request. | ||
| #[derive(Debug, PartialEq, thiserror::Error)] | ||
| pub enum AuthenticationError { |
| "Specified ingress_expiry not within expected range: \ | ||
| Minimum allowed expiry: {}, \ | ||
| Maximum allowed expiry: {}, \ | ||
| Provided expiry: {}", |
There was a problem hiding this comment.
the variability in these different error messages feels like maybe the InvalidIngressExpiry needs to be split into a couple more specific errors? like IngressExpiryTooEarly, IngressExpiryTooLate etc which would allow for moving a lot of the message of the error into the macro-based display string for the error and allow for more targeted handling of the different errors through matching in the caller?
| } else { | ||
| Err(UserIdDoesNotMatchPublicKey( | ||
| UserId::new(id), | ||
| sender_pubkey.to_vec(), |
There was a problem hiding this comment.
not sure if the error display impl allows for the hex encoding of these bytes but unless you want to be able to match on this error and make use of the raw bytes of the pubkey in the error handling code, why not just do the display-encoding here and then interpolate that value into the error message without encoding it there?
| DelegationContainsCyclesError { public_key } => write!( | ||
| f, | ||
| "Chain of delegations contains at least one cycle: first repeating public key encountered {}", | ||
| hex::encode(public_key) |
There was a problem hiding this comment.
especially if these are ultimately ed25519 public keys it might be more conventional to base58 code this when displaying
| pubkey: Vec<u8>, | ||
| root_of_trust_provider: &R, | ||
| ) -> Result<(), 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)?; | ||
|
|
||
| for sd in signed_delegations { | ||
| let delegation = sd.delegation(); | ||
| let signature = sd.signature(); | ||
|
|
||
| validate_delegation( | ||
| validator, | ||
| signature, | ||
| delegation, | ||
| &pubkey, |
There was a problem hiding this comment.
| pubkey: Vec<u8>, | |
| root_of_trust_provider: &R, | |
| ) -> Result<(), 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)?; | |
| for sd in signed_delegations { | |
| let delegation = sd.delegation(); | |
| let signature = sd.signature(); | |
| validate_delegation( | |
| validator, | |
| signature, | |
| delegation, | |
| &pubkey, | |
| pubkey: &[u8], | |
| root_of_trust_provider: &R, | |
| ) -> Result<(), 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)?; | |
| for sd in signed_delegations { | |
| let delegation = sd.delegation(); | |
| let signature = sd.signature(); | |
| validate_delegation( | |
| validator, | |
| signature, | |
| delegation, | |
| pubkey, |
| } | ||
|
|
||
| fn validate_token_user_id_and_signature(token: &Token) -> Result<(), RequestValidationError> { | ||
| let _signature = &token.signature; |
No description provided.