fix(run): mask the container's own AWS credential state - #56
Open
stefanwb wants to merge 3 commits into
Open
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes a gap where two
external-cli-toolsscenarios were false as shipped.The asymmetry
run.shalready treats the AWS CLI credential cache as too sensitive to bringin from the host. The scoped
--awsmount excludes~/.aws/credentialsand~/.aws/cli/cache(run.sh:394-395), and the spec restates it as an absolute:"
~/.aws/credentialsand~/.aws/cli/cache/SHALL NEVER be mounted, even under--aws."The container's own
/root/.awsthen got none of that care. It sits onclaude-code-root— read-write, shared by every session — with no tmpfs mask.grep -n -- '--tmpfs' run.shreturned three paths and AWS was not among them.So the STS cache an
--awssession wrote persisted after that session exitedand was readable by a later session that opted into nothing.
Two scenarios were consequently false:
:53/root/.aws/"does not exist"--awssession:66--aws, writes to/root/.aws/"fail with EROFS"The cause is structural.
Credentials opt-instates the desired end statefor
/root/.aws; masking is actually specified byIn-container gh login persists only under --gh, which was written for the three CLIs that have anin-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
--glabscenario, where the whole config directory isbind-mounted
:roso the claim holds by construction; under--awsonlyconfig(a file) andsso/are mounted, leaving the directory itself on theread-write volume.
Not overstated: that cache only ever holds short-lived SSO/STS-derived
material — the long-lived
credentialsfile was never in the container — soanything 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, thewhole directory. With it, just
cli/cache, so the:rohost mounts at/root/.aws/configand/root/.aws/ssostay visible to the session that askedfor them. Unlike
gh, there is no in-container login to preserve, so there isno unmask state.
"does not exist"becomes"is empty": a tmpfs makes a path exist-but-empty,which is already how the
glab-cliandterraform.dlines in that samescenario 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.shdoes not invokerun.sh— it re-implements the mask list, and its own comment says "mirrors run.sh". A
mask missing from
run.shtherefore could not fail the smoke suite, because theharness would simply not add it either. The suite tests the mirror.
Separately,
assert-in-container.shdoes check that a non-granted opt-in'sconfig path is absent or empty, but its AWS entry points at
/root/.aws/config— a file absent without
--awswhether or not a mask exists — so that assertionpassed 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.shitself,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 testthat 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
--awscache mask is the only mask that must stay writable — the CLIwrites derived STS into it, and
entrypoint.sh's chown walk usesfind -xdev,so it never descends into a tmpfs and cannot fix ownership there. That should
hold on Docker's default
--tmpfsmode, but I could not verify it here withouta daemon, so
assert-in-container.shnow asserts the write succeeds rather thanassuming it. CI's smoke cell going green is the confirmation.
Out of scope (named in the proposal, not silently skipped)
The host
~/.aws/ssobind-mount is conditional on that directory existing(
run.sh:399), so where it does not, an in-containeraws sso loginstillcaches on the volume. That state is masked from every non-
--awssession eitherway, so the boundary this PR is about holds; what remains is persistence between
two
--awssessions, which is inside the trust boundary that flag grants. Theconditional-on-host-state pattern appears elsewhere in
run.shand deserves itsown change.
Checks
shellcheck --severity=warning(CI's threshold) — clean. Note it is notinstalled in the dev image and there is no root or pip there;
uv tool run --from shellcheck-py shellcheckworks.python3 -m unittest discover -s tests— 106 tests, all pass.openspec validate mask-aws-container-state --strict— valid, then archivedwith the specs synced, per
CONTRIBUTING.mdstep 5.bash -non every edited script.docker buildplus the smoke cell — no Docker daemon availablehere, so it is left to CI's
docker-buildjob. Archived with that task openrather than ticked.
🤖 Generated with Claude Code