hyperledger / anoncreds-rs

anoncreds-rs
https://wiki.hyperledger.org/display/anoncreds
Apache License 2.0
74 stars 55 forks source link

Revoking one credential of many, of the same type, creates failed proofs for all credentials. #336

Closed jamshale closed 6 months ago

jamshale commented 6 months ago

Related to https://github.com/hyperledger/aries-cloudagent-python/issues/2934

Recently it was discovered that the implementation of anoncreds in aca-py has an issue where a holder receives multiple credentials of the same type (cred-def) and one of them gets revoked, it causes every proof to fail, regardless of with credential is used.

If gone over the code in aca-py, and compared the anoncreds and indycredx implementations and haven't been able to detect anything wrong so far.

I believe the problem could be related to anoncreds-rs, when generating the revocation states.

Particularly, with this block, calling the anoncreds-rs library.

            rev_state = await asyncio.get_event_loop().run_in_executor(
                None,
                CredentialRevocationState.create,
                rev_reg_def,
                rev_list,
                int(cred_rev_id),
                tails_file_path,
            )

It seems the state may be incorrect, and causing the proofs to fail. Even though I'm fairly certain the arguments are correct.

swcurran commented 6 months ago

@andrewwhitehead — help! @jamshale — is there a concise example for this issue? I’ve not read the issue — I’m hoping there is something there.

Definitely an odd issue, as I don’t understand how the holder having multiple instances of a credential can affect the verification of one of them. AnonCreds Rust should have no knowledge of anything except the credential being verified.

It is possible that when one of multiple credentials is picked for presentation, the index of the credential is left pointing to a different credential?

jamshale commented 6 months ago

This is only known to be an issue in aca-py. There's a chance it is an aca-py problem. However, I have looked in detail at the objects and the anoncred-rs calls, and everything seems to be correct. If it could be confirmed with credo-ts, it would help isolate the issue.

It was first noticed with the acapy demo. Only an issue when the holder (alice) is anoncreds. The issuer/verifier (faber) doesn't matter if it's anoncreds or not.

Faber sends multiple credentials of the same cred-def, rev_reg to alice. All proofs pass. Then faber revokes any credential (2nd one issued). Now, all proofs will fail, regardless of the revocation status of the slected credential.

I thought maybe it was an issue with the demo, or which credential was being selected, but it's not. I was able to confirm this manually.

jamshale commented 6 months ago

I think this has to be a problem with the revocation list as an argument to anoncreds-rs. I did a test with another agent which has only one non-revoked credential, but from a cred-def/rev_reg which has revoked credentials, and it fails the proof :/

So revoking any credentials is effecting all the holders. The only thing they have in common, is they are sending the same revocation list object like [0,1,0,...], to anoncreds-rs to get the revocation state. I think this must be where the problem is happening but I still don't know if aca-py is doing something incorrect or there is an issue with anoncreds-rs

TimoGlastra commented 6 months ago

Would be great if you could create a minimal reproducable example using just AnonCreds RS (e.g. the Python wrapper). Then we can easily debug the issue, as well as add a test afterwards to make sure it doesn't happen again in the future.

TimoGlastra commented 6 months ago

I created a repro with the TS wrapper, but wasn't able to reproduce the issue: https://github.com/TimoGlastra/anoncreds-rs-revocation-bug. This is what I'm testing:

const credential1 = issueCredential(1);
const credential2 = issueCredential(2);

// Both should succeed
assert.equal(verifyCredential(credential1), true);
assert.equal(verifyCredential(credential2), true);

// Revoke a credential
revokeCredential(1);

// This one should fail now, as it's revoked
assert.equal(verifyCredential(credential1), false);

// This one should still succeed
assert.equal(verifyCredential(credential2), true);
jamshale commented 6 months ago

@TimoGlastra Ok. Thanks for testing it with the typescript wrapper. It seems the problem is likely with the aca-py implementation. I will do a minimum example with the python wrapper and see if it helps me figure out what is going wrong.

jamshale commented 6 months ago

I replicated the minimum example with the python wrapper and it works the same as the typescript wrapper.

However, One thing I have noticed, is the current aca-py implementation is sending in a revocation list where when the first credential is revoked, it is at index 0, like [1,0,0,0, ....]. But, If I try to issue a credential at index 0 with the typescript and python minimum examples I get a Invalid state: Revocation index is outside of valid range error. And when I revoke credential 1 the list is always[0,1,0,0, ...]. Index 0 is never revoked in the list.

I think this could potentially be causing the issue in aca-py. Seems like the indexing is off. But i'm still not sure if this was by design or not.

Screenshot from 2024-05-10 14-38-34

swcurran commented 6 months ago

It's by design. @andrewwhitehead can explain - I'm not sure why. But it goes from 1 up, not 0.

andrewwhitehead commented 6 months ago

That was just the convention in Indy, the first issued credential is always index 1.

when I revoke credential 1 the list is always [0,1,0,0, ...]. Index 0 is never revoked in the list.

That does seem incorrect. It should be [1,0,0,0, ...] in that case.

jamshale commented 6 months ago

That was just the convention in Indy, the first issued credential is always index 1.

when I revoke credential 1 the list is always [0,1,0,0, ...]. Index 0 is never revoked in the list.

That does seem incorrect. It should be [1,0,0,0, ...] in that case.

I'm a bit confused by this comment.

I've confirmed that I can fix the problem in aca-py by prepending a 0 to the front of the revocation list. This can probably be closed.

However, I'm not sure if this should be documented better or if it was just a mistake when it was implemented in aca-py. I'm going to change aca-py to revoke credentials starting at index 1.

swcurran commented 6 months ago

I thought there was a fix for this — prepending the 0 because of the off-by-one error? Not sure how to interpret the “not planned” closing.

jamshale commented 6 months ago

There's nothing to do with anoncreds-rs. I ended up finding out that the aca-py implementation was intentionally omitting the first index ex [0,1,1,0] --> [1,1,0] and sending that reduced list to anoncreds-rs. All it needed was to include the first index from the ledger result for legacy_indy implementation.

image

Pretty confusing and easy to do if you didn't know anoncreds-rs expected the first index to be included, even though it's always 0.

swcurran commented 6 months ago

Ah — got it. I was thinking this was in ACA-Py. Thanks for the clarification.