-
Notifications
You must be signed in to change notification settings - Fork 112
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 embedded securing mechanism specification requirements #1403
Conversation
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
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.
(To avoid confusion: I have made an earlier comment that has proven to be irrelevant, because I missed the proposed changes in §5.13. Apologies for this. The only remark that is relevant is repeated here, the original comment has been hidden...)
No changes have been made on the section on Presentations (§4.11 in the new setting). I believe that the formal specification of proof, listed alongside other properties, must be removed.
§5.13 (in the new setting) does say that an embedded proof must cover a VC as well as a VP, but it may be helpful to say that explicitly in either §4.11 or (maybe better) in the general section on securing mechanisms. Actually, I wonder whether this statement should not be made in general, i.e., including the enveloped proofs; is it so obvious for them?
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 am o.k. with this PR as it is now; from my point of view it can be merged.
BUT: we must look at the DI spec to correctly "bind" that spec with the requirements described in §5.13. Indeed, I believe it is not specified in the DI spec that:
The property MUST relate the verifiable credential or verifiable presentation to a separable and securable proof graph.
In fact, the term "proof graph" is not present in the DI spec at all (actually, the term "graph" is not used, I believe).
I believe the simplest is to add something to the DI spec whereby the DI spec defines a mechanism for embedded proofs for VC and VP, and it behaves as defined in §5.13. Or something like that.
At this point in time we should probably raise an issue in the DI repository to remember doing that.
@iherman wrote:
I believe we're already tracking this in w3c/vc-data-integrity#190. I've cross-linked to @iherman's comment to make it abundantly clear what text is desired. |
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 only skimmed this, but it looks reasonable, and like it makes things shorter (🎉).
@@ -2077,6 +2038,46 @@ <h3>Proofs</h3> | |||
} | |||
</pre> | |||
|
|||
<pre class="example nohighlight" | |||
title="A verifiable credential utilizing an enveloping proof"> | |||
eyJhbGciOiJFUzM4NCIsImtpZCI6IkdOV2FBTDJQVlVVMkpJVDg5bTZxMGM3U3ZjNDBTLWJ2UjFTT0 |
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.
It'd probably be useful to say what format the enveloping proof is in (https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-07.html ?), or to ensure that people reading the spec can easily see the base-64 decoding of at least the header block.
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.
Fixed in 9a08756.
The issue was discussed in a meeting on 2024-01-03
View the transcript1.3. Add embedded securing mechanism specification requirements (pr vc-data-model#1403)See github pull request vc-data-model#1403. Brent Zundel: next 1403: add embedded security mechanism specification requirements. believe changes have been addressed. will be merged soon. |
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.
This is an improvement.
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
9a08756
to
4fdf91f
Compare
Normative, multiple reviews, changes requested and made, no objections, merging. |
This PR is an attempt at addressing issue #1400 and #1377 by adding embedded securing mechanism specification requirements as requested by @iherman and removing the proof section, as requested by @selfissued.
Preview | Diff