-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify semantics between jwt claims and vc properties #226
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -413,13 +413,24 @@ <h2>JOSE Header Parameters and JWT Claims</h2> | |||||||||||||||||||
defined in <a data-cite="VC-DATA-MODEL-2.0#validity-period">Validity Period</a>, | ||||||||||||||||||||
which represent the validity of the data that is being secured. | ||||||||||||||||||||
</p> | ||||||||||||||||||||
<p> | ||||||||||||||||||||
The claims and security provided by this specification are independent of the data | ||||||||||||||||||||
secured and semantics provided by the [[VC-DATA-MODEL-2.0]]. This means that while the security | ||||||||||||||||||||
features of this specification ensure data integrity and authenticity, they do | ||||||||||||||||||||
not dictate the interpretation of claim data. | ||||||||||||||||||||
</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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. verifiable credential properties when the claims and properties refer to the same concept, for example, |
||||||||||||||||||||
`iss` and `issuer`, `jti` and `id`, and `sub` and `credentialSubject.id`. | ||||||||||||||||||||
Comment on lines
+423
to
+425
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @David-Chadwick — Answering your comment --
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:
Suggested change
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe try manually using human language and Markdown formatting around your suggested changes, something like --
I've created issue #235 to track this going forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many thanks |
||||||||||||||||||||
</p> | ||||||||||||||||||||
<p> | ||||||||||||||||||||
The JWT Claim Names <code>vc</code> and <code>vp</code> | ||||||||||||||||||||
MUST NOT be present. | ||||||||||||||||||||
</p> | ||||||||||||||||||||
<p> | ||||||||||||||||||||
Additional members may be present as header parameters and claims. | ||||||||||||||||||||
If they are not understood, they MUST be ignored. | ||||||||||||||||||||
If they are not understood, they MUST be ignored. | ||||||||||||||||||||
</p> | ||||||||||||||||||||
</section> | ||||||||||||||||||||
</section> | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHOULD -> MUST
otherwise if an implementation does set conflicting values what does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.