feat: es crypto features to nodejs and browser #106

lukasjhan · 2024-02-25T04:03:25Z

I implement

Ed25519 and ES256 signer and verifier for crypto-nodejs
ES256 signer and verifier for crypto-browser (not tested)

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

codecov · 2024-02-25T04:05:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.54%. Comparing base (e105ef2) to head (b0a453e).
Report is 4 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #106      +/-   ##
==========================================
+ Coverage   94.96%   95.54%   +0.57%     
==========================================
  Files          21       21              
  Lines        1529     1705     +176     
  Branches      235      246      +11     
==========================================
+ Hits         1452     1629     +177     
  Misses         72       72              
+ Partials        5        4       -1

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

lukasjhan · 2024-02-25T09:33:28Z

hmm.. I have no idea how to test browser crypto subtle module. Any ideas?

cre8 · 2024-02-25T12:03:43Z

hmm.. I have no idea how to test browser crypto subtle module. Any ideas?

It depends on the test framework and I read that vitest with jsdom does not support the crypto apis... I tested it manually by running a crypress test with angular because then it is using a chrome instance in the background

lukasjhan · 2024-02-25T12:10:27Z

@cre8 Yeah, we definitely use some framework that use headless chrome(or other browser)

How about trying this? https://playwright.dev/

cre8 · 2024-02-25T12:14:00Z

@cre8 Yeah, we definitely use some framework that use headless chrome(or other browser)

How about trying this? https://playwright.dev/

I can take a look at this

lukasjhan · 2024-02-25T12:17:25Z

@cre8 Yeah, we definitely use some framework that use headless chrome(or other browser)
How about trying this? https://playwright.dev/

I can take a look at this

Thank you :)

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

lukasjhan · 2024-02-25T14:55:22Z

I want to support Ed25519 and ES256 as JWK but,

browser - doesn't seem to support Ed25519 natively.
node - bad jwk support

I turned this PR to draft and keep working on it.

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

lukasjhan · 2024-02-25T15:09:34Z

In nodejs I used webcrypto

but it is only support node >= 20. I tested in node 18 it failed.

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

lukasjhan · 2024-02-25T15:28:18Z

@cre8 Looks like test are working well ;)

cre8 · 2024-02-25T15:32:13Z

@lukasjhan I think we need to support ES256 since it's the only algorithm supported by both platforms. Of course we can implement dozens of others, but in the first place we should have one algorithm people can use in both environments to sign and verify credentials.

Are you sure with the support? According to the docs webcrypto is supported since v15.

Signed-off-by: Mirko Mollik <[email protected]>

cre8 · 2024-02-25T21:24:11Z

@lukasjhan I implemented a matrix check in #111 and it allowed to run the node test on version 16, 18 and 20 (more details in the last PR message).

To update the errors in the test, please add the same skip conditions as I have done with the others. I am not sure if we can skip it by wrapping it in a if statement or just return a standard test so the file is not empty.

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

lukasjhan · 2024-02-26T01:12:30Z

@cre8 I applied your matrix check :)
I remove Ed25519 in nodejs-crypto package. Like you said, At first let's supporting ES256

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

cre8

LGTM

cre8 · 2024-02-25T09:00:39Z

packages/browser-crypto/src/crypto.ts

+    );
+
+    // Convert to PEM format
+    const publicKey = spkiToPEM(exportedPublicKey);


Is PEM the correct way to go? I would suggest to use JWKs.

cre8 · 2024-02-25T09:01:32Z

packages/node-crypto/src/crypto.ts

+  generateKeyPair: () => {
+    return generateKeyPairSync('ed25519', {
+      publicKeyEncoding: {
+        type: 'spki', // Recommended format for public key


Same like browser, using JWK

cre8 · 2024-02-25T09:03:36Z

packages/node-crypto/src/crypto.ts

+  createPrivateKey,
+  createPublicKey,
+  generateKeyPairSync,
+} from 'crypto';


When we use the available methods from import { subtle } from 'node:crypto';, the function code is identical with the browser one and we would have more reusable code.

See: https://nodejs.org/api/webcrypto.html

cre8 · 2024-02-25T15:34:38Z

packages/node-crypto/src/crypto.ts

+  exportPKCS8,
+  exportSPKI,
+  KeyLike,
+} from 'jose';


Is jose still required? @lukasjhan I don't see any usage of the imported methods here

cre8 · 2024-02-25T15:35:15Z

packages/node-crypto/src/test/crypto.spec.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from 'vitest';
-import { generateSalt, digest } from '../crypto';
+import { generateSalt, digest, ES256, Ed25519 } from '../index';

 describe('This file is for utility functions', () => {
  test('crypto', () => {


since we included tests, we can delete this dummy test, can't we?

cre8 · 2024-02-26T14:45:04Z

packages/node-crypto/package.json

@@ -18,6 +18,9 @@
    "test:node": "vitest run ./src/test/*.spec.ts",
    "test:cov": "vitest run --coverage"
  },
+  "dependencies": {
+    "jose": "^5.2.2"


do we need this here?

…#106) Signed-off-by: Lukas.J.Han <[email protected]> Signed-off-by: Mirko Mollik <[email protected]> Signed-off-by: Lukas <[email protected]> Co-authored-by: Mirko Mollik <[email protected]>

Signed-off-by: Lukas.J.Han <[email protected]> Signed-off-by: Mirko Mollik <[email protected]> Signed-off-by: Lukas <[email protected]> Co-authored-by: Mirko Mollik <[email protected]>

lukasjhan added 2 commits February 25, 2024 13:01

feat: add crypto-nodejs Ed25519 and ES256

a77a827

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

feat: add ES256 in browser-crypto

8283205

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

lukasjhan added the feature New functionality. label Feb 25, 2024

lukasjhan self-assigned this Feb 25, 2024

lukasjhan requested review from zustkeeper, berendsliedrecht and cre8 as code owners February 25, 2024 04:03

cre8 self-assigned this Feb 25, 2024

lukasjhan added 3 commits February 25, 2024 21:36

Merge remote-tracking branch 'upstream/next' into dev/es-crypto

395b1d5

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

feat: revise crypto key use JWK

6d886f7

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

test: add browser crypto tests

7cd6d67

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

lukasjhan marked this pull request as draft February 25, 2024 14:53

lukasjhan added 4 commits February 25, 2024 23:55

wip: fixing node crypto

b7b2e86

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

feat: fix node-crypto with webcrypto

c89823c

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

test: add test for node-crypto

164455e

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

fix typo

600f1fe

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

lukasjhan marked this pull request as ready for review February 25, 2024 15:08

ci: fix

1c97db9

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

Merge branch 'next' into dev/es-crypto

8d6b17e

Signed-off-by: Mirko Mollik <[email protected]>

cre8 closed this Feb 25, 2024

cre8 reopened this Feb 25, 2024

lukasjhan added 4 commits February 26, 2024 10:00

test: skip test if nodejs version under 20

b2edbc9

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

chore: remove unused dep

ac25395

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

test: fix node compatible

d134189

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

chore: remove Ed25519 in nodejs

4cfbbc7

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

chore: remove jose dep

b0a453e

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

cre8 approved these changes Feb 26, 2024

View reviewed changes

cre8 merged commit 2d8206e into openwallet-foundation:next Feb 26, 2024
10 checks passed

cre8 mentioned this pull request Feb 28, 2024

Proposal: add crypto implementations as optional import #66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: es crypto features to nodejs and browser #106

feat: es crypto features to nodejs and browser #106

lukasjhan commented Feb 25, 2024

codecov bot commented Feb 25, 2024 •

edited

Loading

lukasjhan commented Feb 25, 2024

cre8 commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

cre8 commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

cre8 commented Feb 25, 2024

cre8 commented Feb 25, 2024 •

edited

Loading

lukasjhan commented Feb 26, 2024

cre8 left a comment

cre8 Feb 25, 2024

cre8 Feb 25, 2024

cre8 Feb 25, 2024

cre8 Feb 25, 2024

cre8 Feb 25, 2024

cre8 Feb 26, 2024

feat: es crypto features to nodejs and browser #106

feat: es crypto features to nodejs and browser #106

Conversation

lukasjhan commented Feb 25, 2024

codecov bot commented Feb 25, 2024 • edited Loading

Codecov Report

lukasjhan commented Feb 25, 2024

cre8 commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

cre8 commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

lukasjhan commented Feb 25, 2024

cre8 commented Feb 25, 2024

cre8 commented Feb 25, 2024 • edited Loading

lukasjhan commented Feb 26, 2024

cre8 left a comment

Choose a reason for hiding this comment

cre8 Feb 25, 2024

Choose a reason for hiding this comment

cre8 Feb 25, 2024

Choose a reason for hiding this comment

cre8 Feb 25, 2024

Choose a reason for hiding this comment

cre8 Feb 25, 2024

Choose a reason for hiding this comment

cre8 Feb 25, 2024

Choose a reason for hiding this comment

cre8 Feb 26, 2024

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2024 •

edited

Loading

cre8 commented Feb 25, 2024 •

edited

Loading