Pre-CR review from Jeffrey Yasskin about vc-data-model

[OR13]

Seems like it might be nice to split these up into separate issues, I'm happy to help address them, but I fear doing so on this issue will create a mega thread that is not resolvable.

[jyasskin]

FWIW, I'm happy for you to go through this however you want, but if it were my spec, I'd probably turn each paragraph into a task as described at https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/about-task-lists, and check them off as you either file issues for them, fix them with simple PRs, or decide they're not actually issues.

[jyasskin]

Yep, Manu forwarded my email in https://lists.w3.org/Archives/Public/public-vc-wg/2023Sep/0054.html.

[iherman]

The issue was discussed in a meeting on 2023-09-26

• no resolutions were taken

View the transcript

2.1. Pre-CR review from Jeffrey Yasskin (issue vc-data-model#1285)

See github issue vc-data-model#1285.

Kristina Yasuda: Pre-CR review from Jeffrey Yaskin, it's a lot.

Manu Sporny: This is a massive review by Jeffrey Yaskin from Google. The good news is that there is only one thing that I think he really wants us to fix is to see an algorithm -- it needs to be turned into an issue. He would like to see the algorithm for verification and wants it defined in the spec.
… He notes that Google might object if it isn't present.
… He spent the whole trip back from TPAC doing a large review, lots of commentary, will take a long time to triage.
… This is a heads up to the group that it will take significant work to close this out.
… Jeffrey said he doesn't care how we process it, one issue is before CR. We could say everything else is post CR, but sometimes when you start talking about the changes it becomes evident that a normative change is desirable.
… It's going to take weeks, easily, to fully process.

Kristina Yasuda: Thank you.
… I don't think it's realistic to keep processing this review in this one issue. I'll take an action to separate this into multiple issues. Ideally not per comment, but ... editorial ones, big ones. Thematically / topic-wise. Then we'll take it from there.
… I'll try to label pre/post CR based on editorial/normative.
… Any objections?

Manu Sporny: No objections, thank you, lots of work. I started going through his comments and marking them as editorial or normative and hoping that he provides some feedback on it, hopefully that helps you categorize.
… Again, don't block on me doing that though.
… I could keep doing that over the next week, or would you rather just process on your own time frame.

Kristina Yasuda: Sorry, you're suggesting to keep going back and forth with Jeffrey?

Manu Sporny: No, I'll put a link.

Kristina Yasuda: I will build up on that.
… Anything else you want to add to what you started here? Obviously you didn't get to the end.

Manu Sporny: Yes, after two hours only through section 4. I would continue through section 8 or whatever.

Kristina Yasuda: I will turn things up to section 4 into issues and then I can build up on the next bit.

Manu Sporny: Sounds great, thanks.

Kristina Yasuda: For everyone else, if you're willing to take a look at the issue, if you'd rather wait for Manu/me to break up it up, that's fine too, but input welcome.
… Moving on.

[msporny]

@jyasskin wrote:

FWIW, I'm happy for you to go through this however you want

A few follow up questions:

1. You said you had a "serious issue" -- I think you mean that to be the "algorithm on how you trust the issuer isn't defined" concern you raised? If not, which was your "serious issue"?
2. We are trying to go into CR, a large number of your comments are arguably editorial -- would you be opposed to us handling your editorial comments after we get into CR? IOW, could you make it clear which of your comments you believe to be substantive/not editorial so we can prioritize addressing them before CR (and then leave the editorial comments for during CR)?

[msporny]

This is a review submitted by @jyasskin on the VCDM v2.0:

@jyasskin, I'm going to try to go through and do a first pass of thoughts on your review and clearly outline if I think they're editorial or not. This is just a first-glance triaging, feel free to disagree with the triaging and note things that you feel strongly about.

I found one maybe-serious issue and a big pile of minor issues and nitpicks

Which issue is that, could you highlight it please?

General

[Important] Where is the algorithm that a verifier is supposed to run in order to check that the claims in a VC have been vouched by an issuer it trusts?

That is a part of validation, which is out of scope. The general algorithm that most implementations use today is:

1. Check the securing mechanism, is it signed by an entity that is trusted by the verifier?
2. Check to see if the issuer is associated with the signer of the VC in some way.

The Data Integrity specification (one of the securing mechanisms that can be used for VCs) has an algorithm that is run to bind the signing key to the controller of that key: https://w3c.github.io/vc-data-integrity/#retrieve-verification-method .. the controller of that document is typically what is placed in the issuer field of the VC. We have a section on that here: https://w3c.github.io/vc-data-integrity/#relationship-to-verifiable-credentials

Are "attribute" and "property" synonyms? Probably align on just one of the terms, or clarify the difference.

Yes, they are, and agreed.

This is editorial.

There are lots of definitions of properties, but very few statements that say which properties may or must appear on Verifiable Credentials. Did I miss a section for that? 4.11 Presentations does it for Verifiable Presentations.

Each section states whether the property MUST be on a VC, these include: @context, type, credentialSubject, and issuer. What would you prefer we do here, the spec is clear which properties MAY/MUST appear on VCs.

If we do any correction here, it would be editorial.

1.2 Ecosystem Overview

It's unfortunate that the different efforts in this space use different terminology. E.g. the verifier would probably be called the "relying party" in an OAuth or FedCM context. An issuer might be an "attester" in a privacy pass context. Would it be possible to mention those translations in this section next to the synonyms that are used in this document? I see that they're defined in 2 Terminology, but I think this might all be clearer if the definitions were in the sections where they naturally appear instead of being duplicated into a single Terminology section.

We had attempted to have Terminology throughout the specification a /very long time ago/ -- people didn't like it and wanted a terminology section at the top so that people could get acquainted with the basic terminology before reading the specification.

If we do any correction here, it would be editorial.

1.3 Use Cases and Requirements

As this section is non-normative, it's best to avoid using the normative keywords like "Issuers revoking verifiable credentials should distinguish…" even if those keywords are kept lowercase so that RFC8174 says they don't have their normative meanings. Alternatively, you could move these statements to a section of conformance requirements for issuers (or their other subjects).

Agreed, we can shift to non-normative wording. We might move this section out of the specification completely to the use cases document.

If we do any changes here, it would be editorial.

1.4 Conformance

I think you have 2 or 3 conformance classes that aren't mentioned here. Since the spec includes statements like "An issuer MUST" and "A verifier SHOULD", software can conform to this spec by being a conforming issuer or a conforming verifier. And you should probably have more statements that constrain verifiers, for example to enforce the properties desired in 7.10 Validity Checks. I suspect that holders (or credential repositories?) don't need to be a conformance class, but you might want to turn statements like "Holders should be warned by their software" into active requirements on the repositories, which would make them a conformance class.

Hrm, we could add the conformance classes you mention. Since this is a data model specification, we have traditionally focused on just "processors". We could add the two conformance classes you mention.

This would be a substantive/normative change.

2. Terminology

https://w3c.github.io/vc-data-model/#dfn-issuers are trusted by verifiers, but I don't see a statement of what exactly identifies the thing that's trusted. Is it a public key? Control of a URL?

The thing that identifies and issuer in a VC is the issuer field, which is defined here:

https://w3c.github.io/vc-data-model/#issuer

This section should be normative because it defines terms used by normative sections.

Hrm, the group has decided that terminology should be informative and as gone to great lengths to avoid using RFC language in the terminology. The thing that makes whether or not the definition is normative is its inclusion in normative statements elsewhere in the specification. While we COULD make the section normative, there are no RFC keywords in it to have any effect. What do you prefer we do here?

This change would be arguably substantive, but not normative (given the lack of RFC keywords in the section).

3.2 Credentials

This talks about claims being "made by" an entity, but 3.1 Claims did not introduce the concept of quoting.

This also shows a "Verifiable Credential Graph" being used as the subject of a claim, which isn't introduced in 3.1.

I see that "proof" is defined as {..."@container": "@graph"}, but if I'm reading https://www.w3.org/TR/json-ld/#graph-containers correctly, that makes its target a graph, not its source.

3.2 is meant to be a general introduction to the space, not a deep dive. Introducing the concepts you're highlighting that early might make it harder to get the basic concepts down. What specific changes are you requesting in this section? I'm not sure it would be a good idea to introduce the latter two items in this section (this early in the specification).

This change would be editorial, if we were to make one.

3.3 Presentations

"arbitrary additional data encoded as JSON-LD": I'd thought the -LD part was optional?

It was in v1.0 and v1.1, that is no longer true in v2.0 (though, we have gone to great lengths to make sure that people that don't want to use -LD don't have to think about it (the default context contains a default vocabulary that does not require the definition or usage of any further context files): https://w3c.github.io/vc-data-model/#getting-started

There is no change required here.

4.1 Getting Started

This section should probably be non-normative.

Agreed.

This change would be editorial.

---

I'm going to have to make several passes at the comments to make progress.

[jyasskin]

I don't have an opinion on when you'll be ready to go to CR, and since I'm not in the WG, I wouldn't get a vote anyway. :) You can also update the CR stage as many times as you want. I'm trying to anticipate issues that I'd complain about or formally object to during the AC review at the PR stage. You're also right that lots of my comments here are editorial and minor enough that I wouldn't bother saying anything during AC review, but I mentioned them now in case it helps improve the document.

The serious issue is the one about defining the algorithm for verification. The definition of validation in this spec says that verification is in scope: "This specification is constrained to verifying verifiable credentials and verifiable presentations regardless of their usage. Validating verifiable credentials or verifiable presentations is outside the scope of this specification." To be clear, I'll encourage Google to formally object to this spec if an algorithm for verification isn't moved to Proposed REC at the same time as this spec. It's fine for this algorithm to call out to algorithms in other specs, or to look up subroutines in a registry, but the top-level algorithm needs to be defined. That's for two reasons:

1. It's hard to be confident of interoperability if implementations have to gather verification requirements from many places across this and other specifications. If an implementation misses one or finds an extra one, it won't interoperate with implementations that found a different set of requirements.
2. The security and privacy properties of VCs depend critically on the exact algorithm that a verifier follows. Privacy, for example, gets compromised if a verifier fetches an extra URL that happens to identify the credential that it's verifying. In order for security and privacy reviewers to check that this spec meets its goals, they have to be able to read the verification algorithm.

---

Answers to the more minor questions:

• On whether terminology is normative, Francois Daoust pointed out https://www.w3.org/TR/qaframe-spec/, but that meta-spec doesn't give a clear answer either. My reasoning is that everything an implementer needs to know in order to write a conforming implementation needs to be in a normative section, and so if a MUST statement uses a term whose definition is critical to obeying the MUST, that term's definition is normative too, even if the definition doesn't use RFC2119 language.
• 3.2 Credentials has a Figure 6 that shows [<graph> "proof" <node>]. (This confused me because I learned RDF 20 years ago and hadn't noticed that y'all solved the quoting problem. 🙃 But someone who's learning RDF from this spec's introduction might also be confused.) But more importantly, if I'm reading https://www.w3.org/TR/json-ld/#graph-containers correctly and ran the right experiment with the playground, the relation that actually appears in VCs is [<node> "proof" <graph>]. The figure should match the data model.

[Sakurann]

@msporny @jyasskin how was this review submitted? I originally thought there was an email on the ML but couldn't find (https://lists.w3.org/Archives/Public/public-vc-wg/2023Sep/author.html). why Jeff has not opened the issue in this repository himself?

[msporny]

@msporny @jyasskin how was this review submitted?

Over email to a mailing list that was associated with the group a while ago, but was then shut down for some reason. @jyasskin cc'd both Brent and myself on the review, which is how I got it. @iherman is aware of the issue. I think we need to update the VCDM spec to provide the correct mailing list to send review comments to.

why Jeff has not opened the issue in this repository himself?

When the email didn't go through, @jyasskin asked me to post it to the appropriate place, which is when I raised this issue so that we can track the entirety of his review comments.