-
Notifications
You must be signed in to change notification settings - Fork 127
Clarification of JWT encoding #828
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
Conversation
| <li> | ||
| Transform the remaining JWT specific headers and <a>claims</a>, and add the | ||
| results to the new JSON object. | ||
| results to the new <a>credential</a> or <a>presentation</a> JSON object. |
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.
this is so great to see! I wonder if we might fully define these transformation functions, I would like to see them made explicit.
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.
@OR13 -- I suggest creating a new issue "to see [these transformation functions] made explicit".
OR13
left a comment
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.
My only real concern is: https://github.com/w3c/vc-data-model/pull/828/files#r740230917
This PR is excellent, and very much needed. thank you.
|
@brentzundel Thankyou for clarifying the position wrt normative statements. Given the importance of adding this clarification text to v1, it is more important to keep the changes without adding any new normative statements and issue this PR in v1.1. We can add normative statements in v2 if they are needed. |
|
@David-Chadwick -- This PR is showing merge conflicts. They appear to be pretty simple, and, I think, should be easily resolved. |
|
As discussed on the call we're going to remove the MAY to SHOULD change on line 3455 so that we can be certain that this text remains as a purely editorial change. I've labelled it as such since that's what was agreed to by the author of this PR. |
|
The issue was discussed in a meeting on 2021-11-03
View the transcript3.1. Clarification of JWT encoding (pr vc-data-model#828)See github pull request vc-data-model#828. David Chadwick: what i need to understand is what happens to the existing PR for v1.1 not in the text manu has pointed to.. Manu Sporny: let me try to explain. my understanding of the whole process has been changing every 2 days, because the process document doesn't outline everything that comes into play.. David Chadwick: is 1.1 containing the normative changes in there from 1.2?. Manu Sporny: yes. David Chadwick: we should put in the changes this week then. Brent Zundel: we can merge any PRs that are editorial and within scope of 1.1, we can merge into the v1.1 branch, and accumulate a body of editorial changes.. Kyle Den Hartog: with the intent of doing a V2 spec pretty soon after, we have to work through the chartering stuff. the intent of any of these substantive changes that are coming in is to have them fit into the pure view of the v2 scope. some of the more substantive changes we've been looking at, related to jwt work, we're talking about separating out the proof suites into separate documents about them specifically. Kyle Den Hartog: so it could be useful to hold off on the jwt work, because we would want to see jwt into its own document. in that case it would make sense for those things to work together. while it's difficult for us to get stuff into 1.1, it's not the intent of the WG to stall the work, there's just a slight delay due to rechartering.. David Chadwick: that's fine, i don't mind a separate document for JWT next time around, but the situation we're in at the moment, is that the current JWT section in the spec lacks clarity, and the PR brings the clarity. in my opinion, it's imperative that those changes go in the 9th of november.. Kyle Den Hartog: is this actually editorial?. David Chadwick: the current spec doesn't say anything about how bearer credentials are dealt with. this PR clarifies what people can infer but may have inferred wrongly.. Manu Sporny: pull 828?. Brent Zundel: yes. David Chadwick: the one to do with JWT is the important one, which adds text. Manu Sporny: just on the responsiveness, you raised the PR days ago, responses from ted and markus on the day. i retargeted the branch, there were merge conflicts. orie said it was good but don't merge, 2 days ago, esp. with the removal of nonce.
Manu Sporny: there is a change to normative language, like 3455, you're changing a MAY to a SHOULD. Brent Zundel: making an assertion and asking a response to you DavidC. i am asserting that further editorial changes will not be made to the document that is being reviewed by the AC. and having made that assertion, i must ask, in light of that, is it your intention to formally object to the document that is under review?. David Chadwick: i'd like the changes in. Brent Zundel: if that doesn't happen, would you formally object?. David Chadwick: i will discuss with the people who have asked for the changes and see if it's acceptable or not. i was in the fortunate position of understanding the JWTs because i was part of the VC drafting process. but the majority of the people implementing do not have the background and knowledge.. Brent Zundel: no one is saying we don't want these changes. it's about timing-wise. David Chadwick: will the draft be visible to people?. Brent Zundel: yes, it will be possible to point to people to a published document that we can say will be coming on jan 15th. David Chadwick: are you saying that the pointer in the document which manu pointed us to, which is w3c recommendation 9th of november, 1.1, where it says latest editor's draft, that will have the PR in it for JWT once it's been agreed to. is that right?. Brent Zundel: yes. David Chadwick: so people will have that visible?. Brent Zundel: yes, it won't be TR space. David Chadwick: that may be acceptable. Brent Zundel: totally agreed, it's just about the timing & process. David Chadwick: i don't want to give you hours of work, that's not fair. it's just sad that this wasn't done more quickly. Brent Zundel: no disagreement. Kyle Den Hartog: when you're in conversations with these people who may object, the solution we've put forward, can you be sure to mention that the text going into v1.1 editor's draft will be there. i'll put a link before closing the PR so it's easier for people to find in the editor's draft in the intermediary.. David Chadwick: they will just need to see the v1.1, click the "latest editor's draft" and see it anyway. that's what brent said. Manu Sporny: quick reminder, it came from "i'm representing..." it's problematic to potentially cross anti-trust boundaries at W3C. big companies have been known to hire small companies to pack a room, not saying you're doing that, but it's really that kind of behavior that's frowned upon at w3c. these people can jump on the issue tracker and provide the feedback to this group, and they should not be doing it through you. Manu Sporny: it makes it difficult to have a direct conversation with people who have the issue... |
|
The issue was discussed in a meeting on 2021-11-03
View the transcript4.3. Clarification of JWT encoding (pr vc-data-model#828)See github pull request vc-data-model#828. Brent Zundel: david's PR, 828, you're welcome to walk us through this set of changes. David Chadwick: i don't see the same text i saw when manu said it...the MAY/SHOULD is not visible to me. Brent Zundel: sharing my screen. on 3455, we have may going to should. David Chadwick: this is the only one that is still contentious. i didn't think this was a normative change from the implementation perspective, therefore there's no mandatory requirement. ted, are you available to comment on this?. Ted Thibodeau Jr.: it is a normative change but does not make an implementation of the MAY non-compliant.. David Chadwick: so the real issue is: is this prohibited from the v1.1 editorial?. Kyle Den Hartog: the precedent we've held throughout this WG is any change to normative text requires it to be a substantive change, based on my reading of the different classification of changes in the W3C process, so my preference is to leave it as a MAY for now, SHOULD later when it's easier. Manu Sporny: the danger DavidC is that if we make that an editorial change, any W3C member can force us to put everything through a candidate correction, making all the editorial changes we've made under review, adding months to timeline. David Chadwick: agreed, we will keep the normative text as-is. Brent Zundel: we are out of time. David Chadwick: already in the PR. Brent Zundel: the process of determining this is still ongoing.. David Chadwick: yeah probably not. Brent Zundel: Thanks to everyone that came, still have v1.1. issues. |
|
The issue was discussed in a meeting on 2021-11-10
View the transcript3.4. Clarification of JWT encoding (pr vc-data-model#828)See github pull request vc-data-model#828. David Chadwick: The last time we discussed, I understood -- 2 changes, remove the SHOULD believing that wasn't normative..
Brent Zundel: suggested we keep the nonce, and then describe nonce more fully later for all the cases it is present. Manu Sporny: nonce is defined in OIDC as a valid claim name so we can use it. |
Changed SHOULD back to MAY and added a note to describe the nonces that have been added
Fixing merge conflicts
@TallTed - some are, which I have made, and some are not. The latter appear to be changes between this PR and v1.1 that concern JWTs that are part of this PR. They should be incorporated into this PR so as to include all the changes to the JWT description that need to be made |
The nonce has been re-inserted along with an explanatory sentence
@kdenhartog This has now been done. |
msporny
left a comment
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.
Only one remaining normative change. Once that's fixed, this is ready to be merged.
|
I've gone and resolved the merge conflict at the request of @David-Chadwick. I made my best effort to align the text with my understanding of what has been agreed to at this point, but there were some instances where the conflicts were related to the text still in the process of being discussed. Can we get the reviewers (@peacekeeper @TallTed @brentzundel @OR13 @msporny) as well as the author (@David-Chadwick) to take a look at this again when they have the time to just to make sure I didn't accidentally change something incorrectly. |
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.
Saddly, I agree that we can't call this change editorial:
https://github.com/w3c/vc-data-model/pull/828/files#r749558939
I really, really, wish we could fix this, but IMO its not a good move to do it in this change set, and without clearer code review from JWT implementers.
cc @selfissued
| <li> | ||
| <code>vc</code>: JSON object, which MUST be present in a JWT | ||
| <a>verifiable credential</a>. The object contains the | ||
| <a>verifiable credential</a> according to this specification. |
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.
Can you clarify why a vast majority of the "verifiable" terms are being removed?
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.
probably because its not actually the same type... in JWT land, jwt.payload.vc.issuanceDate is maybe not required if the 'instead of' path is taken... leading to type ambiguity if the term verifiable credential is used to refer to the vc payload... the jwt.payload.vc is in fact not verifiable... only the jwt (the outer container that includes it) make sit verifiable.
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.
this is in line with the langauge we use as well:
credential: JSON with an @context that contains all required fields except for proof
-> issue ->
verifiable credential <JWT or LD Proof>
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.
if issuanceDate is a required attribute of a verifiable credential (thats what the spec says today)...then removing it destroys interoperability and conformance.... calling the thing that might not have that property the same as the thing that has to have that property is problematic.
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.
if issuanceDate is a required attribute of a verifiable credential (thats what the spec says today)...then removing it destroys interoperability and conformance
Yes, and (removing issuer and issuanceDate and all other JWT-encoded properties) was exactly this was suggested on the call yesterday, which tells me that the entire VC-JWT ecosystem is not interoperable today.
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.
IMO, this change should be accepted, since vc is obviously not a verifiable credential under the current normative text... its possibly also not a credential under the current text, but this is a step in the right direction. We should not be standing in the way of a "more truthful" editorial refinement.
|
Can we all agree that a credential that is produced from all the different flavours of VCs (JWT with/without duplication, LD-Proofs of different types, ZKPs etc) after the proof has been removed will all be identical? This is what we have already said in PR #808 that has been merged. |
| <p> | ||
| For backward compatibility with JWT processors, the following registered JWT | ||
| claim names MUST be used instead of their respective standard | ||
| claim names MUST be used instead of, or in addition to, their respective standard |
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.
| claim names MUST be used instead of, or in addition to, their respective standard | |
| claim names MUST be used, instead of or in addition to, their respective standard |
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 -- You marked this resolved, and said that such marking meant you had applied my suggested change. This change was not applied. I am applying it now in a new PR.
|
@David-Chadwick I don't think we can necessarily agree to what you wrote without making substantive changes to the spec. but i think we agree in spirit.
|
|
@OR13 -- You've drawn a distinction between the classes "Verifiable Credential" and "Credential". I think this is the wrong distinction. I think that the classes "Verifiable Credential" and "[Unverifiable] Credential" (where "[Unverifiable]" is usually left unstated) are disjoint subclasses of, and together comprise the totality of, the class "Credential". This is definitely confusing, and I fear there will be a great many changes, hopefully all Editorial, because of this, but I think that is probably the rigorous, and therefore correct, thing to do. (The changes I speak of are primarily putting "Unverifiable Credential" in place of "Credential" wherever the subclass is intended, but there may be follow-on changes needed.)
-- therefore changes to --
|
I am just restating what the spec says today:
https://w3c.github.io/vc-data-model/#proofs-signatures But I agree with this part very strongly:
|
I cannot agree more strongly with this statement. I've been saying that for years... yes, it can be fixed, but people have been promising to do that for years as well (and yet, it keeps not happening). Anyway, we'll deal with this in the VC 2.0 work, if it's not formally objected into oblivion. :P |
|
@OR13 "I don't think we can necessarily agree to what you wrote without making substantive changes to the spec. but i think we agree in spirit."
|
|
@OR13 "especially aware that the current Microsoft verifiable credentials implementation uses "instead of" and NOT "in addition to"..." |
|
@TallTed I disagree with your credential classification. Credential is the superclass. A verifiable credential is a subclass of credential and is a credential with a proof property. Therefore a currently unverifiable credential i.e. a credential without a proof property is still a credential and cannot be differentiated from it, whereas a verifiable credential can be differentiated as it is a specialisation and has a proof property. If you are implying that we should introduce the class of object "never verifiable credential" to be a subclass of credential that can never have a proof property added to it, then I reject this notion. It has no place in the VC standard and we should not introduce it. |
That's what I said. You do not appear to disagree with me.
Indeed, we should not introduce this notion, and I wish you had not done so here! Having reread my writings above, I don't see how anything I wrote could be construed this way! In bastardized (ETA: and incomplete; there are many attributes of these, and possibly other entities, which would need to be present for this to be complete; the snippet below is purely to clarify my understanding of Perhaps that's clearer? |
Slight correction after reading through @OR13 post.
This aligns with what I agree with and I glossed over the response to quickly to realize that nuance.
Yeah, this is why I raised the question is because when reading it we say the credential in JWT format and it made me think "wait, is this implying the full JWT is a 'credential' or is this just referring to the payload portion". If the intent of this change was to refer only to the JWT format payload, then I agree that these should be set to credential, but that wasn't clear to me while reading it hence raising the question. I think the difference here is we're aligned at the 10,000 foot view, but the text may be leading us to different conclusions for implementations as @OR13 points out which means we may need to be more thorough in the updates we're making to the text here. If this means that we need to make normative statements then so be it, and so I think the best we can do is raise warnings in the text to call this out similar to what I did for the I'm concerned about the idea of trying to overly lead people in a particular direction at this point while the group has a limited number of people participating because I don't want V1.1 changes to lead us in one direction and then revert course when we get to V2 work and more people start attending. If we can avoid that in your second option mentioned above @David-Chadwick I'd be alright doing option 2, but my preference would be to go with option 1 just because I think that's more likely to lead us to a robust long term outcome. |
|
@kdenhartog. I agree that it is important that we all agree on the principles. My original wording about credential was quickly written, here is a more precise version for us to agree to.
|
|
@TallTed . It all depends upon how you define the superclass and then we dont need an [edited by @TallTed to add appropriate codefences] |
[Note: I have edited the above quote, codefencing the HTML tags here which were codefenced in my original but not in what I'm guessing @David-Chadwick copied-and-pasted. -- @TallTed] Your changes are agreed to, which is why I marked it as resolved. I tried to commit your changes using my browser but every time I do this for your comments I get an error invalid email address. I dont know if this means your address is invalid or mine. But I have committed other people's changes OK, so is there something wrong with your email address? |
| <li> | ||
| <code>vp</code>: JSON object, which MUST be present in a JWT | ||
| <a>verifiable presentation</a>. The object contains the | ||
| <a>verifiable presentation</a> according to this specification. |
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.
This change should be accepted for the reasons I gave in https://github.com/w3c/vc-data-model/pull/828/files#r753698782
|
|
||
| <p> | ||
| If no explicit rule is specified, <a>properties</a> are encoded in the same way | ||
| as with a standard <a>verifiable credential</a>, and are added to the |
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.
This change should be accepted for the reasons I gave in https://github.com/w3c/vc-data-model/pull/828/files#r753698782
|
@msporny please re-review this PR and suggest any concrete changes you require for it to be accepted. |
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Please understand: If you apply my suggestion manually, but you don't explicitly acknowledge my suggestion and note that you've applied it in commit xyz, I get no notice of your agreement nor of your manual application of my suggested change, which I do need, because it is then necessary for me to manually review that commit to confirm that my suggestion was applied as intended, and not misunderstood nor accidentally misapplied, both of which have happened in the past. When/if you want to mark as "resolved" a future suggestion that you have applied or will be applying manually, please first make the change in your branch/fork/whatever, and second make a comment in reply to the suggestion with a link to the relevant commit such that we may review and confirm that the suggestion has been applied as intended, and third -- after the suggester comments to confirm that the commit accurately addressed the suggestion -- mark the suggestion, "resolved."
To the best of my knowledge, you are the only person who receives this email address error. I receive innumerable email notices from GitHub, so I'm pretty sure there's no issue with my email address. If you will provide the complete error message you're receiving, along with any context of that message, I will review them, and may then be able to determine what's actually wrong, such that you or I or whomever else is appropriate may correct it. |
Your descriptions don't eliminate the existence of the This would be more complete and more accurate -- First thing, This does not say anything about the ability (nor inability) to convert an |
|
@TallTed " If you will provide the complete error message you're receiving," I did copy you the exact error message I received. It was followed by "please try again" or similar, but this leads to exactly the same outcome. So then I have to revert to using my local copy and pushing a change up to GitHub, which is obviously more time consuming and error prone that trying to make the change using my browser. |
I am not trying to make your life more difficult nor drive you to more time consuming or error prone activities, though these seem to be side-effects of trying to patch what I see as bugs in your PRs. My request for the details of the error is intended to help enable you to use the faster and less error prone paths to applying my change requests/suggestions.
This local copy is why I need you to explicitly indicate your acceptance of a suggestion, and to provide a link to the commit, on GitHub -- because otherwise there is no indication of the acceptance of my suggestion, nor the commit which applies it, until you've pushed your changes up, which has typically been some while after you've marked my suggestion as "resolved". I have doubts that the complete error message is contained within this --
-- and certainly, this --
-- is not "the complete error you received" -- which is made plain by the "or similar"! Possibly you might need to review system and/or application logs, or look into your browser's developer views, to see the error which the browser is receiving which it might be reducing to "invalid email address". This might lead to reporting a bug against the browser, or against GitHub, or something else in the mix. What browser are you using? Any chance you might try using another to take the same steps? (fwiw, I have been primarily using Chrome on macOS for some years without issue against GitHub. If you have not, I would suggest trying -- in no particular order -- Chrome, Safari, Firefox, Opera, and/or [if on Windows] Edge.) |
|
@David-Chadwick /cc @OR13 -- The suggested issue was to track making the referenced transformation functions explicit. If that effort is being tracked some other way, that's fine with me. I just didn't want @OR13's request to be lost. |
Preview | Diff