Skip to content

fix(run): mask the container's own AWS credential state - #56

Merged
stefanwb merged 3 commits into
mainfrom
fix/mask-aws-container-state
Sep 10, 2026
Merged

fix(run): mask the container's own AWS credential state#56
stefanwb merged 3 commits into
mainfrom
fix/mask-aws-container-state

Conversation

@stefanwb

@stefanwb stefanwb commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Closes a gap where two external-cli-tools scenarios were false as shipped.

The asymmetry

run.sh already treats the AWS CLI credential cache as too sensitive to bring
in from the host. The scoped --aws mount excludes ~/.aws/credentials and
~/.aws/cli/cache (run.sh:394-395), and the spec restates it as an absolute:
"~/.aws/credentials and ~/.aws/cli/cache/ SHALL NEVER be mounted, even under
--aws."

The container's own /root/.aws then got none of that care. It sits on
claude-code-root — read-write, shared by every session — with no tmpfs mask.
grep -n -- '--tmpfs' run.sh returned three paths and AWS was not among them.
So the STS cache an --aws session wrote persisted after that session exited
and was readable by a later session that opted into nothing.

Two scenarios were consequently false:

Spec Asserted Actual
:53 with no flags, /root/.aws/ "does not exist" exists, and populated after any --aws session
:66 under --aws, writes to /root/.aws/ "fail with EROFS" the directory is writable

The cause is structural. Credentials opt-in states the desired end state
for /root/.aws; masking is actually specified by In-container gh login persists only under --gh, which was written for the three CLIs that have an
in-container login flow, so AWS was never added to it. The spec asked for an
outcome that no requirement provided a mechanism for. The EROFS claim looks
carried over from the --glab scenario, where the whole config directory is
bind-mounted :ro so the claim holds by construction; under --aws only
config (a file) and sso/ are mounted, leaving the directory itself on the
read-write volume.

Not overstated: that cache only ever holds short-lived SSO/STS-derived
material — the long-lived credentials file was never in the container — so
anything in it expires shortly after the session that wrote it ends. This is a
break in the opt-in boundary, not a live credential leak. It earns a fix
because the mask is two lines and the spec already required the behaviour.

The fix

AWS is masked in both directions; only the scope changes. Without --aws, the
whole directory. With it, just cli/cache, so the :ro host mounts at
/root/.aws/config and /root/.aws/sso stay visible to the session that asked
for them. Unlike gh, there is no in-container login to preserve, so there is
no unmask state.

"does not exist" becomes "is empty": a tmpfs makes a path exist-but-empty,
which is already how the glab-cli and terraform.d lines in that same
scenario read, and literal non-existence is not something a mount can express —
the original wording was never satisfiable by the mechanism the rest of the
scenario relies on.

Why the test reads run.sh instead of asserting in a container

This is the part worth reviewing. smoke/smoke.sh does not invoke run.sh
— it re-implements the mask list, and its own comment says "mirrors run.sh". A
mask missing from run.sh therefore could not fail the smoke suite, because the
harness would simply not add it either. The suite tests the mirror.

Separately, assert-in-container.sh does check that a non-granted opt-in's
config path is absent or empty, but its AWS entry points at /root/.aws/config
— a file absent without --aws whether or not a mask exists — so that assertion
passed vacuously and the credential cache beside it went unexamined.

Both halves worked as written; neither could observe this. So tests/test_masks.py
(the first test of any kind over the mask set) asserts against run.sh itself,
and additionally that the mirror still matches it, so the two cannot drift apart
silently again. The mask set is checked by equality, so a mask added without
a spec delta fails here rather than shipping unreviewed.

I verified the new test fails against the unfixed run.sh — a mask test
that passes before the fix tests nothing. Two of the six fail on HEAD:run.sh;
the other four pass.

One risk worth a reviewer's eye

The --aws cache mask is the only mask that must stay writable — the CLI
writes derived STS into it, and entrypoint.sh's chown walk uses find -xdev,
so it never descends into a tmpfs and cannot fix ownership there. That should
hold on Docker's default --tmpfs mode, but I could not verify it here without
a daemon, so assert-in-container.sh now asserts the write succeeds rather than
assuming it. CI's smoke cell going green is the confirmation.

Out of scope (named in the proposal, not silently skipped)

The host ~/.aws/sso bind-mount is conditional on that directory existing
(run.sh:399), so where it does not, an in-container aws sso login still
caches on the volume. That state is masked from every non---aws session either
way, so the boundary this PR is about holds; what remains is persistence between
two --aws sessions, which is inside the trust boundary that flag grants. The
conditional-on-host-state pattern appears elsewhere in run.sh and deserves its
own change.

Checks

  • shellcheck --severity=warning (CI's threshold) — clean. Note it is not
    installed in the dev image and there is no root or pip there;
    uv tool run --from shellcheck-py shellcheck works.
  • python3 -m unittest discover -s tests — 106 tests, all pass.
  • openspec validate mask-aws-container-state --strict — valid, then archived
    with the specs synced, per CONTRIBUTING.md step 5.
  • bash -n on every edited script.
  • Not run: docker build plus the smoke cell — no Docker daemon available
    here, so it is left to CI's docker-build job. Archived with that task open
    rather than ticked.

🤖 Generated with Claude Code

stefanwb and others added 3 commits September 8, 2026 09:36
run.sh already treats the AWS CLI's credential cache as too sensitive to
bring in from the host — the scoped --aws mount excludes ~/.aws/credentials
and ~/.aws/cli/cache (run.sh:394-395), and the spec restates it as an
absolute. The container's own /root/.aws then got none of that care: it
sits on claude-code-root, mounted read-write and shared by every session,
with no tmpfs mask. So the STS cache an --aws session writes persisted
after that session exited and was readable by a later session that opted
into nothing.

Two scenarios in external-cli-tools were false as shipped:

  :53  with no flags, "/root/.aws/ does not exist inside the container"
  :66  under --aws, "writes to /root/.aws/ ... fail with EROFS"

The cause is structural: Credentials opt-in states the desired end state
for /root/.aws, while masking is actually specified by "In-container gh
login persists only under --gh" — written for the three CLIs that have an
in-container login flow, so AWS was never added to it. The spec asked for
an outcome no requirement provided a mechanism for. The EROFS claim looks
carried over from the --glab scenario, where the whole config directory is
bind-mounted :ro so it holds by construction; under --aws only config (a
file) and sso/ are mounted, leaving the directory on the volume.

Masks AWS in both directions, changing only scope: without --aws the whole
directory, with it just cli/cache, so the :ro host mounts stay visible.
Corrects "does not exist" to "is empty" — a tmpfs makes a path
exist-but-empty, which is already how the glab-cli and terraform.d lines in
that same scenario read, and literal non-existence is not something a mount
can express.

Not overstated: that cache only ever holds short-lived SSO/STS-derived
material — the long-lived credentials file was never in the container — so
anything in it expires shortly after the session ends. This is a break in
the opt-in boundary, not a live credential leak.

Adds tests/test_masks.py, the first test of any kind over the mask set, and
it had to read run.sh rather than add another container assertion.
smoke/smoke.sh does not invoke run.sh; it re-implements the mask list ("mirrors
run.sh"), so a mask missing from run.sh could not fail the smoke suite —
the harness would simply not add it either. Separately,
assert-in-container.sh checked /root/.aws/config for the masked case, a file
absent without --aws whether or not a mask exists, so that assertion passed
vacuously. Both halves worked as written; neither could observe this. The
new test also asserts the mirror still matches run.sh, so the two cannot
drift apart silently again.

Verified the test fails against the unfixed run.sh — a mask test that passes
before the fix tests nothing.

The --aws cache mask is the only mask that must stay writable, since the CLI
writes derived STS into it and entrypoint.sh's chown walk uses find -xdev and
never descends into a tmpfs. That is asserted in the smoke run rather than
assumed from Docker's default tmpfs mode.

Out of scope, named in the proposal: the host ~/.aws/sso bind-mount is
conditional on that directory existing (run.sh:399), so where it does not, an
in-container aws sso login still caches on the volume. That state is masked
from every non---aws session either way; what remains is persistence between
two --aws sessions, inside the boundary the flag grants. The
conditional-on-host-state pattern appears elsewhere in run.sh and deserves
its own change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two requirements in external-cli-tools change:

Credentials opt-in gains the rule that the container's own
/root/.aws/cli/cache/ must not survive the session that wrote it, and has
two assertions corrected to match a mechanism that now exists — "/root/.aws/
does not exist" becomes "is empty" (a tmpfs makes a path exist-but-empty,
which is already how the glab-cli and terraform.d lines beside it read), and
the blanket EROFS-on-the-directory claim becomes the two mounted paths that
actually are read-only plus the cache's non-persistence.

In-container gh login persists only under --gh gains AWS alongside the
glab-cli and terraform.d masking rules it already carried, plus a rule that
masking never be conditional on host-side state, and two AWS scenarios.

The requirement title is left as-is deliberately: it already governs glab and
terraform.d, so it is a misnomer before this change and no more of one after,
and retitling would mean a REMOVED plus ADDED pair in the delta and a rename
in the synced spec for no behavioural gain.

Archived with 3 tasks open, all intentional: the docker build plus smoke cell
needs a Docker daemon and is left to CI's docker-build job, and two are
follow-ups scoped out of this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The assertion added in 08a69a5 required /root/.aws (or, under --aws, its
credential cache) to be present as an empty tmpfs. That is right for the
eight EPHEMERAL=0 smoke cells and wrong for the --ephemeral one: run.sh's
whole mask block lives inside its EPHEMERAL=0 branch, because --ephemeral
mounts no named volumes and so has nothing to mask. The assertion would
have failed cell 8 for behaving exactly as designed.

It would also have failed any --ephemeral run combined with --aws, since
setup_fake_aws appends the :ro credential mounts to MOUNT_ARGS rather than
VOLUME_ARGS, so those survive --ephemeral while the masks do not.

assert-in-container.sh had no notion of ephemeral mode and smoke.sh did not
pass one, so this adds EXPECT_EPHEMERAL alongside the existing EXPECT_OPTINS
and EXPECT_RO. Under it, the assertion drops the tmpfs requirement and checks
the property the masks exist to provide instead: no AWS credential cache
present at all. Defaults to 0 when unset, so a container run without the
variable still gets the strict check.

Caught by reading ci.yml's cell inventory rather than by a runner — the
change had not been picked up from the queue yet.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@dtump dtump left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against AGENTS.md/CONTRIBUTING.md's OpenSpec workflow and threat-model requirements, the full external-cli-tools spec (not just the delta), the existing smoke/test harness, and live CI output. No findings — approving as-is.

What was checked

Spec correctness. The delta correctly identifies that Credentials opt-in stated an outcome for /root/.aws that no requirement actually mechanized, and homes the fix in the existing In-container gh login persists only under --gh masking requirement rather than inventing a parallel one. The two corrected scenario assertions (does not existis empty, blanket EROFS → the two mounted paths) now match what a tmpfs mount can actually guarantee. Leaving the requirement's gh-centric title alone is a reasonable call — it was already a misnomer covering glab/tfe, and renaming it would be a REMOVED/ADDED pair for no behavioral gain.

Code correctness. Read run.sh:868-874 (PR branch) directly: the AWS mask sits inside the existing EPHEMERAL=0 block, masking the whole /root/.aws when --aws is absent and narrowing to /root/.aws/cli/cache when present — leaving the :ro config/sso bind-mounts (added earlier, at run.sh:397-400) visible. Mount ordering is correct: the narrower tmpfs is layered after the parent volume and the :ro mounts, so it shadows only the cache subpath.

Verified, not just read. Checked out the PR branch in a worktree and ran the suite directly rather than trusting the PR description:

  • python3 -m unittest discover -s tests -p 'test_*.py' -v → 106 tests pass, matching the claim.
  • Swapped main's pre-fix run.sh in against the new tests/test_masks.py — exactly 2 of 6 tests fail (test_mask_set_is_exactly_the_reviewed_set, test_aws_is_masked_in_both_directions...), matching the PR's claim that the test is not vacuous.
  • Pulled the actual docker-build CI log (run 34212175465): aws-cache-masked, aws-cache-masked-mount, aws-cache-masked-scope, and aws-cache-writable all PASS under a real non-root UID (1001), and the --ephemeral cell correctly takes the no-tmpfs short-circuit instead of failing. This directly confirms the PR's one flagged risk — that the cache mask stays writable despite entrypoint.sh's -xdev chown walk never descending into it (entrypoint.sh:64-78).

Reuse. Confirmed smoke/assert-in-container.sh's existing optin_config_path mapping for aws still points at /root/.aws/config — a new, dedicated check_aws_state_masking function was in fact necessary here, not a missed opportunity to extend the existing loop, since repointing that mapping at the directory would break the granted-branch assertion that already depends on it.

Test quality. The mask set is checked by equality, not containment, so a future mask added without a spec/test update fails closed. The SmokeMirrorTest closes the exact gap that let this bug ship in the first place — smoke.sh mirrors run.sh's mask list rather than invoking it, so a missing mask couldn't fail the old suite.

Strengths

  • The root-cause framing is unusually good: distinguishing "the outcome was specified in one requirement, the mechanism in another" from "nobody wrote a mask" makes the fix's placement (extending the existing masking requirement) obviously correct rather than a judgment call.
  • Testing philosophy matches the failure mode: a test that reads run.sh's source was the right call given the smoke suite's mirror-not-invoke architecture, and verifying the new test fails pre-fix is exactly the discipline that makes a regression test trustworthy.
  • The out-of-scope note (conditional ~/.aws/sso host mount) is scoped correctly — named as a real, separate gap rather than either silently ignored or folded into this fix.

@stefanwb
stefanwb merged commit d5dc607 into main Sep 10, 2026
8 checks passed
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