Require digest verification for related resources. #1567
Conversation
msporny
requested review from
BigBlueHat,
davidlehn,
dlongley,
gobengo,
jandrieu,
iherman,
dmitrizagidulin,
decentralgabe,
selfissued,
KDean-Dolphin,
brentzundel and
PatStLouis
msporny
added
normative
|
@msporny I believe there is a github manipulation error. The 'diff' file shows over 1000 changes, mostly editorials, which makes it difficult to review. I presume the subject of this PR is only §5.3 on the Integrity of Related Resources... |
This is what I see when I view the diff on Github:
There was an issue initially, which created the issue you mention, but I fixed that last night. I expect you probably hit a cache somewhere that gave you the old version. Try a hard refresh? |
|
It may be there is a cache issue, but not mine... I have made a clean reload, and I have also looked at the diff file from browsers that I have never used for this purpose. I still see changes in the abstract or the introduction, for example: 
Anyway. I can concentrate on the part that you highlight, maybe this exchange is good enough for other reviewers to disregard the rest... |
|
B.t.w., if I look at the "Files changed" tab, then I get what you really changed only. Something with the preview cache... |
index.html
Outdated
| the `id` of an object inside a [=conforming document=] with a corresponding | ||
| cryptographic digest MUST check the digest against the retrieved resource. If |
There was a problem hiding this comment.
Just being picky...
| the `id` of an object inside a [=conforming document=] with a corresponding | |
| cryptographic digest MUST check the digest against the retrieved resource. If | |
| the `id` of an object inside a [=conforming document=] with a corresponding | |
| cryptographic digest appearing in a `relatedResource` object value MUST check the digest against the retrieved resource. If |
It is pickiness. I will a accept either way.
There was a problem hiding this comment.
Hmm, this is in the related resource section. The id is in the relatedResource object, that's the object that is being referred to here, i.e., relatedResource.id, so I don't think your suggestion is correct and / or it is redundant in a strange way. IMO, if we need to, we can say "the id of a relatedResource object" instead of "the id of an object".
Note the sentence right before the above paragraph says:
Any objects for which selective disclosure or unlinkable disclosure is desired SHOULD NOT be included as an object in the
relatedResourcearray.
This "an object" ^ is the same one referred to in the paragraph above.
index.html
Outdated
| the `id` of an object inside a [=conforming document=] with a corresponding | ||
| cryptographic digest MUST check the digest against the retrieved resource. If |
There was a problem hiding this comment.
Hmm, this is in the related resource section. The id is in the relatedResource object, that's the object that is being referred to here, i.e., relatedResource.id, so I don't think your suggestion is correct and / or it is redundant in a strange way. IMO, if we need to, we can say "the id of a relatedResource object" instead of "the id of an object".
Note the sentence right before the above paragraph says:
Any objects for which selective disclosure or unlinkable disclosure is desired SHOULD NOT be included as an object in the
relatedResourcearray.
This "an object" ^ is the same one referred to in the paragraph above.
index.html
Outdated
| specification MUST produce a validation error unless the resource matches the | ||
| expected media type and cryptographic digest. | ||
| A [=conforming verifier implementation=] that makes use of a resource based on | ||
| the `id` of an object inside a [=conforming document=] with a corresponding |
There was a problem hiding this comment.
If necessary for clarity, here's an alternative suggestion to what Ivan suggests below:
| the `id` of an object inside a [=conforming document=] with a corresponding | |
| the `id` of a `relatedResource` object inside a [=conforming document=] with a corresponding |
There was a problem hiding this comment.
Yep, that works and is mathematically more precise.
👍
|
The issue was discussed in a meeting on 2024-10-09
View the transcript2.1. Require digest verification for related resources. (pr vc-data-model#1567)See github pull request vc-data-model#1567. Manu Sporny: I took an action to raise a PR in VCDM 2.0 for requiring digest verification if it's provided, there has been some discussion on it, I wanted the group to see that this is out there, there are some suggested changes, I will process this and merge it by the end of the week, please get in there and provide commentary, hopefully the PR reflects consensus in the group. I plan to close the DI issue based on the merge of requiring digest verification if it's provideds provided. |
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
|
Normative, multiple reviews, changes requested and made, no objections, merging. |
This PR is an attempt to address w3c/vc-data-integrity#272 by requiring digest verification for related resources.
Preview | Diff