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

feat(w3cCredentials)!: add jwt vc support #1468

Merged
merged 4 commits into from
May 29, 2023

Conversation

TimoGlastra
Copy link
Contributor

Adds support for JWT VCs in addition to the already supported JSON-LD VCs.

It became a bigger PR than anticipated, mainly because I moved some files around and made some changes to e.g. how verification methods are handled so they can be used without JSON-LD as well.

The PR has extracted the JSON-LD specific functionality from the W3cCredentialService into a new W3cJsonLdCredentialService, and we've got a new W3cJwtCredentialService. The general w3c credential service exposes a generic api to work with both, while the specific classes implement the logic. Records are only handled through the general w3c credential service. The idea is that users will mostly use the W3cCredentialService, and not the underlying imlementations, that is more to split up logic based on format.

The API has changed to now require a format when signing credentials / presentations:

w3cCredentialService.signCredential({
  format: 'jwt_vc',
  credential: /* the credential */,
  alg: 'ES256'
})

For the verification the API has also changed to support a more rich API. This rich API has been fully implemented for JWT VCs, but could not be fully supported for JSON-LD library due to the way errors are thrown in that library (see #1466, this will be addressed in a follow up pr).

The idea for the results is that now everything is handled by a custom validation object that has the following structure. The specific validations differ a bit between a VC and VP, but the idea is the same.

{
  isValid: true,
  validations: {
    // credential has no credentialStatus, so always valid
    credentialStatus: {
      isValid: true,
    },
    // This both validates whether the credential matches the
    // data model, as well as whether the credential is expired etc..
    dataModel: {
      isValid: true,
    },
    // This validates whether the signature is valid
    signature: {
      isValid: true,
    },
    // This validates whether the issuer is also the signer of the credential
    issuerIsSigner: {
      isValid: true,
    },
  },
};

For now, for JSON-LD verifications the result looks like this. This means the same data is available before this PR (the VC.js structure was followed), but a bit more nested. Over time we want to remove the use of vc.js and use the same structure as above for both credential formats. This allows for fine grained control over what you want to validate.

{
  isValid: true,
  validations: {
    vcJs: {
      isValid: true,
      // results from the VC.JS library
      results: []
     }
  }
}

@TimoGlastra TimoGlastra requested a review from a team as a code owner May 24, 2023 10:28
packages/core/src/crypto/JwsService.ts Fixed Show fixed Hide fixed
}

public static fromSerializedJwt(serializedJwt: string) {
if (typeof serializedJwt !== 'string' || !Jwt.format.test(serializedJwt)) {

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](3) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](4) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](5) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](6) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](7) may run slow on strings starting with '-.-' and with many repetitions of '--'.
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Merging #1468 (b184851) into main (5075658) will decrease coverage by 13.25%.
The diff coverage is 45.30%.

@@             Coverage Diff             @@
##             main    #1468       +/-   ##
===========================================
- Coverage   85.84%   72.59%   -13.25%     
===========================================
  Files         912      882       -30     
  Lines       21759    21338      -421     
  Branches     3727     3712       -15     
===========================================
- Hits        18679    15491     -3188     
- Misses       2901     5428     +2527     
- Partials      179      419      +240     
Impacted Files Coverage Δ
...ncreds/src/formats/LegacyIndyProofFormatService.ts 78.01% <ø> (-2.10%) ⬇️
.../src/updates/0.3.1-0.4/credentialExchangeRecord.ts 100.00% <ø> (ø)
packages/core/src/error/ClassValidationError.ts 22.22% <0.00%> (-52.78%) ⬇️
...ckages/core/src/modules/dids/methods/jwk/DidJwk.ts 15.62% <0.00%> (-84.38%) ⬇️
packages/core/src/modules/vc/W3cCredentialsApi.ts 41.17% <ø> (-47.06%) ⬇️
packages/core/src/modules/vc/constants.ts 100.00% <ø> (ø)
.../vc/data-integrity/__tests__/contexts/X25519_v1.ts 100.00% <ø> (ø)
...les/vc/data-integrity/__tests__/contexts/bbs_v1.ts 100.00% <ø> (ø)
...ata-integrity/__tests__/contexts/citizenship_v1.ts 100.00% <ø> (ø)
...ata-integrity/__tests__/contexts/citizenship_v2.ts 100.00% <ø> (ø)
... and 113 more

... and 225 files with indirect coverage changes

@swcurran
Copy link
Contributor

Awesome stuff, Timo. Any thought to how AnonCreds in W3C format could use this? Would it be a matter of adding an “algo” for AnonCreds and a corresponding handler? My expectation is that anoncreds-rs could detect W3C format AnonCreds objects as needed and convert internally, and on creation, be passed a flag to indicate the generated objects be converted to W3C form when needed. Do you see that as possible?

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR! I see a lot of manual validation is happening. Is this something we might be able to extract into a non-AFJ library which can be used by anyone, also non AFJ-projects.

packages/core/src/crypto/JwsService.ts Outdated Show resolved Hide resolved

/**
* Base64url encoded protected header
*/
protected: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it is common JWS/JWT practise, but can we not call this protectedHeader? Might be easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a direct mapping of the format as used by the specification. In other places we call it protectedHeader, but if we change this, the JWS service will create invalid JWSes.

packages/core/src/crypto/JwsTypes.ts Outdated Show resolved Hide resolved
packages/core/src/crypto/jose/jwt/Jwt.ts Outdated Show resolved Hide resolved
* is performed against current time (`now`), the validation can be of by the skew
* time.
*/
const DEFAULT_SKEW_TIME = 300
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that can be changed by the user?

Also, might be good to reference: https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can't change the value of this constant. But everywhere this constant is used, the user can provide a custom skew time value. (so this is only used if the user didn't provide a value)

packages/core/src/utils/array.ts Show resolved Hide resolved
packages/openid4vc-client/src/OpenId4VcClientService.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR! I see a lot of manual validation is happening. Is this something we might be able to extract into a non-AFJ library which can be used by anyone, also non AFJ-projects.

@TimoGlastra
Copy link
Contributor Author

I see a lot of manual validation is happening. Is this something we might be able to extract into a non-AFJ library which can be used by anyone, also non AFJ-projects.

Yeah I think we can definitely extract the JWT-VC part to a separate library. Only thing is that we would need to replace the class-transformer/class-validator parts with something else

Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra
Copy link
Contributor Author

Any thought to how AnonCreds in W3C format could use this? Would it be a matter of adding an “algo” for AnonCreds and a corresponding handler?

@swcurran, for this specific addition that wouldn't be possible as this just focuses on JWT credentials using Json Web Signatures. For the AnonCreds mapping we need a custom proof object and type, which we don't support yet.

But for JSON-LD credentials that would exactly how it would work yes, We can add a signature suite called AnonCredsProof2023, or however it is called that will handle the signing and verification of the AnonCreds credential.

let payload: string

if (typeof jws === 'string') {
if (!JWS_COMPACT_FORMAT_MATCHER.test(jws))

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](3) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](4) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](5) may run slow on strings starting with '-.-' and with many repetitions of '--'. This [regular expression](1) that depends on [library input](6) may run slow on strings starting with '-.-' and with many repetitions of '--'.
Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra enabled auto-merge (squash) May 29, 2023 13:58
@TimoGlastra TimoGlastra merged commit 25c76ae into openwallet-foundation:main May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants