Repository navigation
Encrypt session tokens with a dedicated key - #3709
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge only after kf-console #76 is deployed to production. (This change doesn't call the console and works without #76. The line is here because every PR in this batch carries it. Drop it if you prefer.)
PubPub derived its AES-256-GCM key from
OIDC_CLIENT_SECRET. That key encrypts the OIDC state (which carries the PKCE verifier), the one-time custom-domain session-set token, and thekfRefreshTokenstored on each express session. So rolling the client secret, which kf-console now supports with a grace period, would log everyone out. This PR gives that encryption its own key,KF_SESSION_ENCRYPTION_KEY.What changed
server/kf/oidc.server.ts:encryptPayloadusesKF_SESSION_ENCRYPTION_KEYwhen it is set.decryptPayloadtries that key first, then falls back to the key derived from the client secret.decryptPayloadWithKeyInfoalso reports whether a value was read with the fallback.server/kf/sessionCheck.ts: when the periodic liveness check reads a refresh token stored under the old key, it re-encrypts it under the new key, and the existing session save persists it. There is no bulk migration. State and session-set tokens live for minutes, so they need nothing: old ones decrypt through the fallback, and new ones use the new key.server/envSchema.ts,infra/ENV.md: declareKF_SESSION_ENCRYPTION_KEY(optional; it must decode to 32 bytes; generate it withopenssl rand -base64 32). Startup fails on a malformed value. I added the one row toENV.mdby hand: the file has drifted from the schema, and regenerating it rewrites about 60 unrelated rows.When the fallback can go
KF Auth issues refresh tokens for 30 days (
refreshTokenExpiresIn). PubPub writes the token once, at login, and introspection never rotates it. So 30 days after the key is live in production, every token written under the old key has expired: its session is torn down at the next check whether or not it can be decrypted. After that (allow a day's margin), removelegacyDerivedKey()and the fallback, which are markedTODO(session-key-fallback), and make the env var required. Only then can the client secret roll without logging anyone out.Rollout
Add
KF_SESSION_ENCRYPTION_KEY(a different value per environment) toinfra/.env.dev.enc(DuqDuq) andinfra/.env.enc(prod), and optionally toinfra/.env.local.enc.The first rolling update that sets the key briefly mixes replicas with and without it. A replica without the key can't read what one with the key wrote. Two things can go wrong in that window, which lasts minutes:
Both are rare.
How it was checked
server/kf/__tests__/sessionEncryption.test.ts(11 tests):null;null;pnpm run lint(biome andtsc --noEmit) passes.server/kf,server/login,server/org,server/userandserver/authTokenall pass locally (86 tests). The full suite with the Firebase emulator runs in CI. Locally,firebase-toolsdoesn't run on Node 26, and the Homebrew Node 24 install on this machine is broken.Not changed:
decodeIdTokenClaimsdecodeIdTokenClaimsreads the ID token without checking its signature. The token comes straight from the token endpoint, in the response to PubPub's own authenticated POST. In production that is HTTPS to the public KF Auth URL, becauseOIDC_ISSUER_INTERNAL_URLis only set locally. OIDC Core 3.1.3.7 allows TLS server validation in place of a signature check in that case.Only
sidis used from it: it targets thesession.revokedwebhook. Identity comes fromuserinfoover the same channel, so anyone able to forge the ID token could forgeuserinfotoo, and verifying the ID token alone adds little.The fix, if wanted: verify the token with
jose(jwtVerifyagainstcreateRemoteJWKSet(discovery.jwks_uri), checkingiss,audequal to the client id, andexp), and check that itssubequals theuserinfosub. This also guards against a misconfigured internal URL that uses plain HTTP outside Docker.