Repository navigation
Conversation
|
Converted to draft while I ensure this works for msb as well. |
f7938b9 to
89a09d3
Compare
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head The CF CLI install is done well: exact 1. That one entry covers every cloud.gov subdomain — control plane, UAA/login, logs, and every deployed tenant application route on a shared But the concrete surface is enumerable from the kit's own files:
That's ~5-6 exact hosts plus a logs endpoint, versus one wildcard. In a file whose entire job is the egress boundary for untrusted agent code, enumerating them is what least privilege means here. If the wildcard is genuinely required for app routes the operator deploys, it would help to say which and why in the spec comment, rather than leaving the breadth asserted. 2. The token guard is shape-based, so the fail-closed claim is narrower than the PR states ( The guard refuses Assert the positive instead: validate the value is the expected acq placeholder shape and refuse everything else. That inverts it from blocklisting one token shape to allowlisting the one you expect. Nit, unrelated to the kit: the Process question: @mogul's only comment here (2026-09-09) says "Converted to draft while I ensure this works for msb as well," but the PR currently reports as not-draft with no follow-up. Is the msb verification done, or did it come out of draft incidentally? Verified clean, recording affirmatively: no listening port, no server process, no UNVERIFIED: the actual shape of acq's injected Method: static analysis at head |
|
AI-assisted adversarial review (nexus-agents Critical: the auth mechanism has no working input (all 5 voters, independently derived)
The complementary path is also broken: if a non-JWT placeholder does reach So: real token → startup fails closed. Placeholder → CLI can't parse it. No value threads both. This is the kit's entire value proposition, and nothing in the default High:
|
|
Addressed the review feedback and pushed commit |
|
AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed fix commit 1. The critical defect (no value threads both the guard and real auth) is still open — the fix moved to a different placeholder shape, not a working one
I live-tested msb's actual placeholder shape rather than assuming it: created a real sandbox with msb's real placeholder is the literal, unexpanded string
So on msb — the kit's own declared backend — the script exits 1 during 2. The wildcard narrowing (
|
|
Re-review requested for the cloud.gov acq kit updates. Summary:
Verification:
Blocked locally:
AI assistance: OpenCode Agent assisted with this update; human review required before merge. |
Add an sbx cloud.gov acq mixin kit that installs the pinned Cloud Foundry CLI, allow-lists cloud.gov and Cloud Foundry docs egress, and configures CF auth through acq secret placeholders. Tracks deferred msb wildcard parity in #407. Co-authored-by: OpenCode Agent <agent@gsa.gov>
Update the cloud.gov kit registry and docs after verifying that the quickstart msb wildcard fix translates per-kit **.cloud.gov to allow@*.cloud.gov. This depends on GSA-TTS/agentic-coding-quickstart#451 for msb runtime support. Co-authored-by: OpenCode Agent <agent@gsa.gov>
Tighten CF token placeholder handling, use an msb-compatible cloud.gov wildcard, and update verification/docs to reflect the supported auth path. Co-authored-by: OpenCode Agent <agent@gsa.gov>
Accept synthetic msb-style backend placeholders while rejecting raw bearer/JWT-like token material before writing CF CLI auth state. Replace wildcard cloud.gov egress with exact control-plane and documentation hosts, and extend offline verification for placeholder and allow-list assumptions. Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
c2b09db to
d16c1a8
Compare
|
AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head Two of three prior findings are resolved. The critical one is still open, and I can now show it end-to-end on current software. Resolved — the egress findings (my items 2 and 3 from the 2026-09-30 round)The wildcard is gone, replaced with exact hosts: # Exact cloud.gov control-plane and documentation hosts. Avoid wildcard
# semantics so sbx and msb translate the same least-privilege egress shape.
- api.fr.cloud.gov:443
- uaa.fr.cloud.gov:443
- login.fr.cloud.gov:443
- doppler.fr.cloud.gov:443
- log-cache.fr.cloud.gov:443
- cloud.gov:443
- www.cloud.gov:443
- docs.cloud.gov:443
…13 hosts, zero wildcardsThis resolves both prior findings at once rather than trading one for the other, which is what the single-character
The README's Backend Parity section also no longer carries the claim I measured as false ("sbx accepts it as a cloud.gov subdomain wildcard … including API, login/UAA"). The replacement text is accurate: "Both sbx and msb receive the same exact host list." One limit I want to be straight about: I could not re-verify the sbx side this round. Still open (critical) — the auth guard rejects the placeholder msb actually injectsThe guard now accepts two more shapes: acq_placeholder_?*|sbx-cs-?*|MSB_PLACEHOLDER?*|\<MSB_PLACEHOLDER*\>)msb does not emit either of them. Measured on msb 0.7.7 (my prior round was 0.7.4, so I re-measured rather than citing it): Unchanged from 0.7.4, and still what quickstart documents — Then I ran this PR's own
For completeness, the full pattern set against the real value and the shapes that do match: The pattern across three roundsEach round has fixed the specific string and left the method unchanged — the guard is written against an assumed placeholder shape and not checked against what the backend produces. That is now three different never-matching patterns ( Two ways out, and I'd suggest the second:
Either way the fix wants a test that asserts the guard against a measured placeholder rather than a literal in the test file, or round four is likely. Verification method, and one trap worth knowingLive msb sandbox ( A trap I hit: my first Sandbox removed after the measurement. NetThe egress work is done and done correctly — the enumerated-host approach is what both prior rounds were asking for, and the README is accurate now. The auth guard is still non-functional on msb, and it is the kit's central mechanism. I'd recommend Happy to verify a next pass the same way — live sandbox, in-guest, both backends once |
basiliskus
left a comment
There was a problem hiding this comment.
Verdict: request changes — 48b71a03582c43f3d0884b6b0764bf9810983a13
The prior auth-placeholder finding remains open. Current quickstart documents and implements the msb guest placeholder as literal $MSB_<env>, so this kit receives $MSB_CF_OAUTH_TOKEN. The startup guard at integrations/isolation/acq-kits/cloud-gov/files/home/cloud-gov-configure-cf-auth.sh:27 accepts MSB_PLACEHOLDER... forms instead, and the real msb shape falls through to the refusal at line 35. I reproduced that exact value against this script: it printed "not a recognized backend placeholder" and exited 1. Because this is a startup command and the kit declares msb support, a correctly provisioned msb sandbox cannot start with the documented secret binding.
Please accept the actual $MSB_... placeholder shape and cover $MSB_CF_OAUTH_TOKEN in the offline guard test, or move placeholder recognition to an authoritative backend-aware interface.
Minor: integrations/isolation/acq-kits/README.md:23 says the exact-host kit allows app-route domains, but the kit deliberately excludes app routes. Please make the catalog description match the implemented boundary.
Assisted-by: OpenCode [usai/gpt-5.6-sol-latest-guardrails-defaultv2] skill:change-review
Accept the documented msb guest-secret placeholder in the cloud.gov auth guard and cover it in offline verification. Align kit documentation with the implemented domain boundary. Co-authored-by: OpenCode Agent <user@gsa.gov>
|
AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head The blocker is fixed. I can show it on the real value this time. Resolved — the guard now accepts what msb actually injects
Then I ran this PR's actual auth script, unmodified, inside that guest against that real value: Startup no longer aborts on msb. The negative half still holds, measured in the same guest: Removing the two tests that asserted Offline verifier, in a network-off container at this head:
Nit (non-blocking): the allow-list arm admits shell metacharacters
I chased this to a conclusion rather than flagging it on appearance, and it is not exploitable as written. The value is only ever compared in env CF_OAUTH_TOKEN= cf api https://api.fr.cloud.govI grepped the whole kit; no other code path consumes the variable. So this is defense-in-depth, not a defect. If you want it tighter, Nit: the new test asserts a shape msb does not emitMSB_PLACEHOLDER='$'MSB_SECRET_PLACEHOLDERmsb emits This is the same method that produced the first three rounds: the guard's positive arm is written against an assumed shape and the test encodes that assumption as a literal. It now matches reality, so it is no longer a blocker — but the structural point from my last round stands, and if a backend ever changes its placeholder format this breaks again silently. UNVERIFIED
Sandbox removed after measurement ( NetThe finding I raised three times is genuinely closed, verified against the real injected value rather than a literal. The egress work was already correct. All 7 checks pass at this head and the PR is mergeable. No blocking findings remain from me. The two nits above are optional and neither needs a new round. |
basiliskus
left a comment
There was a problem hiding this comment.
Verdict: approve — e2a3091b081eff1609f803f36d0bcb9b29611870
The prior startup blocker is resolved at integrations/isolation/acq-kits/cloud-gov/files/home/cloud-gov-configure-cf-auth.sh:28: the quoted case pattern matches the documented literal $MSB_<env> prefix. The replacement test at integrations/isolation/acq-kits/cloud-gov/scripts/verify:85 now exercises that real shape, while the bearer and JWT-like negative cases remain. Although the suffix accepts shell metacharacters, I found no evaluation or config-write path: the startup script only validates the value, and the documented/verification consumers pass its expansion inside a quoted HTTP-header argument.
The prior catalog issue is also resolved at integrations/isolation/acq-kits/README.md:23, which no longer advertises app-route access.
Assisted-by: OpenCode [usai/gpt-5.6-sol-latest-guardrails-defaultv2] skill:change-review
|
AI-assisted follow-up (OpenCode), advisory. New Blocker found after my previous comment cleared the auth guard. I had marked the sbx half UNVERIFIED for host reasons; that is now fixed on my side, and running the real path immediately surfaced something diff inspection did not.
MeasuredRead the ordering in that log: the zscaler cert is staged at create time, the cloud-gov install runs and fails, and the zscaler step that actually installs the CA into the trust store is only registered as a startup command — it has not run yet. CauseThe two kits disagree about phase:
url="https://github.com/cloudfoundry/cli/releases/download/v${CF_CLI_VERSION}/${asset}"
curl -fsSL "$url" -o "$work/cf-cli.tgz"On an intercepted network that chain terminates at the corporate root, which is not in the container trust store until the zscaler kit's This is the same trap the zscaler kit documents for itself, in its own README §"Why initFiles + startup, not files + install":
That kit solved its own ordering problem and recorded the reasoning in an ADR. cloud-gov then put a TLS fetch in the phase that runs first. ScopeI checked every kit with an cloud-gov is the only one. That is also why this has not been hit before. Why my offline verification missed it
Suggested fixes
I'd suggest 1. Option 2 trades a hard failure for a silent capability gap, which is the degraded-path shape worth avoiding. Unaffected by thisThe auth-guard fix I verified in my previous comment still stands — that was measured in an msb guest with no TLS fetch involved. The egress host list is also fine. This is specifically a create-time ordering defect on intercepted networks. Note on my own environment, for reproducibilityMy host also hit a Go 1.27 CA issue that is not related to this PR: golang/go#77865 made Sandboxes created for this measurement were removed ( |
Summary
Adds a new cloud.gov acq mixin kit for working with Cloud Foundry on cloud.gov.
The kit:
cf oauth-token, without storing the real token in the kit or workspace;Context
This supports cloud.gov application work from isolated acq sandboxes while keeping Cloud Foundry authorization in acq's secret mechanism.
msbsupport is now unblocked by merged quickstart PRGSA-TTS/agentic-coding-quickstart#451, which translates per-kit**.cloud.govinto msb's suffix rule form. I verified the cloud-gov kit spec against quickstartorigin/main:**.cloud.gov:443emitsallow@*.cloud.govfor msb.Verification
PATH="$PWD/.venv/bin:$PATH" make validate- passedPATH="$PWD/.venv/bin:$PATH" make validate-kits- passedPATH="$PWD/.venv/bin:$PATH" integrations/isolation/acq-kits/cloud-gov/scripts/verify- passed offline checks: 4 passed, 0 failed; live sandbox check skipped becauseRUN_ACQ=1was not setpython3 scripts/scan_unsafe_shell.py integrations/isolation/acq-kits/cloud-gov- passed, 0 errors / 0 warningsbash -non all new shell scripts - passednpx --prefix .github/linters markdownlint-cli2 "**/*.md" "#node_modules" "#.github/linters/node_modules" "#templates" "#CHANGELOG.md"- passedorigin/mainemittedallow@*.cloud.govand notallow@**.cloud.govSecurity Impact
cloud.gov,**.cloud.gov,docs.cloudfoundry.org,cli.cloudfoundry.org,github.com, andobjects.githubusercontent.comfor this kit.**.cloud.govis intentionally broad because the requested working surface is all cloud.gov subdomains; custom domains outside cloud.gov remain project-specific.Rollback
Revert this PR to remove the cloud-gov kit, registry entry, and Markdown Lint toolchain update.
AI Assistance
This PR was generated with assistance from OpenCode, an AI coding agent. Human review is required before merge.