openid / OpenID4VP

44 stars 11 forks source link

Utilize "must ignore if not understood" for query language parameters #164

Open selfissued opened 2 months ago

selfissued commented 2 months ago

I suggest that we enable query language extensibility via the usual OAuth/JSON "must ignore parameters that are not understood" methodology.

Then, for instance, we could express things like Purpose and Intent-to-Retain via query parameters that are understood and acted upon by some ecosystems and ignored by others where they aren't needed or useful.

This is related to #157 and #160.

David-Chadwick commented 2 months ago

What about a parameter that has potentially negative consequences on the user, such as "Intent to Pass to Security Services" that is not understood by the wallet?

jogu commented 1 month ago

I think as there's no way for the verifier to know what a wallet supports, there may be a need for a "if you don't understand this extension then don't process the request" (i.e. like the 'crit' header in jwt: https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.11 )

selfissued commented 1 month ago

As background, the OpenID Federation spec is also using "crit" in JWT Claims Sets in this way. See https://openid.net/specs/openid-federation-1_0-34.html#name-crit-critical-claim .

David-Chadwick commented 1 month ago

Yes I agree that using the crit field is a much better solution than simply saying ignore everything you don't understand. This then mirrors the tried and tested X.509 critical extensions flag. But there is one difference. In X.509 the superior of an entity sets the criticality flag, thereby ensuring that the subordinate's X.509 PKC will always be understood or rejected. But in OpenID4VPs we have the RP setting the crit flag in its own query (if we introduce it) and its in the RP's interest to get credentials from the wallet when the wallet does not understand the critical properties. So there is no incentive for the RP to ever set the crit flag. Therefore the OpenID4VPs specification must state that a wallet must reject queries in which there are properties it does not understand i.e. the crit flag is assumed to be always set so is not needed. Otherwise the wallet is in danger of releasing the user's PII to an untrustworthy RP.

jogu commented 1 month ago

So there is no incentive for the RP to ever set the crit flag

Like everything else in the query, it's set by the RPs. They need to set the query correctly if they want to only receive credentials that can actually result in the user completing their flow, this applies as much to any 'crit' flags as it does the rest of the query. The problem of untrustworthy RPs seems completely orthogonal to me.

David-Chadwick commented 1 month ago

You are missing the point. If the crit flag is set and the property is unknown, then the user wont be able to complete the flow, but if the crit flag is not set the user will complete the flow, with possibly negative consequences that they were unaware of.

Furthermore we are already building several controls in to protocol to protect against untrustworthy RPs, so this situation should be no different

jogu commented 1 month ago

You are missing the point. If the crit flag is set and the property is unknown, then the user wont be able to complete the flow, but if the crit flag is not set the user will complete the flow, with possibly negative consequences that they were unaware of.

That's exactly the same case as if the RP had made a query without including the part of the query the wallet doesn't understand. Or put another way, rejecting fields the wallet sent because they're unknown has no advantage given the untrustworthy RP could have just omitted that part of the query.

In both cases the user would see the same end result - the credential the user authorises to be released is released to the indicated RP. I don't see the negative consequences the user was unaware of.

The only place it makes a difference is if something (e.g. the browser?) is trying to show the query (rather than the credential that will be released) to the user, but most systems do not plan to do that, and there are other ways to solve that than your suggested solution that would prevent extending the query language later.