Remove DID validation from Issue Credential Request processing

[swcurran]

There is an issue with this currently failing integration test: ./run_bdd -t @T001-RFC0454-DID-PEER. Resulting error is below.

The issue is:

The "prover_did" coming from the Holder need not be a DID at all, and in fact, per the AnonCreds v1.0 specification what used to "prover_did" is now simply a string that is used for generating entropy (called "entropy"). In the original AnonCreds (Ursa) implementation, it was called prover_did, but there is no cryptographic checking of the validity of the DID, no use of the public key associated with the DID, and there is no reason for it, in fact, to be a DID. It definitely need not be an Indy DID.

Thus, the verification that it is a DID must be removed. A string check could be made, I guess, if that makes sense.

Suggest the description be something like: metadata={"description": "Prover DID/Random String/UUID", "example": "<insert UUID>"},

INFO:aiohttp.access:172.17.0.1 [16/Jan/2024:19:46:10 +0000] "POST /webhooks/topic/issue_credential_v2_0/ HTTP/1.1" 200 149 "-" "Python/3.9 aiohttp/3.9.1"
Bob.agent  | Credential: state = request-sent, cred_ex_id = 2b49f4c5-5a47-4584-8d86-e69a8f2c3856
Faber.agent | 2024-01-16 19:46:10,142 aries_cloudagent.messaging.models.base ERROR V20CredRequest message validation error:
Faber.agent | Traceback (most recent call last):
Faber.agent |   File "/home/aries/aries_cloudagent/messaging/models/base.py", line 200, in deserialize
Faber.agent |     schema.loads(obj) if isinstance(obj, str) else schema.load(obj),
Faber.agent |   File "/home/aries/.local/lib/python3.9/site-packages/marshmallow/schema.py", line 723, in load
Faber.agent |     return self._do_load(
Faber.agent |   File "/home/aries/.local/lib/python3.9/site-packages/marshmallow/schema.py", line 910, in _do_load
Faber.agent |     raise exc
Faber.agent | marshmallow.exceptions.ValidationError: {'prover_did': ['Value did:peer:2.Vz6MkvLuJ8sKw4oKQr4wcsjgSKJejawwC1ofE1cNbFz99fByk.SeyJpZCI6IiNkaWRjb21tLTAiLCJ0IjoiZGlkLWNvbW11bmljYXRpb24iLCJwcmlvcml0eSI6MCwicmVjaXBpZW50S2V5cyI6WyIja2V5LTEiXSwiciI6W10sInMiOiJodHRwOi8vMTcyLjE3LjAuMTo4MDMwIn0 is not an indy decentralized identifier (DID)']}
Faber.agent | 2024-01-16 19:46:10,143 aries_cloudagent.core.dispatcher ERROR Message parsing failed: Error deserializing message: V20CredRequest schema validation failed, sending problem report
INFO:aiohttp.access:172.17.0.1 [16/Jan/2024:19:46:10 +0000] "POST /webhooks/topic/problem_report/ HTTP/1.1" 200 149 "-" "Python/3.9 aiohttp/3.9.1"
Bob.agent  | Received problem report: Error deserializing message: V20CredRequest schema validation failed
      Assertion Failed: FAILED SUB-STEP: Then "Bob" has the credential issued
      Substep info: Traceback (most recent call last):
        File "/home/aries/.local/lib/python3.9/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
        File "/home/aries/.local/lib/python3.9/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features/steps/0453-issue-credential.py", line 341, in step_impl
          assert False
      AssertionError
      
      Traceback (of failed substep):
        File "/home/aries/.local/lib/python3.9/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
        File "/home/aries/.local/lib/python3.9/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features/steps/0453-issue-credential.py", line 341, in step_impl
          assert False
      

[swcurran]

The error message is from here in the DidIndy method. In that case, perhaps the message and function should remain the same -- but should be removed in the place it is being checked.

[swcurran]

Error is here where the validation is "validate=INDY_DID_VALIDATE" and it shouldn't be -- it should be any string value.

[ianco]

The anoncreds-rs library allows use of either a prover did or an "entropy" value. If you provide a prover did then it will validate it. If you provide "entropy" instead (pass our "prover did" as the "entropy") then we get a failure later on because we don't have "entropy" in out marshmallow models.

I think we'll need to include both prover did and entropy in our models.

[swcurran]

We can provide both, but there is no need to verify that a “prover_did” is a DID. There is no requirement it be a DID — just bad naming by an implementer.

[swcurran]

If we do want to make a DID permissible, it needs to support any DID.

[ianco]

For anoncreds, it makes sense to allow the holder to include either the prover_did or entropy, and I agree the validation for prover_did should allow any did.

All of the libraries (anoncreds-rs, credx and indy-sdk) allow prover_did and all do some kind of validation (I haven't checked in detail what is/isn't allowed for each of these. credx and indy-sdk don't support entropy, so if we allow entropy as one of the inputs then the exchange will break if one of the agents is running credx or indy-sdk (or an older version of aca-py).

I think for now we need to make sure that the anoncreds-rs prover-did validation is updated to allow our new did formats (I can create an issue on the anoncreds-rs repo for this).

I don't think is makes sense yet to start using the entropy field until we're confident that most agents are updates to use anoncreds-rs.

[swcurran]

We need to support peer DIDs in general, which is where the issue arose. peer DIDs need to work with all libraries, so we need to be sure that the validation doesn’t break anything.

Neither credx or indy-sdk support entropy.

As long as the validation doesn’t affect any of the AnonCreds implementations, we’re good. I do think it is fine to get rid of any DID validation, since it is just a mistake that it is called that.

[ianco]

Created issue hyperledger/anoncreds-rs#307

[TimoGlastra]

To remain backwards compatible the AnonCreds spec describes that entropy MAY only be used if a qualified identifier is used (then we're sure the implementation supports entropy as qualified identifiers came with the new anoncreds spec).

If unqualified identifiers are used, prover_did should be used. I think indy_sdk did not verify the value of prover_did at all -- and so it's odd we do it in AnonCreds RS. That should be removed, yes

[ianco]

Should be fixed now