openid / OpenID4VP

57 stars 20 forks source link

vp_token encoding is not 100% clear #295

Open markuskreusch opened 1 month ago

markuskreusch commented 1 month ago

The encoding of the vp_token parameter is not 100% clear for response_mode fragment or direct_post.

From 7.1 on vp_token:

In case Presentation Exchange was used, it is a JSON String or JSON object that MUST contain a single Verifiable Presentation or an array of JSON Strings and JSON objects each of them containing a Verifiable Presentations.

So the value can be a JSON String, JSON object or JSON array. If it is a single value and a string, would the thing to encode in the URI be "the_vp_token_value" (including quotes, so parsable as JSON string, weird IMHO) or just be the_vp_token_value (without quotes, so not parsable as JSON string). I've not seen "the_verifiable_presentation" thing but taking the spec by word this should be how it is done.

My current approach, to be prepared for both situations, is to try parsing as JSON string first and if that fails, use the vp_token value as is as a string. The problem with that approach is, that any value is then used in the raw string form, even a value that should be parsable as JSON string but contains an error. This will most likely lead to a less specific error when the vp_token is used later on. Instead a parse error should be reported.

For response_mode direct_post.jwt this is ok, because we are parsing JSON content and thus the plain string option is not valid there.

jogu commented 1 month ago

The behaviour/expectation I've implemented in the conformance suite is that if the response is expected to be a single credential in mdoc or sd-jwt vc format then the direct_post response will be a plain string. I'm not sure many people have tested with that though, everyone seems to have been using direct_post.jwt.

I agree the spec could be clearer here and the reference to JSON String should probably be changed to just 'string'.

danielfett commented 1 month ago

This has some overlap with https://github.com/openid/OpenID4VP/issues/148, which I probably should not have marked as being closed by #266. Half of it is solved (the proper definition of the VP Token), and half of it may be solved in future (in case PE is removed before final).

Sakurann commented 6 days ago

will close this issue in a week unless there are objections

jogu commented 4 days ago

I don't think we can close this (without fixing the definition of VP token for PE) unless/until we remove PE.