refactor!: fetch attestation as part of verifyCredential #708

rflechtner · 2023-02-01T23:41:48Z

fixes https://github.com/KILTprotocol/ticket/issues/2397

also fixes https://github.com/KILTprotocol/ticket/issues/2305

This changes the function signature of verifyCredential and verifyPresentation so that all verification steps are covered, including querying the attestation. They now return the credential enriched with attester and revocation info. Assertions about both are NOT made, but these depend a lot on your application logic and are trivial to do with the data returned from these functions.

I like the idea of having a data model for a VerifiedCredential bc it allows introducing another function, called refreshRevocationStatus here, which queries the attestation again and throws not only failing if the attestation has been deleted or has been recreated with incoherent data, but also if it has been recreated by a different attester. This avoids potential exploits where a verifier who only checks the revoked flag once a credential has been verified is tricked by a claimer who simply self-attests their credential after the original attester had deleted the attestation.

If preferred, I could return just revocation status and attester though, which would then mean refreshRevocationStatus would take both the credential and the last status as input parameters.

Usage Example

const didsWithAccess = new Set()
try {
  const verifiedCredential = await verifyPresentation(presentation, {
    ctype,
    challenge,
  })
  if(verifiedCredential.revoke === false) {
    // access granted
    didsWithAccess.add(presentation.claim.owner)
  }
  // periodically recheck if credential is still valid
  setInterval(() => {
    recheckRevocationStatus(verifiedCredential)
      .then(c => if(c.revoked !== false) throw new Error())
      .catch(
        () => {
          // access revoked
          didsWithAccess.delete(presentation.claim.owner)
        }
      )
  }, 60_000)
} catch {
  // access denied
}

How to test:

Test coverage in unit tests and integration tests has been added.

Checklist:

I have verified that the code works
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)

ntn-x2

So now verifying a delegation and the list of trusted attesters would be outside of any function calls, is that correct?

ntn-x2 · 2023-02-02T07:36:05Z

packages/core/src/credential/Credential.ts

 * @returns Information on the attester and revocation status of the on-chain attestation, as well as info on which trust policy led to acceptance of the credential.
 */
 export async function verifyAttested(
  credential: ICredential,
-  allowedAuthorities?: TrustPolicies,
-  allowRevoked = false
+  throwOnRevoked = true


Why doesn't this function also returns a VerifiedCredential? Plus, I agree that removing the throwOnRevoked flag is more future-proof and less prone to mistakes, as people might want to check other stuff as well.

Because it does not actually verify a credential. Just checks the attestation. Name suggestions for that function are welcome.

rflechtner · 2023-02-02T12:10:11Z

So now verifying a delegation and the list of trusted attesters would be outside of any function calls, is that correct?

Correct! And given you agree that the revocation check can go to, so would be checking if revoked === false.

ntn-x2 · 2023-02-02T12:47:30Z

+1 for this solution then. I like it. Not sure if the additional type would be more confusing for the users. Maybe @weichweich can provide his opinion on this a well. Maybe @arty-name also?

rflechtner · 2023-02-02T12:55:40Z

Yes, would love to hear their opinions. Compare this one as well against having a separate function for checking the attestation (i.e. not changing verifyCredential at all - see for example #709, but could also be a regular function) and let me know which of the three you think is best.

arty-name · 2023-02-02T14:47:19Z

I like this much more than the proposal I have seen previously! The alternative of having a separate function does not appeal to me either.

rflechtner · 2023-02-14T16:04:39Z

So to summarise, this seems to be the preferred solution rn, and we'd probably drop the throwOnRevoked parameter too. In that case I'd probably rename recheckRevocationStatus to refreshRevocationStatus (or updateRevocationStatus).
You'd get an object back which gives you revocation status and attester, and you'd compare their values against your expectations (e.g. you reject revoked credentials or not, or you only allow attesters from a self-curated list).

If the VerifiedCredential type is confusing, we can also introduce a CredentialStatus type instead, which would change the signature of refreshRevocationStatus too:

type CredentialStatus = {
  attester: DidUri
  revoked: boolean
}

async function refreshRevocationStatus(
   credential: ICredential,
   lastStatus: CredentialStatus
): Promise<CredentialStatus>

Dudleyneedham

Mostly comments. The code looks good.

Dudleyneedham · 2023-02-17T08:15:27Z

packages/core/src/credential/Credential.ts

+/**
+ * Verifies data structure & data integrity of a credential object.
+ * This combines all offline sanity checks that can be performed on an ICredential object.
+ * A credential is valid only iff it is well formed and there is an onchain attestation record referencing its root hash.


Suggested change

* A credential is valid only iff it is well formed and there is an onchain attestation record referencing its root hash.

* A credential is valid only if it is well formed and there is an on-chain attestation record referencing its root hash.

I’m not the only one thrown off by the usage of "iff" in the sense "if and only if" :)

Is this even true? If I have a well structured credential with an on-chain attestation record it can still be invalid? E.g. if the delegation record is wrong or doesn't exist.

You're right, this isn't complete; the attestation also needs to match the credential, i.e. delegation and ctype ids need to be identical to what was committed to in the credential. BTW that's something we should have designed differently; the runtime should combine at least delegation & ctype info into the credential hash so there is no chance these data do not match if the hash is identical. Ideally also the attester and block number, just to make sure the same attestation is never re-created.

Anyway, we would do these checks as part of verifyCredential now.

This should be part of the discussion for credentials v2. We can build on the current limitations and do better.

Dudleyneedham · 2023-02-17T08:18:56Z

packages/core/src/credential/Credential.ts

+): Promise<VerifiedCredential> {
+  const { revoked, attester } = await verifyAttested(
+    verifiedCredential,
+    throwOnRevoked


Do we not want this always to be the case? We decided that if there was an error, we'd always throw it.

Is there a reason why we might not?

The consensus was actually to get rid of this flag entirely, so that it never throws when the attestation is revoked. This was to control whether or not you consider a revoked attestation invalid for the sake of you application.

packages/core/src/credential/Credential.ts

Dudleyneedham · 2023-02-17T08:20:41Z

packages/core/src/credential/Credential.ts

-   */
-  isDelegator?: boolean
+type VerifyOptions = {
+  ctype?: ICType


I find it strange that CType isn't required. Is there a specific reason why?

see previous comment

rflechtner · 2023-02-21T15:02:45Z

If the VerifiedCredential type is confusing, we can also introduce a CredentialStatus type instead, which would change the signature of refreshRevocationStatus too:
type CredentialStatus = {
  attester: DidUri
  revoked: boolean
}

async function refreshRevocationStatus(
   credential: ICredential,
   lastStatus: CredentialStatus
): Promise<CredentialStatus>

@arty-name @ntn-x2 @weichweich @Dudleyneedham
Can I ask again about people's preferences regarding the return type of verifyCredential & verifyPresentation ? I liked the VerifiedCredential type for having all relevant info in one place, plus it's a good way to communicate at which point you can trust the information in a credential (-> as soon as it's a VerifiedCredential). The CredentialStatus felt a bit like a duplication of the IAttestation object, so personally I'm not too fond of it, but I wouldn't want to return an IAttestation either, simply because the attester is called owner there, which I think is very confusing in this context. If you like the CredentialStatus better though, I'd go this route, would be interested to hear where you're coming from though.

ntn-x2 · 2023-02-21T15:33:51Z

Just a point, that relying too much on Typescript could leave Javascript developers in the dark. But I like the name VerifiedCredential. It also does not feel right that a verify function returns a CredentialStatus. Different is, if the function is renamed, then it might make sense.

arty-name · 2023-02-22T12:15:33Z

I think both options are fine, and I have a slight preference for the current VerifiedCredential

rflechtner · 2023-02-22T12:36:31Z

Just a point, that relying too much on Typescript could leave Javascript developers in the dark.

I hope that can be resolved with good docstrings.

weichweich · 2023-02-23T14:54:26Z

Just a point, that relying too much on Typescript could leave Javascript developers in the dark. But I like the name VerifiedCredential. It also does not feel right that a verify function returns a CredentialStatus. Different is, if the function is renamed, then it might make sense.

I agree with Antonio. In JS code i would just use

if (verifyCredential(...)) {
  ...
}

And that's always true.

rflechtner · 2023-02-23T15:39:32Z

In JS code i would just use
if (verifyCredential(...)) {
  ...
}
And that's always true.

And thaaaaaat's why we banished you to the Rust dungeon! Please don't ever do that ever. You're calling an async function, so it returns a Promise which is always truthy, even if the function throws an exception, and even if the value it wraps is itself falsy.

So I don't think the average JS developer works on the basis of 'just call some function and assume it returns a boolean'. You read the docstring at the very least, how else would you know what it does, when it throws, and what it returns? The same approach would fail miserably with vc.js's verify function signature (https://github.com/digitalbazaar/vc#verifying-a-verifiable-presentation), where you get the final result from the verified property of the (awaited) return value. I think Antonio's point was rather to not assume people have access to interfaces and types, which I don't think is a big issue here though. I think we can describe the required structure by saying 'this structure, but with two fields added' or simply by saying 'the return value of this function', and we should have JS developers covered?

ntn-x2

Looks very good to me! I could not find any mention in the JSDocs about the consumer actually having to check the trusted attester list and the delegation information.

packages/core/src/credential/Credential.ts

rflechtner · 2023-02-28T12:14:44Z

Looks very good to me! I could not find any mention in the JSDocs about the consumer actually having to check the trusted attester list and the delegation information.

Again, you don't have to match the attester to some list of trusted attesters in your application; your application could also limit itself to simply displaying the attester DID or web3name to the user, like most of our apps do. That's why this solution is better than the previous candidate, which I think was too opinionated. I could add something like "this data can be used in logic deciding whether to accept or reject a credential submission", but a naive user would probably need more context to understand what that means, which they can only get from the docs - the docstring wouldn't be the right place to explain conceptual fundamentals, would it?

ntn-x2

I reviewed a VERY old version of this PR, somehow. The new changes look way better, sorry about that. Great work!

rflechtner mentioned this pull request Feb 1, 2023

feat: verification helpers #702

Closed

5 tasks

ntn-x2 reviewed Feb 2, 2023

View reviewed changes

rflechtner mentioned this pull request Feb 14, 2023

feat: credential verification derives #709

Closed

5 tasks

Dudleyneedham reviewed Feb 17, 2023

View reviewed changes

rflechtner changed the base branch from rf-verification-helpers to develop February 20, 2023 14:50

rflechtner changed the title ~~refactor: return from verifyCredential instead~~ refactor!: return from verifyCredential instead Feb 20, 2023

rflechtner changed the title ~~refactor!: return from verifyCredential instead~~ refactor!: fetch attestation as part of verifyCredential Feb 21, 2023

rflechtner added 13 commits February 21, 2023 14:15

feat: credential attestation verification helper

7ecc267

fix: verifyAgainstCredential didn't check delegation

42a2dcd

feat: trust policies parameter for verifyAttested

6f7841b

feat: call verifyAttested from verifyCredential

e8b1596

chore: add {} to all ifs

e6bd37b

feat: report mismatching attributes in error message

33b9554

feat: return VerifiedCredential from verify{Credential,Presenation}

5792f88

chore: remove throwOnRevoked param

a8d4b62

chore: update docstrings

2be8aba

fix: credential.spec

52b06e8

test: add tests for attestation recheck

d10e0cf

fix: make sure refreshRevocationStatus gets a VerifiedCredential

1251f05

fix: integration tests

8a1f011

rflechtner force-pushed the rf-return-from-verify branch from cdd2782 to 8a1f011 Compare February 21, 2023 20:39

feat: report mismatching attributes in error message

65b0808

rflechtner marked this pull request as ready for review February 21, 2023 21:13

rflechtner self-assigned this Feb 21, 2023

rflechtner requested a review from ntn-x2 February 27, 2023 10:54

ntn-x2 reviewed Feb 27, 2023

View reviewed changes

packages/core/src/credential/Credential.ts Show resolved Hide resolved

ntn-x2 approved these changes Feb 28, 2023

View reviewed changes

chore: improve docstrings

1a1e59c

rflechtner merged commit f8b54cd into develop Mar 1, 2023

rflechtner deleted the rf-return-from-verify branch March 1, 2023 11:52

weichweich mentioned this pull request May 2, 2023

feat: real login KILTprotocol/web3-login-demo#15

Merged

rflechtner mentioned this pull request Jun 8, 2023

chore: release 0.33.0 #758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: fetch attestation as part of verifyCredential #708

refactor!: fetch attestation as part of verifyCredential #708

rflechtner commented Feb 1, 2023 •

edited

Loading

ntn-x2 left a comment

ntn-x2 Feb 2, 2023

rflechtner Feb 20, 2023

rflechtner commented Feb 2, 2023

ntn-x2 commented Feb 2, 2023

rflechtner commented Feb 2, 2023

arty-name commented Feb 2, 2023

rflechtner commented Feb 14, 2023

Dudleyneedham left a comment

Dudleyneedham Feb 17, 2023

arty-name Feb 17, 2023

weichweich Feb 19, 2023

rflechtner Feb 20, 2023

rflechtner Feb 20, 2023

ntn-x2 Feb 20, 2023

Dudleyneedham Feb 17, 2023

rflechtner Feb 20, 2023

Dudleyneedham Feb 17, 2023

rflechtner Feb 20, 2023

rflechtner commented Feb 21, 2023

ntn-x2 commented Feb 21, 2023

arty-name commented Feb 22, 2023

rflechtner commented Feb 22, 2023

weichweich commented Feb 23, 2023

rflechtner commented Feb 23, 2023

ntn-x2 left a comment

rflechtner commented Feb 28, 2023

ntn-x2 left a comment

	* A credential is valid only iff it is well formed and there is an onchain attestation record referencing its root hash.
	* A credential is valid only if it is well formed and there is an on-chain attestation record referencing its root hash.

refactor!: fetch attestation as part of verifyCredential #708

refactor!: fetch attestation as part of verifyCredential #708

Conversation

rflechtner commented Feb 1, 2023 • edited Loading

fixes https://github.com/KILTprotocol/ticket/issues/2397

also fixes https://github.com/KILTprotocol/ticket/issues/2305

Usage Example

How to test:

Checklist:

ntn-x2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rflechtner commented Feb 2, 2023

ntn-x2 commented Feb 2, 2023

rflechtner commented Feb 2, 2023

arty-name commented Feb 2, 2023

rflechtner commented Feb 14, 2023

Dudleyneedham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rflechtner commented Feb 21, 2023

ntn-x2 commented Feb 21, 2023

arty-name commented Feb 22, 2023

rflechtner commented Feb 22, 2023

weichweich commented Feb 23, 2023

rflechtner commented Feb 23, 2023

ntn-x2 left a comment

Choose a reason for hiding this comment

rflechtner commented Feb 28, 2023

ntn-x2 left a comment

Choose a reason for hiding this comment

rflechtner commented Feb 1, 2023 •

edited

Loading