Audit remediation and embedded web UI #3
Loading…
Reference in a new issue
No description provided.
Delete branch "audit-and-web-ui"
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 branch bundles two entangled bodies of work against
main:internal/web+web/) — an opt-in React management UI served bykbackupdover its own HTTP(S) listener, with an in-process gRPC bridge, setup-token auth, and committedweb/distembedded viago:embed. (Commits2e03d91,d7a717e.)internal/web/middleware.go), so they ship together.The durability core was re-confirmed sound and left unchanged: crash-safe
packs → index → snapshotordering with an upload barrier, unconditional post-backup verification that fails the run, atomic+fsync backend writes, and complete prune reachability for file and image snapshots.Audit fixes (most severe first)
ntfs$Bitmap(fragmented via$ATTRIBUTE_LIST, go-ntfs drops a continuation record) under-reported used blocks andcaptureVolumeholed out real data behind a "verified" image backup. The coverage guard sat only in the(0, nil)branch, but go-ntfs'sRangeReadersignals truncation as(0, io.EOF), bypassing it. Now theio.EOFpath is guarded → fail-safe to a full read.restore/imageOpenFileTargetlackedO_TRUNCwhilePreZeroed()returnedtrueunconditionally, so restoring over a pre-existing file left stale bytes in every blockmap hole — corrupt, reported success. AddedO_TRUNC.pruneErrRepoRolledBack.backend/retryretry.Listreplayed a non-idempotent callback on a transient error → duplicate snapshot IDs poisoned the anti-rollback counter, which could refuse all future backups. List is now attempt-safe;snapshot.Listdedups.repoopenCache's additive reconciliation let a stale cache reference a peer-pruned pack; dedup + verify would both pass on a dangling reference. Now rebuilds when an ingested segment is gone from the backend.capture/fstreewebOriginheader caused a pre-auth nil-pointer panic in the same-origin check; guarded.notifybaseCtx); now delivered on a background ctx with a bounded drain.jobrepoCloseno longer turns an advisory-lock-removal blip into a failed backup; made idempotent.prune/cacheTesting
go build ./...,go vet ./...clean.go test ./...— 52 packages, 0 failures.Deliberately out of scope (documented, not gaps)
Part A -- audit: a multi-agent find/verify pass (7 dimension finders, adversarial verification) surfaced 43 confirmed findings (1 critical, 19 high, 19 medium, 4 low); all are fixed here. Highlights: - lock: repository locks now fail closed (create-then-verify, reject on conflict) instead of warn-and-proceed; authoritative cross-host leasing tracked in docs/audit-backlog.md. - backend: local deletes + lazily-created dirs are fsynced; SFTP saves are atomic-or-fail with fsync failures propagated. - job/progress: bounded queue-overlap admission, child-context cleanup, terminal-run eviction unblocked, settings/maintenance race closed, slow SSE subscribers disconnected with ResourceExhausted. - crypto/restore/notify: tighter Argon2 caps + per-open KDF work budget; restore symlink escapes closed (O_NOFOLLOW / reparse-safe opens); webhook and shoutrrr errors redact secret-bearing URLs. - Windows: CBT fails closed on fingerprint problems; USN parse failures force a full read; VSS/CBT accept image-job volume spellings; ADS + metadata survive long paths. - Linux: dm-era state bound to metadata identity/geometry; LVM snapshot cleanup survives cancellation and whitespace mountpoints; setuid/setgid/ sticky + xattr/ACL restore fixed; raw-image restores fsynced. - service/config: RPC upserts enforce the same contract as YAML load; job delete rejected while a schedule references it; settings round-trips preserve non-proto fields; duplicate schedules rejected; overlay file gets the same Linux permission checks as the base config. - tooling: 0600 packaged config, ProgramData ACL hardening, clean-room proto regen, deb/rpm version normalization, postinstall failure propagation, pinned goversioninfo, conservative systemd hardening. - daemon log tightened to owner-only (0600/0700). See docs/AUDIT.md for the full findings table. Part B -- web UI: opt-in HTTP(S) listener embedded in kbackupd serving a React SPA (go:embed) plus a JSON/SSE API that calls the existing gRPC service implementations in-process (one behavior across both transports). - config: root web{} block (base-file-owned); non-loopback binds require TLS or an explicit insecure_http acknowledgment. - auth: first-run setup token written to an owner-only file; Argon2id verifier; in-memory sessions; CSRF synchronizer + scheme/host Origin checks on every mutation; per-IP login rate limiting with lockout; context-aware + globally-bounded KDF gate. - API: full /api/v1 surface (runs/jobs/repos/snapshots/restores/schedules/ maintenance/mounts/notifications/settings), SSE progress stream with session re-validation and shutdown-aware cancellation, lazy tree browse. - frontend: React 19 + Vite + Tailwind 4 + shadcn/ui + TanStack Query + protobuf-ES types; management-parity pages; committed web/dist. - packaging/docs: Build-Installer.ps1 and build-packages.sh rebuild the UI (Node prereq); README + config examples document setup, TLS policy, and password recovery. Verified: go build/vet/test ./... green on Windows; GOOS=linux build/vet for platform-only fixes; go test -race ./internal/web; frontend typecheck/lint/test/build; a real-daemon HTTP e2e (setup -> login -> repo create -> job run -> live SSE summary -> history).mark.go still claimed image snapshots were unwalkable ("nil in M8, M10 doesn't exist yet") though WalkBlockMap is wired via DefaultConfig. ingest.go warned pack-stat writes would need read-modify-write "once prune exists" -- prune exists and safely recomputes stats from the live set, so the stored zeros are harmless. Comment-only.kReview review
Verdict: 1 High, 3 Medium
High risk remains because repository lock objects still expire during operations lasting more than six hours, allowing conflicting destructive work to proceed. Backend listings are also fully buffered in memory before callbacks run.
High (1)
internal/lock/repolock.go:99A backup, restore, or prune lasting beyond the six-hour
DefaultTTLcan lose its cross-process exclusion while still running, allowing another process to treat its lock as stale and begin conflicting work. The acquisition path writes and scans the lock once, then returns without any visible renewal mechanism; refresh the lock periodically before expiry until release.Suggested fix: Start a bounded renewal loop after successful acquisition that rewrites the same lock with a refreshed timestamp well before TTL expiry, stop it during release, and fail the active operation if renewal cannot be confirmed.
Medium (3)
Avoid buffering the entire backend listing —
internal/backend/retry/retry.go:326Repositories with very large object listings can exhaust daemon memory because every
backend.FileInfois accumulated before the first caller callback runs. This also delays streaming consumers until enumeration completes; preserve attempt safety using bounded paging, a temporary disk-backed spool, or backend-specific continuation support rather than an unbounded slice.Suggested fix: Replace the unbounded in-memory slice with a bounded or disk-backed attempt buffer, or extend the backend listing contract with stable pagination so completed pages can be forwarded without replaying earlier callbacks.
Preserve Linux target ownership during replacement —
internal/restore/image/target_linux.go:55Restoring over an image owned by another UID/GID replaces it with a fresh inode owned by the daemon, potentially changing who can manage or access the file. The replacement copies only permission bits with
Fchmod; preserve the existingst.Uidandst.GidwithFchownbefore renaming the staged file into place.Suggested fix: Capture the existing regular file's UID and GID from
Fstatatand apply them to the temporary file withFchownbeforeRenameat, handling privilege failures explicitly.Preserve the full Windows security descriptor —
internal/restore/image/target_windows.go:274Replacing a Windows image target creates a new file owned by the restoring account and applies only its DACL; owner, primary group, and auditing metadata are discarded or inherited. For a target owned by another principal, this changes its security descriptor even though the restore otherwise claims to preserve permissions.
Suggested fix: Capture the existing file's owner, group, DACL and applicable control flags, then apply them to the staged file before the rename; preserve SACL metadata when the process has the required privilege.
Excluded as generated or vendored (not reviewed):
17 excluded files
build/linux/README.mdbuild/linux/build-packages.shbuild/linux/nfpm.yamlbuild/linux/scripts/postinstall.shbuild/linux/scripts/postremove.shbuild/linux/scripts/preremove.shbuild/linux/systemd/kbackupd.servicebuild/windows/Build-Installer.ps1build/windows/installer/README.mdbuild/windows/installer/harden-data-acl.ps1build/windows/installer/kbackup.exe.manifestbuild/windows/installer/kbackup.issbuild/windows/installer/versioninfo.jsonweb/dist/assets/index-BDMIidHk.jsweb/dist/assets/index-BlrQG5F3.cssweb/dist/index.htmlweb/package-lock.jsonReviewed by kReview at
cfc1ad8f50. This comment is conservative and based only on the PR diff and metadata.Est. cost ~$115.78 total (20.6M in / 563.1k out) · this run ~$2.61 (469.8k in / 10.3k out) / gpt-5.6-sol.
Handles the reviewer's second-pass findings (validated against the code; one suggested fix was rejected as unsafe -- see below). Integrity / security: - crypto/kdf.go: tighten the per-attempt Argon2 caps a repository-controlled keyfile may request (1 GiB/12/16 -> 256 MiB/6/8), bounding a single pre-auth unlock's allocation. Decouple repo's cumulative keyfile-unlock budget from the per-attempt cap (max of "16 default keyfiles" and "one max-cost attempt") so tightening the caps never rejects a legitimate multi-keyfile repo. - restore/files: eliminate the intermediate-symlink TOCTOU on Linux -- every restore create/open now resolves the target's parent via openat2(RESOLVE_NO_SYMLINKS) (fallback: component-by-component openat(O_NOFOLLOW)) and acts relative to that verified dirfd (mkdirat/openat/symlinkat), so no operation re-resolves an attacker-controlled parent path. Behavior otherwise preserved; the residual path-based pre-checks in node.go can now only cause a spurious restore failure, never a misdirected write. NOTE: the Linux traversal is COMPILE-VERIFIED ONLY (built and vetted under GOOS=linux); its runtime behavior is UNVERIFIED (no Linux host here) and must be exercised on a real Linux kernel before merge. - restore/files/open_windows.go: normalize the restore file path to extended-length \?\ form (mirroring fsmeta's win32Path), restoring >MAX_PATH support; device/VSS namespace paths pass through unchanged. - web/ratelimit.go: gate the progressive login lockout on tlsActive, so a TLS-terminating reverse proxy's shared RemoteAddr can no longer let one client trigger a global admin lockout; the token bucket + global KDF admission cap still bound guessing when behind a proxy. - repo/write.go: bounded, fresh-context retries when removing the advisory lock object (already-gone counts as success), shrinking the window a transient backend failure could strand it. Correctness / UX: - web notification detail: add GET /api/v1/notifications/{name} returning a REDACTED ChannelConfigSpec (name+type only, never the secret URL/params), so the edit dialog loads without leaking secrets. (The reviewer's suggested "return the full spec" would have leaked webhook secrets that ListChannels deliberately redacts -- rejected in favor of the redacted variant.) - web repo edit form: make the repository name immutable in edit mode and always submit the original name, so an edit no longer orphans the config. - web/auth_store.go: count password length in runes, not bytes. - web format.ts: guard timestamps against JS Date's actual +/-8.64e15 range to avoid a RangeError. - cmd/kbackupd/logfile.go: chmod existing log dir/file to 0700/0600 on Unix so upgraded installs don't keep world-readable logs (no-op on Windows). web/dist rebuilt from the two source changes (typecheck/lint/test/build/ verify:dist all pass).Data-safety: an apply could permanently forget a different set of snapshots than the operator previewed and confirmed (e.g. a scheduled backup created a new snapshot between preview and apply). ApplyForget now accepts the previewed forget IDs and refuses (ErrForgetSetChanged -> FAILED_PRECONDITION) without deleting anything if the recomputed set differs. The web apply pins the previewed decisions; the automated space-reclaim caller stays unbound. - api: add ApplyForgetRequest.expected_forget_ids (regenerated Go/TS) - prune: ApplyForget expected-set guard + order-independent set compare - service: map ErrForgetSetChanged to FAILED_PRECONDITION - web: send expectedForgetIds from the previewed decisions; refresh on reject - web: changePassword sends {currentPassword,newPassword}; reset tree expansion/path when the repo or snapshot changes- web/auth: close a TOCTOU where a login racing a password change could mint a session after revokeAll ran, letting an old-password holder survive the change. changeMu is now an RWMutex: handleLogin holds the read lock from verifier capture through issueSession; the change keeps the write lock through revokeAll. (-race clean) - repos form: switching SFTP auth type now also sets the clear flag for the now-inactive secret, so an edit cannot leave the old password/passphrase persisted alongside the new key -- the config contract is "exactly one of password or private_key_path". - schedules: rewrite nextCronFire to advance field by field (skip whole non-matching months/days/hours) instead of scanning up to ~4.7M minutes synchronously; a valid-but-impossible expression (e.g. "0 0 31 2 *") now settles in a few hundred iterations rather than freezing the UI on render. - schedules: reject malformed cron ranges/steps with extra separators ("1-2-3", "1/2/3", "1-", "-5") instead of silently truncating them. - tests lock the new cron walk (impossible-date null, daily/same-day fire, dom/dow OR semantics, step ranges) and the tightened validation.- config: reject an enabled web.listen with an empty port (trailing colon). net.SplitHostPort accepts "127.0.0.1:" and the port was discarded, so the listener would bind an arbitrary ephemeral port instead of the operator's intended one, contradicting the documented explicit-port rule. - restore (windows): handle a volume/UNC-share root as the restore target. filepath.Dir("C:\\")=="C:\\" and Base=="\\", so the anchored parent+leaf open passed a bare separator as the object name under a non-null RootDirectory. Open such roots directly by NT path (retaining OBJ_DONT_REPARSE). Linux is unaffected: openat ignores the dirfd for an absolute path. Adds a non-mutating volume-root open test. - web (jobs): don't validate the hidden source array. The default empty folder path stayed validated after switching sourceType to image, blocking submit. Source-array items are now validated per the selected sourceType in superRefine; excludes keep per-item validation. Adds a regression test. - api (mounts): drop the stale "Unimplemented until M13" note from MountService's doc comment. The service is implemented and tested on both platforms (fuse/winfsp) and wired through the web bridge; regenerated Go+TS. retry.go listing buffering is intentionally unchanged: buffering the page is required for exact-snapshot listing consistency (streaming was a reverted regression).nextCronFire decided the crontab(5) day-of-month / day-of-week OR-vs-AND rule with fields[i].includes("*"), but Vixie cron treats a day field as unrestricted only when it *begins* with "*". A field like "1,*/2" contains a "*" yet starts with a value, so it is restricted and must OR with a restricted day-of-week. The old check wrongly treated it as unrestricted and ANDed, so "0 0 1,*/2 * 1" previewed the next fire as Mon Jul 27 instead of Mon Jul 20. Use startsWith("*") and add a regression test for the mixed-list case.