feat(azure): refuse expired provider credentials before any cloud spend - #245
Conversation
Nothing in firstmate refreshes a provider token and nothing checked one before staging it, so the crosscheck lane provisioned a real VM per review and died inside the guest with an unrefreshable session. Add bin/fm-credential-expiry.py as the single owner of the expiry question for one account profile: it classifies usable, refreshable, expired, or unusable from the real codex/pi/claude credential shapes and never emits token material. Azure Crosscheck runs it against the selected reviewer before the lane wait, before any Azure call, and before any staged object, so an expired reviewer is skipped rather than provisioned; refreshable is refused too, because the compartment egress allowlist has no auth host to refresh against. Add fm-azure-validation.sh auth-seed so an operator can publish a re-authenticated credential onto fm-auth-home instead of hand-running az. It plans locally, refuses anything not usable, and uploads into the one home-shaped layout the guest actually reads. Give the guest a durable auth write-back signal: a pull records that a write-back is owed, a failed push records the failure, a successful push clears both, and the surviving marker reaches the cell report the same way auth-needed does.
…y in front of it An adversarial review of this branch found the preflight could not do the one thing it exists to do. The check ran before `acquire_review_lane`, which is a blocking FIFO wait bounded by FM_AZURE_CROSSCHECK_QUEUE_WAIT_SECONDS: 7200 by default, 86400 at its maximum. The margin covers the review, not the queue in front of it. So a credential with 1800 seconds of life was admitted, the caller blocked for up to two hours, and the VM was provisioned with a token that had died ninety minutes earlier. That is exactly the outcome the preflight was written to prevent, and it fires when all lanes are busy, which is when spend is highest. The comment and the docs both described the pre-lane position as the feature; it was the bug. The check now runs twice: once to fail fast at no cost, and once with the lane held, which is the call every billable action sits behind. It costs one local file read. The reason it shipped is that the margin had no test. Every "stale" fixture was already expired when it was built, so the margin never participated in an assertion; zeroing it or deleting the argument left the suite green. There is now a fixture with 1200 seconds of life, which sits between the module default margin (900) and the caller's review margin (1800), so only the caller's own margin refuses it. Three more things this branch asserted but could not prove: - The two assertions carrying "refused before any lane or Azure call" could not go red. `runtime_config` raised on an absent Azure environment before either could be reached, so they held identically with the whole preflight deleted. The unit now has a complete Azure scope and a positive control proving a usable credential does reach a lane or the CLI. - The second check is proved by expiring the credential during the lane wait, so a pre-lane check alone cannot satisfy it. - The new guest markers were fatal. The guest runs under `set -euo pipefail`, and both markers were bare redirects, so an unwritable or full state directory turned a note to the operator into a run-killer: the pull marker aborted before the run started, and the push marker aborted a completed, already-paid-for run before it packaged its result or echoed its completion marker. The old code wrote to stderr, which cannot fail. Marker writes are best-effort again, and the test driver runs the guest's own shell options instead of a weaker set that could not observe this. Also corrected, all found by the same review: - The owed marker claimed "auth pulled from share" on the two branches where nothing was pulled. It now names the actual origin, and the shim can fail a pull so that branch is reachable at all. - `check --min-state unusable` was a gate that could never refuse. Removed from the choices. - docs/azure-validation.md said seeding stops cells booting with a dead token. `dispatch_cell` does not re-check the share, so the doc now says what the code does and names the gap. - The "reports the skipped write-back on its next attempt" claim held only while the share was still stale. Stated that way. Mutations, each confirmed to change the file before running: margin zeroed, margin argument deleted, second check deleted, both checks deleted, push marker made fatal, pull marker made fatal, owed-origin reverted -- all red. One mutation did not apply and was caught by its own guard rather than reported as a pass.
|
Adversarial review of this branch returned DO NOT MERGE with two blockers. Both are fixed in Blocker 1 - the preflight could not do the thing it exists to do. It ran before The check now runs twice: once to fail fast at no cost, once with the lane held. The second one is what every billable action sits behind. Blocker 2 - the margin had no test, which is why Blocker 1 shipped. The PR body claimed mutation M5 (margin dropped) was red. It was green, in both forms. Every "stale" fixture was already expired when built, so the margin never participated in any assertion. There is now a fixture with 1200s of life, which sits between the module default margin (900) and the caller's review margin (1800), so only the caller's own margin refuses it. M5 and M5b are now red. Also fixed:
Mutations, each confirmed to change the file before the run: margin zeroed, margin argument deleted, second check deleted, both checks deleted, push marker made fatal, pull marker made fatal, owed-origin reverted. All red. One mutation failed to apply and was caught by its own guard instead of being reported as a pass. Not fixed here, deliberately: gating |
…sioning in the margin A second adversarial review of the fix round found three blockers. The first fix wrapped the three marker WRITES so a failed write could not abort the guest, and left the two marker REMOVALS unwrapped 28 lines away. Under the guest's own set -euo pipefail a failing `rm -f` aborts exactly as a failing printf did, so the run-killer survived in two places: a stale auth-needed marker plus an unwritable state directory kills the guest before the run starts, and a SUCCESSFUL push kills it while clearing its own markers, after the run is finished and paid for. The new test could not see either, because both unwritable-directory cases cleared the state directory immediately before making it read-only, so `rm -f` never had a file to fail on. Both cleanups are best-effort now, and the cases seed a marker before revoking write. The lane assertion added to prove the new refusal path could not detect a lane leak. Occupancy is an flock on a lock file that is created once and never unlinked, so a held lane and a released one produce identical directory listings; deleting the release left the suite green. It is a real occupancy probe now - a non-blocking flock from a separate process - with a self-check that fails if the probe cannot observe a lane it holds itself. Deleting the release call still passes, and that is correct rather than a gap: CPython closes the handle as the frame unwinds, so there is no leak to see. A mutation that keeps a live reference, which is a real leak, is red. The margin covered the review and nothing in front of it. Scope verification, VM create, boot, and bundle upload all happen after the lane is granted, and poll_model_run already budgets 900 seconds for exactly that interval. A credential with 1801 seconds of life cleared the check and then died before the reviewer's first token. The margin now includes that allowance, reading the same constant as the poll deadline so the two cannot drift, and a fixture that outlives the review but not the provisioning is refused. Also from the same review: the `az` shim was never invoked, because verify_scope_and_foundation raises inside the lane before any CLI call, so the "no Azure call" assertion could not fail and the comment claiming a complete Azure scope was wrong. The shim, the assertion, and the claim are gone; the lane assertion is the one that is armed, and its positive control now proves it. The empty-share marker was the one write with no unwritable-directory case. `check --min-state unusable` had no test. And a credential alive now but dying inside the margin was reported as "expired at <a future instant>", contradicting `report`, which calls the same profile usable. Mutations, each confirmed to change the file first, all red: margin zeroed, margin argument deleted, second check deleted, both checks deleted, each of the three marker writes made fatal, each of the two marker cleanups made fatal, the owed-origin branching reverted, and a genuine lane leak.
|
Second adversarial review returned DO NOT MERGE with three blockers. Fixed in 1. The run-killer survived 28 lines from the ones I fixed. The first round wrapped the three marker writes and left the two marker removals unwrapped. Under the guest's own My test could not see either, because both unwritable-directory cases cleared the state directory immediately before making it read-only, so 2. The lane assertion could not detect a lane leak. Occupancy is an Worth stating precisely: deleting 3. The margin covered the review and nothing in front of it. Scope verification, VM create, boot, and bundle upload all happen after the lane is granted, and Also fixed: the Process note: the green CI on this PR was against Deferred, written down rather than implied covered: gating |
Problem
Nothing in firstmate refreshes a provider token, and nothing checked one before staging it onto a cloud compartment. A live audit today found the pi pool fully expired, five of eight codex profiles expired, and all three claude profiles unusable, while the crosscheck lane kept provisioning a real VM per review and dying inside the guest with an unrefreshable OAuth session.
What this adds
A reusable expiry preflight.
bin/fm-credential-expiry.pyis the single owner of "is this account profile's credential usable, and until when". It reads the real provider shapes (codexauth.jsonJWTexp, piopenai-codex.expiresin milliseconds, claudeclaudeAiOauth.expiresAtplusrefreshTokenExpiresAt) and classifiesusable,refreshable,expired, orunusable. It never logs in, never refreshes, never mutates a profile, and never emits token material: expiry instants, harness, credential file name, and profile path only.reportwalks the Agent Fleet pool;checkgates one profile.refreshableis deliberately distinct fromusable. Nothing on the host turns one into the other.Wired into Azure Crosscheck.
run_azure_reviewnow runs the preflight as its first statement, before the FIFO lane wait, before any Azure call, and before any staged object. The reviewer credential must beusableand must outlive the review deadline.refreshableis refused there for a code-grounded reason: the model compartment's egress allowlist is Azure DNS plus exactly one provider API host (docs/azure-crosscheck/network-policy.json), and no auth host is on it, so a CLI inside the compartment can never refresh. The refusal is an ordinaryCrosscheckToolError, so the roster records the account and rotates to the next reviewer instead of ending the review.Host-side seeding.
bin/fm-azure-validation.sh auth-seedpublishes a re-authenticated credential ontofm-auth-homeinstead of a hand-runaz storage file upload. It plans locally with no Azure call, refuses anything notusable, and on--applyrequires--confirm-seedplus the exact--confirm-subscription, then re-reads the share to prove the exact byte count landed.The share layout was verified against both consumers rather than invented. The guest's
auth_home_pullcopies the whole share into one cell home and exportsCODEX_HOME=$HOME/.codexorCLAUDE_CONFIG_DIR=$HOME/.claude; crosscheck reviewers never read the share at all. A read-onlyazlisting confirms the live share holds exactly.codex/auth.json,.codex/config.toml, and.claude/.credentials.json. There is no consumer for a multi-profile layout, so none is created, and only the credential file is uploaded.Durable auth write-back signal.
auth_home_pushfailure was a stderr warning that died with the guest. A pull now recordsauth-push-owedon the worktree disk, a failed push recordsauth-push-failed, a successful push clears both, and the surviving marker reaches the cell report the same wayauth-neededalready does.Verification
tests/fm-credential-expiry.test.sh(new, registered hermetic intests/test-capabilities.tsvandtests/behavior-test-durations.tsv), 5 cases, exit 0.bash bin/fm-behavior-shards.sh --plan 8covers 113 tests including the new one, exit 0.bash bin/fm-lint.shon the full set, exit 0.tests/fm-crosscheck-azure.test.shandtests/fm-azure-validation.test.shstill pass, exit 0.azcalls made were read-only listings of the existing share.The guest write-back test executes the real
auth_home_pullandauth_home_pushfunctions extracted from the guest, with the auth-sync helper PATH-shimmed, rather than grepping the script.Mutation results
Each mutation was applied to the production call site, confirmed present by grep, then the suite was judged by exit code alone.
unusableinstead ofusableruntime_configcodex/instead of.codex/--applyproceeds without--confirm-seedM4 and M6 initially survived and are recorded here only in their strengthened form. M4's first fixture set could not discriminate a zeroed stamp, so two claude fixtures were added. M6's first attempt was a no-op mutation whose assignment was overwritten by every later branch, so it was reapplied at the branch that actually returns.
Findings worth flagging
inspect_reviewer_credentialraisesAzureCrosscheckError, which is a plainRuntimeErrorand is not caught by the reviewer rotation loop inbin/fm-crosscheck.py. A credential-shape failure there ends the whole review rather than rotating. The new preflight raisesCrosscheckToolErrorso it rotates correctly, but the older path is unchanged and is worth a separate look.refreshTokenExpiresAt. They classifyunusable, which matches the existing core, but they will silently never work until an interactive login.