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(sd-jwt-vc): Module for Issuer, Holder and verifier #1607

Merged
merged 27 commits into from
Nov 3, 2023

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Oct 17, 2023

Module for interacting with SD-JWTs as an issuer, holder and verifier.

The module is called SdJwtModule, however since we only deal with SdJwtVc I think it should be renamed to SdJwtVcModule.

As an issuer you get the SdJwtModule.create function which allows you to create an sd-jwt-vc where the framework sets all the required properties (like iat). It returns a record and a compact form of the sd-jwt-vc. Since key binding is required in sd-jwt-vc, a holderDidUrl is required for creation, which will be turned into a JWK and embedded in the payload as a cnf claim.

The holder gets the SdJwtModule.receive and SdJwtModule.present. The receive function simply validates that the signature is correct and the correct holder key is used. The present method allows a user to remove certain items from the sd-jwt-vc. The present method returns a compact format of the sd-jwt-vc which can be send to the verifier.

The verifier gets the SdJwtModule.verify method. This checks the keybinding, issuer signature and whether all the required fields are included.

Note: The module also exposes some repository functionality as we do not have a generic repository module, yet.

packages/core/src/crypto/jose/jwk/transform.ts Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtService.ts Outdated Show resolved Hide resolved
}
}

public getTags() {
Copy link
Contributor

Choose a reason for hiding this comment

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

No tags at all will make it especially hard to query the credentials for input in a VP...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I was wondering what the best tag structure would be for this. Right now I added the disclosure keys as an array, but I am not sure whether that is supported within askar.

Will likely also include the claimKeys, combined with the selectively disclosable keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add arrays. It will currently be added as an <attr>:<value>: 1/0 I think. So disclosureKeys:age: 1. Maybe askar also supports something for this natively, which we maybe should look into

Copy link
Contributor Author

@berendsliedrecht berendsliedrecht Nov 2, 2023

Choose a reason for hiding this comment

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

Tags will be left as-is for now. Once we do the integration with openid they can be added as at that point we will know exactly how to query.

yarn.lock Outdated Show resolved Hide resolved
packages/sd-jwt/package.json Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

Merging #1607 (c95fe6a) into main (1785479) will increase coverage by 0.00%.
The diff coverage is 86.38%.

@@           Coverage Diff            @@
##             main    #1607    +/-   ##
========================================
  Coverage   85.66%   85.67%            
========================================
  Files         950      959     +9     
  Lines       22879    23082   +203     
  Branches     4022     4049    +27     
========================================
+ Hits        19600    19775   +175     
- Misses       3095     3123    +28     
  Partials      184      184            
Files Coverage Δ
packages/core/src/crypto/jose/jwk/Jwk.ts 80.00% <100.00%> (ø)
packages/core/src/crypto/jose/jwk/index.ts 100.00% <100.00%> (ø)
packages/core/src/index.ts 100.00% <100.00%> (ø)
...odules/generic-records/repository/GenericRecord.ts 100.00% <ø> (ø)
...ages/openid4vc-client/src/OpenId4VcClientModule.ts 100.00% <ø> (ø)
packages/sd-jwt-vc/src/SdJwtVcError.ts 100.00% <100.00%> (ø)
packages/sd-jwt-vc/src/SdJwtVcModule.ts 100.00% <100.00%> (ø)
...ckages/sd-jwt-vc/src/__tests__/sdjwtvc.fixtures.ts 100.00% <100.00%> (ø)
packages/sd-jwt-vc/src/index.ts 100.00% <100.00%> (ø)
packages/sd-jwt-vc/src/repository/SdJwtVcRecord.ts 100.00% <100.00%> (ø)
... and 5 more


export type SdJwtReceiveOptions = {
issuerDid: Key
holderKey?: Key
Copy link
Contributor

Choose a reason for hiding this comment

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

These will both already be in the JWT right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to issuerKey and holderkey which are used to confirm the signature and check whether the cnf claim is correct. If this info is invalid the sd-jwt cannot be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now using issuerDidUrl and holderDidUrl.

packages/sd-jwt/src/SdJwtOptions.ts Outdated Show resolved Hide resolved
@berendsliedrecht
Copy link
Contributor Author

berendsliedrecht commented Oct 27, 2023

Somehow I can't reply to your comment...

Why does the issuer need to provide both an issuerDid and an issuerKey? I think with other places we've required to pass a key reference within the did then (did:key:123#123) and based on that we can get the issuerDid and issuerKey.

Usually I try to avoid two params that can clash (as you can provide a key that doesn't belong to the did). We can call the parameter the keyId (or something like that, forgot what it's called) and also add it to the header of the JWT

Yeah I was thinking about this for a bit and a did with key reference would be good. I did not do it like that for now as you would need to resolve your own did. I will add it where you have to pass a did with key reference. It will make the method slower though.

@TimoGlastra
Copy link
Contributor

Somehow I can't reply to your comment...

Why does the issuer need to provide both an issuerDid and an issuerKey? I think with other places we've required to pass a key reference within the did then (did🔑123#123) and based on that we can get the issuerDid and issuerKey.

Usually I try to avoid two params that can clash (as you can provide a key that doesn't belong to the did). We can call the parameter the keyId (or something like that, forgot what it's called) and also add it to the header of the JWT

Yeah I was thinking about this for a bit and a did with key reference would be good. I did not do it like that for now as you would need to resolve your own did. I will add it where you have to pass a did with key reference. It will make the method slower though.

I think that is fine for now. We have this in a few other places, where we resolve our own did. But I thin we should solve this generically by allowing the did to be retrieved from local storage / cache.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Nice work @berendsliedrecht. Mostly some small improvements

packages/core/src/crypto/jose/jwk/Jwk.ts Show resolved Hide resolved
packages/core/src/crypto/jose/jwk/transform.ts Outdated Show resolved Hide resolved
packages/sd-jwt/README.md Outdated Show resolved Hide resolved
packages/sd-jwt/package.json Outdated Show resolved Hide resolved
packages/sd-jwt/package.json Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtOptions.ts Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtOptions.ts Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtOptions.ts Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtService.ts Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtService.ts Outdated Show resolved Hide resolved
@berendsliedrecht
Copy link
Contributor Author

@karimStekelenburg @TimoGlastra Quickly before merging, should the module be called SdJwtVc or SdJwt? It is SdJwt right now but we only really handle SdJwtVc.

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Copy link
Contributor

@karimStekelenburg karimStekelenburg left a comment

Choose a reason for hiding this comment

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

Looking good overall. I left a few comments here and there, but they're mostly suggestions. I only see happy flow tests though. Doesn't have to bne noew, but I think we should add sad flow tests when we combine it with OID4VC.

})
})

describe('SdJwtService.present', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test where the holder tries to disclose a non-disclosable value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point regarding the non-happy flow tests. I would like to add them in a separate PR to keep this one "small".

packages/sd-jwt/src/SdJwtOptions.ts Outdated Show resolved Hide resolved
packages/sd-jwt/src/SdJwtApi.ts Outdated Show resolved Hide resolved
packages/sd-jwt/package.json Outdated Show resolved Hide resolved
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

I like the simplicity of the API, seems to be very fun to use!

Just left a comment regarding an old discussion that we never completely materialized in code (maybe for a further breaking change PR now we are introducing a new major version).

return await this.sdJwtService.getAllCredentialRecords(this.agentContext)
}

public async findCredentialRecordsByQuery(query: Query<SdJwtRecord>): Promise<Array<SdJwtRecord>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the convention used in other module APIs (and services in general) we'd call it findAllCredentialRecordsByQuery, to make more explicit about the fact that we expect multiple records to be retrieved. Actually, in most modules we make naming simpler (e.g. findAllByQuery), but I see it's not the case here and W3cCredentialsApi (the API this module is based on I guess).

Of course if we change this here and leave W3cCredentialsApi as is, it would not be consistent. And if we change it in W3cCredentialsApi as well we'll introduce a breaking change. But I just wanted to point out this to bother you 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point! I think we can change this in a separate PR which changes both the w3c and this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually not sure what the best name would be here. I think findAllByQuery is the best as it is already scoped under sdJwt within the agent.modules api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think all of them to findById, findAllByQuery. They will be removed in the future and I think they were too verbose before.

config: { label, walletConfig: { id: utils.uuid(), key: utils.uuid() } },
modules: {
sdJwt: new SdJwtModule(),
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the problem here that required a ts-ignore? It's about not using askar-nodejs as direct dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expected ariesAskar type did not match the one from the module as I think it used 0.2.0-dev.1 and I used 0.1.0. I think I can remove it again

@@ -0,0 +1,3 @@
import 'reflect-metadata'

jest.setTimeout(120000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect SD-JWT to take that long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from another test. It is quite quick, but askar and agent initialization makes some steps a bit slow. Better overestimate here I guess ;).

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
@berendsliedrecht berendsliedrecht changed the title feat(sd-jwt): Module for Issuer, Holder and verifier feat(sd-jwt-vc): Module for Issuer, Holder and verifier Nov 2, 2023
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
@berendsliedrecht berendsliedrecht merged commit ec3182d into openwallet-foundation:main Nov 3, 2023
8 checks passed
@berendsliedrecht berendsliedrecht deleted the sd-jwt branch November 3, 2023 11:10
auer-martin pushed a commit to auer-martin/aries-framework-javascript that referenced this pull request Nov 15, 2023
…oundation#1607)

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Martin Auer <martin.auer97@gmail.com>
amitpadmani-awts pushed a commit to ayanworks/aries-framework-javascript that referenced this pull request Nov 29, 2023
* fix: listen to incoming messages on agent initialize not constructor (openwallet-foundation#1542)

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>

* fix(cheqd): make cosmos payer seed optional (openwallet-foundation#1547)

Signed-off-by: Timo Glastra <timo@animo.id>

* fix: create message subscription first (openwallet-foundation#1549)

Signed-off-by: Timo Glastra <timo@animo.id>

* fix(transport): Use connection in WebSocket ID (openwallet-foundation#1551)

* fix: priority sorting for didcomm services (openwallet-foundation#1555)

Signed-off-by: Timo Glastra <timo@animo.id>

* fix: bump missing dependencies version (openwallet-foundation#1557)

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

* chore(release): v0.4.1 (openwallet-foundation#1548)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix: do not send package via outdated session (openwallet-foundation#1559)

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>

* build(deps): bump @types/indy-sdk from 1.16.26 to 1.16.27 (openwallet-foundation#1564)

Signed-off-by: dependabot[bot] <support@github.com>

* fix(cheqd): changed the name formatting to a encoded hex value (openwallet-foundation#1574)

Signed-off-by: Tom Lanser <tom@animo.id>

* build(deps): bump @cheqd/ts-proto from 2.2.0 to 2.2.2 (openwallet-foundation#1568)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): bump @cosmjs/proto-signing from 0.31.0 to 0.31.1 (openwallet-foundation#1566)

Signed-off-by: dependabot[bot] <support@github.com>

* fix(core): remove node-fetch dependency (openwallet-foundation#1578)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: log and throw on WebSocket sending errors (openwallet-foundation#1573)

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

* fix(oob): support oob with connection and messages (openwallet-foundation#1558)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat: allow connection invitation encoded in oob url param (openwallet-foundation#1583)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: duplicate service ids in connections protocol (openwallet-foundation#1589)

* fix: implicit invitation to specific service (openwallet-foundation#1592)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix(askar): throw error if imported wallet exists (openwallet-foundation#1593)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: service validation in OOB invitation objects (openwallet-foundation#1575)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: update tsyringe for ts 5 support (openwallet-foundation#1588)

Signed-off-by: Timo Glastra <timo@animo.id>

* chore(release): v0.4.2 (openwallet-foundation#1591)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix: save AnonCredsCredentialRecord createdAt (openwallet-foundation#1603)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat: sped up lookup for revocation registries (openwallet-foundation#1605)

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>

* build(deps): bump @types/express from 4.17.17 to 4.17.18 (openwallet-foundation#1596)

Bumps [@types/express](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/express) from 4.17.17 to 4.17.18.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/express)

---
updated-dependencies:
- dependency-name: "@types/express"
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump actions/checkout from 3 to 4 (openwallet-foundation#1599)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump amannn/action-semantic-pull-request from 5.2.0 to 5.3.0 (openwallet-foundation#1598)

build(deps): bump amannn/action-semantic-pull-request

Bumps [amannn/action-semantic-pull-request](https://github.com/amannn/action-semantic-pull-request) from 5.2.0 to 5.3.0.
- [Release notes](https://github.com/amannn/action-semantic-pull-request/releases)
- [Changelog](https://github.com/amannn/action-semantic-pull-request/blob/main/CHANGELOG.md)
- [Commits](amannn/action-semantic-pull-request@v5.2.0...v5.3.0)

---
updated-dependencies:
- dependency-name: amannn/action-semantic-pull-request
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: abandon proof protocol if presentation fails (openwallet-foundation#1610)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat!: upgrade shared components (openwallet-foundation#1606)

* feat: update dockerfile to node 18 and sample mediator to askar (openwallet-foundation#1622)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat(sd-jwt-vc): Module for Issuer, Holder and verifier (openwallet-foundation#1607)

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>

* feat(indy-vdr): ability to refresh the pool manually (openwallet-foundation#1623)

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>

* build(deps): bump @babel/traverse from 7.21.4 to 7.23.2 (openwallet-foundation#1608)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): bump uuid and @types/uuid (openwallet-foundation#1597)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): use node's built-in fetch (openwallet-foundation#1631)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* build(deps): bump react-devtools-core from 4.27.6 to 4.28.5 (openwallet-foundation#1630)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): bump @mattrglobal/bbs-signatures from 1.1.0 to 1.3.1 (openwallet-foundation#1601)

Signed-off-by: dependabot[bot] <support@github.com>

* fix: some log messages (openwallet-foundation#1636)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat: bump indy-vdr version (openwallet-foundation#1637)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix(core): allow string for did document controller (openwallet-foundation#1644)

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

* feat(anoncreds): issue revocable credentials (openwallet-foundation#1427)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* refactor(anoncreds)!: move supportRevocation to options (openwallet-foundation#1648)

Signed-off-by: Timo Glastra <timo@animo.id>

* refactor: move message-pickup directory (openwallet-foundation#1650)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

---------

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Tom Lanser <tom@animo.id>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Co-authored-by: Niall Shaw <100220424+niall-shaw@users.noreply.github.com>
Co-authored-by: Timo Glastra <timo@animo.id>
Co-authored-by: Alexander Shenshin <93187809+AlexanderShenshin@users.noreply.github.com>
Co-authored-by: ericvergnaud <eric.vergnaud@wanadoo.fr>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tom Lanser <tommy.lanser@gmail.com>
Co-authored-by: Ariel Gentile <gentilester@gmail.com>
Co-authored-by: Wade King <wkingnumber2@gmail.com>
Co-authored-by: Berend Sliedrecht <61358536+berendsliedrecht@users.noreply.github.com>
auer-martin pushed a commit to auer-martin/aries-framework-javascript that referenced this pull request Dec 4, 2023
…oundation#1607)

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Martin Auer <martin.auer97@gmail.com>
genaris pushed a commit to genaris/credo-ts that referenced this pull request Dec 4, 2023
…oundation#1607)

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
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.

None yet

5 participants