-
Notifications
You must be signed in to change notification settings - Fork 103
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 renderMethod
as reserved extension point.
#1108
Conversation
cd96431
to
330df11
Compare
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.
Can we split this PR up, so we can address each of these independently?
I am supportive of reserving confirmationMethod
, and but I object to renderMethod
, presentationSchema
.
The reason for my objection is that both of these extensions when leveraged by an issuer, impact complexity on the holder / verifier, which I believe might lead to fragmentation and harm adoption.
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.
lgtm
We can just refactor this PR after we have a discussion in it. The changes are small enough that a refactor after we get consensus on the properties will be easier than having a parallel discussion on each one.
Hmm, that doesn't make sense to me, perhaps there is a miscommunication of some kind. Could you explain this a bit more? The impact that Also, note the issue marker here: https://github.com/w3c/vc-data-model/pull/1108/files#diff-0eb547304658805aad788d320f10bf1f292797b5e6d745a3bf617584da017051R3200-R3206 Which states that all three are at risk, and will be removed by the end of the Candidate Recommendation phase, unless a specification and two implementations exist (no requirement for a test suite, no requirement for interoperability on each feature, but at least a demonstration that people are implementing against the extension point). |
index.html
Outdated
<tr> | ||
<td>`termsOfUse`</td> | ||
<td> | ||
An example entry for a reserved property. |
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.
Should this be here?
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.
No, it shouldn't... bad merge. Fixed in cabcea1. Thank you!
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.
these seem useful to define now to enable adoption. I dont know of any other vocabulary that these short names would collide with.
index.html
Outdated
<p class="issue" title="(AT RISK) Reserved properties"> | ||
The following reserved properties are at risk and will be removed from the | ||
specification if at least one specification with two independent implementations | ||
are not demonstrated by the end of the Candidate Recommendation Phase: | ||
`confirmationMethod` / `confidenceMethod`, `presentationSchema`, and | ||
`renderMethod`. | ||
</p> |
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 isn't clear to me if this means only from the pertinent sections of the spec, or from this table as well.
Would it be better to put this text before each at risk property?
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 isn't clear to me if this means only from the pertinent sections of the spec, or from this table as well.
Yeah, good point, I wasn't thinking about the existing sections of the spec.
We should probably just add at-risk markers to evidence
, refreshService
, and termsOfUse
to be crystal clear wrt. their current state (at risk) as we enter CR. I'll do that in a separate PR.
Would it be better to put this text before each at risk property?
Yes, I can do that. It felt like it would be repetitive to copy/paste the same text to each entry. In the grand scheme of things, it doesn't really matter because the entry and/or text would be removed as we move into PR. I'll do the revision in an update to this PR.
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 a86f020.
Can we please discuss issue #1102 along with this issue please. Thanks. |
@OR13 I don't now if your review is still blocking this PR, or if we achieved consensus to merge this PR during one of the calls this week (that I missed -- the minutes are not clear on the status of this PR). If @OR13 is good with this PR, we can merge. If not, we're going to have to shift it into DO NOT MERGE / discuss. |
c256ebc
to
44f0e7d
Compare
index.html
Outdated
<tbody> | ||
<tr> | ||
<td>`confidenceMethod`</td> |
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.
<td>`confidenceMethod`</td> | |
<td>`confirmationMethod`</td> |
index.html
Outdated
A property used for specifying how a <a>verifier</a> can gain more confidence | ||
in, the presentation of the credential being associated with a particular | ||
<a>subject</a>. The associated vocabulary URL MUST be | ||
`https://www.w3.org/2018/credentials#confidenceMethod`. |
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.
`https://www.w3.org/2018/credentials#confidenceMethod`. | |
`https://www.w3.org/2018/credentials#confirmationMethod `. |
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 would be good to not invent new security terminology that can so easily be confused with existing RFCs that do exactly the same thing:
By including a "cnf" (confirmation) claim in a JWT, the issuer of the
JWT declares that the presenter possesses a particular key and that
the recipient can cryptographically confirm that the presenter has
possession of that key.
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.
Except that they don't do exactly the same thing. The cnf
claim (and, I think, the related confirmationMethod
) is specifically tied to key possession, while the confidenceMethod
is explicitly broader and non-specific, though it might include key possession.
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.
@TallTed you are correct they don't do the same thing, and one of them is more dangerous because it has an unbound domain... https://en.wikipedia.org/wiki/Confidence_trick
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.
Adding confirmation like this is going to cause interoperability problems with existing securing formats, for example JWTs.
Reserving these properties is not necessary, since JSON-LD supports extension of credential attributes using @context
, and there is no working group consensus on how these terms will be used in the future, we should allow for implementation experience to drive the standards development process.
This PR groups 3 experimental properties into one change to the document, I think the entire PR is a mistake, but my only blocking feedback is regarding confirmation method.
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.
For the confidenceMethod
, I would like to see #1054 merged first.
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 would want to get PR #1054 to get merged before or not include confidence method as a reserved property but instead make it part of the core VCDM.
@OR13 wrote:
Assertion stated without explanation. It seems as if this was discussed at length during the VCWG special topic call and the group came to consensus that using https://www.w3.org/2017/vc/WG/Meetings/Minutes/2023-05-02-vcwg-special
This is going back on the consensus position of the group to have reserved properties. There doesn't have to be WG consensus beyond that the terms are reserved and that there is a simple statement related to what the term does.
It seems as if you were not present at the meeting that settled the topic on https://www.w3.org/2017/vc/WG/Meetings/Minutes/2023-05-02-vcwg-special Are you stating that consensus was not reached because you are objecting to the consensus reached on the call? |
@awoie wrote:
We could have done that, but #1054 is now blocked by @OR13.
https://w3c.github.io/vc-data-model/#reserved-extension-points Why should To reiterate the purpose of this PR:
We might want to add something that explains the above in an issue marker in this PR -- would anyone be averse to doing so?
We should update the description to match #1054. |
21ea2c4
to
a86f020
Compare
index.html
Outdated
<td>`confidenceMethod`</td> | ||
<td> | ||
A property used for specifying how a <a>verifier</a> can gain more confidence | ||
in, the presentation of the credential being associated with a particular |
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.
two suggestions. First, the comma after in is not needed. Second, 'being associated with a particular subject' is either ambiguous or unnecessary. All credentials are associated with a subject as they have a subject property, so does this mean that the presenter is associated with a particular subject or not?
I suggest simply remove 'being associated with a particular subject' or clarify to 'can gain more confidence
that the presenter of the credential is associated with the subject"
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.
Definitely, remove the extraneous comma.
in, the presentation of the credential being associated with a particular | |
in the presentation of the credential being associated with a particular |
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.
A less definite change, but no less definitely a problem, is that, as discussed, confidenceMethod
is not limited to connecting a presenter nor a presentation with a subject, nor to connecting a credential with a presenter. confidenceMethod
has been discussed as applicable to any attribute (and its value), meant to let the issuer guide the verifier to increased confidence in that assertion/claim by the issuer. I haven't come up with a more pithy way to put that, but maybe the above helps someone else to a concise and accurate description.
index.html
Outdated
<tbody> | ||
<tr> | ||
<td>`confidenceMethod`</td> |
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.
<td>`confidenceMethod`</td> | |
<td>`confidenceMethod`</td> | |
<td>`confirmationMethod`</td> |
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.
-1 to bringing back confirmationMethod
.
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.
how would you suggest we map cnf
then?
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.
how would you suggest we map
cnf
then?
A specific, single-key-style proof-of-possession type of confidenceMethod
(which someone will need to define) can be mapped to cnf
. Other types clearly wouldn't map, for example:
{
...,
"confidenceMethod": [{
"type": "FooSingleKeyProofOfPossessionConfirmationThing",
"cnf": { /* same kind of stuff that goes in 'cnf' }
}, {
"type": "DoesNotWorkWithCnf",
...
}]
}
The issue was discussed in a meeting on 2023-05-10
View the transcript1.6. Add new v2.0 reserved properties. (pr vc-data-model#1108)See github pull request vc-data-model#1108. Manu Sporny: PR 1108 - Add new v2.0 reserved properties.
Manu Sporny: think Oliver and Kristina wanted to see confidence method merged before this PR is merged, which we're not doing today. Orie Steele: seems like render method would go in by itself. there's work going in the CCG and people would agree with this.
Orie Steele: today Orie is ok with the confidence method in the table. Not a fan of it in any format and prefer to have it reviewed for terms independently. Manu Sporny: refactor PR to only refer to render method - any objections to that? Brent Zundel: no objections heard.
|
Co-authored-by: Brent Zundel <brent.zundel@gmail.com>
a86f020
to
1d1fc11
Compare
renderMethod
as reserved extension point.
I have reworked this PR to only include
Therefore: Normative, multiple reviews, changes requested and made, no objections, merging. |
This PR builds upon PR 1097 and adds three new properties (and their reserved URLs) to the reserved properties table:
confirmationMethod
/confidenceMethod
presentationSchema
renderMethod
There is now also an example specification that demonstrates how these reserved properties could be leveraged by external specifications: Verifiable Credential Rendering Methods
The specification notes that it utilizes the reserved property in the VC Data Model, and then defines a type that can be used with the extension point.
Preview | Diff