feat: apply new version of SD JWT package #1787

lukasjhan · 2024-03-03T09:42:37Z

Hi, This PR is for applying new version of SD JWT package.
I'm developing SD JWT package with @berendsliedrecht in https://github.com/openwallet-foundation-labs/sd-jwt-js.

Changes were made to reflect the latest standards and provide missing features.

The types of disclosureFrame and presentationFrame have changed. also replaced the tests with credentials that meet the latest standards.

This is my first time contributing to credo.
I'm open to changes so if you need anything please feel free to let me know. :)

Thank you.

Signed-off-by: Lukas.J.Han <[email protected]>

This reverts commit f289a82. Signed-off-by: Lukas.J.Han <[email protected]>

lukasjhan · 2024-03-03T15:47:52Z

btw, I want to continuously contribute to Credo-ts by working on sd-jwt-related integration. :)

TimoGlastra

Thanks for the contribution @lukasjhan. Overall LGTM, except for the change in how presentationFrame works (but i might be convinced otherwise).

We also will have to update PEX and sphereon/ssi-types.

packages/core/src/modules/sd-jwt-vc/SdJwtVcService.ts

packages/core/tsconfig.build.json

cre8 · 2024-03-04T08:18:48Z

FYI: There is another problem with the referencing of the holder did:

cnf: {
          // We need to include the whole didUrl here, otherwise the verifier
          // won't know which did it is associated with
          kid: holder.didUrl,
        },

The reference to a did url is not standartised yet, see: oauth-wg/oauth-sd-jwt-vc#205

To be compliant to the standard right now it has to include it key as a jsonwebkey. But of course it is loosing a lot of features with it like status validation of the key since there is no information to the did document (or the ledger behind it to request different versions for validation)

Signed-off-by: Lukas <[email protected]>

This reverts commit aa82727. Signed-off-by: Lukas <[email protected]>

Signed-off-by: Lukas <[email protected]>

TimoGlastra · 2024-03-06T07:05:52Z

To be compliant to the standard right now it has to include it key as a jsonwebkey. But of course it is loosing a lot of features with it like status validation of the key since there is no information to the did document (or the ledger behind it to request different versions for validation)

Last time i read the spec using cnf.jwk was RECOMMENDED. However there's nothing that prevents kid to be used. The current version of sd-jwt library we depend on doesn't resolve the kid and it's up to the library user to provide the correct JWK for verification. I think the new sd-jwt library should do the same and allow to deviate from RECOMMENDED approaches (e.g. using kid with did for binding)

cre8 · 2024-03-06T08:58:23Z

To be compliant to the standard right now it has to include it key as a jsonwebkey. But of course it is loosing a lot of features with it like status validation of the key since there is no information to the did document (or the ledger behind it to request different versions for validation)

Last time i read the spec using cnf.jwk was RECOMMENDED. However there's nothing that prevents kid to be used. The current version of sd-jwt library we depend on doesn't resolve the kid and it's up to the library user to provide the correct JWK for verification. I think the new sd-jwt library should do the same and allow to deviate from RECOMMENDED approaches (e.g. using kid with did for binding)

I just wanted to point out that using the did url in the kid is not standardized. The referenced RFC however says: The content of the "kid" value is application specific. For instance, some applications may choose to use a JWK Thumbprint [[JWK.Thumbprint](https://www.rfc-editor.org/rfc/rfc7800.html#ref-JWK.Thumbprint)] value as the "kid" value. The sd-jwt-vc spec just says we need to follow the RFC 7800

My point is that other systems will not be able to handle the public key resolution since resolving a did in a cnf is neither a standard in the 7800 nor defined in the sd-jwt-vc

TimoGlastra · 2024-03-07T07:49:43Z

Even though it's not standardized, nothing prevents us from taking this approach while the details are being figured out on what should be standardized (and it's not against the standard, we're relying on the application specific logic part). We want to do did binding today, and this solution seems simple and straightforward.

Once there's a standard way to do it we're happy to change it to that approach. Using a did url as kid has been used for quite a while in JWT VCs, so the approach seems reasonable to me.

TimoGlastra · 2024-03-08T12:09:33Z

We're about to make a 0.5 release of Credo (somewhere next week). So if we want to get this in, we would need to have a stable sd-jwt 0.3 release that we can add.

Otherwise we need to push it to a 0.5.1 release (the SD-JWT api is experimental, so it's "okay" to push breaking changes in a 0.5.x release for now). But it'd of course be better if we can get it in for 0.5 (not sure though how it'll work with changes to Sphereon libraries, as we also need new releases of PEX, SSI-types, openid4vci, openid4vp-siop, which is going to take a while I think. But maybe it will work fine if we are on 0.3 and those libs are still on 0.2 of sd-jwt?)

Please let me know what you think is the right approach here :)

lukasjhan · 2024-03-08T14:07:11Z

We're about to make a 0.5 release of Credo (somewhere next week). So if we want to get this in, we would need to have a stable sd-jwt 0.3 release that we can add.

Otherwise we need to push it to a 0.5.1 release (the SD-JWT api is experimental, so it's "okay" to push breaking changes in a 0.5.x release for now). But it'd of course be better if we can get it in for 0.5 (not sure though how it'll work with changes to Sphereon libraries, as we also need new releases of PEX, SSI-types, openid4vci, openid4vp-siop, which is going to take a while I think. But maybe it will work fine if we are on 0.3 and those libs are still on 0.2 of sd-jwt?)

Please let me know what you think is the right approach here :)

@TimoGlastra 0.4.0 version is released I'll update this PR. and I'll try the implement other library in this weekend. I opened a PR in SSI-types, I'm currently working on PEX. And I'm trying to look into openid4vci and openid4vp-siop.

TimoGlastra · 2024-03-08T14:20:59Z

Thanks so much @lukasjhan!! That sounds great.

Signed-off-by: Lukas.J.Han <[email protected]>

This reverts commit 6500149. Signed-off-by: Lukas.J.Han <[email protected]>

lukasjhan · 2024-03-12T05:54:33Z

I think jwt verification keep failing in my sd-jwt test. I'll look into it.

Hmm.. the simpleJwtVc is not verified in test. I think I need some help on signing and verifying. @TimoGlastra

const simpleJwtVc =
  'eyJ0eXAiOiJ2YytzZC1qd3QiLCJhbGciOiJFZERTQSIsImtpZCI6IiN6Nk1rdHF0WE5HOENEVVk5UHJydG9TdEZ6ZUNuaHBNbWd4WUwxZ2lrY1czQnp2TlcifQ.eyJjbGFpbSI6InNvbWUtY2xhaW0iLCJ2Y3QiOiJJZGVudGl0eUNyZWRlbnRpYWwiLCJjbmYiOnsiandrIjp7Imt0eSI6Ik9LUCIsImNydiI6IkVkMjU1MTkiLCJ4Ijoib0VOVnN4T1VpSDU0WDh3SkxhVmtpY0NSazAwd0JJUTRzUmdiazU0TjhNbyJ9fSwiaXNzIjoiZGlkOmtleTp6Nk1rdHF0WE5HOENEVVk5UHJydG9TdEZ6ZUNuaHBNbWd4WUwxZ2lrY1czQnp2TlciLCJpYXQiOjE2OTgxNTE1MzJ9.vLkigrBr1IIVRJeYE5DQx0rKUVzO3KT9T0XBATWJE89pWCyvB3Rzs8VD7qfi0vDk_QVCPIiHq1U1PsmSe4ZqCg~';

const publicKeyBase58 = 'FPdUn1sksw3gHN2C7svR9YentF5vH5HyKfopnE5B5hb8';

It's not verified with test publickey of issuer FPdUn1sksw3gHN2C7svR9YentF5vH5HyKfopnE5B5hb8

TimoGlastra · 2024-03-13T20:56:15Z

Hey @lukasjhan sorry I've been busy with some OID4VC stuff to get Credo 0.5 out before the OWF anniversary event tomorrow.

As we have a working implementation of SD-JWT in Credo already and this PR isn't ready yet we have to postpone it to a future release (hopefully very soon) of Credo.

I'll try to look into tests somewhere next week.

lukasjhan · 2024-03-14T00:33:41Z

Hey @lukasjhan sorry I've been busy with some OID4VC stuff to get Credo 0.5 out before the OWF anniversary event tomorrow.

As we have a working implementation of SD-JWT in Credo already and this PR isn't ready yet we have to postpone it to a future release (hopefully very soon) of Credo.

I'll try to look into tests somewhere next week.

Okay. Thank you :)

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra · 2024-03-18T11:28:15Z

@lukasjhan I pushed a fix. The issue was that the signature was passed as base64url, but it wasn't decoded as base64. So the string was converted to bytes, but not decoded from base64.

I think it may be nice to already pass the signature as bytes/buffer/uint8array in the sd-jwt library, so there's no decoding needed to be done (and the type is not ambiguous)

TimoGlastra · 2024-03-18T11:28:40Z

I also updated the sd-jwt version. there seems to be some type issues still, which I haven't looked at. If you can get those addressed, we can get this merged as 0.5.1 I think

lukasjhan · 2024-03-18T14:19:34Z

I think it may be nice to already pass the signature as bytes/buffer/uint8array in the sd-jwt library, so there's no decoding needed to be done (and the type is not ambiguous)

@TimoGlastra Thank you. :)
When you say type issue, you mean ssi-type and PEX, right?
The SSI-type and PEX is not ready yet. But I opened a PR for each repo.

TimoGlastra · 2024-03-18T14:21:17Z

No if you look at this pr, some type check fails related to oid4vc which is using the sd-jwt interfaces

lukasjhan · 2024-03-18T14:25:47Z

No if you look at this pr, some type check fails related to oid4vc which is using the sd-jwt interfaces

Got it :) I'll check it right away

Signed-off-by: Lukas.J.Han <[email protected]>

lukasjhan · 2024-03-19T01:37:57Z

@TimoGlastra I updated PR to fix type validation.

I think we will need to change the type to be inferred later by applying a generic to the OpenId4VciSignCredential type.
since The disclosureFrame type is defined from the claim. Like Disclosure<typeof claim>

TimoGlastra

Nice, great work @lukasjhan. I have a few questions / comments remaining. I think the only real blocker for me is the type cast needed for oid4vc, as it will not provide a good user experience to have it broken like this.

TimoGlastra · 2024-03-19T08:54:18Z

packages/core/src/modules/sd-jwt-vc/SdJwtVcService.ts

+    const verificationResult: VerificationResult = {
+      isValid: false,
+      isSignatureValid: false,
+    }
+
+    await sdjwt.verify(compactSdJwtVc, requiredClaimKeys, !!keyBinding)
+
+    verificationResult.isValid = true
+    verificationResult.isSignatureValid = true
+    verificationResult.areRequiredClaimsIncluded = true


Shouldn't this be in a try/catch where isValid is set to false if it throws an error?

I think it can be a bit vague now in that it sometimes throws an error and sometimes returns the object.

Do you think it would also be possible to maybe update the verify method is sd-jwt to return the fields related to the checks? We want to move more towards complex verification objects where you can see in detail what checks were ✅ and what checks were ❌.

I think there's also several properties on the verificadtionResult that are never set, e.g. isNotBeforeValid?: boolean.

TimoGlastra · 2024-03-19T08:56:27Z

packages/core/src/modules/sd-jwt-vc/SdJwtVcService.ts

+    if (!sdJwtVc.jwt?.payload) {
+      throw new SdJwtVcError('Credential not exist')
+    }
+
+    if (!sdJwtVc.jwt?.payload['iss']) {
+      throw new SdJwtVcError('Credential does not contain an issuer')
+    }


For a sd-jwt to be valid, you would need the property iss. Is there a method that checks this and then updates the type of the payload to be the minimal required structure from the sd-jwt-vc spec?

I was also missing that in the previous implementation, and a UnverifiedSdJwt and VerifiedSdJwt interface could help a lot in this case, as you then don't have to do all these checks every time you want to use a property from the payload (this is not blocking in any way, just thinking how we could improve and simplify the usage of the sd-jwt library)

TimoGlastra · 2024-03-19T08:57:56Z

packages/core/src/modules/sd-jwt-vc/__tests__/sdjwtvc.fixtures.ts

+  iat: 1698151532,
+  claim: 'some-claim'
+}
+ */
 export const sdJwtVcWithSingleDisclosure =


Why did these change? Do these not verify with the new library? Would be curious to know why

I'll check it and let you know :)

TimoGlastra · 2024-03-19T08:59:09Z

packages/openid4vc/tests/openid4vc.e2e.test.ts

@@ -97,7 +98,7 @@ describe('OpenId4Vc', () => {
                      method: 'did',
                      didUrl: verificationMethod.id,
                    },
-                    disclosureFrame: { university: true, degree: true },
+                    disclosureFrame: { _sd: ['university', 'degree'] } as DisclosureFrame<SdJwtVcPayload>,


Any idea why this is needed? Can we make it this type by default? It's a bit of a bummer if the default interface gives you TS errors when using it, so I'd rather have an overly permissive type, than a type that doesn't allow you to write valid code

This is because the type of disclosureFrame in our package is set to a generic type created based on the type of claim.
I think we can do one of:

Add generic in openid4vc type.

Add as in somewhere middle (e.g. sdJwtVcService.ts)

TimoGlastra · 2024-03-19T08:59:40Z

packages/core/src/modules/sd-jwt-vc/SdJwtVcOptions.ts

+export type SdJwtVcGetPresentationKeysOptions = {
+  compactSdJwtVc: string
+}


I think this can be removed, as we don't need the getPresentableKeys method anymore right?

Yes your correct. I'll remove all code related getPresentableKeys

TimoGlastra · 2024-03-19T09:00:08Z

packages/core/src/modules/sd-jwt-vc/SdJwtVcOptions.ts

  /**
   * Use true to disclose everything
   */


I thin true was removed, what is now the way to disclose everything vs nothing? Can you update the comment?

I'll update it :)
FYI:

disclose everything: presentationFrame: undefined

disclose nothing: presentationFrame: {} // empty object

TimoGlastra · 2024-04-12T07:22:32Z

Hey @lukasjhan, just wanted to check whether you are able to get the final fixes in soon, so we can get this merged and released :)

I had one additional question: the docs website for sd-jwt mentions you support draft 8 and 3. We currently support Draft 6 and 3, do you know if this will cause incompatible changes? Will older SD-JWT vcs not validate anymore or are it mostly smaller changes?

TimoGlastra · 2024-04-12T07:26:34Z

It seems these are the breaking changes:

_sd_hash has been renamed to sd_hash
'iat' is now optional and allowed to be selectively disclosable (but this is loosened in newer versions so not acutally a problem)

So only the _sd_hash vs sd_hash may be a problem.

lukasjhan · 2024-04-12T07:27:46Z

@TimoGlastra Yeah That would be the breaking changes.
btw: I'll follow rest of your comments this weekends before IIW. I was busy lately.

TimoGlastra · 2024-04-12T07:29:27Z

Ah looking at the sd-jwt lib implementation, there is backwards compat taken into account, that is great 🙌

lukasjhan · 2024-04-12T07:35:22Z

Yeah, Let's check it before release. To be sure :)

Signed-off-by: Timo Glastra <[email protected]>

Signed-off-by: Lukas.J.Han <[email protected]>

TimoGlastra · 2024-04-24T22:57:10Z

Made some small changes locally @lukasjhan, ready to merge :)

Thanks a lot for the work 🚀 🚀 🚀

Signed-off-by: Lukas.J.Han <[email protected]>

lukasjhan · 2024-04-24T23:00:19Z

Made some small changes locally @lukasjhan, ready to merge :)

Thanks a lot for the work 🚀 🚀 🚀

Yeah I saw that version updates to 0.6.1. thank you :)
I just change the type for wider use cases.

lukasjhan · 2024-04-24T23:01:29Z

Oh sorry, that I didn't know you start merge.
Can you try it again please? @TimoGlastra

lukasjhan and others added 9 commits March 3, 2024 18:39

chore: fix test for quick

e877104

Signed-off-by: Lukas.J.Han <[email protected]>

feat: upgrade sd-jwt package

53a196b

Signed-off-by: Lukas.J.Han <[email protected]>

fix: tests

111b52f

Signed-off-by: Lukas.J.Han <[email protected]>

test: fix e2e test

84f7a4e

Signed-off-by: Lukas.J.Han <[email protected]>

fix verify

5cafd53

Signed-off-by: Lukas.J.Han <[email protected]>

feat: update sdjwtvc package

0c050e9

Signed-off-by: Lukas.J.Han <[email protected]>

fix yarn lock

62146f2

Signed-off-by: Lukas.J.Han <[email protected]>

test: add description(decoded) on test sd-jwt

f0f7c92

Signed-off-by: Lukas.J.Han <[email protected]>

Revert "chore: fix test for quick"

87fd571

This reverts commit f289a82. Signed-off-by: Lukas.J.Han <[email protected]>

TimoGlastra reviewed Mar 4, 2024

View reviewed changes

packages/core/src/modules/sd-jwt-vc/SdJwtVcService.ts Outdated Show resolved Hide resolved

packages/core/tsconfig.build.json Outdated Show resolved Hide resolved

lukasjhan mentioned this pull request Mar 4, 2024

feat: add jwk type openwallet-foundation/sd-jwt-js#130

Merged

lukasjhan added 4 commits March 6, 2024 11:57

chore: include ts dom lib, to find JsonWebKey

3495bcd

Signed-off-by: Lukas <[email protected]>

Revert "chore: include ts dom lib, to find JsonWebKey"

054dbd9

This reverts commit aa82727. Signed-off-by: Lukas <[email protected]>

chore: update sd-jwt package version

07e3c78

Signed-off-by: Lukas <[email protected]>

chore: update sdjwt package

ba30c19

Signed-off-by: Lukas <[email protected]>

lukasjhan force-pushed the upgrade/sdjwt branch from 5057f1a to ba30c19 Compare March 6, 2024 02:58

lukasjhan and others added 6 commits March 10, 2024 22:54

chore: update package 0.4.0

55bb6fb

Signed-off-by: Lukas.J.Han <[email protected]>

feat: restore presentationFrame

190a38e

Signed-off-by: Lukas.J.Han <[email protected]>

chore: fix test for quick

6500149

Signed-off-by: Lukas.J.Han <[email protected]>

feat: restore type

af95b18

Signed-off-by: Lukas.J.Han <[email protected]>

test: fix test

d029bc5

Signed-off-by: Lukas.J.Han <[email protected]>

Revert "chore: fix test for quick"

3593597

This reverts commit 6500149. Signed-off-by: Lukas.J.Han <[email protected]>

TimoGlastra added 2 commits March 18, 2024 12:20

Merge remote-tracking branch 'upstream/main' into upgrade/sdjwt

42eb16e

fix: signature is base64 url

a6f666a

Signed-off-by: Timo Glastra <[email protected]>

test: fix type

e5254ea

Signed-off-by: Lukas.J.Han <[email protected]>

TimoGlastra reviewed Mar 19, 2024

View reviewed changes

TimoGlastra and others added 2 commits April 25, 2024 00:33

updates

31dda2a

Signed-off-by: Timo Glastra <[email protected]>

Merge remote-tracking branch 'upstream/main' into upgrade/sdjwt

31fd6ab

Signed-off-by: Lukas.J.Han <[email protected]>

TimoGlastra approved these changes Apr 24, 2024

View reviewed changes

TimoGlastra enabled auto-merge (squash) April 24, 2024 22:56

feat: use wider type def

064da30

Signed-off-by: Lukas.J.Han <[email protected]>

auto-merge was automatically disabled April 24, 2024 22:59
Head branch was pushed to by a user without write access

TimoGlastra enabled auto-merge (squash) April 24, 2024 23:24

Merge remote-tracking branch 'upstream/main' into upgrade/sdjwt

cdf2d3d

TimoGlastra merged commit b41e158 into openwallet-foundation:main Apr 25, 2024
12 checks passed

feat: apply new version of SD JWT package #1787

feat: apply new version of SD JWT package #1787

Conversation

lukasjhan commented Mar 3, 2024

lukasjhan commented Mar 3, 2024 • edited Loading

TimoGlastra left a comment

Choose a reason for hiding this comment

cre8 commented Mar 4, 2024

TimoGlastra commented Mar 6, 2024

cre8 commented Mar 6, 2024

TimoGlastra commented Mar 7, 2024

TimoGlastra commented Mar 8, 2024

lukasjhan commented Mar 8, 2024

TimoGlastra commented Mar 8, 2024

lukasjhan commented Mar 12, 2024 • edited Loading

TimoGlastra commented Mar 13, 2024

lukasjhan commented Mar 14, 2024

TimoGlastra commented Mar 18, 2024

TimoGlastra commented Mar 18, 2024

lukasjhan commented Mar 18, 2024

TimoGlastra commented Mar 18, 2024

lukasjhan commented Mar 18, 2024

lukasjhan commented Mar 19, 2024

TimoGlastra 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

TimoGlastra commented Apr 12, 2024

TimoGlastra commented Apr 12, 2024

lukasjhan commented Apr 12, 2024

TimoGlastra commented Apr 12, 2024

lukasjhan commented Apr 12, 2024

TimoGlastra commented Apr 24, 2024

lukasjhan commented Apr 24, 2024

lukasjhan commented Apr 24, 2024

lukasjhan commented Mar 3, 2024 •

edited

Loading

lukasjhan commented Mar 12, 2024 •

edited

Loading