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(did-comm): add support for some of the AES based content and key encryption algorithms #1180

Merged
merged 11 commits into from
Aug 1, 2023

Conversation

mirceanis
Copy link
Member

@mirceanis mirceanis commented May 18, 2023

What issue is this PR fixing

partially fixes #1083

What is being changed

Most relevant changes are in the did-comm package, where the new capabilities of the did-jwt@7.2.0 are used to create a wider set of encrypters and logic to pick the proper decrypter too.
This PR adds support for A256KW key wrapping and for A256GCM and A256CBC-HS512 content encryption.

These can be set as options to packDIDCommMessage. Example:

const packed = agent.packDIDCommMessage({
  packing: 'authcrypt',
  message,
  enc: 'A256GCM', // default for `authcrypt` or `anoncrypt`
  alg: 'ECDH-1PU+A256KW' // default for `authcrypt`
}),

There are some corner cases that may require an API review for the packing method:
authcrypt implies ECDH-1PU, so specifying an ECDH-ES alg will get overridden.
anoncrypt implies ECDH-ES so specifying an ECDH-1PU alg will be overridden.

The only supported key agreement type for now is X25519. Besides supporting other key types, there is one more missing piece to full did-comm v2 compliance:

  • our ECDH-1PU implementation is still using the v3 draft of the spec, while the did-comm spec points to the v4 draft. The diff between the 2 versions suggests they will be incompatible because of the cctag (section 2.3)

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.

Details

This PR brings many seemingly unrelated upgrades but they are necessary due to the complicated dependency tree:

  • @stablelib/* dependencies were replaced with @noble/* where available (similar to did-jwt)

@mirceanis mirceanis requested a review from nickreynolds May 18, 2023 17:23
@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Patch coverage: 85.75% and project coverage change: +0.33% 🎉

Comparison is base (688f59d) 84.51% compared to head (777ec8f) 84.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1180      +/-   ##
==========================================
+ Coverage   84.51%   84.85%   +0.33%     
==========================================
  Files         158      167       +9     
  Lines       16661    18056    +1395     
  Branches     1829     2027     +198     
==========================================
+ Hits        14081    15321    +1240     
- Misses       2580     2735     +155     
Files Changed Coverage Δ
packages/did-comm/src/encryption/p256-ECDH-1PU.ts 0.00% <0.00%> (ø)
packages/did-comm/src/encryption/p256-ECDH-ES.ts 0.00% <0.00%> (ø)
packages/did-provider-key/src/key-did-provider.ts 77.27% <14.28%> (-3.05%) ⬇️
...s/credential-ld/src/suites/JsonWebSignature2020.ts 40.40% <35.52%> (+1.94%) ⬆️
...ata-store-json/src/identifier/private-key-store.ts 92.38% <71.42%> (-1.74%) ⬇️
...ges/data-store/src/identifier/private-key-store.ts 90.24% <83.33%> (-1.76%) ⬇️
packages/utils/src/did-utils.ts 83.72% <86.04%> (+0.88%) ⬆️
...d-comm/src/protocols/trust-ping-message-handler.ts 89.88% <86.66%> (+0.86%) ⬆️
packages/did-comm/src/utils.ts 90.37% <87.17%> (-1.71%) ⬇️
packages/did-provider-ion/src/functions.ts 94.14% <89.79%> (-1.55%) ⬇️
... and 20 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mirceanis mirceanis force-pushed the didcomm-aes-refactor branch 3 times, most recently from f9a64cd to 49a0671 Compare June 12, 2023 16:32
@mirceanis mirceanis requested a review from simonas-notcat June 13, 2023 12:37
@mirceanis mirceanis marked this pull request as ready for review June 13, 2023 12:37
@@ -167,7 +167,7 @@ describe('database initial migration tests', () => {
encoding: 'utf-8',
})
expect(signedMessage).toEqual(
'vzDocUViJh7ooOCZ-jBHKZddEsTa4yClHwhIL9SHJwjAv3bC6TZIcUnX36ZqNBWvLbnNAQvdtzqrVf3l0pv3QQ',
Copy link
Member Author

Choose a reason for hiding this comment

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

JWT signing for ES256K now uses canonical signatures, with a low S value (through the use of @noble/curves)
This eliminates signature malleability, just like what happens for ethereum signatures, but it also means tests will change.

@@ -403,7 +403,7 @@ export default (testContext: {
hello: 'world',
},
credentialStatus: {
id: 'override me',
id: 'override:me',
Copy link
Member Author

Choose a reason for hiding this comment

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

The latest @transmute libraries fail if the ID value doesn't look like an IRI

@mirceanis mirceanis force-pushed the didcomm-aes-refactor branch from 49a0671 to 777ec8f Compare August 1, 2023 14:25
@mirceanis mirceanis merged commit 5294a81 into next Aug 1, 2023
@mirceanis mirceanis deleted the didcomm-aes-refactor branch August 1, 2023 14:40
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.

[proposal] Add AES and NIST curve support to the DIDComm implementation
3 participants