search

w3c / vc-jose-cose

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

Add guidance around using JWK #220

Closed decentralgabe closed 6 months ago

decentralgabe commented 7 months ago

fix #210

Preview | Diff

iherman commented 7 months ago

The issue was discussed in a meeting on 2024-01-24

View the transcript #### 1.2. adjust language in example 13 (pr vc-jose-cose#220) _See github pull request [vc-jose-cose#220](https://github.com/w3c/vc-jose-cose/pull/220)._ **Brent Zundel:** Adjust language in example 13 -- again an editorial change. … Ted has reviewed and his change requests have been made. I encourage other folks to check out this PR. **Manu Sporny:** Just a question -- I approved the PR fine as is. There's this fully specified algorithm stuff that is making it's way through IETF. How does that spec involve the language here? … I think this one doesn't make mention of fully specified algorithms or not ... it probably doesn't need to but do we need any updates around this? **Michael Jones:** For the language in example 13? **Manu Sporny:** Yeah. The PR also adds some normative text around the `alg` property. It says the `alg` property is optional and is recommended to be included. … It says that `crv` and `kid` need to be specified. **Michael Jones:** I need to review that then because `alg` is never optional. **Manu Sporny:** This PR says it is. I'll put your comment into it. **Brent Zundel:** Sorry, my understanding is that the normative bits have just been moved around, so it may be more normative than I expected. **Gabe Cohen:** Yeah, I was going to say what you said -- I think I made a mistake with that first sentence so I will adjust that after Mike gives it a review. **Manu Sporny:** I don't think you made a mistake, it's confusing or wrong original text that you didn't add Gabe. **Ted Thibodeau Jr.:** I added the all-caps OPTIONAL based on the following text -- it needs to be consistent whatever it is. **Michael Jones:** JOSE requires `alg`. **Brent Zundel:** Good feedback on that PR, it's actionable, encourage folks to keep an eye on it as it is reviewed and updated.
decentralgabe commented 6 months ago

@msporny noting that after discussion we realized a misunderstanding, alg is optional in a JWK.

selfissued commented 6 months ago

I misunderstood the context when we were discussing this on the call. alg is a required header parameter, which is what I thought we were discussing. alg is not required in JWKs.

brentzundel commented 6 months ago

@msporny and @TallTed I believe the changes you've requested have been made, could you please re-review?

iherman commented 6 months ago

The issue was discussed in a meeting on 2024-02-14

View the transcript #### 1.3. Add guidance around using JWK (pr vc-jose-cose#220) _See github pull request [vc-jose-cose#220](https://github.com/w3c/vc-jose-cose/pull/220)._ **Gabe Cohen:** One thing I forgot to mention -- there's some outstanding discussion around 220 ... around the JsonWebKey text that we should discuss to get clarity around some confusion that came up. … This PR is clarifying language on the JsonWebKey spec on how to use the JsonWebKey type. There was discussion on a call a few weeks ago on whether we should add language on including properties like `alg` and `kid` and at first there was agreement to add back normative guidance on those properties. … But then Mike and I agreed we didn't want to repeat language from another RFC -- and so we removed that. Ted said he wanted the language back. **Ted Thibodeau Jr.:** the language that was removed was removed during misunderstanding of what was being discussed...point being the four words were added with intent and removed without that intent, which is why I've asked them to be re-added. **Manu Sporny:** the language being modified is normative language that is significant. need to update the title of the PR, since it's broader than the example. somewhat confused...had said we'd have explicit guidance on iss, kid, etc. that guidance was not provided...may be a different issue. if we're talking about keys and just a JWT, and if we're just repeating what's said in the other spec we don't need to repeat it here. somewhat confusing...since kid matters. _See github pull request [vc-jose-cose#226](https://github.com/w3c/vc-jose-cose/pull/226)._ **Gabe Cohen:** The changes you're referring to Manu, went into 226. The changes we're talking about ... the PR is unfortunately named. The language I moved was originally in an example. … The issue was to move it to normative guidance, outside of the example. … The PR adds some guidance around using the JsonWebKey. **Ted Thibodeau Jr.:** On Jan 18th, I said optional or required should be clearly stated for all properties, that's generally true what's happening but not true for the couple that were added with this PR. … Those changes went in and were merged but then Mike said that some requirements were wrong. … `alg` is optional in JWKs -- and that's what I want put back. **Michael Jones:** Are we talking about a change to header params or JWKs? **Gabe Cohen:** JWKs. **Michael Jones:** It's optional there, what does it say now? **Gabe Cohen:** Nothing. **Michael Jones:** It's fine to say that. … This is one of the PRs I was trying to get a sense of ... is this one controversial or is that another one? **Gabe Cohen:** It sounds like we're clear on this one, I'll apply Ted's suggestion and then we're good. … The other has a rendering script problem. **Michael Jones:** Ok, 220 should be ready once we get the suggestion in.
decentralgabe commented 6 months ago

changes addressed, merging.