Skip to content
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

Merged
merged 15 commits into from
Apr 13, 2023
Merged

Editorial and non editorial changes #68

merged 15 commits into from
Apr 13, 2023

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Mar 31, 2023

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

@iherman
Copy link
Member

iherman commented Mar 31, 2023

"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.
Copy link
Contributor

@andresuribe87 andresuribe87 Mar 31, 2023

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 to vc+jwt. Explicitly say that this is not recommended (I think it isn't, but correct me here if I'm wrong).
  • From vc+jwt to vc+ld+json. This is what you added in the appendix.
  • From vc+ld+json to vc+ld+jwt. I would add recommendations of when to do this. I think it's important to explicitly describe the relationship between an embedded proof, and the external proof mechanism of jwt.
  • From vc+ld+jwt to vc+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.

Copy link
Contributor Author

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:

#69

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
OR13 and others added 2 commits March 31, 2023 17:47
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>
@OR13
Copy link
Contributor Author

OR13 commented Apr 1, 2023

@TallTed @andresuribe87 Thank you for your reviews!

Copy link
Contributor

@mprorock mprorock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Collaborator

@selfissued selfissued left a 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.

@iherman
Copy link
Member

iherman commented Apr 5, 2023

The issue was discussed in a meeting on 2023-04-05

  • no resolutions were taken
View the transcript

2. 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.
… Other PRs look fairly straightforward and simple.
… Data integrity spec is fine, nothing stuck there, same for EdDSA and ECDSA.
… Need to do first public working drafts for EdDSA and ECDSA.
… StatusList2021, not a whole not of progress. If folks are looking for work, that would be a good place to start.

Brent Zundel: FPWD is important so that other W3C groups can review and so that patent declaration has time.
… Want to enter FPWD as soon as possible.

See github pull request vc-jwt#68.

Orie Steele: #68 is an open pull request on VC-JWT what we want to merge.
… Document is still close to the format it was in when it was copied out of the core spec.
… Talked to Tobias at IETF about BBS, still in the process of adopting it.
… Happy to do that work.

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.

Michael Prorock: yep - on the lookout.

Copy link
Contributor

@awoie awoie left a 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.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
OR13 and others added 2 commits April 6, 2023 07:46
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@brentzundel
Copy link
Member

I'm not challenging the move, but am curious about the reasoning behind moving the transformation to an appendix.

@Sakurann
Copy link
Contributor

Sakurann commented Apr 6, 2023

I personally think because it is a way to transform (not THE way), it should be in the appendix?

OR13 and others added 3 commits April 6, 2023 12:16
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>
@OR13
Copy link
Contributor Author

OR13 commented Apr 6, 2023

@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>
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<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>.
Copy link
Member

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?

Copy link
Contributor Author

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>
Copy link
Member

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.

Copy link
Contributor Author

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>
Copy link
Member

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?

Copy link
Contributor Author

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>
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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"?

Copy link
Contributor Author

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".

index.html Outdated Show resolved Hide resolved
Comment on lines +939 to +942
<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.
Copy link
Member

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.

Copy link
Contributor Author

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>
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in 76b5c6c

Comment on lines +1003 to +1010
<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>
Copy link
Member

@msporny msporny Apr 8, 2023

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.

Copy link
Contributor Author

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:

#73

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>"
Copy link
Member

@msporny msporny Apr 8, 2023

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

Copy link
Contributor Author

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 Show resolved Hide resolved
index.html Outdated
Comment on lines 1054 to 1055
the <code>NumericDate</code> described in RFC 7519 to a <code>dateTime</code> as
described in XMLSCHEMA11-2.
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

@msporny msporny left a 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.

OR13 and others added 2 commits April 11, 2023 09:46
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
@OR13
Copy link
Contributor Author

OR13 commented Apr 11, 2023

@msporny

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.

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.

  • 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.

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.

  • 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).

This PR does not add this, it moves the existing section, which was added in:

https://github.com/w3c/vc-jwt/pull/51/files#diff-0eb547304658805aad788d320f10bf1f292797b5e6d745a3bf617584da017051R707

  • 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.

This PR does not define the transformation, it moves the transformation that was supplied in:

#60

To the appendix, as was requested here:

#60 (comment)

and partially implemented by @brentzundel

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".

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.

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,

Thanks I think I have captured the major one here:

#73

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.

@iherman
Copy link
Member

iherman commented Apr 13, 2023

The issue was discussed in a meeting on 2023-04-12

  • no resolutions were taken
View the transcript

3.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.
… Open pull request for vc-jwt. Some awkward sections cut from ver. 1. Have added around that some vision for enhancements and improvements for securing JSON-LD with JWTs but structure of doc difficult to revise.

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.
… what are we doing with transformation algorithms, are the testable, etc.

Dave Longley: +1 to the idea that there's a lot that needs to change in the VC-JWT document to reach consensus and make something usable, but FPWD can happen first.

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.

Manu Sporny: I'm not super concerned either way, but would prefer we get it in better shape before FPWD, but we can also do that after FPWD..

Orie Steele: mappings and interpretations of the mapping in the core model and in the spec dir are differing...
… anything that needs done in this context?

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.
… starts the clock on patent disclosures and says its a tech we as a working group want to pursue.
… encourages folks to look through the PR that Orie has drafted, if something causes concern look for an issue that is already open on it, consider raising it later.

Orie Steele: I agree with JoeAndrieu, its not clear what you want me to do.

Joe Andrieu: Sounds like it's both a working draft and editor's draft.

Dave Longley: +1 to brent's suggestion.

Orie Steele: happy to do whatever..

Manu Sporny: +1 to "Orie go for it, we'll review, then FPWD" -- and time box the review to a week so people can't hold it up..

PhilF: +1 to Orie getting it squared away first.

Brent Zundel: should we ask Orie to finish it and then review the FPWD?
… no objections to Orie shaping the document as an editor and presenting it to the group.

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.

@OR13 OR13 requested review from TallTed and msporny April 13, 2023 14:13
@OR13
Copy link
Contributor Author

OR13 commented Apr 13, 2023

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.

@OR13 OR13 merged commit 7335c51 into main Apr 13, 2023
@TallTed
Copy link
Member

TallTed commented Apr 14, 2023

I am befuddled by the merge a minute after the request for review.

What are you asking @msporny and I to review?

@msporny
Copy link
Member

msporny commented Apr 14, 2023

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 main" capacity and will notify the WG to do a full review of the spec, including all of his unreviewerd changes, before FPWD.

@OR13 OR13 deleted the fixes-and-examples branch June 7, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants