hyperledger / anoncreds-spec

The specification for AnonCreds verifiable credential exchange.
https://hyperledger.github.io/anoncreds-spec/
Apache License 2.0
45 stars 24 forks source link

Determine what to do with the prover_did property #107

Closed TimoGlastra closed 1 year ago

TimoGlastra commented 1 year ago

It is not required to use AnonCreds with dids. And as I understand it the prover_did doesn't actually have to be a did. It's used internally by ursa as a context string. So I think it just needs to be unique.

@whalelephant you know more about the exact working of this :)

My suggestion would be to rename it to something else that represents a unique identifier, but doesn't necessarily have to be a did. One downside of changing this is that's it is sent to the issuer, meaning it will break backwards compability.

We could also deprecate but allow it, and add a new property that doesn't have to be a did. this way we keep backwards compat. Or we keep the property but allow it to not be a did, but this will break e.g. ACA-Py that currently vaildates whether it is a did.

Thoughts?

whalelephant commented 1 year ago

@TimoGlastra

IIUC, you are right that it does not need to be a DID in Ursa and that the format is actually not checked.

I understand that the prover_did is in the credential request that the indy-sdk builds. Just a note: this field is not included in the cryptographic material in the request (which only requires blindedSecret + blindedSecretCorrectnessProof), i.e. this is for the issuer to identify the prover sending the request and the prover's revocation index

In terms of backwards compatibility, is there something ACA-Py does with this field other than validating the format?

swcurran commented 1 year ago

I just checked in the ACA-Py Alice-Faber demo and the field is populated by the holder's DID in its connection with the issuer -- which makes no sense at all. The DIDComm DIDs have nothing to do with the issuance of an AnonCreds verifiable credential. I suspect that when the code was developed, the ACA-Py developer did not know what AnonCreds meant to be put in the field, so they used the only DID the holder had in context. And since it didn't break anything, it was ignored.

In searching ACA-Py for prover_did, its referenced in the indy credential request model, and used (badly) in tests. In some of the tests, it is populated with the issuer's DID. In the tests it looks like the developer assumed it would be an Indy DID (did:sov), but that can't be the case, as in the general case, the holder does not have a ledger-based (public) DID.

My recommendation is to try to do a bit of archeology into the indy-sdk AnonCreds (and perhaps anoncreds-rs) code and see if the purpose of the field can be determined. Perhaps it is meant as a unique identifier for the holder that the issuer can hold on to -- but it has lots of such values to use.

My bet is that it is never used in AnonCreds.

@TimoGlastra -- if I'm right that it is never used in AnonCreds, I suggest we flag it to be deprecated and plan on dropping it vs. renaming it. As you note, controller (business) code above the Aries Framework may be using it.

swcurran commented 1 year ago

Tried to do a little digging into the Indy project JIRA board and found this issue: IS-1224: The prover create_cred_request function requires a "prover_did" that (likely) does not make sense. I was hoping that it would have something useful for us...

Sadly, I was the submitter of the report (in March 2019 no less) and the issue did not receive a response. There are a couple of links to some code, but that's it.

Oh well.

berendsliedrecht commented 1 year ago

Do we have a path for what we want to do with the prover_did? I am trying to clear out the indy-utils inside the anoncreds repo, but quite a big part of it has to do with the prover_did and the validation of it. Do we want to keep it in for now and deprecate it (maybe remove the validation if we deprecate it) or just remove it entirely?

swcurran commented 1 year ago

AFAIK, we don't have a solution on this. We are looking for a report of how the prover_did is used, and hence what we could replace it with. Since it is not checked by the verifier, we think it can be any random string, but we don't know that yet. We need some code tracing to look at how it is used in the credential creation process to understand more about what it is and how/if we can/should replace it. And we at least want some useful documentation.

whalelephant commented 1 year ago

In libursa the prover_did string is used to generate cred_context which is part of the primary credential signature and non-revocation credential. In both types of credentials, cred_context is used in the signing and also returned as plain text (encoded as m_2) as part of the signature. There is no format validation done in ursa if this is a DID or not, it is turned into bytes (for non-revocation case, concat w/ rev_idx) and then hashed. The verification process verifies a masked version of this like all other parts of the credential.

Since the prover_did is a part of the CredentialRequest provided by the prover. It seems to be a self-attested value that adds entropy. I do not think it should be in the CredentialRequest, but perhaps just like rev_idx, a value that the issuer adds from (maybe) the did comm with the prover to retrieve their rev_idx.

swcurran commented 1 year ago

@whalelephant -- how is rev_idx created?

swcurran commented 1 year ago

@swcurran to take a pass at updating the spec. with information about this item.

TimoGlastra commented 1 year ago

To reiterate what was discussed (at least how I understood it):

Is that correct?

cc @blu3beri

swcurran commented 1 year ago

After reviewing the code in anoncreds.rs and Ursa (specifically, this module), I think we should keep the prover_did as a required, as I think it used by the holder to ensure that the issuer provides sufficient entropy. It's source can be made more generic -- e.g. it can be a random string -- but I think it should be produced by the holder. I don't think the issuer should be responsible for generating it, as they could reduce the entropy, which could (in theory) weaken the credential.

The Holder is able to verify that the value of m2, by hashing the combined prover_did and rev_idx, both of which the holder knows. If the issuer generates the prover_did the holder cannot do that.

swcurran commented 1 year ago

Discussed at the 2023.02.06 meeting -- update the PR to change the item to entropy and that it be required from the holder.

@blu3beri