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.

Sign up for GitHub

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

Jump to bottom

PLT 155 - Introduce jwt signing js backend #1786

Merged
merged 2 commits into from
Oct 12, 2023
Merged

PLT 155 - Introduce jwt signing js backend #1786

merged 2 commits into from
Oct 12, 2023

Conversation

Nikpolik
Copy link
Member

@Nikpolik Nikpolik commented Sep 27, 2023 •
edited by dimkl
Loading

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
Copy link

changeset-bot bot commented Sep 27, 2023 •
edited
Loading

🦋 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 reviewed Sep 28, 2023
View reviewed changes
Copy link
Contributor

@SokratisVidros SokratisVidros left a comment •
edited
Loading

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?

packages/backend/src/shared/isomorphicAtob.ts Outdated Show resolved Hide resolved
@Nikpolik
Copy link
Member Author

Nikpolik commented Sep 29, 2023 •
edited
Loading

@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 dimkl and anagstef 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
georgepsarakis
georgepsarakis reviewed Oct 10, 2023
View reviewed changes
Copy link
Contributor

@georgepsarakis georgepsarakis left a comment

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.

packages/backend/src/tokens/fixtures.ts Outdated Show resolved Hide resolved
packages/backend/src/tokens/jwt/index.ts

export type { VerifyJwtOptions } from './verifyJwt';
export type { SignJwtOptions } from './signJwt';
Copy link
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.

packages/backend/src/tokens/jwt/signJwt.test.ts Outdated Show resolved Hide resolved
nikosdouvlis
nikosdouvlis approved these changes Oct 10, 2023
View reviewed changes
.changeset/cold-bags-watch.md Outdated Show resolved Hide resolved
dimkl
dimkl approved these changes Oct 10, 2023
View reviewed changes
packages/backend/src/tokens/jwt/assertions.test.ts Show resolved Hide resolved
packages/backend/src/tokens/jwt/signJwt.ts Outdated Show resolved Hide resolved
packages/backend/src/tokens/jwt/signJwt.ts Outdated Show resolved Hide resolved
packages/backend/src/tokens/jwt/algorithms.ts Outdated Show resolved Hide resolved
@dimkl
Copy link
Contributor

dimkl commented Oct 11, 2023

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
@Nikpolik
refactor(backend): Refactor hasValidSignature to also work with PEM keys
c9cb4ca 
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
@Nikpolik
feat(backend): Add new method for signing jwt tokens
f47a6ba 
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
5 checks passed
@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
Merged
@clerk-cookie
Copy link
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.
Reviewers

@dimkl dimkl dimkl approved these changes

@SokratisVidros SokratisVidros SokratisVidros approved these changes

@georgepsarakis georgepsarakis georgepsarakis left review comments

@nikosdouvlis nikosdouvlis nikosdouvlis approved these changes

@anagstef anagstef Awaiting requested review from anagstef

Assignees

@Nikpolik Nikpolik

Labels
backend nextjs sdk-node
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Nikpolik @dimkl @clerk-cookie @georgepsarakis @SokratisVidros @nikosdouvlis