-
Notifications
You must be signed in to change notification settings - Fork 37
Add session transcript for redirect-based oid4vp flow #610
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
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.
For info for others, this is different to the handover structure in 18013-7 annex b, but it's quite closely aligned with what we have for DC API. To me the difference looks like origin is replaced with client_id + response_uri, which seems reasonable.
Not 100% sure I'm a fan of the weird characters in the spec where the sha256 contents is shown as a string in the cbor diagnostics, but we have that for DC API and it seems to render fine so I guess it's okay.
tplooker
left a comment
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.
LGTM
|
@andprian I updated the PR again to clarify that all referenced request parameters (also note that JAR says the Request Object contains authorization request parameters hence it is ok to refer to them as request parameters), i.e., nonce, client_id, response_uri, redirect_uri, have to be obtained from the authz request query params unless the request is signed and then the params have to obtained from the signed request object. |
c2bo
left a comment
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.
PR looks fine and I quickly skimmed over the CBOR content and that looks good to me.
One thing that feels a bit weird is the asymmetry between the 2 session transcripts (DC API and non-DC API).
| ### `Handover` and `SessionTranscript` Definitions | ||
|
|
||
| #### Invocation via the Digital Credentials API | ||
| #### Invocation via Redirects |
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.
please help bikeshed the title
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.
Any objections to using the term "invocation via redirects"? I vote for "Invocation via Redirects" since we use that terminology already.
Possible alternatives:
- Invocation via Authorization Endpoint
- Invocation via URL
- Invocation via Vanilla OpenID4VP
| OpenID4VPHandoverInfoBytes = bstr .cbor OpenID4VPHandoverInfo | ||
| OpenID4VPHandoverInfo = [ | ||
| clientId, |
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.
Including clientId will break all the multi-auth cases, right? (at least without just 'trying all of them').
Essentially the same general problem we had with DC API until we removed it, no?
It's possible that just relying on responseUri (and remove clientId from here) solves this, otherwise we need to either communicate which clientId was used, or just say 'try them all'.
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.
multi-auth is currently not supported for the vanilla flows (only DC API), but yes it would break them.
That is one of the things that feel odd between DC API and vanilla flow right now to me.
|
WG discussion:
|
This PR fixes #402
Also beautified CDDL (editorial)