Repository navigation
feat(containment): allowlist compose keys; namespace all mounts; stage baked files - #142
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ea71b68dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if os.getenv("GREFFON_CATALOG_OPTIONAL") == "1": | ||
| pytest.fail( | ||
| "catalog not found and GREFFON_CATALOG_OPTIONAL=1 " | ||
| "— gate explicitly skipped" | ||
| ) |
There was a problem hiding this comment.
On every clean GitHub Actions run, the workflows check out only this repository, so _find_catalog_root() cannot find a sibling greffon-catalog checkout and enters this branch. Both the default path and the documented GREFFER_CATALOG_OPTIONAL=1 opt-out call pytest.fail, making the entire application suite—and therefore the release job—fail unconditionally unless another checkout step is added or the opt-out actually skips the test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ea71b6→03f7430 (before this comment was read — an independent adversarial review round found the same defect as its P1-2). The resolution is the dual one you described: ci.yml now checks out the pinned catalog (greffon-catalog@5652a75) as a sibling, and the opt-out is both corrected (GREFFER_→GREFFON_CATALOG_OPTIONAL) and made to actually skip. The default remains fail-never-skip per the render-oracle design rule — a gate that can silently cover nothing is not a gate — and the round-2 verifier executed all four env/checkout combinations.
| for name, service_def in services.items(): | ||
| for key in service_def: |
There was a problem hiding this comment.
When parsed YAML contains a non-iterable service definition such as services: {app: null} or app: 1, this loop raises TypeError before the existing service-definition shape check runs. The start route catches only ComposeShapeError, so input that previously produced the intended diagnostic 422 now becomes an opaque 500; perform the mapping check before iterating service_def.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 03f7430 — an independent review round caught this as P1 ("iterating one here would turn that into an uncaught TypeError — the bare-500 class this epic kills") before codex posted. The guard skips non-mapping service defs in _validate_keys, leaving the named refusal to the shape pass; the verifier executed all three shapes (int/None/str) end-to-end: clean ComposeShapeError 422 naming the service, identical on both paths.
| "image", "environment", "command", "entrypoint", "depends_on", | ||
| "healthcheck", "labels", "restart", "user", "working_dir", "tmpfs", | ||
| "stop_grace_period", "expose", "init", "platform", "shm_size", | ||
| "ulimits", | ||
| } |
There was a problem hiding this comment.
For any catalog service that specifies logging, the new catalog allowlist rejects the compose before deployment. This conflicts with compose.py's _inject_instance_log_rotation, which explicitly preserves an existing catalog-authored logging block instead of injecting the default, so a previously supported configuration now returns 422; include logging in the catalog allowlist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Legitimate catch, folded at 3cc3362: logging is now on the BASE set (both trust scopes) — _inject_instance_log_rotation's documented "author's explicit choice wins" contract was unreachable behind the refusal, exactly as you describe. Added a test that asserts the key passes both sets AND that the injection preserves an author-declared syslog block rather than overwriting it. Note the live catalog uses no logging key today (the gate scan confirms), so nothing shipped was broken — this was a contract-reachability defect, not an outage.
3c8f8cf to
3cc3362
Compare
e12c34f to
1365325
Compare
3cc3362 to
05426e1
Compare
1365325 to
2418b37
Compare
05426e1 to
85b6bec
Compare
…e baked files
compose-containment Feature 3, three changes that share one theme —
stop trusting the shape of what arrives. Stacks on feat/compose-shape
(Feature 2's normalizer and error channel).
Key allowlist (trust-scoped, because a flat list breaks the catalog):
a positive allowlist, not a denylist — devices/volumes_from/build/ipc/
userns_mode and whatever a future spec adds are refused by not being on
it. Catalog composes (PR-reviewed) keep the broad set including
cap_add/cap_drop/devices/sysctls (wireguard's exact surface, asserted);
custom composes (strict=True, selected by the start path when the
manager sends an inline compose — wired now, activates with the
foundation's transport) get the base set. env_file is refused in BOTH
sets and is NEVER grantable via the operator override (it re-opens the
cross-instance baked-secret read, a hole that harms tenants, not the
machine owner who would grant it): compose resolves env_file
client-side relative to the project dir, so
env_file: ../<other-instance>/realm.json reads another instance's
secrets — the bind-mount reach by another door. Operator grant:
GREFFER_COMPOSE_EXTRA_ALLOWED_KEYS. Unknown keys refuse with the
services.<name>.<key> path; a non-mapping service definition is left to
the shape pass's named refusal (iterating it here was a bare-500
regression the review caught).
Mount namespacing: an UNDECLARED named source is now prefixed with the
instance id exactly like a top-level-declared one. The legacy passthrough
let two instances mounting 'dd:/data' share one host volume — the
cross-instance leak class the _0001 ops-migration was written to repair.
MIGRATION HAZARD, named honestly: an instance deployed BEFORE this
change with an undeclared source has its data in the verbatim docker
volume ('dd'); its next start targets the namespaced one ('i1_dd') and
the old volume is orphaned, not migrated. No catalog entry is exposed
(all 31 declare every source — verified by the gate), custom composes
do not exist yet, and the affected population is out-of-catalog
repository_url users; if that population turns out to be real, the
_0001-style advisory ops-migration is the repair shape.
Baked-file staging: file/json destinations resolve inside a dedicated
<instance>/config/ subdirectory with bare-filename + control-char
validation, killing the ../ traversal write AND the control-file
collision class (l4_ports.json, .backup_inprogress, the rendered
compose) by construction. Validation runs BEFORE the file branch's
remove_compose_file side effect (a refused destination must not delete
the previous rendered compose). The destination volume must be declared
by the instance's compose and must never be greffon_nginx (its files
list carries the instance certificate; a baked file overwriting pem.crt
is a silent TLS MITM). Refusals raise ConfigRenderError (clean 422).
Catalog gate (the HLD's merge condition, in this PR as demanded): every
pinned catalog entry must pass the catalog allowlist — CI checks out
greffon-catalog@5652a75 as a sibling; the gate fails, never silently
skips, when the checkout is absent (GREFFON_CATALOG_OPTIONAL=1 the
deliberate visible opt-out, the render oracle's convention). 23 new
tests; suite 988.
Independent adversarial review (DO NOT SHIP): the non-mapping-service
500 regression, the gate that could not pass this repo's own CI (no
catalog checkout; a typo'd non-functional opt-out), the NUL-byte 500,
the env_file grant hole, and the side-effect ordering — all fixed here;
the migration hazard and stale-root-file hygiene (an advisory purge
migration's shape) named above.
Codex CLI review round 2 (bundled 0.162), two P1s folded:
- The catalog gate only ancestor-searched; CI checks the catalog out to
.greffon-catalog and exports GREFFON_CATALOG_DIR, which the search
cannot find (dot-directory, different name) — the gate would fail
every CI run even with the checkout present. The helper now reads
GREFFON_CATALOG_DIR first (the render oracle's convention). This edit
had been lost to a partial-script failure during the rebase conflict
resolution; codex caught what the local ancestor search masked.
- 0003_namespace_undeclared_volume_mounts: 0001 is ledger-recorded and
never re-runs, so instances whose composes were rendered AFTER 0001
ran still hold undeclared mount sources in raw-name docker volumes —
the Feature-3 render change would mount them EMPTY on next deploy
(apparent data loss). 0003 copies raw -> <id>_raw non-destructively
(target-exists skip = idempotent; 0001-already-copied lands in the
same skip), 7 tests incl. registry ordering.
85b6bec to
8c4f5de
Compare
compose-containment Feature 3 (stacks on #141 /
feat/compose-shape— retargets to main when that merges): three changes that share one theme — stop trusting the shape of what arrives.Key allowlist — trust-scoped. A positive allowlist, not a denylist (
devices,volumes_from,build,ipc,userns_modeand future-spec keys are refused by not being on it). Catalog composes keep the broad set includingcap_add/cap_drop/devices/sysctls(wireguard's exact surface, asserted — a flat list would take a shipped app offline); custom composes get the strict set, selected by the start path when the manager sends an inline compose (wired now, activates with the foundation's transport).env_filerefused in both sets (client-side path traversal — the bind-mount reach by another door). Operator grant:GREFFER_COMPOSE_EXTRA_ALLOWED_KEYS. Unknown keys 422 with theservices.<name>.<key>path.Mount namespacing. An undeclared named source now gets the instance-id prefix exactly like a declared one — the legacy passthrough let two instances mounting
dd:/datashare one host volume (the leak class the_0001ops-migration repaired). The catalog declares every source (verified), so no catalog render changes; the rebuilt volumes block carries the namespaced name (asserted).Baked-file staging.
file/jsondestinations resolve inside a dedicated<instance>/config/subdirectory with bare-filename validation — killing the../traversal write and the control-file collision class (l4_ports.json,.backup_inprogress, the rendered compose) by construction. Destination volume must be declared by the instance and must never begreffon_nginx(its files list carries the instance certificate; a baked file overwritingpem.crtis a silent TLS MITM). Refusals areConfigRenderError→ 422.Catalog gate (the HLD's merge condition, in this PR as demanded): every pinned catalog entry must pass the catalog allowlist — fails, never skips, when the checkout is absent (
GREFFER_CATALOG_OPTIONAL=1the deliberate opt-out).20 new tests; suite 985 passed, 0 skipped.