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

refactor HAIP and add details for mdoc profile of OID4VP over the Browser API #122

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Sakurann
Copy link
Contributor

@Sakurann Sakurann commented Nov 20, 2024

resolves #121
resolves #133

  • mandatory DCQL parameters are tbd
  • in session transcript, removed hashing of the data, and did not introduce mdoc provided nonce (based on the WG discussion on Nov 19th 2024)
  • encryption uses JWE with ECDH as defined in ISO 18013-7

Nov-28-2024

  • updated the text according to the WG discussion. current text should be ready to be merged as a first version.

Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
Co-authored-by: Jan Vereecken <jan.vereecken@hey.com>
* The Credential format identifier MUST be `mso_mdoc`.
* mdoc Credential Format specific DCQL parameters as defined in Section 6.4.2. of [@!OIDF.OID4VP] MUST be used.
* Verifier MAY request more than one credential in the same request.
* DeviceResponse in the VP Token MUST contain only one mdoc. Therefore, when multiple mdocs are being returned, each mdoc should be returned in a separate DeviceResponse, each matching to a respective DCQL query.

Choose a reason for hiding this comment

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

I'm not sure if this is a good limitation, see also the issue with binding a single response to a single DCQL query in openid/OpenID4VP#336

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this restriction makes sense given the constraints we currently have it doesn't make sense to have more than one mdoc in a single VP Token.

(I also agree we should look into that issue you raised.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why shouldn't it be possible to return multiple mdoc device responses in a single vp token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handover = OID4VPDCAPIHandover
OID4VPDCAPIHandover = [
"OID4VPDCAPIHandover"
clientId

Choose a reason for hiding this comment

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

From a security perspective, what kind of attacks are mitigated by inclusion of the clientID in the sessiontranscript?
Inclusion of the clientId also has other issues, like which value is used if there are multiple signatures, or if the wallet ignores RP authentication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that an audience restriction of the device response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we need clientId as audience restriction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Inclusion of the clientId also has other issues, like which value is used if there are multiple signatures, or if the wallet ignores RP authentication.

I think the multiple signature case is probably okay, the clientId applicable to the signature/ecosystem the Wallet/user chose to return a credential from is the one used.

If the wallet is ignoring RP authentication it should probably use the origin as the client id in the same way it would for an unsigned request - which means the origin is then included twice in the handover.

I'm not sure what additional audience restriction clientId is giving us when we already have the origin?

I've opened #135 so we can discuss further.

Copy link
Contributor

@tlodderstedt tlodderstedt Dec 3, 2024

Choose a reason for hiding this comment

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

The wallet must decide which audience a certain credential presentation is intended for, even in case of multiple signatures. It's a selection 1 out of n in case of multiple signatures. I can add text to openid/OpenID4VP#308.

Sakurann and others added 2 commits December 7, 2024 07:36
Co-authored-by: Jan Vereecken <jan.vereecken@hey.com>
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Paul Bastian <paul.bastian@posteo.de>
Copy link
Member

@c2bo c2bo left a comment

Choose a reason for hiding this comment

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

I believe this is in a decent state to start with. Let's merge this bigger PR as a starting point for the restructure and inclusion of mdocs and have smaller scoped issues/PRs for the points that might need further discussion.

Copy link
Collaborator

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

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

I believe this is a good base line

Sakurann and others added 3 commits December 12, 2024 13:42
Co-authored-by: Jan Vereecken <jan.vereecken@hey.com>
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
@Sakurann Sakurann requested a review from javereec December 12, 2024 12:45
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
@@ -214,6 +224,58 @@ This is an example of a Wallet Instance Attestation:
* In the `input-descriptors` object, `id`, `name`, `purpose`, `group`, `format`, and `constraints` properties MUST be supported. In the `constraints` object, `limit_disclosure`, and `fields` properties MUST be supported. In the `fields` object, `path` and `filter` properties MUST be supported. A `path` MUST contain exactly one entry with a static path to a certain claim. A `filter` MUST only contain `type` elements of value `string` and `const` elements.
* In the `submission_requirements` object, `name`, `rule (`pick` only)`, `count`, `from` properties MUST be supported.

# OpenID for Verifiable Presentations over W3C Digital Credentials API

The following requirements apply for both, the Wallet and the Verifier, unless specified otherwise:

Choose a reason for hiding this comment

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

As above, please fix the grammar for all bullet point lists in the document and consider removing "unless specified otherwise" (I don't immediately see where we do specify it otherwise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, the otherwise part intends to say that unless it says "the wallet must... the verifier MAY...", the requirement applies to both wallet and the verifier, which sounds kind of useful clarification?

please fix the grammar for all bullet point lists in the document

I would like to do this in another PR, as it touches the entire document, not just the ones edited in this PR.

Choose a reason for hiding this comment

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

Ah, understood. Then I propose to find a wording in the bullet points to call out explicitly for each point who needs to follow it.

Re the editorial stuff - fine for me.

openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel Fett <mail@danielfett.de>
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
* Issuer identification (as prerequisite for trust management)
* Profile of IETF SD-JWT VC that includes the following aspects
* Status Management of the Credentials, including revocation
* Cryptographic Key Binding
Copy link

Choose a reason for hiding this comment

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

Since we call it cryptographic holder binding in our spec

Suggested change
* Cryptographic Key Binding
* Cryptographic Holder Binding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

we can keep it as cryptographic key binding. i opened a issue here: openid/OpenID4VP#369

Choose a reason for hiding this comment

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

What do you mean with "our spec"? Definitely not in the SD-JWT, as the changelog even says

  • Use the term Key Binding rather than Holder Binding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i put the references, Jan. oid4vp and oid4vci specs use the term cryptographic holder binding

openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
openid4vc-high-assurance-interoperability-profile-1_0.md Outdated Show resolved Hide resolved
@@ -383,6 +443,41 @@ Note: When using this profile with other cryptosuites, it is recommended to be e
</front>
</reference>

Copy link

Choose a reason for hiding this comment

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

Suggested change
<reference anchor="W3C.SRI" target="https://www.w3.org/TR/SRI/">
<front>
<author initials="D." surname="Akhawe" fullname="Devdatta Akhawe">
<organization>
<organizationName>Dropbox, Inc.</organizationName>
</organization>
</author>
<author initials="F." surname="Braun" fullname="Frederik Braun">
<organization>
<organizationName>Mozilla</organizationName>
</organization>
</author>
<author initials="F." surname="Marier" fullname="François Marier">
<organization>
<organizationName>Mozilla</organizationName>
</organization>
</author>
<author initials="J." surname="Weinberger" fullname="Joel Weinberger">
<organization>
<organizationName>Google, Inc.</organizationName>
</organization>
</author>
<title>Subresource Integrity</title>
<date day="23" month="June" year="2016"/>
</front>
</reference>

Comment on lines +257 to +271
OID4VPDCAPIHandover = [
"OID4VPDCAPIHandover"
clientId
origin
nonce
]

clientId = tstr ; using UTF-8
origin = tstr ; using UTF-8
nonce = tstr ; using UTF-8
```

* "OID4VPDCAPIHandover" is a string that identifies the type of handover structure as a security measure to prevent confusion of handovers.
* `clientId` and `nonce` parameters in the Handover MUST be the `client_id` and `nonce` parameters included in the Authorization Request from the Verifier.
* `origin` in the Handover is the origin of the Verifer, obtained from the Web-platform and/or app platform.
Copy link

@awoie awoie Dec 12, 2024

Choose a reason for hiding this comment

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

We should use the hash instead of the raw values to enable remote signature architectures to prevent the remote service to learn more than necessary about the transaction details, i.e., origin, client etc.

Suggested change
OID4VPDCAPIHandover = [
"OID4VPDCAPIHandover"
clientId
origin
nonce
]
clientId = tstr ; using UTF-8
origin = tstr ; using UTF-8
nonce = tstr ; using UTF-8
```
* "OID4VPDCAPIHandover" is a string that identifies the type of handover structure as a security measure to prevent confusion of handovers.
* `clientId` and `nonce` parameters in the Handover MUST be the `client_id` and `nonce` parameters included in the Authorization Request from the Verifier.
* `origin` in the Handover is the origin of the Verifer, obtained from the Web-platform and/or app platform.
```cddl
OID4VPDCAPIHandover = [
"OID4VPDCAPIHandover", ; A fixed identifier for this handover type
OID4VPDCAPIHandoverInfoHash ; Integrity hash of OID4VPDCAPIHandoverInfo
]
clientId = tstr ; UTF-8 encoded string representing the client ID
origin = tstr ; UTF-8 encoded string representing the origin
nonce = tstr ; UTF-8 encoded string representing the nonce
OID4VPDCAPIHandoverInfo = [ clientId, origin, nonce ] ; Array containing handover parameters
OID4VPDCAPIHandoverInfoHash = tstr ; UTF-8 encoded string for the integrity hash of OID4VPDCAPIHandoverInfo
```
`OID4VPDCAPIHandover` parameters:
- The first element MUST be the fixed UTF-8 encoded string `"OID4VPDCAPIHandover"`. This serves as a unique identifier for the handover structure to prevent misinterpretation or confusion.
- The second element MUST be the `OID4VPDCAPIHandoverInfoHash` encoded as a UTF-8 string representing the integrity hash of the `OID4VPDCAPIHandoverInfo` array. The value of `OID4VPDCAPIHandoverInfoHash` MUST comply with the W3C Subresource integrity format as defined in [!W3C.SRI], e.g., `sha256-H8BRh8j48O9oYatfu5AZzq6A9RINhZO5H16dQZngK7T62em8MUt1FLm52t+eX6xO`.
- `client_id` and `nonce` values are UTF-8 encoded strings provided in the Authorization Request from the Verifier.
- `origin` is a UTF-8 encoded string representing the origin of the Verifier. The value for `origin` MUST be obtained from the web platform or app platform being used.

Copy link

Choose a reason for hiding this comment

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

One thing that is potentially missing in OID4VPDCAPIHandoverInfo is the public key of the verifier.

Copy link

@awoie awoie left a comment

Choose a reason for hiding this comment

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

direction looks good. happy to tackle some of my suggestions in dedicated issues.

Co-authored-by: Oliver Terbu <o.terbu@gmail.com>
Co-authored-by: Daniel Fett <mail@danielfett.de>
@Sakurann Sakurann requested a review from awoie December 12, 2024 20:11
Copy link

@awoie awoie left a comment

Choose a reason for hiding this comment

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

Approving first version but SessionTranscript and encryption needs further discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet