feat(ssl): add secure key wiping and tmpfs for SSL Bump CA keys#262
feat(ssl): add secure key wiping and tmpfs for SSL Bump CA keys#262
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.19% | 77.51% | 📈 +0.32% |
| Statements | 77.27% | 77.59% | 📈 +0.32% |
| Functions | 77.17% | 77.41% | 📈 +0.24% |
| Branches | 69.76% | 69.66% | 📉 -0.10% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
75.9% → 76.0% (+0.15%) | 75.2% → 75.4% (+0.15%) |
src/ssl-bump.ts |
32.1% → 60.2% (+28.02%) | 32.1% → 60.5% (+28.36%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
| } | ||
|
|
||
| // Open file for writing (not appending) | ||
| const fd = fs.openSync(filePath, 'w'); |
Check failure
Code scanning / CodeQL
Potential file system race condition High
| } | ||
|
|
||
| // Open file for writing (not appending) | ||
| const fd = fs.openSync(filePath, 'w'); |
Check failure
Code scanning / CodeQL
Insecure temporary file High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, to fix insecure temporary file issues in Node.js, avoid constructing paths in the OS temp directory from untrusted data and then creating files with fs.open/fs.openSync using generic modes like 'w'. Instead, either (a) use a library like tmp that creates securely‑permissioned, unique files, or (b) ensure the file is already expected to exist and open it with flags that will not silently create it (e.g., fs.constants.O_WRONLY | fs.constants.O_EXCL plus O_TRUNC on a verified existing file).
For this specific code, the best change with minimal impact is to harden secureWipeFile so that it can never create a new file: it should fail if the target file does not already exist at fs.openSync time. We already do an existence check, but that’s subject to a race. We can instead replace the call fs.openSync(filePath, 'w') with a call that uses explicit flags O_WRONLY | O_TRUNC, which by themselves still create a file if it does not exist, but we can add O_EXCL or use O_NOCTTY depending on platform. Node doesn’t support a “open only if exists” flag cross‑platform, but we can remove the initial existsSync and instead wrap openSync in a try/catch, treating ENOENT as a benign “file already gone” situation. This changes the flow to: try to open; if it fails with ENOENT, log and return (no file to wipe); otherwise proceed. Because we never create the file elsewhere using secureWipeFile, using fs.openSync(filePath, 'r+') (read/write) is the right semantic: it will fail with ENOENT instead of creating a new file. That fully removes the insecure “create file” behavior while preserving functionality: we still overwrite and delete existing sensitive files, and we safely ignore files that disappear between checks.
Concretely:
- In
src/ssl-bump.ts, insecureWipeFile, remove the initialfs.existsSynccheck. - Change
fs.openSync(filePath, 'w')tofs.openSync(filePath, 'r+')and handleENOENTfromopenSyncas a non‑error (just log debug and return), while other errors still go through the existing catch block. - Keep the rest of the overwrite and delete logic unchanged.
No changes are required in docker-manager.ts or its tests for this specific sink; we only need to harden the sink (secureWipeFile).
| @@ -184,11 +184,6 @@ | ||
| * @throws Error if file operations fail (after attempting cleanup) | ||
| */ | ||
| export async function secureWipeFile(filePath: string): Promise<void> { | ||
| if (!fs.existsSync(filePath)) { | ||
| logger.debug(`File not found for secure wipe: ${filePath}`); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const stats = fs.statSync(filePath); | ||
| const fileSize = stats.size; | ||
| @@ -200,8 +195,18 @@ | ||
| return; | ||
| } | ||
|
|
||
| // Open file for writing (not appending) | ||
| const fd = fs.openSync(filePath, 'w'); | ||
| // Open existing file for read/write (do not create if missing) | ||
| let fd: number; | ||
| try { | ||
| fd = fs.openSync(filePath, 'r+'); | ||
| } catch (openError: any) { | ||
| // If the file disappeared between stat and open, treat as already gone | ||
| if (openError && (openError as NodeJS.ErrnoException).code === 'ENOENT') { | ||
| logger.debug(`File not found for secure wipe (disappeared before open): ${filePath}`); | ||
| return; | ||
| } | ||
| throw openError; | ||
| } | ||
|
|
||
| try { | ||
| // Use chunked writing for memory efficiency |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Overall Status: ❌ FAIL cc: @Mossaka (author/assignee)
|
Smoke Test Results - Claude EngineLast 2 Merged PRs:
Test Results:
Overall Status: PASS
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.19% | 77.51% | 📈 +0.32% |
| Statements | 77.27% | 77.59% | 📈 +0.32% |
| Functions | 77.17% | 77.41% | 📈 +0.24% |
| Branches | 69.76% | 69.66% | 📉 -0.10% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
75.9% → 76.0% (+0.15%) | 75.2% → 75.4% (+0.15%) |
src/ssl-bump.ts |
32.1% → 60.2% (+28.02%) | 32.1% → 60.5% (+28.36%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test ResultsLast 2 merged PRs:
Test Results:
Status: PASS
|
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Overall: FAIL (Playwright test failed)
|
SSL Bump CA private keys were stored on disk with file permissions as sole protection, creating exposure windows during container escape scenarios.
Changes
Secure Key Wiping (
src/ssl-bump.ts)secureWipeFile(): Overwrites with crypto random data (3 passes, 64KB chunks), syncs to disk, then deletescleanupSslBumpFiles(): Wipes private key, removes certificates and SSL databasetmpfs for SSL Certificate Database
/var/spool/squid_ssl_db(64MB limit, mode 0700)Container Updates
cleanupSslBumpFiles()before deleting workDirTypes
tmpfsproperty toDockerServiceinterfaceExample
Tests
Added 7 tests covering secure wipe and cleanup edge cases.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.