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 support for Enveloped Verifiable Credentials (2nd attempt) #1379

Merged
merged 6 commits into from
Dec 15, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented Dec 9, 2023

This PR attempts to address issue #1352 by adding a new feature for embedding "enveloped" Verifiable Credentials in Verifiable Presentations. This PR integrates @iherman's PR #1372.


Preview | Diff

Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

None of my comments are show-stoppers.

index.html Show resolved Hide resolved
Comment on lines +2343 to +2344
The `@context` value of the object MUST follow all of the "MUST" statements in
Section <a href="#contexts"></a>. The `id` value of the object MUST be a `data:`
Copy link
Member

@iherman iherman Dec 10, 2023

Choose a reason for hiding this comment

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

Suggested change
The `@context` value of the object MUST follow all of the "MUST" statements in
Section <a href="#contexts"></a>. The `id` value of the object MUST be a `data:`
The possible `@context` value of the object MUST follow all of the "MUST" statements in
Section <a href="#contexts"></a>. The `id` value of the object MUST be a `data:`

Is it really necessary to include a @context at this point? The only reason to have it is to use the id and type aliases, but that would be inherited from the contexts above, wouldn't it?

Copy link
Member Author

@msporny msporny Dec 10, 2023

Choose a reason for hiding this comment

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

Yes, it's necessary, because (in the v2 context) we clear all context values inside the Verifiable Credential Graph. We have to do this in order to ensure that VCDM v1.1 VCs can be put inside VCDMV v2.0 VPs. If we don't do that, JSON-LD protected mode kicks in and alerts us that we're re-defining some terms (which is the correct behaviour).

Copy link
Member

@iherman iherman Dec 10, 2023

Choose a reason for hiding this comment

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

Ah. Right, my bad.

(For other reviewers, in case they do not follow: there is a "@context": null statement within the definition of the VerifiablePresentation.verifiableCredential property in the V2 @context file. The reason is what @msporny describes above.)

Let me picky, though. The text says:

The @context value of the object MUST follow all of the "MUST" statements in Section 4.2 Contexts.

However, §4.2 only says:

Verifiable credentials and verifiable presentations MUST include a @context property.

Technically, i.e., in the vocabulary, an EnvelopedVerifiableCredential is not a VerifiableCredential (they are different classes), meaning that the statement in §4.2 does not apply, i.e., it is not said that a @context file must be present. You can either change the text in §4.2, or add a statement in the new section whereby a @context property MUST be added.

Also, continuing to be picky, and looking at §5.12, the text says:

For presentations, each value associated with the verifiableCredential property of the presentation is a separate named graph of type VerifiableCredentialGraph which contains a single verifiable credential.

(Emphasis is mine.) I believe it should say something like

which contains a single verifiable credential or and enveloped verifiable credential.

(Or something for that effect.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I believe your statements above are correct, I'll make those modifications in the next iteration of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the second context is required here... if the v2 context defines things correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13,

As explained in the comments above, the second context is indeed necessary as the context is intentionally cleared to support other contexts (such as 1.1). So a context declaration is required.

Copy link
Member

Choose a reason for hiding this comment

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

@OR13

We can put it another way: if the second context is not used, then the right terms for that object must be @type and @id instead of type and id, respectively (this is how the current, v2 version of @context works, and I do not see any way to avoid that.)

It is indeed unfortunate that the usage of those aliases force us to use the second context here. But that is water under the bridge, we decided not to use the original, @-prefixed values.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iherman, I applied your requests in 2ffb884.

index.html Outdated Show resolved Hide resolved
The `@context` value of the object MUST follow all of the "MUST" statements in
Section <a href="#contexts"></a>. The `id` value of the object MUST be a `data:`
URL that expresses a secured <a>verifiable credential</a> using an enveloping
security scheme, such as [[[VC-JOSE-COSE]]] [[VC-JOSE-COSE]]. The `type` value
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there 2 here intentionally?

Copy link
Member

Choose a reason for hiding this comment

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

@OR13, this is a respec trick.

  • [[[VC-JOSE-COSE]]] is converted into the official title of the spec
  • [[VC-JOSE-COSE]] is converted into a reference to the References' section.

So the short answer to your question is: yes.

<span class="highlight">"verifiableCredential": [{
"@context": "https://www.w3.org/ns/credentials/v2",
"id": "data:application/vc+ld+json+sd-jwt;base64,QzVjV...RMjUK==",
"type": "EnvelopedVerifiableCredential"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting... I think with some discussion this could be acceptable.

index.html Outdated Show resolved Hide resolved
@brianorwhatever
Copy link

This PR is great and does a good job in finally disambiguating things that are quite clearly different.

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

I'm supportive of this compromise. Thank you.

@OR13
Copy link
Contributor

OR13 commented Dec 11, 2023

Does this mean that you can't use SD-JWT to create v1 presentations or credentials?

Or that the v2 context is required to make v1 presentations?

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

+1 to @iherman's comments.

index.html Outdated Show resolved Hide resolved
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.

👍

@iherman
Copy link
Member

iherman commented Dec 13, 2023

The issue was discussed in a meeting on 2023-12-13

  • no resolutions were taken
View the transcript

2.12. Clarify section on verifiable credential graph (issue vc-data-model#1365)

See github issue vc-data-model#1365.

Brent Zundel: clarify section of verifiable credential graph.
… what needs to change to address this?

Manu Sporny: there is a clear concern, which i think I can address.
… for the avoidance of doubt, the following values are not allowed in a verifiableCredential.
… I list raw text strings, and URLs.
… it has to be an object of some kind that creates that graph structure.
… and I won't make it exhaustive... its strange to point out, what you cannot put in the field.

Dave Longley: "MUST be an object and can't be something else, such as...".

Andres Uribe: works for me, thanks manu!

Brent Zundel: do you want to mix that into 1379?

Manu Sporny: seems to make sense to put it in 1379.

See github pull request vc-data-model#1379.

Brent Zundel: then it will address that issue as well.
… not hearing anyone opposed to this path.

@iherman
Copy link
Member

iherman commented Dec 13, 2023

The issue was discussed in a meeting on 2023-12-13

  • no resolutions were taken
View the transcript

2.7. Add mechanism to embed externally secured VCs in a VP (issue vc-data-model#1352)

See github issue vc-data-model#1352.

Brent Zundel: add mechanism to secure VPs.
… first PR failed, second PR was raised, can we get an update.

See github pull request vc-data-model#1379.

Manu Sporny: seems it will land.
… we seem to have a solution for securing verifiable presentations without using proof.

@msporny
Copy link
Member Author

msporny commented Dec 15, 2023

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 8f26399 into main Dec 15, 2023
2 checks passed
@msporny msporny deleted the msporny-evc branch December 15, 2023 17:25
@iherman
Copy link
Member

iherman commented Dec 16, 2023

@msporny the PR has been merged, but I do not see any additions reflecting on what I wrote in #1379 (comment). A quick search on the term EnvelopedVerifiableCredential reveals that the only place the term appears is in §4.11.1 Enveloped Verifiable Credentials.

@msporny
Copy link
Member Author

msporny commented Dec 16, 2023

@msporny the PR has been merged, but I do not see any additions reflecting on what I wrote in #1379 (comment).

Woops, apologies. Fixed in 2ffb884.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants