TimoGlastra
commented
7 months ago Open TimoGlastra opened 7 months ago
TimoGlastra
commented
7 months ago TimoGlastra
commented
7 months ago 
swcurran
commented
7 months ago I see the eq, ge and aggregated in both, so the proofs themselves are the same. I see that the primary_proofs element is presented differently. Anything else?
It’s hard to see all the differences in trying to scan the presentations. Any more details on the difference you are calling out.
Note that the Indy SDK still relies on Ursa, which has been archived. Changes have been made to AnonCreds to improve since the cl-signatures crate was moved out of Ursa. As such, it is not surprising that the indy-sdk is falling behind.
TimoGlastra
commented
6 months ago Maybe we can discuss this during the WG call later today?
swcurran
commented
6 months ago Thanks for bringing this up again @TimoGlastra — the difference that you note is not what I had expected. @andrewwhitehead is going to take a look.
The presentation has one source credential used to share one revealed value and one predicate. That should be mean there is (as in the indy-sdk case) one eq_proof for the revealed value, and one ge_proofs entry for the predicate. If there were more predicates, there would be more ge_proofs entries (which should be called ne_proofs as in “not equal”). As Timo notes, in the AnonCreds RS case, there are two primary proofs, one with the eq_proof for the revealed value, and then a second with both an eq_proof (but this time with no revealed values), and the predicate ge_proofs.
TBD why the change, but at this point it appears to be unexpected.
andrewwhitehead
commented
6 months ago Essentially in the anoncreds case (either in AFJ or anoncreds-rs, I'm not sure which yet), the same credential is being referenced separately for the attribute reveal and the predicate. I can reproduce the behaviour in the indy-credx Python wrapper by copying the source credential before adding it to the PresentCredentials structure:
present_creds = PresentCredentials()
present_creds.add_attributes(
cred_received, "test_eq", reveal=True,
)
present_creds.add_predicates(
cred_received, "test_ge",
)
presentation = Presentation.create(
pres_req, present_creds, {}, link_secret, [schema], [cred_def]
)vs.
present_creds = PresentCredentials()
present_creds.add_attributes(
cred_received, "test_eq", reveal=True,
)
cred_copy = cred_received.copy()
present_creds.add_predicates(
cred_copy, "test_ge",
)
presentation = Presentation.create(
pres_req, present_creds, {}, link_secret, [schema], [cred_def]
)Ah interesting. Yeah we have some auto-serialization logic to make it easier to work with the wrapper, but it probably encodes all objects separaetly to a native object.
As an optimization we should probably check whether some objects are equal, and then create the same cred instance for them (although that can lead to issues with not knowing when we can dispose of the object).
However, I think AnonCreds RS could also just look at equality of e.g. signatures to see if the credentials are the same (or the id or something).
This does make me curious whether there's a simple way for someone to check this. At least in AFJ we verify the proof and then we return which attributes/predicates were shared for which group, but as the predicates and attributes are from separate groups, if you just look at the revealed_attrs and revealed_predicates and revealed_attr_groups you won't be able to see whether a predicate and attribute group are from the same credential?
Is this something that we should add a check for? There's not really a way to indicate this requirement in a proof request, so it's up to the user then to decide whether or not to do this (It's valid I think to say I want name and age from the same credential, but for age I only want to know it's >18. Indy SDK seems to create a valid proof in this case, but it's also fine if it is created as two proofs, as we don't request it being from the same cred).
Should AnonCreds request allow to specify that predicate and attribute request groups can be linked to each other to indicate the values must come from the same cred?
For AnonCreds with DIF PE this will actually be possible as we can combine predicates and attribute requirements into a single input descriptor, which should result in a single credential. Then we would have to do some manual processing of the submitted proof to check whether the predicates and attributes are actually submitted within the same proof and not as two separate proofs (as a malicious user could use that to combine two credentials then?)
andrewwhitehead
commented
6 months ago Looking at the AFJ code, it does already dedupe the credentials as it is loading them (by credential ID), but then it adds separate entries for each attribute and predicate here, and I think that logic can be easily updated: https://github.com/openwallet-foundation/agent-framework-javascript/blob/main/packages/anoncreds-rs/src/services/AnonCredsRsHolderService.ts#L147-L159
It would be great to have better ways to specify that attributes and predicates are meant to be from the same credential. Initially there was no support for that, and BC worked with Evernym to get the 'names' property supported for requested attributes, which does allow for attributes (but not predicates) to be grouped.
swcurran
commented
6 months ago Per @andrewwhitehead ’s note, it looks like this is an AFJ issue, not an AnonCreds RS issue, correct? As such, and we leave it to the AFJ team, @TimoGlastra ?
Thanks!
TimoGlastra
commented
6 months ago Well yes we can 'fix' this in AFJ, but I don't think it will matter very much to most people using AnonCreds
As you would have to manually inspect the submitted proof for this to be verified, as AnonCreds lib will allow both proofs
Make we don't want to extend AnonCreds further, but maybe could add an optional ref id or something to make it so that two groups must come from the same cred? That's mostly in the proof request then and the final proof check.
This would also be useful for pex support as you can request multiple attributes + multiple predicates in a single input descriptor.
TimoGlastra
commented
6 months ago cc @2mau we're going to have to manually check that a credential submitted in PEX if it has multiple attributes/predicates they're all from the same proof (and thus cred).
TimoGlastra
commented
6 months ago Thanks for looking into it Andrew, we will at least make the needed changes in AFJ
swcurran
commented
6 months ago Well yes we can 'fix' this in AFJ, but I don't think it will matter very much to most people using AnonCreds
As you would have to manually inspect the submitted proof for this to be verified, as AnonCreds lib will allow both proofs
Make we don't want to extend AnonCreds further, but maybe could add an optional ref id or something to make it so that two groups must come from the same cred? That's mostly in the proof request then and the final proof check.
This would also be useful for pex support as you can request multiple attributes + multiple predicates in a single input descriptor.
Sorry — I don’t understand your response. I had assumed the issue was that the two are different and broken, and the reason they are different (and causing problems) is something manifest in AFJ. Is that not accurate?
I don’t think we should add AnonCreds 1.0 functionality to make predicates and revealed attributes come from the same credential — although I agree it would have been the right thing to do in the first place.
TimoGlastra
commented
6 months ago had assumed the issue was that the two are different and broken, and the reason they are different (and causing problems) is something manifest in AFJ. Is that not accurate?
I think that is partly correct. The problem I see here is that the holder influences whether one or two proofs are created and both are verified correctly by the verifier.
If one proof is the 'right' thing, two proofs should be rejected by the verifier side. So the verifier should be able to pass in that case that the predicates and attributes should be a single proof or not.
I don’t think we should add AnonCreds 1.0 functionality to make predicates and revealed attributes come from the same credential — although I agree it would have been the right thing to do in the first place.
What do you think about my above suggestion to make it a verifier side only verification step? I think currently you should assume predicates and attributes come from different credentials, while the proof can assert that actually attributes and predicates came from the same credential. I think that is very useful information, and something I see as a weakness in the current AnonCreds API.
There's no way to request a cred where name is revealed and age > 15. You either need to request both to be revealed, or request them as attribute and predicate separately and then look into the proofs structure yourself to see whether they are submitted as one or two proofs.
TimoGlastra
commented
1 month ago I made a PR in Credo based on your suggestions @andrewwhitehead and that seems to work well! Thanks for the pointer.
There's no way to request a cred where name is revealed and age > 15. You either need to request both to be revealed, or request them as attribute and predicate separately and then look into the proofs structure yourself to see whether they are submitted as one or two proofs.
Still, I still think it's quite a big gap in the spec / implementation and people are probably assuming that when you request attributes and predicate from the same cred def that they are from the same credential in the presentation.
Cross-posting this from PR https://github.com/openwallet-foundation/agent-framework-javascript/pull/1629.
The proofs created between AnonCreds RS and Indy SDK are different? It seems with Indy when revealing
nameand a predicate onagefrom the same cred it will create one proof, while now with AnonCreds it creates two proofs. I don't know if this has any security implications?See the below structure for the difference:
CC @andrewwhitehead @swcurran