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

Investigate recommended alternative approach for verify #365

Closed
nrempel opened this issue Nov 1, 2019 · 7 comments
Closed

Investigate recommended alternative approach for verify #365

nrempel opened this issue Nov 1, 2019 · 7 comments
Assignees

Comments

@nrempel
Copy link
Contributor

nrempel commented Nov 1, 2019

  1. current state of the 1893 PR.
    It doesn't solve the problem. It just ignore every restrictions by name aside from the current one disclosed attribute.
  2. master branch behaviour - always return false for the case with multiple restrictions
    So 1) may have false-positive result,
  3. false-negative
    That's why I haven't merged it yet
    1 can be extended by application check that all attributes are from the same proof. To do it the app can just make sure that sub_proof_index are the same for whole group of attrs
  4. extension for 1):
    Allow only the restrictions for attributes (by value) which are disclosed near the current one attrrib and does validation in libindy around that.
    It seems like a solution for your particular case, but not the general solution and may be unclear for users
  5. introduce more concrete restriction which would force to use some attributes from the same credential. Kind of grouping of attributes.
proof req {
  requestedAttrs: {
    attrXref: { name: X, restrictions: { attr::X::value == valX, groupedWith == groupRef1 }
    attrYref: { name: Y, restrictions: { attr::Y::value == valY, groupedWith == groupRef1 }
   } 
}

For me the 3rd one seems generic enough and should solve your problem as well (if I understood it correctly)
Let me know are the descriptions of all solutions and the difference are clear enough. Fill free to ask questions. I'd like to be aligned at understanding the options and then we can choose the right solution

@nrempel nrempel self-assigned this Nov 1, 2019
@nrempel
Copy link
Contributor Author

nrempel commented Nov 6, 2019

More discussion about a possible solution here: hyperledger-indy/indy-sdk#1893

@nrempel
Copy link
Contributor Author

nrempel commented Dec 10, 2019

A solution was implemented in https://github.com/hyperledger/indy-sdk/pull/1958/files

I am testing these changes.

@nrempel
Copy link
Contributor Author

nrempel commented Dec 20, 2019

I was blocked by build failures last week. I'm testing changes now with 1.13 stable release.

@nrempel
Copy link
Contributor Author

nrempel commented Dec 30, 2019

This is ready and in review #404.

@esune I'm going to leave the PR open for the moment. We may want to wait for this to land in a stable release before merging. If there is no rush, that is the least disruptive and least effort path.

@esune
Copy link
Member

esune commented Jan 2, 2020

@nrempel I merged the PR and logged #412 to track the need to update the base image to an "official" release.

As far as my understanding goes, having the self-verify functionality work all the way through is required for the launch of ICOB so it is best if we merge the change and track future updates.

@swcurran and @WadeBarnes can confirm or deny my claims ;)

@nrempel
Copy link
Contributor Author

nrempel commented Jan 2, 2020

Sounds good!

@swcurran
Copy link
Contributor

swcurran commented Jan 2, 2020

Not strictly required for ICOB deployment, but definitely something we want ASAP.

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

No branches or pull requests

3 participants