feat(sandbox): Windows sandbox principals (foundation for #662) - #808
feat(sandbox): Windows sandbox principals (foundation for #662)#808Vasanthdev2004 wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Windows sandbox identity provisioning, protected principal-secret storage, batch logon support, runtime token selection, and principal-specific ACL planning with Windows-focused unit and integration tests. ChangesWindows sandbox principal
Estimated code review effort: 4 (Complex) | ~75 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/sandbox/windows_identity_logon_windows.go (2)
48-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the five separate
advapi32.dlllazy loads.Five independent
windows.NewLazySystemDLL("advapi32.dll")calls wherewindows_identity_windows.gouses a single sharednetapi32var for its DLL and derives procs from it. Mirroring that pattern here is cheap and keeps the two files consistent.♻️ Proposed refactor
-var ( - procLogonUserW = windows.NewLazySystemDLL("advapi32.dll").NewProc("LogonUserW") - procLsaOpenPolicy = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaOpenPolicy") - procLsaClose = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaClose") - procLsaAddAccountRights = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaAddAccountRights") - procLsaNtStatusToWinErr = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaNtStatusToWinError") -) +var ( + advapi32 = windows.NewLazySystemDLL("advapi32.dll") + procLogonUserW = advapi32.NewProc("LogonUserW") + procLsaOpenPolicy = advapi32.NewProc("LsaOpenPolicy") + procLsaClose = advapi32.NewProc("LsaClose") + procLsaAddAccountRights = advapi32.NewProc("LsaAddAccountRights") + procLsaNtStatusToWinErr = advapi32.NewProc("LsaNtStatusToWinError") +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_logon_windows.go` around lines 48 - 54, Consolidate the five independent advapi32.dll lazy loads in the proc declarations around procLogonUserW, procLsaOpenPolicy, procLsaClose, procLsaAddAccountRights, and procLsaNtStatusToWinErr by defining one shared lazy DLL variable and deriving each procedure from it, matching the shared-DLL pattern used by the neighboring Windows identity implementation.
195-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant/fragile "keep alive" idiom repeated across both files.
Both files independently reinvent a "keep the buffer alive after the syscall" step, but the object is already retained through the call by the compiler's special-case handling of
uintptr(unsafe.Pointer(x))appearing in the.Call()argument list (perunsafepackage docs, this also applies toLazyProc.Callon Windows), and pointer fields nested inside that object are reachable transitively via normal GC tracing. None of these five sites add real protection, and if protection were ever genuinely needed,_ = buffer[0]/_ = infois not the guaranteed primitive for it —runtime.KeepAliveis.
internal/sandbox/windows_identity_logon_windows.go#L195-L203: replace theruntimeKeepAliveUint16helper with a directruntime.KeepAlive(buffer)call at each use (or drop it, since the buffer is already protected viaentryin the.Call()argument).internal/sandbox/windows_identity_logon_windows.go#L150-L152: swapruntimeKeepAliveUint16(buffer)forruntime.KeepAlive(buffer), or remove the line.internal/sandbox/windows_identity_windows.go#L202-L204: dropdefer func(){_=info}()inensureWindowsSandboxGroup, or replace withdefer runtime.KeepAlive(&info)if you want to keep the intent explicit.internal/sandbox/windows_identity_windows.go#L239: same for theinfodefer inensureWindowsSandboxUser.internal/sandbox/windows_identity_windows.go#L262: same for theentrydefer inaddWindowsSandboxUserToGroup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_logon_windows.go` around lines 195 - 203, Remove the redundant fragile keep-alive idioms and rely on the syscall argument retention; in internal/sandbox/windows_identity_logon_windows.go:150-152 and :195-203, remove runtimeKeepAliveUint16 and its uses (or replace each with runtime.KeepAlive(buffer) if explicit intent is retained). In internal/sandbox/windows_identity_windows.go:202-204, :239, and :262, remove the defer closures referencing info or entry, or replace them with defer runtime.KeepAlive(&info) / defer runtime.KeepAlive(&entry) respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_acl.go`:
- Around line 85-91: Validate each value in ProtectedMetadataNames before
constructing the WindowsACLEntry, accepting only a single non-empty path
component and rejecting empty values, "."/"..", and any value containing path
separators. Do not call filepath.Join for rejected names; add tests covering
traversal and separator-containing inputs while preserving valid-name
materialization.
---
Nitpick comments:
In `@internal/sandbox/windows_identity_logon_windows.go`:
- Around line 48-54: Consolidate the five independent advapi32.dll lazy loads in
the proc declarations around procLogonUserW, procLsaOpenPolicy, procLsaClose,
procLsaAddAccountRights, and procLsaNtStatusToWinErr by defining one shared lazy
DLL variable and deriving each procedure from it, matching the shared-DLL
pattern used by the neighboring Windows identity implementation.
- Around line 195-203: Remove the redundant fragile keep-alive idioms and rely
on the syscall argument retention; in
internal/sandbox/windows_identity_logon_windows.go:150-152 and :195-203, remove
runtimeKeepAliveUint16 and its uses (or replace each with
runtime.KeepAlive(buffer) if explicit intent is retained). In
internal/sandbox/windows_identity_windows.go:202-204, :239, and :262, remove the
defer closures referencing info or entry, or replace them with defer
runtime.KeepAlive(&info) / defer runtime.KeepAlive(&entry) respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2343104-e3d2-400c-8739-a6f655821fe1
📒 Files selected for processing (6)
internal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_identity_acl.gointernal/sandbox/windows_identity_acl_test.gointernal/sandbox/windows_identity_logon_windows.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
| for _, name := range root.ProtectedMetadataNames { | ||
| entries = append(entries, WindowsACLEntry{ | ||
| Action: WindowsACLDenyWrite, | ||
| Path: filepath.Join(cleaned, name), | ||
| Capability: input.PrincipalSID, | ||
| Materialize: true, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject protected metadata names that escape the write root.
ProtectedMetadataNames is documented as names, but filepath.Join accepts .. and separator-containing values. A malformed value can materialize a deny ACE outside root.Root. Require one non-empty path component and add rejection tests.
Proposed fix
for _, name := range root.ProtectedMetadataNames {
+ if name == "" || name == "." || name == ".." || filepath.Base(name) != name {
+ return WindowsACLPlan{}, fmt.Errorf(
+ "windows principal ACL plan: invalid protected metadata name %q", name,
+ )
+ }
entries = append(entries, WindowsACLEntry{
Action: WindowsACLDenyWrite,
Path: filepath.Join(cleaned, name),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, name := range root.ProtectedMetadataNames { | |
| entries = append(entries, WindowsACLEntry{ | |
| Action: WindowsACLDenyWrite, | |
| Path: filepath.Join(cleaned, name), | |
| Capability: input.PrincipalSID, | |
| Materialize: true, | |
| }) | |
| for _, name := range root.ProtectedMetadataNames { | |
| if name == "" || name == "." || name == ".." || filepath.Base(name) != name { | |
| return WindowsACLPlan{}, fmt.Errorf( | |
| "windows principal ACL plan: invalid protected metadata name %q", name, | |
| ) | |
| } | |
| entries = append(entries, WindowsACLEntry{ | |
| Action: WindowsACLDenyWrite, | |
| Path: filepath.Join(cleaned, name), | |
| Capability: input.PrincipalSID, | |
| Materialize: true, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/sandbox/windows_identity_acl.go` around lines 85 - 91, Validate each
value in ProtectedMetadataNames before constructing the WindowsACLEntry,
accepting only a single non-empty path component and rejecting empty values,
"."/"..", and any value containing path separators. Do not call filepath.Join
for rejected names; add tests covering traversal and separator-containing inputs
while preserving valid-name materialization.
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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/sandbox/windows_command_runner_windows.go (2)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the operator an exit when the principal backend breaks.
This is the one path that hard-fails instead of falling back, and the message is a bare wrapped error. Since the whole feature is opt-in, tell the user how to opt back out — the
ensureWindowsUnelevatedSetupmessage at Line 136 is a good model for actionable runner errors.♻️ Suggested wording
principalToken, ok, err := windowsSandboxPrincipalToken(config) if err != nil { - fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + fmt.Fprintf(stderr, "%s: sandbox principal is provisioned but unusable: %v — "+ + "re-run `zero sandbox setup` from an elevated terminal, or unset %s to fall back to the restricted-token sandbox\n", + WindowsSandboxCommandRunnerName, err, windowsSandboxIdentityEnv) return 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_command_runner_windows.go` around lines 84 - 88, Update the error handling around windowsSandboxPrincipalToken so the stderr message explains that the Windows sandbox principal backend failed and gives the operator an actionable way to disable or opt out of the opt-in feature, following the guidance style used by ensureWindowsUnelevatedSetup. Preserve the existing immediate exit with status 1.
89-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the principal lookup above the restricted-token SID computation.
capabilitySIDs,offlineSID,tokenSIDs, andwriteRestrictedare all computed unconditionally and discarded on the principal path. Moving thewindowsSandboxPrincipalTokencall to just after the network-policy validation makes the two backends read as a clean either/or and avoids the wasted SID resolution. (Only do this if the network-enforcement question above resolves in favor of keeping the principal path independent of those SIDs.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_command_runner_windows.go` around lines 89 - 97, Move the windowsSandboxPrincipalToken lookup and its success-path handling to immediately after network-policy validation, before computing capabilitySIDs, offlineSID, tokenSIDs, or writeRestricted. Keep the principal-token execution via runWindowsCommandAsUser unchanged, and ensure the restricted-token SID calculations run only on the fallback path.internal/sandbox/windows_identity_secret_windows.go (1)
139-166: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider DPAPI for the on-disk secret. The ACL blocks other users, but the password is still stored in plaintext. If you want defense in depth against offline inspection or backup exposure, encrypt it with DPAPI before writing it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_secret_windows.go` around lines 139 - 166, Update writeWindowsSandboxSecret to protect the password with Windows DPAPI before persisting it, writing the encrypted bytes instead of plaintext while preserving the existing owner ACL and cleanup behavior. Reuse the repository’s existing DPAPI encryption helper if available; otherwise add the minimal Windows-specific encryption step and report encryption failures without writing the secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_command_runner_windows.go`:
- Around line 78-97: Update the principal execution branch in the Windows
command runner so deny-mode commands cannot bypass network isolation: either
make the WFP filter use the provisioned principal SID, or bypass the principal
path and continue through the restricted-token backend when NetworkDeny is
enabled. Ensure the existing windowsRuntimeTokenSIDs-based deny behavior remains
enforced.
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 106-127: Update provisionWindowsSandboxPrincipalForSetup to reset
the password for existing principals before writeWindowsSandboxSecret persists
the credential. Reuse ensureWindowsSandboxUser’s existing account-handling
behavior or adjust the provisioning flow so nerrUserExists accounts receive the
newly generated password, while preserving fresh-account provisioning and
subsequent logon-rights setup.
In `@internal/sandbox/windows_identity_secret_windows_test.go`:
- Around line 181-196: Update windowsSecretACEList to inspect the generic
ACE_HEADER returned by GetAce before interpreting it as ACCESS_ALLOWED_ACE.
Accept only the supported allow-ACE type, and return a clear error for deny,
object, or any other unsupported ACE type so invalid SID offsets cannot be
decoded as trustees.
---
Nitpick comments:
In `@internal/sandbox/windows_command_runner_windows.go`:
- Around line 84-88: Update the error handling around
windowsSandboxPrincipalToken so the stderr message explains that the Windows
sandbox principal backend failed and gives the operator an actionable way to
disable or opt out of the opt-in feature, following the guidance style used by
ensureWindowsUnelevatedSetup. Preserve the existing immediate exit with status
1.
- Around line 89-97: Move the windowsSandboxPrincipalToken lookup and its
success-path handling to immediately after network-policy validation, before
computing capabilitySIDs, offlineSID, tokenSIDs, or writeRestricted. Keep the
principal-token execution via runWindowsCommandAsUser unchanged, and ensure the
restricted-token SID calculations run only on the fallback path.
In `@internal/sandbox/windows_identity_secret_windows.go`:
- Around line 139-166: Update writeWindowsSandboxSecret to protect the password
with Windows DPAPI before persisting it, writing the encrypted bytes instead of
plaintext while preserving the existing owner ACL and cleanup behavior. Reuse
the repository’s existing DPAPI encryption helper if available; otherwise add
the minimal Windows-specific encryption step and report encryption failures
without writing the secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90fab087-5f05-4a9a-ae92-73e983828792
📒 Files selected for processing (4)
internal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_secret_windows.gointernal/sandbox/windows_identity_secret_windows_test.go
|
Validation update: the provisioning chain has now been run for real, elevated, on Windows 11. and the objects it created were really there, confirmed independently afterwards: Verified end to end: NetLocalGroupAdd, NetUserAdd, NetLocalGroupAddMembers and the SID lookup all succeed against the real APIs; a second provision returns the same username and SID, so the idempotent "already exists" handling is correct; and lookup finds what provisioning created. Notably there was no ERROR_PASSWORD_RESTRICTION, so the generated password satisfies the default complexity policy. That also means the hand-rolled USER_INFO_1, LOCALGROUP_INFO_1 and LOCALGROUP_MEMBERS_INFO_3 layouts marshal correctly, which matters because they are passed as raw buffers where a wrong field order fails or corrupts memory rather than erroring cleanly. Still not verified: that test exercises provisionWindowsSandboxIdentity only. LsaAddAccountRights (the batch-logon grant and the deny-interactive hardening) and LogonUser (minting the token) have still never executed, so the identity is proven to exist but not yet proven usable. CI cannot cover either, since it runs unelevated. Also still open: the provisioning entry points have no non-test callers yet. Keeping this a draft until the logon half is exercised too. |
|
Setup is wired now, so the feature is reachable end to end rather than inert.
Provisioning is folded into setup's existing rollback rather than each later failure path having to remember it, and the rollback revokes ACEs before deleting the account. Doing it the other way round would leave ACEs naming a SID that no longer resolves, which is the orphaned residue this model exists to avoid. Everything stays behind How to exercise it, on a machine where creating local accounts is acceptable: Validation status: provisioning (group, account, membership, SID, idempotency) is confirmed working elevated on Windows 11. The logon half now has a test, TestGrantLogonRightsAndMintPrincipalToken, which exercises LsaAddAccountRights and LogonUser and asserts the minted token's user SID is the principal rather than the caller. It has not been run yet; Smart App Control blocks freshly built unsigned binaries on the machine available to me, so it needs a box without that restriction. That is the last unproven primitive and the reason this is still a draft. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_runtime_windows_test.go`:
- Around line 11-29: Make TestWindowsSandboxIdentityGating hermetic by clearing
windowsSandboxIdentityEnv from the process environment before running the table,
so the "absent" case cannot fall back to an externally set value. Restore the
original environment after the test using the standard test cleanup mechanism.
In `@internal/sandbox/windows_setup_windows.go`:
- Around line 38-64: Add coverage in the Windows sandbox setup tests for the
flow around runWindowsSandboxSetup: verify opt-out does not call
setupWindowsSandboxPrincipal, and verify an opt-in principal-setup failure still
invokes the existing ACL rollback. Use the test’s existing configuration and
rollback helpers, preserving current success and error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb64b652-8bb9-4259-8b0e-53533dd380cf
📒 Files selected for processing (3)
internal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_setup_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/windows_identity_runtime_windows.go
| // Provision this workspace's sandbox principal, when opted in. A principal is | ||
| // a separate local account, so it is created only on an explicit opt-in: it | ||
| // is visible in `net user`, and account creation is exactly the kind of thing | ||
| // endpoint protection and enterprise policy object to. Without the opt-in the | ||
| // capability-SID backend above is the whole of setup, unchanged. | ||
| if windowsSandboxIdentityEnabled(config.commandConfig().Env) { | ||
| principalRollback, err := setupWindowsSandboxPrincipal(config.commandConfig()) | ||
| if err != nil { | ||
| if rollbackErr := rollback(); rollbackErr != nil { | ||
| fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) | ||
| return 1 | ||
| } | ||
| fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) | ||
| return 1 | ||
| } | ||
| // Fold the principal into the existing rollback so every later failure | ||
| // path undoes it too, rather than each one having to remember. | ||
| aclRollback := rollback | ||
| rollback = func() error { | ||
| principalErr := principalRollback() | ||
| aclErr := aclRollback() | ||
| if principalErr != nil { | ||
| return principalErr | ||
| } | ||
| return aclErr | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate existing setup-level regression coverage without executing repository code.
rg -nP --glob '*_test.go' -C 3 '\brunWindowsSandboxSetup\s*\(' internal/sandbox
rg -nP --glob '*_test.go' -C 3 '\bsetupWindowsSandboxPrincipal\s*\(|\bwindowsSandboxIdentityEnabled\s*\(' internal/sandboxRepository: Gitlawb/zero
Length of output: 1982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the setup implementation and its tests.
git ls-files 'internal/sandbox/*windows*' 'internal/sandbox/*_test.go'
printf '\n--- windows_setup_windows.go (outline) ---\n'
ast-grep outline internal/sandbox/windows_setup_windows.go --view expanded || true
printf '\n--- matching tests and references ---\n'
rg -n -C 3 'runWindowsSandboxSetup|setupWindowsSandboxPrincipal|windowsSandboxIdentityEnabled|rollback' internal/sandbox/*_test.go internal/sandbox/*.goRepository: Gitlawb/zero
Length of output: 28424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- windows_setup_test.go outline ---'
ast-grep outline internal/sandbox/windows_setup_test.go --view expanded || true
printf '\n%s\n' '--- references to runWindowsSandboxSetup and related setup helpers in windows_setup_test.go ---'
rg -n -C 4 'runWindowsSandboxSetup|setupWindowsSandboxPrincipal|windowsSandboxIdentityEnabled|WindowsSandboxSetupConfig|WriteWindowsSandboxSetupMarker|applyWindowsACLPlan|applyWindowsNetworkPlan' internal/sandbox/windows_setup_test.go
printf '\n%s\n' '--- first 220 lines of windows_setup_test.go ---'
cat -n internal/sandbox/windows_setup_test.go | sed -n '1,220p'Repository: Gitlawb/zero
Length of output: 13408
Add runWindowsSandboxSetup coverage in internal/sandbox/windows_setup_test.go. The current tests stop at marker/config helpers; add one case for opt-out skipping setupWindowsSandboxPrincipal, and one for opt-in failure still calling the ACL rollback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/sandbox/windows_setup_windows.go` around lines 38 - 64, Add coverage
in the Windows sandbox setup tests for the flow around runWindowsSandboxSetup:
verify opt-out does not call setupWindowsSandboxPrincipal, and verify an opt-in
principal-setup failure still invokes the existing ACL rollback. Use the test’s
existing configuration and rollback helpers, preserving current success and
error behavior.
Source: Coding guidelines
|
Thanks, this was a useful pass. Went through all three. Network enforcement (the hedge on the second point) turned out to be the real finding. Chasing it down: Fixed in fb8e39b: the principal stands down whenever the network is denied and the restricted-token path runs instead. Keying the filters to the principal's own SID is the follow-up that lifts the restriction, and I would rather do that with the privileged paths validated on a clean box than bolt it on here. Worth flagging that my first regression test for this was worthless. It called Actionable error: taken. The message now names DPAPI: also taken, in deb3a98. The ACL is still the primary control and the thing that keeps the principal from reading its own credential, but you are right that it only binds while the filesystem is the one being asked, so a backup or a mounted image gives up the password in the clear. Hoisting the lookup above the SID computation: leaving it. Now that the principal path is gated on network mode, it is no longer independent of those SIDs, so the ordering earns its keep. Still unproven and called out in the description: |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested.
Two things drive that. The lookup path below discards a check you deliberately wrote, and it should be fixed regardless of what else happens. Separately, the privileged half of this change has never been executed by anyone, and account provisioning, logon-rights assignment and credential storage are not things I am willing to approve unrun, however sound the design reasoning is. Neither point is a criticism of the direction, which I think is right.
The design reasoning here is unusually clear, and the honesty about what has and has not been run is appreciated.
One practical note before anything else: the description opens by calling this a draft, but the pull request is not marked as a draft on GitHub, so it currently sits open for review and merge. Converting it would match your stated intent. Related, the Smoke jobs for macOS, Ubuntu and Windows, along with Zero Review, were still pending when I looked, so the CI signal you describe as the check for the wiring commit has not yet reported.
What I was able to verify. On macOS, make fmt-check, go build ./... and go vet ./... are clean, and the full suite passes at 82 packages with no failures. More usefully for a change of this shape, GOOS=windows go vet ./internal/sandbox/... exits cleanly and GOOS=windows go test -c compiles the test binary, which type-checks the roughly 1,500 lines of _windows.go that never compile on a non-Windows host. That is not execution, but it does confirm the Win32 call sites, struct definitions and build tags hold together across the whole addition.
I also mutated the ACL ordering to check the test does real work: reversing the entry order returned by buildWindowsPrincipalACLPlan fails TestPrincipalACLPlanEmitsDeniesBeforeAllows. The deny-before-allow invariant is genuinely asserted rather than only documented.
Two further things came back clean and are worth recording. Password generation draws 24 bytes from crypto/rand and encodes them with unpadded base32, giving roughly 120 bits with no modulo bias, and the fixed prefix covering the complexity classes is a reasonable approach. Account naming leaves 11 hex characters of the SHA-256 digest after the nine-character prefix, so 44 bits, which puts a birthday collision far beyond any plausible number of workspaces on one machine.
One substantive finding. lookupWindowsSandboxIdentity (internal/sandbox/windows_identity_windows.go:338-345) collapses every error from resolveWindowsSandboxSID into errWindowsSandboxIdentityUnavailable, which discards the deliberate check you wrote at lines 274-276 refusing a name that resolves to a non-user account.
The effect is that if zero-sbx-<hash> is squatted by a pre-existing local group or alias, resolveWindowsSandboxSID correctly refuses it, but the caller reads that refusal as "not provisioned" and windowsSandboxPrincipalToken (lines 73-76 of windows_identity_runtime_windows.go) falls back quietly to the restricted token. Your own description draws the line in the right place, that only a provisioned-but-unusable identity should surface an error, and this is precisely that case reaching the operator as silence. Distinguishing ERROR_NONE_MAPPED from other lookup failures would preserve the fallback for the common "setup has not run" case while surfacing the rest.
A smaller one: the comment at windows_identity_windows.go:122 refers the reader to sandboxRuntimeKey for how the workspace key is hashed, but no such symbol exists. The function is windowsSandboxWorkspaceKey in windows_identity_runtime_windows.go:44.
On the question you raised for decision. Creating real local accounts being visible to endpoint protection, enterprise policy and net user seems worth settling before this leaves draft, and I agree it is a product call rather than a design flaw. The inversion argument is persuasive on its merits: unreachable by construction is a stronger boundary than an enumerated deny list, and the trustee-keyed revocation answers a real gap.
Limitations of this review. I have no Windows host and no elevated session, so NetUserAdd, LsaAddAccountRights, NetUserDel and LogonUser are unexecuted by me as well. I did not check the raw Win32 struct layouts against the SDK, and I did not review the LSA byte-versus-rune length handling beyond confirming it compiles. Everything above rests on reading the code and on cross-compilation.
Worth flagging for coordination: this addresses the same credentialDenyReadPaths weakness on Windows that I raised on #801, where removing the sandbox HOME and XDG_CONFIG_HOME overrides makes real credential locations the resolution target. The two changes point at the same boundary from opposite sides and would benefit from being sequenced deliberately.
Merge is kevin's call per the program gate.
|
CI is green now. The Windows smoke failure was not from this branch, and it is worth saying what it actually was rather than just re-running until it passed. Three tests failed, all in Fixes are up separately rather than folded in here, since they have nothing to do with the sandbox work and one of them touches product code:
I also opened #811 for something that fell out of the reproduction and is a genuine user-facing bug rather than a test problem: the provider-command timeout is a floor, not a bound. Process creation happens before the timer is armed and the drain after Nothing on this branch changed for any of that. Once #809 and #810 land I will rebase this one. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/sandbox/windows_identity_runtime_windows_test.go (1)
11-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTable is still not hermetic.
The
"absent"case falls through toos.Getenv, so this test fails on any machine that actually hasZERO_WINDOWS_SANDBOX_IDENTITY=1exported — precisely the machines doing the elevated validation runs for this PR. Addt.Setenv(windowsSandboxIdentityEnv, "")before the table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_runtime_windows_test.go` around lines 11 - 22, Make TestWindowsSandboxIdentityGating hermetic by setting windowsSandboxIdentityEnv to an empty value with t.Setenv before iterating over the test cases, ensuring the "absent" case cannot inherit the host environment.internal/sandbox/windows_identity_secret_windows_test.go (1)
183-198: 🎯 Functional Correctness | 🟡 Minor | 💤 Low valueStill assumes every ACE is an
ACCESS_ALLOWED_ACE.
GetAcereturns a genericACE_HEADER; a deny or object ACE would put the SID at a different offset and this helper would decode garbage, making the "unexpected trustee" assertion misleading rather than failing cleanly. Gate onace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPEand return an error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_secret_windows_test.go` around lines 183 - 198, The windowsSecretACEList helper must validate each ACE type before interpreting its SID layout. After GetAce returns, check ace.Header.AceType and return an error for any type other than windows.ACCESS_ALLOWED_ACE_TYPE; only then cast to ACCESS_ALLOWED_ACE and copy the SID.internal/sandbox/windows_identity_acl.go (1)
85-92: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPath traversal via
ProtectedMetadataNamesstill unaddressed.
filepath.Join(cleaned, name)accepts../separator-bearing values, so a malformedProtectedMetadataNamesentry can materialize a deny ACE outsideroot.Root. This was flagged in a prior review and is still present with no validation added.🔒 Proposed fix
for _, name := range root.ProtectedMetadataNames { + if name == "" || name == "." || name == ".." || filepath.Base(name) != name { + return WindowsACLPlan{}, fmt.Errorf( + "windows principal ACL plan: invalid protected metadata name %q", name, + ) + } entries = append(entries, WindowsACLEntry{ Action: WindowsACLDenyWrite, Path: filepath.Join(cleaned, name),Add a regression test in
windows_identity_acl_test.gocovering a traversal/separator-bearing name once this validation lands. As per coding guidelines,**/*_test.go: "add regression tests for behavior changes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_acl.go` around lines 85 - 92, Validate each entry from root.ProtectedMetadataNames before constructing the WindowsACLEntry, rejecting traversal or separator-bearing names that could escape cleaned/root.Root; only append entries for safe metadata names. Add a regression test in windows_identity_acl_test.go covering both traversal and separator-bearing input.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/sandbox/windows_identity_windows.go (1)
196-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
runtime.KeepAliveinstead of a deferred no-op.
defer func() { _ = info }()does keepinfoalive (the closure captures it), but it reads as dead code and a future cleanup will delete it, silently reintroducing a use-after-free window. The same pattern repeats at Lines 239 and 262.♻️ Proposed change
status, _, _ := procNetLocalGroupAdd.Call( 0, // local machine 1, // level: LOCALGROUP_INFO_1 uintptr(unsafe.Pointer(&info)), 0, ) - // Keep info alive across the call: the struct holds pointers into Go memory - // that the syscall dereferences. - defer func() { _ = info }() + // Keep info (and the Go strings it points at) alive across the call. + runtime.KeepAlive(info) return netAPIStatus("NetLocalGroupAdd", status, nerrGroupExists, errorAliasExists)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_windows.go` around lines 196 - 205, Replace the deferred no-op keeping info alive in the NetLocalGroupAdd call with runtime.KeepAlive(info) after the syscall returns. Apply the same change to the corresponding patterns around the related calls at Lines 239 and 262, and add the runtime import if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_logon_windows.go`:
- Around line 108-154: The native Windows calls need explicit GC liveness
guarantees for all borrowed arguments. In grantWindowsSandboxLogonRights, add
runtime.KeepAlive for attributes after procLsaOpenPolicy.Call and for entry
after procLsaAddAccountRights.Call, while retaining the buffer keep-alive; also
update the LogonUserW call site to keep the user, domain, and secret pointers
alive after the call returns.
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 139-145: Update the Windows sandbox identity flow around
ensureWindowsSandboxUser and writeWindowsSandboxSecret so a pre-existing
account’s password is actually synchronized before writing the secret. Remove
the inaccurate claim that the caller resets the password, and ensure the stored
secret matches the account password for both new and existing users.
In `@internal/sandbox/windows_identity_secret_windows.go`:
- Around line 186-196: Update readWindowsSandboxSecret to map permission-denied
errors, including Windows ERROR_ACCESS_DENIED, to
errWindowsSandboxIdentityUnavailable alongside missing-file errors so callers
fall back to the restricted token. Update removeWindowsSandboxSecret to treat
the same unreadable or inaccessible-secret condition as non-fatal, allowing
principal cleanup to continue while preserving other error propagation.
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 213-241: The existing-user path in ensureWindowsSandboxUser must
reset the account password via NetUserSetInfo at level 1003 using USER_INFO_1003
before returning success; update internal/sandbox/windows_identity_windows.go
lines 213-241 accordingly while preserving normal creation behavior. In
internal/sandbox/windows_identity_runtime_windows.go lines 139-145, revise the
related comment to accurately describe that ensureWindowsSandboxUser performs
the password reset.
---
Duplicate comments:
In `@internal/sandbox/windows_identity_acl.go`:
- Around line 85-92: Validate each entry from root.ProtectedMetadataNames before
constructing the WindowsACLEntry, rejecting traversal or separator-bearing names
that could escape cleaned/root.Root; only append entries for safe metadata
names. Add a regression test in windows_identity_acl_test.go covering both
traversal and separator-bearing input.
In `@internal/sandbox/windows_identity_runtime_windows_test.go`:
- Around line 11-22: Make TestWindowsSandboxIdentityGating hermetic by setting
windowsSandboxIdentityEnv to an empty value with t.Setenv before iterating over
the test cases, ensuring the "absent" case cannot inherit the host environment.
In `@internal/sandbox/windows_identity_secret_windows_test.go`:
- Around line 183-198: The windowsSecretACEList helper must validate each ACE
type before interpreting its SID layout. After GetAce returns, check
ace.Header.AceType and return an error for any type other than
windows.ACCESS_ALLOWED_ACE_TYPE; only then cast to ACCESS_ALLOWED_ACE and copy
the SID.
---
Nitpick comments:
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 196-205: Replace the deferred no-op keeping info alive in the
NetLocalGroupAdd call with runtime.KeepAlive(info) after the syscall returns.
Apply the same change to the corresponding patterns around the related calls at
Lines 239 and 262, and add the runtime import if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4be32672-966b-47b1-955b-a7e02d7e5891
📒 Files selected for processing (13)
internal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_identity_acl.gointernal/sandbox/windows_identity_acl_test.gointernal/sandbox/windows_identity_dpapi_windows.gointernal/sandbox/windows_identity_logon_windows.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_identity_secret_windows.gointernal/sandbox/windows_identity_secret_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.gointernal/sandbox/windows_setup_windows.go
|
Thanks, this is a good review, and the lookup finding is right. The squatted-name case. Fixed in 9e1e651. You are right that it lands exactly where the description says the line should sit, and I had written the check and then thrown it away one call later. It was worse than the one site you found: The decision sits in its own function rather than inline, because the lookup derives its account name from a workspace key, so a test cannot hand it a name that resolves to a group. The test drives that classifier with a real error from a well-known local group, needs no privilege, and I checked it fails if the old collapse-everything behaviour is restored: The stale comment. Fixed, it is The draft framing. That was stale and I have rewritten the opening. This is not a draft: it is opt-in behind an environment variable and I would rather it be reviewed than sit hidden. The provisioning half has since been run on a real elevated session, so account and group creation are no longer unexecuted. CI. It has reported since, and is green on all nine checks. Three Windows tests did fail on the first run, none of them in code this branch touches. I reproduced two of them locally under CPU contention on a clean tree, so they were pre-existing flakes rather than anything here; they are fixed in #810 and #809, and #811 covers a genuine product bug that fell out of the reproduction. On sequencing with #801. Agreed, and worth being concrete: these do point at the same boundary from opposite sides. #801 removes the sandbox Also worth flagging for the same reason: this backend currently stands down whenever the network is denied, which is the default. WFP filters key on the offline-marker SID and a The two things you verified that I could not, the cross-compiled vet and |
|
Both taken, and the first one was a real bug rather than a documentation slip. The pre-existing account. You are right, and the effect is worse than the comment being wrong. Fixed in e33dce0. The gated provisioning test now provisions twice and logs on with the password from the second run. That is the only assertion worth having here: a stale password is indistinguishable from a correct one until something actually authenticates with it, so checking that the two runs return the same identity would have passed straight through this bug. The keep-alives. Also taken.
On the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 239-246: After provisioning the test principal in the gated
identity test, register a t.Cleanup callback that revokes SeBatchLogonRight and
removes the test principal, ensuring cleanup runs on every subsequent failure
path. Keep the existing grantWindowsSandboxLogonRights and
logonWindowsSandboxPrincipal flow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d03dfa6a-7671-40c4-b4c8-5d77781ed16c
📒 Files selected for processing (4)
internal/sandbox/windows_identity_logon_windows.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sandbox/windows_identity_logon_windows.go
- internal/sandbox/windows_identity_runtime_windows.go
- internal/sandbox/windows_identity_windows.go
|
Taken, and it was pointing at more than the test. You are right that the round trip left residue: it granted a real batch logon right to a real local account and had no cleanup at all, so anyone running the gated suite kept both. That is on me, and it got worse when I added the logon step in the last commit. The part worth flagging is that the same hole was in the production teardown. Fixed in fbe340b:
One thing I did not want to take on trust. Treating "this account holds no rights" as success depends on
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at fbe340b3995c, base ac50a5a840d2, re-confirmed against the live head before posting.
I withdraw both findings from my previous review. Each is fixed, and the first is fixed in the way I hoped rather than the cheapest way.
lookupWindowsSandboxIdentity no longer collapses every lookup failure into "not provisioned". classifyWindowsSandboxLookupError (internal/sandbox/windows_identity_windows.go) maps ERROR_NONE_MAPPED to errWindowsSandboxIdentityUnavailable and returns everything else unchanged, so the deliberate refusal in resolveWindowsSandboxSID for a name resolving to a non-user account now reaches the operator instead of degrading quietly to the restricted token. TestLookupWindowsSandboxIdentityRejectsNonUserAccount covers exactly that case. The sandboxRuntimeKey comment now names windowsSandboxWorkspaceKey, which exists.
On the execution question, which was my other reason for requesting changes. The position has changed materially. Account and group provisioning have now been run on a real elevated session, the description says so precisely, and all three Smoke jobs plus Zero Review are passing, including windows-latest. The logon half — LsaAddAccountRights and LogonUser — remains unexecuted, and the description says that too, in those words.
I am approving with that gap open rather than in spite of it, for two reasons. The whole surface is behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 and off by default, so no existing install changes behaviour. And the disclosure is accurate and specific rather than implied, which is the standard the review protocol asks for. An unrun privileged path that nobody reaches without opting in, declared plainly, is a reasonable posture for foundation work.
On the new material in this delta. The DPAPI wrapping is well-judged. CRYPTPROTECT_UI_FORBIDDEN is the right flag for a path that may run without an interactive desktop, the LocalFree of the DPAPI-allocated output is correctly deferred, and the ciphertext is copied out rather than aliased. I checked the one thing that looked like a documentation mismatch and it was not: the comment says the principal name is the entropy, and windowsSandboxSecretEntropy derives it from the secret's own filename, which is the principal name — so read and write agree by construction, as the comment claims.
Resetting the password when the account already exists is a real bug fix rather than a refinement. NetUserAdd leaves an existing account untouched, so without NetUserSetInfo the stored secret would not have been the account's password, and the failure would have surfaced much later as an unexplained logon failure. Revoking logon rights before deleting the principal, and keeping the restricted token when the network is denied, are both correct orderings.
Two smaller things came back clean and are worth recording. Replacing defer func() { _ = info }() with runtime.KeepAlive is the correct idiom — the deferred closure did not reliably keep the pointed-to Go memory alive across the syscall, and KeepAlive does. And the KeepAlive calls were added for name and comment as well, not only the struct.
Verification. On macOS, go build ./..., go vet ./... and gofmt -l are clean and the suite passes. More usefully for this change, GOOS=windows go vet ./internal/sandbox/... exits 0 and GOOS=windows go test -c compiles, which type-checks the entire Windows surface including the new DPAPI file. That is not execution, but it confirms the Win32 call sites, struct definitions and build tags hold together across the whole addition.
Limitations. I have no Windows host and no elevated session. LsaAddAccountRights, LogonUser, CryptProtectData and NetUserSetInfo are unexecuted by me. I did not check the raw struct layouts against the SDK beyond confirming the existing layout tests still pass.
This does not clear CodeRabbit's outstanding review, and #812 is stacked on this branch, so landing order matters.
Merge is kevin's call per the program gate.
|
Both findings are correct. I checked each against the head before agreeing, and neither is a misreading. Fixed in 6ccf4cf. 1, the account takeover. Confirmed. Ownership is now read back from the comment provisioning stamps before anything is touched, and a name held by an account Zero did not create fails with a typed The irony is not lost on me. I added exactly this guard to the deletion path in the follow-up PR after CodeRabbit raised deleting-by-derived-name, and did not think to look at the adoption path, which is the more dangerous of the two. Deleting the wrong account is loud. Resetting its password and quietly running as it is not. 2, the partial-failure residue. Also confirmed, and your description of why is precise: the rollback is only constructed after Provisioning now unwinds what the run actually did, in reverse, on every failure path, tracking the four things you listed. One deliberate difference from your list, worth stating because it is a judgement rather than an oversight. Cleanup is scoped to what THIS run created. An account that already existed and belongs to Zero is a working principal from an earlier setup, so deleting it because a later run failed would turn a partial failure into a total one. For the pre-existing case the repair is dropping the stored secret instead: this run reset the password, so the secret no longer matches, and absent beats stale because the command path treats a missing secret as "not provisioned" and falls back to the restricted token rather than failing. If you think that is the wrong call I will change it. 3, the unexecuted LogonUser path. Agreed, and I have said so in the description since the start rather than being talked into it. It is the central runtime path and it has not run end to end on an elevated machine. Smart App Control on my box blocks freshly built unsigned binaries, which is exactly the class of binary the gated provisioning test produces. I am not going to claim that as verified, and I do not think opt-in gating substitutes for running it. You also asked for a test with an unrelated existing account on the derived name. Added, driven against
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 146-190: Update the provisioning cleanup flow around the undo
closure and grantWindowsSandboxLogonRights call: compute secretPath immediately
after identity provisioning succeeds, before granting logon rights, and remove
the secretWritten condition so undo removes any resolved secret path on
subsequent failure. Preserve the existing no-op behavior when secretPath is
empty and keep successful secret writing unchanged.
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 389-432: Update setupWindowsSandboxPrincipal to remove the
existing Windows sandbox secret when provisioning succeeds in changing or
reusing an account but setup fails before writeWindowsSandboxSecret. Ensure the
rollback error path deletes the stale .secret file, while preserving the normal
secret write and unrelated provisioning error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24ed1d1c-eaa0-4d51-843c-1e1a7a825854
📒 Files selected for processing (3)
internal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
|
Both findings are the same hole seen from two angles, and you are right: the cleanup I added missed the one window it was written for. Provisioning always sets the account's password, including resetting a pre-existing owned account's, so from the moment it returns the stored secret is already stale. My Fixed in 832f53a: the path is resolved from the account name before anything can fail, and removal is unconditional rather than gated on Worth naming the pattern, since this is twice now on this PR. The takeover fix and this one are both cases where I reasoned correctly about what should happen and then wrote a condition that did not cover the case I was reasoning about. Reading the comment I had written would have told you the intended behaviour; only reading the code shows it did not happen.
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at 832f53a98d74, base 5d1869e, re-confirmed against the live head before posting. My earlier approval at fbe340b was dismissed by the push; this replaces it, and the new work is strictly better.
The two commits since then are both real improvements, not polish.
windowsSandboxUserIsManaged closes a hazard that was live in the version I approved. The account name is derived from a workspace hash rather than discovered, so it can be occupied by an account with nothing to do with Zero — and provisioning would previously have adopted it and reset its password. Reading back the comment stamp before adopting, and refusing with a named error otherwise, is the right shape, and the same predicate is reused on the delete path in #812. Dropping the stored secret when provisioning fails closes the matching half: a secret file that no longer corresponds to any account is worse than none, because it looks provisioned.
One substantive finding, non-blocking, on the adoption gate.
provisionWindowsSandboxIdentity proves ownership using the comment field alone. It does not inspect the adopted account's group memberships. An account named zero-sbx-<hash>, carrying Zero's comment, and also a member of Administrators would pass the gate: Zero resets its password, adds it to the sandbox group, and mints principal tokens for it. The sandboxed child then runs as an administrator, which inverts the property this whole design rests on — your description's argument is that a separate account has no access to the caller's profile by construction, and an adopted account with extra memberships is precisely the case where that stops being true by construction.
I want to be fair about reachability: planting such an account requires administrator rights already, so this is not fresh escalation. It is a persistence and laundering path — something that had admin once leaves a stamped account behind, and Zero thereafter grants it sandbox duty on every run — and it is also the shape a botched or partial earlier provisioning could leave behind on its own. Given that the model's selling point is a boundary that holds by construction, asserting the adopted account's memberships (at minimum, that it is not in Administrators) rather than only its comment would make the claim true rather than nearly true. A comment is a stamp, not a capability check.
What I verified. On macOS: gofmt, go build ./..., go vet ./... clean, suite passing. GOOS=windows go vet ./internal/sandbox/... exits 0 and GOOS=windows go test -c compiles, which type-checks the whole Windows surface including the two new netapi32 procs and the USER_INFO_1 read-back. That is type-checking, not execution.
Limitations, unchanged and still the main thing a reader should weigh. I have no Windows host and no elevated session. NetUserGetInfo, NetApiBufferFree, NetUserSetInfo, LsaAddAccountRights and LogonUser are unexecuted by me. Your description remains accurate about which halves you have run, and that accuracy is why I am comfortable approving with the logon path still unrun: the feature is behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 and off by default, so nothing changes for an existing install.
CodeRabbit's changes-requested from 08:17 is still outstanding and is separate from this.
Merge is kevin's call per the program gate.
Groundwork for closing the Windows half of #662 and #675, where credentialDenyReadPaths is a no-op today. Every Windows backend currently derives its token from the calling user via CreateRestrictedToken, so the sandbox can constrain writes but not reads: a deny ACE that would stop the sandboxed child reading a credential store names the same account Zero runs as, and would lock Zero out too. That is why deny-read is skipped on Windows rather than merely unimplemented. This adds a separate local account per workspace, held in one managed group, so the sandbox has an identity of its own: - provisioning: managed group, stable per-workspace account name inside the 20-character limit, crypto/rand password meeting complexity policy, SID resolution, idempotent so setup re-runs converge - logon rights: grants only SeBatchLogonRight and explicitly denies interactive, network, remote-interactive and service logon, then mints a token with LogonUser pinned to the local machine - ACLs keyed to the principal: denies emitted before allows so carve-outs survive, workspace granted read+write, read roots granted read, protected metadata denied write and materialized - removal: revocation by trustee, so retiring a principal drops every ACE naming it without needing a record of what was granted The inversion is the point. A separate account has no access to the caller's profile at all, so credential stores are unreachable by construction rather than by enumerating deny rules, and the same SID is what a write grant or a firewall rule can be keyed to. Nothing is wired into command execution yet: these paths are additive and no existing behavior changes. See the pull request for the open question about where the principal's password lives.
Wires the principal model into the runner and settles where the account's password lives. The secret is stored under the sandbox home with an explicit, inheritance-protected DACL naming only the invoking user and SYSTEM. The sandbox principal is deliberately absent from it: a principal that could read the file could mint its own token and the identity boundary would be decorative. The ACL is applied to an empty file before the password is written, so the bytes never exist under the config directory's inherited permissions, and PROTECTED drops any inherited ACE outright. At command time the runner asks for a principal token first and uses it in place of the restricted token, because a separate account has reads denied by the filesystem rather than left open the way a same-user restricted token must leave them. The lookup is fail-soft: opt-out, no provisioned account, or no stored secret all report "not available" and the existing restricted-token path runs unchanged. Only a provisioned-but-unusable identity surfaces an error, since that means setup ran and the sandbox is broken rather than absent. The backend stays behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 while the privileged paths are unvalidated, so no existing install changes behaviour.
|
Applied CodeRabbit's ACE type finding in
Nothing under test builds anything but allowed ACEs, so no current outcome changes. I confirmed the guard is live rather than dead code by pointing the comparison at @gnanam1990 @anandh8x apologies, this push dismissed both your approvals. Unavoidable, since a CodeRabbit change request only clears on a push. The delta from The caveat I flagged earlier is unaffected and still stands: the elevated |
|
@coderabbitai review The ACE type finding is addressed in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current PR state =='
git status --short
git rev-parse --short HEAD
git show -s --format='%h %s' HEAD
printf '%s\n' '== Changed files relative to merge base =='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --name-status "$base"...HEAD
printf '%s\n' '== Symbol outlines =='
ast-grep outline internal/sandbox/windows_identity_secret_windows_test.go --items all --match 'windowsSecretACEList' --view expanded || true
ast-grep outline internal/sandbox/windows_identity_secret_windows.go --items all --view expanded || true
ast-grep outline internal/sandbox/windows_identity_runtime_windows.go --items all --view expanded || true
ast-grep outline internal/sandbox/windows_identity_windows.go --items all --view expanded || true
printf '%s\n' '== ACE helper and its callers =='
rg -n -A45 -B12 'func windowsSecretACEList\b|\bwindowsSecretACEList\s*\(' internal/sandbox
printf '%s\n' '== Full current PR diff =='
git diff --find-renames --find-copies "$base"...HEAD -- internal/sandboxLength of output: 126315 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Provisioning and cleanup with line numbers =='
nl -ba internal/sandbox/windows_identity_runtime_windows.go | sed -n '120,280p'
nl -ba internal/sandbox/windows_identity_windows.go | sed -n '380,455p'
printf '%s\n' '== ACL apply/revert implementation and plan types =='
ast-grep outline internal/sandbox/windows_acl_apply_windows.go --items all --view expanded
rg -n -A100 -B20 'func applyWindowsACLPlan\b|func windowsExplicitAccessEntries\b|type WindowsACLPlan\b|type WindowsACLEntry\b|type WritableRoot\b' internal/sandbox
printf '%s\n' '== Callers and tests for provisioning failures =='
rg -n -A12 -B8 '\bprovisionWindowsSandboxIdentity\s*\(|\bprovisionWindowsSandboxPrincipalForSetup\s*\(|\bsetupWindowsSandboxPrincipal\s*\(' internal/sandbox
printf '%s\n' '== Diff for latest ACE fix only =='
git show --stat --oneline dbdbda1
git show --format=fuller --find-renames dbdbda1 -- internal/sandbox/windows_identity_secret_windows_test.goLength of output: 1385 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_range() {
file="$1"
start="$2"
end="$3"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
printf '%s\n' '== Provisioning and cleanup with line numbers =='
show_range internal/sandbox/windows_identity_runtime_windows.go 120 280
show_range internal/sandbox/windows_identity_windows.go 380 455
printf '%s\n' '== ACL apply/revert implementation and plan types =='
ast-grep outline internal/sandbox/windows_acl_apply_windows.go --items all --view expanded || true
rg -n -A100 -B20 'func applyWindowsACLPlan\b|func windowsExplicitAccessEntries\b|type WindowsACLPlan\b|type WindowsACLEntry\b|type WritableRoot\b' internal/sandbox
printf '%s\n' '== Callers and tests for provisioning failures =='
rg -n -A12 -B8 '\bprovisionWindowsSandboxIdentity\s*\(|\bprovisionWindowsSandboxPrincipalForSetup\s*\(|\bsetupWindowsSandboxPrincipal\s*\(' internal/sandbox
printf '%s\n' '== Diff for latest ACE fix only =='
git show --stat --oneline dbdbda1
git show --format=fuller --find-renames dbdbda1 -- internal/sandbox/windows_identity_secret_windows_test.goLength of output: 68712
I found one remaining cleanup bug:
|
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed the latest head dbdbda1. Two issues need fixing before this foundation is safe to merge.
[P1] internal/sandbox/windows_acl_apply_windows.go:226 — WindowsACLAllowWrite grants FILE_GENERIC_WRITE but neither DELETE nor FILE_DELETE_CHILD. That was sufficient for the old same-user token because the caller already had inherited rights, but the new principal has no such access. It can create and modify files yet cannot reliably delete or rename pre-existing workspace files/directories, so normal coding and git operations will fail. The principal write grant needs delete semantics, with an elevated Windows test that creates a file as the caller and deletes or renames it through the principal.
[P2] internal/sandbox/windows_identity_windows.go:424-429 and windows_identity_runtime_windows.go:166-168 — if NetUserAdd creates the account and group attachment or SID resolution then fails, provisionWindowsSandboxIdentity returns created=true with an empty identity. The advertised cleanup calls removeWindowsSandboxIdentity(identity.Username), therefore tries to remove an empty name and strands the newly-created local account. Preserve the derived username on partial failure or perform cleanup inside provisioning, and cover an injected post-creation failure.
… find it Two findings from review, both consequences of the principal being a separate account rather than the calling user. WindowsACLAllowWrite granted FILE_GENERIC_WRITE, which covers creating and modifying but not removing or renaming, and a rename needs delete on the source. Under the old same-user token this was invisible because the caller already held inherited rights on its own tree. A principal inherits nothing, so it could write files it could never delete, which fails ordinary editing and most git operations rather than an edge case. DELETE and FILE_DELETE_CHILD are now part of the grant, matching WindowsACLDenyWrite, which already treats delete as part of write. WRITE_DAC and WRITE_OWNER stay out: they are denied so the principal cannot rewrite its own restrictions. provisionWindowsSandboxIdentity returned a zero identity alongside created=true when group attachment or SID resolution failed after NetUserAdd had already created the account. The caller's rollback deletes by identity.Username, so it was asked to delete the empty string and left the account behind. Group attachment is the case that matters, being both the enforcement boundary and something local policy can refuse. The name now comes back with the error. The four provisioning calls are indirected so the failure paths are reachable in a test. Seaming only the post-creation pair would not have been enough: every step needs an elevated caller, so the test would have stopped at the group check and passed without reaching what it names. Also seeds the empty-secret test with a genuinely empty file. The previous whitespace seed was several bytes, so it never reached the length check and failed later in DPAPI instead, which another test already covers.
|
Both taken, in [P1] delete rights. Correct, and worse than a missing capability: Worth noting the asymmetry that hid it: I could not run your elevated test, so the coverage is at the mask instead: the grant is asserted to carry [P2] rollback. Also correct. The name now comes back with the error from both post-creation paths. The interesting part was making it testable. I seamed all four provisioning calls rather than just the two you named, because every step needs an elevated caller: a test that replaced only the post-creation pair would stop at Injected failure tests cover group attachment and SID resolution, plus a case asserting CodeRabbit's stale-secret finding was already addressed before its review landed: #812 is rebased on this and carries the same fix for the offline-group step, which is the one your #812 note called out. |
|
@coderabbitai review Both findings are addressed in |
|
@kevincodex1 status update rather than a merge request, because these are not ready for you yet and I would rather say so than have you look and find nothing to press. #808 and #812 are the last of my open PRs; everything else merged today. Both are green, and @anandh8x's change requests are addressed in When they do land, order matters: #808 first, then #812. #812's base is The decision I flagged earlier still stands unchanged and is the real reason I am writing. The elevated Landing a foundation with an unexercised path behind an off-by-default opt-in ( anandan's P1 is worth reading as evidence for that caution: the principal could not delete or rename files, because |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== PR head and follow-up commit =='
git rev-parse --short HEAD
git show -s --format='%h %s' HEAD
git merge-base --is-ancestor 3d49e20f HEAD && echo '3d49e20f is contained in HEAD'
printf '%s\n' '== Follow-up commit =='
git show --find-renames --stat --oneline 3d49e20f
git show --find-renames --format=fuller 3d49e20f -- internal/sandbox
printf '%s\n' '== Focused symbol outlines =='
ast-grep outline internal/sandbox/windows_identity_runtime_windows.go --items all --match 'provisionWindowsSandboxPrincipalForSetup|setupWindowsSandboxPrincipal' --view expanded || true
ast-grep outline internal/sandbox/windows_identity_windows.go --items all --match 'provisionWindowsSandboxIdentity|ensureWindowsSandboxUser' --view expanded || true
ast-grep outline internal/sandbox/windows_identity_secret_windows_test.go --items all --match 'windowsSecretACEList' --view expanded || true
printf '%s\n' '== Focused implementations and regression tests =='
rg -n -A120 -B15 \
'func provisionWindowsSandboxPrincipalForSetup\b|func provisionWindowsSandboxIdentity\b|func windowsSecretACEList\b|secretPath|errWindowsSandboxNameCollision|injected|inject|addWindowsSandboxUserToGroup|resolveWindowsSandboxSID' \
internal/sandbox/windows_identity_runtime_windows.go \
internal/sandbox/windows_identity_windows.go \
internal/sandbox/windows_identity_windows_test.go \
internal/sandbox/windows_identity_secret_windows_test.goLength of output: 78009
No remaining finding on these points. |
|
@anandh8x @gnanam1990 these two are the last open PRs on the board, so putting the ask in one place rather than on both. Both are green and both change requests are addressed. The only reviews on either right now are CodeRabbit's, and both of those predate the fixes, so they should clear on the re-run I have triggered. What is missing is human approvals: my own fix pushes dismissed the ones you had already given, so #808 and #812 are each at zero of three. Sorry, that one is on me. To make re-review cheap, what actually changed since each of you last looked: #808
#812
Every fix is mutation-checked in both directions rather than just added, so reverting any one of them fails a named test. Merge order when it comes to it: #808 first, then #812, since #812's base is @gnanam1990 the delta from what you approved before is the ACL grant, the rollback return, and the test seams. No change to the identity model or the network design either of you already signed off. Separately and not blocking your review: the elevated |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Stop deleting a pre-existing principal when ACL setup fails
internal/sandbox/windows_identity_runtime_windows.go:221-228
provisionWindowsSandboxPrincipalForSetupdeliberately avoids deleting an account it did not create (created=false), butsetupWindowsSandboxPrincipalcallsremovePrincipal()on any ACL plan/build/apply error with no such guard. Re-running elevated setup on a machine that already has a working principal and hitting a transient ACL failure therefore deletes the local account, removes the secret, and revokes logon rights. The same unconditional teardown runs from the folded setup rollback when a later step fails (windows_setup_windows.go:56-62,windows_identity_runtime_windows.go:230-239). Please scope teardown to what this run created, matching thecreatedguard already used inprovisionWindowsSandboxPrincipalForSetup. -
[P1] Do not rotate an existing principal's password until the new secret is committed
internal/sandbox/windows_identity_windows.go:435-437
internal/sandbox/windows_identity_runtime_windows.go:154-168
On re-setup of a managed principal,resetWindowsSandboxUserPasswordruns insideprovisionWindowsSandboxIdentitybefore logon-rights grant, secret write, or ACL apply. If any later step fails,undo()removes the on-disk secret and does not delete the pre-existing account (created=false), leaving a live account authenticated by a password nothing on disk knows. Commands then treat the missing secret as "not provisioned" and silently fall back to the restricted-token backend. Please defer password rotation until immediately beforewriteWindowsSandboxSecret, or restore the previous password on rollback. -
[P2] Apply policy
DenyWritepaths in the principal ACL plan
internal/sandbox/windows_identity_runtime_windows.go:215-219
internal/sandbox/windows_identity_acl.go:51-115
The capability-SID setup path emitsWindowsACLDenyWriteACEs for everyPermissionProfile.FileSystem.DenyWriteentry (windows_acl.go:52-63). The principal path only denies write on protected metadata and read-only subpaths inside write roots; it never passes policyDenyWriteintobuildWindowsPrincipalACLPlan. When the runner uses a principal token (windows_command_runner_windows.go:93-100), those policy denies are not enforced at the OS layer and shell children can write where the restricted-token backend would block. Please mirror the capability plan'sDenyWritehandling for principals. -
[P2] Materialize principal
DenyReadtargets the way capability deny-read does
internal/sandbox/windows_identity_acl.go:63-68
internal/sandbox/windows_acl_apply_windows.go:93-97
Capability deny-read entries setMaterialize: trueso absent paths still get a deny ACE before they exist (windows_acl.go:72-75). Principal deny-read entries omitMaterialize, soapplyWindowsACLPlanskips any target that does not exist at setup time. A deny-read path created later never gets a principal deny ACE. Please setMaterialize: trueon principal deny-read entries, matching the capability model. -
[P2] Roll back partial LSA logon-right grants before deleting a new principal
internal/sandbox/windows_identity_logon_windows.go:143-162
internal/sandbox/windows_identity_runtime_windows.go:163-165
grantWindowsSandboxLogonRightsadds rights one at a time and returns on the first failure, butrightsGrantedis only set after the full loop succeeds. If provisioning created a new account and grant fails mid-loop,undo()deletes the account without callingrevokeWindowsSandboxLogonRights, leaving orphaned LSA entries keyed to a SID that no longer resolves. Please revoke whatever rights were granted before account deletion, or track partial grant state in rollback. -
[P2] Reconsider silent downgrade when opt-in is set but the secret is unusable
internal/sandbox/windows_identity_runtime_windows.go:107-112
internal/sandbox/windows_command_runner_windows.go:84-101
This is intentional fail-soft behavior, not an accidental regression: if the local principal account exists but the secret is missing, empty, corrupt, or cannot be DPAPI-decrypted, the runner falls through to the same-user restricted-token path with no stderr warning. That is weaker than principal isolation and unlike the explicit error path for logon failure (:116-120). If fail-soft is the right product choice, please at least warn on stderr whenZERO_WINDOWS_SANDBOX_IDENTITY=1is set and the principal cannot be used, so operators know they are not getting the backend they opted into. -
[P3] Disambiguate workspace hashes before truncating local account names
internal/sandbox/windows_identity_windows.go:151-152
windowsSandboxWorkspaceKeyproduces a 64-character hex digest, butwindowsSandboxUserNamekeeps only the first 11 characters after thezero-sbx-prefix because of the 20-character NetAPI limit. Two workspaces whose digests share that prefix would collide on one local account, secret file, and ACL identity. Low probability on a single host, but please fold more entropy into the surviving name or detect and refuse collisions so distinct workspaces cannot share a principal.
Notes (not raised as merge blockers)
- Network deny vs principal backend: Under default
NetworkDenypolicy, the principal backend is deliberately disabled because WFP filters key on the offline-marker SID (windows_identity_runtime_windows.go:69-76). That is tested and documented scope, not a bug; extending WFP to principal SIDs is follow-on work for #662. - Prior review items on head
3d49e20f: Delete rights onWindowsACLAllowWriteand pre-failuresecretPathderivation for rollback appear addressed on the current head relative toanandh8xand CodeRabbit feedback on older commits.
Six findings from review, all on the elevated setup path. Teardown was not scoped to what the run created. provisionWindowsSandbox PrincipalForSetup was careful never to delete an account it had adopted, and then setupWindowsSandboxPrincipal called removePrincipal on any ACL failure with no such guard. Re-running elevated setup on a working machine and hitting one transient ACL error therefore deleted the local account, its secret and its logon rights. It now returns whether it created the principal and the outer teardown honours it; ACEs are still reverted, since this run applied them. Password rotation moved to immediately before the secret is committed. Resetting an adopted account's password at the top of provisioning meant every later step ran against an account whose password had been replaced with no copy stored. Any failure there left a live account authenticated by a password nothing on disk knew, and since the account pre-existed the rollback correctly declined to delete it, so the command path read the absent secret as "not provisioned" and fell back to the weaker backend for good. The two operations are now adjacent. The rollback also stops removing the secret when this run neither created the account nor rotated it, because that secret still works. Policy DenyWrite now reaches the principal ACL plan. The capability plan has always emitted these; the principal plan denied write only on protected metadata and read-only subpaths, so once the runner used a principal token a policy deny elsewhere was not enforced at all. Principal deny-read entries are materialized, matching the capability plan, so a path created after setup still gets a deny ACE. Logon-right revocation is keyed to the attempt rather than to success. Rights are added one at a time and the grant returns on first failure, so a partial grant left LSA entries behind pointing at a SID that deleting the account then made unresolvable. The ownership comment now carries the full workspace key. The account name holds only 11 characters of the digest, so two workspaces could derive one name and silently share an account, a secret and an ACL identity; a mismatch is now refused. Accounts provisioned before the key was recorded are still adopted. Also warns once on stderr when the opt-in is set and a provisioned principal cannot be used, rather than downgrading in silence.
|
All seven addressed in [P1] teardown scoping. Correct, and the reason it happened is worth stating: I added the [P1] password rotation. Also correct. Rotation moved out of provisioning to sit immediately before Covered by a test asserting provisioning does not rotate an adopted account. Reverting it fails with "provisioning rotated the password; the window this closes lasts until the secret is committed". [P2] policy DenyWrite now reaches [P2] partial LSA grants. Revocation is now keyed to the attempt rather than to the loop completing, so a grant that failed halfway is still unwound before the account is deleted. Revoking a right that was never granted is harmless, which is what makes that safe. [P2] silent downgrade. Fail-soft is the right behaviour, staying quiet about it was not. It now warns once per process on stderr when the opt-in is set and a provisioned principal cannot be used. Once rather than per command, because this sits on the command path and a warning on every tool call gets filtered out by the reader rather than acted on. [P3] name truncation. Taken as exact detection rather than more entropy. The ownership comment carries the full workspace key, so a collision is refused instead of two workspaces silently sharing one account, one secret and one ACL identity. Accounts provisioned before the key was recorded are still adopted. I also threaded the key into the delete path, since proving ownership from the marker alone would have let a collision delete another workspace's account. On your note about prior items being addressed on The |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/windows_identity_windows.go (1)
348-383: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLegacy bare-marker accounts still bypass workspace scoping at
internal/sandbox/windows_identity_windows.go:348-383
windowsSandboxUserIsManagedaccepts the unkeyedwindowsSandboxUserCommentfor anyworkspaceKey, but never rewrites it towindowsSandboxUserCommentFor(workspaceKey). That keeps pre-key accounts reusable across colliding workspace names; migrate the comment here or make the legacy acceptance explicitly scoped elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_windows.go` around lines 348 - 383, The legacy bare marker branch in windowsSandboxUserIsManaged must be workspace-scoped. When comment equals windowsSandboxUserComment, migrate or rewrite the account comment to windowsSandboxUserCommentFor(workspaceKey) before returning managed, or otherwise ensure acceptance is explicitly limited to the intended workspace; preserve the keyed-comment comparison for normal accounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 348-383: The legacy bare marker branch in
windowsSandboxUserIsManaged must be workspace-scoped. When comment equals
windowsSandboxUserComment, migrate or rewrite the account comment to
windowsSandboxUserCommentFor(workspaceKey) before returning managed, or
otherwise ensure acceptance is explicitly limited to the intended workspace;
preserve the keyed-comment comparison for normal accounts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f227da9-7534-49da-94a7-3cac5db69aee
📒 Files selected for processing (8)
internal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_identity_acl.gointernal/sandbox/windows_identity_policy_windows_test.gointernal/sandbox/windows_identity_rollback_windows_test.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_secret_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sandbox/windows_identity_acl.go
- internal/sandbox/windows_identity_runtime_windows.go
- internal/sandbox/windows_identity_secret_windows_test.go
- internal/sandbox/windows_identity_windows_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@anandh8x @jatmn re-review please when you have a moment. You two are the only thing left on this one now: CodeRabbit came back COMMENTED rather than blocking on the current head, and 9 of 9 are green. Both your change requests sit on commits older than the fix. Everything you each raised is addressed in Shortest version of the delta since you last looked:
Every one of those fails a named test if reverted, so if something looks unverified it is more likely I mislabelled it than that it is untested. Say so and I will point at the specific test. The |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head d67267f. The two issues from my previous review are fixed: the principal write grant now includes DELETE and FILE_DELETE_CHILD, and post-creation failures return the derived username so rollback can remove the account. The new tests bind both fixes.
Two issues remain before this foundation is ready.
[P1] internal/sandbox/windows_identity_runtime_windows.go:195-202 — the new rollback still revokes every LSA right from an adopted principal whenever any later step fails. rightsAttempted is set for both newly-created and pre-existing accounts, while revokeWindowsSandboxLogonRights uses AllRights=1. On a re-run, a transient grant, secret-path, or secret-write failure therefore keeps the adopted account but removes its existing SeBatchLogonRight and deny-logon rights, destroying the working principal this commit is specifically trying to preserve. Only revoke all rights when this run created the account. For an adopted managed account, leaving a partial set of the desired rights is safer than deleting all pre-existing rights. Add an injected adopted-account failure test that asserts revocation is not called.
[P2] internal/sandbox/windows_identity_secret_windows.go:186-193 — the runtime warning and fail-soft contract say a missing or unreadable secret falls back, but ERROR_ACCESS_DENIED still becomes a hard error. This occurs when setup is elevated with another administrative account through runas or over-the-shoulder UAC: the protected DACL belongs to that account, so the normal user cannot read it and every sandbox command fails. Map permission-denied reads to errWindowsSandboxIdentityUnavailable so the existing warning and restricted-token fallback run. I would not swallow permission errors from removeWindowsSandboxSecret: reporting incomplete teardown is safer than deleting an account while its secret remains.
Second round of review findings, both on the elevated setup path. The rollback revoked logon rights whenever they had been attempted, without regard to whether this run created the account. revokeWindowsSandboxLogonRights passes AllRights, which drops every right the account holds and deletes its LSA object outright. On an adopted principal that is not a rollback but destruction: a transient grant, secret-path or secret-write failure during a re-run stripped the SeBatchLogonRight and deny-logon rights an earlier setup had established, leaving exactly the broken-but-present principal this path exists to avoid. Revocation is now scoped to accounts this run created. The rights granted to an adopted account are the ones it is supposed to hold, so leaving them is the safe direction. A secret the current user cannot read now falls back instead of failing the command. The secret's DACL names whoever ran setup, so an operator who elevated with a separate administrative account, through runas or an over-the-shoulder UAC prompt, leaves a secret their ordinary account cannot open. That is the documented fail-soft case, and treating it as a hard error made every sandboxed command fail on a machine that was merely set up by a different admin. Permission errors from the removal path are deliberately still reported, since incomplete teardown is worth knowing about. Both are covered by injected-failure tests and fail if the guard is removed. The secret read is seamed to inject the permission error, because producing a real ERROR_ACCESS_DENIED needs DACL surgery and would test the platform rather than the mapping.
|
Both fixed in [P1] rights revocation. You are right and I had not looked at what Revocation is now scoped to Test is the injected adopted-account failure you asked for, with the created case alongside it so the guard cannot be satisfied by never revoking at all. Reverting to the unconditional form fails it with "rights were revoked for an adopted account; AllRights drops its pre-existing rights and deletes the LSA object". [P2] permission-denied secret read. Also taken. I left One note on how that is tested. The read is seamed and the permission error injected, rather than manufactured. I tried the cheap ways first and neither works on Windows: a 0000 file is still readable, and reading a directory reports "Incorrect function" rather than access denied. A real |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before I'm comfortable merging this.
Findings
-
[P1] Grant the principal write access to sandbox runtime directories added at command time
internal/sandbox/windows_identity_runtime_windows.go:267-300
internal/sandbox/runtime_state.go:39-53, 230-241
internal/sandbox/windows_command_runner_windows.go:93-100
internal/sandbox/windows_runner.go:330-345
Each sandboxed command callspermissionProfileWithRuntime, which appendsruntimeState.Root(under%LOCALAPPDATA%\zero\runtime\…, not inside the workspace) toWriteRootsand redirectsHOME,GOCACHE,npm_config_cache, and similar env vars into that tree. Elevated setup applies principal ACLs from the profile at setup time and never sees that runtime root. On the default restricted-token path the child still runs as the caller, so it can write the user cache without an extra ACE. A principal is a separate local account with no inherited rights there, and oncewindowsSandboxPrincipalTokensucceeds the runner does not apply capability ACLs for that command (ensureWindowsUnelevatedSetuponly runs for the unelevated tier, and those ACEs name capability SIDs anyway). I expect opt-in principal commands to hitACCESS_DENIEDon normal npm/go/cargo/pip cache writes. Please grant the principal write access to the live runtime root on the command path (or extend elevated setup so the runtime tree is covered before commands run). -
[P2] Verify workspace ownership at command-time lookup, not only during setup
internal/sandbox/windows_identity_windows.go:517-523
internal/sandbox/windows_identity_runtime_windows.go:93-132
Setup refuses a truncated-name collision by checking the account comment against the full workspace key (windowsSandboxUserIsManaged,errWindowsSandboxNameCollision).lookupWindowsSandboxIdentityonly mapswindowsSandboxWorkspaceKey→windowsSandboxUserName→ SID and never reads the comment. With real workspace keys I think accidental collision is rare: the key is a full SHA-256 hex digest and the account name keeps only 11 hex characters, so we're looking at roughly 2⁻⁴⁴ per pair. The hole is still real—workspace B can fail setup yet bind to workspace A's principal, secret file, and ACL identity when the suffix collides (your test keysaaaaaaaaaaaabbbbbbbbandaaaaaaaaaaaaccccccccforce this). I'd like the stored comment verified against the caller's workspace key before lookup returns a principal, matching what provisioning already does. -
[P2] Materialize git carveout deny-write ACEs for principals
internal/sandbox/windows_identity_acl.go:98-103
internal/sandbox/windows_acl_apply_windows.go:89-97
internal/sandbox/profile.go:65-69
Principal deny-write entries forReadOnlySubpaths(.git/config,.git/hooksfromgitMetadataWriteCarveouts) omitMaterialize: true, while protected metadata and policy deny paths set it.applyWindowsACLPlansilently skips absent targets withoutMaterialize. On a fresh workspace where.gitdoes not exist yet, setup completes without those carveout ACEs; once git creates the paths, the principal still has inherited workspace write access and can modify git config or install hooks. The capability plan uses the same non-materialized pattern for subpath denies, but there the restricted token runs as the caller and the carveout is less load-bearing; for a separate principal account I think this gap is newly user-visible. Please setMaterialize: trueon those subpath denies, matching policy deny-read handling. -
[P2] Fix or replace the gated provisioning round-trip test
internal/sandbox/windows_identity_windows_test.go:202-264
internal/sandbox/windows_identity_windows.go:455-470
TestProvisionWindowsSandboxIdentityRoundTripcallsprovisionWindowsSandboxIdentitytwice, thenlogonWindowsSandboxPrincipal(again.Username, secondPassword). For an existing account,provisionWindowsSandboxIdentitydeliberately does not rotate the password; rotation is deferred toprovisionWindowsSandboxPrincipalForSetup. The second returned password is a fresh random string the account does not hold, so I believe the logon step fails when the test is enabled. CI skips the test today, so this is latent rather than blocking CI. The stale comment at lines 212-214 also says provisioning resets the password on leftover accounts. Please align the test with the deferred-rotation contract (exerciseprovisionWindowsSandboxPrincipalForSetup, or stop asserting logon with the second low-level return value). -
[P2] Document or align setup-time and command-time opt-in for
ZERO_WINDOWS_SANDBOX_IDENTITY
internal/sandbox/windows_setup_windows.go:43
internal/sandbox/windows_setup.go:151-158
internal/sandbox/windows_identity_runtime_windows.go:35-39
Elevatedzero sandbox setupcallswindowsSandboxIdentityEnabled(config.commandConfig().Env)with noEnvfield populated, so it reads only the process environment. Sandboxed commands pass--env-json, andwindowsSandboxIdentityEnabledtreats an explicit map entry as authoritative (TestWindowsSandboxIdentityEnvOverridesProcess). Setup can skip principal provisioning while commands opt in through child env, or provision a principal while commands opt out. That's avoidable when setup and commands share the same shell export, but the two paths can diverge silently. Please document that setup must export the variable, or thread the intended value through setup args. -
[P2] Revoke stale principal ACEs when the profile shrinks or setup is re-run
internal/sandbox/windows_identity_runtime_windows.go:267-300
internal/sandbox/windows_identity_acl.go:138-159
Principal ACL application merges new explicit entries into existing DACLs (ACLFromEntries(accessEntries, oldDACL)). Re-setup only applies the current plan;windowsPrincipalRevokePlanis implemented and tested but I don't see a production caller. Shrinking write/read roots or policy deny lists therefore leaves prior principal ACEs on disk. This matters mainly when profiles shrink or drift across versions; steady-state re-runs that only refresh the same grants are less affected. I'd like trustee-revocation (or snapshot-revert) before applying a new plan, or a call towindowsPrincipalRevokePlanover the paths that were previously granted. -
[P3] Make
TestWindowsSandboxIdentityGatinghermetic for the absent-env case
internal/sandbox/windows_identity_runtime_windows_test.go:11-28
The"absent"case passes an emptyenvmap, sowindowsSandboxIdentityEnabledfalls through toos.Getenv. On a machine where the test process already hasZERO_WINDOWS_SANDBOX_IDENTITY=1, the absent case fails. Please clear the process variable (or pass an explicit unset sentinel) before that subtest.
Claim drift (not merge blockers for me)
These are wording or scope notes, not bugs I'm asking you to fix in this PR unless you want to.
-
#662 / default network-deny: I see the principal backend is intentionally disabled when
Network.Mode == NetworkDeny(windowsSandboxPrincipalEligible), and default policy is network-deny. With onlyZERO_WINDOWS_SANDBOX_IDENTITY=1, commands still use the restricted same-user token andcredentialDenyReadPathsremains a Windows no-op. You document the tradeoff in the body; I'd still narrow the title/body so it doesn't read like #662 is closed for the default install. Principal read confinement needs elevated setup plus a network-allow command profile until WFP is keyed to the principal SID. -
“No existing install changes behaviour”: Principal provisioning is gated behind the env var, but the shared
WindowsACLAllowWritechange (addingDELETEandFILE_DELETE_CHILD) applies to capability-SID ACL plans on every elevated setup re-run, not only when the opt-in is set. That's a real behaviour change for installs that re-run setup without the env var. Worth a line in the PR notes; it's separate from principal logic.
Where I stand on prior review
My changes-requested review at 3d49e20 asked you to stop deleting adopted principals on ACL failure, defer password rotation until secret commit, carry policy DenyWrite into the principal plan, materialize principal DenyRead, and roll back partial LSA grants safely. At head a1a0edc those look addressed: removePrincipal is gated on created, rotation happens immediately before writeWindowsSandboxSecret, policy deny-write and materialized deny-read are in buildWindowsPrincipalACLPlan, and undo() revokes LSA rights only when created is true (TestSetupRollbackRevokesRightsOnlyForCreatedPrincipals).
anandh8x's changes-requested review at d67267f asked for delete bits on the principal write grant and to stop revoking adopted principals' logon rights on rollback. I see DELETE | FILE_DELETE_CHILD on WindowsACLAllowWrite and the rightsAttempted && created guard at head; those two look fixed to me as well.
CodeRabbit's open changes-requested thread looks largely stale relative to head (password rotation timing, permission-denied secret reads, adopted-account rollback). The ACE-type check in windows_identity_secret_windows_test.go now validates AceType before decoding.
gnanam approved at 832f53a and noted a non-blocking follow-up to assert adopted accounts are not in Administrators; I still don't see that membership check.
Related work
Open PR #812 (feat/windows-sandbox-offline-online) extends this identity model with separate offline/online principals; I treat that as follow-on work, not a duplicate of this foundation PR.
4 participants
Opt-in behind
ZERO_WINDOWS_SANDBOX_IDENTITY=1. The provisioning half has now been run on a real elevated session; the logon half has not, and that is called out below.Why
credentialDenyReadPathsopens withif runtime.GOOS == "windows" { return nil }, so on Windows no credential path is protected (#662, and the Windows half of #675). That is not an oversight and not a one-line fix.Every Windows backend derives its token from the CALLING user via
CreateRestrictedToken. A deny-read ACE that would stop the sandboxed child reading~/.awsnames the same account Zero itself runs as, so it would lock Zero out too. The one existing escape hatch is costly: the runner dropsWRITE_RESTRICTEDwhenever any DenyRead path is configured, because the kernel skips restricted-SID deny ACEs for reads under that flag, and a fully restricted token then cannot open executables. That is the same wall #640 hit.What this does
Gives the sandbox an identity of its own: a separate local account per workspace, in one managed group.
The inversion is the point. A separate account has no access to the caller's profile at all, so credential stores are unreachable by construction rather than by enumerating deny rules. The interesting direction becomes what to GRANT, and the same SID is what a write grant or a firewall rule keys to.
SeBatchLogonRight, and explicitly denies interactive, network, remote-interactive and service logon, so the account cannot be signed into even if its password leaked.LogonUseris pinned to"."so a same-named domain account is never picked up.CryptProtectData, since an ACL only binds while the filesystem is the one being asked and a backup or a mounted image would otherwise give it up in the clear. The principal name is the entropy, so a blob copied onto another principal's path fails to decrypt rather than authenticating the wrong account.Gated behind
ZERO_WINDOWS_SANDBOX_IDENTITY=1, so no existing install changes behaviour.Verification, and what is not verified
gofmt,go vet,go build ./...clean; builds for linux, darwin and windows. 29 tests, all passing when I ran them, covering name derivation and truncation, password complexity, "already exists" handling, the raw Win32 struct layouts, LSA byte-vs-rune lengths, deny-before-allow ordering, trustee scoping, root grants, metadata materialization, revocation, secret round-trip and overwrite, path traversal, and idempotent removal.Two of those matter most and do real work rather than asserting intent: one reads the stored secret's DACL back and fails if any trustee other than the owner and SYSTEM appears, and another asserts
SE_DACL_PROTECTEDso an inherited ACE cannot reach it.One deliberate restriction. Network denial is enforced by WFP filters keyed to the offline-marker SID. The restricted token carries that SID; a token from
LogonUsercannot, because it names the account rather than a synthetic capability SID. A principal would therefore have left those block filters matching nothing, anddenyis the default mode. So the principal stands down whenever the network is denied and the restricted-token path runs instead, which means this backend currently engages only for network-allowed commands. Trading network denial for read confinement would have been the wrong way round. Keying the filters to the principal's own SID is the follow-up that lifts the restriction.Honest caveats:
NetUserAdd,LsaAddAccountRights,NetUserDelandLogonUserall need administrator rights. They compile and are layout-checked, but nobody has run them. The provisioning round-trip test is gated behindZERO_WINDOWS_IDENTITY_PROVISION_TEST=1plus an elevation check. Account and group creation have since been confirmed on a real elevated session; the logon path has not.TestGrantLogonRightsAndMintPrincipalTokenhas not run to completion: Smart App Control on this machine blocks freshly built unsigned binaries, so it needs a clean elevated box. Everything that does not require elevation runs here, including the secret round-trip, which asserts the password does not appear verbatim in the stored bytes.Worth deciding before this leaves draft
Creating real local accounts is user-visible in a way the current sandbox is not: AV and EDR commonly flag
NetUserAdd, enterprise policy often blocks local account creation, and the accounts appear innet userand Settings. None of that blocks the design, but it should be a deliberate call rather than a surprise in a merged PR.Summary by CodeRabbit