feat: jwt signing #1967

thepiwo · 2024-03-21T18:45:04Z

Currently based on the latest 13.2.2 release, thus the conflicts to try it easily with the current superhero wallet implementation.

related superhero wallet integration superhero-com/superhero-wallet#2875

thepiwo · 2024-03-22T15:45:43Z

examples/browser/aepp/package.json

@@ -18,6 +18,7 @@
  "devDependencies": {
    "@vue/cli-plugin-babel": "~5.0.8",
    "@vue/cli-service": "~5.0.8",
+    "postcss": "^8.4.37",


seems to be needed to start the example

thepiwo · 2024-03-25T13:12:48Z

@davidyuk any feedback from your side?

davidyuk · 2024-03-26T15:46:22Z

The motivation is unclear. Have you considered using typed data instead of JWT? Or it is not suitable for your use case?

thepiwo · 2024-03-26T15:48:31Z

JWT is nice as its a widely used standard and supported by our curve, so we could expose it (and e.g. use it in superhero product login)

davidyuk

Not sure about motivation, seems that JWTs signed by a centralized provider are more useful/compatible that self-signed JWTs.

davidyuk · 2024-03-28T02:03:37Z

src/account/Base.ts

+    message: object,
+    options?: {
+      aeppOrigin?: string;
+      expireAt?: number; // unixtime ms


I would accept it as message.exp

davidyuk · 2024-03-28T02:07:24Z

src/utils/crypto.ts

+): Promise<string> {
+  const header = { alg: 'EdDSA', typ: 'JWT' };
+  const payload = { ...message, exp: expireAt };
+  const body = `${base64url.encode(JSON.stringify(header))}.${base64url.encode(JSON.stringify(payload))}`;


buffer.toString('base64url') should work instead, it is available since node@16 nodejs/node@f8ab632

Also, you can use canonicalize (sdk already uses it in typed-data) instead JSON.stringify to avoid fluctuations of JWT because of different key order

nice, didn't know

buffer.toString('base64url') doesn't seem to work once built in the browser

davidyuk · 2024-03-28T02:08:39Z

src/utils/crypto.ts

@@ -142,6 +143,18 @@ export function sign(data: string | Uint8Array, privateKey: string | Uint8Array)
  return nacl.sign.detached(Buffer.from(data), Buffer.from(privateKey));
 }

+export async function signJWT(
+  message: object,
+  expireAt: number,


looks to be an extra argument 🤷‍♀️

davidyuk · 2024-03-28T02:14:24Z

src/utils/crypto.ts

+  message: object,
+  expireAt: number,
+  privateKey: Uint8Array,
+): Promise<string> {


it also can return

Promise<`${string}.${string}.${string}`>

davidyuk · 2024-03-28T02:16:51Z

src/account/Rpc.ts

+  override async signMessageJWT(message: object, options?: {
+    expireAt?: number;
+  }): Promise<string> {
+    const { signature } = await this._rpcClient


technically it is a payload + signature

davidyuk · 2024-03-28T02:26:58Z

src/account/Base.ts

@@ -54,6 +54,15 @@ export default abstract class AccountBase {
    },
  ): Promise<Uint8Array>;

+  abstract signMessageJWT(


Suggested change

abstract signMessageJWT(

abstract signJWT(

"Message" looks not necessary, because T already is "Token"

davidyuk · 2024-03-28T02:28:28Z

src/account/Memory.ts

+    const secretKey = secretKeys.get(this);
+    if (secretKey == null) throw new UnexpectedTsError();
+    const expireAt = options?.expireAt === undefined
+      ? new Date().getTime() + 30 * 60 * 1000 // default to 30min


Suggested change

? new Date().getTime() + 30 * 60 * 1000 // default to 30min

? Date.now() + 30 * 60 * 1000 // default to 30min

do you think its ok to keep the default expiry there?

Not really, there are different account types and I'm not sure each of them should define it. But here is not much logic to extract to "generate jwt" function. We can do nothing here and allow the wallet to handle jwt parameters (like not allowing signing a too general token).

I removed it :)

davidyuk · 2024-03-28T02:37:30Z

src/utils/crypto.ts

+  expireAt: number,
+  privateKey: Uint8Array,
+): Promise<string> {
+  const header = { alg: 'EdDSA', typ: 'JWT' };


EdDSA is a correct value

nacl.sign implements ed25519 https://www.npmjs.com/package/tweetnacl

ed25519 the same as EdDSA https://stackoverflow.com/a/66894047/6176994

davidyuk · 2024-03-28T02:42:06Z

src/utils/crypto.ts

+  const header = { alg: 'EdDSA', typ: 'JWT' };
+  const payload = { ...message, exp: expireAt };
+  const body = `${base64url.encode(JSON.stringify(header))}.${base64url.encode(JSON.stringify(payload))}`;
+  const signature = sign(body, privateKey);


Insignificant concern is that all payloads we signed before have a special not overlapping prefix.

if we add sub_jwk in the payload the risk should be even smaller

davidyuk · 2024-03-28T02:54:59Z

src/utils/crypto.ts

+  privateKey: Uint8Array,
+): Promise<string> {
+  const header = { alg: 'EdDSA', typ: 'JWT' };
+  const payload = { ...message, exp: expireAt };


Would be good to store signer address somehow in message or header. It can be

sub_jwk Public key used to check the signature of an ID Token

https://www.iana.org/assignments/jwt/jwt.xhtml

Or something own like ethereum/EIPs#1341 (Issuer field?)

thepiwo requested a review from davidyuk March 21, 2024 18:45

thepiwo self-assigned this Mar 21, 2024

thepiwo marked this pull request as draft March 21, 2024 18:45

thepiwo force-pushed the feat/jwt-signature branch from dac1175 to f41e021 Compare March 22, 2024 15:45

thepiwo commented Mar 22, 2024

View reviewed changes

feat: jwt signing

0711b7a

thepiwo force-pushed the feat/jwt-signature branch from f41e021 to 0711b7a Compare March 22, 2024 15:46

thepiwo marked this pull request as ready for review March 22, 2024 15:48

davidyuk reviewed Mar 28, 2024

View reviewed changes

thepiwo force-pushed the feat/jwt-signature branch from be6de22 to 49d1fc3 Compare March 28, 2024 13:12

chore: adjustments for PR review

4ad056b

thepiwo force-pushed the feat/jwt-signature branch from 49d1fc3 to 4ad056b Compare March 28, 2024 13:27

davidyuk mentioned this pull request Apr 5, 2024

feat: jwt utilities #1973

Merged

thepiwo closed this Apr 29, 2024

davidyuk deleted the feat/jwt-signature branch June 17, 2024 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: jwt signing #1967

feat: jwt signing #1967

thepiwo commented Mar 21, 2024 •

edited

Loading

thepiwo Mar 22, 2024

thepiwo commented Mar 25, 2024

davidyuk commented Mar 26, 2024

thepiwo commented Mar 26, 2024

davidyuk left a comment

davidyuk Mar 28, 2024

davidyuk Mar 28, 2024

davidyuk Mar 28, 2024

thepiwo Mar 28, 2024

thepiwo Mar 28, 2024

davidyuk Mar 28, 2024

davidyuk Mar 28, 2024

davidyuk Mar 28, 2024

thepiwo Mar 28, 2024

davidyuk Mar 28, 2024

davidyuk Mar 28, 2024

thepiwo Mar 28, 2024

davidyuk Mar 29, 2024

thepiwo Apr 2, 2024

davidyuk Mar 28, 2024

davidyuk Mar 28, 2024

thepiwo Mar 28, 2024

davidyuk Mar 28, 2024

thepiwo Mar 28, 2024

	? new Date().getTime() + 30 * 60 * 1000 // default to 30min
	? Date.now() + 30 * 60 * 1000 // default to 30min

feat: jwt signing #1967

feat: jwt signing #1967

Conversation

thepiwo commented Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

thepiwo commented Mar 25, 2024

davidyuk commented Mar 26, 2024

thepiwo commented Mar 26, 2024

davidyuk 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

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

thepiwo commented Mar 21, 2024 •

edited

Loading