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]

This issue has been completely triaged at this point and checklists for normative changes (issue #1347) and non-normative changes (issue #1348) have been created. Those are the new tracking issues for @jyasskin's review. I'm closing this PR to ensure that we're tracking at the new locations. Please provide commentary on the new issues.

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

[iherman]

The issue was discussed in a meeting on 2023-11-01

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

Brent Zundel: This is a long review received from the AC rep for google. My understanding that there are only one or two things requesting that could lead to a formal rejection if not addressed. Specifically, he is looking for an algorithm that a verifier should run to verify claims from an issuer.
… a number of things have been extracted into individual issues. The intention is for that to continue.

Manu Sporny: I got up to his comments on section 4.1. Still need to go to section 8.2. Kristina has done a pass as well. This is in process. I have the PR where he wants to see the verification algorithm half worked up, but there will be a PR in the next two weeks.

Brent Zundel: not seeking to assign a person to this issues because it will be broken into individual issues.

[msporny]

4.2 Contexts

This says "For reference, a copy of the base context is provided in Appendix B.1 Base Context.", but it's not there. I think it should be, but I think the WG decided not to include it, in which case this section should be consistent.

This is old text and should be aligned w/ what the WG decided to do (point to the context and provide a cryptographic hash of its contents.

This change is editorial.

I don't think "ordered set" is a valid type for a value in either JSON or JSON-LD?

It's not in JSON, though the "ordered set" and "unordered set" language is used in JSON-LD. What we mean here is "use JSON array syntax, but don't presume order matters". We can try to find better language.

This change would be editorial.

"each URL in the @context be one which, if dereferenced, results in a document containing machine-readable information about the @context." <- I think the URL should return information about itself, not the whole @context? Or … does this mean to say each URL in the array? If that's the right interpretation, wouldn't it break the JSON-LD interpretation if these URLs don't return actual @context values?

It means "each URL in the array", and yes, not returning an actual @context value would break the JSON-LD interpretation. Given that the data model for VC v2.0 /is/ JSON-LD, we can probably just remove this statement.

This change would be normative.

The "This specification requires for a @context property to be present;" note is redundant with a normative statement to this effect a paragraph up. Maybe simplify this to just saying that @context is defined by JSON-LD?

Agreed.

This change would be editorial (since it's not changing implementation behaviour).

What does it mean to "express context information"?

It means "MUST be a JSON-LD Context, either by reference (URL points to JSON-LD Context document) or by value (an object that is a JSON-LD Context). We should clarify this language.

This change would be normative (it's really editorial, because implementations wouldn't change, but I can see someone arguing that it's normative).

This says "Verifiable credentials and verifiable presentations have many attributes and values that are identified by URLs", but very little, if any, of the rest of the document identifies the attribues or values by URL.

Yes, this is old language where search/replace has probably messed with it's meaning over the years. We should probably re-write that paragraph to introduce contexts in a different way.

This change would be editorial.

4.3 Identifiers

"The id property MUST NOT have more than one value." What does this mean? In the language of 3.1 Claims, it's probably "There must be at most one claim for the id property about a given subject." In the language of JSON, it's probably that the id property must be a string rather than an array or object.

Yes, your interpretation is correct.

It's probably enough to have 3.1 Claims say what you mean by a property "having" values, and then https://w3c.github.io/vc-data-model/#syntaxes will explain the serialization.

Hrm, are you saying 3.1 already says this... or needs to be modified to say this?

Merge the id

and the "If the id property is present:" block.

Ok, can do.

This change will be editorial.

4.4 Types

"The value of the type property MUST be, or map to (through interpretation of the @context property), one or more URLs." <- That's natural and impossible to avoid in JSON-LD, but in JSON, there's no definition of how to interpret the @context property. What does this rule mean under JSON processing? Alternately, if section 6 is serious about all of this being a description of the data model that's then mapped to a concrete syntax, you can remove the "or map to" bit.

What we're trying to say here is: "You can use a full URL, or you can use a JSON-LD term that maps to a URL." That is, you can't use a number, string, object, etc. I believe the text is correct as is and if we removed "or map to", then the only allowable property becomes a URL, no?

If we made a change here, it would be editorial (since implementations wouldn't have to change).

Several objects must have a type specified, and the table of those objects says "A valid proof type.", etc. What defines the "valid" types here?

The VC extensibility model is decentralized, so a valid type is described by external specifications. We do suggest that people register their specifications here:

https://w3c.github.io/vc-specs-dir/#property-based-extensions

... but that's not required. We could point to the VC Specifications Directory, would that address your concern?

This section has "All credentials, presentations, and encapsulated objects MUST specify, or be associated with, additional more narrow types", but B.3 Differences between Contexts, Types, and CredentialSchemas has "Whilst it is good practice to include one additional value depicting the unique subtype of this verifiable credential, it is permitted to either omit or include additional type values in the array." These should be consistent.

Agreed, we should probably downgrade that "MUST" to a "SHOULD".

This change would be normative.

Is there a normative statement somewhere about how a type determines whether a "concrete expression of the data model [] complies with the normative statements in this specification"? This section says that "This enables implementers to rely on values associated with the type property", but I couldn't find anything actually enabling that.

No such normative statement exists in the specification. It's a feature of any sort of language that contains a type system... if something is of a certain type, that type is associated with a variety of properties. I'm not sure what to change here that would improve the situation, though I doubt that adding more normative statements would help (as they'd be repeating what the JSON-LD spec says anyway).

That said, this section has some fairly old text from the v1.0 days and could benefit from a clean-up and rewording.

This change would be editorial.

4.7 Issuer

"the URL …, if dereferenced, results in a document … that can be used to verify the information expressed in the credential." <- But the privacy properties of VCs require that the verifier not dereference URLs that give the issuer information about the holder/subject. This should say something about how verifiers are supposed to check that they trust the issuer, without fetching URLs. Is it just by URL equality?

Fetching the issuer URL wouldn't provide information about the holder/subject unless the issuer URL is a tracking URL that's unique to a particular holder/subject. The "if dereferenced, results in a document containing machine-readable information" was alluding to controller documents when it was written way back in the VC v1.0 days (before the DID specification was written).

Now that we have normatively established the existence of controller documents, we could refer to them normatively. There are some in the group that might object to us doing that, and the controller document spec that we split out (two weeks ago) is on a different timeline to CR than the data model spec. We could remove the line, or have it refer to the controller document specification.

If we made this change it be (arguably) normative.

4.9 Securing Verifiable Credentials

I don't see anywhere that tells a verifier to check the securing mechanism before trusting the content.

We merged a PR recently that says much more about verification and validation here:

https://github.com/w3c/vc-data-model/pull/1334/files

We are also working on a PR to say more about verification here:

#1338

Could this section say to look up the proof's type in a registry, and use the specification found there to determine whether the credential was provably provided by a trusted issuer?

Yes, it could. This is being worked on in this PR (which you're currently providing feedback on):

#1338

This section should mention that the "proof" property is a graph container … except in https://www.w3.org/TR/vc-data-integrity/#verify-proof, I don't see any expectation that there's a graph on either side of the "proof" property, so I'm not sure it actually should be.

The spec now says that "proof" contains a graph:

https://w3c.github.io/vc-data-model/#securing-verifiable-credentials

Namely:

Each proof is a separate named graph (referred to as a proof graph) containing a single proof.

[msporny]

5.1 Lifecycle Details

The graph mentions a "Registry" that doesn't appear in the rest of the section. I see the "verifiable data registry" mentioned elsewhere in the document, but nothing says how any of the parties are expected to use it. Could it just be removed?

The registry comes from the concept of a DID registry or a status (revocation) registry. I expect there to be push back to removing it, so perhaps we can add text on what it is and how it's expected to be used.

If we do this change, the change would be editorial.

These details include several operations, several of which should probably be defined as algorithms. The most critical for this spec is probably verification, as mentioned above. I suspect issuance and presentation can be defined in the appropriate proof specifications, and revocation can be defined in the appropriate status specification. Transfer and deletion should probably be here, even if they're trivial "transfer by copying the bytes" and "delete by deleting the bytes", to avoid making people worry that something more complicated is subtly needed.

Ok, we can add that text.

This change would be editorial

5.2 Trust Model

I don't think it's true that "The verifier trusts the issuer to issue the credential that it received."? The verifier uses the proof methods to prove that, so it doesn't need to take that on trust.

What that text is trying to say is that the verifier trusts the issuer to make the statements it's making after verification has been performed. We can try to wordsmith this to make it more clear.

This change would be editorial.

I don't think "All entities trust the verifiable data registry" because the registry isn't used in this specification.

The registry is used if status lists or DIDs are used, we should probably elaborate on this. We should add text to clarify this.

This change would be editorial.

I don't think the holder needs to trust the issuer.

When the subject is the holder, they trust the issuer... when the holder isn't the subject, they still trust the issuer to make truthful statements. There might be exceptions to this, such as when the holder is trying to catch a lying issuer, but then, they trust the issuer to make the statements they want to make. How would you like this language changed/modified given the previous sentences?

If we change this, the change would be editorial.

The statement about the verifier trusting the issuer is close, but there's something to say about the verifier choosing the issuers they want to trust.

Yes, agreed, let's add that text.

This change would be editorial.

The holder also trusts the verifier not to report information about the holder back to the issuer.

In most cases yes, in some cases no (such as the issuance of a single-use token that is not meant to be double-spent -- reporting of some kind needs to be performed in that case). The statement above is use-case dependent.

5.3 Extensibility

An issuer might want to require the verifier to process a particular extension point before it trusts the credential. e.g. if the issuer uses revocation to make their threat model work out, they need the verifier to process the credentialStatus extension point. On the other hand, a verifier might be able to safely ignore the referenceNumber extension. Have you considered a way of marking extensions critical?

Yes, we've considered it but use cases haven't surfaced yet where the feature was necessary. IOW, processing is performed by checking the type of credential, and the credential type typically lists processing instructions for all fields in the specification that defines the credential (as well as what's mandatory vs. optional). This is a part of validation, and those rules are typically credential-type-specific.

That is, if a verifier is validating a particular type of credential, the business rules for that verifier are clear based on the type of credential. Generic processing during validation is typically not a thing... a verifier has strict business rules that it applies to a particular credential type where some feature asserting a "mandatory to process extension" has not been necessary to date.

We /could/, for example, introduce a JSON Pointer-based mechanism that specifies which objects in a VC MUST be processed, but as I said before, this has not come up as a strong requirement in the past. We do realize that this is used in systems like x509 certs and might be more necessary there because x509 certs don't include a typing system.

Thoughts?

5.3.1 Semantic Interoperability

"is expected to be published" diffuses responsibility. Use active voice.

Agreed.

This change would be editorial.

5.4 Integrity of Related Resources

"MAY include a property named relatedResource" doesn't say what the subject of that property needs to be.

Hrm, there are normative statements around id in that section. What specifically would you want said about the subject that's not already said in the section? I do agree that this section needs a significant amount of editorial clean up to make it easier to read.

Editorial clean up would be, unsurprisingly, an editorial change.

"MAY contain a property named mediaType that indicates the expected media type for the indicated resource" <- does this mean that when the resource is fetched over HTTP, the fetch should include an Accept: header with that media type? Or does request happen with no Accept: header, and the integrity check fails if the response Content-Type: isn't the expected value?

Yes, excellent point. I would imagine the author of this section (not me) meant that the Accept header would be used for that media type and the returned document would need to have that media type listed in the Content-Type.

This change would be a normative change.

What's a validator supposed to do if the digestSRI doesn't match?

Good question, we could make verification fail if the digest doesn't match... or we could punt that rejection to validation. If we make the check a part of verification, digestSRI (and digestMultibase) could be used as a DoS attack vector (it would force verifiers to download those resources and check them)... but we could make that happen /after/ a base signature check has been performed. So, it's possible to move this process into verification (which would make the process fail early before validation is performed). We could also move the process to validation. Do you have a preference on which one we should do?

This change would be normative.

5.5 Refreshing

This is the first mention of the issuer creating a verifiable presentation. https://w3c.github.io/vc-data-model/#dfn-issuers, for example, only says they create VCs

I'll add some text in https://w3c.github.io/vc-data-model/#presentations to say that issuers and verifiers can use presentations to prove who they are to a holder by providing their own credentials via a presentation. Note that this is only true in some protocols (like the ones VC API uses), but giving an entity the ability to present data is a fairly universal thing.

This change would be editorial.

5.6 Terms of Use

The note here talks about delegating a verifiable credential, but that's the only mention of delegating in the document. What's it mean?

That text has been deleted in the latest version of the specification (it was confusing others as well).

5.8 Zero-Knowledge Proofs

"The verifiable credential MUST contain a Proof" <- Is that true? You can't do external ZKPs?

This text is from the v1.0 and v1.1 days, JWP exists now, so it's no longer true. We should remove the normative statement.

This change would be normative.

"If a credential definition is being used" is the first use of "credential definition". I think this means "credential schema"?

Yes, this is old text from the CL Signature days. CL Signatures still exist and are being ported to use the Data Integrity specification and /do/ use credential definitions (which instruct the verifier how the proof is constructed). So, no, it doesn't mean credential schema, but this text belongs in a securing specification and not this specification. Also note that this entire section around ZKPs needs a big update. We were waiting on vc-di-bbs to move forward, and it's been lagging until recently. We will almost certainly rewrite the entire section to refer to https://w3c.github.io/vc-di-bbs/ and shape the language around the things that are generalizable between CL Signatures and BBS.

"A verifiable presentation MUST NOT leak information" should describe how to accomplish that (or delegate that description and this requirement to the proof specification).

Yeah, we should delegate this to the securing specifications.

This change would be normative.

"The verifiable presentation SHOULD contain a proof property to enable …" seems to say that merely including the proof property accomplishes those things, but I think you mean that the particular proof type should do that?

These normative statements probably just need to go now that we've externalized the selective disclosure and unlinkability features outside of the specification.

This change would be normative.

"The purpose of this section is to guide implementers" <- maybe the purpose should be to guide future specification authors? Those might overlap with implementers, but the requirements in this section seem like requirements on a new design rather than requirements on implementations of an existing design.

I couldn't find this text in the latest specification. Perhaps it was removed between the time you reviewed the specification and when we processed your comments?

If a variation of this language still exists in the specification, we could probably remove it or reword it to be more acceptable.

This change would be editorial.

5.10 Reserved Extension Points seems like a good mechanism to handle these future use cases.

Glad you agree. :)

5.11 Ecosystem Compatibility

"according to the rules provided in this section" made me think this section was going to provide an algorithm to transform documents. Maybe you mean to say that if there's a specification that defines how to transform another format into a VC, then the other format is compatible?

Yes, that's what was meant here. I'll try to update the text such that the intent here is more clear.

This change would be editorial.

[msporny]

6.1.1 Syntactic Sugar

Link "graph containers" to its definition in JSON-LD.

This has been done.

6.3 Proof Formats

"The sections detailing" should probably be "the specifications detailing".

Good catch, will fix.

This change would be editorial.

7.2 Personally Identifiable Information

"Implementers are strongly advised to warn holders" should probably say "credential repositories SHOULD". If it's also other kinds of implementations, list them too. That'll imply that this section should be normative, but privacy considerations are often normative.

The entire Privacy and Security considerations section is non-normative, so we don't put normative language in it; we did intend to say "strongly advised". We don't want implementers to have to go hunting through the privacy and security considerations for normative implementation statements and given that this is a data model specification, there is much about how the ecosystem should be implemented that we can't say normatively here. That said, "implementers" is probably wrong... it should be something like "Implementers of software utilized by holders".

This change would be editorial.

"implementers are strongly advised to … protect the data" <- I think this is credential repositories too?

Yeah, it's really all software that stores and transports VCs (all software used by issuers, verifiers, and holders). We can make that adjustment.

Do you want to say that specifications for fields that hold PII should themselves identify the field as worth warning users about?

Yes, we've discussed this over the years. As you say, it's worth saying something more.

The problem here is that it might be easier to specify the credential types that /don't/ hold PII given how expansive the definition of PII has become (in many privacy regulatory frameworks it's /any/ information that can be mapped back to an individual, including information that is normally not PII but can be combined with other non-PII to uniquely identify an individual, such as home zip code + last 4 digits of a credit card number in an area with a population of less than 10,000 people). That sort of thing makes just about every field PII, including a digital signature. So, perhaps we should say that people should assume a VC will leak PII unless the credential and securing mechanism is designed specifically to avoid correlation (and there are credential types designed specifically for this purpose).

I'll try to add some language to that effect.

This change would be editorial.

7.3 Identifier-Based Correlation

Something needs to identify the proper schemes for this. Will that be in the Proof specifications? If so, this section should probably instruct those specs to include it.

Yes, that would largely fall onto the proof specifications. Now that we have some specifications to point to, we should probably do that in this section.

This change would be editorial.

I suspect the holder also doesn't have full control over which credential schemes they use: that's up to the issuer, right?

Correct.

The holder can only get influence if the credential repository tells them that they're about to share a correlatable Presentation. And the warning for that would ideally go in an algorithm that says how to create a VP.

Yes, for example, we could add that sort of warning here:

https://w3c.github.io/vc-di-bbs/#add-derived-proof-bbs-2023

... though it's debatable that doing so is too low level.

We should add guidance to securing specification authors and implementers here.

This change would be editorial.

7.4 Signature-Based Correlation

Same here: say who's responsible for this.

Agreed, in fact, this subsection needs a refresher, it's from the v1.0 days.

This change would be editorial.

7.5 Long-Lived Identifier-Based Correlation

"Organizations providing software to holders" are a new conformance class for this document. ;) I think you can just say the credential repositories SHOULD.

Agreed (modulo the normative text). :)

We should probably fold this section into the "Identifier-based Correlation" section and provide a refresh on the text.

This change would be editorial.

7.7 Favor Abstract Claims

This is advice for issuers, right? Then it should say "issuers should favor abstract claims".

Yes, agreed. I'll make that change.

This change would be editorial.

7.8 The Principle of Data Minimization

This has good advice for issuers and verifiers. You could also take this opportunity to remind credential repositories to disclose what fields the verifier is asking for, so that users can push back.

Yes, good point. I'll make that change.

This change would be editorial.

7.9 Bearer Credentials

"Holders should be warned by their software" -> "Credential repositories should warn holders"

I'll make that change.

This change would be editorial.

7.10 Validity Checks

You should try to move this section's content into the algorithms that would otherwise expose extra data to issuers.

Hrm, which section are you suggesting it be moved into? "Appendix A: Validation"?

If we make this change, it would be editorial.

Is there any way to improve the credentialStatus mechanism to prevent issuers from using per-credential revocation lists? e.g. require the issuer to send their revocation-checking URL to the verifier out of band along with their public keys? This section could state that sort of requirement on the credentialStatus specifications.

I believe the approach you're suggesting would definitely result in a phone-home/tracking issue because the verifier would need to identify which list they're interested in, and in doing so, they'd either uniquely identify the credential, or use the "tracking URL" provided by the issuer in the first place. I'm probably misunderstanding your proposal.

We could put a requirement in the credentialStatus extension point that all specifications MUST NOT enable individual tracking and conforming issuers MUST NOT generate credential status URLs that individually track people (though, testing that is not going to be possible in the spec test suite).

This change would be normative.

7.11 Storage Providers and Data Mining

A statement in a specification isn't really enough to warn holders. ;)

It is if we include the specification as a part of the click-through EULA for all credential repository software! :P

Is there any way that civil society groups or regulators could detect that a credential repository is abusing its trust?

If the credential repository is careful about how they do their data brokering, it would be difficult, I imagine. While the comment before this one suggested using a EULA, perhaps that is the best we have right now. Either the credential repository EULA has a "no data re-selling / data brokering" clause in it or the software encrypts all content before it's sent up to a credential repository storage system (and decrypts it locally on the client, using client-only keys/processing). Vetting of open source systems is also a mechanism that could be used. There are mechanisms that could be used to increase trust, but you're ultimately beholden to the people writing the software or running the systems to be honest about what they're doing.

It's worth saying that civil society groups and regulators should play a part in this ecosystem. I can add a bullet point to that effect in that section.

This change would be editorial.

7.13 Usage Patterns

Try to localize the mitigations to particular actors. For example, "Designing revocation APIs" should be "Authors of specifications for credentialStatus types", and "Using a globally-unique identifier" could be guidance for credential repositories to warn uses when they're re-using an ID.

Good suggestion, I'll implement this.

This change would be editorial.

8.2 Content Integrity Protection

I think the ?hl= parameter in example 37 is banned by https://w3c.github.io/vc-data-model/#contexts.

Yes, you're correct. That's old v1.0 text, before relatedResource existed. We should just speak to the new feature in this section. I'll update the text to do so.

This change would be editorial.

A.1 Credential Type

"A verifiable credential of a specific type is expected to contain specific properties" <- How is one supposed to figure out which properties are expected for a given type? Is there a registry of types? Do the documents at the type's URL describe the properties in a particular way? You should specify that.

The documents at the type's URL are expected to describe the available properties and their usage. Agreed that it should be specified. I'll do that.

This change would be editorial.

[msporny]

Ok, @jyasskin, I have performed a complete triage of your review. Thank you again for spending the time to do the review.

The next steps, as I see them, are:

• Create issues for all of the changes you're requesting that would be normative. We would need to get these in before going to CR. I will probably bunch them into a handful of issues (potentially one per major section in the specification). I'll use a checklist in each issue to provide where we are in getting all of the items addressed.
• Create issues for all of the non-normative changes. We will probably address these after going into CR. I'll group these changes by major section in the specification. I'll use a checklist in each issue to provide where we are in getting all of the items addressed.

I'll refer to this issue from those subissues (they will show up below). This issue will be closed once I'm confident that we have all of the subissues tracked.