openid / OpenID4VCI

68 stars 20 forks source link

`iss` in Key Proof, conditionally required or generally optional? #349

Open mickrau opened 5 months ago

mickrau commented 5 months ago

There were different interpretations during the LSP interop event whether the iss element in key proof (both jwt and cwt) is conditionally required or generally optional?

In Section 7.2.1.1. (and 7.2.1.3.) it says:

iss: OPTIONAL (string). The value of this claim MUST be the client_id of the Client making the Credential request. This claim MUST be omitted if the access token authorizing the issuance call was obtained from a Pre-Authorized Code Flow through anonymous access to the token endpoint.

Is there a reason to make it optional in the case the client_id is known to the issuer?

I would prefer to change it to REQUIRED when ..., MUST NOT ... (as has been done elsewhere)

Sakurann commented 5 months ago

the intention here was

bc-pi commented 5 months ago

why have an iss at all in the proofs? honest question

mickrau commented 5 months ago

@Sakurann If this is the intention, the current description fits.

Then i would agree to @bc-pi question.

Is the iss is security-relevant? If yes, it should be required or the issuer should have the possibility to inform the user that he requires it in my opinion.

With the current version we end up with a (worthless?) check on the issuer's side:

check(signedJwt.jwtClaimsSet.issuer == clientId || signedJwt.jwtClaimsSet.issuer == null) {
   "iss MUST be the clientId or null"
}
jogu commented 5 months ago

I think various security-ish issues end up being related and it's quite hard to reason about them all given the amount of optionality in the specification.

iss in the key proof is one way to bind the key proof to the client id (although in some cases there will be other mechanisms for binding the key proof to the client, e.g. the suggestion to include public keys in wallet attestations that was discussed by a small group at EIC, which I think was an iteration on https://github.com/openid/OpenID4VCI/issues/305 but that ticket hasn't been updated yet after that discussion).

https://github.com/openid/OpenID4VCI/issues/19 proposes an alternate mechanism for that binding that I think would make iss unnecessary.

For what it's worth I agree that iss is mostly worthless currently if it's completely optional for the client to include it, my question would be whether we try to fix that or remove it in favour of other mechanisms.

babisRoutis commented 5 months ago

For what it's worth I agree that iss is mostly worthless currently if it's completely optional for the client to include it, my question would be whether we try to fix that or remove it in favour of other mechanisms.

Currently, VCI doesn't describe when this iss attribute must be provided.

This creates the following situation: As a wallet better included, just in case issuer requires it (in the authorization code flow) As an issuer better don't check it, or check it as per @mickrau comment.

IMHO, iss should be removed.