-
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
Editorial and non editorial changes #68
Conversation
"preview" does not handle respec's includes properly, so it is pretty much useless here. This seems to work: https://raw.githack.com/w3c/vc-jwt/fixes-and-examples/index.html |
<a href="https://www.iana.org/assignments/jwt/jwt.xhtml#claims">IANA JSON Web Token Claims Registry</a> whenever possible. | ||
</p> | ||
<p class="note"> | ||
Production of this representation does not use <code>vc+ld+json</code> as an input. |
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.
I think I understand the intention of this note. But it took me a bit of reading to follow along. Perhaps readers would be better served if we add a more explicit explanation of the directions a developer may want to go. I would recommend to separate in the following fashion:
- From
vc+ld+json
tovc+jwt
. Explicitly say that this is not recommended (I think it isn't, but correct me here if I'm wrong). - From
vc+jwt
tovc+ld+json
. This is what you added in the appendix. - From
vc+ld+json
tovc+ld+jwt
. I would add recommendations of when to do this. I think it's important to explicitly describe the relationship between an embeddedproof
, and the external proof mechanism of jwt. - From
vc+ld+jwt
tovc+jwt
. Although this is obvious, I think it doesn't hurt to call it out. For this mapping, I also believe it's important to talk any relationship between the proof mechanism.
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.
I agree, we should provide more guidance than saying "it is possible / required by the resolutions of the wg"... however, that guidance is going to be hard to get consensus on.
I will file an issue to discuss first:
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Andres Uribe <andresuribe87@gmail.com> Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@TallTed @andresuribe87 Thank you for your reviews! |
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.
lgtm
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.
I believe this PR improves the specification.
The issue was discussed in a meeting on 2023-04-05
View the transcript2. Work Item status updates/PRs.Brent Zundel: Moving on to work item status updates and PRs. Manu Sporny: On VC Data Model, there are six PRs that are effectively stuck, that we need to discuss how we get them unstuck. Brent Zundel: FPWD is important so that other W3C groups can review and so that patent declaration has time. See github pull request vc-jwt#68. Orie Steele: #68 is an open pull request on VC-JWT what we want to merge. Manu Sporny: Just a note that I have sent a list of steps to Gabe, who is raising PR on transitioning from CCG to VC-JWT.
|
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.
Definitely a major improvement to the previous version and supportive of merging this. I suggest merging this PR and creating tickets for upcoming PRs.
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
I'm not challenging the move, but am curious about the reasoning behind moving the transformation to an appendix. |
I personally think because it is a way to transform (not THE way), it should be in the appendix? |
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@brentzundel What @Sakurann said, it was also a change request on your original PR. We feel it is important to be clear that different organizations might want to produce different JSON-LD based on their business use cases, at the face to face, we discussed this in depth, we don't want readers concluding there is "only one way" to map a JWT to a JSON-LD object. |
</p> | ||
<section> | ||
<h2>Credential Header</h2> | ||
<p><a data-cite="rfc7519#section-5.1">typ</a> MUST use the media type <code>vc+jwt</code>.</p> |
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.
vc+jwt
isn't a media type. I don't know what the JWT spec calls this thing, but we can't call it a media type.
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.
vc+jwt
is as much a media type as other requested media types... see also:
We are requesting this registration in the spirit of previous registrations:
<pre data-include="./examples/vc-2.0/templates/vc+jwt/claimset.json" data-include-format="json"></pre> | ||
</aside> | ||
<p class="note"> | ||
The <code>vc</code> and <code>vp</code> claims MUST NOT be present when the content type header parameter is set to <code>credential-claims-set+json</code>. |
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.
credential-claims-set+json isn't a registered IANA type... where is this value coming from?
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.
its a requested registration, that was merged by here:
</section> | ||
</section> | ||
<section> | ||
<h2>Securing <code>application/vp+ld+json</code> with JOSE</h2> |
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.
We clearly need a PR in the main spec to register this media type.
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.
I am happy to do one.
title="verifiable presentation" | ||
> | ||
<p>An encoded verifiable presentation (using external proof).</p> | ||
<pre data-include="./examples/vc-2.0/vp+ld+jwt.jose" data-include-format="text"></pre> |
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.
Same question here, is +ld+jwt
going to be a registered structured syntax suffix?
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.
same answer as before, we are unsure based on the latest suffixes draft debate.
seeing as the suffixes registrations only require expert review, we are assuming the draft will go to WGLC soon, and then we can ask for the experts to review a family of "multiple suffixes" together including +ld+json
<section> | ||
<h2>Securing <code>application/vc+ld+json</code> with COSE</h2> | ||
<p>[[rfc8152]] MAY be used to secure this media type.</p> | ||
<p><code>type (TBD)</code> MUST be <code>vc+ld+cwt</code></p> |
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 the VC-JWT specification, not the VC-CWT specification. This seems to be a change in scope for this specification, with technologies that are not listed in our charter.
How are we going to defend these additions, of COSE and CWT, to the Director and the W3C Membership when our charter doesn't mention these technologies at all?
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.
In practice, once vc+ld+json
is registered we should expect it to start showing up in all the places where cty
is allowed.
I think it is better to address the alignment between jwt
and cwt
secured vc+ld+json
up front, to avoid the problems we saw in the previous work, where the vc-jwt format was designed without a broader context.
The same "key resolution" problems will occur for COSE and JOSE, and they can both be used to secure application/vc+ld+json
, if we don't comment on it, people will just do it... many different ways.
The day 3 resolution also allows for this to be addressed outside the working group with a mapping from vanilla CWT to JSON-LD.
I have not attempted to define that mapping, but perhaps we should for consistency?
<section id="vc-ld-jwt-media-type"> | ||
<h2><code>application/vc+ld+jwt</code></h2> | ||
<p> | ||
This specification registers the <code>application/vc+ld+jwt</code> Media Type specifically for |
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 media type feels wrong... I don't understand why it's not application/vc+jwt
-- is the reason because "It's JSON-LD"?
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.
Yes, the payload (claimset) is known to be JSON-LD, instead of "just normal JSON, with registered / private claim names".
<code>application/vc+ld+jwt</code> values are encoded | ||
as a series of base64url encoded values (some of which may be the | ||
empty string) each separated from the next by a single period | ||
('.') character. |
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 needs to refer to the JWT spec for normative guidance on what an encoding should look like.
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.
https://www.iana.org/assignments/media-types/application/at+jwt
^ for example, I think this language should stay but perhaps extra citation should be added.
index.html
Outdated
</tr> | ||
<tr> | ||
<td>Security considerations: </td> | ||
<td>As defined in this specification.</td> |
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 should refer to the JWT spec's security considerations.
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.
yes!
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.
added in 76b5c6c
<h2>Appendix</h2> | ||
<section> | ||
<h2>Example Mapping</h2> | ||
<p> | ||
The following describes a mapping from <code>application/vc+jwt</code> to | ||
<code>application/vc+ld+json</code>. This is one possible unidirectional mapping | ||
between 2.0 VC-JWTs and the VC Data Model; other such mappings are possible. | ||
</p> |
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.
I expect that section runs afoul of a presumption in a resolution made at the most recent VCWG F2F Meeting, namely that "Transformation rules MUST be defined", specifically, I expect that a number of the individuals in the WG presumed that the intent here was to have normative language defined somewhere that was testable. The normative language doesn't have to be the only way "other transformation mechanism MAY be used to translate to the core data model" (understanding that language like that introduces interoperability challenges). Fundamentally, the language in this section isn't normative and therefore won't be tested by the WG.
It is imperative that if the WG is defining transformation rules, that those rules be testable and demonstrated to provide interoperability at some layer. Not providing that assurance (demonstrable interoperability) with a specification being produce by the WG would be a failure of our remit.
https://www.w3.org/2017/vc/WG/Meetings/Minutes/2023-02-16-vcwg#resolution1
This section needs to be something that has normative language, is tested during CR, and results in measurable demonstrations of multi-vendor interoperability.
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.
I am opened this issue to continue this discussion:
If the value of <code>iss</code> is a URL, use the value of <code>iss</code>. | ||
</li> | ||
<li> | ||
If the value of <code>iss</code> is not a URL, use the concatenation of "<code>urn:vc:</code>" |
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.
Is urn:vc:
going to be registered and the syntax for it explained somewhere? For example, defined in this specification and registered here? https://www.iana.org/assignments/urn-namespaces/urn-namespaces.xml
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.
Possibly, I believe it is a requirement that @id
/ id
be valid IRIs, so in the case that you need to map from a "non" IRI to a URN, it seemed necessary to invent... is there a better solution you can think of?
index.html
Outdated
the <code>NumericDate</code> described in RFC 7519 to a <code>dateTime</code> as | ||
described in XMLSCHEMA11-2. |
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.
Same refs issue as above.
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.
Thanks, this is copied from the version 1 section, and needs to be cleaned up. I will address in this PR.
the concatenation of "urn:vc:" and the value of <code>sub</code>. | ||
</li> | ||
<li> | ||
Add all of the subject claims. |
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.
There are claims in the IANA registry that don't map well to the concept of a "credential subject"... for example: acr
, azp
, locale
, zoneinfo
, auth_time
, amr
, sip_*
, sid
, act
, scope
, sph
, and token_introspection
.
What is the guidance in those scenarios?
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.
They are automatically mapped by the current transformation rules, see #60
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.
High level comments on this PR:
- It's huge and changes many normative statements simultaneously, please avoid massive PRs like this, it makes it very easy to miss important changes.
- The use of data-include has broken the PR Preview, which means that all examples are not viewable inline and the spec now requires you to download and run it locally to see the updates/changes. I doubt that most of the reviewers are doing this.
- It adds CWT and COSE to the scope of VC-JWT, after feature freeze, which seems to be difficult to reconcile with scope of the VCWG Charter (and could lead to formal objections).
- It defines the 'transformation' algorithm in a way that won't be tested by this WG and thus we won't demonstrate interoperability on the transformation algorithm in this specification.
It took over an hour to review this PR, and I'm still not confident that I understand all of the details that are changing. It's difficult to approve a PR that introduces multiple questionable changes in scope and testing simultaneously under a vague description of "Editorial and non-editorial changes".
I'm a strong +1 to rapidly updating the spec to make it better, so in that vein, I'd be a +1 to merging this, IF 1) issues are raised for all items of concern, and 2) the WG is made aware of the change in scope for VC-JWT and the signal to not test the transformation algorithm in VC-JWT.
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
Yes, sadly this document started as a few sections cut out of the v1.1 specification and has received very few "large PRs" like this.... it needs a lot more contribution.
Interesting, do you think we should no use data include moving forward? I thought it was a best practice to use the features of ReSpec, but perhaps this one is not good to use.
This PR does not add this, it moves the existing section, which was added in:
This PR does not define the transformation, it moves the transformation that was supplied in: To the appendix, as was requested here: and partially implemented by @brentzundel
Thank you for your review. I am sorry the document is not in better shape, it is not yet FPWD, perhaps we should delay FPWD and continue to make improvements to the document cc @brentzundel @Sakurann @selfissued . If the document is not yet FPWD, is there any change to process that I should be following to help this document catch up in terms of quality? Recall this documented started as a cut out of version 1.1 and has received relatively few changes since then, as most of our time has been spent discussing how the media types in the core data model interact with it.
Thanks I think I have captured the major one here: There are already several open issues that are addressing some of the comments you have left, specifically:
and 2) the WG is made aware of the change in scope for VC-JWT and the signal to not test the transformation algorithm in VC-JWT. Happy to provide an update on the next call. |
The issue was discussed in a meeting on 2023-04-12
View the transcript3.4. Editorial and non editorial changes (pr vc-jwt#68)See github pull request vc-jwt#68. Orie Steele: for VC JWT request for FPWD made and resolved in a previous meeting. Follow ups to possible objections made but it doesn't appear there will be objections. Go forward and prosper. Manu Sporny: almost of the opinion to not do a PR for JWT until its in a shape for all to see it in. The specific concerns are the way the transformation algorithm is stated is non-testable. Why are we discussing something that is optional.
Orie Steele: there are many things that need work in the PR. Media type discussion in the vc specs dir, and the securing specs referring to them, etc.
Orie Steele: mappings and interpretations of the mapping in the core model and in the spec dir are differing... Michael Jones: supports the recommendation that Orie prepare it before it becomes a working draft. Brent Zundel: process info -- publishing as a FWD has patent implications. Working drafts do not represent a consensus of the WG beyond an agreement to work on the topic.
Joe Andrieu: Sounds like it's both a working draft and editor's draft.
Brent Zundel: should we ask Orie to finish it and then review the FPWD? Orie Steele: next time does Orie need to wait 7 days or what? Brent Zundel: with the work mode proposed Orie should be able raise PRs people think are needed. Concerns can be tracked in the issue. |
I have addressed the feedback provided. Based on the call yesterday, I am merging this PR and will merge directly to main with no PRs until the document is ready to be called for FPWD. |
I am befuddled by the merge a minute after the request for review. What are you asking @msporny and I to review? |
He cleared our request for changes and merged. I don't think there was an expectation that we'd re-review. @OR13 is now operating in a "commit to |
Lots of changes in this PR, since the document is in pretty rough shape.
I moved sections around, updated examples, adjusted old text copied from version 1.1, clarified how examples map to resolutions made regarding the core data model, and removed a should requirement for an unregistered and extraneous media type.
Preview | Diff