-
Notifications
You must be signed in to change notification settings - Fork 37
Implement signature verification requirements #598
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
Sakurann
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.
this is not my understanding of what we agreed to add to the specification.
I am not ok with changing processing rules for each of the client id prefixes. I was expecting one paragraph on "it is up to the ecosystem to decide whether to signature validation is mandatory or not" that applies to al client id schemes
|
WG discussion as documented here: #561 (comment) |
tlodderstedt
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.
The currently proposed text does not distinguish between DC API and vanilla OpenID4VP. Made a suggestion how it could be modified to differentiate.
|
|
||
| * `format`: REQUIRED. A string that identifies the format of the attestation and how it is encoded. Ecosystems SHOULD use collision-resistant identifiers. Further processing of the attestation is determined by the type of the attestation, which is specified in a format-specific way. | ||
| * `data`: REQUIRED. An object or string containing an attestation (e.g. a JWT). The payload structure is defined on a per format level. The Wallet MUST validate this signature and ensure binding. | ||
| * `data`: REQUIRED. An object or string containing an attestation (e.g. a JWT). The payload structure is defined on a per format level. Whether the Wallet uses the information from the verifier_attestation is out of scope of this specification, but if the Wallet uses the information from the verifier attestation the Wallet MUST validate this signature and ensure binding. |
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.
| * `data`: REQUIRED. An object or string containing an attestation (e.g. a JWT). The payload structure is defined on a per format level. Whether the Wallet uses the information from the verifier_attestation is out of scope of this specification, but if the Wallet uses the information from the verifier attestation the Wallet MUST validate this signature and ensure binding. | |
| * `data`: REQUIRED. An object or string containing an attestation (e.g. a JWT). The payload structure is defined on a per format level. In case of using OpenID4VP over DC API it is at the discretion of the wallet whether it uses the information from the verifier_attestation, but if the Wallet uses the information from the verifier attestation the Wallet MUST validate this signature and ensure binding. |
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 verifier_attestations, I would agree with Martijn that this is true for both cases - how to deal with the additional information provided within those should be defined on an ecosystem level imho? (this section is about verifier_attestations, not verifier_attestation)
For the client id schemes/prefixes I would agree that at least for the time being we need to differentiate between vanilla and DC API and enforce validation for vanilla flows and leave more freedom for DC API.
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.
Clarified the text to bring it in line with the client identifier text, but did not make it only applicable in the context of the DC API.
Make client id verification requirements dependent on the DC API and make them applicable to all client ids
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
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'm generally ok with this direction. Editorially, I think the DC API exception should go into the DC API Annex since the Annex is supposed to be self-contained.
Something like this:
The client_id parameter MUST be omitted in unsigned requests defined in Appendix A.3.1. The Wallet MUST ignore any client_id parameter that is present in an unsigned request.
Parameters defined by a specific Client Identifier Prefix (such as the trust_chain parameter for the OpenID Federation Client Identifier Prefix) are also supported over the W3C Digital Credentials API.
The client_id parameter MUST be present in signed requests defined in Appendix A.3.2, as it communicates to the wallet which Client Identifier Prefix and Client Identifier to use when authenticating the client through verification of the request signature or retrieving client metadata.
NEW:
In case of using OpenID4VP over DC API as defined in (#dc_api) it is at the discretion of the Wallet (for example based on policies from ecosystems, profiles, issuers or Holders) whether it uses the information from a client identifier. If the Wallet uses information from a client identifier, the Wallet must perform all the verification requirements for that client identifier as defined below. If it doesn't use the information from a client identifier, the Wallet does not need to implement the verification requirements for that client identifier as defined below.
tlodderstedt
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.
Will approve pending Kristina's proposed change is incorporated.
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.
+1 for Kristina's suggestions. I don't think it's super important but I'd also agree with Oliver that having the DC API specific parts in the DCP section makes sense.
|
wg discussion |
Implement review suggestions Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
Minor edit Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
awoie
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.
approving but we need to update doc history.
Resolves #561