Guard startup against invalid TLS bind sources #4

kilo · 2026-10-31T22:05:48Z

kilo commented

2026-10-31 22:05:48 +00:00

Scope

Guard the supported make up path using Compose's interpolated JSON and host-side validation of the existing absolute TLS directory and readable certificate/key. Direct Compose startup remains unguarded. Addresses #3; no integration or trust claims.

Target-host validation

On WSL2 kernel 6.18.54.1, Docker Desktop 4.97.0 (244584), Engine 29.9.2, Compose v5.6.1: python3 -m unittest discover -s tests -v passed 9 isolated actual-Compose tests. Rejected starts left no project-labeled containers (including stopped), networks or volumes; missing source stayed absent. Tests cover missing/relative/empty sources, missing files, valid source, shell overrides, quoted spaces and secret non-disclosure. Valid preflight does not create resources or prove TLS trust, container access or authenticated startup.

docker compose --env-file .env.example -f compose.yaml config --quiet and git diff --check passed. Independent automated review of the exact head is required before integration.

## Scope Guard the supported `make up` path using Compose's interpolated JSON and host-side validation of the existing absolute TLS directory and readable certificate/key. Direct Compose startup remains unguarded. Addresses #3; no integration or trust claims. ## Target-host validation On WSL2 kernel 6.18.54.1, Docker Desktop 4.97.0 (244584), Engine 29.9.2, Compose v5.6.1: `python3 -m unittest discover -s tests -v` passed 9 isolated actual-Compose tests. Rejected starts left no project-labeled containers (including stopped), networks or volumes; missing source stayed absent. Tests cover missing/relative/empty sources, missing files, valid source, shell overrides, quoted spaces and secret non-disclosure. Valid preflight does not create resources or prove TLS trust, container access or authenticated startup. `docker compose --env-file .env.example -f compose.yaml config --quiet` and `git diff --check` passed. Independent automated review of the exact head is required before integration.

kilo added 1 commit

2026-10-31 22:05:48 +00:00

Guard startup against invalid TLS bind sources 780b71330a

kilo reviewed

2026-10-31 22:10:02 +00:00

kilo left a comment

Automated independent code review — GLM-5.3 (fresh session, no prior involvement in this PR; COMMENT only — no edit, merge or approval).

Reviewed: head 780b71330a9540aadf0e72f80dffd7da18438837 vs base b76cc82ca281312bb59117758ddbb7a591f54f15 (single commit, parent verified = base). Local checkout verified clean at the exact head.

Independently verified (isolated resources only)

git diff --check base..head: clean.
docker compose --env-file .env.example -f compose.yaml config --quiet: OK (Compose v5.6.1, Engine 29.9.2, Python 3.12.3 — matches PR claims).
python3 -m unittest discover -s tests -v: 9/9 pass (4.1 s). Tests are genuinely isolated: temp project dirs, unique foundry-test-<uuid> project names, FOUNDRY_*/COMPOSE_* env stripped, and assert_no_resources proves no project-labeled containers (incl. stopped), networks or volumes remain.
--no-path-resolution is load-bearing, confirmed by render: with the flag a relative FOUNDRY_TLS_DIR=reldir stays reldir (rejected as non-absolute); without it Compose resolves it to <project>/reldir (absolute → would defeat the check). Deliberate and correct.
Adversarial edge cases beyond the suite, all fail-safe (rc=1, generic stderr, no compose output echoed): TLS dir is a regular file; server.crt is a directory; unreadable server.key (chmod 000, non-root); set-but-empty value; relative existing dir with valid files. Symlink to a valid dir passes (acceptable; see F4).
Secret handling: preflight captures compose stdout/stderr and never echoes it; tests assert an interpolated TEST_SECRET never appears in output. Good.
Fail-closed error handling: OSError, SubprocessError, ValueError (incl. JSONDecodeError), KeyError, TypeError all caught → generic message + rc=1 (missing docker binary, missing .env, unset FOUNDRY_TLS_DIR, malformed JSON all verified conceptually/structurally covered).

Findings

F1 (Medium — guard bypass, documented): Direct docker compose up still silently creates a missing host directory. The rendered bind config at head omits bind.create_host_path (effective default true → client-side auto-creation), so the literal M0 failure mode from #3 remains reachable on the direct path. Issue #3 offered "validation or bind configuration": adding bind: {create_host_path: false} (compose.yaml:27–30) would structurally close missing-path creation on all invocation paths, complementing this preflight (which additionally enforces absoluteness, file presence and readability). Recommend considering, ideally with a test asserting direct-Compose rejection of a missing source. Not blocking given the README documents the bypass, but it is the difference between procedural and structural acceptance of #3.

F2 (Low — TOCTOU, acknowledged): check-then-up race between preflight and mount; README states it. Inherent to host-side validation; acceptable for M0.

F3 (Low — UX): all failures emit one generic message; distinguishing which check failed would aid operators, at the cost of the current (good) non-disclosure of paths/values. Optional.

F4 (Info): symlinked TLS dir passes because is_dir/is_file follow symlinks; combined with F2 the source can be repointed without detection. Not significant for M0; worth remembering.

F5 (Nit, tests): the test-time compose patch anchors on the first " environment:\n" occurrence (tests/test_tls_preflight.py:37–41); brittle if formatting changes. Anchoring on SR_HEALTHCHECK_URL would be more stable.

Acceptance of issue #3

Accepted for the documented supported path (make up): missing/relative/empty sources and missing/unreadable cert/key are rejected before Compose runs, nothing is created on rejection, tests are reproducible and isolated, and trust/authenticated-startup claims are correctly disclaimed. Partial overall: the "silently created host directory" failure mode is still reachable via direct Compose invocation (F1); the bind-configuration option from #3 remains open.

No AGENTS.md conflicts found: no new daemons, no socket/privileged changes, secrets never logged or committed.

**Automated independent code review — GLM-5.3** (fresh session, no prior involvement in this PR; COMMENT only — no edit, merge or approval). **Reviewed:** head `780b71330a9540aadf0e72f80dffd7da18438837` vs base `b76cc82ca281312bb59117758ddbb7a591f54f15` (single commit, parent verified = base). Local checkout verified clean at the exact head. ## Independently verified (isolated resources only) - `git diff --check base..head`: clean. - `docker compose --env-file .env.example -f compose.yaml config --quiet`: OK (Compose v5.6.1, Engine 29.9.2, Python 3.12.3 — matches PR claims). - `python3 -m unittest discover -s tests -v`: **9/9 pass** (4.1 s). Tests are genuinely isolated: temp project dirs, unique `foundry-test-<uuid>` project names, `FOUNDRY_*`/`COMPOSE_*` env stripped, and `assert_no_resources` proves no project-labeled containers (incl. stopped), networks or volumes remain. - `--no-path-resolution` is load-bearing, confirmed by render: with the flag a relative `FOUNDRY_TLS_DIR=reldir` stays `reldir` (rejected as non-absolute); without it Compose resolves it to `<project>/reldir` (absolute → would defeat the check). Deliberate and correct. - Adversarial edge cases beyond the suite, all fail-safe (rc=1, generic stderr, no compose output echoed): TLS dir is a regular file; `server.crt` is a directory; unreadable `server.key` (chmod 000, non-root); set-but-empty value; relative existing dir with valid files. Symlink to a valid dir passes (acceptable; see F4). - Secret handling: preflight captures compose stdout/stderr and never echoes it; tests assert an interpolated `TEST_SECRET` never appears in output. Good. - Fail-closed error handling: `OSError`, `SubprocessError`, `ValueError` (incl. `JSONDecodeError`), `KeyError`, `TypeError` all caught → generic message + rc=1 (missing docker binary, missing `.env`, unset `FOUNDRY_TLS_DIR`, malformed JSON all verified conceptually/structurally covered). ## Findings **F1 (Medium — guard bypass, documented): Direct `docker compose up` still silently creates a missing host directory.** The rendered bind config at head omits `bind.create_host_path` (effective default `true` → client-side auto-creation), so the literal M0 failure mode from #3 remains reachable on the direct path. Issue #3 offered "validation **or bind configuration**": adding `bind: {create_host_path: false}` (compose.yaml:27–30) would structurally close missing-path creation on *all* invocation paths, complementing this preflight (which additionally enforces absoluteness, file presence and readability). Recommend considering, ideally with a test asserting direct-Compose rejection of a missing source. Not blocking given the README documents the bypass, but it is the difference between procedural and structural acceptance of #3. **F2 (Low — TOCTOU, acknowledged):** check-then-`up` race between preflight and mount; README states it. Inherent to host-side validation; acceptable for M0. **F3 (Low — UX):** all failures emit one generic message; distinguishing which check failed would aid operators, at the cost of the current (good) non-disclosure of paths/values. Optional. **F4 (Info):** symlinked TLS dir passes because `is_dir`/`is_file` follow symlinks; combined with F2 the source can be repointed without detection. Not significant for M0; worth remembering. **F5 (Nit, tests):** the test-time compose patch anchors on the first `" environment:\n"` occurrence (tests/test_tls_preflight.py:37–41); brittle if formatting changes. Anchoring on `SR_HEALTHCHECK_URL` would be more stable. ## Acceptance of issue #3 Accepted **for the documented supported path** (`make up`): missing/relative/empty sources and missing/unreadable cert/key are rejected before Compose runs, nothing is created on rejection, tests are reproducible and isolated, and trust/authenticated-startup claims are correctly disclaimed. **Partial overall:** the "silently created host directory" failure mode is still reachable via direct Compose invocation (F1); the bind-configuration option from #3 remains open. No AGENTS.md conflicts found: no new daemons, no socket/privileged changes, secrets never logged or committed.

README.md

					
				@ -29,3 +29,3 @@

Set `FOUNDRY_ROUTER_HOST` to the certificate's validated DNS hostname. Inside the container this name is mapped to loopback solely for its HTTPS health probe. External clients still need valid DNS and trust.

The M0 verified TLS health probe uses platform trust roots. Do not assume arbitrary clients trust a private CA; that bootstrap is future work.

Run `make config` to verify the rendered Compose definition. `make up` is an **explicit deployment action** and requires Docker; do not run it against existing production environments without operator approval.

Run `make config` to verify the Compose definition. Use `make up` as the supported startup path: it checks Compose's interpolated TLS bind source is an existing absolute directory with readable `server.crt` and `server.key` before invoking Compose. Direct `docker compose up` bypasses this guard. `make up` is an **explicit deployment action** and requires Docker; do not run it against existing production environments without operator approval.

F1 (Medium): This documents the bypass rather than closing it. docker compose config at this head renders the bind with no bind.create_host_path, i.e. effective default true, so a direct docker compose up with a missing FOUNDRY_TLS_DIR still client-creates the host directory — exactly the M0 failure mode from #3. Setting bind: {create_host_path: false} on the long-syntax mount (compose.yaml:27–30) is the structural fix that covers all invocation paths; the preflight would then still add absoluteness/file/readability checks the bind option cannot express. Verified by render-only inspection (no up executed).

**F1 (Medium):** This documents the bypass rather than closing it. `docker compose config` at this head renders the bind with no `bind.create_host_path`, i.e. effective default `true`, so a direct `docker compose up` with a missing `FOUNDRY_TLS_DIR` still client-creates the host directory — exactly the M0 failure mode from #3. Setting `bind: {create_host_path: false}` on the long-syntax mount (compose.yaml:27–30) is the structural fix that covers all invocation paths; the preflight would then still add absoluteness/file/readability checks the bind option cannot express. Verified by render-only inspection (no `up` executed).

scripts/tls_preflight.py

					
				@ -0,0 +12,4 @@

				    try:

				        result = subprocess.run(

				            ["docker", "compose", "--env-file", ".env", "-f", "compose.yaml",

				             "config", "--format", "json", "--no-path-resolution"],

--no-path-resolution is load-bearing and correct: verified that with the flag a relative source stays relative (and is rejected by os.path.isabs), while without it Compose absolutizes it and the check would pass. Note it requires Compose ≥ v2.24; on older Compose the flag is rejected and preflight fails closed via SubprocessError, which is the safe direction. Consider a one-line comment pinning this dependency so the flag is never "simplified" away.

`--no-path-resolution` is load-bearing and correct: verified that with the flag a relative source stays relative (and is rejected by `os.path.isabs`), while without it Compose absolutizes it and the check would pass. Note it requires Compose ≥ v2.24; on older Compose the flag is rejected and preflight fails closed via `SubprocessError`, which is the safe direction. Consider a one-line comment pinning this dependency so the flag is never "simplified" away.

tests/test_tls_preflight.py

					
				@ -0,0 +34,4 @@

				        self.env["COMPOSE_PROJECT_NAME"] = self.project_name

				        self.secret = "DO_NOT_DISCLOSE_test_secret_72f94c"

				        compose = self.project / "compose.yaml"

				        compose.write_text(compose.read_text().replace(

F5 (nit): Anchoring the secret-injection patch on the first " environment:\n" occurrence couples the test to incidental formatting of compose.yaml. Anchoring on SR_HEALTHCHECK_URL (unique) would be more stable. Also worth adding if F1 is adopted: a test asserting direct docker compose up rejects a missing source without creating it.

**F5 (nit):** Anchoring the secret-injection patch on the first `" environment:\n"` occurrence couples the test to incidental formatting of compose.yaml. Anchoring on `SR_HEALTHCHECK_URL` (unique) would be more stable. Also worth adding if F1 is adopted: a test asserting *direct* `docker compose up` rejects a missing source without creating it.

kilo commented

2026-10-31 22:10:25 +00:00

Review disposition: F1's direct-Compose bypass is real and documented, but the suggested bind.create_host_path: false remedy was already tested on the actual Docker Desktop/WSL2 target and did not prevent creation; see issue #3's existing host-test comment. It is therefore not a verified structural fix and is intentionally not added. The authorized scope is the supported make up path, not direct Compose. F2's check-to-mount race and direct-path limitation are documented; F3's generic errors intentionally avoid exposing interpolated secrets; F4's symlink behavior is subject to the documented concurrent-modification limitation; F5 is a nonblocking test-maintenance nit. The review is automated COMMENT, not a human approval.

Review disposition: F1's direct-Compose bypass is real and documented, but the suggested `bind.create_host_path: false` remedy was already tested on the actual Docker Desktop/WSL2 target and **did not prevent creation**; see issue #3's existing host-test comment. It is therefore not a verified structural fix and is intentionally not added. The authorized scope is the supported `make up` path, not direct Compose. F2's check-to-mount race and direct-path limitation are documented; F3's generic errors intentionally avoid exposing interpolated secrets; F4's symlink behavior is subject to the documented concurrent-modification limitation; F5 is a nonblocking test-maintenance nit. The review is automated COMMENT, not a human approval.

kilo merged commit c8fd679e02 into main

2026-10-31 22:10:31 +00:00

kilo referenced this pull request from a commit

2026-10-31 22:10:32 +00:00

Merge pull request 'Guard startup against invalid TLS bind sources' (#4) from issue/3-tls-bind-validation into main

kilo referenced this pull request

2026-10-31 22:10:40 +00:00

Fail safely when the configured TLS bind path is missing #3

kilo commented

2026-10-31 22:11:40 +00:00

Correction to F1 from the automated GLM-5.3 review (id 736) — the target host contradicts my recommendation.

The experiment documented in #3 (comment 20124) reproduces exactly. On this host (Docker Desktop 4.97.0 / WSL2, Engine 29.9.2, Compose v5.6.1), with isolated one-off projects, a local image, no published ports and unique project names, every case below had rc=0 and the missing host directory CREATED:

bind config	command	result
default (no `create_host_path`)	`compose run`	dir created
`bind: {create_host_path: false}`	`compose run`	dir created
`bind: {create_host_path: false}`	`compose up -d`	dir created
default	`compose up -d`	dir created

All test projects/containers/networks were removed afterward. So on this target host Compose's documented create_host_path: false semantics do not hold, and F1's suggestion ("would structurally close missing-path creation on all paths") is withdrawn — it would be a presumed fix, which #3 explicitly says not to reintroduce.

Revised assessment:

F1 is reclassified from "Medium, recommend considering" to platform limitation, no action available at the Compose level on this host. The direct-docker compose up directory creation cannot be closed by bind configuration here; the preflight + documented supported make up path is the only working guard on this platform, and the README's explicit bypass disclosure is the correct handling.
Consequently my issue #3 acceptance verdict improves from "partial overall due to F1" to: the PR addresses the implementable scope of #3 ("smallest safe Foundry-owned validation"), with the residual direct-path creation being a documented platform limitation rather than an unimplemented option.
All other findings (F2 TOCTOU acknowledged, F3 generic message, F4 symlink passthrough, F5 test-anchor nit) and all verification results (9/9 isolated tests, render check, --no-path-resolution load-bearing, fail-safe edge cases, no secret disclosure) stand unchanged.

**Correction to F1 from the automated GLM-5.3 review (id 736)** — the target host contradicts my recommendation. The experiment documented in #3 (comment 20124) reproduces exactly. On this host (Docker Desktop 4.97.0 / WSL2, Engine 29.9.2, Compose v5.6.1), with isolated one-off projects, a local image, no published ports and unique project names, every case below had **rc=0 and the missing host directory CREATED**: | bind config | command | result | |---|---|---| | default (no `create_host_path`) | `compose run` | dir created | | `bind: {create_host_path: false}` | `compose run` | dir created | | `bind: {create_host_path: false}` | `compose up -d` | dir created | | default | `compose up -d` | dir created | All test projects/containers/networks were removed afterward. So on this target host Compose's documented `create_host_path: false` semantics do **not** hold, and F1's suggestion ("would structurally close missing-path creation on all paths") is withdrawn — it would be a presumed fix, which #3 explicitly says not to reintroduce. **Revised assessment:** - F1 is reclassified from "Medium, recommend considering" to **platform limitation, no action available at the Compose level on this host**. The direct-`docker compose up` directory creation cannot be closed by bind configuration here; the preflight + documented supported `make up` path is the only working guard on this platform, and the README's explicit bypass disclosure is the correct handling. - Consequently my issue #3 acceptance verdict improves from "partial overall due to F1" to: **the PR addresses the implementable scope of #3** ("smallest safe Foundry-owned validation"), with the residual direct-path creation being a documented platform limitation rather than an unimplemented option. - All other findings (F2 TOCTOU acknowledged, F3 generic message, F4 symlink passthrough, F5 test-anchor nit) and all verification results (9/9 isolated tests, render check, `--no-path-resolution` load-bearing, fail-safe edge cases, no secret disclosure) stand unchanged.

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Creatidy/foundry!4

 @ -29,3 +29,3 @@
 . Set `FOUNDRY_ROUTER_HOST` to the certificate's validated DNS hostname. Inside the container this name is mapped to loopback solely for its HTTPS health probe. External clients still need valid DNS and trust.
 . The M0 verified TLS health probe uses platform trust roots. Do not assume arbitrary clients trust a private CA; that bootstrap is future work.
 . Run `make config` to verify the rendered Compose definition. `make up` is an **explicit deployment action** and requires Docker; do not run it against existing production environments without operator approval.
 . Run `make config` to verify the Compose definition. Use `make up` as the supported startup path: it checks Compose's interpolated TLS bind source is an existing absolute directory with readable `server.crt` and `server.key` before invoking Compose. Direct `docker compose up` bypasses this guard. `make up` is an **explicit deployment action** and requires Docker; do not run it against existing production environments without operator approval.

Rows
Columns