Implement Enterprise-Grade Database Encryption System #244
Reference in New Issue
Block a user
Delete Branch "main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
This PR implements a comprehensive database encryption system that provides enterprise-grade security for SSH credentials, passwords, and authentication tokens stored in Termix.
🔐 Core Security Features
🛡️ Data Protection Coverage
🚀 User Experience Improvements
🔧 Technical Implementation
EncryptedDBOperationswrapper📊 Migration & Status Features
🏗️ Architecture Highlights
This implementation follows security best practices and provides enterprise-level protection while maintaining ease of use for administrators.
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Walkthrough
Adds database field-level encryption, key management, and migration utilities; exposes encryption REST endpoints; updates server startup to initialize encryption; expands DB schema for SSH keys; rewires DB CRUD through encrypted operations; enhances SSH credential routes with key parsing/generation/deployment; updates SSH runtime to prefer new privateKey; and adds corresponding admin and credential UI plus API wrappers.
Changes
SECURITY.mdsrc/backend/utils/encryption.ts,src/backend/utils/database-encryption.ts,src/backend/utils/encryption-key-manager.ts,src/backend/utils/master-key-protection.tssrc/backend/utils/encrypted-db-operations.ts,src/backend/utils/encryption-migration.ts,src/backend/utils/encryption-test.tssrc/backend/starter.ts,src/backend/database/database.ts/encryption/status,/encryption/initialize,/encryption/migrate,/encryption/regenerate.src/backend/database/db/index.ts,src/backend/database/db/schema.tsssh_credentialscolumns:private_key,public_key,detected_key_typeand corresponding TS schema fields; migration guarded by add-if-not-exists.src/backend/database/routes/credentials.ts,src/backend/utils/ssh-key-utils.tssrc/backend/ssh/terminal.ts,src/backend/ssh/tunnel.ts,src/backend/ssh/server-stats.ts,src/backend/database/routes/ssh.tsprivateKeyover legacykey; host/credential fetches wrapped with encrypted selects.src/ui/Desktop/Admin/AdminSettings.tsxsrc/ui/Desktop/Apps/Credentials/CredentialEditor.tsx,src/ui/Desktop/Apps/Credentials/CredentialsManager.tsx,unified_key_section.tsx,src/ui/main-axios.tssrc/types/index.tspublicKey?: stringtoCredentialandCredentialData.Sequence Diagram(s)
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
@coderabbitai generate docstringsto improve docstring coverage.✅ Passed checks (2 passed)
Comment
@coderabbitai helpto get the list of available commands and usage tips.Actionable comments posted: 28
🧹 Nitpick comments (46)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between
0f75cd4d16and0839cb4134.⛔ Files ignored due to path filters (3)
package.jsonis excluded by!**/*.jsonsrc/locales/en/translation.jsonis excluded by!**/*.jsonsrc/locales/zh/translation.jsonis excluded by!**/*.json📒 Files selected for processing (24)
SECURITY.md(1 hunks)src/backend/database/database.ts(4 hunks)src/backend/database/db/index.ts(1 hunks)src/backend/database/db/schema.ts(1 hunks)src/backend/database/routes/credentials.ts(9 hunks)src/backend/database/routes/ssh.ts(2 hunks)src/backend/ssh/server-stats.ts(4 hunks)src/backend/ssh/terminal.ts(1 hunks)src/backend/ssh/tunnel.ts(2 hunks)src/backend/starter.ts(2 hunks)src/backend/utils/database-encryption.ts(1 hunks)src/backend/utils/encrypted-db-operations.ts(1 hunks)src/backend/utils/encryption-key-manager.ts(1 hunks)src/backend/utils/encryption-migration.ts(1 hunks)src/backend/utils/encryption-test.ts(1 hunks)src/backend/utils/encryption.ts(1 hunks)src/backend/utils/master-key-protection.ts(1 hunks)src/backend/utils/ssh-key-utils.ts(1 hunks)src/types/index.ts(2 hunks)src/ui/Desktop/Admin/AdminSettings.tsx(6 hunks)src/ui/Desktop/Apps/Credentials/CredentialEditor.tsx(10 hunks)src/ui/Desktop/Apps/Credentials/CredentialsManager.tsx(6 hunks)src/ui/main-axios.ts(9 hunks)unified_key_section.tsx(1 hunks)🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Files:
src/backend/database/db/index.tssrc/types/index.tssrc/backend/ssh/terminal.tssrc/backend/ssh/tunnel.tssrc/backend/utils/ssh-key-utils.tssrc/backend/utils/master-key-protection.tssrc/backend/utils/encryption-migration.tssrc/backend/utils/encryption.tssrc/backend/ssh/server-stats.tssrc/ui/Desktop/Admin/AdminSettings.tsxunified_key_section.tsxsrc/backend/database/db/schema.tssrc/backend/utils/database-encryption.tssrc/backend/utils/encryption-test.tssrc/backend/starter.tssrc/backend/utils/encrypted-db-operations.tssrc/backend/database/routes/ssh.tssrc/backend/database/database.tssrc/ui/main-axios.tssrc/ui/Desktop/Apps/Credentials/CredentialsManager.tsxsrc/backend/utils/encryption-key-manager.tssrc/ui/Desktop/Apps/Credentials/CredentialEditor.tsxsrc/backend/database/routes/credentials.ts**/backend/**/*.{ts,js}
⚙️ CodeRabbit configuration file
Files:
src/backend/database/db/index.tssrc/backend/ssh/terminal.tssrc/backend/ssh/tunnel.tssrc/backend/utils/ssh-key-utils.tssrc/backend/utils/master-key-protection.tssrc/backend/utils/encryption-migration.tssrc/backend/utils/encryption.tssrc/backend/ssh/server-stats.tssrc/backend/database/db/schema.tssrc/backend/utils/database-encryption.tssrc/backend/utils/encryption-test.tssrc/backend/starter.tssrc/backend/utils/encrypted-db-operations.tssrc/backend/database/routes/ssh.tssrc/backend/database/database.tssrc/backend/utils/encryption-key-manager.tssrc/backend/database/routes/credentials.ts**/types/**/*.{ts,js}
⚙️ CodeRabbit configuration file
Files:
src/types/index.ts**/*.md
⚙️ CodeRabbit configuration file
Files:
SECURITY.md**/main-axios.ts
⚙️ CodeRabbit configuration file
Files:
src/ui/main-axios.ts🧬 Code graph analysis (16)
src/backend/utils/ssh-key-utils.ts (1)
src/backend/utils/master-key-protection.ts (1)
src/backend/utils/encryption-migration.ts (3)
src/backend/ssh/server-stats.ts (3)
src/ui/Desktop/Admin/AdminSettings.tsx (1)
unified_key_section.tsx (1)
src/backend/utils/database-encryption.ts (3)
src/backend/starter.ts (1)
src/backend/utils/encrypted-db-operations.ts (3)
src/backend/database/routes/ssh.ts (3)
src/backend/database/database.ts (2)
src/ui/main-axios.ts (1)
src/ui/Desktop/Apps/Credentials/CredentialsManager.tsx (2)
src/backend/utils/encryption-key-manager.ts (3)
src/ui/Desktop/Apps/Credentials/CredentialEditor.tsx (3)
src/backend/database/routes/credentials.ts (4)
🪛 markdownlint-cli2 (0.17.2)
SECURITY.md
192-192: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
197-197: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
202-202: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Gitleaks (8.27.2)
src/backend/utils/ssh-key-utils.ts
[high] 10-31: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 64-67: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 70-75: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 79-148: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 Biome (2.1.2)
src/backend/utils/encrypted-db-operations.ts
[error] 147-147: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 152-152: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 157-157: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (18)
@@ -0,0 +38,4 @@- Cryptographically random- Unique per installation- Safely backed up⚠️ Potential issue
Align key requirements with implementation (64‑hex recommended).
Docs say “≥16 chars,” but the code requires ≥32 and the system expects a 32‑byte key; recommended generation uses
-hex 32. Tighten the requirement and call out hex explicitly.Also applies to: 36-41
🤖 Prompt for AI Agents
@@ -258,0 +299,4 @@}});app.post("/encryption/migrate", async (req, res) => {⚠️ Potential issue
Potential information disclosure in error response.
The endpoint returns the first 8 characters of the master key in the response, which could aid attackers in brute-force attempts.
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Key regeneration endpoint lacks safeguards.
The
/encryption/regenerateendpoint could lead to data loss if called accidentally. It should require additional confirmation or be restricted to specific environments.Consider adding:
🤖 Prompt for AI Agents
⚠️ Potential issue
Environment variable parsing is fragile.
The code doesn't validate environment variable values and uses loose comparisons that could lead to unexpected behavior.
📝 Committable suggestion
⚠️ Potential issue
Excessive console logging in production endpoints.
The key detection endpoints contain extensive console.log statements that could expose sensitive information in production logs.
Remove or guard console.log statements:
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Unsafe grep pattern in key existence check.
Using the key content directly in grep could fail or cause issues with special regex characters.
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Command injection vulnerability in SSH key deployment.
The public key is directly interpolated into shell commands without proper escaping, which could allow command injection.
The current escaping only handles single quotes but not other shell metacharacters:
🤖 Prompt for AI Agents
⚠️ Potential issue
Only this endpoint uses the encryption wrapper; other routes still bypass it (plaintext risk + broken reads)
Reads/writes elsewhere (GET /db/host, GET /db/host/:id, create/update/bulk-import, resolveHostCredentials) still use direct db.* calls. That will (a) persist secrets in plaintext and (b) return ciphertext for encrypted fields. Route all ssh_data and ssh_credentials operations through EncryptedDBOperations.
Apply patterns like below across the file:
If EncryptedDBOperations exposes insert/update helpers, use them for create/update/bulk-import; otherwise encrypt/decrypt via the provided field helpers before persisting/after reading.
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -0,0 +104,4 @@table: tableName});throw error;}⚠️ Potential issue
Update() returns encrypted rows; decrypt before returning.
Inconsistent with insert()/select(); callers will get ciphertext.
Apply this diff:
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -0,0 +78,4 @@await this.storeKey(environmentKey);this.currentKey = environmentKey;return environmentKey;}⚠️ Potential issue
Env key validation rejects strong hex keys; accept 64‑hex or derive from passphrases.
As written,
validateKeyStrengthflags a cryptographically strong hex key likeopenssl rand -hex 32as “too weak” (no specials/upper), causing env keys to be rejected. This contradicts SECURITY.md and will confuse operators.Action:
Patch (minimal, hex‑first):
If you want KDF support for non‑hex env keys, I can add PBKDF2+HKDF here to normalize to 32 bytes.
Also applies to: 250-262
@@ -0,0 +111,4 @@});return newKey;}⚠️ Potential issue
Store updates should be atomic; wrap dual settings writes in a transaction.
db_encryption_keyandencryption_key_createdare updated separately; partial failure can desync metadata.Suggest using a Drizzle transaction:
Also applies to: 116-169
🤖 Prompt for AI Agents
@@ -0,0 +213,4 @@});return null;}}⚠️ Potential issue
Don’t auto‑generate a new key on read/parse failure; fail fast to prevent data loss.
getStoredKey()returnsnullon any error, causinginitializeKey()to generate a fresh key—breaking decryption of existing data.Make
getStoredKey()returnnullonly when the setting is absent; throw on parse/decrypt errors so initialization fails instead of rotating the key silently.Also applies to: 45-61
@@ -0,0 +84,4 @@// Validate key strengthif (this.config.masterPassword.length < 16) {throw new Error('Master password must be at least 16 characters long');}⚠️ Potential issue
Weak password strength validation.
The migration only checks for minimum length of 16 characters but doesn't validate entropy or complexity, which could allow weak passwords like "aaaaaaaaaaaaaaaa".
Consider implementing proper password strength validation using a library like zxcvbn or implementing entropy checks.
🤖 Prompt for AI Agents
@@ -0,0 +112,4 @@const backupPath = path.join(dataDir, `db-backup-${Date.now()}.sqlite`);if (fs.existsSync(dbPath)) {fs.copyFileSync(dbPath, backupPath);⚠️ Potential issue
Use of synchronous file operations blocks the event loop.
Using
fs.existsSyncandfs.copyFileSyncin an async function blocks the Node.js event loop, which could impact server performance during migration.📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -0,0 +389,4 @@console.error('Migration failed:', error.message);process.exit(1);});}⚠️ Potential issue
Script execution security concern.
When run as a CLI script, it reads sensitive encryption keys from environment variables which could be logged or exposed in process listings.
Consider reading the master password from a secure file or stdin instead of environment variables:
🤖 Prompt for AI Agents
@@ -0,0 +38,4 @@} catch {return false;}}🛠️ Refactor suggestion
isEncrypted() can misclassify valid ciphertexts (empty data).
Encrypted empty strings produce empty ciphertext chunks but a valid tag/iv. Current truthiness check rejects those.
Apply this diff:
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -0,0 +16,4 @@private static readonly VERSION = 'v1';private static readonly KEK_SALT = 'termix-kek-salt-v1';private static readonly KEK_ITERATIONS = 50000;⚠️ Potential issue
Align KEK iteration count with PR spec (100,000).
Spec says PBKDF2 + HKDF with 100k iterations. This class uses PBKDF2 with 50k and no HKDF.
Apply this diff (and consider adding HKDF if required by your threat model):
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -0,0 +26,4 @@process.cwd(),this.getFileSystemFingerprint(),this.getNetworkFingerprint()];⚠️ Potential issue
Unstable device fingerprint risks permanent lock‑out.
Including values like hostname and process.cwd() makes the KEK drift across reboots/deploys/paths. Users could lose access after benign changes.
Example minimal change:
Would you like me to propose a machine-id based variant?
Also applies to: 31-47
🤖 Prompt for AI Agents
@@ -0,0 +112,4 @@let encrypted = cipher.update(masterKey, 'hex', 'hex');encrypted += cipher.final('hex');const tag = cipher.getAuthTag();⚠️ Potential issue
Incorrect cipher encodings (encrypt/decrypt will corrupt non‑hex keys).
You’re treating the plaintext master key as hex on both encrypt and decrypt. If the key is a regular string (e.g., from env), it will break.
Apply this diff:
Also applies to: 167-169
🤖 Prompt for AI Agents
⚠️ Potential issue
Critical: Binary string conversion loses data integrity
Converting Buffer to 'binary' string can corrupt the data due to character encoding issues. This could lead to incorrect key type detection.
Then search for the key type indicators directly in the Buffer:
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Critical: Binary string conversion issue repeated
Same binary string conversion issue that can corrupt data during PKCS#8 format detection.
Search for OIDs directly in the Buffer:
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Unreliable key type detection based on content length
Using content length to determine key type is highly unreliable and could lead to incorrect key type identification. Key sizes can vary significantly within the same key type.
Consider removing this fallback or implementing a more robust detection mechanism:
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Critical: Binary string conversion in public key detection
Same issue with binary string conversion that can corrupt data during public key type detection.
🤖 Prompt for AI Agents
⚠️ Potential issue
Unreliable public key type detection based on length
Using content length for public key type detection is unreliable and error-prone.
📝 Committable suggestion
⚠️ Potential issue
Missing passphrase parameter in detectKeyType function
The
detectKeyTypefunction doesn't accept a passphrase parameter, which means it will fail for encrypted keys. This is inconsistent with theparseSSHKeyfunction.📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Misleading validation result when unable to verify
Returning
isValid: truewhen unable to verify the key pair is misleading and could lead to security issues.📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -254,0 +314,4 @@} finally {setEncryptionLoading(false);}};🛠️ Refactor suggestion
Do not use fetch directly; route via main-axios.ts API helpers
Project policy: “All API interactions should go through main-axios.ts.” Replace the three fetch calls with centralized helpers for consistent base URL, JWT, error handling, and i18n.
Add these helpers to main-axios.ts:
Then import and use them here.
Also applies to: 289-317, 319-358
⚠️ Potential issue
Insecure file upload pattern - accepts all file types.
The file input accepts
*along with specific extensions, which allows users to upload any file type. This could lead to uploading malicious files or non-key files.Apply this diff to restrict file types:
📝 Committable suggestion
🤖 Prompt for AI Agents