fix(sandbox): deny reads of git's credential stores (#815) - #816
Conversation
git's credential-store backend keeps host passwords and personal access tokens in cleartext, at ~/.git-credentials or ~/.config/git/credentials depending on whether the user is on the XDG layout. Neither was on the deny list, so any command the agent runs could read them. Scoped to those two files deliberately. #815 also covers SSH private keys and the GPG keyring, and those are a harder trade: denying ~/.ssh stops a sandboxed git push over SSH from working, and excluding that directory is not even sufficient, since an IdentityFile directive can point key material anywhere. These two cost nothing by comparison, because git reads them to authenticate rather than to know who the user is, so a sandboxed git keeps working and simply cannot act as the user. The XDG one denies the file rather than the directory around it, which is the opposite of the choice made for ~/.config/zero and is deliberate: ~/.config/git also holds the global git config, which userGitConfigReadPaths grants on purpose so a sandboxed git can read user.name and aliases instead of failing outright. Denying the directory would take that away. Tests assert both stores are denied, that the git config beside the XDG one stays readable, and that an explicit read grant still re-includes them like every other entry. Dropping either entry fails the first; widening the XDG entry to the directory fails the config assertion.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCredential deny-path generation now covers home and XDG Git credential stores. Tests verify existence-based inclusion, preservation of Git configuration access, and explicit allow-read precedence. ChangesGit credential deny coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at 6140e840945e, base 81a5e6c, re-confirmed against the live head before posting. Up to date with main.
Declaring an interest first: this closes a gap I raised as non-blocking on #801, so I am reviewing a fix to my own finding. That does not change whether the code holds up, but you should weigh my approval accordingly.
The fix is verified, not taken on trust. Both paths now come back denied, and both mutations bind: dropping ~/.git-credentials fails TestCredentialDenyReadPathsCoversGitCredentialStores, and dropping the XDG entry fails it too. TestGitCredentialDenyYieldsExplicitAllowRead covers the opt-out, which matters because it is the behaviour the existing entries already have and the one a user relies on to get a path back.
The asymmetry you explain is correct, and I checked it rather than accepting the reasoning. Denying the file ~/.config/git/credentials rather than the directory is right because userGitConfigReadPaths grants ~/.gitconfig and ~/.config/git/config — the config files — so a sibling-file deny does not collide with that grant, where a directory deny would have taken it away. The contrast with ~/.config/zero in #801 is genuine and the reason is the one you give.
Scoping to the credential stores and leaving SSH alone is the right call, and your second argument for it is stronger than the one #815 makes. "Denying ~/.ssh costs a sandboxed git push" is a trade. "Excluding ~/.ssh is not even sufficient, because an IdentityFile directive or an Include chain can put key material anywhere" is a reason the directory rule is the wrong shape, which is a different and better claim.
One non-blocking observation, because that argument cuts both ways. git's credential store is not fixed to these two locations either: credential.helper = "store --file=/anywhere" puts it wherever the user says, exactly as IdentityFile does for SSH keys. So this covers the defaults and not the mechanism. I do not think that argues against merging — covering the default locations is worth doing and the alternative is unbounded — but it is worth stating in the issue so #815 does not read as "SSH is hard, git is solved". The honest framing is that both are default-only, and neither can be closed by enumerating paths.
Verification. On macOS (darwin/arm64): gofmt -l, go vet ./... and git diff --check clean; ./internal/sandbox/... passing; builds clean for linux/amd64 and windows/amd64.
Limitations. The deny list returns early on Windows and is a no-op on macOS, where seatbelt reads are allow-listed, so this only changes Linux behaviour — and I exercised it through the profile builder, never against a live Linux sandbox. That gap is now standing across #801, #815 and this: nobody on any of these threads has run the enforcement path.
Process. The body has no Fixes #815; the reference is in the title only, so tooling reads this as unparented. #815 also carries no issue-approved label yet. Neither affects the code, and you are a collaborator so the gate is process rather than quality, but three of your open PRs now reference issues in a form nothing can see (#808 "foundation for #662", #813, and this).
Merge is kevin's call per the program gate.
…ts it The allow-read case only checked that the credential store was absent from the deny list once an explicit grant covered it. A deny rule that was never added satisfies that just as well, so the test passed with the production change reverted and proved nothing about the interaction it was named for. Check both runs: denied with no grant, absent with one. The first leg is what establishes there is something for the grant to override.
afbe47f
|
Self review before this goes to anyone else. One of the two tests was not testing anything.
Now asserted from both sides: denied with no grant, absent with one. The first leg is what establishes there is anything for the grant to override. Verified it fails under the same revert. Two limits worth stating rather than leaving implied:
|
|
@kevincodex1 ready to merge from my side. All nine checks green on Same situation as #814 and it is my doing: @gnanam1990 approved at What the push fixed: on a second pass over my own work, gnanam, the delta since your approval is confined to |
|
@gnanam1990 same ask as #814, and same cause: my follow up push dismissed your approval at Delta is To check the fix rather than take my word for it, revert both deny lines in On the old head the coverage test failed and the allow read test passed, which is what gave it away: a deny rule that was never added satisfies "absent when granted" just as well as one that was. On the current head both fail under that revert. |
…fixes Resolves the two sandbox conflicts deliberately rather than by side: - internal/sandbox/profile.go keeps this branch's multi-home credentialDenyReadPathsIn and ports Gitlawb#816's git credential-store denies into it. ~/.git-credentials joins candidates only, not dirs, because it is a file and dirs drives directory-shaped handling; the XDG credentials file is added before the zero-directory handling so a normalization failure there cannot drop it. Gitlawb#816's tests come with it, rewritten against the new helper, and the policy JSON baseline in internal/cli/sandbox_test.go now lists both paths so the exported contract catches a regression. - internal/sandbox/profile_test.go stays this branch's external-package file; the ported Gitlawb#816 tests need unexported symbols and live in their own internal test file. runner.go's ZERO_DAEMON_REMOTE_TOKEN_FILE scrub from Gitlawb#677 merged cleanly and is verified present. internal/config/command.go and the rest of main come through untouched. Also carries the review's two design fixes and the introspection note; see the following commit message trailers in the PR discussion for what each one does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Takes option 1 from #815: the git credential stores only.
What
~/.git-credentialsand~/.config/git/credentialsare git's credential-store backend. They hold host passwords and personal access tokens in cleartext, and neither was on the deny list, so any command the agent ran could read them.Measured on
mainbefore the change, with the files materialised, since the builder only emits paths that exist:Why only these two, and not the rest of #815
#815 also covers SSH private keys and the GPG keyring. Those are a genuinely harder trade and this deliberately does not attempt them:
~/.sshstops a sandboxedgit pushover SSH from working. That is a functional cost, not a free win.~/.sshis not even sufficient. AnIdentityFiledirective, or anIncludechain, can point key material anywhere, so a directory rule leaves the key readable exactly when a careful user has moved it.The credential stores have neither problem. git reads them to authenticate, not to know who the user is, so a sandboxed git keeps working and simply cannot act as the user. That is the behaviour you want from a sandbox.
One asymmetry worth explaining
The XDG entry denies the FILE,
~/.config/git/credentials, not the directory around it. That is the opposite of the choice made for~/.config/zeroin #801, and it is deliberate.~/.config/gitalso holds the global git config, anduserGitConfigReadPathsgrants that on purpose so a sandboxed git can readuser.nameand aliases rather than failing with "unable to access". Denying the directory would take that away. Its comment already says the surrounding directory should not be exposed, which is the same instinct, applied from the allow side on macOS.Verification
gofmt,go vet,GOOS=windows go vet, and builds for linux, darwin and windows are clean.internal/sandboxpasses.Three tests, each mutation-checked rather than merely written:
~/.git-credentialsfails the coverage testThe third is the one worth having. It is what stops someone "simplifying" this later into a directory deny and quietly breaking git identity inside the sandbox.
Scope
Linux in practice, like the rest of the list: it is skipped on Windows by design, and macOS allow-lists reads. #815 stays open for the SSH and GPG half, which still needs the posture decision.
Summary by CodeRabbit