WICG / digital-credentials

Digital Credentials, like driver's licenses
https://wicg.github.io/digital-credentials/
Other
82 stars 9 forks source link

What would OpenID4VP look like over the Digital Credentials API? #80

Closed samuelgoto closed 8 months ago

samuelgoto commented 9 months ago

@marcoscaceres and I were wondering what (a profile of?) OpenID4VP may look like over the Digital Credentials API, to get a sense of whether we'd missing big architectural elements in the API.

Reading the OpenID4VP spec and trying to reconcile with the Digital Credentials API spec, here is more or less what I think a request would look like:

const credential = await navigator.identity.get({
  digital: {
    providers: [{
      protocol: "OpenID4VP",
      request: JSON.stringify({
        nonce: "0xcafe",
        reponse_type: "vp_token", // maybe this is unnecessary?
        presentation_definition: {
          id: "mDL-sample-req",
          input_descriptors: [{
            id: "org.iso.18013.5.1.mDL",
            format: { "mso_mdoc": { "alg": ["EdDSA", "ES256"] } },
            constraints: {
              fields: [{
                  path: ["$['org.iso.18013.5.1']['family_name']"],
                }, {
                  path: ["$['org.iso.18013.5.1']['portrait']"],
                }, {
                  path: ["$['org.iso.18013.5.1']['driving_privileges']"],
                }, {
                  path: ["$['domestic_namespace']['domestic_data_element_id']"],
              }]
            }
          }]
        }
      })
    }]
  }
});

const {data} = credential; 
const response = new URLSearchParams(data)
const vp_token = response.get("vp_token");
const presentation_submission = response.get("presentation_submission");
// Checks responses based on the following algorithm:
// https://openid.github.io/OpenID4VP/openid-4-verifiable-presentations-wg-draft.html#name-response

@Sakurann @awoie @tlodderstedt @tplooker does this look more or less right to you?

marcoscaceres commented 9 months ago

This looks good, but not loving all the ["$['org.iso.18013.5.1']['family_name']"] micro-syntax... looks like it could be abused/confused. and would need a custom parser.

Is it possible to simplify that to something like:

{
   spec: "org.iso.18013.5.1",
   field: "family_name"
}

Or is that stuff standardized?

OR13 commented 9 months ago

Are those COSE algs? Because if so, EdDSA and ES256 are ambiguous without a key type.

If they are JOSE algs, ES256 is not ambiguous but EdDSA still is.

OR13 commented 9 months ago

response type is necessary, even though the result is being intercepted, and the presentation is being extracted... For completeness you should show the presentation being submitted to the RP.

OR13 commented 9 months ago

The json path stuff seems weird for mDoc, I don't know if CBOR path has ever been specified in an RFC, the presentation exchange spec does not cover that part I think.

marcoscaceres commented 9 months ago

I guess the larger question is: is any of the structural representation of mDoc in OpenID4VP standardized? Be nice if we could simplify it a bit.

OR13 commented 9 months ago

Regarding JSON Path standardization, it's in Auth48 at IETF right now ... https://datatracker.ietf.org/doc/html/draft-ietf-jsonpath-base-21

See section 4.1 for attacks.

marcoscaceres commented 9 months ago

Using "JSON path" for something that's not JSON seems... strange? What am I missing?

Also, we should probably not make path be an array? Might be simpler to just declare fields independently.

Anyway, we should probably resolve a lot of this on a call... I have a lot of questions 😸

cabo commented 9 months ago

JSONPath indeed is for JSON values. If the data item you want to select items from is a JSON value, JSONPath can be used, even if it wasn't encoded as a JSON text:

1.3.  JSON Values

  The JSON value a JSONPath query is applied to is, by definition, a
  valid JSON value.  A JSON value is often constructed by parsing a
  JSON text.

(implied:) or in some other way, e.g., by ingesting an encoded CBOR data item that limits itself to the JSON generic data model. But then, I don't think that all of ISO 18013-5 actually is a JSON value.

Why would the full syntax of JSONPath be needed here?

[“$['org.iso.18013.5.1']['family_name']”]

…could be expressed simpler by:

[“org.iso.18013.5.1”, “family_name”]

plus a sentence that the elements of the array descend down a nested map structure. I wouldn't add text syntax where CBOR (JSON) structure would do.

msporny commented 9 months ago

Using "JSON path" for something that's not JSON seems... strange? What am I missing?

It is, you're not missing anything. Things are currently badly specified with the ground shifting fairly regularly.

One consideration for the query language is avoiding things like regex bomb attacks, which can be performed in a variety of the existing query languages (like JSON Path) that support arbitrary regular expressions. Subsetting the language requires effort as well. JSON Pointer is almost certainly a better choice (because it doesn't support arbitrary state machines in the query language).

All that to say, I don't think OID4VP in it's current state (which we implement) is a safe thing to build on if the query language is going to be executed in a browser environment. We are going to need a more constrained query language that is fit for purpose for browser environments.

cabo commented 9 months ago

One consideration for the query language is avoiding things like regex bomb attacks, which can be performed in a variety of the existing query languages (like JSON Path) that support arbitrary regular expressions. Subsetting the language requires effort as well. JSON Pointer is almost certainly a better choice (because it doesn't support arbitrary state machines in the query language).

Yes. We did all this work already. JSONPath (RFC 9535-to-be) uses I-Regexp (RFC 9485), which addresses ReDoS.

However, I think you don't need to introduce new syntax here, neither JSONPath nor JSON Pointer (RFC 6901), because all you need appears to be descending through maps. A simple array of map keys solves this problem nicely, without adding a parser that increases the attack surface.

tlodderstedt commented 9 months ago

Just a simple key is not enough in my experience, just think about data structures like addresses and there are much deeper structured schemas in education space. However, based on the experience we gathered so far ourselves and from other implementers, simple path expressions determining claims within a credential are sufficient.

bc-pi commented 9 months ago

... not loving all the ["$['org.iso.18013.5.1']['family_name']"] micro-syntax ...

That comes from OID4VP's use of Presentation Exchange 2.0 (and its use of JSONPath) as the credential request/query language. OID4VP's dependency on Presentation Exchange is a topic with a lot of diverse opinions (to put it mildly).

marcoscaceres commented 9 months ago

Ok, yeah, JSONPath is not something I'd want to implement in WebKit (given the above responses, it seems completely unnecessary and adds security risks).

@cabo wrote:

…could be expressed simpler by: [“org.iso.18013.5.1”, “family_name”]

I'd like to avoid arrays if possible (too easy to mess up, not clear what the relationship is between values). If we can do 1:1 mappings, that would be better (below).

@tlodderstedt wrote:

However, based on the experience we gathered so far ourselves and from other implementers, simple path expressions determining claims within a credential are sufficient.

That's my feeling too. If we can get this down to:

{
   spec: "org.iso.18013.5.1",
   field: "family_name"
}

That would be much safer, explicit, simpler, and legible. It would be good to run through scenarios to see if that above covers all requests possible request attributes.

marcoscaceres commented 9 months ago

My larger question remains: "is any of the structural representation of mDoc in OpenID4VP standardized?" Specifically, I'm guessing @Sakurann and @tplooker would definitively know.

If not, then we should do that work here instead to make the request format suitable/safe for ingestion and interpretation by web browsers. It can then be upstreamed to OpenID4VP or whatever once we determine it's safe.

tplooker commented 9 months ago

That's my feeling too. If we can get this down to:

{ spec: "org.iso.18013.5.1", field: "family_name" }

This might work for credential formats that dont support any nested attributes but wont work for say formats like SD-JWT.

I'd like to avoid arrays if possible (too easy to mess up, not clear what the relationship is between values). If we can do 1:1 mappings, that would be better (below).

I'd be interested in discussing this further as an array of strings where the ordering implies the path through the data structure doesn't seem like a bad idea, nor one that would be too complex for implementations. As I said above 1:1 mappings IIUC won't work for credential formats that aren't just a flat map of attributes like mDocs.

My larger question remains: "is any of the structural representation of mDoc in OpenID4VP standardized?"

OpenID4VP is standards track at OIDF so no it hasn't completed this process but a lot of work has gone into the protocol to date including multiple rounds of implementation feedback.

Sakurann commented 9 months ago

My larger question remains: "is any of the structural representation of mDoc in OpenID4VP standardized?" Specifically, I'm guessing @Sakurann and @tplooker would definitively know.

as others have described, currently, OpenID4VP uses DIF Presentation Exchange v2.0 as a query language for any credential format, including mdocs.

If not, then we should do that work here instead to make the request format suitable/safe for ingestion and interpretation by web browsers. It can then be upstreamed to OpenID4VP or whatever once we determine it's safe.

Given that Presentation Exchange has been on track to be updated to v3.0 anyway (mainly to move away from JSON Path), I would prefer to take that as an opportunity to update query language syntax in OpenID4VP and address issues of PE v2.0. One of which is making it work with CBOR-based formats, ie mdocs.

Need to put more thought, but currently would prefer array of strings approach as mentioned by @cabo …could be expressed simpler by: [“org.iso.18013.5.1”, “family_name”]. That is the approach OpenID4VC WG is leaning towards in OpenID4VCI (issuance spec), where we had a need similar to query language in OpenID4VP to express requirements on the claims in the issued credential. and after comparing simple path expressions, JSON Path, JSON Pointer, etc, array of strings approach has been preferred in this PR (mainly from line 2255). mDL defined in 18013-5 has a nested structure (driving_privileges), too.

OR13 commented 9 months ago

Please make it work for flat and nested maps, and allow it to support integer and unicode keys.... or explain what the legal values are, and what code points are allowed and forbidden.

marcoscaceres commented 9 months ago

Please make it work for flat and nested maps, and allow it to support integer and unicode keys.... or explain what the legal values are, and what code points are allowed and forbidden.

I think we will get to that when we do a mapping from the request format to WebIDL. That is, we can use enums as needed, DOMString or USVString, the appropriate number format from WebIDL, etc... first we just need the object hierarchy, and the rest will follow.

selfissued commented 8 months ago

See my related comment on tailoring OpenID4VP to fit with the WICG credential presentation API at https://github.com/WICG/digital-identities/issues/52#issuecomment-1975276606.

tlodderstedt commented 8 months ago

I would like to let you know the Digital Credentials Protocols (DCP) Working Group at OpenID Foundation has started to work on a proposal for an OpenID 4 VP profile for the Digital Credentials API.

The respective issue is https://github.com/openid/OpenID4VP/issues/125. Feel free to chime in and comment on the draft.

We plan to contribute this proposal as work item to the DCP WG and will present it to this group as soon as possible.

We are investigating how OpenID 4 VP could be best fit into the protocol/request scheme as set out by the Digital Credentials API. Based on the work so far we think extending the Digital Credentials API with support for Wallet-provided nonces would be beneficial. I have filed a separate issue on that topic in this repository.

Sakurann commented 8 months ago

@marcoscaceres have you taken a look at how 18013-7 profiled PE for mdocs? It does not use JSON Path:

path is a JSON array where each entry is a JSON String containing a requested data element from the requested document type as follows: $['<namespace>']['<data element identifier>']. For example, to request a data element with an data element identifier family_name from the namespace org.iso.18013.5.1, the following JSON String is used: $['org.iso.18013.5.1']['family_name'].

18013-7 also mandates input_descriptor.id to be doctype.

samuelgoto commented 8 months ago

I would like to let you know the Digital Credentials Protocols (DCP) Working Group at OpenID Foundation has started to work on a proposal for an OpenID 4 VP profile for the Digital Credentials API.

That's great to hear! Thanks for kicking this off @tlodderstedt !

I'm going to close this issue and point any open questions and discussions to this proposal instead, where we'll find a community of experts that can help us resolve them.