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

Wallet Attestation: Option with new request/response parameters #318

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

c2bo
Copy link
Member

@c2bo c2bo commented Nov 12, 2024

Relevant issue #141.
This is the option of new dedicated request & response parameters.

@peppelinux
Copy link
Member

interesting approach, my points below:

  1. How can an RP request a specific wallet attestation over another, given that wallet attestations are tied to the trust frameworks in use and, like digital credentials, may have varying schemes and identifiers?

  2. This PR would benefit from an example where the presentation_submission includes a Wallet Attestation, specifying to the Verifier the index position within the vp_token array where the Wallet Attestation is provided.

  3. Considering point 1, why isn't the Wallet Attestation included in the presentation_definition/duckle query like other digital credentials? Why would using a request parameter be considered superior to a standard presentation query?

I support this work

@c2bo
Copy link
Member Author

c2bo commented Nov 14, 2024

interesting approach, my points below:

  1. How can an RP request a specific wallet attestation over another, given that wallet attestations are tied to the trust frameworks in use and, like digital credentials, may have varying schemes and identifiers?
  2. This PR would benefit from an example where the presentation_submission includes a Wallet Attestation, specifying to the Verifier the index position within the vp_token array where the Wallet Attestation is provided.
  3. Considering point 1, why isn't the Wallet Attestation included in the presentation_definition/duckle query like other digital credentials? Why would using a request parameter be considered superior to a standard presentation query?

I support this work

There are 2 approaches right now

  • special parameters for request/response (this also means attestation is not in vp_token, but directly in the dedicated response paramter) -> this is the option I've tried to describe in this PR and was the one that most people agreed with the last time this was discussed
  • integregate wallet attestation into dcql query and vp_token. I am planning to do a second draft PR with this option (which I personally prefer). I will likely choose the variant with a dedicated request parameter that links to a credential_id in the dcql request as described here: Verifier/Relying Parties need to be able to verify authenticity and validity of (European Digital Identity) Wallets #141 (comment)

Comment on lines +988 to +989
`wallet_attestation`:
: REQUIRED if the `wallet_attestation` parameter was present and its value `true` in the Authorization Request (see (#vp_token_request) for more details). The Wallet Attestation presentation MUST be bound to the nonce that was provided in the Authorization Request. The expected format of a Wallet Attestation and validation rules are defined in section TODO of [@!OpenID.VCI]. If the validation of the Wallet Attestation fails, the response is rejected. Additional claims for individual implementations and ecosystems can be added to a Wallet Attestation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`wallet_attestation`:
: REQUIRED if the `wallet_attestation` parameter was present and its value `true` in the Authorization Request (see (#vp_token_request) for more details). The Wallet Attestation presentation MUST be bound to the nonce that was provided in the Authorization Request. The expected format of a Wallet Attestation and validation rules are defined in section TODO of [@!OpenID.VCI]. If the validation of the Wallet Attestation fails, the response is rejected. Additional claims for individual implementations and ecosystems can be added to a Wallet Attestation.
`wallet_attestation`:
: REQUIRED if the `wallet_attestation` parameter was present and its value `true` in the Authorization Request (see (#vp_token_request) for more details). The Wallet Attestation presentation MUST be bound to the nonce that was provided in the Authorization Request. The expected format of a Wallet Attestation and validation rules are defined in [@!OpenID.VCI]. If the validation of the Wallet Attestation fails, the response is rejected. Additional claims for individual implementations and ecosystems can be added to a Wallet Attestation.

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan was to wait for the merge of the VCI PR and adjust this PR with the correct section

@martijnharing
Copy link

I don't see the benefit of the approach from this PR over using using the mechanisms that are already present (e.g. DCQL) for requesting wallet attestation. As far as I'm aware using the request and response logic using existing credentials can support all the features required for the wallet attestation use case without the need of any special handling.

Using the approach in this PR doesn't support certain features that the DCQL does support like making queries that say "for this credential I need a wallet attestation, but for this one I don't" or support of wallet attestations in different formats than the one considered in the OpenID4VCI spec. We could of course add these features, but then we are just making another version of DCQL.

It was mentioned that one of the reasons to make a special parameter was that it allows wallets to treat the request for wallet attestation different than other credentials. But since wallet UI / UX is out of scope of the specification there is nothing in the current specification that stops a wallet from implementing that logic based on the credential type indicator that is already present in DCQL (e.g. vct_values).

Co-authored-by: Giuseppe De Marco <demarcog83@gmail.com>
@c2bo
Copy link
Member Author

c2bo commented Nov 19, 2024

I don't see the benefit of the approach from this PR over using using the mechanisms that are already present (e.g. DCQL) for requesting wallet attestation. As far as I'm aware using the request and response logic using existing credentials can support all the features required for the wallet attestation use case without the need of any special handling.

Using the approach in this PR doesn't support certain features that the DCQL does support like making queries that say "for this credential I need a wallet attestation, but for this one I don't" or support of wallet attestations in different formats than the one considered in the OpenID4VCI spec. We could of course add these features, but then we are just making another version of DCQL.

It was mentioned that one of the reasons to make a special parameter was that it allows wallets to treat the request for wallet attestation different than other credentials. But since wallet UI / UX is out of scope of the specification there is nothing in the current specification that stops a wallet from implementing that logic based on the credential type indicator that is already present in DCQL (e.g. vct_values).

I wouldn't agree that UI/UX is fully out of scope - there are enough parts in the OpenID4VC protocols where certain aspects were introduced to make a better UX possible. Examples being things like the display metadata in VCI & new proof type to avoid user interaction for re-iussuance in VCI.

On the topic: I do believe there should be some kind of part of the request (whether a request parameter or part of DCQL) that marks an attestation request explicitly as such. At the end of the day, a Wallet needs to be able to deal with an attestation accordingly, but we should rather help implementations to fail fast if they do now know an attestation by making it explicit imho.

@martijnharing
Copy link

I don't see the benefit of the approach from this PR over using using the mechanisms that are already present (e.g. DCQL) for requesting wallet attestation. As far as I'm aware using the request and response logic using existing credentials can support all the features required for the wallet attestation use case without the need of any special handling.
Using the approach in this PR doesn't support certain features that the DCQL does support like making queries that say "for this credential I need a wallet attestation, but for this one I don't" or support of wallet attestations in different formats than the one considered in the OpenID4VCI spec. We could of course add these features, but then we are just making another version of DCQL.
It was mentioned that one of the reasons to make a special parameter was that it allows wallets to treat the request for wallet attestation different than other credentials. But since wallet UI / UX is out of scope of the specification there is nothing in the current specification that stops a wallet from implementing that logic based on the credential type indicator that is already present in DCQL (e.g. vct_values).

I wouldn't agree that UI/UX is fully out of scope - there are enough parts in the OpenID4VC protocols where certain aspects were introduced to make a better UX possible. Examples being things like the display metadata in VCI & new proof type to avoid user interaction for re-iussuance in VCI.

On the topic: I do believe there should be some kind of part of the request (whether a request parameter or part of DCQL) that marks an attestation request explicitly as such. At the end of the day, a Wallet needs to be able to deal with an attestation accordingly, but we should rather help implementations to fail fast if they do now know an attestation by making it explicit imho.

You are correct, we definitely design features to support good UI/UX. I meant that there isn't an explicit requirement to actually implement that in a certain way. And In this particular case the credential type (vct_value, doctype) achieves the goal of identifying that something is a credential attestation.

@David-Chadwick
Copy link
Contributor

The other defining feature is that the subject of the credential is the wallet app, and not the user. Given that wallets should be designed to hold credentials of multiple subjects e.g. children, pets, wallet apps etc. then any good wallet GUI will have tabs for the different subjects so that the holder can see which credentials s/he holds for whom.
For this reason I can see no good reason to differentiate the wallet app credential from any other credential.

Copy link
Collaborator

@tlodderstedt tlodderstedt left a comment

Choose a reason for hiding this comment

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

lgtm

@tlodderstedt
Copy link
Collaborator

then any good wallet GUI will have tabs for the different subjects so that the holder can see which credentials s/he holds for whom.

You are making a good point. I would not assume that the user interface shows wallet attestations to the user, it's a technical mechanism like RP credentials to ensure the security of the ecosystem. To me this is a good reason to treat the wallet attestation differently.

@David-Chadwick
Copy link
Contributor

I would not assume that the user interface shows wallet attestations to the user, it's a technical mechanism like RP credentials to ensure the security of the ecosystem. To me this is a good reason to treat the wallet attestation differently.

On the contrary, I think it is a very good idea to show this to the user since it will help to provide the user with confidence and trust in the wallet app, and will differentiate this wallet app from a cheap substitute (that isn't certified). After all, consumers like to see trust marks of various kinds on physical items that they purchase, to give them confidence that they are safe to use, so why not for virtual items that they obtain?

@@ -310,6 +313,9 @@ The following is a non-normative example of a transaction data content, after ba
}
```

`wallet_attestation`:
: OPTIONAL. A boolean value that indicates whether or not a Wallet Attestation is requested by the Verifier. If the parameter is not present, the default value is `false`. If the value is `true`, a Wallet Attestation is expected to be part of the response (see `wallet_attestation` in (#response-parameters)).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this approach if we can ensure that we leave the door open to later (in future versions) add a DCQL query or other complex query here if needed.

Not sure if this is the best way:

Suggested change
: OPTIONAL. A boolean value that indicates whether or not a Wallet Attestation is requested by the Verifier. If the parameter is not present, the default value is `false`. If the value is `true`, a Wallet Attestation is expected to be part of the response (see `wallet_attestation` in (#response-parameters)).
: OPTIONAL. A value that indicates whether or not a Wallet Attestation is requested by the Verifier. If the parameter is not present, or is `false`, no Wallet Attestation is requested. Otherwise, a Wallet Attestation is expected to be part of the response (see `wallet_attestation` in (#response-parameters)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add something like "Note: Instead of offering just the boolean values true/false, extensions of this specifications may define custom queries to be used in this parameter. Therefore, any value other than false MUST be treated as a request for a Wallet Attestation."

@@ -978,6 +985,9 @@ If Presentation Exchange was used for the request, the `presentation_submission`

Including the `presentation_submission` parameter as a separate response parameter allows the Wallet to provide the Verifier with additional information about the format and structure in advance of the processing of the VP Token, and can be used even with the Credential formats that do not allow for the direct inclusion of `presentation_submission` parameters inside a Credential itself.

`wallet_attestation`:
: REQUIRED if the `wallet_attestation` parameter was present and its value `true` in the Authorization Request (see (#vp_token_request) for more details). The Wallet Attestation presentation MUST be bound to the nonce that was provided in the Authorization Request. The expected format of a Wallet Attestation and validation rules are defined in section TODO of [@!OpenID.VCI]. If the validation of the Wallet Attestation fails, the response is rejected. Additional claims for individual implementations and ecosystems can be added to a Wallet Attestation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
: REQUIRED if the `wallet_attestation` parameter was present and its value `true` in the Authorization Request (see (#vp_token_request) for more details). The Wallet Attestation presentation MUST be bound to the nonce that was provided in the Authorization Request. The expected format of a Wallet Attestation and validation rules are defined in section TODO of [@!OpenID.VCI]. If the validation of the Wallet Attestation fails, the response is rejected. Additional claims for individual implementations and ecosystems can be added to a Wallet Attestation.
: REQUIRED if the `wallet_attestation` parameter was requested in the Authorization Request (see (#vp_token_request) for more details). The Wallet Attestation presentation MUST be bound to the nonce that was provided in the Authorization Request. The expected format of a Wallet Attestation and validation rules are defined in section TODO of [@!OpenID.VCI]. If the validation of the Wallet Attestation fails, the response MUST be rejected. Additional claims for individual implementations and ecosystems can be added to a Wallet Attestation.

Copy link
Contributor

@danielfett danielfett left a comment

Choose a reason for hiding this comment

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

Approved under the condition that we can leave the door open for more complex queries in the future.

@c2bo
Copy link
Member Author

c2bo commented Dec 2, 2024

Approved under the condition that we can leave the door open for more complex queries in the future.

Do I understand your comment correctly that you would expect more complex queries to exist in the wallet_attestation parameter instead of directly being part of the DCQL query?

@martijnharing
Copy link

I'm still opposed to the approach from this PR and think using the existing mechanisms provided by DCQL is the better solution. Adding a second solution that has a number of issues that are solved by DCQL already (I want this document with attestation, or this document without attestation) or (I want the attestation in this format). Will lead te wallet attentions being requested both through DCQL as well as through this mechanism, making implementations more complex.
Furthermore, mandating a credential format and key binding mechanism that is potentially different than the other credentials that are request as part of the transaction adds more complexity than it resolves in certain cases.

All of this is to say that I see the benefit of this additional mechanism compared to what's already provided by DCQL as is, and having two mechanisms to do the same thing (this specific mechanism as well as DCQL) is worse than having one (DCQL).

@David-Chadwick
Copy link
Contributor

I am also opposed to this PR. I maintain that treating the wallet attestation like any other credential is the right approach, and then it can selected by the RP in the same way that it selects any other credential to be returned to it.

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

Successfully merging this pull request may close these issues.

7 participants