business-partner-agent icon indicating copy to clipboard operation
business-partner-agent copied to clipboard

Cannot sattistfy request with String value == restriction.

Open Jsyro opened this issue 3 years ago • 13 comments

This proof request

{
   "id":"88719e13-4129-41a4-8434-f08e7a1e616b",
   "partnerId":"19b0d483-905b-46a3-ad19-938de8b5dd0f",
   "exchangeVersion":"V1",
   "state":"request_received",
   "role":"prover",
   "updatedAt":1630441579762,
   "stateToTimestamp":{
      "request_received":1630441579761
   }"typeLabel":"BC MInes Act Permit",
   "proofRequest":{
      "name":"BC MInes Act Permit",
      "version":"1.0",
      "nonce":"113436436443681711055586",
      "requestedAttributes":{
         "QVdfoxctydwAvScxGkUyor:2:mines_act-permit:1.0":{
            "names":[
               "Permitee",
               "Permit-number"
            ]"nonRevoked":{
               "from":1630441579,
               "to":1630441579,
               "set":true
            }"restrictions":[
               {
                  "schema_id":"QVdfoxctydwAvScxGkUyor:2:mines_act-permit:1.0",
                  "issuer_did":"QTNLwTnFaqtVTgjj2DZuC9",
                  "attr::Permit-number::value":"P-100"
               }
            ]
         }
      }"requestedPredicates":{
         
      }
   }"canBeFullfilled":false
}

Is saying it cannot be fulfilled, here is the matching credential in my wallet that should satisfy it.

{
   "id":"d0441610-4782-487e-a95f-079526df3778",
   "issuedAt":1630441206651,
   "state":"credential_acked",
   "isPublic":false,
   "issuer":"gov",
   "schemaId":"QVdfoxctydwAvScxGkUyor:2:mines_act-permit:1.0",
   "credentialDefinitionId":"QTNLwTnFaqtVTgjj2DZuC9:3:CL:1902:test",
   "connectionId":"10026690-25fa-4c56-94c3-a8eb7e4b4eff",
   "revocable":false,
   "exchangeVersion":"V1",
   "typeLabel":"mines_act-permit",
   "credentialData":{
      "Permitee":"Big Mining Inc",
      "Permit-number":"P-100"
   }"label":""
}

if i manually enable the button and click it. I get cannot get credentialInfo of undefined, PresentationExList.vue line 283.

This is the JSON of the proof request template that made the presentaitonRequest.

{
  "id": "4ac1c81f-33b7-47e5-966e-6cb99272b94a",
  "createdAt": "2021-08-31T20:26:19.380+0000",
  "name": "BC MInes Act Permit",
  "attributeGroups": [
    {
      "schemaId": "ecca792a-ef4f-403e-a37e-34e6bbc31841",
      "attributes": [
        {
          "name": "Permitee",
          "conditions": []
        },
        {
          "name": "Permit-number",
          "conditions": [
            {
              "operator": "==",
              "value": "P-100"
            }
          ]
        }
      ],
      "nonRevoked": true,
      "schemaLevelRestrictions": {
        "issuerDid": "QTNLwTnFaqtVTgjj2DZuC9"
      }
    }
  ]
}

Possibly related is that i cannot inspect the details of the proof template (picture below is of the verifier) same screen as the above json blob.

image

Jsyro avatar Aug 31 '21 20:08 Jsyro

I tried different variations of restrictions and Cred Def id restriction also failed.

Schema id is redundant as it's added automatically anyways.

More testing results will be posted later.

Jsyro avatar Aug 31 '21 21:08 Jsyro

Thanks for reporting. Could you provide the raw data from the presentation exchange dialog? If there's no matching credential in this data it would be helpful if you'd try the matching credentials endpoint of the BPA API directly.

domwoe avatar Sep 01 '21 06:09 domwoe

I reconstructed the above and aca-py returns an empty array for the wallet matches in this case

etschelp avatar Sep 01 '21 08:09 etschelp

I played around some more and it seems that upper case characters in a schema attribute will cause this none match. The following example works:

        "proofRequest": {
            "name": "Permit dash underscore",
            "version": "1.0",
            "nonce": "1034732971840382951225191",
            "requestedAttributes": {
                "EraYCDJUPsChbkw7S1vV96:2:mines_act-permit:3.0": {
                    "names": [
                        "permitee",
                        "permit-number"
                    ],
                    "nonRevoked": {
                        "from": 1630486421,
                        "to": 1630486421,
                        "set": true
                    },
                    "restrictions": [
                        {
                            "schema_id": "EraYCDJUPsChbkw7S1vV96:2:mines_act-permit:3.0",
                            "attr::permit-number::value": "p-100"
                        }
                    ]
                }
            },
            "requestedPredicates": {}
        }

I guess the easiest solution to this is to sanitize all attribute names when we create the schema. Meaning allowing only a-z,0-9,_-

etschelp avatar Sep 01 '21 09:09 etschelp

Looking at https://github.com/hyperledger/indy-sdk/blob/113b79cd64a238130d20e19b972326f72047c550/libindy/src/api/anoncreds.rs#L1529 it should not matter.

It might be a bug in ACA-Py. The normalize method here should probably include making the names lowercase.

domwoe avatar Sep 01 '21 15:09 domwoe

I think validation and attribute names should follow the spec, if the spec allows it upper case attributes should not be normalized. This looks like there is some normalization going on during request matching and then the wallet value is not found because permit != Permit

etschelp avatar Sep 01 '21 15:09 etschelp

I was never certain whether the normalization was happening in ACA-Py (seems wrong), or because the Indy-SDK required it (also seems wrong). Unfortunately, the developer that knew about this (@sklump) is not with the project anymore and so not available to answer. @ianco or @andrewwhitehead -- any ideas?

swcurran avatar Sep 01 '21 19:09 swcurran

I did a small test.

  1. I created a schema with attribute MyAttrib and issued a credential based on this.
  2. I tried to fetch the credential with ACA-Py's GET: /credentials endpoint using a WQL query
  • Query { "attr::MyAttrib::marker": "1"} provides an empty result
  • Query { "attr::myattrib::marker": "1"} provides the credential

Based on this I think we should change ACA-Py's normalize function to include making the attributes lowercase.

domwoe avatar Sep 02 '21 07:09 domwoe

@domwoe I see some issues with that. 1. The normalize function also gets called when a credential gets loaded, which seems odd. 2. I think that this does not fix the root cause. According to your description the root issue seems to be in the wallet query. 3. Any change needs to happen in a backwards compatible way, and then you need indeed normalize when loading credentials.

etschelp avatar Sep 02 '21 07:09 etschelp

I got confused...

We have different behavior depending on usage of indy-sdk or askar/credx:

Indy SDK

  • ACA-Py just delegates everything to indy-sdk
  • When indy-sdk creates tags to find a credential it does the following:
pub fn attr_common_view(attr: &str) -> String {
    attr.replace(" ", "").to_lowercase()
}
  • However indy-sdk does not normalize the attribute names in the WQL query or in the restrictions. We might want to that in ACA-Py

Result: attr::{attribute}::marker and attr::{attribute}::value restrictions do not work for attributes containing upper case letters or spaces.

Askar/Credx

  • ACA-Py normalizes attributes in tags by stripping spaces
  • When searching a matching credential restrictions are just added to the tag_filter without normalizing

Result: attr::{attribute}::marker and attr::{attribute}::value restrictions do not work for attributes containing spaces.

Another thing we noticed. The get_credentials function is not implemented in askar/credx. Hence, ACA-Py's GET: /credentials endpoint will always return null in this case.

Suggestion

If normalization is necessary, normalize the restrictions/WQL query accoding to the implemented normalization function which differs between indy-sdk and askar.

domwoe avatar Sep 02 '21 10:09 domwoe

Thanks for pushing on this. @andrewwhitehead -- FYI on this.

What is the right handling at the storage level, lower(), strip_spaces(), nothing, both, something else?

Presumably, the corresponding handling should be at the lowest level possible in using the attributes. Ideally at the WQL level, but if we don't want to change the indy-sdk, then at the ACA-Py level, at least for when Indy is the backend.

swcurran avatar Sep 02 '21 13:09 swcurran

I recommend making the change in aca-py, so we can ensure the behaviour is consistent across the underlying sdk's. (Also it is a challenge to release a new indy-sdk version.)

ianco avatar Sep 02 '21 14:09 ianco

Yep, sounds like we need to do more processing on the WQL to normalize the attribute names. For the record Indy uses this method to normalize them (not sure why it's not applied to the WQL): https://github.com/hyperledger/indy-sdk/blob/dbd89cf94a73e7a62611c4150a874c38b810ff8d/libindy/src/services/anoncreds/helpers.rs#L20

andrewwhitehead avatar Sep 02 '21 18:09 andrewwhitehead