fix(encryption): Harden encryption: AES-256-GCM for 2FA secrets, mandatory ENCRYPTION_KEY in production#212
Merged
therealbrad merged 1 commit intomainfrom Apr 15, 2026
Conversation
…crets and add legacy support - Introduced AES-256-GCM encryption for TOTP secrets, replacing the previous XOR method. - Added versioning for encrypted secrets with prefixes `v1:` for legacy XOR and `v2:` for AES-256-GCM. - Implemented backward compatibility to decrypt legacy `v1:` records. - Enhanced tests to cover new encryption methods and legacy decryption paths. - Updated the authorization flow to upgrade legacy secrets to the new format during user login. This update improves security for two-factor authentication by using a stronger encryption method while maintaining compatibility with existing records.
Contributor
Author
|
🎉 This PR is included in version 0.21.15 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Closes #204.
Summary
Two encryption-subsystem gaps from the security review, fixed together because they touch the same boot path:
EncryptionService+ENCRYPTION_KEY. The previous XOR-with-repeating-key path (acknowledged as a placeholder in an in-file comment) was cryptographically broken against anyone with read access to theUsertable. The legacyv1:format is still decryptable for backward compatibility during migration; new writes arev2:.ENCRYPTION_KEY.getMasterKey()throws in production when the key is unset, instead of silently falling back to the built-in default and effectively publishing every encrypted secret in the DB. Non-production keeps the convenient dev fallback with a warning.instrumentation.tscallsassertEncryptionConfigured()on server boot so misconfiguration surfaces immediately, not on the first encrypt/decrypt mid-request.server/auth.ts): when a user's stored secret is stillv1:, re-encrypt asv2:and persist. Best-effort — a failure doesn't block login.Pre-deploy migration (already completed in production)
Before this PR is deployed, every production tenant must have
ENCRYPTION_KEYset — otherwise the startup probe crashes the container. This was done ahead of this PR:ENCRYPTION_KEYin their.env.production+ k8s Secret.Integration.credentials,LlmIntegration.credentials,CodeRepository.credentials,UserIntegrationAuth.accessToken/refreshToken,User.twoFactorSecret) were cleared and affected integrations markedINACTIVE. Audit showed only 4 customer tenants (allego, ashwini, bhargavqa, rubenmariorossi) had any affected rows; zero 2FA enrollments and zero OAuth tokens across the fleet.bradfreetrialDB (and its role) was dropped — it had no corresponding deployment.Test plan
pnpm type-check— cleanpnpm test— 5,593/5,593 passing (full suite)pnpm vitest run lib/two-factor.test.ts utils/encryption.test.ts— covers: AES round-trip, legacyv1:decrypt backward-compat, unknown-version rejection, non-determinism of AES output, production-mode startup failure whenENCRYPTION_KEYis unset, dev-mode warn path,isLegacyEncryptionhelper[startup] ENCRYPTION_KEY configured ✓in each tenant's first-boot logsNotes for reviewers
utils/encryption.tsgainedassertEncryptionConfigured()as an explicit startup-probe export;getMasterKey()is still the primary entry point for encrypt/decrypt callers.decryptLegacyV1) stays in the codebase untilv1:records are confirmed purged. Track removal as a follow-up.🤖 Generated with Claude Code