-
Notifications
You must be signed in to change notification settings - Fork 106
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
add context integrity capabilities to the core data model #1140
Conversation
Co-authored-by: Orie Steele <[email protected]>
Is there an issue which led to this PR? |
In general, the issue of context files changing causing signature failures. This mechanism provides assurance around the content of the context at issuance time, not just for data integrity, but for any security mechanism that secures vc+ld+json. |
thanks @OR13 Co-authored-by: Orie Steele <[email protected]>
this is closely tied to this issue and note on 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.
I think I would be supportive of a generalized version of this PR. That said, what @OR13 said in w3c/vc-jose-cose#90 (comment) resonates:
@OR13 wrote:
This should be handled like all the other "core data model extensions", in the core spec.
And I would object there a well, unless a similar bar to what has been met for renderMethod is achieved.
IOW, why is this specific for JSON-LD Contexts -- why aren't we also supporting referencing external images, PDF files, .json documents and the like?
I'll note that this PR is necessary because of the VC-JWT specification, and isn't required by the Data Integrity specification. The PR did start in the VC-JWT spec, was rejected, and moved here for consideration.
Some concrete proposals that would lead to a +1 from me:
Option 1: Solve the "referencing external files with cryptographic integrity" issue in a uniform manner. The issue for that is #831, which the VCWG closed. This PR, more or less, re-opens that issue. If we address the issue holistically, we could mark the feature as at risk (will be removed if there are not enough implementations) and include it in the spec now (I'd be a +1 for that).
Option 2: Use the same process for extensions that we used for renderMethod
-- define it externally, external context, reserved property in spec, integrate into main spec if we get multiple implementations by the end of CR.
excellent feedback @msporny - please see 94b5aa1 - this is a first pass on ensuring that this can work for any remote resource including contexts I think there is some value on the data integrity side as well - just thinking of our implementation, we maintain a local cache of trusted contexts for operations, if the hash is present in a VC we are requested to verify but that hash does not match our trusted and reviewed copy this acts as an early signal that something may be off. |
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.
Please indicate that the hash
value is base64url encoded and then I will approve.
Co-authored-by: Ted Thibodeau Jr <[email protected]>
working these adjustments in now - they should be up shortly |
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.
Overall, LGTM. Please add two issue markers that notes items still under discussion. The other change request is to align w/ the new text.
Co-authored-by: Manu Sporny <[email protected]>
Co-authored-by: Manu Sporny <[email protected]>
Co-authored-by: Manu Sporny <[email protected]>
thanks! should be incorporated |
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.
minor nits
@TallTed @dmitrizagidulin @dlongley please re-review |
Co-authored-by: David I. Lehn <[email protected]>
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.
👍 from me. Thank you so much for your hard work here, @mprorock!
no - thank you for all the thought, review and feedback! |
@OR13 — |
Co-authored-by: David I. Lehn <[email protected]>
@mprorock commit history is verbose/messy (multiple "Apply suggestions from code review" and "Update index.html") on this one because of all of the back and forth. Do you want to clean it up so I can rebase and merge, or do you want me to just squash and merge? I don't want to just squash and merge because it might lose some history you want to keep. Up to you, let me know what you want to do. |
no preference from my side - i am ok with a squash @msporny |
Normative, multiple reviews, changes requested and made (issue markers raised for unresolved concerns), no objections, merging. |
The issue was discussed in a meeting on 2023-06-27
View the transcript1. add context integrity capabilities to the core data model (pr vc-data-model#1140)See github pull request vc-data-model#1140. Kristina Yasuda: goal of the call today is to seek approval for the PR.
Brent Zundel: this tend to work best if PRs are always associated with issues. This PR came from an issue from another repo. Strongly encourage folks to raise and issue. Kristina Yasuda: 164 PR comments is not ok. We are close to merging so we will move forward, but going forward be careful [use issues]. Phillip Long: issue 831 may be considered the antecedent to this issue This one was closed by the WG and this PR opens it. Should we reconsider. Michael Prorock: Opened on VC JWT originally for a good reason. There is a JSON LD data model that implies use of a context. If you are using the context to transform data its important that the context.
Michael Prorock: This PR was open specifically to deal with a way to encode a hash for the context so you know on both sides (issuer and verifier) that its the same.
Michael Prorock: it expanded out to become a little broader but its not the intention to expand to a PR like 831.
Michael Prorock: from a capability standpoint I welcome feedback but I i think everyone thinks this is a good capability. Manu Sporny: speak in favor of the PR. A number of significant changes from last week. This can be used as a general mechanism to refer to a URL within a verifiable credential.
Manu Sporny: which allows for other digest mechanisms from other communities. A number of issue markers have been added. Dmitri Zagidulin: Thanks Mike for the hard work on this. Really helpful and necessary +1. In my ideal world I would love to see an optional canonicalization mechanism.
David Lehn: Has anyone implemented this yet? Is that part of the PR or coming in the future?
Kristina Yasuda: has anyone else implemented?
Michael Prorock: as far as implementation David, we have a test implementation in Go and Python. Python will be released as open source. Go implementation will push to aries framweork Go.
Michael Prorock: One comment back to dmitriz. The has method was defined originally as a separate property. I went with this approach after feedback that a separate term could cause graph problems.
Manu Sporny: 2 things. other issues marker is a statement that says that if related resources exists, you must include all context in the document in the related resource array.
Manu Sporny: Digital bazaar would reject this language as is, but expect further conversation on the issue marker.
Manu Sporny: and then if you have separate hashes you would have to match each hash with the value. So just using SRI or mutibase/multihash because when you read it.
Manu Sporny: you know what resource you are dealing with.
Kristina Yasuda: There is still language to be fleshes out. are we merging it?
Michael Prorock: manu made a comment about multiple hashes. By pointing to SRI we permit that. Specifically for constrained environments.
Manu Sporny: Can I merge after the call? Kristina Yasuda: Thinking about moving to the next agenda. Does the group want to keep discussing the details?
Kristina Yasuda: manu, please merge after the call. |
per conversation and feedback on this pr on vc-jwt this is probably better suited to be available in the core data model for all securing formats
Preview | Diff