Clarify semantics between jwt claims and vc properties #226
Conversation
Co-authored-by: Ted Thibodeau Jr <[email protected]>
| `issuer` with `iss`, `id` with `jti`, and `credentialSubject.id` with `sub`. | ||
| </p> | ||
| <p> | ||
| Implementers SHOULD avoid setting JWT claims to values that conflict with |
There was a problem hiding this comment.
SHOULD -> MUST
otherwise if an implementation does set conflicting values what does this mean?
There was a problem hiding this comment.
It means that implementer (of Issuer and/or Holder/Presenter software) SHOULD understand that they're introducing inconsistencies to their data, and that downstream implementers (of Holder/Receiver and/or Verifier software) MAY reject such JWTs and their enclosed VCs.
There was a problem hiding this comment.
@David-Chadwick responding here. I agree with @TallTed's interpretation.
If @selfissued also believes this language is better as a MUST I will make a revision PR.
| </p> | ||
| <p> | ||
| Implementers SHOULD avoid setting JWT claims to values that conflict with | ||
| verifiable credential properties, especially with pairs such as |
There was a problem hiding this comment.
verifiable credential properties when the claims and properties refer to the same concept, for example,
|
The issue was discussed in a meeting on 2024-02-14
View the transcript1.3. Add guidance around using JWK (pr vc-jose-cose#220)See github pull request vc-jose-cose#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. 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. 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. 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. 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. Gabe Cohen: It sounds like we're clear on this one, I'll apply Ted's suggestion and then we're good. Michael Jones: Ok, 220 should be ready once we get the suggestion in. |
|
@decentralgabe This PR was merged without addressing my requested editorial change. |
| Implementers SHOULD avoid setting JWT claims to values that conflict with | ||
| verifiable credential properties, especially with pairs such as | ||
| `iss` and `issuer`, `jti` and `id`, and `sub` and `credentialSubject.id`. |
There was a problem hiding this comment.
@David-Chadwick — Answering your comment --
@decentralgabe This PR was merged without addressing my requested editorial change.
I think you're referring to this comment as your "requested editorial change".
Please note that your intent was not obvious because you did not submit it as a change request using the GitHub tooling, which would have shown the existing PR content in contrast to your suggestion, as you will see below, in my suggested massage of your suggestion:
| Implementers SHOULD avoid setting JWT claims to values that conflict with | |
| verifiable credential properties, especially with pairs such as | |
| `iss` and `issuer`, `jti` and `id`, and `sub` and `credentialSubject.id`. | |
| Implementers SHOULD avoid setting JWT claims to values that conflict with | |
| the values of verifiable credential properties when a claim and property | |
| pair refer to the same conceptual entity, especially with pairs such as | |
| `iss` and `issuer`, `jti` and `id`, and `sub` and `credentialSubject.id`. | |
| For example, JWK claim `iss` should not be set to a value which conflicts | |
| with the value of verifiable credential property `issuer`. |
I think a new PR (possibly starting with a new issue) will be needed to enact this (to my eyes, editorial) change, which I believe should only touch this paragraph.
There was a problem hiding this comment.
Correct. It is only this paragraph that needs changing. But note that the GitHub tooling did not work (again) otherwise I would have used it.
There was a problem hiding this comment.
note that the GitHub tooling did not work (again) otherwise I would have used it
Maybe try manually using human language and Markdown formatting around your suggested changes, something like --
I suggest line 424 above --
verifiable credential properties, especially with pairs such as-- be changed to the following --
verifiable credential properties when the claims and properties refer to the same concept, for example,
I've created issue #235 to track this going forward.
fix #205
open to feedback @msporny @selfissued
💥 Error: 502 Bad Gateway 💥
PR Preview failed to build. (Last tried on Feb 1, 2024, 8:58 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.