feat: use jwks to fetch public keys #113

littlemight · 2025-02-14T01:48:25Z

Problem

Tech debt, public key is hardcoded in the SDK. If our signing keys are compromised, rotation would be such a big hassle. Modifying the SDK to fetch the keys from an publicly accessible JWKS endpoint instead.

Closes FRM-876
Hopefully closes #53, #74, #75, #84.

Reference

RFC for JWKS standard
RFC for representing Ed25519 keys
These are not hard guides to follow, as we can freely define our own JWKS schema that's convenient for us. It's just a general guard rails and starting point.

Solution

All existing public keys are hosted on a public jwks endpoint. During SDK init it will fetch and cache, and re-fetched whenever cache goes stale. This should also help reducing manual intervention during key rotation.

Features:

Exit criteria should be

[~] Sane migration guide
Keys from JWKS will take precedence over hardcoded configs
On failure, hardcoded keys from file/env will be used as fallback
Each of the modules should have their public key refetched instead of using the previously hardcoded value during module init
During rotation, and array of keys instead will be used. SDK will then try till it finds a key that works.
Alternatively, we identify via key id (kid)

Improvements:

Configurable timeout when fetching JWKS
Configurable cache duration
A sane upper bound for cache duration to prevent stale keys living too long?
I'm not considering node-cache for caching, as it is unmaintained.

Bug Fixes:

Details ...

Before & After Screenshots

BEFORE:
[insert screenshot here]

AFTER:
[insert screenshot here]

Tests

What tests should be run to confirm functionality?
~~Unit tests... which I haven't written~~ 🤡
Unit and integration tests.

Deploy Notes

Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.

New environment variables:

env var : env var details

New scripts:

script : script details

New dependencies:

axios-retry: lightweight lib to configure axios client for plentiful retry options like exp backoff

New dev dependencies:

nock: mocking JWKS server in integration tests

linear · 2025-02-14T02:15:13Z

FRM-876 Upgrade FormSG SDK to allow public key rotation

littlemight · 2025-02-15T05:06:58Z

braindump;
There's two ways we could populate value of the keys

at bootstrap, fetch from jwks then just pass it as static value
pass the public key getters to the modules (Crypto, Webhook, Verification)

1 favors simplicity, but in the case of key rotation, a redeployment need to happen.
2 is robust, as in keys are fetched lazily and stale keys eventually get overriden in the case of key rotation.

however, due to the nature of fetching the keys (async requests), a bunch of fns that weren't async are now async. changing the function signatures a bit and thus, a bit more work for people bumping the sdk to adjust to this version.

I think 2 should be the play, given the robustness and zero-downtime nature during rotation.

KenLSM · 2025-02-17T13:12:51Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "@opengovsg/formsg-sdk",
-  "version": "0.12.0",
+  "version": "0.14.0-alpha.0",


/nit because we're changing the function definition (from sync to async), I think this warrants a bump on the major version to 1.0.0 (can declare that as 1.0.0-alpha.0 or 1.0.0-rc.0).

We'll need a changelog with migration notes as well, this can be done later! 🙏

definitely!
migration notes as in smth like MIGRATION.md, form guide, or both?

MIGRATION.md like storybook or better

KenLSM · 2025-02-17T13:18:23Z

src/crypto.ts

@@ -87,7 +91,15 @@ export default class Crypto extends CryptoBase {
      }

      if (verifiedContent) {
-        if (!this.signingPublicKey) {
+        if (!this.getSigningPublicKey) {


Can this be done on the constructor instead? The crypto class should not be instantiated if there's no function provided. This allows us to throw the error earlier rather than later.

I was doing exactly this in the earlier commits! but then it broke this test, where it allows class instantiation without a public key but "disallows" certain function being called (....by throwing an error).

I assume it's a tradeoff made at the time? So to keep behaviour backward compatible I decided not too mess around with that area of the code.

But I feel this is somewhat a confusing pattern, as every potential downstream method of the class have to be wary of that property.

Anyways since we're jumping to a major version already (from 0.13.0 -> 1.0), we can consider changing this particular behaviour and adjusting that test :p

Agreed. I feel that it doesn't really make sense to allow incomplete instantiation resulting in downstream function having to cleanup.

paiseh just got time to dedicate wrapping up work on this PR.

@KenLSM just had a revelation why it's coded this way - SDK is used in 3 places:

Form frontend: admin page, for decrypting responses. Doesn't need public signing key.

Form backend: for crafting signatures andallthat. Doesn't need public signing key.

Webhook consumer: for verifying said signatures. Need public signing key.

So the same class is conditionally instantiated for 2 different usecases, which in nature requires different prerequisite.

Let me think the best way to approach this, if the pursuit of cleanliness ends up introducing a fat refactor on the main form codebase then I would defer the change and take baby steps instead.

src/index.ts

KenLSM · 2025-02-17T13:20:09Z

src/verification/index.ts

@@ -47,7 +51,11 @@ export default class Verification {
      )
    }

-    if (!this.verificationPublicKey) {
+    if (!this.getVerificationPublicKey) {


Same suggestion as Crypto class, can this be checked in the constructor?

examples/webhook-server/webhook-server.ts

examples/webhook-server.ts

+    (req, res, next) => {
+      try {
+        const signature = req.get('X-FormSG-Signature')
+        if (!signature) {
+          console.error('No signature found in headers')
+          return res.status(401).send({ message: 'Signature missing' })
+        }
+
+        formsg.webhooks.authenticate(signature, webhookUrl)
+        console.log('Webhook authenticated successfully')
+        return next()
+      } catch (e) {
+        console.error('Authentication failed:', e)
+        return res.status(401).send({ message: 'Unauthorized' })
+      }
+    },


To fix the problem, we need to introduce rate limiting to the Express application. The best way to do this is by using the express-rate-limit package, which allows us to easily set up rate limiting middleware. We will configure the rate limiter to allow a maximum of 100 requests per 15 minutes and apply it to all routes in the application.

Install the express-rate-limit package.

Import the express-rate-limit package in the examples/webhook-server.ts file.

Set up the rate limiter with the desired configuration.

Apply the rate limiter middleware to the Express application.

littlemight changed the title ~~feat: use jwks as public key~~ feat: use jwks to fetch public keys Feb 14, 2025

littlemight self-assigned this Feb 14, 2025

KenLSM reviewed Feb 17, 2025

View reviewed changes

src/index.ts Show resolved Hide resolved

KenLSM reviewed Feb 17, 2025

View reviewed changes

github-advanced-security bot found potential problems Mar 4, 2025

View reviewed changes

examples/webhook-server/webhook-server.ts Fixed Show resolved Hide resolved

littlemight added 21 commits March 5, 2025 15:15

feat: init jwks with mock json

cf79d82

feat: fetch using axios

046b527

feat: fallback to prev keys if jwks fail

efe6d5d

feat: prepare boilerplate for cache logic

44d4ba9

feat: barebones string cache

46757a6

test: make sure DIY cache works

12716e6

chore: readd comment back

0afa54a

refactor: modules to use async pub key getters

efdabba

chore: meaningful kid

cec90c8

refactor: group util tests in same folder

c0585d7

test: jwks util

f4ca1ca

test: init spec failing

36707f4

feat: fetch jwks during init

1735e43

test: init SDK with JWKS

8dd5e6a

test: webhook spec

86556b8

test: crypto spec

04dc557

fix: quirk when making public key getter optional

ba43a2a

test: verification spec

868f52f

chore: cleanup resource

9939655

chore: alpha versioning

18340eb

feat: rotate thru keys

372338c

littlemight added 9 commits March 5, 2025 15:16

chore: major ver bump

67ee3c6

chore: ver on package-lovk

b4e465c

fix: naming and spec

58cac69

feat: exponential backoff and fix tests

37eefe1

feat: use axios-retry instead + integration test

91c1719

feat: allow overriding backoffs

f7188ea

chore: add examples folder for easier SDK sandbox

a3e8665

chore: node keypair generator

c8a15c0

docs: init migration.md

2fae696

littlemight force-pushed the feat/jwks branch from 26a678f to 2fae696 Compare March 5, 2025 07:17

littlemight added 6 commits March 7, 2025 10:25

docs: redo MIGRATION.md

1ad412e

feat: pass optional keyId on headers, allow custom static pubkey on init

4aac9aa

refactor: examples folder

cb67814

test: key fallbacks

9b26722

docs: comment regarding compile and runtime validation

16809ef

docs: b64 vs b64url diff

080bd8b

github-advanced-security bot found potential problems Mar 9, 2025

View reviewed changes

littlemight added 2 commits March 12, 2025 02:51

fix(test): trust me bro

848cee4

chore(ci): don't be greedy

705251e

@@ -3,2 +3,3 @@
             import formSgSDK from '../src'
+            import rateLimit from 'express-rate-limit'
@@ -9,2 +10,11 @@
+            // Set up rate limiter: maximum of 100 requests per 15 minutes
+            const limiter = rateLimit({
+              windowMs: 15 * 60 * 1000, // 15 minutes
+              max: 100, // max 100 requests per windowMs
+            })
+            // Apply rate limiter to all requests
+            app.use(limiter)
             // Get the form secret key from environment variables

@@ -12,3 +12,4 @@
                 "express": "^4.18.2",
-                "dotenv": "^16.3.1"
+                "dotenv": "^16.3.1",
+                "express-rate-limit": "^7.5.0"
               },

Package	Version	Security advisories
express-rate-limit (npm)	7.5.0	None

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: use jwks to fetch public keys #113

feat: use jwks to fetch public keys #113

Uh oh!

littlemight commented Feb 14, 2025 •

edited

Loading

Uh oh!

linear bot commented Feb 14, 2025

Uh oh!

littlemight commented Feb 15, 2025 •

edited

Loading

Uh oh!

KenLSM Feb 17, 2025

Uh oh!

littlemight Feb 19, 2025

Uh oh!

KenLSM Feb 20, 2025

Uh oh!

KenLSM Feb 17, 2025

Uh oh!

littlemight Feb 17, 2025

Uh oh!

KenLSM Feb 17, 2025

Uh oh!

littlemight Mar 5, 2025

Uh oh!

Uh oh!

KenLSM Feb 17, 2025

Uh oh!

littlemight Mar 5, 2025

Uh oh!

Uh oh!

Check failure

Copilot Autofix

Uh oh!

feat: use jwks to fetch public keys #113

Are you sure you want to change the base?

feat: use jwks to fetch public keys #113

Uh oh!

Conversation

littlemight commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Reference

Solution

Before & After Screenshots

Tests

Deploy Notes

Uh oh!

linear bot commented Feb 14, 2025

Uh oh!

littlemight commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check failure

Uh oh!

Copilot Autofix

Uh oh!

littlemight commented Feb 14, 2025 •

edited

Loading

littlemight commented Feb 15, 2025 •

edited

Loading