Repository navigation
fix(security): harden trust boundaries with CI forward fix - #677
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
dec8f09 to
23785e6
Compare
gotempsh/postgres-walg:17-bookworm is in the allowlist but was never published (only 18-bookworm exists on Docker Hub), so the upgrade guard test failed pulling it. Use timescale/timescaledb-ha:pg17/pg18, which are both allowlisted and real, matching the pattern already used in postgres_upgrade.rs test fixtures.
postgres:18 was never in ALLOWED_POSTGRES_DOCKER_IMAGES (added by an earlier commit on this branch), so test_upgrade_postgres_image_parameter_update and test_can_update_postgres_docker_image failed creating their initial service. Use timescale/timescaledb-ha:pg18, which is allowlisted and distinct from the gotempsh/postgres-walg:18-bookworm target both tests update to, preserving the "same major version, different variant" intent.
|
Investigated the `Scenario E2E / Scenario (multinode mTLS)` failure on this run — looks like DinD networking flakiness rather than a regression from this branch. Control-plane logs show a completely clean boot: setup completes, migrations apply, and `temps serve` starts listening on `0.0.0.0:80` within ~13s of container start, with no errors or panics afterward (Console API, proxy server, all subsystems report ready). `worker-1` never gets past a bare TCP `Connection refused (os error 111)` trying to reach `10.52.0.10:80` (the control-plane's own address), across 90 retries × several full retry cycles, for the entire ~10 minute test. Since the control-plane is provably listening on all interfaces the whole time, this is a network-level failure between the two outer DinD containers, not an application issue — I checked the full diff for anything touching server bind addresses or internal-proxy routing to `/api/internal/nodes/register` and found nothing that would explain a network-layer refusal. I don't have admin rights on this repo to re-run just this job. Could someone with access kick off a re-run of this check when convenient? |
… exists compute_desired_enable_archiving() called walg_env_exists_in_container(), which requires a live container since Docker's archive-download API operates on a container's filesystem, not a volume directly. On any recreate-from-scratch path (force_recreate, node failover, or a container removed externally while the data volume persists), the probe failed closed and the recreated container baked archive_mode=off even though walg.env still existed on the volume -- contradicting the function's own documented guarantee that it survives container recreates, restarts, and node failovers. Fall back to a throwaway, never-started container that only mounts the named volume when no live container exists. The probe container never starts, so nothing in its image ever executes; it reuses the service's own already-vetted image rather than introducing a new one.
The new production_https redirect default (no external_url configured -> treat every host as production, force HTTPS) applied to every plain HTTP request through the proxy, including requests whose Host never matched a project domain. That incorrectly caught internal cluster-management API calls such as node registration/heartbeat, which run over plaintext HTTP by design before any TLS material exists — the multinode E2E cluster never configures external_url, so every join attempt got redirected off a listener with no TLS configured and the worker could never register. Extract the gating logic into should_apply_production_https_default() and require a resolved ctx.environment (actual project/environment traffic) before applying the production assumption, matching the feature's stated intent (CDN-terminated project environments) rather than all proxy traffic.
|
Traced the multinode mTLS failure — not DinD flakiness as I first guessed (thanks for the correction, David). Root cause: Pushed a fix ( This also fixes a more general case beyond this test: any fresh self-hosted install that hasn't configured Complementary to your harness change (real |
|
Update: the mTLS registration fix worked — this run got much further than before (worker joined, negotiated mTLS, project created, deployment kicked off) before failing at a new, later step: `worker application to GET http://production.e2e-msuwub0r-opnng5-mn.temps.local did not converge within 60s (last: {"code":1,"stdout":"","stderr":"wget: server returned error: HTTP/1.1 403 Forbidden\n"})` This is a different area than the redirect logic we just fixed — |
Summary
This replacement PR carries forward the security-hardening bundle from #676 and includes the forward fixes required by the current upstream test suite.
Forward fixes
gotempsh/postgres-walg:17-bookwormandgotempsh/postgres-walg:18-bookworm.https://control-plane.temps.test.The original #676 security-hardening commits remain attributed to their original contributor. The focused CI compatibility commits are layered on top so the complete branch can be reviewed and merged together.
Evidence
Shell and Compose contracts
All commands exited 0 with no diagnostics.
Scenario TypeScript
cd apps/temps-e2e bun install --frozen-lockfile bun run typecheckExit status: 0.
Generated certificate trust and hostname verification
An ephemeral container generated the same ECDSA certificate as the topology, installed its public certificate into the Debian trust store, started an OpenSSL TLS server, and verified the connection with:
The dedicated DinD image also built successfully with OpenSSL installed.
Full multinode runtime
The local ARM Docker VM exhausted memory (
rustcreceived SIGKILL while compilingtemps-entities) before the control plane could start. This path builds the full workspace inside Docker. GitHub CI uses the already-built Linux artifact, so the dedicated multinode job on this PR is the required full registration/deployment runtime proof.Review
/run, is mode0600, is never shared with the worker, and is removed after encrypted database import.