Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add utils for processing verifiable presentations #2094

Merged
merged 3 commits into from
Dec 1, 2023
Merged

Conversation

przydatek
Copy link
Collaborator

@przydatek przydatek commented Dec 1, 2023

Add to vc_util a function verify_ii_presentation_jwt_with_canister_ids to verify presentations that are delivered to RP during the attribute sharing flow on the IC.

@przydatek przydatek marked this pull request as ready for review December 1, 2023 12:34
Copy link
Contributor

@frederikrothenberger frederikrothenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it fits this PR, but I think we should tighten the JWS validation, see comments.

Otherwise, LGTM! Thanks.

@@ -184,6 +296,10 @@ fn extract_id_alias(claims: &JwtClaims<Value>) -> Result<AliasTuple, JwtValidati
let id_alias = principal_for_did(alias).map_err(|_| {
inconsistent_jwt_claims("malformed \"has_id_alias\" claim in id_alias JWT vc")
})?;
println!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left over from debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, indeed. Removed now.

.ok_or(PresentationVerificationError::Unknown(
"missing id_alias vc".to_string(),
))?;
let alias_tuple = get_verified_id_alias_from_jws(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking again at get_verified_id_alias_from_jws, I think it should check exp as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but for this we'd need generation of test-data on-the-fly (or some long-lived test VCs), so I've filed a ticket for that.

.ok_or(PresentationVerificationError::Unknown(
"missing requested vc".to_string(),
))?;
let claims = verify_credential_jws_with_canister_id(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above verify_credential_jws_with_canister_id should check iss and exp as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed for exp, not so sure for iss, as this depends on the context. I added a note to the ticket.

Copy link
Collaborator Author

@przydatek przydatek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @frederikrothenberger , will merge when CI is happy.

@@ -184,6 +296,10 @@ fn extract_id_alias(claims: &JwtClaims<Value>) -> Result<AliasTuple, JwtValidati
let id_alias = principal_for_did(alias).map_err(|_| {
inconsistent_jwt_claims("malformed \"has_id_alias\" claim in id_alias JWT vc")
})?;
println!(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, indeed. Removed now.

.ok_or(PresentationVerificationError::Unknown(
"missing id_alias vc".to_string(),
))?;
let alias_tuple = get_verified_id_alias_from_jws(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but for this we'd need generation of test-data on-the-fly (or some long-lived test VCs), so I've filed a ticket for that.

.ok_or(PresentationVerificationError::Unknown(
"missing requested vc".to_string(),
))?;
let claims = verify_credential_jws_with_canister_id(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed for exp, not so sure for iss, as this depends on the context. I added a note to the ticket.

@przydatek przydatek enabled auto-merge December 1, 2023 15:29
@przydatek przydatek added this pull request to the merge queue Dec 1, 2023
Merged via the queue into main with commit 7ebdaa1 Dec 1, 2023
52 of 53 checks passed
@przydatek przydatek deleted the bartosz/vp-utils branch December 1, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants