Merging Process #1 from sd-jwt-ts #60

lukasjhan · 2024-02-12T11:28:44Z

I'm working on merging with https://github.com/berendsliedrecht/sd-jwt-ts which is berend's work.
Due to the huge PR, I'm going to add sd-jwt-vc & splitting into multiple package in next PR.

What's included

Remove jose
Remove node, broswer crypto dependencies
Include pure js base64 package
refactor interface for crypto function injection. (mostly followed from sd-jwt-ts package)
Include pure js sha256 function from '@noble/hash'
Fix tests & examples
Remove default instance in index.ts

…e64url-encoded. (openwallet-foundation-labs#57)" This reverts commit 025786b. Signed-off-by: Lukas.J.Han <[email protected]>

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

lukasjhan · 2024-02-12T11:29:32Z

package.json

@@ -33,11 +33,13 @@
    "@types/jest": "^29.5.11",
    "@types/node": "^20.10.2",
    "jest": "^29.7.0",
+    "jose": "^5.1.3",


jose is here for testing purpose

It's devdependencies

lukasjhan · 2024-02-12T11:31:23Z

src/type.ts

 export type Signer = (data: string) => OrPromise<string>;
 export type Verifier = (data: string, sig: string) => OrPromise<boolean>;
-export type Hasher = (data: string) => Promise<string>;
-export type SaltGenerator = (length: number) => string;
+export type Hasher = (data: string, alg: string) => OrPromise<Uint8Array>;
+export type SaltGenerator = (length: number) => OrPromise<string>;
+export type HasherAndAlg = {
+  hasher: Hasher;
+  alg: string;
+};


Mostly I followed the type from sd-jwt-ts

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

berendsliedrecht

Have to get a bit more familiar with the code base but these seem like good changes! Some minor code style comments only.

berendsliedrecht · 2024-02-12T15:27:51Z

package.json

    "ts-jest": "^29.1.1",
    "ts-node": "^10.9.1",
    "typescript": "^5.3.2"
  },
  "dependencies": {
-    "jose": "^5.1.3"
+    "@noble/hashes": "1.0.0",
+    "js-base64": "^3.7.6"


nice! Better solution IMO compared to using buffer.

berendsliedrecht · 2024-02-12T15:29:48Z

src/base64url.ts

+const decode = (input: string): string => {
+  return Base64.decode(input);
 };


Suggested change

const decode = (input: string): string => {

return Base64.decode(input);

};

const decode = (input: string): string => Base64.decode(input);

Can also do const decode = Base64.decode; but there might be some overloading that we do not want to re-expose.

That's right. But this was created for consistency in case anyone gets confused with base64.

I think it can be reduced a bit by not wrapping it in a Base64Url object. Please check the change and tell me what you think :) @berendsliedrecht

Fix looks good!

berendsliedrecht · 2024-02-12T15:30:36Z

src/index.ts

+    if (!this.userConfig.kbSigner) {
+      throw new SDJWTException('Key Binding Signer not found');
+    }
+    if (!this.userConfig.kb_sign_alg) {


Why is the casing of this.userConfig.kb_sign_alg different?

berendsliedrecht · 2024-02-12T15:30:54Z

src/index.ts

+      throw new SDJWTException('SaltGenerator not found');
+    }
+
+    if (!this.userConfig.sign_alg) {


Same here re the casing.

berendsliedrecht · 2024-02-12T15:31:29Z

src/sdjwt.ts

-  disclosureFrame?: DisclosureFrame<T>,
-  hasher: Hasher = hash,
-  saltGenerator: SaltGenerator = generateSalt,
+  disclosureFrame: DisclosureFrame<T> | undefined,


Suggested change

disclosureFrame: DisclosureFrame<T> | undefined,

disclosureFrame?: DisclosureFrame<T>,

It can't be optional because its not the last parameter.

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

lukasjhan · 2024-02-13T09:32:04Z

in sha256 function the text to uint8array method only works in ASCII char. we should find another way to convert.

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

lukasjhan · 2024-02-13T09:49:23Z

in sha256 function the text to uint8array method only works in ASCII char. we should find another way to convert.

I fixed the issue

berendsliedrecht · 2024-02-14T09:21:43Z

src/test/sha256.spec.ts

+    expect(s1).toStrictEqual(s2);
+  });
+
+  test('test#13', async () => {


Isn't this a duplicate test with test#14?

Yes It's dup. my mistake

@berendsliedrecht removed.

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

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

lukasjhan added 19 commits February 11, 2024 20:31

Revert "fix: the bytes of the output of the hash function must be bas…

d55879c

…e64url-encoded. (openwallet-foundation-labs#57)" This reverts commit 025786b. Signed-off-by: Lukas.J.Han <[email protected]>

feat: migrate base64url

d611fa7

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

fix: restore tests

061c3d2

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

feat: fix decoy and disclosure

c0abd2e

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

feat: fix digest of encoded disclosure

cf7784a

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

chore: Apply prettier

c17a886

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

feat: Add test for utin8array encoding

716b8c7

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

feat: Remove jose in jwt sign and verify

4c15ffc

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

feat: Remove jose in types

b29e314

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

feat: Fix sdjwt tests

915278b

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

feat: implement new interface

837b193

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

feat: Fix e2e tests and type

9a375a8

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

feat: Fix index tests

4f7337d

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

refactor: using constant

2b2874d

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

feat: add tests for crypto functions

da5b16e

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

feat: Add default sha256 function

d6b1285

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

feat: fix sign_alg, hash_alg config

4511026

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

feat: fix examples

162585c

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

doc: Add dependencies

0168470

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

lukasjhan self-assigned this Feb 12, 2024

lukasjhan requested review from zustkeeper and berendsliedrecht as code owners February 12, 2024 11:28

lukasjhan commented Feb 12, 2024

View reviewed changes

doc: add decoding example

ce0aacc

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

berendsliedrecht previously approved these changes Feb 12, 2024

View reviewed changes

refactor: Remove base64 function overloading

097c4a3

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

lukasjhan dismissed berendsliedrecht’s stale review via 097c4a3 February 13, 2024 02:51

refactor: unify variable convention

25d0ef0

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

lukasjhan requested a review from berendsliedrecht February 13, 2024 04:52

Merge branch 'main' into dev/merge1

613f4fe

feat: Add utf8 encode function and tests

3b34b0c

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

zustkeeper previously approved these changes Feb 14, 2024

View reviewed changes

berendsliedrecht reviewed Feb 14, 2024

View reviewed changes

test: Remove dup test

fa6e0d7

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

lukasjhan dismissed zustkeeper’s stale review via fa6e0d7 February 14, 2024 09:44

berendsliedrecht approved these changes Feb 14, 2024

View reviewed changes

lukasjhan merged commit dcb76ce into openwallet-foundation-labs:main Feb 14, 2024
2 checks passed

lukasjhan mentioned this pull request Feb 15, 2024

Proposal: add crypto implementations as optional import #66

Closed

cre8 pushed a commit that referenced this pull request Mar 8, 2024

Merging Process #1 from sd-jwt-ts (#60)

3692498

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging Process #1 from sd-jwt-ts #60

Merging Process #1 from sd-jwt-ts #60

lukasjhan commented Feb 12, 2024

lukasjhan Feb 12, 2024

lukasjhan Feb 12, 2024

lukasjhan Feb 12, 2024

berendsliedrecht left a comment

berendsliedrecht Feb 12, 2024

berendsliedrecht Feb 12, 2024

lukasjhan Feb 13, 2024

berendsliedrecht Feb 14, 2024

berendsliedrecht Feb 12, 2024

berendsliedrecht Feb 12, 2024

berendsliedrecht Feb 12, 2024

lukasjhan Feb 13, 2024

lukasjhan commented Feb 13, 2024

lukasjhan commented Feb 13, 2024

berendsliedrecht Feb 14, 2024

lukasjhan Feb 14, 2024

lukasjhan Feb 14, 2024

	disclosureFrame: DisclosureFrame<T> \| undefined,
	disclosureFrame?: DisclosureFrame<T>,

Merging Process #1 from sd-jwt-ts #60

Merging Process #1 from sd-jwt-ts #60

Conversation

lukasjhan commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berendsliedrecht 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

lukasjhan commented Feb 13, 2024

lukasjhan commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment