veramo icon indicating copy to clipboard operation
veramo copied to clipboard

Error Handling in Veramo Credential-w3c package.

Open daniyal-khalil opened this issue 2 years ago • 7 comments

Is your feature request related to a problem? Please describe. We are working heavily with the Veramo credential-w3c package and whenever we try to verify a presentation which is created with a broken credential for example with invalid issuance date or expiration date, we do not receive the error for why it failed and only receive a boolean stating false.

Describe the solution you'd like We would like to see the reason for why the verification failed as well. To find out what is broken in the credential or the verifiable presentation.

Additional context Code snippet where the error needs to be returned.

try { 
    const verification = await verifyPresentationJWT(jwt, resolver, {
      challenge: args.challenge,
      domain: args.domain,
      audience,
    })
    return true
} catch (e: any) {
    //TODO: return a more detailed reason for failure
    return false
}

daniyal-khalil avatar Jun 29 '22 13:06 daniyal-khalil

Thanks for taking the time to post it!

The problem that needs to be solved there is that verification failures for legitimate reasons and ones for exceptional situations are treated the same way. And this is because of the way the underlying did-jwt/did-jwt-vc stack works.

The solution that I see is to interpret the errors thrown by verifyCredentialJWT/verifyPresentationJWT. Then, for legitimate failures (like invalid VC/VP format, bad signature, expired or not valid yet) to return false while for exceptional scenarios (like malformed JWT, missing iss property, iss not a DID, failure during DID resolution, or JWA/verificationMethod not supported, etc) to rethrow the error.

mirceanis avatar Jun 30 '22 09:06 mirceanis

@mirceanis Sounds exactly like what we are looking for. Because we already have error handling at our hand for legitimate failures.

daniyal-khalil avatar Jul 04 '22 08:07 daniyal-khalil

@mirceanis Hey maybe I'm misunderstanding your point. But wouldn't it be better to throw the errors in case of failure of verifyPresentationJWT? Otherwise the module would have to check for when to throw an error and when to not throw an error and just return false. And returning false would mean abstracting away information that might be useful for the user, such as why did the verifyPresentationJWT failed even in legit cases.

daniyal-khalil avatar Jul 26 '22 13:07 daniyal-khalil

You are right, a simple boolean result for legitimate failures is not enough. Likely a result object with some details is necessary.

There's a target API we're aiming for regarding verifications, so perhaps my answer makes more sense in context:

  • verify*() methods should accept options for verification
  • the options can be used to override some verification policies that are otherwise enabled by default (timestamps, credentialStatus, issuer DID document, schemas, holder binding, etc)
  • The result of verification methods is an object with a verified boolean property for the overall result and detail properties for all the policies.
  • Errors should be thrown only in exceptional situations, where the verification could not be completed (for example, because of a DID resolver error) or is inconclusive (for example, when the agent doesn't support a particular policy required by the credential)
  • Legitimate errors should be returned as details in the result object, and for simplicity, the verification should stop at the first policy with an error

There are legitimate reasons to be able to override policies. For example, "was the credential valid at a particular timestamp?", "does the credential verify against this older version of the DID document?", "is the credential valid but revoked?", "is the signature valid regardless of schema?". Overrides should not necessarily be encouraged, but they should still be possible.

mirceanis avatar Jul 26 '22 13:07 mirceanis

@mirceanis Love the approach of this.

It very much reflects what we already tried to deal with on an HTTP Layer. See "Request Parameters" here -> https://open-credentialing-initiative.github.io/api-specifications/latest/index.html#/VerifiablePresentation/postVerfiablePresentationGeneration

I think these Params we have in place there are pretty much what you imagine to be a 'verification option'.

In case of failed business cases we return a structure like this:

{
  "success": false,
  "errors": [
    "Generation of Verifiable Presentation failed because Credential has been revoked"
  ],
  "errorCodes": [
    "vc_revoked",
    "vc_proof_invalid"
  ]
}

I think this is pretty much 1:1 the same concept, just applied to another layer. From Client HTTP communication to Javascript/Typescript error handling and datastructures.

Really looking forward to the merge request @daniyal-khalil !

lleifermann avatar Jul 26 '22 14:07 lleifermann

@mirceanis Thanks for clarifying the approach further. Just wanted to ask since I was going through the codebase more in-depth. The verify methods have a dependency upon did-jwt and did-jwt-vc but the dependencies do not have any policies that can be specified for the checks to go through. Hence do you believe in making the change on a bottom-up level (allowing did-jwt and did-jwt-vc to also accept the policies) or only a top-down approach through veramo?

daniyal-khalil avatar Jul 28 '22 14:07 daniyal-khalil

@mirceanis Thanks for clarifying the approach further. Just wanted to ask since I was going through the codebase more in-depth. The verify methods have a dependency upon did-jwt and did-jwt-vc but the dependencies do not have any policies that can be specified for the checks to go through. Hence do you believe in making the change on a bottom-up level (allowing did-jwt and did-jwt-vc to also accept the policies) or only a top-down approach through veramo?

I think it will have to be bottom-up. And it's a good reminder to also raise these issues on those repositories to clarify any questions. Thanks for bringing this up!

mirceanis avatar Jul 28 '22 14:07 mirceanis