openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
412 stars 512 forks source link

[Question] Verifying presentations with the `/vc/ldp/verify` endpoint #2693

Closed PatStLouis closed 8 months ago

PatStLouis commented 9 months ago

The endpoint accepts a "vp" in the body, so I'm assuming it can verify presentations. However, when I provide a vp it returns the following errors:

{
  "vp": {
    "type": [
      "type must include VerifiableCredential"
    ],
    "issuer": [
      "Missing data for required field."
    ],
    "issuanceDate": [
      "Missing data for required field."
    ],
    "credentialSubject": [
      "Missing data for required field."
    ]
  }
}

It looks like its still expecting a vc, is this the intended behavior?

swcurran commented 9 months ago

Can you add the VP you provided? I’m guessing it did have those fields?

swcurran commented 9 months ago

Note that a VP embeds an array of VCs in it, so it is not surprising that is expects a VC. Might not be the issue, but hard to tell without seeing the data.

PatStLouis commented 9 months ago

Here's a presentation. It can be verified with the uni verifier but aca-py returns the previous errors

{
    "@context": [
        "https://www.w3.org/2018/credentials/v1"
    ],
    "type": [
        "VerifiablePresentation"
    ],
    "holder": "did:web:trace.opsec.id:organization:test-20",
    "verifiableCredential": [
        {
            "@context": [
                "https://www.w3.org/2018/credentials/v1"
            ],
            "type": [
                "VerifiableCredential"
            ],
            "issuer": {
                "id": "did:web:trace.opsec.id:organization:test-20"
            },
            "issuanceDate": "2022-05-01T00:00:00Z",
            "credentialSubject": {
                "id": "did:key:123"
            },
            "id": "urn:uuid:cc173c16-aabb-11ee-a29f-0242ac1b0006",
            "proof": {
                "type": "Ed25519Signature2018",
                "verificationMethod": "did:web:trace.opsec.id:organization:test-20#verkey",
                "proofPurpose": "assertionMethod",
                "created": "2024-01-04T04:43:26Z",
                "jws": "eyJhbGciOiAiRWREU0EiLCAiYjY0IjogZmFsc2UsICJjcml0IjogWyJiNjQiXX0..XNFLfGs2kjPp78CyoVZ0_N8rwrWnB60AAtkZ85pvCKpfi0TUMArEkONROCYL638E0VJu9jSY3PVBHOJS2jg9CA"
            }
        }
    ],
    "proof": {
        "type": "Ed25519Signature2018",
        "verificationMethod": "did:web:trace.opsec.id:organization:test-20#verkey",
        "proofPurpose": "assertionMethod",
        "created": "2024-01-04T04:51:59Z",
        "jws": "eyJhbGciOiAiRWREU0EiLCAiYjY0IjogZmFsc2UsICJjcml0IjogWyJiNjQiXX0..bl4CQueD30oTDiV1JdIPCTDXN_M1LfozScPnI6lOcVK1E5V3GuG4x9kmjzjY-XC_f5zXkv-o0636HThcA3QJAg"
    }
}
swcurran commented 9 months ago

Definitely looks like the validation is expecting a VC, not a VP. In the embedded VC, all of the “missing items” are present, so it is not checking that.

A bit weird that the issuer is also presenting the credential, but I suppose anything is permitted.

PatStLouis commented 9 months ago

I just signed a random presentation of a VC I issued to test the feature, I agree this is a bit redundant.

PatStLouis commented 9 months ago

They seem to use the same code so that would make sense: https://github.com/hyperledger/aries-cloudagent-python/blob/7ea0f1983a820aaf42cfef0b9b369f82aedf4be0/aries_cloudagent/vc/routes.py#L108C11-L108C11

swcurran commented 9 months ago

@dbluhm — could you please take a quick look at this? Looks like it was part of this PR #2533 . Seems to be copy/paste error — same logic, but needs to be updated from a VC to a VP.

PatStLouis commented 9 months ago

It's the de-serialization step, it currently uses the VerifiableCredential class for both the vc and vp de-serialization. Looking at the VerifiablePresentation class, it could use some love and add some assertions to bring it up to par with the VerifiableCredential class and de-serialize properly.

I can take this on.

I think down the line it would be great to decouple vc and vp into their own respective routes (adding /vp/ldp/issue+verify and limiting the /vc/ldp/issue+verify for vc exclusively).

I would also not get rid of the jsonld routes until issuance of a vp is enabled since the jsonld route can do that currently.

swcurran commented 9 months ago

Thanks @PatStLouis . Agree on the /vc and /vp routes.

PatStLouis commented 8 months ago

closing since #2710 was merged