openid / OpenID4VCI

56 stars 15 forks source link

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

Open jogu opened 6 months ago

jogu commented 6 months ago

https://github.com/openid/OpenID4VCI/pull/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 commented 5 months ago

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

Sakurann commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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:

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

Sakurann commented 4 months ago

@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 commented 4 months ago

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:

jogu commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago
  • 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. https://github.com/openid/OpenID4VCI/pull/276#discussion_r1500427191 )

paulbastian commented 4 months ago
  • 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. https://github.com/openid/OpenID4VCI/pull/276#discussion_r1500427191 )

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 commented 4 months ago

@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 commented 3 months ago

@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.