feat(sandbox): Windows sandbox principals (foundation for #662, does not close it) - #808
feat(sandbox): Windows sandbox principals (foundation for #662, does not close it)#808Vasanthdev2004 wants to merge 39 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
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
|
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.
832f53a to
99fefdc
Compare
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Review at 99fefdc
PR #808 — Windows sandbox principals (foundation for #662). 14 files, +2559, 12 commits, all new *_windows.go files (build-constrained) except windows_identity_acl.go which is pure-Go ACL-plan logic that compiles on all platforms. Opt-in behind ZERO_WINDOWS_SANDBOX_IDENTITY=1.
Verdict: approve. The design is sound, the fail-soft contract is right, and the honest caveats are the right ones.
What this does
Gives the sandbox its own identity on Windows: a separate local account per workspace in one managed group. This inverts the read-confinement problem — instead of trying to deny the caller's own account (which locks Zero out too), a separate account has no access to the caller's profile by construction, so credential stores are unreachable without enumerating deny rules.
What's good
- The inversion is the right design. Every other Windows backend derives its token from the calling user via
CreateRestrictedToken, which is whycredentialDenyReadPathsis a no-op on Windows. A separate account makes "what to GRANT" the interesting question instead of "what to DENY," and the same SID keys write grants and firewall rules. - Fail-soft contract is correct. No provisioned account, no stored secret, or opt-in off →
ok=false, nil error, restricted-token backend runs unchanged. Only a provisioned-but-unusable identity surfaces an error (broken sandbox, not absent sandbox). The runner integration (windows_command_runner_windows.go) is a clean 25-line addition that tries the principal first and falls back. - Network-denial tradeoff is honest. A principal token from
LogonUsercan't carry the offline-marker SID that WFP filters key on, so the principal stands down when the network is denied and the restricted-token path runs instead. The PR explicitly says "trading network denial for read confinement would have been the wrong way round." Keying filters to the principal's own SID is the named follow-up. - Provisioning is idempotent. "Already exists" statuses are success. Re-running
zero sandbox setupconverges instead of accumulating accounts. Password is reset on re-provisioning so the stored secret stays in step with the account. - Squat protection.
windowsSandboxUserIsManagedreads back the comment stamp before adopting an existing account. Refuses with a named error (errWindowsSandboxNameCollision) if the name is taken by a non-Zero account. This closes the "reset a stranger's password" hazard. - Secret storage is layered. DACL naming only the invoking user + SYSTEM, applied to an empty file before the password is written (bytes never exist under inherited permissions),
SE_DACL_PROTECTEDso inherited ACEs can't reach it, plus DPAPI (CryptProtectData) encryption with the principal name as entropy so a blob copied to another path fails to decrypt. The testTestStoredSecretDACLNamesOnlyOwnerAndSystemreads the DACL back and fails if any other trustee appears; another assertsSE_DACL_PROTECTED. - ACL plan is deny-before-allow. Carve-outs survive Windows DACL evaluation order. Trustee-keyed revocation drops every ACE naming the principal without needing a record of what was granted — the cleanup path the capability-SID model lacks.
- Rollback is thorough.
provisionWindowsSandboxPrincipalForSetupcomputessecretPathearly (before anything can fail), the undo closure removes the secret unconditionally ("provisioning has already replaced the account's password by the time any of this can fail, so whatever is on disk cannot authenticate"), andsetupWindowsSandboxPrincipalcallsremovePrincipal()on ACL-plan failure, which removes secret → logon rights → account in that order. - Logon rights are least-privilege. Only
SeBatchLogonRightgranted; interactive, network, remote-interactive, and service logon explicitly denied.LogonUserpinned to"."so a same-named domain account is never picked up. - Platform separation is clean.
windows_identity_acl.go(plan logic, no build tag, compiles everywhere, testable on Linux) vs*_windows.go(syscall execution, build-constrained). Cross-compile forGOOS=windowsclean;GOOS=windows go test -ctype-checks the full Windows surface includingnetapi32procs andUSER_INFO_1layout.
Verification performed
GOOS=windows go vet ./internal/sandbox/...— cleanGOOS=windows go test -c— compiles (type-checks all Windows-specific code)go build ./internal/sandbox/...(Linux) — cleango test ./internal/sandbox/(Linux, from non-/tmppath) — pass, all 14 tests greengo vet ./internal/sandbox/...— clean
CodeRabbit's findings are addressed
CodeRabbit's latest CHANGES_REQUESTED (08:17Z) asked for (1) computing secretPath before granting logon rights and removing the secretWritten condition, and (2) removing the stale .secret file when provisioning succeeds but setup fails before writeWindowsSandboxSecret. Both are addressed by commits 99fefdc and 52f843a (pushed 09:46Z, after the review). The undo closure now computes secretPath early and removes it unconditionally; setupWindowsSandboxPrincipal's rollback calls removePrincipal() which removes the secret first.
gnanam's non-blocking finding (acknowledged, not blocking)
gnanam's APPROVED review notes that the adoption gate (windowsSandboxUserIsManaged) checks the comment field alone, not the account's group memberships. An account named zero-sbx-<hash> with Zero's comment but also in Administrators would pass the gate. gnanam correctly frames this as a persistence/laundering path (not fresh escalation, since planting requires admin already). The fix — asserting the adopted account is not in Administrators — is a reasonable follow-up but not a blocker given the opt-in gate and the admin prerequisite for exploitation.
Honest caveats (from the PR description, still accurate)
- The logon half is unproven.
NetUserAdd,LsaAddAccountRights,LogonUserneed elevation; they compile and are layout-checked but haven't run to completion (Smart App Control blocked the test binary). The provisioning round-trip test is gated behindZERO_WINDOWS_IDENTITY_PROVISION_TEST=1plus an elevation check. - Creating real local accounts is user-visible. AV/EDR commonly flag
NetUserAdd; enterprise policy often blocks local account creation; accounts appear innet userand Settings. The opt-in gate makes this a deliberate call.
These are the right caveats for a foundation PR. The feature is off by default; nothing changes for an existing install.
Verdict
Approve. The design inverts the Windows read-confinement problem correctly, the fail-soft contract is sound, the rollback paths are thorough, and the honest caveats are the right ones. gnanam's non-blocking finding (membership check on adoption) is worth a follow-up. CodeRabbit's two actionable findings are addressed by the latest commits. Ready for kevin to merge.
The principal branch handed the raw LogonUser token to CreateProcessAsUser. That token is a full token for the account, so the sandboxed child kept every write its ambient memberships grant. The ACL plan can add grants and denies at named paths, but it cannot revoke what BUILTIN\Users, Authenticated Users or NT AUTHORITY\BATCH already allow elsewhere — so an opted-in command whose profile permitted writes only to the workspace and runtime roots could still write C:\Users\Public\Documents, which grants BATCH modify and which a batch logon therefore satisfies. The principal now gets its own identity AND the restricted token, not one or the other: reads stay confined by its ACEs, writes by the restricted-SID check. The principal's own SID joins the capability SIDs deliberately. applyWindowsPrincipalACLs grants the workspace to identity.SID rather than to a capability SID, so omitting it would leave the workspace grant matching nothing in the restricted list — a jail that locks out the inmate and no one else. The SID is read back from the token itself rather than threaded through the call, so it cannot drift from the identity actually running. Not fixed here, and separate from this finding: worldSID is unconditionally in the restricted-SID list, so any path whose DACL grants Everyone still satisfies the restricted check on both this path and the pre-existing fallback. That predates the principal work and is raised with the maintainers separately. Reported by jatmn on #808.
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] Revoke principal ACEs from roots removed by a policy change
internal/sandbox/windows_identity_runtime_windows.go:521
Re-setup buildsplanfrom the replacement profile and revokes the trustee only fromwindowsACLPlanPaths(plan). If a prior setup granted an extra write/read root and that root is later removed, it is absent from this plan and its inherited principal-SID ACE survives. The runner deliberately adds that SID to the restricted-token SID set, so the old grant continues to satisfy the write jail after the policy was narrowed. Persist/recover the previously applied target paths (and use their union with the new plan for setup and teardown) before accepting the updated marker; the new stale-ACE test currently supplies the old paths by hand rather than exercising this production path. -
[P1] Make materialization and rollback safe against replacement races
internal/sandbox/windows_acl_apply_windows.go:369
makeWindowsACLDirChainNoFollowcloses the checked handle before its pathnameos.Mkdir, and the file path similarly verifies the parent beforeos.OpenFileat line 322. A workspace owner can replace that parent with a junction in this interval, so elevated setup creates the next component outside the workspace before a later check notices. The failure cleanup also uses pathnameRemoveAll, which can follow the swapped junction and recursively delete its external target. Create relative to retained no-follow handles (and bind cleanup to those handles) rather than checking one pathname and creating/deleting through it later. -
[P1] Preserve the Git carve-outs when
.gitis replaced
internal/sandbox/windows_acl_apply_windows.go:257
The new inheritableDELETEgrant applies to the workspace root, while the deny ACEs are attached only to.git/configand.git/hooks. The principal can rename the whole.gitdirectory, create a replacement, and then write a fresh config/hooks tree that inherited the root allow but has no carve-out denies. That bypasses the protection forcredential.helper,core.hooksPath, and hooks; the author’s current comment acknowledges this route. Deny deletion/renaming of.gititself or otherwise ensure replacement metadata receives the deny entries, with an end-to-end ACL test. -
[P2] Carry the explicit principal opt-in through the elevated setup protocol
internal/sandbox/windows_setup.go:20
Commands honor an explicitZERO_WINDOWS_SANDBOX_IDENTITYfrom their serialized environment, but setup args/config carry no environment andcommandConfigsuppliesEnv == nil. The elevated helper therefore consults only its own process environment; UAC/runas can omit the variable, leaving setup to write the normal marker without provisioning a principal while a later command believes it opted in and silently falls back to the weaker same-user token. Serialize the intended opt-in in setup args/config, or reject/report a mismatch instead of treating it as a successful setup.
Elevated setup and the commands that later use a principal each read ZERO_WINDOWS_SANDBOX_IDENTITY from their own process environment, and setup runs in a separate, UAC-elevated process whose environment is not the caller's. commandConfig() returned a nil Env, so the gate in windows_setup_windows.go fell through to os.Getenv in the elevated helper. The two halves could disagree and nothing detected it. Direction B is the dangerous one: command opted in, setup did not. Marker validation passed (the marker recorded nothing about the opt-in), the principal lookup then declined with a nil error, and the command ran on the same-user restricted token — which by its own comment does not confine reads — while the operator believed an account boundary was isolating it. No warning fired. Direction A left an orphaned account holding batch-logon rights and workspace ACEs that teardown never retires, because teardown sits inside the same opt-in branch. The opt-in is now resolved in the shell the user typed `zero sandbox setup` into and serialized across the UAC boundary as --sandbox-principal 0|1. The marker records it (schema 4 -> 5) and validation refuses on mismatch, before the ACL and network checks. Both directions refuse rather than fall back silently: an unreadable value is rejected outright, since guessing "off" would provision a weaker sandbox than asked for and report success. PrincipalOptIn is a *bool, not a bool. Unset means "consult the environment" rather than "opted out", so the existing smoke-test callers that do not set it keep working instead of serializing --sandbox-principal 0 while the command half still consults os.Getenv. Dropped the deny-mode fallback warning added alongside this. Announcing it looked right, since deny is the default mode and an opted-in operator therefore never gets a principal for ordinary commands. But this runner is re-exec'd per command, so its sync.Once is once per COMMAND: the notice would print on nearly every tool call, and noise that repeats gets filtered rather than acted on. It is also not actionable per command. `zero doctor` carries the opt-in now and is the right surface for a standing configuration fact. The deny-mode behaviour stays pinned by TestPrincipalBackendDefersToRestrictedTokenWhenNetworkDenied. Reported by jatmn on #808.
applyWindowsPrincipalACLs revoked the trustee only from the paths of the plan it was about to apply. A root that LEFT the policy is by definition absent from that plan, so its ACE survived the re-setup marker validation forces and the principal kept write access the current policy no longer grants -- the sandbox widened as a result of being tightened. Teardown repeated the same current-plan-only calculation, so retiring the principal cleaned every path except that one, and then deleted the account, leaving the ACE naming a SID nothing could resolve. Nothing on Windows can answer "which paths hold an ACE for this SID" without walking every volume, so the grants are now written down as they are made: a per-principal record beside the secret, keyed the same way because one sandbox home serves every workspace on the machine. Setup revokes over the union of the recorded paths and the new plan's; teardown revokes over the same union. The record is written as that union BEFORE any DACL changes and narrowed to the granted set after, so a crash in between leaves a superset rather than a record missing paths the run granted. A superset is the safe direction: revoking a path that holds no ACE for the trustee is a no-op. The interesting case is a principal from an earlier setup with no record. Proceeding with an empty prior set would be the fail-open, on the one path where the prior set is not empty but unenumerable. Setup retires that account before provisioning instead: Windows never reuses a deleted local account's RID, so whatever ACEs cannot be found end up naming a principal that no longer exists, and the SID minted next is one no DACL on the machine can already carry. It needs no new operator action, which matters -- there is no `zero sandbox teardown` to send anyone to. Every guard is mutation-checked. Reverting the union to the new plan's paths fails the end-to-end test against real DACLs with "the principal kept its grant on a root the narrowed policy removed"; moving the record after the grant, dropping the retirement, reverting teardown, and trusting an unknown schema each turn a different test red. Two existing tests move to the new applyWindowsPrincipalACLs signature.
|
@jatmn last one from your review is in — You were right that the marker couldn't help here. It builds from the capability-SID plan and has no principal SID in scope, and it's also used for validation, where minting a principal to compute its paths would be actively wrong. So the grants go in a small per-principal record beside the secret instead, keyed the same way — one sandbox home serves every workspace, and a single shared file would have let one workspace's setup overwrite another's and reproduce the bug one level up. Setup revokes over the union of the recorded paths and the new plan's. Teardown does the same, which closes the other half you flagged: it used to clear every path except the one that had left the policy, then delete the account, stranding that ACE on a SID nothing could resolve. The part I want you to push on is the missing-record case. Proceeding with an empty prior set there is the fail-open — it's the one path where the prior set isn't empty, it's unenumerable. Setup retires the account before provisioning instead, on the grounds that Windows never reuses a deleted local account's RID, so any ACE we can't find ends up naming a principal that doesn't exist and the next SID is one no DACL here can already carry. I went that way rather than refusing because there's no Also written down: the record is the union before any DACL changes and narrowed after, so a crash in between leaves a superset rather than a record missing paths the run granted. Mutation-checked rather than asserted. Reverting the union to the new plan's paths fails the end-to-end test against real DACLs with One heads-up for #812: it adds That's 5 of 5 from your review. The World-SID gap is still yours to call. |
The two new record tests failed on the Windows runner and passed everywhere else. The plan builder runs every root through normalizeProfilePath, whose EvalSymlinks expands the 8.3 short name GitHub's runners hand out for TEMP, so the record held C:\Users\runneradmin\... while t.TempDir() had returned C:\Users\RUNNER~1\... and an EqualFold on the raw spellings called two spellings of one directory different paths. Production was never affected: the recorded paths and the newly planned paths both go through that same normalization, so setup and teardown agree with each other. This was only the test being naive about what "same path" means on Windows, and a developer whose volume has 8.3 name generation disabled cannot reproduce it — which is how it shipped. The comparison now normalizes both sides through normalizeProfilePath and keys them with windowsCapabilityPathKey, which is what the ACL plans themselves use. Re-checked against the mutation that matters: reverting the revocation to the new plan's paths alone still fails TestReSetupRevokesARootTheNarrowedPolicyDropped, so the looser-looking comparison has not defanged the assertion.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed the current head c3bfbc1. Two blocking Windows security gaps remain.
P1 — internal/sandbox/windows_acl_apply_windows.go:322 and 369-374: materialization verifies an ancestor, closes that handle, then creates through the pathname with os.Mkdir or os.OpenFile. A workspace owner can replace the parent with a junction in that interval, causing the elevated setup to create outside the workspace before the later verification detects it. Cleanup is also pathname based. Create relative to retained no-follow directory handles and bind cleanup to those handles.
P1 — internal/sandbox/windows_identity_acl.go:110-139 and internal/sandbox/windows_acl_apply_windows.go:234-257: the write-root ACE inherits DELETE while the Git denies cover only .git/config and .git/hooks. The principal can rename the whole .git directory, recreate it, and create replacement config and hooks paths that inherit the allow without the carve-out denies. Protect replacement of .git itself and add an end-to-end ACL regression test.
The current Windows package cross-compiles and passes Windows vet, and CI is green. Those checks do not exercise these adversarial races and replacement paths.
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] Close the materialization race between verification and creation
internal/sandbox/windows_acl_apply_windows.go:315-377
makeWindowsACLDirChainNoFollowopens a component withFILE_FLAG_OPEN_REPARSE_POINT, verifies it, closes the handle, and only then calls pathname-basedos.Mkdir/os.OpenFile. A workspace owner can replace the next ancestor with a junction in that window, so elevated setup creates the missing component outside the approved tree before the post-create check fires.TestMaterializeRefusesAncestorJunctionBeforeCreatingonly covers a junction that already exists at the start of materialization, not a swap between verify and create. Create each new component relative to a retained no-follow parent handle (and bind failure cleanup to that handle) instead of checking one path and creating through it later. -
[P1] Bind materialization rollback to handles, not pathnames
internal/sandbox/windows_acl_apply_windows.go:108-118,283-285
When materialization fails after a partial create, or whenrollbackWindowsACLSnapshotsremoves a freshly materialized target, cleanup usesos.RemoveAll(snapshot.Path)on the original pathname. If an ancestor was swapped to a junction after the object was created, that cleanup can recurse outside the workspace and delete unrelated trees. Use the same no-follow handle strategy as the apply path so rollback cannot follow a post-hoc reparse point. -
[P1] Block
.gitreplacement from bypassing the git carve-out denies
internal/sandbox/windows_identity_acl.go:110-139
internal/sandbox/windows_acl_apply_windows.go:234-257
The principal write grant on the workspace root is inheritable and now includesDELETE, while deny-write ACEs are attached only to.git/configand.git/hooks. A sandboxed principal can rename the entire.gitdirectory, recreate it, and then create freshconfig/hookspaths that inherit the workspace allow without the carve-out denies — restoring control ofcredential.helper,core.hooksPath, and hooks. The comments correctly refuseFILE_DELETE_CHILDon the parent grant, but that does not stop renaming the.gitdirectory itself. Deny deletion/replacement of.git(without blocking the object writes git needs underneath) or otherwise ensure replacement metadata paths receive the deny entries, and add an end-to-end ACL regression test for the rename path. -
[P2] Retire a provisioned principal when setup is re-run with the opt-in off
internal/sandbox/windows_setup_windows.go:38-64
internal/sandbox/windows_setup.go:379-387
Marker validation tells operators to "re-runzero sandbox setup… without"ZERO_WINDOWS_SANDBOX_IDENTITYto retire a principal, butrunWindowsSandboxSetuponly callssetupWindowsSandboxPrincipalwhen the opt-in is true. Re-running elevated setup with--sandbox-principal 0rewrites the marker and reapplies capability ACLs while leaving the local account, DPAPI secret, LSA rights, principal ACEs, and ledger in place. Wire an explicit opt-out retirement path into elevated setup (or stop advertising re-setup as the retirement mechanism). -
[P2] Include principal read-root grants in the setup marker fingerprint
internal/sandbox/windows_setup.go:274-306
internal/sandbox/windows_identity_acl.go:141-147
BuildWindowsSandboxSetupMarkerfingerprints onlyBuildWindowsACLPlan, which ignoresReadRoots. Principal setup separately emitsWindowsACLAllowReadACEs fromPermissionProfile.FileSystem.ReadRoots. Tightening or widening read exposure in policy therefore does not invalidate the marker, so re-setup is never forced and stale principal read grants can survive after the policy changed. -
[P2] Restore the ACL ledger when principal setup rolls back after ACL apply
internal/sandbox/windows_identity_runtime_windows.go:608-655,375-385
internal/sandbox/windows_setup_windows.go:53-63
applyWindowsPrincipalACLsnarrows the ledger to the newgrantedset only after revoke+grant succeed. If a later elevated step then fails (network apply or marker write),setupWindowsSandboxPrincipal's rollback callsrevertACL(), which puts the pre-revocation DACLs back on disk but does not restore the ledger entry that existed before this run. On an adopted principal (created == false) the account survives with DACLs wider than the ledger now records, so a subsequent policy narrowing can miss revoking paths that were dropped from the policy but restored by that rollback. This is a narrow failure path, but it reopens the stale-ACE widening class the ledger was added to close.
Prior review threads
The ledger-backed stale-ACE fix (applyWindowsPrincipalACLs unioning recorded paths, TestReSetupRevokesARootTheNarrowedPolicyDropped), the explicit --sandbox-principal setup protocol, runtime-root grants (TestSetupGrantsTheRuntimeRootCommandsActuallyUse), principal write-jail composition, and rebase onto current main all look correct on c3bfbc17. The three P1 items above remain open on this head and match the blocking feedback from the latest human review.
Needs maintainer decision
- The author documents that
LogonUser/ batch-logon minting has not been exercised on a clean elevated machine yet. Acceptable for a foundation PR only if maintainers are willing to merge before the command-time token path is proven. - With
ZERO_WINDOWS_SANDBOX_IDENTITY=1and defaultNetworkDeny, commands intentionally use the restricted-token path with no per-command warning. That is documented PR behaviour, not a sandbox escape. The remaining gap is claim drift:TestWindowsSandboxPrincipalFallbackIsAnnouncedsays that fact "belongs tozero doctornow", butwindowsSandboxSetupChecknever reports principal inertness under default deny. Please either wire that into doctor or revise the test comment and operator-facing copy.
The write-denied carveouts guarding .git are attached to .git/config and .git/hooks as OBJECTS. The workspace allow grant is inheritable and carries DELETE, and nothing denied DELETE on .git itself, so a principal could rename .git aside, recreate it, and create fresh config and hooks that inherit the allow with no deny of their own. That restores credential.helper and core.hooksPath, and with them arbitrary code execution on the next git command. .git could not simply join sandboxFullyProtectedMetadataNames next to .zero and .agents: that list emits DenyWrite, whose mask includes FILE_GENERIC_WRITE, and git has to write index, objects and refs. Its absence from that list was correct and was also the hole. Add WindowsACLDenyDelete: DELETE, WRITE_DAC and WRITE_OWNER only. Renaming a directory needs DELETE on that directory, so denying it is what closes the replacement. WRITE_DAC and WRITE_OWNER come along because a guard the principal can rewrite is not a guard. FILE_GENERIC_WRITE and FILE_DELETE_CHILD stay out so git keeps working. The ACE is applied uninherited, which is why the narrow mask is safe: inherited onto .git's children it would deny DELETE on every file inside and git could not remove a lock file or a ref. windowsExplicitAccessEntries hardcoded SUB_CONTAINERS_AND_OBJECTS_INHERIT for every directory entry, so inheritance is now decided per action. The entry is not materialized. git creates .git, and an empty one made by setup breaks git init. Reported by @jatmn on #808.
|
@jatmn the Your diagnosis was right and the root cause sits one level below where you pointed. The denies are indeed only on New action The part worth checking closely: the ACE is applied uninherited, which is what makes that narrow mask safe. Inherited onto Four tests: the plan emits the deny for Still open from your review, and I would rather do them properly than quickly: the materialization TOCTOU and the pathname-bound rollback. They are one change to me, since both need creation and cleanup bound to a retained no-follow parent handle, which means |
|
@gnanam1990 @anandh8x f0716ac closes one of jatmn's three P1s here, the one where a principal renames Asking for eyes on that commit specifically, not an approval. The other two P1s are still open, so this is not ready to merge and I would rather nobody approves it into that state. The part worth your attention is the inheritance decision rather than the mask. Worth knowing why @anandh8x your changes-requested from 4 August is the standing one on this PR. Worth a look at whether what it raised is settled at the current head, separately from this commit. Still open and not started: the materialization TOCTOU and the pathname-bound rollback. Both need creation and cleanup bound to a retained no-follow parent handle. |
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] Close the materialization and rollback TOCTOU with handle-relative creation and cleanup
internal/sandbox/windows_acl_apply_windows.go:302-333
internal/sandbox/windows_acl_apply_windows.go:335-401
Two surfaces, one fix class.makeWindowsACLDirChainNoFollowverifies the deepest existing ancestor, then creates missing components with pathname-basedos.Mkdir/os.OpenFile, so a workspace owner can junction-swap an ancestor in the verify-to-create window and elevated setup can materialize outside the approved tree.rollbackWindowsACLSnapshotsremoves materialized targets withos.RemoveAll(snapshot.Path)and restores existing targets by pathname after apply has closed its no-follow handle; your comment at:317-320already notes the residual restore TOCTOU. You flagged both as still open onf0716ace. Please bind creation and cleanup to a retained no-follow parent handle (NtCreateFilewithOBJECT_ATTRIBUTES.RootDirectory, as you described) rather than pathname re-resolution. -
[P1] Apply the
.gitdeny-delete ACE on the common fresh-workspace setup path
internal/sandbox/windows_identity_acl.go:136-140
internal/sandbox/windows_acl_apply_windows.go:72-74
internal/sandbox/windows_acl_apply_windows.go:96-100
f0716aceadds the right plan entry:WindowsACLDenyDeleteon.git, uninherited, withoutMaterialize(materializing empty.gitwould breakgit init). On a workspace with no.gityet — the caseTestPrincipalACLPlanMaterializesGitConfigAsFiletreats as common —groupWindowsACLPlanByPathsorts…\.gitbefore…\.git\config. The deny-delete group is applied first, seesos.ErrNotExist, hasMaterialize: false, and returns silently. Carveout materialization then creates.gitwhile applying…\.git\config/…\.git\hooks, and nothing revisits the skipped group. The ledger still records.git, so setup believes the rename guard exists when it may not. Tests prove plan emission and carveout shapes, not the on-disk DACL on.gitafter apply. Aftergit initon a workspace that had setup run first, the principal can still rename.gitaside and recreate it without carveouts. Please defer deny-delete until after carveout materialization creates.git, add a second pass for non-materialized directory guards, or apply deny-delete when.gitfirst appears without materializing an empty.git. -
[P2]
zero doctordoes not surface opt-in plus default network-deny
internal/doctor/hardening.go:101-122
internal/sandbox/windows_identity_runtime_windows.go:92-95
Protocol drift, not a sandbox bypass. WithZERO_WINDOWS_SANDBOX_IDENTITY=1and elevated setup, default-policy commands intentionally run on the restricted token because WFP deny filters need the offline-marker SID. You removed the per-command warning for noise and assigned the standing notice tozero doctor. Doctor only callsValidateWindowsSandboxSetupMarkertoday, so it can PASS while the principal backend is disabled for the operator's default network mode. Please add a doctor check when opt-in is set, the marker shows a provisioned principal, and the resolved policy is network-deny. -
[P2] Opt-out elevated setup does not retire an existing principal
internal/sandbox/windows_setup_windows.go:43-64
internal/sandbox/windows_setup.go:385-387
Protocol drift between error text and behavior.ValidateWindowsSandboxSetupMarkertells the operator to re-run elevated setup "without it to retire the principal."runWindowsSandboxSetuponly callssetupWindowsSandboxPrincipalwhen opt-in is enabled. Re-running with the opt-in unset writesprincipalOptIn: falseand succeeds while leaving the account, DPAPI secret, principal ACEs, and ACL ledger in place. Please callremoveWindowsSandboxPrincipalForSetupon the opt-out path, or change the marker error text so it does not promise retirement setup does not perform.
Merge and CI
-
Required checks on head
f0716ace:Zero Reviewfailed;Smoke (macos-latest)andSmoke (ubuntu-latest)failed;Smoke (windows-latest)passed.mergeable_stateisblocked. These need to be green before merge. -
mainhas advanced since base8e266797(for exampleview_imagein #843). Please rebase onto currentmainbefore merge.
Maintenance note (not merge-blocking on this head)
BuildWindowsSandboxSetupMarkerhashes onlyBuildWindowsACLPlan(internal/sandbox/windows_setup.go:274-306). Principal-only ACL semantics applied insetupWindowsSandboxPrincipaldo not affectACLPlanHash. Schema 5 already forces re-setup for capability-plan changes; a future principal ACL fix without a schema bump could leave stale on-disk ACEs while marker validation passes. Worth addressing before calling the principal ACL surface stable, but not an active exploit on this head.
Acknowledged scope (not raised as defects)
Per your recent comments:
- Principal backend disabled for
NetworkDenyis intentional; opt-in alone does not close #662 on default-policy commands. - Fallback runtime trees under
%TEMP%/%TMP%inherit from scope write roots with inheritable allow-write; separate per-processMkdirTempgrant is not needed for the common temp layout. - Lease-fallback command-time grant remains a follow-up only if a workspace needs it outside inherited temp rights.
Prior review threads
- Revocation-scope P1 (
f318d867): verified fixed. - Unrestricted-principal-token P1 (
331a265): verified fixed inwindows_command_runner_windows.go. - anandh8x materialization TOCTOU: still open (first finding above).
.gitrename P1: plan emission fixed inf0716ace; apply-order gap remains (second finding above).
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Request changes — one concrete blocker, and the security core is good
Re-review: my earlier review was dismissed and a dozen commits landed since, so I read the delta fresh rather than trusting it. Checked out at f0716ace.
Blocker: the newest commit's tests are Windows-only but have no build tag
internal/sandbox/windows_git_rename_guard_test.go (from f0716ace) has no //go:build windows constraint, so it runs everywhere and cannot pass off-Windows:
windows_git_rename_guard_test.go:43: no deny-delete entry for C:\/work/repo/.git ...
windows_git_rename_guard_test.go:91: the write-deny carveout for C:\/work/repo/.git/config disappeared
--- FAIL: TestThePrincipalCannotRenameTheGitDirectory
--- FAIL: TestTheGitRenameGuardDoesNotBlockGitsOwnWrites
C:\/work/repo/.git is the tell: a hardcoded C:\ joined with POSIX separators. go test ./internal/sandbox/ is green on origin/main and fails on this branch, so it is this PR, and CI agrees — Smoke (macos-latest) and Smoke (ubuntu-latest) fail while Smoke (windows-latest) passes.
Adding the build tag (matching the other *_windows_test.go files here) should be the whole fix. Everything else below assumes that lands.
The security core is careful work
Secret-on-disk ordering is right. writeWindowsSandboxSecret creates the file empty, locks the DACL, encrypts, then writes — so a secret is never on disk unprotected, and the comment states the reason ("the ACL below is applied to whatever inode ends up at this path"). Every failure path removes the file rather than leaving a partial one.
DPAPI is the right second layer, for the stated reason. The ACL is primary; CryptProtectData covers what an ACL cannot — a backup, a mounted image, or a copy taken by anyone who can bypass the DACL. Using the principal name as entropy is the detail that makes it more than decoration: moving one secret file over another fails to decrypt instead of silently authenticating the wrong principal. CRYPTPROTECT_UI_FORBIDDEN is correct for a CLI that may run without an interactive desktop.
Logon rights are minimised, not merely scoped: only SeBatchLogonRight, with interactive/network/remote/service explicitly denied, so a leaked password still cannot sign in. LogonUser pinned to "." closes the same-named-domain-account substitution.
The ordering fixes in the delta are the right ones — revoke logon rights before deleting a principal, drop the stored secret whenever provisioning fails, refuse a squatted account name and clean up partial provisioning. Each is a real half-finished-state hazard.
What I did not verify
This is ~6,800 lines and I have no Windows machine, so nothing Win32 was executed: DPAPI, LogonUser, the LSA rights calls and the ACL application are reviewed by reading plus GOOS=windows go build ./... and go vet, both clean. I read the credential and provisioning paths closely and the rest more lightly — worth another reviewer on the WFP/filter half specifically.
I want to credit the PR description: it states plainly that this does not close #662 for a default install, and that the logon half has not been run on real hardware. That is the kind of scoping that makes a foundation PR reviewable, and it is why the blocker above is the only thing I am holding on.
Groundwork for the two remaining materialization P1s. No call sites yet;
the walk and the rollback move onto these next.
Every pathname-based call re-resolves the whole path inside the kernel
when it runs, so verifying a component and then creating through it are
two separate resolutions of the same string. A workspace owner can swap
an ancestor for a junction in that gap. Demonstrated, not theorised: with
the same swap performed between verify and create, os.Mkdir put the new
directory OUTSIDE the approved tree ("landed inside approved tree: false,
ESCAPED outside approved tree: true"), and the post-create check cannot
un-create it.
A handle pins the object rather than the name, so a create resolved
against it cannot be redirected however the path is later rearranged.
os.Mkdir, os.OpenFile and os.RemoveAll are pathname-based by construction
with no relative form on Windows, hence NtCreateFile with
OBJECT_ATTRIBUTES.RootDirectory. x/sys/windows already exposes every
piece, so this adds no hand-rolled syscall bindings.
createWindowsACLChildDirectory reports whether it created or opened,
because rollback must delete only what it made; removing a directory that
already existed would destroy a user's data over an unrelated failure.
deleteWindowsACLChildDirectory is the counterpart that rollback needs:
os.RemoveAll on a pathname whose ancestor has since become a junction
recurses outside the workspace.
The fix also makes the race testable. Against the old code the swap had
to be threaded into the middle of a function; here it is three ordinary
lines between an open and a create, because the handle is held across
them. Both tests perform the real swap and assert the operation stayed
inside the verified directory, and the delete test leaves a bystander
under the decoy whose survival proves the delete never resolved by path.
Refs #808.
The two rename-guard tests built their expectations from a hardcoded C:\work\repo, but buildWindowsPrincipalACLPlan normalizes every write root before it names an ACE. On Windows that root is already absolute, so normalizing is a no-op and both tests passed locally. On Linux and macOS it is not absolute, so the plan named a different path and both tests failed. The plan builder is portable code with no build tag, so these tests run on every platform. Use an OS-neutral root and normalize the expected path the same way the plan does, which is what the other untagged test in this package already does. The Windows-only assertions about the ACE mask and its inheritance stay where they are, in the file that is tagged for it.
Closes the two path-swap findings on this PR. Both had the same cause: an object was addressed by a pathname that the kernel re-resolves at the moment the call runs, so a workspace owner could change what that name meant in the gap between checking it and using it. Junctions need no privilege, so this was reachable by exactly the unprivileged user the sandbox exists to contain. Materialization verified a component, closed the handle, then handed the same string to os.Mkdir. Swapping the verified ancestor for a junction inside that gap got the component created outside the approved tree, as Administrator, and verifying again afterwards cannot un-create it. It now opens the deepest existing ancestor once and creates each missing component relative to the handle of the level above, so the tree it descends is pinned to objects rather than named by strings. Rollback called os.RemoveAll on the target pathname, so an ancestor swapped to a junction after creation sent a recursive elevated delete into an unrelated tree. It now records what it created as single components under an anchor and unwinds them handle-relative, deepest first. It also removes the whole chain rather than only the final component, which the old comment claimed was fixed and was not, and it removes only what the kernel confirmed this run created, so a racing creator no longer gets its directory deleted on teardown. Three things this turned up that were not in the original findings: The delete primitive used FILE_DELETE_ON_CLOSE, which defers the removal to cleanup and reports a non-empty directory to nobody: open succeeded, close succeeded, directory still there. Wired into rollback that would have been worse than the os.RemoveAll it replaced, since rollback would report success while leaving state on disk. It now sets the disposition explicitly, so STATUS_DIRECTORY_NOT_EMPTY comes back to the caller. Residue is preferable to recursing through a path someone else controls; lying about it is not. The child create never inspected the handle it returned, so an existing junction was handed back as the next parent in the walk and every deeper create landed on the far side of it. Component names are now validated as single components too: NtCreateFile resolves a relative name containing separators the ordinary way, which would have walked straight through an intermediate junction. Rollback re-opens the anchor by pathname and checks its volume and file index against what materialization saw, because replacing a directory with another real directory of the same name needs no reparse point at all and passes every no-follow check there is. Holding the handle instead would be stronger, but three call sites deliberately discard the rollback closure and would leak. Testing. makeWindowsACLDirChainNoFollow carries a seam that fires between verifying the anchor and creating anything, because a race nobody can trigger on demand is not a regression test. The existing junction test plants its junction before the walk starts and so never reached this. The new test has a control arm that performs the identical swap and creates by pathname, and asserts the object does escape, so the fixed arm proves the hole was open rather than proving some code ran. Also covers the file target, rollback through the closure callers actually hold, and the non-empty case that the old empty-directory-only delete test hid. Validation now runs before the filesystem is touched, so a malformed entry no longer creates a chain and then fails.
|
@jatmn @anandh8x ready for another look. Head is Both of the blocking items in each of your reviews are addressed. The The materialization and cleanup races are Three things turned up that were not in either of your reviews, and they are the parts I would most like challenged. The delete primitive I had already landed used The child create never inspected the handle it returned, so an existing junction was handed back as the next parent and every deeper create landed on the far side of it. Component names are now validated as single components too, because Rollback re-opens the anchor by pathname and checks its volume serial and file index against what materialization saw. This is the weaker of the two options and I want it looked at. Holding the handle from materialization through rollback is strictly stronger, but three call sites deliberately discard the rollback closure, so holding would leak handles, one of them per command in a long-lived process. The identity check closes the specific attack that motivated holding: replacing a directory with another real directory of the same name, which needs no reparse point and passes every no-follow check in the package. @anandh8x your last line was that CI is green and those checks do not exercise these races. That was correct and it is the part I spent longest on. |
|
@gnanam1990 your blocker is fixed, but not the way you suggested, so flagging the deviation rather than letting you find it. You proposed adding Your diagnosis was exactly right, including the tell. Since your review, One thing from your review I have not solved: you asked for another reviewer on the WFP and filter half specifically, and that is still true. Nobody has read that half closely, and I am not counting your approval of the credential paths as covering it. |
ProtectedMetadataNames is documented as names, but it is joined onto the write root to place a deny ACE and to materialize the directory that ACE protects. A value containing ".." or a separator puts both outside the workspace: the deny lands on a directory the sandbox does not own, and elevated setup creates it there. Raised by CodeRabbit on this PR. Not reachable today, since the only caller passes a package constant. It is guarded so that stays true if a future caller sources these from config, which is the kind of change that would not obviously be a security decision. The component check moves to the portable file so both users share it. The handle-relative primitives need it because NtCreateFile resolves a relative name containing separators the ordinary way, following any junction inside it; the plan builder needs it for the escape above. Separators are matched explicitly rather than through filepath.Base, because these are Windows paths whatever the build host is, and on Linux filepath.Base leaves a backslash-joined name untouched and would wave it through. Rejection tests are in the portable test file, so they run on all three platforms alongside the rest of the plan-builder coverage.
|
@jatmn @anandh8x @gnanam1990 head has moved again since I pinged this morning, and every thread on the PR is now closed, so this is the state rather than a repeat ask. Head is New since the earlier ping: CodeRabbit's open thread on Worth flagging that I did not take the suggested fix. CodeRabbit proposed Where each of your reviews stands: @jatmn both P1s at @anandh8x both P1s at @gnanam1990 your blocker at None of you need to re-read the whole thing. If what you filed is addressed, clearing the changes-requested is enough; if it is not, say which part and I will keep going. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed 209898df, re-confirmed as the live head before posting. My own blocker is fixed; I am keeping changes-requested because one of @jatmn's P1s is verifiably still open on this head, and because of a rebase consequence below that nobody has flagged yet.
My earlier blocker: fixed, and better than what I asked for
I asked for //go:build windows on windows_git_rename_guard_test.go. 30e4e8c9 instead split it — portable cases stay untagged, Windows-only cases move to windows_git_rename_guard_windows_test.go, where the filename suffix carries the constraint anyway. That keeps the portable assertions running on every platform instead of hiding them behind a tag, which is the better shape. go vet ./internal/sandbox/ is clean, the guard tests pass on darwin, and GOOS=darwin|linux|windows go build ./... all succeed.
jatmn's P1 #1 (materialization/rollback TOCTOU): fixed
5d1d827d + 8a8c19b5 add windows_acl_relative_windows.go, which does exactly what was asked — NtCreateFile with OBJECT_ATTRIBUTES.RootDirectory for handle-relative creation, and an unwind that the comment at windows_acl_apply_windows.go:151 explicitly forbids from falling back to pathname re-resolution. Verified present rather than taken from the commit message.
jatmn's P1 #2 (.git deny-delete): still open on this head
209898df adds windowsACLGroupRequiresExistingTarget, which turns a missing target into a hard error instead of a silent skip. That is the right shape — but it only fires for one action:
func windowsACLGroupRequiresExistingTarget(group windowsACLPathGroup) bool {
for _, entry := range group.Entries {
if entry.Action == WindowsACLAllowWrite { // DenyDelete is not covered
return true
}
}
return false
}windows_acl_apply_windows.go:264-271
The original chain is therefore intact on a fresh workspace with no .git:
- The plan emits
.gitasWindowsACLDenyDeletewith noMaterialize—windows_identity_acl.go:145-148, deliberately, since materializing an empty.gitbreaksgit init. - Groups are sorted lexicographically by path key, so
…\.gitis applied before…\.git\config—windows_acl_apply_windows.go:116-118. - Applying it: target missing →
os.ErrNotExist,!group.Materialize, andwindowsACLGroupRequiresExistingTargetis false because the entry is DenyDelete, not AllowWrite →return windowsACLSnapshot{}, false, nil—:168-177. - The carveout groups then materialize
.gitas a side effect, and nothing revisits the skipped guard.
Net: the rename guard is silently absent on exactly the workspace shape TestPrincipalACLPlanMaterializesGitConfigAsFile calls common. The four guard tests cover mask semantics and plan emission; none covers the on-disk state after apply on a workspace with no .git, which is why this survives a green suite.
Being explicit about my evidence: this is a code-reading conclusion, not an observed failure. windows_acl_apply_windows.go is _windows.go-tagged and the path needs elevated setup, so I could not execute it. Each of the four steps is individually verifiable at the cited lines, but if the author sees a reason the group is revisited, I would rather be corrected than have this stand.
Rebase consequence worth knowing before you rebase
I ran the built binary through complete feature runs on real Windows — file writes, nested paths, workspace-escape refusal, --add-dir grants, control characters, specialist child processes, and exec_command:
| ref | Windows |
|---|---|
209898df (this PR) |
7/7 |
7f39a630 (current main) |
6/7 |
That looks like this PR is better than main, and it is — but not for a reason that survives a rebase. exec_command is broken on current main and this branch is 10 commits behind, so it simply predates the break. I bisected it on real Windows runners:
| ref | exec_command |
|---|---|
8e266797 (this PR's base) |
pass |
021281eb (#827) |
pass |
91b413c5 (#865) |
fail |
7f39a630 (main) |
fail |
#865, "stop the Windows write jail honouring Everyone-granted paths," is the culprit. Under it PowerShell cannot start inside the sandbox — .NET fails crypto init with Unable to load DLL 'BCrypt.dll' … (0x8007045A) — so exec_command cannot run so much as echo. That is mechanically consistent with the note already in windows_command_runner_windows.go:61-64: a WRITE_RESTRICTED write check must also match one of the token's restricted SIDs, and Everyone was one of them.
None of that is this PR's fault. It matters here for two reasons: jatmn's rebase request will pull #865 in, so the clean 7/7 will become 6/7 through no change of yours; and since this PR is the one rewriting Windows principals and write-jail semantics, whoever fixes #865 should probably coordinate with it rather than patch around it. I am reporting #865 separately.
Also verified
go test ./internal/sandbox/ ./internal/tools/— both green at209898df.- Cross-compiles for darwin, linux, and windows.
- The other open reviews: @anandh8x's materialization TOCTOU is subsumed by jatmn's P1 #1 and now fixed; jatmn's two P2s (doctor not surfacing opt-in + default network-deny, and opt-out setup not retiring an existing principal) I did not re-check and take no position on.
Verdict
Changes requested, on jatmn's P1 #2 alone. Everything raised against this PR that I can verify is now fixed, including my own item and the harder of the two P1s, and the handle-relative rewrite is good work. Extending windowsACLGroupRequiresExistingTarget to cover WindowsACLDenyDelete would surface the gap loudly instead of silently, though jatmn's suggestion of deferring deny-delete until after carveout materialization is the version that also keeps fresh workspaces working. Either way it wants a test that asserts the DACL on .git after apply, starting from a workspace that has none.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Requesting changes. I reviewed the latest head (209898df) and found several security/lifecycle issues that are not exercised by the currently green CI.
Blocking findings
-
Fresh-workspace
.gitrename protection is not applied. The plan adds a non-materializing deny-delete entry for<root>\.git(windows_identity_acl.go), butgroupWindowsACLPlanByPathsorts paths ascending. During application, the missing.gitgroup is encountered first and skipped becauseMaterializeis false. Later.git\configand hooks entries create.git, but the skipped root entry is never revisited. The resulting fresh workspace has no deny-delete ACE on.git, so renaming/replacing it can bypass the protected config/hooks carve-outs. Existing tests check plan shape but not the final DACL after applying the plan to a workspace without.git. -
The production profile can install an inheritable principal read ACE at the current drive root.
PermissionProfileFromPolicyalways includesprofileRootPath()(\on Windows) inReadRoots; principal planning converts every read root intoWindowsACLAllowRead, and directory entries use container/object inheritance. During elevated setup this typically becomes a persistent read grant onC:\that may propagate into unprotected descendants, far beyond the intended workspace/runtime roots. Principal ACL construction should exclude the synthetic read-all root and use explicit bounded roots, with a regression test derived from the production profile. -
Elevated setup is not serialized. Two setup processes can interleave password rotation and secret persistence, leaving the account with password B but secret A. The principal ACL ledger has the same full read-modify-write race: concurrent policies can leave a broad ACE installed while the final ledger records only a narrower path set. Please add an interprocess per-workspace lock covering the complete setup transaction, not only individual atomic writes.
-
The setup marker does not fingerprint the principal ACL plan/read roots.
BuildWindowsSandboxSetupMarkerhashesBuildWindowsACLPlan, while principal read grants are built separately. Changing/removing principal read roots therefore does not invalidate setup and can leave stale grants active. -
Adopted-principal rollback restores DACLs without restoring the old ledger.
applyWindowsPrincipalACLswrites the narrowed ledger before the complete setup transaction succeeds. If network setup or marker persistence then fails, the rollback restores old DACLs but leaves the narrowed ledger, so later cleanup can miss restored stale grants.
Additional actionable findings
-
Opt-out does not retire an existing principal. The validation error tells users to rerun setup without the opt-in variable to retire it, but the opt-out path only writes a marker and leaves the account, secret, rights, ACEs, and ledger installed.
-
Doctor does not surface that principal execution is inactive under the default network-deny policy. Eligibility returns false and silently falls back to the same-user restricted token. The implementation comment says doctor should report this, but the current doctor path only validates the marker.
-
Cross-admin cleanup can strand the principal. Reading a secret owned by another setup administrator treats access denial as unavailable, but removing that secret returns the permission error and aborts cleanup before removing the account/rights/ACEs.
-
Teardown discards ACE-revocation errors.
removeWindowsSandboxPrincipalForSetupignores the result ofrevokeWindowsPrincipalACEs, then can delete the account and ledger and report success. That can leave orphaned ACEs while also removing the metadata needed for a retry. Cleanup errors must be reported and recovery state preserved. -
The promised legacy ownership-comment upgrade is not implemented.
windowsSandboxUserIsManagedaccepts the old bare comment and says it will be rewritten, but provisioning only changes the password; it never updates the account comment. -
Required static analysis fails on PR-introduced code:
internal/sandbox/windows_identity_acl.go:204:6: func windowsACLPlanPaths is unused (unused)The helper is portable but only consumed by Windows-tagged code. It should be moved behind the Windows build tag or receive a legitimate portable consumer.
-
The branch needs rebasing onto current
main. Its merge base is8e266797, while current main is7f39a630, and it overlaps the Windows sandbox security fix from #865. A synthetic merge was clean and retained that change, but this repository requires a fresh base before review.
Validation performed: formatting, Windows-targeted vet, Windows sandbox test cross-compilation, repository vet, build, smoke, vulnerability scan, diff hygiene, and go test -race ./internal/sandbox. Those passed. Full tests had only the two existing baseline doctor-test failures. make lint-static failed on the PR-introduced unused helper above. The real elevated LogonUser/batch-logon path still needs validation on a clean elevated Windows machine.
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.What this does NOT do yet
Two corrections to how an earlier version of this description read, both raised in review.
This does not close #662 for a default install. The principal backend is deliberately disabled whenever the network mode is deny (
windowsSandboxPrincipalEligible), because WFP block filters key on the offline-marker SID that only a restricted token can carry. Default policy IS network-deny. So with nothing butZERO_WINDOWS_SANDBOX_IDENTITY=1set, commands keep using the restricted same-user token andcredentialDenyReadPathsremains a no-op on Windows. Principal read confinement needs elevated setup AND a network-allow command profile, until the filters are also keyed to the principal SID. This PR is the foundation for #662, not its fix.One change here is not gated by the opt-in.
WindowsACLAllowWritenow includesDELETEandFILE_DELETE_CHILD. That mask is shared with the capability-SID plans, so it applies on every elevated setup re-run whether or not the env var is set. It is a fix rather than a regression (without it a sandboxed command could create files it could never delete or rename), but it is a real behaviour change for installs that never opt in, and it belongs in the release notes rather than buried in a principal PR.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