-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DIF presentation exchange context and cache document #3093
Add DIF presentation exchange context and cache document #3093
Conversation
Signed-off-by: George Mulhearn <[email protected]>
Signed-off-by: George Mulhearn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
This will need to be updated with main. Not sure why I can't do it with this PR. |
Thanks @jamshale , done |
Quality Gate passedIssues Measures |
Hey @jamshale i get the following error trying to load that: ForbiddenRequest has expired8e22ee14:19021c9bc1d:6e5e7:120e |
Sorry. Here's the stack trace instead.
|
Looks like maybe the |
Hmm, yea it seems that the VP is missing the "proof" field; prover perhaps sent a VP without an LD proof. Is this just one exception that fails? Or do other similar tests fail? Also is this a private test suite (given that the test suite that ran in CI for this PR succeeded)? |
It's a different test suite. It's public via the AATH tests for interoperability https://github.com/hyperledger/aries-agent-test-harness/actions/workflows/test-harness-acapy.yml. These tests are just acapy to acapy. https://allure.vonx.io/allure-docker-service-ui/projects/acapy. A few similar tests failed. I thought they would have been covered by the repo's tests as well. We need to work on consolidating all these tests on PR's. |
If you download the logs from the github action you can see the same exception happened 7 times for the same thing, so I believe that's the only problem. |
just debugging what i've seen so far. it seems that the credential was issued without a Here's the VP that fails, note that there is not {
"@context": [
"https://www.w3.org/2018/credentials/v1",
"https://identity.foundation/presentation-exchange/submission/v1"
],
"type": [
"VerifiablePresentation",
"PresentationSubmission"
],
"verifiableCredential": [
{
"@context": [
"https://www.w3.org/2018/credentials/v1",
"https://w3id.org/security/bbs/v1",
{
"dl": "http://example.com/drivers-license#",
"AATHDriversLicense": "dl:AATHDriversLicense",
"address": "dl:address",
"DL_number": "dl:DL_number",
"expiry": "dl:expiry",
"age": "dl:age"
}
],
"type": [
"VerifiableCredential",
"AATHDriversLicense"
],
"issuer": "did:key:z6MkrMihCwgc54xPRnUTJbrY43Fz3z46esXDMZTdoFWppYdu",
"issuanceDate": "2024-07-15T02:43:03Z",
"credentialSubject": {
"address": "947 this street, Kingston Ontario Canada, K9O 3R5",
"DL_number": "09385029529385",
"expiry": "10/12/2022",
"age": "30"
},
"proof": {
"type": "Ed25519Signature2018",
"proofPurpose": "assertionMethod",
"verificationMethod": "did:key:z6MkrMihCwgc54xPRnUTJbrY43Fz3z46esXDMZTdoFWppYdu#z6MkrMihCwgc54xPRnUTJbrY43Fz3z46esXDMZTdoFWppYdu",
"created": "2024-07-15T02:43:09+00:00",
"jws": "eyJhbGciOiAiRWREU0EiLCAiYjY0IjogZmFsc2UsICJjcml0IjogWyJiNjQiXX0..u2uL_uloYFjB0BkaHWJqZerPyzXrwUaUzcaNIfoQsSjIUiOhdygGCxkM7IRqdqjbDwjbi94RM0Y5ceodjWgyAg"
}
}
],
"presentation_submission": {
"id": "9e62b6d5-088e-4bbe-9923-0e8b53b25478",
"definition_id": "5ab84299-c066-4b34-bcc6-d356aa44c826",
"descriptor_map": [
{
"id": "drivers_license_input_1",
"format": "ldp_vc",
"path": "$.verifiableCredential[0]"
}
]
}
} |
@jamshale I had a look at VPs created in the previous logs of acapy (before my PR), they seem to be missing signatures on their VPs too. My guess so far, is that ACAPY was previously "getting away" with "verifying" VPs that didn't have a |
Ohhh... Ok. thanks for your help. This might be related to another PR that was recently merged then. Probably this PR. Sorry. I was wondering what could have happen with yours. I looked fine to me. |
no worries. just a final note, it may be worth investigating ACApys verification of VPs at AATH. Above i said ACApy was previously "getting away" with it, but i don't think that's true, i think perhaps the AATH was getting away with it. Looking at logs of previous acapy Faber verifiers, many of the presentation exchange objects are logged with having # TODO validate presentation structure here
if "proof" not in presentation:
raise LinkedDataProofException('presentation must contain "proof"') (and then caught and turned into verified=false). So i suppose, if previously the AATH tests were passing (despite verified=false), then might need to be looked at. Edit: i've taken this to a question in the AATH discord: https://discord.com/channels/905194001349627914/941708308459438121/1262534220467667004 |
I'm a little confused here. ACAPy issued the credential a few steps before the verify in this test scenario. Is ACAPy supposed to create the credentialSubject.id when the credential is issued? As for the "proof" in the presentation, Is this only required for JSON LD credentials? Indy credentials work without "proof". |
@nodlesh ignoring special W3C VC types (such as new anoncreds w3c creds), i believe the typical way to bind a W3C VC to a specific holder, is by including The My gut feeling on what is happening here: ACApy is offering/requesting/issuing the W3C VC without a
good question, i think this is a question for the ACApy AATH backchannel. The typical flow i've seen with Aries W3C VC issuance (ignoring the new VC-DI aries attachment format being implemented) is:
This shortcuts the negotiation required for including the credentialSubject.id. ACApy holder APIs support this directly by allowing holders to "accept the offer and request a credentialSubject.id be included" (see the V2 send-request API in ACApy). So perhaps something ACApy AATH could do around this, is have their {
"holder_did": "did:key:ahsdkjahsdkjhaskjdhakjshdkajhsdkjahs"
} |
Alternatively, making sure the credential is bound to a credentialSubject.id DID could be something that is enforced at the AATH level. That might be more difficult though |
But a related concern, is that i believe the AATH processor is not checking that verified=true from the verifiers POV when verifying the VPs (as raised in discord) |
…foundation#3093) * add static cache, and append fields Signed-off-by: George Mulhearn <[email protected]> * linting and try load the test doc loader Signed-off-by: George Mulhearn <[email protected]> --------- Signed-off-by: George Mulhearn <[email protected]> Co-authored-by: George Mulhearn <[email protected]> Co-authored-by: gmulhearn-anonyome <[email protected]>
…foundation#3093) * add static cache, and append fields Signed-off-by: George Mulhearn <[email protected]> * linting and try load the test doc loader Signed-off-by: George Mulhearn <[email protected]> --------- Signed-off-by: George Mulhearn <[email protected]> Co-authored-by: George Mulhearn <[email protected]> Co-authored-by: gmulhearn-anonyome <[email protected]> Signed-off-by: George Mulhearn <[email protected]>
…foundation#3093) (#3) * add static cache, and append fields * linting and try load the test doc loader --------- Signed-off-by: George Mulhearn <[email protected]>
Fixes #2634