-
Notifications
You must be signed in to change notification settings - Fork 37
add credential format specific sections for IAR endpoint binding in VPs #602
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need extra line before examples for compilng
| OpenID4VCIIAEHandoverBytes = bstr .cbor OpenID4VCIIAEHandoverInfo | ||
| OpenID4VCIIAEHandoverInfo = [ | ||
| iaeUrl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent that this is iaeUrl, but we prefix with iar elsewhere. We should be consistent (don't care which).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't prefix with origin: in DC API either. My thought was that the handover type (i.e. OpenID4VCIIAEHandover) in the SessionTranscript would replace the prefixing and this is the way we provide the context on the flow for session transcripts across flows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed on today's WG call: consensus to rename "iar:" prefix to "iae:"
| 1. The URL of the Interactive Authorization Request endpoint becomes the Origin for the request; i.e., the Wallet MUST ensure that `expected_origins` contains the Interactive Authorization Request endpoint URL. | ||
| 2. The audience in the response (for example, the `aud` value in a Key Binding JWT) MUST be the Interactive Authorization Request, prefixed with `iar:`, for example `iar:https://example.com/iar`. A response containing a different audience value MUST NOT be accepted. | ||
| 3. If a `SessionTranscript` is needed, it is generated according Appendix B.2.6.2 of [@!OpenID4VP]. As above, the value for origin is the Interactive Authorization Request endpoint URL. | ||
| 1. The URL of the Interactive Authorization Endpoint becomes the Origin for the request; i.e., the Wallet MUST ensure that `expected_origins` contains the Interactive Authorization Endpoint URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments:
- since expected_origins is optional (or conditional), should we add "if present"?
- expected_origins can only contain origins, not URLs. While they can match, they don't always match. Is the idea that the expected_origins is converted by the wallet before matching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the current definition of Origin and expected_origins in OID4VP clashes with this normative statement. expected_origins contains origin values and is conditional based on if the request was signed. If somebody wants to write a parser for requests, this would be prone to errors since with the current text both, URLs and origins, are allowed.
I also agree that it would be cleaner to say "if present" although I don't know whether this was intentional and part of the original threat mitigation model specifically for IAE requests. I believe it wasn't, so I'm fine with adding "if present".
What we could do (non-breaking):
- Keep using
expected_originsand use the origin part of the URL only. Probably has some security implications but perhaps this is sufficient. For OID4VP it was sufficient to check the origin and not the full URL. - same as 1. (since required for signed requests) but additionally define a new optional parameter for the request for signed requests with similar logic to
expected_origins, i.e.,expected_urls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed on today's WG call - seem to have a consensus for option 1
|
I updated the PR:
I kept using the IAE (URL) for audience and SessionTranscript binding. However, OID4VP has this for DC API:
I'm not sure if the current language in this PR would clash with this definition, thoughts? @GarethCOliver @jogu @martijnharing @danielfett. If it does, I can update this PR to use the derived Origin also for the audience and session transcript binding. |
For a proposal on how to use the derived origin instead of the endpoint for the binding, see this PR: #629 |
|
WG discussion:
|
0ee5a46 to
08b7bc8
Compare
Is it clear enough that Credential Formats correspond to the requested Credential in the IAR, and not to the requested Credential in the VCI request? I did not update the audience in the VP, but propose a fix in £629 al here fix: just do derived origin binding #629 fixes #590 fixes #620 (changes by Oliver, moved from the 1.0 to 1.1 spec by Joseph)
08b7bc8 to
11451b5
Compare
|
I've updated this PR to target the 1.1 spec - please review. As per @Sakurann's comment above, we'll discuss whether to move some of this to the VP spec and will separately handled the discussion about origin vs url in #629, so please ignore those two aspects when reviewing this so we can get the rest of this PR merged (as we have further IAE PRs to create and want to reduce the potential for conflicts). |
This PR fixes #590 and #620 by adding credential format specific sections for IAR endpoint binding in VPs.
SessionTranscriptneed to be regenerated