hyperledger / aries-vcx

aries-vcx is set of crates to work with DIDs, DID Documents, DIDComm, Verifiable Credentials and Hyperledger Aries.
https://didcomm.org
Apache License 2.0
122 stars 81 forks source link

Cannot create a revocation proof with two credentials with the same revocation registry ID #834

Open gmulhearn-anonyome opened 1 year ago

gmulhearn-anonyome commented 1 year ago

Problem

When using the Prover handler to create a presentation for a verifier, lots of calls are made to assemble the data that is ultimately passed into the BaseAnonCreds::prover_create_proof method. generate_indy_proof does most of this data construction.

Part of the data construction for data that prover_create_proof needs, is the revoc_states_json. Where generate_indy_proof assembles revoc_states_json as a JSON string of:

Map<String(rev_reg_id), Map<String(timestamp), String(anoncreds_revocation_state_json)>>

So it's "revocation_state by timestamp by the revocation_registry_id".

The problem with this is that if we want to create a proof (non-revokable proof), for say "reft-A & reft-B", and we use "cred-1" for "reft-A" and "cred-2" for "reft-B", and "cred1" & "cred2" are from the same revocation_registry_id "rev-reg-1", then only 1 revocation state is created for "rev-reg-1" (since revocation_registry_id is the unique key in the revoc_states map). And therefore, prover_create_proof only creates a proof of the revocation state for 1 of the credentials..

Potential Solution

Part of the problem is that Anoncreds::prover_create_proof does not specify in its documentation what it expects the revoc_states_json String to be shaped as. But drilling down into the Indy/vdrtools implementation of prover_create_proof, they appear to have solved this problem by defining revoc_states_json as follows:

    /// rev_states_json: all revocation states participating in the proof request
    ///     {
    ///         "rev_reg_def1_id or credential_1_id": {
    ///             "timestamp1": <rev_state1>,
    ///             "timestamp2": <rev_state2>,
    ///         },
    ///         "rev_reg_def2_id or credential_1_id"": {
    ///             "timestamp3": <rev_state3>
    ///         },
    ///         "rev_reg_def3_id or credential_1_id"": {
    ///             "timestamp4": <rev_state4>
    ///         },
    ///     }

The big difference is that this structure's map uses String(rev_reg_id OR cred_id) as the first key, rather than JUST rev_reg_id. Using cred_id as the key fixes the use-case I described above, and is in-general less restrictive I believe.

So we could potentially solve this by:

Other Problems

I believe that build_rev_states_json does not support having different timestamps under the same rev_reg_id either. E.g. if I want to prove "reft-A" from "rev-reg-1" at timestamp "1", I cannot also prove "reft-B" from "rev-reg-1" at timestamp "2". There is a note in the code about this too;

// TODO: proover should be able to create multiple states of same revocation policy for different timestamps
// see ticket IS-1108

So this could be fixed as well in this ticket

I think the loop of build_rev_states_json could use a general clean up too.

gmulhearn-anonyome commented 1 year ago

Happy to take on this issue if we agree it's a problem