Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
update normative statements in ZKP section #818
update normative statements in ZKP section #818
Changes from 12 commits
f397abb
3b126be
301d284
741b9d5
a35df85
064e389
4120582
bd99a7b
557649b
d6bc9e4
96a8c06
f5b3945
c750517
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Somehow I missed this in previous reviews.
derived <a>verifiable credentials</a>
have to have beenissued to the same <a>holder</a>
?derived <a>verifiable credentials</a>
issued at all?<a>verifiable credentials</a>
?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.
Indy anoncreds zkp-credentials, which were used as the exemplar for much of the material in this section, require that issued credentials be bound to a holder. One reason for this is that anoncreds aren't bound to a subject, but are rather issued to the subject, so binding to the holder is a way to accomplish connection of the credential to the subject. Another is to give verifiers more confidence that credentials are linked even when the disclosed claims are kept to a minimum. Binding to a holder also makes sharing credentials more detectable and unwieldy, which is seen as valuable in the anoncreds ecosystem.
That said, there are more zkp-credential schemes now than there were when the section was written, and not all of them require holder binding, even though many still consider that best practice. This is why the MUST is being changed to a SHOULD.
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 suggestion appears to be the same as the current text in there. I presume this review comment was more about raising the questions which I think @brentzundel addressed. Please let me know if there's any additional changes that I missed to resolve this comment.
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.
Correct. However, I do not think that the answers provided by @brentzundel remove my concerns with this section.
I am mostly concerned about the "bound"/"binding" terminology in use here, which I think is a significantly stronger relationship than is actually present between these VCs and their Subjects or Holders. I think it would be better to say that xyz property/ies in the Credentials under specific discussion are (or SHOULD be or MUST be) set to strings or URIs which identify the Holder who is expected to Present the VC to any Verifiers downstream. Then, the Verifier is expected to check that either some other qrs property/ies were (or SHOULD have been or MUST have been) set to strings or URIs which identify the Subject, OR the Subject is always the same as the Holder identified by the values of xyz property/ies, ... Or something along those lines.
I think that "anoncreds aren't bound to a subject, but are rather issued to the subject, so binding to the holder is a way to accomplish connection of the credential to the subject" translates roughly to "anoncreds are Issued to their Subject, which is defined as a sapient being, and which is not identified as the Subject within the VC. Identifying the Holder is an indirect way to accomplish association of the VC with its Subject," which Subject is not really obscured by this method, since the identified Holder is always the Subject and anyone who can access the value of the field identifying the Holder is thereby able to access to identifier of the Subject. Leading to another concern with this section, since it seems that the Subject anonymity desired to be delivered through ZKP, at least in part by identifying one Holder who must be the Subject, instead of identifying the Subject, isn't actually delivered.
I'm sure I'm missing something exceedingly clever, and I hope someone can explain it in terms I can understand and that we can swiftly adopt (and/or edit slighly to adopt) for this section.
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. And if there are cases where there aren't actually any properties in the VC itself with this information because some mechanisms include it via secrets and/or proof data, then that can be stated as an option as well. I think it's important that we indicate that we're talking about setting expectations -- and not assert a level of control that isn't possible. It's also fine to say that there are mechanisms that attempt to impede adversaries that try to subvert these expectations, but such statements should include a friendly warning that they all have limitations that need to be well understood by implementers (and readers should consult external documentation for that).
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 thread continues in #821.
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.
Do we want to action this within this PR - if so can you propose some suggested changes based on the discussions so far.
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.
@kdenhartog -- There are no changes to make here in this PR based on this comment.