Round-3 audit: 28 contained fixes (TLS roots, lock guards, staging, env recovery, process groups) - #4
Merged
Merged
Conversation
…tion-window prune, fail-closed pins A01: attempt TLS paths move from the flat /deployments/caddy/tls/att root to /deployments/caddy/tls/att/<app>/<hash>.<id>/ (container-side /etc/caddy/tls/att/<app>/…). PruneAttempts sweeps only the pruning app's artifact root and TLS root; the legacy flat root is never swept — a flat entry's owning app cannot be proven, and sweeping it with one app's keep set deleted OTHER apps' live certificates whenever hashes differed (two apps can even share a hash string). A02: the attempt-prune protection window widens from current+previous+pins to also cover every release that still has containers on the server — keep_versions retention holds releases beyond current+previous whose records and Caddy routes reference attempt-scoped TLS/env bytes. A version leaving the keep window loses containers at step 15b of the same deploy, so its artifacts first prunable on the next. A03: an unreadable pin file now SKIPS attempt pruning entirely, matching version pruning's fail-closed policy, instead of pruning with current+previous protection only. NewAttempt also validates the app name against the config grammar (A17 boundary piece): attempt paths interpolate the app into host and container-side directories.
… staging A04: ReleaseLockFenced runs the release under the holdership guard when a fence handle is present — grep owner info, then rm -rf, in one shell invocation. After a takeover the stale holder's deferred release is refused (ErrFenceLost treated as success: the lock belongs to someone else and leaving it alone IS the correct outcome) instead of deleting the successor's lock and admitting a third operation. A nil handle keeps the historical unfenced release (admin unlock, pre-F16 paths). A handle for a different app is refused outright (A17 lease-correspondence rule, also enforced in WriteFenced). A06: WriteFenced and the lock renewal stage to unique owner-scoped siblings (state.json.tmp-<owner>-<nonce>) instead of the fixed state.json.tmp-fence / .lock/info.renew names shared by every generation, so a stale holder can no longer upload into the successor's staging path and ride its guarded rename into authority. Renewal staging also moves into the app directory: a stale renewal whose .lock was already removed can no longer recreate the directory (Upload's mkdir -p) and ghost-lock the app for a full staleLockTTL. MockExecutor now models rm -rf -- as a recursive removal of recorded files, so the guarded release is provable against recorded state.
…blish-branch route restore, detached compensation, honest recovery reporting, logged image A08: the same-version path no longer force-removes a RUNNING _replaced container — after a failed attempt renamed the serving predecessor, a retry used to delete it before any healthy replacement existed, taking the app down. A running _replaced is refused with recovery direction; a stopped corpse is still cleared. Rename failures are no longer swallowed: an unclassified failure with the source container still present aborts the deploy instead of leaving snapshot and candidate names disagreed. A10: abortStateCommit's fixed-port branch (host ingress OR any publish entry) restores the Caddy route for caddy-ingress apps before removing the candidates — a caddy+publish app's commit failure used to return from the branch with Caddy still pointing at the removed candidate names. On route-restore failure the candidates are restarted instead of routing to nothing. A11: abortStateCommit runs every compensation (stops, restarts, route restore, log) on a detached bounded recovery context — a commit failure caused by a cancelled deploy context used to skip them via the dead ctx. A13: restoreDisplacedAndStarted reports every failed stop/remove/restart (itemized in output and in the returned error) — 'restored' was true whenever zero candidates had started, and one successful restart of a multi-container recovery read as full recovery. A26: logDeploy populates LogEntry.Image (the open half of TCL-19).
…-scoped assets, plan validation, default port, image pinning, worker readiness, partial-run reconcile A15: asset bridging builds THIS attempt's private asset tree under meta/att/<hash>.<id>/assets (seeded from the previous attempt's tree with a real cp -a — hardlink seeding would let a later write truncate shared inodes) instead of copying into the shared /deployments/<app>/assets the running release still reads; a failed candidate can no longer mutate the live app's files. Extraction switches from 'docker run IMAGE sh -c cp' (image ENTRYPOINTs could wrap/replace it) to docker create + docker cp, and the volumes map is cloned before the mount is added, so the caller's map is no longer mutated through the Config copy. A52: DeployFenced resolves the immutable image ID once and creates every web replica and worker from it — a mutable tag re-pointed by a concurrent pull/build/tag between creates can no longer mix images within a release (the requested ref stays the recorded provenance; resolution failure warns and falls back to the requested reference). A17: Config.validate now checks identity grammar (app via config. ValidateName, version, process names), rejects unknown ingress modes, and rejects publish+replicas>1 (fixed ports cannot be load balanced). releasemeta.Path validates the app against the config grammar (was: non-empty only). SplitHostPort rejects ports outside 1..65535. A18: ContainerPort == 0 normalizes to 80 once at the top of DeployFenced and the normalized value drives host ports, Caddy upstreams, the diagnosis, and every create — a ':0' upstream can no longer be rendered. A23: workers must still be running (not exited/dead/restarting/unhealthy) one second after their detached run, or the deploy fails with full cleanup; an unreadable state inspect degrades to a warning. A14: a failed docker run reconciles the candidate name — a created-but- unstarted corpse is removed so the next deploy cannot collide; a RUNNING container under the name is never touched.
…s, ambiguity-refusing port inspectors, honest prune accounting
A19: the primary -p binding is built by publishBinding (net.JoinHostPort,
IP-validated, both port-ranged) and quoted like every other interpolated
argument — a bare IPv6 bind such as ::1 used to concatenate into an
ambiguous '::1:49152:80' and was the run command's only unquoted value.
A21: HostPort and InternalPort refuse containers with multiple DISTINCT
ports instead of taking the first field (publish: entries and multi-EXPOSE
images make those real; a wrong pick probed an auxiliary listener or
routed Caddy at one); HostBindIP reports '' on mixed binds. The release
record's TCL-14 primary designation remains the authority these fallbacks
defer to.
A25: PruneVersions counts a version as pruned only when every container
removal succeeded and returns the joined failures; the deploy caller
reports partial cleanup ('version prune incomplete') instead of printing
nothing or claiming failed removals as pruned.
…lation, derived provisioning identity A27: LocalExecutor.Upload mirrors RemoteExecutor.Upload's contract — the resident autodeploy path runs on this executor, so the remote hardening did not cover it. The write lands in a private sibling temp, is chmod'd and fsync'd BEFORE publication, and is renamed over the destination (replacing a leaf symlink itself, never following it); a failed or cancelled upload leaves the previous contents intact and no staging siblings behind. A28: local commands run in their own process group with Cancel killing the whole group (SIGKILL) and WaitDelay bounding the post-cancellation wait — exec.CommandContext's default killed only the shell, leaving descendants running with stdout/stderr open, which blocked CombinedOutput/Wait indefinitely on the resident engine and kept mutated state behind a released lease. Build-tagged: unix sets the group; the non-unix fallback kills the shell directly and is explicit about the weaker guarantee. A32: new PublicKeyBytes derives the provisioning public key FROM the requested private identity and verifies any existing .pub against it; setup uses it. PublicKeyPath no longer falls through to unrelated default .pub files for an explicit key (a key without .pub is an error pointing at derivation). The x/crypto/ssh import in remote.go is aliased (gossh) for the explicit references this adds.
…n AOF gate, post-stop snapshot + start compensation, collision-proof backup ids, unsigned cron grammar at the sink
A39: the .env commit is a set -eu script staging BOTH files as private
mktemp siblings on the destination filesystem — the old copy's 'cp -p'
ran inside an if without chaining, so its failure was skipped and the
destructive mv ran anyway, and the new env was published by a
cross-filesystem mv from /tmp chmod'd only after it was live.
A40: the redis restore's AOF preflight is a Go-level check requiring a
proven 'appendonly no' reply — the old 'config get appendonly | tail -n
1' masked a failed docker exec (auth, transport) as empty output and
fell through to the destructive RDB replacement; only a genuine
substring 'yes' refused. The backup script's gate is strict for the same
reason (set -eu aborts on the failed exec).
A41: the redis restore snapshots the original dump AFTER the stop (a
graceful shutdown writes a final RDB the old copy-before-stop could
miss), the copy is mandatory when a dump exists, and a failed final
docker start now invokes the same restore_original compensation as a
failed install instead of exiting with redis down.
A44: backup ids carry a random 16-hex suffix — two backups of one app in
the same second used to target the same S3 key and silently replace each
other. ValidateDate accepts both the legacy timestamp form and the new
ids (with a real-date check); LatestBackupDate ordering still works since
the timestamp remains the prefix.
A46: cron fields must be unsigned decimals ('+1' and signed steps are
rejected), and SetSchedule itself validates the schedule plus rejects
line breaks/NUL in command and marker — a direct caller can no longer
bypass validation or split one job into unintended crontab lines.
…content-authoritative replay protection, race-free bounded prefix writers, cancellation-aware fleet slots A51: PrefixWriter serializes its buffer (one writer is routinely handed to BOTH stdout and stderr of the same command — docker.ScanImage does — so concurrent stream copies raced before the shared sink lock), caps the partial-line buffer at 64 KiB (newline-free output used to buffer without bound), and surfaces write/flush failures into the deploy result instead of discarding them. parallelDeploy acquires its concurrency slot with a ctx-aware select and re-checks cancellation after (possibly) waiting, so a cancelled fleet operation no longer queues for slots or launches callbacks the caller already abandoned. A36: dedup persistence snapshots and writes under one mutex, publishes atomically (temp + rename), and reports failures to the log — concurrent requests used to race os.WriteFile on the same path, an older snapshot could overwrite a newer one, and every error was ignored. The delivery-ID header no longer suppresses anything: it is log metadata only, because every legitimate provider retry replays the same signed body (caught by content dedup), while a REUSED delivery ID carrying different authenticated content is a distinct event the old ID-only check wrongly swallowed.
…ding deferrals with evidence folded in
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
1 participant
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.
28 of 52 findings fixed with regression tests; 24 standing deferrals with round-3 evidence folded in. Race suite green.