Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

Cannot sattistfy request with String value == restriction. #591

Open
Jsyro opened this issue Aug 31, 2021 · 13 comments
Open

Cannot sattistfy request with String value == restriction. #591

Jsyro opened this issue Aug 31, 2021 · 13 comments

Comments

@Jsyro
Copy link
Contributor

Jsyro commented Aug 31, 2021

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

Jsyro commented Aug 31, 2021

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 Jsyro changed the title Cannot sattistfy request with IssuerDid restriction. Cannot sattistfy request with String value == restriction. Aug 31, 2021
@domwoe
Copy link

domwoe commented Sep 1, 2021

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.

@etschelp
Copy link
Contributor

etschelp commented Sep 1, 2021

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

@etschelp
Copy link
Contributor

etschelp commented Sep 1, 2021

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,_-

@domwoe
Copy link

domwoe commented Sep 1, 2021

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.

@etschelp
Copy link
Contributor

etschelp commented Sep 1, 2021

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

@swcurran
Copy link

swcurran commented Sep 1, 2021

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?

@domwoe
Copy link

domwoe commented Sep 2, 2021

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.

@etschelp
Copy link
Contributor

etschelp commented Sep 2, 2021

@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.

@domwoe
Copy link

domwoe commented Sep 2, 2021

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.

@swcurran
Copy link

swcurran commented Sep 2, 2021

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.

@ianco
Copy link
Contributor

ianco commented Sep 2, 2021

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.)

@andrewwhitehead
Copy link

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

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

No branches or pull requests

6 participants