From 8a80978aefe6ea67c7e5991a9dcb91cfdbf79ed8 Mon Sep 17 00:00:00 2001 From: Philipp Date: Thu, 4 Jun 2026 15:08:54 +0200 Subject: [PATCH] added security settings changed fixes and issues --- agent/src/config.js | 22 ++++-- agent/src/index.js | 10 ++- docs/fixes-todo.md | 39 +++++++++- docs/issues.md | 107 +++++++++++++++++++++++++++ frontend/src/components/Settings.jsx | 3 +- 5 files changed, 169 insertions(+), 12 deletions(-) diff --git a/agent/src/config.js b/agent/src/config.js index d4c2160..5ea39b4 100644 --- a/agent/src/config.js +++ b/agent/src/config.js @@ -83,10 +83,15 @@ export function resticProcessEnv() { export async function readEnvSettings() { const fileValues = await readEnvFile(); - return editableEnv.map((field) => ({ - ...field, - value: fileValues[field.key] ?? process.env[field.key] ?? '', - })); + return editableEnv.map((field) => { + const value = fileValues[field.key] ?? process.env[field.key] ?? ''; + // Secrets are write-only: never echo their value back to callers. The UI + // only learns whether a value is currently set via `hasValue`. + if (field.secret) { + return { ...field, value: '', hasValue: Boolean(value) }; + } + return { ...field, value }; + }); } export async function writeEnvSettings(values) { @@ -94,14 +99,19 @@ export async function writeEnvSettings(values) { const currentValues = await readEnvFile(); const nextValues = { ...currentValues }; + const secretKeys = new Set(editableEnv.filter((field) => field.secret).map((field) => field.key)); for (const [key, value] of Object.entries(values || {})) { if (!allowedKeys.has(key)) continue; - if (key === 'API_TOKEN' && String(value || '').length < minApiTokenLength) { + const text = String(value ?? ''); + // Secrets are write-only: an empty submission keeps the existing value so + // the masked UI does not wipe credentials when saving unrelated fields. + if (secretKeys.has(key) && text === '') continue; + if (key === 'API_TOKEN' && text.length < minApiTokenLength) { const error = new Error(`API_TOKEN must be at least ${minApiTokenLength} characters long.`); error.status = 400; throw error; } - nextValues[key] = String(value ?? ''); + nextValues[key] = text; } const body = editableEnv diff --git a/agent/src/index.js b/agent/src/index.js index 8c09ad8..f573413 100644 --- a/agent/src/index.js +++ b/agent/src/index.js @@ -1,5 +1,6 @@ import cors from 'cors'; import express from 'express'; +import crypto from 'node:crypto'; import http from 'node:http'; import https from 'node:https'; import { readFile } from 'node:fs/promises'; @@ -25,7 +26,7 @@ app.use((req, res, next) => { return; } const header = req.get('authorization') || ''; - if (header === `Bearer ${config.apiToken}`) { + if (config.apiToken && timingSafeEqual(header, `Bearer ${config.apiToken}`)) { next(); return; } @@ -36,6 +37,13 @@ function normalizeIp(value) { return String(value || '').replace(/^::ffff:/, ''); } +function timingSafeEqual(a, b) { + const bufA = Buffer.from(String(a)); + const bufB = Buffer.from(String(b)); + if (bufA.length !== bufB.length) return false; + return crypto.timingSafeEqual(bufA, bufB); +} + app.use('/api/health', healthRouter); app.use('/api/vms', vmsRouter); app.use('/api/snapshots', snapshotsRouter); diff --git a/docs/fixes-todo.md b/docs/fixes-todo.md index 39e1f63..4e4c4ef 100644 --- a/docs/fixes-todo.md +++ b/docs/fixes-todo.md @@ -46,6 +46,11 @@ Sortiert nach Risikoklasse. Datei-/Zeilenreferenzen beziehen sich auf den Stand - [x] **8. Bearer-Token aus Frontend-`localStorage` entfernen** ✅ erledigt - `frontend/src/api.js` — nur noch Cookie-Auth (`withCredentials: true`). `localStorage`-Token und `VITE_API_TOKEN` entfernt. +- [x] **45. Secrets in `GET /settings` maskieren (Write-only)** ✅ erledigt + - Datei: `agent/src/config.js` (`readEnvSettings`/`writeEnvSettings`), `frontend/src/components/Settings.jsx` + - Problem: `readEnvSettings` gab den Wert *aller* Felder zurück — inkl. `RESTIC_PASSWORD`, `AWS_SECRET_ACCESS_KEY`, `API_TOKEN`. Über den Management-Proxy konnte damit jeder eingeloggte UI-User das Restic-Verschlüsselungspasswort und die S3-Credentials enthüllen (= alle Backups entschlüsseln und löschen). + - Fix: Secret-Felder liefern beim Lesen keinen Wert mehr, nur `hasValue: true|false`. Beim Schreiben gilt ein leeres Secret-Feld als „unverändert" (bestehender Wert bleibt erhalten). Frontend zeigt Platzhalter „gesetzt – leer lassen zum Beibehalten". + --- ## 🟠 Hoch @@ -88,6 +93,21 @@ Sortiert nach Risikoklasse. Datei-/Zeilenreferenzen beziehen sich auf den Stand - Datei: `agent/src/routes/settings.js` - Hängt mit Fix #2 zusammen — nach #2 automatisch erfüllt, hier zur Sicherheit dokumentieren/testen. +- [x] **46. Agent-Token-Vergleich timing-safe machen** ✅ erledigt + - Datei: `agent/src/index.js:28` + - Problem: `header === \`Bearer ${config.apiToken}\`` — normaler String-Vergleich am root-Agent (das wertvollste Ziel), während das Login-Passwort bereits `timingSafeEqual` nutzte. + - Fix: Vergleich über `crypto.timingSafeEqual` mit Längen-Guard. Leerer/fehlender konfigurierter Token verweigert weiterhin (`config.apiToken &&`). + +- [ ] **47. Pre-Restore-Volumes aufräumen (Disk-Space-GC)** + - Datei: `agent/src/routes/restore.js` + - Aktuell: Jeder erfolgreiche Restore behält das alte Volume als `*.pre-restore-*` Rollback-Kopie — es gibt aber keinen Cleanup. Jeder Restore verdoppelt den Plattenbedarf der VM dauerhaft; bei mehreren Restores läuft der ZFS-Pool voll. + - Fix: Aufbewahrungsregel (z.B. „keep last N pre-restore/failed-restore Volumes pro VM") oder expliziter Cleanup-Schritt/UI-Aktion. Mindestens im Health/Operations-View sichtbar machen. + +- [ ] **48. Agent-Tokens im Management-SQLite nicht im Klartext speichern** + - Datei: `management/src/store.js` (`nodes.token`), `management/src/db.js` + - Aktuell: `nodes.token` liegt im Klartext in der Management-DB. DB-Diebstahl = alle Agent-Tokens = root auf allen Hosts. + - Fix: Verschlüsselung at-rest mit einem Management-Key (z.B. aus `SESSION_SECRET`/dediziertem Key abgeleitet), oder zumindest DB-Dateipermissions/Disk-Encryption-Anforderung im Deployment-Doc verbindlich dokumentieren. + --- ## 🟡 Mittel @@ -142,6 +162,11 @@ Sortiert nach Risikoklasse. Datei-/Zeilenreferenzen beziehen sich auf den Stand - [ ] **28. Pre-Backup VM-Zustand prüfen** - Live-Migration, laufende interne Snapshots, fehlende Berechtigungen → klare Fehler statt halb durchgeführter Pipeline. +- [ ] **49. Abgelaufene Sessions serverseitig löschen + Rotation** + - Datei: `management/src/store.js:12` (`getUserBySession`), `createSession` + - Aktuell: Abgelaufene Sessions werden beim Lesen nur gefiltert, nie aus der DB entfernt → unbegrenztes Tabellenwachstum. Außerdem keine Session-Rotation nach erfolgreichem Login. + - Fix: Periodischer Cleanup (`DELETE FROM sessions WHERE expires_at <= now`) und neue Session-ID nach Login ausstellen. + --- ## 🟢 Niedrig / Aufräumen @@ -187,11 +212,17 @@ Sortiert nach Risikoklasse. Datei-/Zeilenreferenzen beziehen sich auf den Stand | 7 | Backup-Verifikation | ⬜ offen | | 8 | Bearer-Token aus localStorage | ✅ erledigt | | 9 | Session-Cookie `Secure`-Flag | ⚠️ teilweise | +| 45 | Secrets in `GET /settings` maskieren | ✅ erledigt | +| 46 | Agent-Token-Vergleich timing-safe | ✅ erledigt | +| 47 | Pre-Restore-Volume-GC | ⬜ offen | +| 48 | Agent-Tokens in DB verschlüsseln | ⬜ offen | +| 49 | Session-Cleanup serverseitig | ⬜ offen | ## Nächste Prioritäten 1. **#7** Backup-Verifikation weiter härten — Pipeline-/Stream-Fehler testen und vollständiger absichern. -2. **#9** `SESSION_COOKIE_SECURE=true` in Deployment-Doku festschreiben. -3. **#12** Toten `SESSION_SECRET` entfernen. -4. **#15** ENV-Escaping vervollständigen (`$`, Backticks, Newlines). -5. **#11 Folgearbeit** ressourcenspezifische Cleanup-Recovery nach Agent-Crash definieren. +2. **#47** Pre-Restore-Volume-GC — sonst läuft der ZFS-Pool bei wiederholten Restores voll. +3. **#9** `SESSION_COOKIE_SECURE=true` in Deployment-Doku festschreiben. +4. **#12** Toten `SESSION_SECRET` entfernen. +5. **#15** ENV-Escaping vervollständigen (`$`, Backticks, Newlines). +6. **#11 Folgearbeit** ressourcenspezifische Cleanup-Recovery nach Agent-Crash definieren. diff --git a/docs/issues.md b/docs/issues.md index 10a6329..285b394 100644 --- a/docs/issues.md +++ b/docs/issues.md @@ -14,6 +14,14 @@ Several earlier backlog items have already landed in the codebase: Known caveat: agent-side jobs are persisted, but subprocesses cannot survive an agent restart. Active jobs are marked `failed` on startup; deeper cleanup recovery for partially changed host resources is still future work. +Current pre-production priorities: + +1. Harden backup verification and add tests for stream/pipeline failure cases. +2. Validate the staged restore workflow on a disposable Incus VM, including rollback scenarios. +3. Improve agent crash cleanup for partially changed ZFS/Incus resources. +4. Surface detailed node health diagnostics in the UI. +5. Keep the root-running agent tightly network-restricted. + ## 1. Persist final agent job status in management Status: mostly done. @@ -85,6 +93,8 @@ Ensure a backup is marked `success` only when the expected source data was fully - [x] Remove failed Restic snapshots with `forget` and `prune` on verification errors. - [ ] Make stream/pipeline failure handling explicit for all sources. - [ ] Avoid marking success if source stream closes early but Restic exits successfully. +- [ ] Add a post-backup Restic integrity check strategy, such as targeted `restic check`/`restic stats` usage that is safe for large repositories. +- [ ] Define an optional restore-probe workflow for critical VMs. - [ ] Add tests with mocked command failures and short reads. ### Acceptance Criteria @@ -92,6 +102,7 @@ Ensure a backup is marked `success` only when the expected source data was fully - [x] Successful backup jobs include a verifiable Restic snapshot ID in management history. - [x] Size mismatches fail the job. - [ ] Simulated source stream errors cannot produce a successful job. +- [ ] A backup can be independently verified without trusting only the successful process exit. - [ ] Verification behavior is covered by automated tests. ## 4. Add per-VM backup policy @@ -139,6 +150,8 @@ Reduce the risk of accidental or unsafe restores. - Optionally offer “create backup before restore” when the VM is accessible. - [x] Log restore intent in audit log before dispatch. - [x] Replace direct `dd` to production ZVOL with a staged restore workflow. +- [ ] Test staged restore on a disposable VM: backup, restore, boot, and confirm data. +- [ ] Test failed staged restore paths: Restic dump failure before swap, rename failure after old volume rename, and VM start failure. - [ ] Define a safe container restore workflow with `zfs receive` or disable container restore surfaces entirely. ### Acceptance Criteria @@ -147,6 +160,7 @@ Reduce the risk of accidental or unsafe restores. - Restore is blocked when required preflight checks fail. - Audit log records restore attempts and results. - [x] Restore stream failures happen on the staged volume, not the production disk. +- [ ] Staged restore behavior is validated on a real Incus/ZFS test node. ## 6. Add failure notifications @@ -240,3 +254,96 @@ Provide a production-ready Docker/Compose deployment for the management API and - Management database survives container recreation. - Cookie login works behind HTTPS. - Management can connect to HTTPS node-agents using the configured CA file. + +## 10. Add automated tests and CI + +The project currently has no automated tests. This is the biggest engineering gap for a backup system because most dangerous failures happen in error paths, not in the happy path. + +### Goal + +Catch regressions in validation, locking, backup verification, restore orchestration, authentication, and management polling before deployment. + +### Tasks + +- Add a test runner for agent and management code. +- Test validators for instance names, snapshot IDs, and ambiguous snapshot prefixes. +- Test job locking, persistence, trimming, and restart behavior. +- Test backup pipeline failure handling with mocked command/process failures. +- Test restore staged-volume command sequencing with mocked ZFS/Incus/Restic commands. +- Test management job polling updates `job_history` correctly for success, failed, timeout, and missing-agent cases. +- Add CI for install, tests, frontend build, and syntax checks. + +### Acceptance Criteria + +- Pull requests run tests automatically. +- Simulated backup stream failures fail the job. +- Simulated restore failures leave the original ZVOL name restored in the command sequence. +- Agent restart behavior is covered by tests. + +## 11. Harden root-running agent exposure + +The agent runs with root-level host access because it needs Incus, ZFS, `/dev/zvol`, Restic, and device operations. A compromised agent is therefore a host-level incident. + +### Goal + +Reduce the network and systemd blast radius of the root-running node-agent. + +### Tasks + +- Require private management-to-agent connectivity in production documentation, such as NetBird or a dedicated management network. +- Prefer HTTPS agents with a private CA; document local HTTP only as development mode. +- Enable and document `ALLOWED_MANAGEMENT_IPS` for production. +- Add systemd hardening where compatible with Incus/ZFS access. +- Add explicit installer warnings when `HTTPS_ENABLED=false` and no `ALLOWED_MANAGEMENT_IPS` is configured. +- Consider replacing `npm start` in systemd with direct `node src/index.js`. + +### Acceptance Criteria + +- Production install docs do not expose the agent publicly by default. +- Installer warns on insecure network exposure. +- systemd unit has a documented minimum hardening baseline. + +## 12. Improve agent crash cleanup and resource recovery + +Agent jobs are persisted and active jobs are marked `failed` on restart, but a process crash can still leave host resources behind, such as temporary Incus snapshots, staged restore volumes, visible snapshot devices, or changed ZFS properties. + +### Goal + +Make agent startup detect and clean up known leftover resources from interrupted jobs where doing so is safe. + +### Tasks + +- Persist cleanup metadata for backup jobs: temporary Incus snapshot name, ZVOL, snapdev state, and backup type. +- Persist cleanup metadata for restore jobs: staged ZVOL, backup ZVOL, failed ZVOL, and swap state. +- On startup, scan failed active jobs and run safe cleanup actions. +- Log cleanup results into the persisted job logs. +- Avoid destructive cleanup when state is ambiguous; surface manual action instead. + +### Acceptance Criteria + +- Crashing during backup streaming does not leave `snapdev=visible` or temporary snapshots unnoticed. +- Crashing before restore swap removes staged restore volumes when safe. +- Crashing after restore swap does not automatically destroy rollback copies. +- Management can show cleanup-required states. + +## 13. Improve health diagnostics in the UI + +The agent health endpoint returns useful structured details, but the management UI currently summarizes this too aggressively. Operators need actionable health reasons without opening logs. + +### Goal + +Show per-node health detail in the UI with concrete failed checks and messages. + +### Tasks + +- Add a detailed health drawer or modal on the Nodes page. +- Show command, ZFS pool, `/dev/zvol`, Restic repository, and credential check results. +- Show last health timestamp and the management-side error if the agent is unreachable. +- Distinguish unreachable, unauthorized, TLS failure, degraded health, and healthy states. +- Avoid global UI timeout banners when only one enabled node is slow or unreachable. + +### Acceptance Criteria + +- A missing command is visible by name in the UI. +- Wrong Restic credentials are visible as a Restic health failure. +- TLS or connectivity failures are distinguishable from degraded agent health. diff --git a/frontend/src/components/Settings.jsx b/frontend/src/components/Settings.jsx index 01707ee..67f837d 100644 --- a/frontend/src/components/Settings.jsx +++ b/frontend/src/components/Settings.jsx @@ -12,7 +12,7 @@ export function Settings({ nodes, onChanged }) { const [error, setError] = useState(''); const requiredMissing = useMemo( - () => fields.filter((field) => field.required && !String(values[field.key] || '').trim()), + () => fields.filter((field) => field.required && !field.hasValue && !String(values[field.key] || '').trim()), [fields, values], ); @@ -100,6 +100,7 @@ export function Settings({ nodes, onChanged }) { className="h-10 min-w-0 flex-1 bg-transparent px-3 text-zinc-100 outline-none" onChange={(event) => setValues((current) => ({ ...current, [field.key]: event.target.value }))} min={field.key.startsWith('RESTIC_KEEP_') ? '0' : undefined} + placeholder={field.secret && field.hasValue ? 'gesetzt – leer lassen zum Beibehalten' : undefined} type={inputType(field, secretVisible)} value={values[field.key] || ''} />