[HDX-5179] feat(api): add a token encryption service for stored tokens - #3112
[HDX-5179] feat(api): add a token encryption service for stored tokens#3112jordan-simonovski wants to merge 3 commits into
Conversation
Slack bot tokens and OAuth tokens need encrypting at rest, and the encryption method has to be replaceable without changing call sites. This adds the provider registry and stored format they share, plus an AES-256-GCM provider. Stored values are `v1:<provider>:<payload>` and decryption dispatches on the provider named in the value, so a deployment can change providers without orphaning tokens already stored. Setting TOKEN_ENCRYPTION_KEY is the whole opt-in; without it tokens are stored verbatim under a `none` provider that is deliberately obvious in the database. Nothing consumes this yet - it is the library on its own.
🦋 Changeset detectedLatest commit: ea1aaa3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryAdds a pluggable service for encrypting stored third-party tokens, including AES-256-GCM and plaintext providers, configuration, startup verification, observability, and unit coverage.
Confidence Score: 4/5The implementation is functionally sound, but the explicit observability requirement for provider-replacement events must be satisfied before merging. Provider replacement is logged as a countable event without a corresponding metric, contrary to the repository’s observability requirement; no blocking encryption-integrity or runtime failure was established. Files Needing Attention: packages/api/src/utils/tokenEncryption.ts
|
| Filename | Overview |
|---|---|
| packages/api/src/utils/tokenEncryption.ts | Implements provider registration, versioned token envelopes, AES-256-GCM encryption, plaintext compatibility, metrics, and startup verification; provider replacement warnings lack a corresponding metric. |
| packages/api/src/utils/tests/tokenEncryption.test.ts | Covers encryption round trips, tampering, provider dispatch, key parsing, plaintext migration behavior, and startup checks. |
| packages/api/src/server.ts | Runs token-encryption verification during API startup before database connection retries. |
| packages/api/src/config.ts | Exposes the optional token-encryption key from the environment. |
| docker-compose.yml | Passes the optional token-encryption key into the API container. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
C[Token caller] --> E[encryptToken]
E --> A{Active provider}
A -->|none| P[v1:none:plaintext]
A -->|local| G[AES-256-GCM]
G --> V[v1:local:payload]
P --> S[(Stored token)]
V --> S
S --> D[decryptToken]
D --> R{Provider in envelope}
R -->|none| N[Plaintext and migration warning]
R -->|local| X[AES-GCM verification and decryption]
Reviews (3): Last reviewed commit: "refactor(api): hold the resolved token e..." | Re-trigger Greptile
Deep Review✅ No critical issues found. The AES-256-GCM implementation is sound — per-message random IV, auth tag, AAD binding, length-checked base64/hex key parsing with a canonicalization guard, and errors that avoid interpolating secrets. Because 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (8): correctness, testing, maintainability, security, reliability, adversarial, kieran-typescript, previous-comments. Testing gaps:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| await expect(verifyTokenEncryption()).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it('passes with a working provider', async () => { |
There was a problem hiding this comment.
🔵 minor — Both verifyTokenEncryption outcome tests assert only resolves.toBeUndefined(), so they cannot tell success from failure
verifyTokenEncryption always resolves to undefined on the non-none path (tokenEncryption.ts:257-286 catches everything), so it('passes with a working provider') at line 271 and it('does not throw when the active provider is broken') at line 265 assert the identical, unconditionally-true thing. The only observable output is the log line, so assert it: spy on logger.info and expect the 'Token encryption verified' call in the success test, and spy on logger.error and expect the 'verification failed' call in the broken-provider test (plus expect(error).not.toHaveBeenCalled() in the success one). As written, deleting the logger.info call, swallowing the error without logging, or dropping the decrypted !== VERIFY_SENTINEL check all keep the suite green.
| ); | ||
| return; | ||
| } | ||
| if (activeProviderName === LOCAL_PROVIDER) { |
There was a problem hiding this comment.
🔵 minor — The local provider and the boot fail-fast branch — the only path that can stop the API starting — have no test
The suite registers its own test-aes* providers and never activates local, so getLocalKey() (line 199), the local registration (line 201), and the if (activeProviderName === LOCAL_PROVIDER) getLocalKey() fail-fast at line 254 are all uncovered; the PR's headline claim "a malformed key stops the API starting" is untested, and removing line 254-256 (or making parseEncryptionKey non-throwing) breaks nothing. Add a test that sets process.env.TOKEN_ENCRYPTION_KEY to a bad value inside jest.isolateModules/after jest.resetModules, re-requires the module, and expects verifyTokenEncryption() to reject — and a companion with a valid key asserting a v1:local: round trip.
| name: string, | ||
| getKey: () => Buffer, | ||
| ): TokenEncryptionProvider { | ||
| const aad = Buffer.from(`${ENVELOPE_VERSION}:${name}`, 'utf8'); |
There was a problem hiding this comment.
🔵 minor — Ciphertext is not bound to the row it belongs to, and adding that binding later invalidates every stored token
The AAD is only v1:<provider>, so any v1:local:… payload decrypts in any row: an attacker with Mongo write access (but no key) can copy team A's encrypted Slack token into team B's webhook document and the app will decrypt and replay it. Add an optional context argument now — encryptToken(plaintext, context?) / decryptToken(envelope, context?), with the context concatenated into the AAD alongside ${ENVELOPE_VERSION}:${name} so callers can pass e.g. webhook:<id> — because once rows exist, changing the AAD makes every stored ciphertext fail to authenticate, which makes this effectively a one-shot decision.
| export function selectDefaultProvider( | ||
| key: string | undefined, | ||
| ): BuiltInProviderName { | ||
| return key ? LOCAL_PROVIDER : NONE_PROVIDER; |
There was a problem hiding this comment.
🔵 minor — A whitespace-only TOKEN_ENCRYPTION_KEY selects local and then blocks boot with "is not set"
selectDefaultProvider tests key untrimmed while parseEncryptionKey (line 128) trims, so TOKEN_ENCRYPTION_KEY=$(cat /run/secrets/key) on an empty secret file (a lone \n) activates local, then getLocalKey() at line 255 throws TOKEN_ENCRYPTION_KEY is not set and index.ts exits 1 — a hard boot failure for what the module's own parser classifies as unset, contradicting "a missing key stays silent". Use return key?.trim() ? LOCAL_PROVIDER : NONE_PROVIDER; so both functions share one definition of unset.
PR Review4 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 4 minor 4 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
E2E Test Results✅ All tests passed • 355 passed • 1 skipped • 1510s
Tests ran across 4 shards in parallel. |
| if (isTokenEnvelope(plaintext)) { | ||
| throw new Error('Value is already a token encryption envelope'); | ||
| } | ||
| const provider = getProvider(activeProviderName); |
There was a problem hiding this comment.
nit: The runtime shouldn't be changing besides on startup so it might make sense to even do provider at the app level instead of storing activeProviderName and needing to call getProivder everywhere.
There was a problem hiding this comment.
Good call! I've updated this in the PR. Seemed easier than a follow-up
brandon-pereira
left a comment
brandon-pereira
left a comment
There was a problem hiding this comment.
LGTM - besides some non-blocking feedback on init
The active provider is set once at startup, so storing the resolved provider instead of its name drops a lookup on every write and keeps a late re-registration from swapping the implementation out from under it. Reads still resolve by name: decryptToken follows the provider recorded in each stored value, which varies per row.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| providers.set(provider.name, provider); | ||
| } | ||
|
|
||
| /** Selects the provider `encryptToken` writes with. Decryption is unaffected. */ |
There was a problem hiding this comment.
Replacing a registered provider emits a warning but does not record a corresponding metric. This violates the repository directive that countable log events must also emit metrics and prevents operators from alerting on or measuring replacements that may make existing envelopes unreadable. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| export function selectDefaultProvider( | ||
| key: string | undefined, | ||
| ): BuiltInProviderName { | ||
| return key ? LOCAL_PROVIDER : NONE_PROVIDER; |
There was a problem hiding this comment.
🔵 minor — A whitespace-only TOKEN_ENCRYPTION_KEY selects local and then kills the boot with "is not set"
selectDefaultProvider tests the raw value for truthiness while parseEncryptionKey trims it, so TOKEN_ENCRYPTION_KEY="\n" (a secret file/kubectl create secret --from-file that carries a newline — the exact case the tolerates surrounding whitespace from a piped secret test anticipates) picks local, then verifyTokenEncryption → getLocalKey() throws TOKEN_ENCRYPTION_KEY is not set and index.ts:38 exits the process. Trim before the check: return key?.trim() ? LOCAL_PROVIDER : NONE_PROVIDER;, and add a selectDefaultProvider(' ') case next to the '' one.
| }); | ||
|
|
||
| describe('verifyTokenEncryption', () => { | ||
| it('does not throw when the active provider is broken', async () => { |
There was a problem hiding this comment.
🔵 minor — The verifyTokenEncryption tests pass even if the function does nothing
does not throw when the active provider is broken and passes with a working provider only assert resolves.toBeUndefined(), so replacing the whole body with return; keeps both green — neither asserts the logger.error/logger.info that is the entire output of the check. Assert logger.error was called (with provider: 'broken') on the failure path and logger.info on the success path, and add the one behaviour the PR description headlines — a malformed key makes verifyTokenEncryption() reject, so Server.start() fails — which currently has no test at all (mock @/config under jest.isolateModules to reach the local branch).
| ); | ||
| return; | ||
| } | ||
| if (activeProvider.name === LOCAL_PROVIDER) { |
There was a problem hiding this comment.
🔵 minor — Only the built-in local provider gets a fatal config check; a plugged-in provider's misconfiguration is swallowed
if (activeProvider.name === LOCAL_PROVIDER) getLocalKey(); is what makes a bad key fatal; every other provider's configuration error (bad KMS key ARN, wrong region) is raised inside the try and only logged, so the process boots "healthy" and fails on the first token write — which undercuts the "a new file and one registration call" pluggability claim. Move the check onto the interface as an optional verify?(): Promise<void> (or a preflight that is allowed to throw) that verifyTokenEncryption awaits outside the try, so any provider can declare a fatal misconfiguration instead of hardcoding one name here.
|
|
||
| // Checked before Mongo so a bad encryption key or KMS policy is reported | ||
| // even while the Mongo connect below is still retrying. | ||
| await verifyTokenEncryption(); |
There was a problem hiding this comment.
🔵 minor — The scheduled-task process, which is the one that will read stored Slack tokens, never runs the startup check
verifyTokenEncryption() is wired only into Server.start(); packages/api/src/tasks/index.ts is a separate entrypoint (the deployment split behind RUN_SCHEDULED_TASKS_EXTERNALLY) and it is where Slack notifications are sent from (packages/api/src/tasks/checkAlerts/transports/slack.ts), so a TOKEN_ENCRYPTION_KEY that is missing or malformed in the task container passes unnoticed until an alert fires. Await verifyTokenEncryption() in the task entrypoint before instrumentedMain(argv) as well.
2 participants
Slack bot tokens and OAuth tokens need encrypting at rest before anything stores them. This adds the service that does it. Nothing calls it yet.
TOKEN_ENCRYPTION_KEY(32 bytes, base64 or hex) and stored tokens are encrypted with AES-256-GCM. Set nothing and they are stored readable, which stays a supported choice.hyperdx.token_encryption.disabled.Two deviations from the acceptance criteria, both worth a look: there is no base64 provider, since it is readable by anyone with database access and that is the threat it was meant to address; and the
testendpoint is a startup check rather than a route, so nobody has to remember to call it.Rotating the key makes existing rows unreadable. That is accepted for now, and the version in the stored format is how a key id gets added when it matters.
Implementation detail
v1:<provider>:<payload>, with the version and provider prefix bound into AES-GCM as additional authenticated data, so a payload cannot be relabelled and replayed under another provider sharing the key.decryptTokenwould otherwise put that token into the logs.utils/instrumentation.tshandles conflicting instrument definitions.yarn ci:unitinpackages/api(1021 tests),tsc --noEmit, and eslint at 299 warnings against the 302 ceiling.