openid / OpenID4VP

58 stars 20 forks source link

The proposal for a mandatory to implement minimum profile of PEv2 in OpenID4VP. #129

Closed Sakurann closed 6 months ago

Sakurann commented 8 months ago

This is the proposal for a standalone section in OpenID4VP that defines a query language that is a subset/profile of PEv2.

It would be required for the Wallet and the Verifier to implement query language as defined in this section. This profile is meant to be a strict subset of PE as all mandatory to implement PE features are included. Features of PE not mentioned in this profile do not need to be implemented to be compliant with this profile. The Wallet and the Verifier may decide to additionally implement other features defined in PEv2. How they agree upon this additional profile of PEv2 is out of scope for this specification.

https://docs.google.com/document/d/1r7S36RFNsnCOrYbkyTFE5ybPBalT-hERSnzKQ2rzcX8/edit

I think below is the status of addressing concerns listed in https://github.com/openid/OpenID4VP/issues/112 :

@nklomp @decentralgabe @kimdhamilton @TimoGlastra

decentralgabe commented 8 months ago

I am supportive of this proposal.

kimdhamilton commented 8 months ago

I support this proposal

selfissued commented 8 months ago

I wouldn't describe the requirements in the referenced document as being a minimum profile. As I said on the call today, I believe that a minimal profile would only include the features needed when requesting and returning a single credential at a time. I recognize that there are use cases that this would and wouldn't work for, but if we're going to create a "minimum profile", it should actually be minimal.

For instance, the descriptor_map is unnecessary if you know that you're going to receive one (or sometimes zero) presentations. The wallet behavior "Repeat steps described in (*) until all of the Credentials in the Wallet have been evaluated." is unnecessary. It's likely that there are other simplifications possible, but the above two examples serve to illustrate the point.

awoie commented 8 months ago

Cross-posting here (sorry) from https://github.com/openid/OpenID4VP/issues/112#issuecomment-1992606491:

Regarding the PEv2 profile approach, while it's feasible to maintain the same optical syntax across various credential formats, the semantics of parameters like path differ significantly, along with the associated processing rules.

A major drawback of adhering to the PEv2 profile approach is that it restricts us to a specific query vocabulary not designed to cater to the needs of more than 20 credential formats. If a community defining a credential format has unique requirements, they might struggle to adapt the existing PE vocabulary, potentially resulting in awkward implementations and profile specifications of PEv2.

It's worth noting that ISO WG10, the group responsible for the ISO mDL specification, expressed their intention to actively seek input to incorporate their specific query syntax requirements. The recent proposed amendments of ISO 18013-5 to the CBOR version of the mdoc request do not align well with the current PE vocabulary.

Persisting with the PEv2 profile approach could unnecessarily constrain us and other communities, forcing them to retrofit their needs into the PE framework. This approach is not sustainable for future developments and is likely to cause implementation challenges.

Furthermore, this could lead to potentially dangerous situations where the same PE vocabulary yields different semantic outcomes across credential formats, potentially causing confusion among developers. In my opinion, this variability undermines clarity, effectiveness and robustness.

Given that adopting even a minimal profile of PEv2 would necessitate updates to existing implementations, I believe that exploring this option offers no advantages over developing a new query language specifically designed for this purpose.

These points add to what has already been discussed in the proposal for a credential-format specific query language. Based on these considerations, I do not support the proposal raised in this issue.

OR13 commented 8 months ago

I don't support this proposal, but only because of how its framed.

I think people who want to do PE as it exists today, need a name for it, so they can understand what they are doing.

People who want to do this proposal or some refinement of it, need a name for it, so they can tell it apart from PE as it exists today.

People who don't want to spend another minute on PE need a path forward as well, and it needs a name.

All of these considerations are OIDF spec specific, so OIDF needs to register these names, until there are URNs, for versioning the query language part of OIDC4VP I object to discussing it, as I think it will lead to miscommunication.

Specifically, I can see a lot of effort going into PEv2 to make it like "the thing that people are talking about that is not PE"... and while I have said I don't want to spend my time on PEv2, I am not opposed to others spending their time on it.

I think we cannot keep using the word "PE" or "PEv2" especially because of the cross SDO confusion problem.

OIDF needs to first give URNs to these that it controls.

tplooker commented 8 months ago

I dont support this proposal and below I've tried to outline why

In essence it boils down to the proposed profile uses P.E in a way that is entirely at odds with the original design goals of P.E, basically we end up in a halfway house that makes little sense when you step back and look at the query syntax with fresh eyes.

P.E was originally designed to allow common queries down to the input_descriptor level for requesting claims across credential formats. That is where the need for syntaxes/technologies like JSON Path/Pointer comes from. By requiring path inside the fields object of an input descriptor to be an array of 1, that now makes it impossible to construct an input descriptor which works for more then one format. This makes input_descriptors an in-efficient credential format specific data structure where JSON Path/Pointer no longer makes any sense, because if the input descriptor is credential format specific, then why not align the syntax of it to the format of the credential you are requesting? This profile might limit issues with JSON Path/Pointer, but it falls well short of fixing all the issues for example what if I have a path set to $[namespace] rather than $[namespace].[data_element] can I request an entire namespace in an mDoc? How does that effect consent?

Sakurann commented 8 months ago

@selfissued

As I said on the call today, I believe that a minimal profile would only include the features needed when requesting and returning a single credential at a time.

First of all, WG never agreed that. "minimum" is requesting and returning a single credential at a time - that definition seems arbitrary. so I do not see the reason why we should optimize for that (other than adjusting for browser API design in WICG, which is not the goal here).

so, there might be misunderstanding what "minimal" means. the idea is to capture the minimum common denominator that most implementors have implemented.

Sakurann commented 8 months ago

@OR13, I don't agree. right now, OID4VP spec mandates supporting PEv2, without specifying which parts of PEv2, so it kind of implies that entire PEv2 needs to be supported. unless this profile introduces breaking changes to PEv2, which it does not, this profile is consistent with current OID4VP spec and no new versioning for it is needed.

also there might be misunderstanding how spec versioning works in oidf. If this proposal gets into the spec, all implementers who claim to support openid4vp's 3rd implementer's draft will have to implement this as a minimum profile. based on #59, verifier might be able to send the version of oid4vp it supports, but since the final version will eliminate the need for such versioning, oidf historically have not had such mechanism.

Sakurann commented 8 months ago

@awoie, your arguments seems to be against PE, not the minimum profile. there might be a company who is willing to scrap their PE implementation in OID4VP, but there are also implementers who are unwilling to do so, and this proposal aims to make it easier to interoperate between such OID4VP implementations.

If a community defining a credential format has unique requirements, they might struggle to adapt the existing PE vocabulary, potentially resulting in awkward implementations and profile specifications of PEv2.

Do you have an example that is not mdoc/sd-cwt? 18013-7 profiled PE and the result works pretty nicely. (sure, those who also implement 18013-5, need to implement 2 query language syntaxes, but that is a current trade-off to be able to request multiple credential formats using oid4vp.)

Sakurann commented 8 months ago

@tplooker

P.E was originally designed to allow common queries down to the input_descriptor level for requesting claims across credential formats.

Sure, but I think the realization has been that in many cases, when verifier is requesting a credential, it knows which credential format it wants, so this profile actually reflects how PE is being used, and don't see anything wrong with that. when we are talking about JSON-based formats, there are still a lot of common queries that can be reused.

why not align the syntax of it to the format of the credential you are requesting?

because there might be a company who is willing to scrap their PE implementation in OID4VP, but there are also implementers who are unwilling to do so, but who also struggle with some aspects of PE, and this proposal aims to make it easier to interoperate between such OID4VP implementations.

This profile might limit issues with JSON Path/Pointer, but it falls well short of fixing all the issues for example what if I have a path set to $[namespace] rather than $[namespace].[data_element] can I request an entire namespace in an mDoc? How does that effect consent?

that is a question to 18013-7, not this profile.

OR13 commented 8 months ago

Dropping support for features is a breaking change.

If we are going to make breaking changes wrt to PE, we must make it clear which version of PE is supported... If it's not clear, all of PE is supported.

If a draft keeps changing what is meant by PE, it undermines both PE and the reliability of the organization publishing the draft.

As I said, I'm fine if we give names to these things, so we can see the version of PE ISO supports and the version that was just tested, and the new version you are proposing.

But I continue to object to hand waving about which parts of PE are essential while not giving them names.

tplooker commented 8 months ago

Sure, but I think the realization has been that in many cases, when verifier is requesting a credential, it knows which credential format it wants, so this profile actually reflects how PE is being used, and don't see anything wrong with that. when we are talking about JSON-based formats, there are still a lot of common queries that can be reused.

I'm not arguing against the realisation that format specific queries aren't all that bad, I'm saying using P.E to do it is in-efficient and goes against its original design goals.

because there might be a company who is willing to scrap their PE implementation in OID4VP, but there are also implementers who are unwilling to do so, but who also struggle with some aspects of PE, and this proposal aims to make it easier to interoperate between such OID4VP implementations.

In my opinion the re-use of P.E here is more perception then reality, this profile suggests several breaking changes, coupled with the fact that even with the profile many issues remain means it does not address the root problem.

that is a question to 18013-7, not this profile.

How so? mDoc is a supported credential format in OpenID4VP, 18013-7 is only a profile of OpenID4VP. In any case the same issue also exists for SD-JWT, what would a path of $ inside the fields object of an input_descriptor represent? I want the whole SD-JWT?

kimdhamilton commented 8 months ago

Dropping support for features is a breaking change.

If we are going to make breaking changes wrt to PE, we must make it clear which version of PE is supported... If it's not clear, all of PE is supported.

If a draft keeps changing what is meant by PE, it undermines both PE and the reliability of the organization publishing the draft.

As I said, I'm fine if we give names to these things, so we can see the version of PE ISO supports and the version that was just tested, and the new version you are proposing.

But I continue to object to hand waving about which parts of PE are essential while not giving them names.

One thing I want to clear up: in v2 we cleaned up the PE spec so that optional, and rarely used aspects, were moved out of the core spec to features. This was done to ensure compatibility and readability. This proposal is consistent with that "refactoring" and so there are no dropped features.

Please see this section for more details: https://identity.foundation/presentation-exchange/#what-is-new

Sakurann commented 8 months ago

@tplooker have you read 18013-7? it defines a profile of PE for mdocs, anything on presentation exchange defined in OpenID4VP itself is superceded by 18013-7.

for the rest of the points, we are going in circles, i will only respond to the concrete, constructive proposals how to make minimum profile better.

In any case the same issue also exists for SD-JWT, what would a path of $ inside the fields object of an input_descriptor represent? I want the whole SD-JWT?

logically, if entire credential is requested, there is no need to use constraints, so if constraints do not specify anything. probably worth clarifying in the profile.

ThierryThevenet commented 8 months ago

Hello, as i said to @Sakurann our position as an implementer is that standards are changing to fast. Our customers and even many implementers are lost. Our feeling is that if we keep on working that fast the techno will be considered as not matured enough and it wil be counter productive. About PEX : that is too complex for the current use cases we see today and at minimum a subset should be clearly defined like in HAIP or an optional query language for limited use cases ? PEX is the big thing of OIDC4VP and customers are starting to invest, If we restart from scratch on the presentation side today that will delay adoption probably.

tplooker commented 8 months ago

have you read 18013-7? it defines a profile of PE for mdocs, anything on presentation exchange defined in OpenID4VP itself is superceded by 18013-7.

Yes of course, 18013-7 profiles OpenID4VP it doesn't redefine the protocol, therefore it cannot do anything about the query syntax which is based upon P.E. This is an OpenID4VP issue not an 18013-7 issue.

logically, if entire credential is requested, there is no need to use constraints, so if constraints do not specify anything. probably worth clarifying in the profile.

Again I believe you are missing my point, what I'm trying to highlight is that even with this profiling of P.E many of the fundamental issues around the ability to construct ambiguous/dangerous queries remain.

Put simply even with this profile relying parties can continue to construct JSON path/pointer expressions that do not reference "fields" or "claims", instead these path expressions can reference anything including random parts of the credential like the entire credential (via $) or say a sub-element of a claim value which can't be selectively disclosed (i gave the concrete example of driving priviledges for mDL via $[namespace].[driving_priviledges][0].code).

In order to protect against these cases the wallet would need to do a bunch of random credential format specific processing for every path expression listed in every field object of every input descriptor. That is assuming we could even identify all these cases in the first place which would appear difficult.

Beyond that how is one suppose to render consent for the attributes being requested? We've already identified as highlighted in my issue #112 that using name at the field level of an input descriptor is easy to abuse, so without that (assuming it is made forbidden), are we really expecting wallets to pull apart path expressions to understand what is being requested? For instance translating a path of [ "$['org.iso.18013.5.1']['family_name']" to "They are requesting your Family Name"? If so this again just highlights why JSON Path/Pointer is a core issue that needs to be resolved.

jogu commented 8 months ago

The suggested way forward (as discussed on both last week's calls) is to follow a process as described here:

https://docs.google.com/presentation/d/1Ax6H9CDcOVvBNlUpkJ50nIljapGqF4_Zc7_DJXmmWqI/edit#slide=id.g2c1958bc4c0_0_0

I suggest we now focus this issue on discussing potential improvements to the mandatory to implement PE proposal. Comments on the suggested process/requirements/scope can be made on issue #112. Comments about the merits of the proposals will be gathered once the requirements are clear.

Sakurann commented 6 months ago

based on the discussion in this issue specific proposal for an alternative query language has been made in #178 and the WG has agreed to use it as a starting point.