Requesting similar attributes from different credentials fails

[Iskander508]

After including this change: #640
when being requested (as a holder/prover) attributes from 2 different credentials, that happen to use the same identifier, an assertion failure is reported. e.g. consider a proof request like this:

{
  "name": "Example proof",
  "version": "1.0",
  "requested_attributes": {
    "0": {
      "names": [ "name", "issued_on"],
      "restrictions": [
        { "schema_name": "identity card" }
      ]
    },
    "1": {
      "names": [ "address", "issued_on" ],
      "restrictions": [
        { "schema_name": "residence certificate" }
      ]
    }
  },
  "requested_predicates": {},
}

This proof request will fail on the assertion with a message: The proof request contains duplicate items: issued_on. Since the 2 credentials can be completely unrelated and the collision is then purely coincidental, this should be allowed.
I think the assertion should ONLY check whether a predicate is matching with a requested attribute name (and maybe even only if both use the same set of restrictions, otherwise they could again be meant to come from different credentials)

[TimoGlastra]

Thanks for opening this issue @Iskander508! I wasn't aware you could request multiple attributes with the same name from different credentials.

I agree we should loosen the check. Are you willing to open a PR for this?

[Iskander508]

@TimoGlastra I'm not very familiar with the internal codebase, but I'm wondering: Is it actually correct that this kind of check is done in this aries library? Wouldn't it make more sense to do it inside indy-sdk?

[TimoGlastra]

If we don't do the check indy-sdk will eventually blow up with an error that looks something like: CommonInvalidStructure. Which is 100% useless. That's why we tried to add some checks to provide more helpful errors.

Understand you're not very familiar with the internal codebase. I'll add it to my list, but can't guarantee I can work on it in the near feature

[Iskander508]

OK, now I understand why we have it, and I think I could give it a try to implement a better solution. In order to make it right, the logic might be quite complex around the restrictions, though.
For example for this case it is very straightforward, this should not be allowed as we're obviously requesting age raw value as well as predicate:

{
  "name": "Simple example",
  "version": "1.0",
  "requested_attributes": {
    "age": {
      "names": ["name", "age"],
      "restrictions": [{ "cred_def_id": "cred1" }]
    },
  },
  "requested_predicates": {
    "ageOver18": {
      "name": "age",
      "p_type": ">=",
      "p_value": 18,
      "restrictions": [{ "cred_def_id": "cred1" }]
    },
  },
}

However for this case it is not so obvious whether it should be allowed or not:

{
  "name": "Complicated example",
  "version": "1.0",
  "requested_attributes": {
    "age": {
      "names": ["name", "age"],
      "restrictions": [{ "cred_def_id": "cred1" }]
    },
  },
  "requested_predicates": {
    "ageOver18": {
      "name": "age",
      "p_type": ">=",
      "p_value": 18,
      "restrictions": [{ "cred_def_id": "cred1" }, { "cred_def_id": "cred2" }] // this means either using cred1 or cred2
    },
  },
}

Trying to create a proof in this case could potentially fail if we would use the same credential for both the attribute-group as well as the predicate. But if we have both cred1 and cred2 it is possible to create a valid proof (using the cred2 for the predicate).
There are probably other combinations of restrictions that could run into a similar issue (one using cred_def_id, other using schema_id, etc.).

I understand that this is very theoretical indeed, but my opinion would be that if we are not 100% sure it will not work, then we should allow it (and let it fail later, maybe with a more cryptic error). What would be your opinion about it?

[TimoGlastra]

my opinion would be that if we are not 100% sure it will not work, then we should allow it (and let it fail later, maybe with a more cryptic error). What would be your opinion about it?

Yes I think you're right, we shouldn't put restrictions on the usage of indy credentials in AFJ, so if we can't determine it we should just allow it.

[swcurran]

Would it make sense for AFJ (or other Aries Framework) to do a post-mortem on failed proofs? E.g. rather than doing an up front restriction and blocking the proof, do a failed verification set of checks to provide more insight to the caller as to what might have happened?

[TimoGlastra]

Yes that would make a lot of sense! Good idea @swcurran :)

[Iskander508]

I agree @swcurran. That would be a much cleaner solution. Do you plan to create a PR for it?