Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Determine what to do with the prover_did property #107

Closed
TimoGlastra opened this issue Nov 25, 2022 · 11 comments
Closed

Determine what to do with the prover_did property #107

TimoGlastra opened this issue Nov 25, 2022 · 11 comments

Comments

@TimoGlastra
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

swcurran commented Jan 3, 2023

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
Copy link
Contributor

whalelephant commented Jan 9, 2023

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
Copy link
Member

swcurran commented Jan 9, 2023

@whalelephant -- how is rev_idx created?

@swcurran
Copy link
Member

swcurran commented Jan 9, 2023

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

@TimoGlastra
Copy link
Member Author

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

  • Deprecate the prover_did property (but not remove it)
  • Only the legacy unqualified indy (so a request where schema id and cred def id are unqualified) are allowed to use the prover_did property
  • You're allowed to generate a value on your own for the prover_did (or whathever it is called internally) as long as it provides some entropy.
  • Even if the value of prover_did is provided you can generate your own value (Not sure whether this won't break backwards compatibilty?)
  • You can include it if you're using legacy identifiers and interacting with a client that isn't using the updated anoncres spec

Is that correct?

cc @blu3beri

@swcurran
Copy link
Member

swcurran commented Feb 5, 2023

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
Copy link
Member

swcurran commented Feb 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants