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

Can registered JWT claims be used only in the JOSE header?? #135

Closed Sakurann closed 11 months ago

Sakurann commented 1 year ago

Current Registered Claim Names section implies that registered JWT claims be used only in the JOSE header, but there is no normative statements around it. I think the assumption is that the body of JWT is as defined in vc-data-model, which means registered JWT claims are not used. Otherwise, implementers might put iss and issuer in the body and we are back to in addition to logic we had in VCDM v1.1. so... probably wise to add a paragraph stating that "registered JWT claims can be used in the JOSE header but should not be used in the body".

alenhorvat commented 1 year ago

JWT RFC requires defining a signature profile by the "application".

If we follow the specs, nothing prevents using JWT claims in the payload, since this is a property of JWT - JWT is a combination of enveloping and detached signature.

JOSE actually consists of

See: https://github.com/w3c/vc-jose-cose/issues/132#issue-1817291093 - part where issuer != signer.

When JWT is used, the application should define a JWT signature profile and with it whether or not the JWT claims are used. We're only using JWS (compact/JSON) (without JWT claims).

Currently I'm analysing a flow and it may happen that in a VP nonce may be used (in that particular case).

Theoretically JWT claims could appear in the VC (it is a nature of JWT design). I personally believe it is not a good practice (except if very well defined).

alenhorvat commented 1 year ago

@Sakurann , if the WG agrees to add

"registered JWT claims can be used in the JOSE header but should not be used in the body".

I recommend to drop "JWT" and start using JWS (compact/JSON serialised). JWT is JWS compact + payload claims. So JWT without payload claims is JWS compact.

TallTed commented 1 year ago

Pitching JWT seems the best thing to do, because JWT will always mess with the payload, and that's not desirable in any way.

Sakurann commented 1 year ago

@alenhorvat there claims are registered via a JWT RFC, so I think they need to be called "registered JWT claims"...

alenhorvat commented 1 year ago

@Sakurann thank you for the correction. I'll do my best to be more accurate in the future :)

I gave it some thought and since JWTs will be used in the future, it would be beneficial to summarise how registered JWT claims can be used with VCDM (for protection purposes). I understand that this might be controversial to some, but I believe it is better if VCDM protected by JWT is defined, than left open (which will lead to some strange use of the VCDM and JWT)

WDYT?

Sakurann commented 1 year ago

I agree with you @alenhorvat that it needs to be defined which JWT claims must be in the header and which in the body, but I don't think @OR13 agrees.

OR13 commented 1 year ago

It's not about what I believe.

People put iss in the payload.

The original purpose of putting claims in the header was for encrypted payloads.

W3C is really not the right place to change this kind of behavior, so this is what I am recommending:

  1. If it's a registered claim name we expect to see, define it properly in rdf.
  2. Allow people to put key discovery hints in the header, so they can write safer implementations, and not decode the payload to discover key material to verify it... better to decode the header and then verify before looking at the payload.
  3. Protocols exist that require registered claims to be in the payload, don't break them intentionally, as that will cause the W3C implementation to be incompatible with adopted standards.
alenhorvat commented 1 year ago

@OR13 that's a great observation (people putting claims in the payload, not in the header). An interesting issue occurred in the mDL profile, where suddenly the encrypted response cannot be matched against the request (nonce and state are encrypted)

WG should decide whether it wants or not define a profile for "JWT", similar to JOSE. (I still own the JAdES profile; Will do my best to write the proposal next week)

I agree with 1 and 2.

  1. is a bit tricky to cover with a single profile. What if we do the following 3a: Define a JWT signing profile for cases where removing payload claims breaks the backwards compatibility. 3b: Else, use the JOSE profile and don't put any additional claim to the payload

We have observed that 3a cannot be used with remote e-sealing services, since one only sends the digest to be signed to the remote e-sealing service(never the payload) and the service should never modify the payload. If signer or signature-creation related information is inside of the payload, there's an issue. (signer may want to control the iat, iss, for example)

Existing JOSE and JAdES protection mechanisms should cover most new implementations.

selfissued commented 1 year ago

At https://w3c.github.io/vc-jose-cose/#registered-claim-names, I would suggest changing "When found in the Protected Header, or the Protected Claimset" to "When found in the JWT Claims Set or the Protected Header".

It would be better to list the JWT Claims Set first, since that's the normal place for claims. Plus we should be using the correct name "JWT Claims Set".

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-09-14

View the transcript #### 6.16. Can registered JWT claims be used only in the JOSE header?? (issue vc-jose-cose#135) _See github issue [vc-jose-cose#135](https://github.com/w3c/vc-jose-cose/issues/135)._ **Michael Jones:** my new PR has correct names for the things. implicitly that answers the question. **Kristina Yasuda:** . � I don't think that PR addresses this issue, and it changes what Orie originally intended. � There are claims like kid that can only exist in Header, but claims like iss where the optionality was intentionally left, whether header or body. � I was asking for clarification . If both are possible the processor will need to know what to expect. � If addressed byt eh PR, then iss can only exist in claim, so we need more than open PR. **Michael Jones:** understand what you are saying. will note that you can optionally put JWT claims in the header. that's why I felt the PR answers the issue. > *Manu Sporny:* [https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2](https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2). **Michael Jones:** . defined by an RFC. **Manu Sporny:** +1 to what kristina is saying. if you look at the context, iss, exp, etc. can be present in both places (header and the body). ). � this has been an issue for a while. I have been objecting for it for a while because the optionality will hurt interop. > *Paul Dietrich:* +1 manu. I though we were fixing this. **Manu Sporny:** not even sure there is issue for this. might lead to the problem that we had with an approach in vcjwt 1.1 that duplicated fields in "in addition to" approach. > *Manu Sporny:* +1 to correcting things in the way that selfissued proposed. **Michael Jones:** agree that clarification is needed, can do a simple PR. **Kristina Yasuda:** . > *Brent Zundel:* I don't think this is ready for PR. I don't think we have an agreement. I'm leaning toward those claims can be present in the header. If now we believe we have enough implementatiion experience that puts them in the header, that's where we shoudl say they should be. > *Manu Sporny:* agree with kristina, we need to make a decision here. > *Manu Sporny:* here is the issue: [https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L9-L58](https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L9-L58). **Michael Jones:** no place says JWT claims can be in the header. We shouldn't start that. � my PR says follow JWT spec guidance. > *Kristina Yasuda:* I think Orie will object to that but we will see. **Michael Jones:** It deprecates the practive of duplicating them.
OR13 commented 1 year ago

@selfissued we think maybe we can resolve by removing confusing text.

OR13 commented 1 year ago

I think we should state only what header parameters are required... and remove anything suggesting optionality or duplication.

AFAIK, kid and alg are required. typ and cty are optional

OR13 commented 1 year ago

@selfissued We discussed that you would address this in your "clean up" PR

OR13 commented 1 year ago

on our call, @decentralgabe discussed wanting to take a stab at this