From ff252c40bb3bb418d76ed3f84f0b0fa3e7b46f15 Mon Sep 17 00:00:00 2001 From: przydatek Date: Fri, 12 Jan 2024 17:19:45 +0100 Subject: [PATCH] Add to get_verified_id_alias_from_jws a check for the expected vc subject (#2199) * Add to get_verified_id_alias_from_jws a check for the expected vc subject * Update comment * Adjust tests * Adjust tests, for real * Address review comments --- demos/vc_issuer/src/main.rs | 22 +++------------ demos/vc_issuer/tests/issue_credential.rs | 7 ++--- src/vc_util/src/custom.rs | 2 +- src/vc_util/src/lib.rs | 33 +++++++++++------------ 4 files changed, 24 insertions(+), 40 deletions(-) diff --git a/demos/vc_issuer/src/main.rs b/demos/vc_issuer/src/main.rs index cf0aaf5b12..2e5d673ec1 100644 --- a/demos/vc_issuer/src/main.rs +++ b/demos/vc_issuer/src/main.rs @@ -152,22 +152,7 @@ fn apply_config(init: IssuerInit) { fn authorize_vc_request( alias: &SignedIdAlias, - current_time_ns: u128, -) -> Result { - let alias_tuple = extract_id_alias(alias, current_time_ns)?; - - if caller() != alias_tuple.id_dapp { - return Err(IssueCredentialError::UnauthorizedSubject(format!( - "Caller {} does not match id alias dapp principal {}.", - caller(), - alias_tuple.id_dapp - ))); - } - Ok(alias_tuple) -} - -fn extract_id_alias( - alias: &SignedIdAlias, + expected_vc_subject: &Principal, current_time_ns: u128, ) -> Result { CONFIG.with_borrow(|config| { @@ -176,6 +161,7 @@ fn extract_id_alias( for idp_canister_id in &config.idp_canister_ids { if let Ok(alias_tuple) = get_verified_id_alias_from_jws( &alias.credential_jws, + expected_vc_subject, idp_canister_id, &config.ic_root_key_raw, current_time_ns, @@ -194,7 +180,7 @@ fn extract_id_alias( async fn prepare_credential( req: PrepareCredentialRequest, ) -> Result { - let alias_tuple = match authorize_vc_request(&req.signed_id_alias, time().into()) { + let alias_tuple = match authorize_vc_request(&req.signed_id_alias, &caller(), time().into()) { Ok(alias_tuple) => alias_tuple, Err(err) => return Err(err), }; @@ -246,7 +232,7 @@ fn update_root_hash() { #[query] #[candid_method(query)] fn get_credential(req: GetCredentialRequest) -> Result { - let alias_tuple = match authorize_vc_request(&req.signed_id_alias, time().into()) { + let alias_tuple = match authorize_vc_request(&req.signed_id_alias, &caller(), time().into()) { Ok(alias_tuple) => alias_tuple, Err(err) => return Result::::Err(err), }; diff --git a/demos/vc_issuer/tests/issue_credential.rs b/demos/vc_issuer/tests/issue_credential.rs index 1320edc5fd..67d6dc6354 100644 --- a/demos/vc_issuer/tests/issue_credential.rs +++ b/demos/vc_issuer/tests/issue_credential.rs @@ -380,7 +380,7 @@ fn should_fail_prepare_credential_for_wrong_sender() { ) .expect("API call failed"); assert_matches!(response, - Err(IssueCredentialError::UnauthorizedSubject(e)) if e.contains(&format!("Caller {} does not match id alias dapp principal {}.", principal_1(), DUMMY_ALIAS_ID_DAPP_PRINCIPAL)) + Err(IssueCredentialError::InvalidIdAlias(e)) if e.contains("id alias could not be verified") ); } @@ -417,7 +417,7 @@ fn should_fail_get_credential_for_wrong_sender() { ) .expect("API call failed"); assert_matches!(get_credential_response, - Err(IssueCredentialError::UnauthorizedSubject(e)) if e.contains(&format!("Caller {} does not match id alias dapp principal {}.", unauthorized_principal, authorized_principal)) + Err(IssueCredentialError::InvalidIdAlias(e)) if e.contains("id alias could not be verified") ); } @@ -436,7 +436,7 @@ fn should_fail_prepare_credential_for_anonymous_caller() { ) .expect("API call failed"); assert_matches!(response, - Err(IssueCredentialError::UnauthorizedSubject(e)) if e.contains(&format!("Caller 2vxsx-fae does not match id alias dapp principal {}.", DUMMY_ALIAS_ID_DAPP_PRINCIPAL)) + Err(IssueCredentialError::InvalidIdAlias(e)) if e.contains("id alias could not be verified") ); } @@ -570,6 +570,7 @@ fn should_issue_credential_e2e() -> Result<(), CallError> { &id_alias_credentials .issuer_id_alias_credential .credential_jws, + &id_alias_credentials.issuer_id_alias_credential.id_dapp, &canister_sig_pk.canister_id, &root_pk_raw, env.time().duration_since(UNIX_EPOCH).unwrap().as_nanos(), diff --git a/src/vc_util/src/custom.rs b/src/vc_util/src/custom.rs index cd2d56e17a..c5f0dbefa3 100644 --- a/src/vc_util/src/custom.rs +++ b/src/vc_util/src/custom.rs @@ -196,7 +196,7 @@ mod tests { &test_ic_root_pk_raw(), CURRENT_TIME_BEFORE_EXPIRY_NS, ); - assert_matches!(result, Err(e) if format!("{:?}", e).to_string().contains("holder does not match subject")); + assert_matches!(result, Err(e) if format!("{:?}", e).to_string().contains("unexpected vc subject")); } #[test] diff --git a/src/vc_util/src/lib.rs b/src/vc_util/src/lib.rs index 3be5786176..940001d8a0 100644 --- a/src/vc_util/src/lib.rs +++ b/src/vc_util/src/lib.rs @@ -117,6 +117,7 @@ pub fn principal_for_did(did: &str) -> Result { /// validation of the claims in the VC. pub fn get_verified_id_alias_from_jws( credential_jws: &str, + expected_vc_subject: &Principal, signing_canister_id: &Principal, root_pk_raw: &[u8], current_time_ns: u128, @@ -130,7 +131,14 @@ pub fn get_verified_id_alias_from_jws( .map_err(CredentialVerificationError::InvalidJws)?; validate_claim("iss", II_ISSUER_URL, claims.iss()) .map_err(CredentialVerificationError::InvalidClaims)?; - extract_id_alias(&claims).map_err(CredentialVerificationError::InvalidClaims) + let alias_tuple = + extract_id_alias(&claims).map_err(CredentialVerificationError::InvalidClaims)?; + if *expected_vc_subject != alias_tuple.id_dapp { + return Err(CredentialVerificationError::InvalidClaims( + inconsistent_jwt_claims("unexpected vc subject"), + )); + } + Ok(alias_tuple) } /// Verifies the specified JWS credential cryptographically and checks that the signature was @@ -192,11 +200,12 @@ fn parse_verifiable_presentation_jwt(vp_jwt: &str) -> Result, /// Verifies the specified JWT presentation cryptographically, which should contain exactly /// two verifiable credentials (in the order specifed): -/// 1. An id_alias credential which links the holder of the VP to a temporary id_alias. +/// 1. An "Id alias" credential which links the effective subject of the VP to a temporary id_alias. /// This credential should be signed by canister vc_flow_parties.ii_canister_id. /// 2. An actual credential requested by a user. The subject of this credential is id_alias, /// and it should be signed by canister vc_flow_parties.issuer_canister_id -/// Returns holder's identity together with id_alias, and the claims from the requested credential. +/// Verifies that the subject of the first credential matches `effective_vc_subject`. +/// Returns the verified `effective_vc_subject` with id_alias, and the claims from the requested credential. /// DOES NOT perform semantic validation of the returned claims. pub fn verify_ii_presentation_jwt_with_canister_ids( vp_jwt: &str, @@ -217,25 +226,12 @@ pub fn verify_ii_presentation_jwt_with_canister_ids( )?; let alias_tuple = get_verified_id_alias_from_jws( id_alias_vc_jws.as_str(), + &effective_vc_subject, &vc_flow_signers.ii_canister_id, root_pk_raw, current_time_ns, ) .map_err(PresentationVerificationError::InvalidIdAliasCredential)?; - // TODO: change `get_verified_id_alias_from_jws` to additionally take `effective_vc_subject` - // as an argument and to perform the check below internally. - // Also, consider adding `ii_origin`-argument to enable custom values for II-origin, - // and extend should_fail_validate_verified_adult_presentation_if_wrong_vc_flow_signers() - // if needed. - if effective_vc_subject != alias_tuple.id_dapp { - return Err(PresentationVerificationError::InvalidPresentationJwt( - format!( - "holder does not match subject: expected {}, got {}", - effective_vc_subject, alias_tuple.id_dapp - ) - .to_string(), - )); - } let requested_vc_jws = presentation .verifiable_credential @@ -692,6 +688,7 @@ mod tests { fn should_verify_and_extract_id_alias_credential_jws() { let alias_tuple = get_verified_id_alias_from_jws( ID_ALIAS_CREDENTIAL_JWS, + &dapp_principal(), &test_canister_sig_pk().canister_id, &test_ic_root_pk_raw(), CURRENT_TIME_BEFORE_EXPIRY_NS, @@ -843,7 +840,7 @@ mod tests { &test_ic_root_pk_raw(), CURRENT_TIME_BEFORE_EXPIRY_NS, ); - assert_matches!(result, Err(e) if format!("{:?}", e).contains("holder does not match subject")); + assert_matches!(result, Err(e) if format!("{:?}", e).contains("unexpected vc subject")); } #[test]