Skip to content
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

Does verifying a presentation include verifying its VCs? #284

Open
jandrieu opened this issue Apr 14, 2022 · 7 comments
Open

Does verifying a presentation include verifying its VCs? #284

jandrieu opened this issue Apr 14, 2022 · 7 comments
Assignees
Labels
ready for PR Issue ready to be resolved via a Pull Request

Comments

@jandrieu
Copy link
Contributor

When educating clients in this work, I've always said that verifying a VP includes verifying the VCs entailed within.

However, Section 3.3.2 https://w3c-ccg.github.io/vc-api/#verify-presentation doesn't state whether that is correct or not.

Whatever the intention, we should clarify it in the text.

For client-side simplicity, I'd recommend a VP endpoint that DOES verify the VCs, but if we do that we need more resilient responses. In particular, we should be able to report independently on each VC.

Further, for some of the use cases under discussion, it would be exceptionally useful to know why a verification failed. Did it fail the signature check? The expiry check? Or the status check?

@jandrieu
Copy link
Contributor Author

Also, does verifying the VP verify that the subject of the VC is the signer of the VP?

I think not. I think that is a validation rule applied by the Verifier App, not a function verified by the Verifier service. However, the application of this rule could be in either function.

@mkhraisha
Copy link
Collaborator

mkhraisha commented May 17, 2022

When educating clients in this work, I've always said that verifying a VP includes verifying the VCs entailed within.
However, Section 3.3.2 https://w3c-ccg.github.io/vc-api/#verify-presentation doesn't state whether that is correct or not.
Whatever the intention, we should clarify it in the text.

Verifying a VP should verify the VCs within, +1 to update the spec.

For client-side simplicity, I'd recommend a VP endpoint that DOES verify the VCs, but if we do that we need more resilient responses. In particular, we should be able to report independently on each VC.
Further, for some of the use cases under discussion, it would be exceptionally useful to know why a verification failed. Did it fail the signature check? The expiry check? Or the status check?
sample structure:

{
  "verified":false,
  "checks":[
     "string"
  ],
  "warnings":[
     "string"
  ],
  "errors":[
     "credential X failed to verify signature",
     "credential Y failed a status check"
  ]
}

also, does verifying the VP verify that the subject of the VC is the signer of the VP?

No, there are many times a VC is about an org or a person that is not the signer of the VP.

@clehner
Copy link
Member

clehner commented May 18, 2022

Previously: #111

@jandrieu
Copy link
Contributor Author

Thanks, @clehner That is, indeed, essentially the same question and reaches what I think is consensus:

  1. VP verification needs the domain & challenge, and will verify that in the VP
  2. VP verification will verify each VC in the VP
  3. VP verification will NOT apply any business rules about any identifiers in the VCs and the signer of the VP. This means that holder==subject tests are the domain of validation (not verification) and performed by the verifier app, not the verifier service.

What we don't have right now is an endpoint that adequately responds so callers can understand why the VP failed verification. IMO, an http response code of 200 is insufficient.

In fact, it seems that there should be a response that means "input is valid, and verification was successfully performed, and result of verification is that verification failed for reason XYZ."

@clehner
Copy link
Member

clehner commented May 24, 2022

VP verification will NOT apply any business rules about any identifiers in the VCs and the signer of the VP. This means that holder==subject tests are the domain of validation (not verification) and performed by the verifier app, not the verifier service.

OK. What about VP holder ↔ VP proof verification method? (Analogous to VC issuer ↔ VC proof verification method?)

What we don't have right now is an endpoint that adequately responds so callers can understand why the VP failed verification. IMO, an http response code of 200 is insufficient.

In fact, it seems that there should be a response that means "input is valid, and verification was successfully performed, and result of verification is that verification failed for reason XYZ."

I assumed 400 or something else should be returned rather than 200 on a verification failure. Currently the VerifyPresentationResponse/VerificationResult response object is only specified to be returned on the 200 case, and the others are blank; maybe the 400 case should be changed to also return that data structure?
Here: https://w3c-ccg.github.io/vc-api/#verify-presentation

vc-api/verifier.yml

Lines 52 to 66 in 88a2217

responses:
"200":
description: Verifiable Presentation successfully verified!
content:
application/json:
schema:
$ref: "#/components/schemas/VerifyPresentationResponse"
"400":
description: Invalid or malformed input
"413":
description: Payload too large
"429":
description: Request rate limit exceeded.
"500":
description: Internal Server Error

@OR13
Copy link
Contributor

OR13 commented Apr 24, 2023

You might also want to consider the following:

verifying a single presentation or 5 credentials, 2 have holder binding to different keys, 3 have status lists, 2 have multiple statuses 3 have status failures, 1 fails to verify.

@msporny
Copy link
Contributor

msporny commented May 23, 2023

The group discussed this on the 2023-05-23 telecon. @dlongley noted that the default behaviour for the verification endpoint should verify the VP and all of the VPs inside of it by default. We could explore adding an option to skip checking the VCs if there is a use case where that makes sense. Individual implemenations could do that in a proprietary way (if we don't want to standardize that in this revision). @PatStLouis agreed, otherwise there isn't a difference between credentials/verify and presentations/verify. You want to verify what is contained in a presentation, that makes both endpoints different. @jandrieu liked what @dlongley said wrt. default, but could see in debugging (wanting to turn it off)... want to be able to verify presentation feels like useful optionality. @msporny asked what does the error object look like. @dlongley noted that implementations do return error information, but we haven't set how errors should be modelled. In other cases we didn't want to say how errors would work, but this might be a place where we wanted to say which VCs failed, which proofs failed or were not supported, if some proofs pass but others don't, what do you do? VCs could have multiple proofs on them, is subset sufficient, but caller should know what was checked if they want to dive into it, they should be able to dive into it. @jandrieu noted that we need to be concerned about optionality.

This is ready for PR, when verifying a presentation verify all of the VCs in the presentation as well. When errors are returned, ensure that it is possible to understand which VCs were checked, which proofs for each VC was checked, factors of verfication were checked (example: validFrom/validUntil -- and return status of all factors), and errors related to each VC/VP are provided. The text should allow implementers to provide options to disable checking VCs and highlight that the API is compositional such that if you didn't check all the VCs, you could check which ones you wanted to check using the /credentials/verify endpoint.

@msporny msporny added the ready for PR Issue ready to be resolved via a Pull Request label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for PR Issue ready to be resolved via a Pull Request
Projects
None yet
Development

No branches or pull requests

5 participants