Skip to content

feat(isolation): add serviceGateways kit vocabulary - #416

Open
mogul wants to merge 2 commits into
mainfrom
feat/service-gateways-vocab
Open

mogul wants to merge 2 commits into
mainfrom
feat/service-gateways-vocab

Conversation

@mogul

@mogul mogul commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Add the patterns-side neutral hybrid/v1 serviceGateways kit vocabulary for kit-declared, acq-managed service gateways.

This PR includes:

  • ADR 0003 for the service-gateway vocabulary and tradeoffs.
  • Schema support for serviceGateways[], v1 kit-local Compose runtime files, required runtime.compose.service, sandbox/agent exposure through expose.env, and optional interface.port only when inferable from the named Compose service.
  • Validator checks for kit-local Compose files, safe named Compose service existence, service-scoped port inference, no privileged containers, floating-image warnings, and expose.env values mapped to url.
  • README authoring guidance and focused pytest coverage.

Tracking: GSA-TTS/agentic-coding-quickstart#472

ADR

The ADR is integrations/isolation/docs/decisions/0003-service-gateways-vocabulary.md.

It captures the core decisions:

  • Use the neutral field name serviceGateways.
  • v1 uses kit-local Compose files only.
  • v1 requires runtime.compose.service to identify the gateway service in multi-service Compose files.
  • Expose the resolved gateway URL into the sandbox/agent via expose.env.
  • Do not broaden sandbox egress or give API keys to the agent.
  • Keep backend routing details out of the kit schema.
  • Keep future backends such as Kubernetes possible without changing the semantic contract.

Verification

Commands run:

PATH="$PWD/.venv/bin:$PATH" make validate-kits

Result: passed. All 7 acq-kits valid.

uv run --link-mode=copy --extra dev pytest scripts/tests/test_validate_kits_service_gateways.py scripts/tests/test_validate_kits_ports.py scripts/tests/test_validate_kits_volumes.py scripts/tests/test_validate_kits_injection.py scripts/tests/test_validate_kits_network_tier.py -v

Result: passed. 94 tests passed.

uv run --link-mode=copy --extra dev ruff check integrations/isolation/acq-kits/validate-kits.py scripts/tests/test_validate_kits_service_gateways.py

Result: passed.

PATH="$PWD/.venv/bin:$PATH" make validate

Result: passed. Existing advisory warnings were reported for documented PII/CUI terms and network-tier deep-subdomain review signals.

PATH="$PWD/.venv/bin:$PATH" make generate-check

Result: passed. INDEX.yaml and CATALOG.md are up to date.

PATH="$PWD/.venv/bin:$PATH" make test

Result: passed. 422 tests passed.

git diff --check

Result: passed.

AI Assistance

This PR was prepared with AI assistance from OpenCode Agent. A human reviewer remains responsible for review and merge.

Capture the patterns-side decision for the neutral hybrid/v1 serviceGateways field and its v1 Compose runtime tradeoffs.

Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
@mogul
mogul requested a review from a team as a code owner September 15, 2026 04:36
Add the neutral hybrid/v1 serviceGateways schema and validator checks for kit-local Compose gateways, resolved URL exposure, privileged container rejection, and floating image tag warnings.

Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
@wz-gsa

wz-gsa commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head cfce305.

Reading this as a schema change rather than docs+tests: it widens what a kit is allowed to be, letting a kit start host-side Compose services. The ADR is careful and the test file is genuinely thorough (20 cases including degraded paths). Two gaps in the validator, both about the distance between what the ADR asserts in prose and what the code enforces.

1. privileged: true is the only escape primitive checked (integrations/isolation/acq-kits/validate-kits.py:206-211).

def _compose_has_privileged_service(compose_doc: object) -> bool:
    """True when any Compose service declares privileged: true."""
    return any(
        isinstance(service, dict) and service.get("privileged") is True
        for service in _compose_services(compose_doc).values()
    )

I grepped the whole validator for docker.sock, network_mode, pid:, cap_add, security_opt, devices, and build: — zero hits. So a gateway Compose file declaring volumes: ["/var/run/docker.sock:/var/run/docker.sock"], network_mode: host, pid: host, cap_add: [SYS_ADMIN], security_opt: ["apparmor:unconfined"], devices:, or build: passes with zero errors and zero warnings — each reaching privilege equal to or greater than privileged: true.

ADR-0003 states the boundary in prose ("a gateway that needs host-level privileges, broad mounts, or Docker socket access is outside this vocabulary"), while the code enforces one keyword. A validator returning errors == [] over an unmodeled Compose file is reporting measured-clean on something it did not look at. Either extend the check to the full escape set, or make an unmodeled top-level service key an explicit error so a reviewer sees "I did not model this" rather than silence.

2. include: / extends: defeat the kit-local path guarantee (validate-kits.py:342-355, schemas/kit-hybrid-v1.schema.json:247-253).

Both layers constrain only the entry files in runtime.compose.files[] — the schema regex requires a kit-local relative .ya?ml, and the validator does compose_path.relative_to(kit_dir.resolve()). But then it yaml.safe_loads each entry and inspects only services; grepping for include or extends in the validator returns zero hits. Compose's own include: and service.extends.file let a kit-local file pull in ../../../anything.yaml or a workspace path, and nothing resolves them.

So the schema's "may reference only Compose files shipped inside the kit" is true of the declared list and false of the resulting configuration. Reject include: and extends: {file: …} in gateway Compose files, or resolve and re-check them.

One thing I'd like an answer to rather than a change: the trust escalation is documented but not machine-readable. This vocabulary lets a fetched kit start host-side containers, and the control is a sentence — "Remote service-gateway kits must come from trusted kit sources" — with no schema field, no validator check, and nothing telling acq apart a local kit from one pulled via ACQ_EXTRA_KITS (a plain git URL + SHA). Either the trust gate should live somewhere a machine can read it, or the ADR should state plainly that v1 relies entirely on human review of the kit source.

Also noting, not as a finding: floating image tags are a warning, not an error (validate-kits.py:196-204), which the ADR states deliberately. It diverges from this repo's exact-pinning norm, and warnings tend to stay warnings.

Method: static analysis at head cfce305, compared against main's validator and schema. No kit was executed and no Compose file was run. I did not check these findings against a real docker compose config — they are gaps in what the validator inspects, not demonstrated exploits. bin/injection-scan.sh clean.

@wz-gsa

wz-gsa commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

AI-assisted adversarial review (nexus-agents pr_review, 5-role panel: architect/security/devex/catfish/scope_steward), advisory — needs a human to confirm before it drives a change. 5-0 request_changes, all voters completed, high confidence (0.78–0.85). This is a cross-cutting schema vocabulary every future kit author would build against, so the bar here is higher than a single kit — and all 5 voters independently converged on the same structural gap, plus one critical-severity finding.

1. No schema change in the diff, despite this being described as a schema-level vocabulary (all 5 voters, blocking). The diff touches only acq-kits/README.md and validate-kits.py. New code comments assert schema facts that don't exist after this change (_GATEWAY_PROTOCOLS "mirrors the schema enum"; the PR description says "authoring mistakes are caught by schema"). Either additionalProperties: false rejects every kit that declares serviceGateways (the vocabulary is inert), or the schema is permissive and none of the claimed structural guarantees actually exist — a reviewer can't tell which from this diff, and it's the exact question I asked the panel to check.

2. Critical: the stated security boundary is documented, not enforced (catfish, severity: critical; confirmed independently by all 5 voters with concrete fixtures). The ADR's premise is that the gateway — not the agent — is the credential/policy enforcement boundary, running outside the agent's reach. The only container-hardening check is privileged: true. Every voter independently constructed a Compose fixture that passes validation cleanly while granting host-equivalent control:

  • volumes: ["/var/run/docker.sock:/var/run/docker.sock"]
  • cap_add: [SYS_ADMIN]
  • network_mode: host / pid: host / userns_mode: host
  • security_opt: [apparmor:unconfined]

catfish: "checking privileged alone is worse than checking nothing: it signals enforcement that doesn't exist." For a vocabulary whose entire justification is being the enforcement point in an otherwise deny-by-default egress model (ADR 0002), this is the one thing that most needs to actually hold.

3. Path containment is bypassable via Compose's own include:/extends: (Security, catfish). The "kit-local-only, must stay under kit dir" guarantee is enforced only against the literal runtime.compose.files[] list. Compose natively resolves top-level include: and service-level extends: {file: ...} relative to the compose file — neither is inspected, so a compliant-looking kit can pull in arbitrary out-of-kit files (and the privileged/image scans never see them).

4. runtime.compose.files is never required to be non-empty (Architect, DevEx, catfish, Scope Steward). compose_files = compose.get("files") or [] with no emptiness check. A gateway declaring only name/interface/runtime.compose.service/expose.env — with zero Compose files — validates with zero errors, and because inspected_compose/found_named_service stay False, the service-existence check and the port-ambiguity check are both silently skipped too. The strictest-looking invariants are the easiest to bypass, by omission.

5. Host-published ports are accepted as valid endpoint sources (Security, catfish, Scope Steward). _compose_service_candidate_ports reads Compose's ports: (host-published) and expose: (container-internal) interchangeably. Per the PR's own framing, the gateway holds real credentials; a ports: ["8080:8080"] gateway is reachable from the LAN with no auth requirement anywhere in the schema, and the validator both allows this and derives the advertised "narrow, policy-controlled" endpoint from it.

6. ~140 lines of new security-relevant parsing ship with zero tests/fixtures (all 5 voters). No kit in the tree declares serviceGateways, so every new branch — path traversal rejection, port inference, the url-token check, floating-tag warning — is dead code in CI on merge day.

7. ADR numbering collision, confirmed by all 5 voters as requested. integrations/isolation/docs/decisions/0003-service-gateways-vocabulary.md collides with another unmerged PR's 0003-* ADR in this repo. Not a content defect, but whichever merges second needs a renumber — worth resolving before either lands so the history isn't ambiguous.

Minor/non-blocking, several voters: the closing paren in the expose.env invalid-name check is dedented to column 12 (legal Python via implicit line-continuation, but will trip black --check/ruff format); Scope Steward separately flagged the hand-rolled Compose port-parsing (_parse_compose_port, candidate-port union across expose/ports) as reaching past the existing-tool check — docker compose config --format json already resolves services/ports canonically, and it's arguably premature machinery given zero real consumer kits exist yet.

The vocabulary concept itself isn't in question (Scope Steward explicitly separated "is this schema idea worth having" — yes — from "is this specific implementation's enforcement real" — no). Given items 2–5 involve genuine container-escape surface in infrastructure every future kit would inherit, I'd treat this one as higher-priority to resolve than a typical implementation-detail review.

wz-gsa added a commit that referenced this pull request Sep 24, 2026
`integrations/isolation/docs/decisions/0003-` was claimed by two open PRs:
#416 (serviceGateways vocabulary, opened 2026-09-15) and this one (opened
2026-09-18). The filenames differ, so git merges both cleanly and leaves two
ADR-0003s in the same directory — every later "ADR 0003 (isolation)" reference
becomes ambiguous.

#416 claimed it first and its number is not cited anywhere else, so this PR
moves. 0004 is free: `main` has 0001 and 0002, and no other open PR claims it.

Only the file name and the H1 change. The "usai-provider ADR 0003" reference at
line 353 is a DIFFERENT, kit-local ADR under
acq-kits/usai-provider/docs/decisions/ and is deliberately left alone.

AI-assisted (OpenCode). Human review and merge still required.
wz-gsa added a commit that referenced this pull request Sep 24, 2026
#423's ADR moved from `0003-neutral-model-provider-discovery.md` to `0004-`
to clear a collision with #416, which claimed `integrations/isolation/docs/
decisions/0003-` first. Update the two comment references here so they do not
dangle once that lands.

Comment-only; no behavior change. `node --check` and a YAML parse both pass.

AI-assisted (OpenCode). Human review and merge still required.
@wz-gsa

wz-gsa commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Heads-up, no action needed from you: we've moved out of your way on the ADR number.

integrations/isolation/docs/decisions/0003- was claimed by two open PRs — this one (opened 2026-09-15) and my #423 (opened 2026-09-18). Different filenames, so git would have merged both cleanly and left two ADR-0003s in the same directory, with every later "ADR 0003 (isolation)" reference ambiguous.

You claimed it first and your number isn't cited anywhere outside this PR, so #423 is now ADR 0004 (dae9e3a), and I've followed the rename through #436's two comment references plus issues #424/#425/#435. serviceGateways keeps 0003 — nothing for you to change.

Worth noting there's a similar pair still open in quickstart: #507 and #504 both add an 0030-, and #500 already holds 0031 while #504's comment says #510 will take it. Both of those are yours to settle; I raised it on #507 with a suggestion that #504 keeps 0030 since its number is already cited cross-repo from this repo's ADR-0004.

My earlier review findings on this PR (the validator's privileged: true-only escape check, and include:/extends: not being resolved) are unchanged by any of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants