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
121 stars 81 forks source link

Simplify anoncreds trait API `prover_create_credential_req` #950

Open Patrik-Stas opened 11 months ago

Patrik-Stas commented 11 months ago

Quoting George over here https://github.com/hyperledger/aries-vcx/pull/946#discussion_r1301621028

this is something that has bugged me before about the holder API and older versions of credx/indy-sdk in general... i believe this gets attached in the credential request for it to act as a form of nonce for the issuer to sign over... however i don't believe it technically HAS to be a DID.

In the past when i looked into this, deeper ursa code for issuer logic just refers to this value as a string called "prover id", rather than "prover did". unfortunately i believe libs like credx and vdrtools will enforce that this value MUST be a DID (when really it could be any arbitrary DID).

anoncreds-rs does a better job IMO by allowing an "entropy" string to be used instead when constructing a cred req; https://github.com/hyperledger/anoncreds-rs/blob/main/src/services/prover.rs#L107C34-L107C34

The anoncreds API for Prover (creating proof request) looks like this

async fn prover_create_credential_req(
        &self,
        prover_did: &str,
        cred_offer_json: &str,
        cred_def_json: &str,
        master_secret_id: &str,
    ) -> VcxCoreResult<(String, String)>;

According to Geroge's quote, we could perhaps remove the argument from the interface and generate a random DID to satisfy what credx requires under the hood? Well, it needs to be looked at.

swcurran commented 11 months ago

This has been a debate in the AnonCreds specification working group as well. We came to the same conclusion — the value being a “DID” makes no sense (no such thing in the context of AnonCreds), nor even an identifier. I think the use of DID was to suggest that people use a DIDComm DID, which would (usually) be unique per connection. The holder shouldn’t use the same identifier for all issuers as that would be correlating.

In the end we renamed it “entropy” in the spec and just said it was a random number.

However, some question whether the value should be provided by the holder or the issuer. I’m still not clear on what the final thinking is on that from the experts. For now, we have left it as provided by the holder, and is a random value.

Patrik-Stas commented 11 months ago

Awesome @swcurran that sheds some light on this, thank you!

@gmulhearn I suppose we can go with the rename into entropy as well, or perhaps the Holder caller of this doesn't even need to care, and we'd internally randomly generate some random data on their behalf?

gmulhearn commented 11 months ago

@Patrik-Stas this all matches my understanding! I think we could generate on behalf of the user, not sure I can think of a use case where a user would care about what their entropy specifically is