Skip to content

PLT 155 - Introduce jwt signing js backend#1786

Merged
Nikpolik merged 2 commits into
mainfrom
introduce-jwt-signing-js-backend
Oct 12, 2023
Merged

PLT 155 - Introduce jwt signing js backend#1786
Nikpolik merged 2 commits into
mainfrom
introduce-jwt-signing-js-backend

Conversation

@Nikpolik

@Nikpolik Nikpolik commented Sep 27, 2023

Copy link
Copy Markdown
Contributor

Description

Added a new signJwt(payload, secret, options) function to allow us to sign tokens.

Updated the hasValidSignature(jwt, secret) method to be able to verify tokens using PEM formatted keys.

The process for signing JWT was heavily inspired from another library cloudflare-worker-jwt and its sign method.

Quick overview of process for signing JWT

  1. JSON.stringify header and payload
  2. URL encode the two strings
  3. Create the first part of the token headerEncoded.payloadEncoded
  4. Import key
  5. Signing using the algorithm provided in the options and encode signature
  6. Return the final JWT headerEncoded.payloadEncoded.signature

Changes implemented

  1. Extract isomorphicAtob to separate file to reuse for key importing.
  2. Extract jwk to crypto algorithm mapping to separate file. This is will be shared between verify and sign.
  3. Create shared importKey function that handles multiple key formats.
  4. Use new importKey function inside hasValidSignature and verifyJWT also added tests for hasValidSignature.
  5. Implement signing procedure described above.
  6. Export new function.
  7. Added tests and updated docs.

Notes

  1. Since we only use RSASSA-PKCS1-v1_5 currently only added support for importing keys formatted jwk,pkcs8,spki and not raw.
  2. ~~signJwt was prefixed with unstable since its goal is to be used internally. ~~ marked with JSDocs that will be used internally
  3. Isomorphic atob could be imported from @clerk/shared (fix(shared, nextjs): Support importing @clerk/shared in @clerk/backend #1769) Done
  4. In the cloudflare-worker-jwt implementation of the sign method, the IAT (Issued at) field in the payload is updated. I am not sure if this is something that should be handled in the signing process or when we create the payload 🤔 Changed to update IAT.
  5. verifyJWT method was not updated since it has its own implementation of hasValidSignature maybe it should use the updated one? This will allow us to use it PEM pem keys also. Fixed and removed duplicate (unused hasValidSignature)

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

@Nikpolik Nikpolik self-assigned this Sep 27, 2023
@changeset-bot

changeset-bot Bot commented Sep 27, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f47a6ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@clerk/backend Patch
@clerk/fastify Patch
gatsby-plugin-clerk Patch
@clerk/nextjs Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Nikpolik
Nikpolik force-pushed the introduce-jwt-signing-js-backend branch from 9194ca3 to b8128d8 Compare September 27, 2023 12:00
@Nikpolik
Nikpolik force-pushed the introduce-jwt-signing-js-backend branch from b8128d8 to 5f9a83c Compare September 27, 2023 12:09

@SokratisVidros SokratisVidros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Nikpolik excellent PR description!

Can you please elaborate on the comment about iat? Is this about updating the iat of the token when we compute the signature?

Comment thread packages/backend/src/shared/isomorphicAtob.ts Outdated
@Nikpolik

Nikpolik commented Sep 29, 2023

Copy link
Copy Markdown
Contributor Author

@Nikpolik excellent PR description!

Can you please elaborate on the comment about iat? Is this about updating the iat of the token when we compute the signature?

Basically overwrite the iat payload.iat = Math.floor(Date.now() / 1000) to make sure that the signed token always has the correct timestamp of when it was signed. After thinking about it more clearly I need to add.

@Nikpolik
Nikpolik force-pushed the introduce-jwt-signing-js-backend branch from 5f9a83c to 2016926 Compare October 3, 2023 08:37
@nikosdouvlis
nikosdouvlis requested review from anagstef and dimkl October 4, 2023 05:51
@Nikpolik
Nikpolik force-pushed the introduce-jwt-signing-js-backend branch 2 times, most recently from 51b07ba to 7e4189c Compare October 6, 2023 10:02
@Nikpolik
Nikpolik marked this pull request as ready for review October 6, 2023 10:05
@Nikpolik
Nikpolik requested a review from a team as a code owner October 6, 2023 10:05
Comment thread packages/backend/src/tokens/jwt/signJwt.test.ts Outdated

@georgepsarakis georgepsarakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 Nice work! Overall seems fine to me, just not approving since I'm not so well-versed in JS/TS and there might be some subtle issue with crypto I'm missing.

Comment thread packages/backend/src/tokens/fixtures.ts Outdated
export { __unstable__signJwt } from './signJwt';

export type { VerifyJwtOptions } from './verifyJwt';
export type { SignJwtOptions } from './signJwt';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ Does it make sense to prefix these options as unstable? Unlikely to be used, but only for semantics purposes.

Comment thread packages/backend/src/tokens/jwt/signJwt.test.ts Outdated
Comment thread .changeset/cold-bags-watch.md Outdated
Comment thread packages/backend/src/tokens/jwt/assertions.test.ts
Comment thread packages/backend/src/tokens/jwt/signJwt.ts Outdated
Comment thread packages/backend/src/tokens/jwt/signJwt.ts Outdated
Comment thread packages/backend/src/tokens/jwt/algorithms.ts Outdated
@dimkl

dimkl commented Oct 11, 2023

Copy link
Copy Markdown
Contributor

Really nice work! 🚀

@Nikpolik
Nikpolik force-pushed the introduce-jwt-signing-js-backend branch 3 times, most recently from b5b367e to c251845 Compare October 12, 2023 10:24
Nikos Polykandriotis added 2 commits October 12, 2023 14:28
Also extracted some functionality to scr/tokens/jwt/algorithms so it can
be reused in the future and added some spec. Removed constants and tests
for ES256, ES384, ES512
This method is marked as internal since it will be only used on other
clerk packages for now.
@Nikpolik
Nikpolik force-pushed the introduce-jwt-signing-js-backend branch from c251845 to f47a6ba Compare October 12, 2023 11:31
@Nikpolik
Nikpolik added this pull request to the merge queue Oct 12, 2023
Merged via the queue into main with commit 13e9dfb Oct 12, 2023
@Nikpolik
Nikpolik deleted the introduce-jwt-signing-js-backend branch October 12, 2023 12:14
@clerk-cookie clerk-cookie mentioned this pull request Oct 12, 2023
@clerk-cookie

Copy link
Copy Markdown
Collaborator

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@clerk clerk locked as resolved and limited conversation to collaborators Oct 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants