search

w3c / vc-jose-cose

Verifiable Credentials Working Group — VC JSON Web Tokens specification
https://w3c.github.io/vc-jose-cose/
Other
31 stars 13 forks source link

remove securing json #88

Closed mprorock closed 1 year ago

mprorock commented 1 year ago

Remove securing json section - focus on securing core data model

Preview | Diff

mprorock commented 1 year ago

This will let us focus very clearly on securing the core data model section and have something very usable. This is obviously not to say we don't want to secure JSON with registered claims, but that we can do that elsewhere or revisit later

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-05-24

View the transcript #### 1.4. remove securing json (pr vc-jwt#88) _See github pull request [vc-jwt#88](https://github.com/w3c/vc-jwt/pull/88)._ **Michael Prorock:** We have a valid change request in -- for PR #88 -- but it will need discussion. … This one does something potentially controversial. It removes the securing JSON section. It makes VC-JWT focus only on securing the core data model with no transformations or mappings, nothing else. … Kristina has mentioned market interest in vanilla JSON claims in VC-JWT. … So I've raised that to get comments from the community. **Kristina Yasuda:** Any comments on that? **Orie Steele:** Regarding removing VC-JWT media type and securing plain JSON -- I'm in favor of this PR. I'm in favor of this based on the day 3 F2F resolution and the work load for this group. … We have several technical recommendations to work on and it's very unlikely to get consensus on the day 3 resolution in the core data model and without that consensus we will not be able to elevate this item to the point where it will go into CR. … Without adding normative statements that will be contentious and it won't be able to get merged and I foresee this not going anywhere as long as this section stays in the document. I'm making that assessment based on the rate of engagement and so on. **Dave Longley:** +1 to remove the section. > *Phillip Long:* +1 to remove this section. **Kristina Yasuda:** Any other PRs?
paulfdietrich commented 1 year ago

This change might also necessitate some removal in A.1 and A.3, as well as A4.1. I think these sections would be confusing otherwise.

OR13 commented 1 year ago

@mprorock can you apply @paulfdietrich comment? And remove those appendix sections?

mprorock commented 1 year ago

@mprorock can you apply @paulfdietrich comment? And remove those appendix sections?

done, thanks for the careful read @paulfdietrich

OR13 commented 1 year ago

It should be the other way around. There is sufficient market interest in having a vanilla JSON payload in VC-JWT.

@Sakurann we are not chartered to secure arbitrary JSON, we are chartered to secure "vc+ld+json".

@iherman please confirm the charter details, if I am incorrect, we have wasted a tremendous amount of time, over objections that are unfounded.

brentzundel commented 1 year ago

@OR13 our scope is sufficiently broad enough to support specifying how to secure arbitrary JSON using VC-JWT (in addition to how to secure vc+ld+json), but that isn't the same as having consensus to do so.

OR13 commented 1 year ago

From my perspective there is no consensus to define a normative mapping, or to retain optional mappings with no normative teeth... this is the same as admitting there is no consensus to secure JSON, or any other content types in this working group.

@brentzundel @Sakurann please establish consensus on this topic, editors can't advance the document to horizontal review without it.

OR13 commented 1 year ago

Conflict free version is here: https://github.com/w3c/vc-jwt/pull/102/files

iherman commented 1 year ago

It should be the other way around. There is sufficient market interest in having a vanilla JSON payload in VC-JWT.

@Sakurann we are not chartered to secure arbitrary JSON, we are chartered to secure "vc+ld+json".

@iherman please confirm the charter details, if I am incorrect, we have wasted a tremendous amount of time, over objections that are unfounded.

I do not see any charter issues. The charter does not mention media types, only VC-s in general.

alenhorvat commented 1 year ago

The specification should document how to secure JSON using JWT-VCs and mapping between the claims since many use cases today are creating JWS (JAdES) signatures. If this specification doesn't cover the topic, there's a risk of having multiple profiles.

alenhorvat commented 1 year ago

My question above has been answered in discussion in https://github.com/w3c/vc-data-model/pull/1088

Essentially, the proposal here is compliant with definitions of JWT and JWS.

I'm not sure about the 'typ' property: "typ": "vc+ld+jwt"

according to the RFC7515

The "typ" value "JOSE" can be used by applications to indicate that this object is a JWS or JWE using the JWS Compact Serialization or the JWE Compact Serialization. The "typ" value "JOSE+JSON" can be used by applications to indicate that this object is a JWS or JWE using the JWS JSON Serialization or the JWE JSON Serialization. Other type values can also be used by applications.

Hence, if my understanding is correct, the typ should be: vc+ld+JOSE or vc+ld+JOSE+JSON

The model should be fully compatible with the SD-JWT selective disclosure.

RFC7515 already secures an arbitrary JSON. Signature profiles are then further defined by OIDC for the id token, JAdES for advanced signatures - note that JAdES fully secures ANY JSON, regardless of the content or the data model; etc.

mprorock commented 1 year ago

The specification should document how to secure JSON using JWT-VCs and mapping between the claims since many use cases today are creating JWS (JAdES) signatures. If this specification doesn't cover the topic, there's a risk of having multiple profiles.

This work is being done elsewhere, see: draft-prorock-jose-native-jwt-vcs-00 - will be replaced by a version likely merging terbu draft below draft-terbu-sd-jwt-vc-02 - will likely contain all changes from above, with sd-jwt stuff and a related PR

We should merge this PR @brentzundel @Sakurann @selfissued @OR13 Then we can proceed to name this spec VC-xOSE or something and have it cover explicitly (per charter) securing the core data model with JOSE and COSE

OR13 commented 1 year ago

I support merging this PR.

alenhorvat commented 1 year ago

The specification should document how to secure JSON using JWT-VCs and mapping between the claims since many use cases today are creating JWS (JAdES) signatures. If this specification doesn't cover the topic, there's a risk of having multiple profiles.

@mprorock Please check comment: https://github.com/w3c/vc-jwt/pull/88#issuecomment-1591910039. Ignore the one you quoted.

I agree with the VC-*OSE

peacekeeper commented 1 year ago
  • Hence, the document should be titled securing VCs with JWS

I agree with this. Also see https://github.com/w3c/vc-jwt/issues/12.

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-06-21

View the transcript #### 1.2. remove securing json (pr vc-jwt#88) _See github pull request [vc-jwt#88](https://github.com/w3c/vc-jwt/pull/88)._ **Kristina Yasuda:** This PR 88 removes anything JSON from the vc-jwt spec. Me and Mike Jones are requesting changes. … I would approve, but Mike isn't here. **Manu Sporny:** I think the title isn't representing what the PR is doing, which is confusing. … What the removal here is doing is the whole mapping thing? … If you reviewed the PR, please take a look again. … I'm curious why kristina -1'd it in the past. **Kristina Yasuda:** I don't agree with that interpretation. … After speaking with Orie and Mike, what's being removed isn't the mapping. … Instead it's removing how you vanilla JSON payloads. … The reason I originally -1'd was not because of the direction. It was because there is a certain group of implementers that wants to know how to sign vanilla json payloads. … I'm OK with the decision to move the home of those implementers to somewhere else. **Manu Sporny:** Do we have an example of what this JSON payload looks like? … Is it the same as a VCDM sans the context field? **Kristina Yasuda:** I think the difference isn't only that. … In the VCDM v1.1 we define how to map to JWT claims, including "vc" and "vp" claims. … In the JWT/JSON world, it's natural to reuse those JWT claims. … So it's not only missing context, but also the mapping and interpretation of many of those claims. … You aren't relying on the vocabulary only, but also on semantics defined outside. **Brent Zundel:** The way I understand this PR is currently the vc-jwt spec describes how to secure vc+ld+json, vp+ld+json, and json. … The piece that's being removed is the `json` piece only. > *Kristina Yasuda:* that matches my understanding, Brent. **Brent Zundel:** So the spec only describes how to secure the first two. > *Manu Sporny:* ok, I think I understand... **Kevin Griffin:** Is there any room to map what's being removed to another home? Just a thought. … Is the VC specs dir going to be the home? **Kristina Yasuda:** AFAIK there are no plans, but that's uncertain. **Manu Sporny:** Following up on what kevin said. This is related to transformations so it might shed some light on 1100 and 1101. … Seems like the group won't say anything related to transformations. … But the group seems to think it's important. … But we can't come to consensus on what adequately specified means. … I haven't heard anyone say that transformations from the VCDM are ok to operate on as long as they aren't "abused". … This PR seems like a side effect of not being able to define transformations in a legitimate way. … Don't know if that's a legitimate read. Curious what others think. > *Kevin Griffin:* +1 manu. **Kristina Yasuda:** I think that's accurate. **Joe Andrieu:** mute dance issues haha. I'm confused by the language that we won't specify transformations. … How are we going to test vc+jwt to vc+ld+json if there aren't testable transformations? **Kristina Yasuda:** There will be no mapping to test. **Ted Thibodeau Jr.:** I'm still concerned that we are misusing media types with the vc+ld+jwt. > *Manu Sporny:* agree. **Ted Thibodeau Jr.:** That is a JWT, it's not multipart. > *Brent Zundel:* Ted has a point, but that is a separate issue from the one we're discussing. **Kristina Yasuda:** I agree. Maybe using media type is not the correct term for this. … maybe putting this in the header of a JWT is not common. … But, right now, it's a pretty established mechanism. … Yes, it's the same type that you would be putting in the header as in the HTTP header. **Ted Thibodeau Jr.:** It simplifies some things, but it's a misuse of the tool. **Kristina Yasuda:** Agree to disagree. … I'm going to unblock for now. … Mike Jones will need to chime in next.
mprorock commented 1 year ago

can you please also remove mention of vc+jwt in the IANA registry and a note in section 1.1? happy to approve once that is removed.

that should be done in d83f3b2 and 6c76065 - @Sakurann can you put eyes on to make sure i didn't miss something?

Sakurann commented 1 year ago

This scopes vc-jwt spec to securing JSON-LD payload, which I think it a good step forward. securing vanilla JSON payload using JWTs should continue elsewhere.

updated on june-29: the original hope of MSFT when joining VC WG v2.0 last year was to work on improving how vc-jwt specification can be used to secure vanilla JSON, but the WG over the course of last year made it very clear that it is not interested in doing so, nor it has sufficient expertise to do so. hence the comment above.

OR13 commented 1 year ago

I think it's wise for the working group to focus on doing 1 content type well, and letting other working groups focus on other content types.

mprorock commented 1 year ago

@OR13 do you mind putting some extra eyes in to make sure i resolved conflicts correctly and didn't miss anything?

brentzundel commented 1 year ago

@selfissued are you still wanting changes to this PR?

If yes, @OR13 perhaps we should discuss briefly during the WG call next week?

OR13 commented 1 year ago

I removed the labels related to external review... I am hopeful we can clean this up eventually.

OR13 commented 1 year ago

Conflicts exist.

I suggest we merge this PR before merging any others, its making it very difficult to maintain the spec, and the sections in question are not needed.

TallTed commented 1 year ago

@mprorock —

It appears that we need you to resolve the conflicts on this PR, and then maybe @selfissued (who still has an open change request, according to the reviewers list) should re-review, so it can be merged...

mprorock commented 1 year ago

@OR13 please double check that i resolved conflicts correctly - as you noted this PR is a blocker and I really can't do any work until this is in.

mprorock commented 1 year ago

Chairs - @brentzundel @Sakurann can we get consensus on this so we can merge - otherwise we are blocked from further work on prepping this item for CR