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 confidence method to VCDM #1054

Closed
wants to merge 49 commits into from
Closed

Add confidence method to VCDM #1054

wants to merge 49 commits into from

Conversation

awoie
Copy link
Contributor

@awoie awoie commented Feb 27, 2023

Potentially fixes #789, #882

This PR includes normative changes that define a new property confirmationMethod (aka holder binding):

  • Add confirmation method to Advanced Concepts Section
  • Add confirmation method to Types Section
  • Add confirmation method to Extension Points Section

Out-of-scope:

  • Privacy and security considerations which requires a separate PR
  • Validation section which requires a separate PR

Preview | Diff

index.html Outdated Show resolved Hide resolved
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Language and punctuation fixes.

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

@mprorock mprorock 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 this is a great direction - would like removal of liability mentions

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

awoie commented Mar 7, 2023

I think this is a great direction - would like removal of liability mentions

@mprorock I used @TallTed suggested language. Does this work for you now?

index.html Outdated Show resolved Hide resolved
@awoie awoie force-pushed the awoie/fix-789 branch from 5d864b1 to 05b4034 Compare May 9, 2023 18:33
@awoie
Copy link
Contributor Author

awoie commented May 9, 2023

Editor hat off, +1 to this PR (my only personal request is the first bullet item).

Editor hat on, I'm noting a few things that have probably gotten lost in the 99 comments, to date:

  • There is a bikeshedding discussion over the property name and type names, that should be noted in an issue before we merge the PR.
  • It's not clear if @mprorock's change requests have been addressed.
  • It's not clear if @dlongley's change requests have been addressed.
  • It's not clear if @TallTed's change requests have been addressed.
  • It is not clear if the WG is taking the approach of reserving properties such that development of this sort can be performed in parallel, outside of this WG, until there is a demonstration of at least two interoperable implementations before we add it to the core specification (before CR), OR if the WG will not have the concept of reserved properties and core properties will need to be defined in the core spec, without implementation commitments or examples of usage, and then be marked as "at risk" when entering CR.

To be clear, my change requests are to ensure that the WG is being consistent about experimental, stable, and deprecated extension points. I'm trying to clearly summarize where this PR is at present and what needs to be resolved before we can safely merge it.

I'm re-requesting reviews from @TallTed and @dlongley given that they're still showing as blocking. I can't request a re-review from @mprorock since he never registered his request via a change request.

@Sakurann and @brentzundel I believe the last bullet point needs WG discussion, as a decision on that affects how we are going to process extension points in the spec leading up to CR and during CR.

I believe everything from that list should be addressed. Please approve if this is the case. I also updated the reworded the commit messages for all "update index.html" commits as requested.

@David-Chadwick
Copy link
Contributor

a verifier can gain more confidence related to the binding of the credential to a particular subject.
@msporny It is interesting that you should say that, because when the name of this property was being discussed, I suggested "bindingMethod" but people retorted that it was nothing to do with binding. Oh well!!

On a different point, confidenceMethod is not only about binding to a subject, but is also about binding to the presenter of the credential when subject NE holder.

@TallTed
Copy link
Member

TallTed commented May 10, 2023

I submit that @msporny mis-typed. It's still not about binding. Credentials are inherently about particular subjects, however they are identified. confidenceMethod might be aptly used about any claim/assertion in a credential — as in "this confidenceMethod is how I, the issuer, determined that this attribute and value are accurately applied to this subject, and how you, the verifier should accept this claim as accurate".

So far as I can tell, binding is not an appropriate term to use for anything in the realm of Verifiable Credentials, including but not limited to the examples cited by others in these umpteen "binding" threads.

@msporny
Copy link
Member

msporny commented May 10, 2023

@TallTed wrote:

I submit that @msporny mis-typed.

Yes, I was being sloppy with my language. What @TallTed said is far more accurate.

@iherman
Copy link
Member

iherman commented May 11, 2023

The issue was discussed in a meeting on 2023-05-10

  • no resolutions were taken
View the transcript

1.1. Add confidence method to VCDM (pr vc-data-model#1054)

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

Manu Sporny: Confidence method PR - there seemed to be consensus to merge last week but Orie had some objections. Wondering what we're doing with it?

Orie Steele: concerned with this PR - calling it confidence methods is confusing and invites degraded security characteristics but doesn't work across industries. There aren't yet two independent implementations yet.
… tangled up into the table registry at this point which add a bunch of defs to the core spec. It's a valuable thing to look at but not helpful to focus on right now.

Brent Zundel: other comments?

Manu Sporny: To untangle things, one reason to put reserved property in the spec is to acknowledge important work. While Orie is the -1 in the voting but these objections need to be addressed.
… we're listing everything that doesn't have 2 independent implementations, but we don't tell people how to implement the feature -no normative language etc.

Orie Steele: +1 the approach being taken for render method, that seems to be making progress.

Manu Sporny: in the render method spec made it its own specification and defined the normative property of render method in that separate text. Could follow that path for this confidence method mechanism, without txt in the core spec.

Orie Steele: lets use the ccg to do incubation work, not the W3C TR.

Manu Sporny: what specific things will break if we put this into the specification? Need more details beyond concerns to understand how to respond.
… if we get 2 independent implementations we could move that text into the spec in the CR phase.

Michael Prorock: likes manu's approach to create an independent spec. This gets it out of the core spec, do the work in CCG, and bring it back when it's ready.

Orie Steele: Also folks should beware to of the english language issue with "confidence"... https://en.wikipedia.org/wiki/Confidence_man.

Brent Zundel: concerned that we sounded like agreement last week and now we sound differently.

Orie Steele: I objected, outside of the call time.

Orie Steele: sorry I could not make the call.

Orie Steele: I am supportive of reserving terms, and then not working on them in the working group.

Dave Longley: fine with the approach manu has suggested making it a separate spec. The new argument is combining it with confidence associated with "man" but we should proceed with Manu's plan.

Brent Zundel: clarifying which plan?

Michael Prorock: I would note, as i noted on the last call, i am likewise not a fan of confidenceMethod vs confirmationMethod for the technical reason of alignment with cnf and other existing use of this concept.

Michael Prorock: but lets keep work moving.

Orie Steele: +1 to most of what dlongley said.

Dave Longley: add property to reserve table and can add it to the main spec later. If the group doesn't want that at risk text in the spec he could live with it as well.

Orie Steele: I am supportive of the text being developed in ccg.

Brent Zundel: "At risk" has a specific meaning. It's not common to use "at risk" in specifications at this stage.

Orie Steele: +1 to brents comment regarding adding "at risk" to things that are not yet developed pass the ccg draft phase.

Manu Sporny: based on the minutes from the last call we had agreement to bring the defs into the spec. We could poll to see if preference is to include in the spec as "at risk" or move it out to an external thing.
… Manu not sure if polling is useful at this point.

Michael Prorock: not a fan of this at this point. But he's not going to block this at this stage. Strong preference to do the work elsewhere an not a fan of "at risk" in the language.

Orie Steele: I agree that at risk does rightly apply to the existing terms that shipped in v1 without concrete support.

Ted Thibodeau Jr.: last week we did take into account the W3C meaning of "at risk". We were going to list both in table of reserve terms and in the implementation section. whichever one won out would stay in the spect the other removed.

Dave Longley: +1 to TallTed.

Orie Steele: status list and credential schema are in good shape.

Dave Longley: +1 to what TallTed being the best way forward as well.

Ted Thibodeau Jr.: same is true in reverse. Could lie fallow for a period of time and be addressed later.

Gabe Cohen: +1 @TallTed well said.

Dave Longley: +1 to TallTed's comments on the naming.

Ted Thibodeau Jr.: the use of confidence man should not impact the meaning of confidence. That objection has no utility for me.

Shigeya Suzuki: .

Orie Steele: clarifying why confidence is problematic -it's a predicate in JSON-LD. It's an open RDF cookie and could allow adding a confirmation type of sending a person to a home (physically). Not a good idea to have any RDF type.

Ted Thibodeau Jr.: There still is no binding happening here.

Orie Steele: legit to have a technical means to have a binding, but a vocab that invites people to do whatever they want isn't appropriate. Dislike word "confidence" he's amenable for a separate path forward.

Phillip Long: dlongley - in an open world model there will be objections. Confidence is better than confirmation. Terminology is the best we have now and works with an open world model.

Brent Zundel: let's do some polls.

Orie Steele: In some sense it is better that we not use "confirmation" if it does not provide strong security characteristics unless used with a specific RDF type.

Manu Sporny: asks Brent to craft something.

Brent Zundel: poll: we will add confidenceMethod to the reserved table and add the language to the spec with a note that it will be removed if there are not two implementations.

Manu Sporny: +1.

Dave Longley: +1.

Andres Uribe: +1.

Gabe Cohen: +1.

Brent Zundel: +1.

Greg Berstein: +1.

Brent Zundel: poll open.

Samuel Smith: 0.

Andrew Whitehead: +1.

Orie Steele: -1.

Michael Prorock: -0.5.

Phillip Long: +1.

Shigeya Suzuki: -0.5.

Chris Abernethy: 0.

Ted Thibodeau Jr.: +1 "removed from the spec and left in the reserved table".

PhilF: 0.

Ted Thibodeau Jr.: clarification "removed form the spec and left in the reserved table".

Brent Zundel: Orie's -1 is noted.

Shigeya Suzuki: wondering if this method may increase confusion. People in this call understand there are different camps. The people who lead this spec might confuse this with the "confidence method".

Brent Zundel: poll makes it clear we don't have consensus and we'll continue talking about it.

Manu Sporny: should it be marked as "do not merge" and move on?

Orie Steele: can we try a resolution to do what we did with render?

Brent Zundel: label it "discusss".

@awoie
Copy link
Contributor Author

awoie commented May 16, 2023

The issue was discussed in a meeting on 2023-05-10

  • no resolutions were taken

View the transcript

@OR13 after reading the minutes, I can see you have the following concern:

Orie Steele: legit to have a technical means to have a binding, but a vocab that invites people to do whatever they want isn't appropriate. Dislike word "confidence" he's amenable for a separate path forward.

I don't think that this is a valid concern since the W3C VCDM has taken that approach with other extension points as well. For instance, I could define a status method that requires the verifier to call an expensive phone number, or to send a text message with the verifier's home address, so the issuer sends somebody to the verifier's home.

@OR13 what is the difference between these two? Why is the W3C VCDM extensibility model not appropriate for confidence method?

@OR13
Copy link
Contributor

OR13 commented May 16, 2023

@awoie you don't need to make changes to this TR to take advantage of the extensibility model of JSON-LD.

I think it is unsafe to define confidenceMethod in the way this PR attempts, it would be better to define more narrowly, a security suite specific way to establish higher confidence... it is not useful (and harmful and confusing) to do it in the core data model, which is not about establishing security characteristics at all... it is an information data model based on a JSON serialization of RDF... it does not do anything related to "confidence" or "verifiability"... that comes from data integrity or json web tokens, or something else...

@awoie
Copy link
Contributor Author

awoie commented May 17, 2023

@awoie you don't need to make changes to this TR to take advantage of the extensibility model of JSON-LD.

I think it is unsafe to define confidenceMethod in the way this PR attempts, it would be better to define more narrowly, a security suite specific way to establish higher confidence... it is not useful (and harmful and confusing) to do it in the core data model, which is not about establishing security characteristics at all... it is an information data model based on a JSON serialization of RDF... it does not do anything related to "confidence" or "verifiability"... that comes from data integrity or json web tokens, or something else...

Validating the binding between the claims and the presenter would then require to understand whether the specific securing mechanism has support for that since on the VCDM layer this won't be visible. This would have to be implemented on a different layer in that case. It would also mean that some securing mechanisms are more prone to certain attacks or at least less suitable for online presentation protocols since they don't have that feature.

I can see your point and I can see that this can be, indeed a feature of the securing mechanism and effectively on a different layer. In that case, I would add at least a security considerations section to the VCDM that talks about replay / forgery / cloning concerns and that implementer's should be aware of that and choose securing mechanisms accordingly. I'll create an issue for that. I believe we had some ticket on relay but we closed it. This issue is less about relay, more about replay / cloning although relay is related.

I don't understand your comment on @context as a solution for that though since:

  • we would need to define this term somewhere and the VCDM 2.0 would be the best place for that, especially because there can be different methods or RDF classes for those methods
  • you said that this should happen on a different layer anyways, so it sounded a bit contracting to some of the comments you said.

Copy link
Contributor

@mprorock mprorock left a comment

Choose a reason for hiding this comment

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

I would suggest that this would be better handled in a separate specification and not as a part of the core data model

@awoie
Copy link
Contributor Author

awoie commented May 24, 2023

I would suggest adding in a similar way to render method. Furthermore, I'll create issues in the security considerations section to highlight that certain things are not prevented when the VCDM is used without additional securing mechnisms.

@awoie
Copy link
Contributor Author

awoie commented Jun 5, 2023

I asked @swcurran and @paulbastian here for their opinion. I suggest if there is no objection in the next 2 weeks, we can close this PR.

@brentzundel brentzundel added pending close Close if no objection within 7 days and removed discuss labels Jun 5, 2023
@brentzundel
Copy link
Member

It has been proposed that we close this PR in favor of another path forward. I have added the label pending close and will close in one week if there is no objection.

@jandrieu
Copy link
Contributor

jandrieu commented Jun 6, 2023

I'd like this to remain open until the context is in fact updated.

There are too many arguments over what should and shouldn't be in the context for this to be removed before we have consensus on including it in context.

@OR13
Copy link
Contributor

OR13 commented Jun 6, 2023

I don't think we should leave PRs open, that we have no intention of merging... that is what issue markers are for.

I suggest raising a PR to add an issue marker to the spec, and then closing this.

@OR13
Copy link
Contributor

OR13 commented Jun 6, 2023

Given:

This PR should be closed, the PRs above can remain open until they achieve consensus.

@TallTed
Copy link
Member

TallTed commented Jun 7, 2023

I suggest raising a PR to add an issue marker to the spec, and then closing this.

Correct order:

  1. Raise an ISSUE wherein to discuss the problem, and proposed solutions, up to the point where one or more PRs may be drafted that would implement those solutions.
  2. Potentially, raise a PR to add an ISSUE MARKER to the spec, which points to the ISSUE opened in (1).
  3. Raise one or more PRs to offer concrete solutions to the ISSUE.
  4. Upon consensus of which if any PR should be merged, do so, simultaneously with closing other PRs, and ISSUE; this is also when the ISSUE MARKER should be deleted from the spec.

@awoie
Copy link
Contributor Author

awoie commented Jun 29, 2023

Closing this PR since this work was transferred to CCG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending close Close if no objection within 7 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explicit reference should be added about binding the VC to the holder