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 validation section regarding holder #1199

Merged
merged 39 commits into from
Sep 14, 2023
Merged

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Jul 12, 2023

Addresses #959


Preview | Diff

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

+1 overall to the concept of this PR. I've been wanting to have stronger statements around how to verify subject == holder. We haven't put that text in to date because people insisted that had to do with protocol, but this PR demonstrates that we can describe expectations w/o getting into protocol. I'll do a full review of this after the text has settled a bit... just noting preliminary support for "saying something" about this topic in the spec. My preference is we get normative about the statements so it's clear how one can do DIDAuth (or something equivalent) with the data in the VP and VCs (which reflects reality today).

Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

pls address my comment.

index.html Outdated
Comment on lines 4859 to 4860
The value associated with the <code>holder</code> <a>property</a> is expected
to identify an <a>holder</a> that is known to and trusted by the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having issues with the statement that the holder is supposed to be known and trusted by the verifier`. It is certainly a possibility but two questions:

  1. why do you assume such a strong statement about the holder/verifier relationship? The PR suggests that this statement is universal an does not explain why. I also want to say that the securing mechanisms have nothing to do with whether a holder is trusted or known by the verifier. They just protect the integrity of the payload.
  2. why is this even important in the definition of the holder property?

Copy link
Contributor

@awoie awoie Jul 13, 2023

Choose a reason for hiding this comment

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

My suggestion would be to remove that sentence entirely, or are you saying that the holder property should only be used if all of the conditions below apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy pasted the issuer validation section, I referred to the 3 roles in their graph structure, and I described in the abstract what validating that these roles are the same looks like.

I'm having issues with the statement that the holder is supposed to be known and trusted by the verifier`.

So am I, but its unavoidable, since the verifier needs to resolve keys that represent them... and those keys might be in a country the verifier does not trust, or or a blockchain they don't trust, or from an identifier who has previously lied, been convicted, or sanctioned...

Maybe the word trust is the problem here, any suggestions?

  1. I also want to say that the securing mechanisms have nothing to do with whether a holder is trusted or known by the verifier.

This can't be true, since an unsecured presentation that be used to do anything but send secured (possibly stolen) credentials... we also have language thanks to @brentzundel regarding claims the holder makes in a presentation and how they are secured... verifier needs to understand that securing mechanism to rely on holder claims.. like they understand credential securing mechanism to issuer claims.

  1. They just protect the integrity of the payload.

I wish this were true, but data integrity defines retrieve verification method, and OIDC has its own ways of discovering keys... a verifier needs to trust those systems in order to understand that the payload is secure.

2. why is this even important in the definition of the holder property?

Why is it important in the issuer property?

IMO, its for the same reasons, but interested if folks think holders have less security / trust / autonomy than issuers... we should document that better if its true.

My suggestion would be to remove that sentence entirely, or are you saying that the holder property should only be used if all of the conditions below apply?

Similar to the issuer property not being trust worthy when there is no security on the credential.

holder property can be on unsecured presentations, a good use case for this, is when you have previously authenticated a client, and you are relying on channel authentication for messages from the holder.

This is completely legal in the current VCDM... if it seems like a bug we can correct that.

Copy link
Contributor

@dlongley dlongley Jul 13, 2023

Choose a reason for hiding this comment

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

So am I, but its unavoidable, since the verifier needs to resolve keys that represent them... and those keys might be in a country the verifier does not trust, or or a blockchain they don't trust, or from an identifier who has previously lied, been convicted, or sanctioned...

Maybe the word trust is the problem here, any suggestions?

Hmm, the trouble here perhaps stems from a conflation of the holder's identity with its identifier. I agree that the verifier must be able to understand and process the identifier used to refer to the holder. This, however, does not mean that the verifier has a pre-existing relationship or a trust relationship with the holder itself or that the verifier recognizes the holder's "identity" in any way (other than by an identifier that references 'the same holder').

I think this is what is troubling @awoie and I agree.

It's worth noting (since it was mentioned above) that the issuer is fundamentally different in this respect -- issuer claims cannot be accepted (assumed) "as true" without some kind of external understanding or relationship around who the issuer is (the issuer's actual identity), which is more than just what identifier is being used to refer to the issuer. Of course, a verifier similarly needs to also be able to understand, process, and have confidence that an identifier does, in fact, refer to the issuer they think it does, but that's not the same thing.

I'm not sure yet how we resolve this, but the problem seems to stem from what I read as @OR13's desire to clearly indicate that the holder identifier (the "value" of the holder property) needs to be of a sort that the verifier can understand and process (this is certainly true or it won't be accepted) and @awoie's desire to make clear that that doesn't mean the verifier necessarily has any other knowledge about the identity of who the identifier refers to. In fact, a completely new relationship (or "introduction") could be getting established at the very moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, if we would commit my latest suggestion below, this would address my concern: #1199 (comment)

Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

direction looks good, pls address comments. i'm also curious how this relates to https://w3c.github.io/vc-jwt, or is this something that the jose/cose-specific media types need to take care of which might be also fine.

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
Comment on lines 4859 to 4860
The value associated with the <code>holder</code> <a>property</a> is expected
to identify an <a>holder</a> that is known to and trusted by the
Copy link
Contributor

@awoie awoie Jul 13, 2023

Choose a reason for hiding this comment

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

My suggestion would be to remove that sentence entirely, or are you saying that the holder property should only be used if all of the conditions below apply?

OR13 and others added 5 commits July 13, 2023 08:10
Co-authored-by: Oliver Terbu <[email protected]>
Co-authored-by: Oliver Terbu <[email protected]>
Co-authored-by: Oliver Terbu <[email protected]>
@OR13
Copy link
Contributor Author

OR13 commented Jul 13, 2023

i'm also curious how this relates to https://w3c.github.io/vc-jwt, or is this something that the jose/cose-specific media types need to take care of which might be also fine.

I don't think holder or issuer are restricted to specific media types in the core data model.

Some securing mechanisms (like JWT) already have registered claims that support existing verification systems.

See #1149 for more details

@OR13
Copy link
Contributor Author

OR13 commented Jul 13, 2023

@msporny

We haven't put that text in to date because people insisted that had to do with protocol,

We put proof verification material discovery in data integrity and vc-jwt... does that mean we only validate proofs from "issuers" and never "holders"?

... but yes, I had the same thought when copy pasting from issuer validation section.

My preference is we get normative about the statements so it's clear how one can do DIDAuth (or something equivalent) with the data in the VP and VCs (which reflects reality today).

I am in favor of "getting normative" in the core spec, as long as that doesn't mean normative for only 1 securing mechanism.

The other option is to keep normative validation out of the core spec, and refer to informative section of the core spec fro context, while relying on the securing spec to handle securing specific details for issuer and holder.... I think that might end up being more flexible, but it reintroduces the "did method problem" to VCs... now you need to read a billion VC specs in order to get claims... whereas you need to read a billion DID specs to get public keys or verification material.

@OR13
Copy link
Contributor Author

OR13 commented Jul 13, 2023

@msporny @awoie I don't understand what you are requesting changes on, beyond potentially my answering your questions, which I have now done.

Concretely, what text would you like to see added or removed?

Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

lgtm. perhaps accept my latest suggestion.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
OR13 and others added 2 commits July 13, 2023 18:11
Co-authored-by: Brent Zundel <[email protected]>
Co-authored-by: Dave Longley <[email protected]>
Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

lgtm

@TallTed
Copy link
Member

TallTed commented Jul 14, 2023

This PR is hugely problematic. I need to give it a close re-read, including all the prior comments, but just touching on a snippet of one comment --

the verifier must be able to understand and process the identifier used to refer to the holder

There may be use cases where this is so, but an anonymous holder (presenter) is entirely plausible in many cases.

Example: I present a VC coupon to the cashier at my local supermarket. If that coupon is not restricted to one use per customer — such as when it is instead restricted to one use per transaction, or to one use per matching item purchased, etc. — then there is no need to identify myself (as is true for most paper coupons, today) — so there is no need for the verifier to understand nor process any identifier used to refer to me.

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.

A few minor nits in suggestions. Thanks!

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
@OR13
Copy link
Contributor Author

OR13 commented Aug 9, 2023

@jandrieu per the call, I opened #1232

@iherman
Copy link
Member

iherman commented Aug 9, 2023

The issue was discussed in a meeting on 2023-08-09

  • no resolutions were taken
View the transcript

2.2. Add validation section regarding holder (pr vc-data-model#1199)

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

Manu Sporny: PR 1199, about validation section on "holder", waiting on JoeAndrieu feedback.

Joe Andrieu: not sure where I'm at, "validation" versus "verification", no new comments from 3 weeks ago.

Orie Steele: Agree "validation/verification", can file an issue, Joe agrees.

@TallTed
Copy link
Member

TallTed commented Aug 11, 2023

As noted above and elsewhere, I refer all and sundry to RDF Concepts, Introduction, where resource are clearly stated to be synonymous with entity, and both/either may be anything, and may be the subject of a VC.

In order to perform a role, the given entity/resource must be capable of such actions as are required for that role; for instance, a document probably cannot be an issuer or verifier.

The ranges of holder, issuer, verifier, and subject all remain entity (a/k/a resource). The capacity of an individual entity/resource to fulfill a role is a distinct question, which might best fall into the business logic of a verifier.

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

iherman commented Aug 16, 2023

The issue was discussed in a meeting on 2023-08-15

  • no resolutions were taken
View the transcript

1.2. Add validation section regarding holder (pr vc-data-model#1199)

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

Brent Zundel: next 1199 add validation section regarding holder; a few approvals. outstanding change requests from Joe. believe they've been addressed. please re-review.
… Ted also has some outstanding changes.

Joe Andrieu: I have reviewed recently.
… have pending comments. forgot to hit button to send comments.

Brent Zundel: moving in a direction where consensus is still possible.

Co-authored-by: Ted Thibodeau Jr <[email protected]>
@OR13
Copy link
Contributor Author

OR13 commented Aug 22, 2023

@jandrieu I took @TallTed 's suggestion... have your concerns been addressed, or are there others?

Copy link
Member

@brentzundel brentzundel left a comment

Choose a reason for hiding this comment

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

minor nit

index.html Outdated Show resolved Hide resolved
Co-authored-by: Brent Zundel <[email protected]>
@OR13
Copy link
Contributor Author

OR13 commented Aug 25, 2023

AFAIK, this is still waiting on a re-review from @jandrieu .

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

@jandrieu jandrieu left a comment

Choose a reason for hiding this comment

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

It's getting there, but we still have some sections that need work.

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
OR13 and others added 4 commits August 30, 2023 14:42
Co-authored-by: Joe Andrieu <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@OR13
Copy link
Contributor Author

OR13 commented Aug 31, 2023

@jandrieu I took @TallTed 's suggestions which were on top of yours to try to save on review time, please re-review.

index.html Outdated Show resolved Hide resolved
Co-authored-by: Dave Longley <[email protected]>
@iherman
Copy link
Member

iherman commented Sep 3, 2023

The issue was discussed in a meeting on 2023-08-29

  • no resolutions were taken
View the transcript

1.1. Add validation section regarding holder (pr vc-data-model#1199)

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

Manu Sporny: waiting on JoeAndrieu re-review.

Joe Andrieu: will look at again.

@iherman
Copy link
Member

iherman commented Sep 5, 2023

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

  • no resolutions were taken
View the transcript

3.1. Add validation section regarding holder (pr vc-data-model#1199)

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

Manu Sporny: +1199 joe said he would review.

Joe Andrieu: there has been some discussions on this already.

@Sakurann Sakurann merged commit fc5a741 into main Sep 14, 2023
@iherman
Copy link
Member

iherman commented Sep 15, 2023

The issue was discussed in a meeting on 2023-09-14

  • no resolutions were taken
View the transcript

1.1. (pr vc-data-model#1199)

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

Brent Zundel: Raised by orie. Good review and a number of comments. 1 open request for changes from TallTed. Are you're changes unaddressed?

Ted Thibodeau Jr.: Haven't had the chance to review.

Brent Zundel: If it meets your approval, we can merge.

@msporny msporny deleted the fix/959-subject-is-holder branch November 11, 2023 16:01
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.