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 can't always identity whether key proof is required #175

Closed
jogu opened this issue Dec 28, 2023 · 15 comments · Fixed by #392
Closed

Wallet can't always identity whether key proof is required #175

jogu opened this issue Dec 28, 2023 · 15 comments · Fixed by #392

Comments

@jogu
Copy link
Contributor

jogu commented Dec 28, 2023

#87 added a mechanism within credentials_configurations_supported for the wallet to know if the issuer requires proof of possession for a particular credential configuration, and if so what types are supported.

There are some cases where the wallet can't lookup this value though / it isn't clear to the credential issuer which credential the wallet is asking for.

An oversimplified example that shows the problem, e.g. in the pre-authorised code flow where the wallet has been offered more than one credential:

credential_configurations_supported

{
  "credential_1": {
    "format": "my_format",
    "claims": { "family_name": {} }
  },
  "credential_2": {
    "format": "my_format",
    "claims": { "family_name": {} },
    "proof_types": [ "jwt" ]
  }
}

credential offer

{
  "credential_issuer": "...",
  "credential_configurations": [ "credential_1", "credential_2" ],
  "grants": { ... }
}

credential request

{
  "format": "my_format",
  "claims": { "family_name": {} }
}

This could be solved by having the wallet supply credential_configuration_id in the credential request. (Hence making format unnecessary.) i.e.:

credential request

{
  "credential_configuration_id": "credential_2",
  "claims": { "family_name": {} }
}

(Spotted by Taka@Authlete)

@Sakurann
Copy link
Collaborator

I think this is related to issues #132 and #197.

@Sakurann
Copy link
Collaborator

Credential is always identified by the combination of format and type (type claims are mandatory in the credential format profiles).

proof_types_supported is optional to enable the use-cases when issuer wants to issue bearer credentials. we can probably make this clearer in the description that if the issuer requires key binding, the parameter must be present.

if issuer metadata is used, wallet can look up proof_types_supported based on the credential_configuration_id. credential_configuraiton_id can be received in the credential offer. if the wallet is starting auth code flow without the offer, it will still need to know the scope or credential_configuration_id to put into the authorization_details from the issuer metadata, so the wallet should be able to look up proof_types_supported too.

@jogu
Copy link
Contributor Author

jogu commented Feb 21, 2024

Credential is always identified by the combination of format and type (type claims are mandatory in the credential format profiles).

I checked with Taka and although it might be the intention that format & type uniquely identify a credential_configuration_id, we don't seem to actually say that as far as I can see. So e.g. here is an updated example that has vct but still have the ambiguity:

credential_configurations_supported

{
  "credential_1": {
    "format": "vc+sd-jwt",
    "vct": "SD_JWT_VC_example"
    "claims": { "family_name": {} }
  },
  "credential_2": {
    "format": "vc+sd-jwt",
    "vct": "SD_JWT_VC_example"
    "claims": { "family_name": {} },
    "proof_types": [ "jwt" ]
  }
}

credential offer

{
  "credential_issuer": "...",
  "credential_configuration_ids": [ "credential_1", "credential_2" ],
  "grants": { ... }
}

credential request

{
  "format": "my_format",
  "vct": "SD_JWT_VC_example"
}

The fix would be I guess to say something like a vct value must only be present once in an issuer's metadata?

@paulbastian
Copy link
Contributor

paulbastian commented Feb 21, 2024

I have the fear that in the long run, saying format+type is enough could hurt us, because this puts significant requirements on the possibly supported credential formats and maybe in the future this constraint doesn't make sense anymore.

I consider this a flaw of the protocol and it keeps popping up from time to time. The ideal solution in my mind is to either:

  • make credential instance identifier from token response mandatory and use that one in the credential request
  • use credential supported identifier in the credential request

The first one is superior and makes sense in the proposed post-id1 changes but two is still a good improvement

@Sakurann
Copy link
Collaborator

Sakurann commented Feb 21, 2024

@jogu, it feels like multiple problems are being confused. a problem that one of the credential_configurations_supported object lacks proofs_supported so the wallet does not know which proof_type is supported is different from what to do when two credential_configurations_supported objects have different credential_configuration_ids, but the same combination of format and type.

@Sakurann
Copy link
Collaborator

Sakurann commented Feb 21, 2024

for the first problem, as I said previously, we can make it clearer in the description that if the issuer requires key binding, the parameter must be present.

for the second problem, would like to continue a discussion in #132. but to reiterate here:

  • making credential instance identifier from token response mandatory and use that one in the credential request would not work for the implementations that re-use existing ASs and cannot make breaking changes to them.
  • using credential supported identifier in the credential request would work very nicely and aligns to what is being proposed/discussed in using credential_configuration_id in the credential request when scopes are used in the authorization request #132.
  • to say something like a vct value must only be present once in an issuer's metadata would probably limit use-cases, as I think we had a use-case where we were using same format/type combination, but different information for the rest of the metadata (like display).

@jogu
Copy link
Contributor Author

jogu commented Feb 21, 2024

@jogu it feels like multiple problems are being confused. a problem that one of the credential_configurations_supported object lacks proofs_supported so the wallet does not know which proof_type is supported is different from what to do when two credential_configurations_supported objects have different credential_configuration_ids, but the same combination of format and type.

To be clear, the intention of this issue was to discuss the second problem. (It seemed clear to me that a credential_configurations_supported without proofs_supported means that a proof is not required for that credential. It that's not actually clear I'm happy for it to be fixed.)

I think you're right that it overlaps with #132, can we expand #132 to include the case where RAR is used and a credential_configuration_id is not returned?

@David-Chadwick
Copy link
Contributor

make credential instance identifier from token response mandatory and use that one in the credential request

Our implementation experience found that this was essential, especially when a user/wallet can have two credentials of the same type but with slightly different contents e.g. Chemistry Degree and Maths Degree from the same university.

@TakahikoKawasaki
Copy link

  • making credential instance identifier from token response mandatory and use that one in the credential request would not work for the implementations that re-use existing ASs and cannot make breaking changes to them.

This is occasionally stated, but I always wonder how an authorization server implementation that is too inflexible to add a new parameter in a token response can support the OID4VCI specification. The assumption doesn't sound very convincing.

If the discussion on whether to add a new parameter to the token response is hindered to protect the implementation of a specific company, it is a significant concern for the community. (cf. #276 (comment) )

@paulbastian
Copy link
Contributor

  • making credential instance identifier from token response mandatory and use that one in the credential request would not work for the implementations that re-use existing ASs and cannot make breaking changes to them.

This is occasionally stated, but I always wonder how an authorization server implementation that is too inflexible to add a new parameter in a token response can support the OID4VCI specification. The assumption doesn't sound very convincing.

If the discussion on whether to add a new parameter to the token response is hindered to protect the implementation of a specific company, it is a significant concern for the community. (cf. #276 (comment) )

For existing AS, couldn't you make a wrapper AS that supports these additional features and adds the credential instance identifier to the token response?

@jogu
Copy link
Contributor Author

jogu commented Mar 4, 2024

@paulbastian

For existing AS, couldn't you make a wrapper AS that supports these additional features and adds the credential instance identifier to the token response?

In theory, to some extent, in some situations. I don't think it would work and/or be possible in some situations and it potentially gets confusing. I think it would need to be more than a simple wrapper and much closer to a full fledged AS that is federating out to the underlying AS, unless they two ASes because intrinsically coupled in their backends.

The VCI spec has so far catered, albeit with reduced functionality, for existing Authorization Servers without them needing to change (other than adding new scope values and other fairly standard things). Departing from that goal would be a significant change and may harm adoption of VCI.

@Sakurann
Copy link
Collaborator

Sakurann commented Mar 5, 2024

@TakahikoKawasaki there is more than one company that has ASs that have limitations that do not allow them to make changes easily or to have intermediary ASs/wrappers, etc. and as a community we should value every single company's implementation.

@Sakurann
Copy link
Collaborator

@paulbastian @c2bo is this addressed in #389 PR?

@c2bo
Copy link
Member

c2bo commented Oct 28, 2024

@paulbastian @c2bo is this addressed in #389 PR?

I don't think key attestation helps us here - #392 seems more relevant but doesn't seem to fully cover this?

@paulbastian
Copy link
Contributor

I believe that this is covered by the resolution on #392 of today's DCP Hybrid Call 👍

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

Successfully merging a pull request may close this issue.

6 participants