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

add context integrity capabilities to the core data model #1140

Merged
merged 32 commits into from
Jun 27, 2023

Conversation

mprorock
Copy link
Contributor

@mprorock mprorock commented Jun 2, 2023

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

index.html Outdated Show resolved Hide resolved
Co-authored-by: Orie Steele <[email protected]>
index.html Outdated Show resolved Hide resolved
@paulfdietrich
Copy link

Is there an issue which led to this PR?

@OR13
Copy link
Contributor

OR13 commented Jun 3, 2023

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.

index.html Outdated Show resolved Hide resolved
thanks @OR13

Co-authored-by: Orie Steele <[email protected]>
@mprorock
Copy link
Contributor Author

mprorock commented Jun 3, 2023

Is there an issue which led to this PR?

this is closely tied to this issue and note on JSON-LD

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.

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.

@mprorock
Copy link
Contributor Author

mprorock commented Jun 5, 2023

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.

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

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

Please indicate that the hash value is base64url encoded and then I will approve.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <[email protected]>
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@mprorock
Copy link
Contributor Author

  • relatedResource

working these adjustments in now - they should be up shortly

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.

Overall, LGTM. Please add two issue markers that notes items still under discussion. The other change request is to align w/ the new text.

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@mprorock
Copy link
Contributor Author

Overall, LGTM. Please add two issue markers that notes items still under discussion. The other change request is to align w/ the new text.

thanks! should be incorporated

Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

minor nits

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@brentzundel
Copy link
Member

@TallTed @dmitrizagidulin @dlongley please re-review

index.html Outdated Show resolved Hide resolved
Co-authored-by: David I. Lehn <[email protected]>
Copy link
Contributor

@dmitrizagidulin dmitrizagidulin left a 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!

@mprorock
Copy link
Contributor Author

👍 from me. Thank you so much for your hard work here, @mprorock!

no - thank you for all the thought, review and feedback!

@OR13
Copy link
Contributor

OR13 commented Jun 27, 2023

This PR will ensure that verifiers produce the same graph, for the same credential, here is an example of a graph that could be different, based on different verifiers using different context values (which controls how JSON-LD terms are expanded to RDF graphs).

Here is a picture to show the kinds of URLs that will be interoperable identifiers for the claims, after this PR is merged.
244960071-2c568d13-f878-4a4d-a228-5cd6eb91969e-1

@TallTed
Copy link
Member

TallTed commented Jun 27, 2023

@OR13credential, here in your #1140 (comment) should be credential. Here. The here is an example starts a new thought, which should be a new sentence, not a continuation of the first.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Co-authored-by: David I. Lehn <[email protected]>
@msporny
Copy link
Member

msporny commented Jun 27, 2023

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

@mprorock
Copy link
Contributor Author

@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

@msporny
Copy link
Member

msporny commented Jun 27, 2023

Normative, multiple reviews, changes requested and made (issue markers raised for unresolved concerns), no objections, merging.

@msporny msporny merged commit 2f8eabc into w3c:main Jun 27, 2023
@iherman
Copy link
Member

iherman commented Jun 28, 2023

The issue was discussed in a meeting on 2023-06-27

  • no resolutions were taken
View the transcript

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

Kristina Yasuda: 164 PR comments is telling.

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.
… even if the PR is simple.

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.
… is the same on both side. Other conversation going on around context and things like MIM and other modifications that can lead to a bad processor.

Orie Steele: Context integrity mostly matters for the RDF representation, this mostly impacting data integrity proofs at the sign and verify interface, see #1151 (comment).

Orie Steele: it also applies to JSON only representations, if you care to get the same graph out on different verifiers.

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.
… The implication is that this PR was not intended to be a reopening of 831. There is a relation to things described in 831. When this became PR 1140,.

Kristina Yasuda: jfyi: #831.

Michael Prorock: it expanded out to become a little broader but its not the intention to expand to a PR like 831.

Manu Sporny: The suggestion that context integrity mostly matters for RDF is just not true, but we don't have to go there to process this PR.

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.
… its really useful to link to external resources like pdf, images, audio etc. It also now matches the naming of the hash expression is the digest SRI.

Dave Longley: +1 to manu and comments on different digest formats and features in the community.

Manu Sporny: which allows for other digest mechanisms from other communities. A number of issue markers have been added.
… an issue marker to make it clear that its an ongoing discussion on whether this can be used throughout the group.

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.

Orie Steele: -1 to JCS, +1 to hash agility based on an existing hash algorithm registry.

David Lehn: Has anyone implemented this yet? Is that part of the PR or coming in the future?

Dmitri Zagidulin: @Orie - -1 to /any/ canonicalization, or just JCS?

Kristina Yasuda: has anyone else implemented?

Orie Steele: I have proved that you can break things, when you don't do this... does that count?
Dmitri Zagidulin: @Orie - which this?

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.

Orie Steele: In general, using different contexts breaks data integrity proofs in confusing ways for most developers.
Orie Steele: (because you get different n-quads for the verifier, than what the issuer intended to be produced).

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.

Dmitri Zagidulin: @Mike - ohhh I see, makes sense.

Orie Steele: dmitriz see this issue for an example: #1151 (comment).

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.

Orie Steele: +1 to that language not being clear enough.

Michael Prorock: +1 needs improvement.

Manu Sporny: Digital bazaar would reject this language as is, but expect further conversation on the issue marker.
… The topic mike just mentioned "why specific algorithm and value in the same string'. If you break it out, you could have multiple hashes listed.

Orie Steele: is this a feature or a bug?

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.

Dave Longley: each hash algorithm becomes a "property and value" of the resource, which is bizarre -- it also makes schema processing more complicated vs. layering separation.
Dave Longley: if it is broken out as a separate property.

Manu Sporny: you know what resource you are dealing with.

Orie Steele: I think its pretty common to provide multiple hashes for resources, see https://github.com/microsoft/sbom-tool/blob/main/samples/manifest.spdx.json.

Dave Longley: it's absolutely a feature to have consistent data modeling vs. free for all (something the VCDM introduces over "anything goes JSON").

Kristina Yasuda: There is still language to be fleshes out. are we merging it?

Orie Steele: Seems the design comments here, don't match software supply chain security work I an aware of.

Michael Prorock: manu made a comment about multiple hashes. By pointing to SRI we permit that. Specifically for constrained environments.
… There are cases like ARMV7 or v8 where you might only have SHA256 available. There are reason to permit multiple has values. We would up in a better place with this PR.

Orie Steele: +1 mprorock, providing multiple hashes supports constrained environments, that don't ship a kitchen sync of hash algorithms.

Dmitri Zagidulin: +1.

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?

Manu Sporny: I will merge this PR after the call.

Kristina Yasuda: manu, please merge after the call.
… Any objects to move to 1101 and 1100? It was not on the agenda.

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.