feat(sandbox): give each workspace an offline and an online principal - #812
feat(sandbox): give each workspace an offline and an online principal#812Vasanthdev2004 wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughWindows sandbox identity handling now provisions separate offline and online principals, selects them by network mode, applies offline-group network enforcement, and builds and validates network infrastructure after principal provisioning. ChangesWindows identity and network enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant runWindowsSandboxSetup
participant setupWindowsSandboxPrincipal
participant BuildWindowsNetworkInfraPlan
participant WindowsNetworkPlanCoversPrincipals
runWindowsSandboxSetup->>setupWindowsSandboxPrincipal: provision offline and online principals
setupWindowsSandboxPrincipal-->>runWindowsSandboxSetup: return role-specific identities
runWindowsSandboxSetup->>BuildWindowsNetworkInfraPlan: build network plan
BuildWindowsNetworkInfraPlan-->>runWindowsSandboxSetup: return identity SIDs and filters
runWindowsSandboxSetup->>WindowsNetworkPlanCoversPrincipals: verify principal coverage
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/windows_identity_windows.go (1)
179-198: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNo migration path for accounts provisioned before role tags existed. Adding the role tag to
windowsSandboxUserNamemakes every previously provisioned account name unreachable: nothing deletes it, and nothing finds it. On an existing opt-in machine that means a stale local account plus its secret and LSA logon rights linger forever, while commands silently drop to the restricted-token backend with no indication that setup needs re-running.
internal/sandbox/windows_identity_windows.go#L179-L198: delete or adopt the legacy untagged name (windowsSandboxUserPrefix + cleaned) during provisioning, so re-running setup reclaims the old account instead of abandoning it.internal/sandbox/windows_identity_runtime_windows.go#L96-L136: when the role lookup misses but a legacy untagged account exists, emit a one-line stderr hint pointing atzero sandbox setuprather than falling back mutely.🤖 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 179 - 198, The new role-tagged naming in windowsSandboxUserName needs a migration path for legacy accounts. In internal/sandbox/windows_identity_windows.go lines 179-198, during provisioning detect and delete or adopt the legacy untagged name formed from windowsSandboxUserPrefix and cleaned before using the role-tagged name; in internal/sandbox/windows_identity_runtime_windows.go lines 96-136, when role lookup misses but that legacy account exists, emit a one-line stderr hint directing the user to zero sandbox setup instead of silently falling back.
🧹 Nitpick comments (3)
internal/sandbox/windows_identity_windows_test.go (1)
396-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the online role through provisioning, not only name generation.
This regression test proves that offline and online names differ, but the privileged provisioning, logon-rights, token, and lookup tests cover only
windowsSandboxRoleOffline. Add an online-role round-trip case (or parameterize the existing integration test) so a role-specific provisioning or lookup regression cannot pass unnoticed.As per coding guidelines: “
**/*_test.go: Keep tests beside their source files, add regression tests for behavior changes, and run affected concurrent code under the race detector.”🤖 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_test.go` around lines 396 - 425, Add an integration round-trip test for windowsSandboxRoleOnline, covering provisioning, logon-right assignment, token creation, and account lookup alongside the existing offline-role coverage. Prefer parameterizing the existing role-specific integration test so both roles execute the same assertions, while preserving the current offline case and running the affected concurrent test with the race detector.Source: Coding guidelines
internal/sandbox/windows_network_test.go (1)
128-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGlobal hook mutation is safe only while no test in this package is parallel.
Save/restore via
t.Cleanupis right, butresolveWindowsSandboxOfflineGroupSIDHookis package-global. The moment someone addst.Parallel()to any test that callsBuildWindowsNetworkInfraPlan, this becomes a data race that-racewill flag. A short comment stating these tests must stay serial would save the next person the debug.As per coding guidelines, "run affected concurrent code under the race detector".
🤖 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_network_test.go` around lines 128 - 129, Document near the save/restore of resolveWindowsSandboxOfflineGroupSIDHook that tests using this package-global hook, including BuildWindowsNetworkInfraPlan callers, must remain serial and must not use t.Parallel; preserve the existing t.Cleanup restoration.Source: Coding guidelines
internal/sandbox/windows_setup_windows.go (1)
57-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrdering fix is correct. The offline group only exists after provisioning, so building the plan here is what makes the filters actually name it.
Optional: this block, the
applyWindowsNetworkPlanblock, and the marker-write block are now three byte-identical rollback-and-report branches. A smallfailSetup(stderr, err, rollback) inthelper would collapse ~24 lines and guarantee the next failure path can't forget the rollback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_setup_windows.go` around lines 57 - 73, The setup failure handling is duplicated across the plan-building, applyWindowsNetworkPlan, and marker-write branches. Add a failSetup helper that accepts stderr, the setup error, and rollback, performs the existing rollback-and-report behavior, and returns the failure status; replace all three duplicated branches with calls to this helper while preserving their current error messages and rollback semantics.
🤖 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 214-216: Update the privileged tests around
provisionWindowsSandboxIdentity to stop unconditionally removing deterministic
accounts via removeWindowsSandboxIdentity. Use a unique per-test account key, or
track and remove only identities created by the current test, preserving
legitimate pre-existing accounts in both affected test cases.
In `@internal/sandbox/windows_network.go`:
- Around line 84-86: Update the groupSID handling in the identity SID collection
to append the trimmed value produced by strings.TrimSpace, while retaining the
non-empty guard. Ensure IdentitySIDs contains canonical SID strings so
applyWindowsNetworkPlan can consume them directly.
---
Outside diff comments:
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 179-198: The new role-tagged naming in windowsSandboxUserName
needs a migration path for legacy accounts. In
internal/sandbox/windows_identity_windows.go lines 179-198, during provisioning
detect and delete or adopt the legacy untagged name formed from
windowsSandboxUserPrefix and cleaned before using the role-tagged name; in
internal/sandbox/windows_identity_runtime_windows.go lines 96-136, when role
lookup misses but that legacy account exists, emit a one-line stderr hint
directing the user to zero sandbox setup instead of silently falling back.
---
Nitpick comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 396-425: Add an integration round-trip test for
windowsSandboxRoleOnline, covering provisioning, logon-right assignment, token
creation, and account lookup alongside the existing offline-role coverage.
Prefer parameterizing the existing role-specific integration test so both roles
execute the same assertions, while preserving the current offline case and
running the affected concurrent test with the race detector.
In `@internal/sandbox/windows_network_test.go`:
- Around line 128-129: Document near the save/restore of
resolveWindowsSandboxOfflineGroupSIDHook that tests using this package-global
hook, including BuildWindowsNetworkInfraPlan callers, must remain serial and
must not use t.Parallel; preserve the existing t.Cleanup restoration.
In `@internal/sandbox/windows_setup_windows.go`:
- Around line 57-73: The setup failure handling is duplicated across the
plan-building, applyWindowsNetworkPlan, and marker-write branches. Add a
failSetup helper that accepts stderr, the setup error, and rollback, performs
the existing rollback-and-report behavior, and returns the failure status;
replace all three duplicated branches with calls to this helper while preserving
their current error messages and rollback semantics.
🪄 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: a1ae6364-8744-4d97-a882-357b313596ad
📒 Files selected for processing (7)
internal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.gointernal/sandbox/windows_network.gointernal/sandbox/windows_network_test.gointernal/sandbox/windows_setup_windows.go
|
CI is green on all five checks, which closes the gap I flagged in the description. The three tests I could not run locally have now run: Still unexercised, unchanged from #808: the privileged calls. |
|
Both handled in f551a44, though the second one differently than suggested. Deleting accounts by derived name. Right, and the fix belongs deeper than the tests. Ownership is now read back from the comment provisioning stamps on the account, and an account that is not ours is left alone rather than deleted. The gated tests get it for free, since their pre-clean goes through the same helper, so they keep the deterministic key and still converge after a crashed run. The test asserts against The untrimmed append. Taken, but the stated consequence does not hold and I would rather say so than quietly accept it. Local runs are working again, so everything on this branch is now verified here rather than only in CI: the full |
f551a44 to
6c3037f
Compare
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested — on branch state, not on the design.
Reviewed at 6c3037fe2c17, base feat/windows-sandbox-identity, re-confirmed live before posting. This is no longer marked draft, so I am reviewing it as a real pull request rather than leaving it alone as I did earlier.
The blocking point is small and mechanical: this branch is behind the branch it targets.
#808 is at 832f53a; this is at 6c3037f, and git merge-base --is-ancestor pr808 pr812 is false. The commit you are missing is 832f53a "fix(sandbox): drop the stored secret whenever provisioning fails", which is not incidental to this change — it touches provisionWindowsSandboxIdentity, the same function this branch edits, and #808 changed its signature to return an extra bool. A textual auto-merge that reports no conflict is not sufficient evidence here; this repository has already produced a clean merge that did not build. Rebase onto the current #808 head and confirm the merged tree still cross-compiles for Windows before this is ready.
I checked each branch independently: GOOS=windows go vet ./internal/sandbox/... and GOOS=windows go test -c both pass on #808 at 832f53a and on this branch at 6c3037f. I did not get a clean run of the two merged together, which is exactly the gap the rebase closes.
On the substance, which I think is right.
The diagnosis is the strongest part. #808 shipped with the principal backend standing down whenever the network was denied, deny is the default, so the feature was close to inert in ordinary use — and the reason is not visible from the code. LogonUser builds a token from real group memberships and the offline marker is a synthetic capability SID with no account behind it, so the two mechanisms could never meet. A real local group is the right answer, and keying one filter set to it rather than one per workspace is the right trade.
Building the network plan after provisioning is load bearing and I would keep it as prominent as you have it. Planning first installed filters naming only the marker and left every offline principal with an open network while the machine reported itself correctly configured — a security control that reports success while enforcing nothing. That deserves a test asserting the ordering directly rather than only the end state, because it is exactly the kind of thing a later refactor reverses silently.
Proving ownership before deleting is the same predicate #808 now uses before adopting, correctly applied to the other end. Good.
One non-blocking observation on that path. When the account is not ours, removal returns nil. Idempotence makes that defensible — there is no principal of ours, so the goal state holds — but it also means a stranger's account squatting the derived name produces a silent success, and the operator is told cleanup completed when a name they may care about was left in place. A diagnostic, or a distinguishable return, would cost little.
Worth settling alongside #808 rather than here. Two accounts per workspace instead of one doubles what an operator sees in net user, and endpoint protection and enterprise policy were already an open product question on the parent. That is a conversation for the stack, not a defect in this change.
Limitations. No Windows host, no elevated session. Everything above rests on reading and cross-compilation; none of the privileged paths this adds have been executed by me.
832f53a to
99fefdc
Compare
6c3037f to
4fc1bfa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/sandbox/windows_identity_windows_test.go (2)
95-96: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not print generated passwords.
A failed assertion writes the generated credential into test and CI logs. Redact it.
Proposed fix
- t.Fatalf("password %q lacks a required character class", first) + t.Fatal("generated password lacks a required character class")🤖 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_test.go` around lines 95 - 96, Update the failure message in the password character-class assertion to avoid interpolating the generated password variable first. Keep the assertion and required-character-class diagnostic, but report only a non-sensitive message so generated credentials never appear in test or CI logs.
217-274: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd an offline-group membership assertion to the elevated provisioning test.
This round-trip covers provisioning and batch logon, but it still doesn’t check that the offline principal was added toZeroSandboxOffline, which is the part that actually drives network denial.🤖 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_test.go` around lines 217 - 274, Extend the elevated provisioning test after the second provision to verify that the identity from provisionWindowsSandboxIdentity belongs to the ZeroSandboxOffline group. Use the existing Windows group-membership lookup or assertion helper, and fail the test if the provisioned SID is not a member while preserving the current logon and cleanup checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 95-96: Update the failure message in the password character-class
assertion to avoid interpolating the generated password variable first. Keep the
assertion and required-character-class diagnostic, but report only a
non-sensitive message so generated credentials never appear in test or CI logs.
- Around line 217-274: Extend the elevated provisioning test after the second
provision to verify that the identity from provisionWindowsSandboxIdentity
belongs to the ZeroSandboxOffline group. Use the existing Windows
group-membership lookup or assertion helper, and fail the test if the
provisioned SID is not a member while preserving the current logon and cleanup
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae325643-a3d0-46ab-88da-b3072102d409
📒 Files selected for processing (7)
internal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.gointernal/sandbox/windows_network.gointernal/sandbox/windows_network_test.gointernal/sandbox/windows_setup_windows.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/sandbox/windows_network_test.go
- internal/sandbox/windows_identity_runtime_windows_test.go
- internal/sandbox/windows_network.go
- internal/sandbox/windows_setup_windows.go
- internal/sandbox/windows_identity_windows.go
- internal/sandbox/windows_identity_runtime_windows.go
|
Your blocking point is fixed, and your reason for insisting on it was correct in a way I would not have predicted. Rebased. This is now on And you were right to refuse a clean auto-merge as evidence. The rebase reported no conflict and did not build. The evidence you asked for, on the merged tree: Plus On the ordering, I took your suggestion and went a step further. You asked for a test asserting the ordering directly rather than the end state. A test alone would not have helped much, because the thing that breaks is the sequence inside The predicate is separate so it can be asserted on its own: marker-only reads as uncovered, marker-plus-group as covered, case differences do not read as missing, and a host with no group provisioned is not refused, since there is no principal for the filters to miss. Making coverage always return true fails that test. The silent success on removal is fixed too. It returns a distinguishable typed error now, and teardown treats it as success, since "no principal of ours under this name" is the goal state whether we deleted one or declined to. You were right that a bare On the account count, agreed it belongs on the stack rather than here. It is two per workspace now, and the local-account visibility question was already open on Your limitation note applies to me too, more so: the privileged paths in this change have not been executed anywhere, and the offline-group membership is the mechanism that carries the network denial, so it is the thing I would most want proven on real hardware before this ships. |
|
@gnanam1990 your changes-requested here is on Worth repeating the part you called: you refused to accept a clean auto-merge as evidence, and you were right. The rebase reported no conflict and did not build. Evidence on the merged tree, as you asked: On the ordering, I went further than a test. A unit test cannot easily observe a sequence inside Removal no longer reports plain success when it declines to delete an account Zero did not create; it returns a distinguishable typed error, and teardown treats it as success since the goal state holds either way. Full detail is in my earlier comment. Your limitation note applies to me more than to you: the privileged paths here have been executed by nobody, and offline-group membership is the mechanism that carries the network denial, so that is the thing most worth proving on real hardware before this ships. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at 011b26d8ff3c, base feat/windows-sandbox-identity (99fefdc), re-confirmed live before posting. This withdraws my changes-requested.
The blocking point is resolved. This branch is no longer behind the one it targets: git merge-base --is-ancestor pr808 pr812 is now true and there are zero commits on #808 missing here, including 99fefdc, the one that changed provisionWindowsSandboxIdentity's signature and was my specific concern. I re-checked the merged state rather than relying on the ancestry alone — at this head go build ./..., go vet ./... and ./internal/sandbox all pass, and GOOS=windows go vet ./internal/sandbox/... plus GOOS=windows go test -c both succeed, so the stacked tree type-checks for Windows.
My non-blocking observation is also addressed, and better than I framed it. Removal no longer returns a bare nil when the account is not Zero's. errWindowsSandboxForeignAccountRetained is distinguishable, teardown paths treat it as success, and the comment explains the distinction precisely: leaving the account alone is correct, but reporting plain success would tell an operator cleanup completed when a name they may care about was deliberately left in place. That is exactly the right resolution — it keeps idempotence for callers who want the goal state while making the refusal visible to anyone who cares which.
The new assertion that the filters cover principals is worth having for the same reason the ordering is: it pins a property that is invisible from the outside when it breaks.
On the substance, which I said before and still think. The diagnosis is the strongest part of this change. #808 shipped with the principal backend standing down whenever the network was denied, deny is the default, so the feature was close to inert in ordinary use — and nothing in the code reveals why. LogonUser builds a token from real group memberships while the offline marker is a synthetic capability SID with no account behind it, so the two could never meet. A real local group is the right answer, and keying one filter set to it rather than one per workspace is the right trade.
Building the network plan after provisioning remains the load-bearing ordering, since planning first left every offline principal with an open network while the machine reported itself correctly configured. Proving ownership before deleting is the same predicate #808 uses before adopting, correctly applied to the other end.
One thing that belongs to the stack rather than to this PR. Two accounts per workspace instead of one doubles what an operator sees in net user, and endpoint protection and enterprise policy were already an open product question on the parent. Worth settling there before either lands.
Limitations. No Windows host, no elevated session. Everything above rests on reading and cross-compilation; none of the privileged paths this adds have been executed by me, and the same caveat that made #808 approvable applies here — the surface is behind an opt-in flag that is off by default.
CodeRabbit's changes-requested from earlier is separate from this and still outstanding.
Merge order is #808 then this, and merge itself is kevin's call per the program gate.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Review at 011b26d
PR #812 — give each workspace an offline and an online principal. Stacked on #808 (targets that branch). 7 files, +520/-97 incremental over #808's head. 3 commits.
Verdict: approve. The design closes the biggest gap in #808 (network denial was inert under the principal backend), and the load-bearing orderings are pinned by tests.
What this fixes
#808 shipped with the principal backend standing down whenever the network was denied. Deny is the default, so opting in left the restricted-token path doing all the work — the feature was close to inert. The cause: WFP block filters key to the offline-marker SID (a synthetic capability SID), and LogonUser builds a token from an account's real group memberships, so a principal token can't carry the marker.
How
A real local group (ZeroSandboxOffline). The block filters name its SID alongside the marker. A principal is denied the network by being a member of it. Each workspace gets two accounts that differ only in that membership; the command's network mode picks between them. A group rather than per-principal SIDs because one filter set covers every offline principal on the machine — no per-workspace filter and no setup re-run when a workspace appears.
What's good
- The diagnosis is the strongest part. The PR body explains why #808's principal was inert (LogonUser vs. synthetic SID) and why a group is the right fix (one filter set, no per-workspace filters). That's the kind of reasoning that makes the change safe to review without a Windows box.
- Fail-closed for unknown modes. Anything that isn't an explicit
NetworkAllowmaps to the offline principal. An unrecognized mode loses the network rather than keeps it.windowsSandboxRoleForNetworkis a clean 3-line function. - Role tag before the hash in the account name.
zero-sbx-d<hash>(offline) vszero-sbx-n<hash>(online). Truncation at the 20-char limit eats hash characters, never the tag.TestWindowsSandboxUserNameSeparatesRolesproves the two roles never collide, including under truncation — the dangerous collision is an ordinary command silently getting the online principal. - Both roles provisioned together. Setup needs elevation; commands don't. Provisioning lazily would mean an unelevated command discovering it needs an account it can't create. Both get identical filesystem access — an approved network command sees the same filesystem as an ordinary one.
- Network plan built AFTER provisioning. The group it keys to is created there. Planning first installed filters naming only the marker, leaving every offline principal with an open network while the machine reported correctly set up. The PR calls this out explicitly as "the same class of silent-downgrade bug this PR exists to remove."
WindowsNetworkPlanCoversPrincipalspins the ordering. This predicate is asserted in setup before installing anything. Moving the plan build back ahead of provisioning fails loudly.TestNetworkPlanCoverageDetectsPrincipalsLeftUncoveredproves a marker-only plan is detected as not covering principals, while a marker-plus-group plan passes. Case-insensitive SID comparison. Unprovisioned host (no group) is a no-op.- Setup-marker compatibility. The filter identity set is resolved, not assumed, and stays absent until the group exists. A machine that never provisions principals computes exactly the plan it computed before.
TestNetworkInfraPlanIncludesOfflineGroupIdentityproves the hash changes when the group appears (so setup and command path can't disagree silently) and the marker SID stays at position 0. - Lookup failure propagates.
TestNetworkInfraPlanPropagatesOfflineGroupLookupFailureproves a failed group lookup returns an error rather than silently degrading to "no group" (which would produce filters that cover no principal). - Ownership-before-delete.
removeWindowsSandboxIdentitynow callswindowsSandboxUserIsManagedbefore deleting. A foreign account returnserrWindowsSandboxForeignAccountRetained(distinguishable, not bare nil). Teardown paths treat it as success; operators see the refusal. This is gnanam's non-blocking finding from #808, addressed better than asked — the comment explains precisely why bare nil would be wrong. - Rollback is multi-step and ordered.
setupWindowsSandboxPrincipalprovisions both roles in a loop, appending removal and ACL-revert closures to anundoslice. Rollback unwinds in reverse, keeps going after a failure, reports the first error. ACEs reverted before accounts deleted (orphaned-residue prevention).
Verification
GOOS=windows go vet ./internal/sandbox/...— cleanGOOS=windows go test -c— compiles (type-checks all Windows surface)go build ./internal/sandbox/...(Linux) — cleango test ./internal/sandbox/(Linux, from non-/tmppath) — passgo vet ./internal/sandbox/...— clean- Stacked on #808:
git merge-base --is-ancestor pr808 pr812confirmed by gnanam; zero #808 commits missing.
Cost noted in the PR
Two accounts per workspace instead of one. Doubles what an operator sees in net user. The PR explicitly says this is a deliberate trade for making network denial work under a separate identity, and that the endpoint-protection/enterprise-policy product question belongs to the stack (both #808 and this) rather than to this PR alone.
What's not verified (same caveats as #808)
The privileged half is still unexercised: LogonUser, NetUserAdd, LsaAddAccountRights need an elevated Windows box. Three new tests haven't run locally (Smart App Control blocked the test binary). CI is the check. The feature is behind ZERO_WINDOWS_SANDBOX_IDENTITY=1, off by default.
Verdict
Approve. The design closes the network-denial gap in #808 correctly, the two load-bearing orderings (plan after provisioning, role tag before hash) are pinned by tests, and the ownership-before-delete check is the right resolution to gnanam's #808 finding. Merge order is #808 then this.
Stale — superseded by later APPROVED reviews on the latest head (011b26d). gnanam1990 APPROVED at 15:39:03Z; anandh8x APPROVED at 15:42:35Z. Merge state is CLEAN.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed the latest stacked head 011b26d against #808. The offline/online split and WFP group design are sound, but this branch adds a blocking rollback hole.
[P2] internal/sandbox/windows_identity_windows.go:537-540 — after a new offline account has been created and added to the managed group, failure while adding it to ZeroSandboxOffline returns created=true with an empty identity. provisionWindowsSandboxPrincipalForSetup then runs its cleanup with identity.Username empty, so the account this run just created is left behind. This is especially relevant here because group membership is the enforcement boundary and can fail under local policy. Return enough identity for rollback or clean up within provisionWindowsSandboxIdentity, and add a failure-path test. This should be resolved together with the analogous post-creation paths noted on #808.
011b26d to
5db0e6e
Compare
|
Taken, in The offline-group step now returns the account name with its error, same as the other post-creation failures. It is the one that matters most here for the reason you gave: membership IS the network denial, local policy can refuse a group join, and the rollback deleting Covered by an injected failure alongside the other two paths. Reverting just that return fails only the offline case: One thing the rebase turned up that is worth flagging, because it would not have been obvious in review. Also seamed |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/windows_identity_rollback_windows_test.go (1)
145-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the read ACL remains a grant.
modeis discarded, soDENY_ACCESSor an empty mask would still pass as long as no write bits are present. Assertmode == windows.GRANT_ACCESSand at least one contractually required read bit.🤖 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_rollback_windows_test.go` around lines 145 - 164, Update TestWindowsACLAllowReadGrantsNoDelete to retain the mode returned by windowsACLAccess and assert it equals windows.GRANT_ACCESS. Also assert that the mask contains at least one contractually required read-access bit, while preserving the existing checks that forbid delete and write bits.
🤖 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_rollback_windows_test.go`:
- Around line 18-26: Update the test setup and cleanup around the
pre-existing-account regression test to save, stub, and restore
windowsSandboxUserIsManaged and resetWindowsSandboxUserPassword alongside the
existing seams. Configure the stubs so execution reaches the injected group
failure, then assert that injected error is returned, ensuring the test is
deterministic and does not modify real account state.
---
Outside diff comments:
In `@internal/sandbox/windows_identity_rollback_windows_test.go`:
- Around line 145-164: Update TestWindowsACLAllowReadGrantsNoDelete to retain
the mode returned by windowsACLAccess and assert it equals windows.GRANT_ACCESS.
Also assert that the mask contains at least one contractually required
read-access bit, while preserving the existing checks that forbid delete and
write bits.
🪄 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: bd266c61-0a6f-4795-88c7-7d55ece45da1
📒 Files selected for processing (8)
internal/sandbox/windows_identity_rollback_windows_test.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.gointernal/sandbox/windows_network.gointernal/sandbox/windows_network_test.gointernal/sandbox/windows_setup_windows.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/sandbox/windows_identity_runtime_windows.go
- internal/sandbox/windows_network.go
- internal/sandbox/windows_setup_windows.go
- internal/sandbox/windows_network_test.go
- internal/sandbox/windows_identity_runtime_windows_test.go
- internal/sandbox/windows_identity_windows.go
- internal/sandbox/windows_identity_windows_test.go
|
@coderabbitai review Rebased onto the updated #808 head. The offline-group failure path now returns the account name for rollback, and |
|
✅ Action performedReview finished.
|
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
-
[P2] Scope dual-role setup rollback to accounts this run created
internal/sandbox/windows_identity_runtime_windows.go:242-266
provisionWindowsSandboxPrincipalForSetupalready avoids deleting a pre-existing principal on failure (created=falsein its innerundo), butsetupWindowsSandboxPrincipalappends an unconditionalremoveWindowsSandboxPrincipalForSetupfor every role that completed before a later step fails. If offline provisioning succeeds on an account that already existed and online provisioning or ACL application then fails, the outer rollback deletes the working offline principal anyway. That turns a partial re-setup failure into loss of a principal that was fine before the run. The single-principal path on #808 had a similar outer-rollback shape; dual-role provisioning makes this bite more often. Please mirror thecreatedcontract in the outer rollback, or only append removal closures for principals this run actually created. -
[P3] Update the mode-independent infra-plan test for the offline group
internal/sandbox/windows_online_offline_test.go:59
TestBuildWindowsNetworkInfraPlanIsModeIndependentstill requires exactly one identity SID. On a Windows host whereZeroSandboxOfflinealready exists,BuildWindowsNetworkInfraPlannow legitimately returns two SIDs, so this test fails even though the plan is correct. CI stays green because Linux/macOS jobs leave the hook nil and fresh Windows runners usually have no group yet. Please stubresolveWindowsSandboxOfflineGroupSIDHookthe way the new network tests do, or relax the assertion to match the new contract.
Needs maintainer decision
- Machine-global offline group and per-workspace setup markers
internal/sandbox/windows_network.go:79-87andinternal/sandbox/windows_setup.go:182-265
ZeroSandboxOfflineis machine-global by design, andBuildWindowsNetworkInfraPlanfolds its SID intoNetworkInfraHashwhenever the group exists. That means identity-enabled setup in one workspace changes the expected infra hash for every other sandbox home on the machine, so their markers read as out of date until each re-runs elevated setup, even if they never opted into the principal backend. That is a predictable consequence of the chosen design, not an accidental bug. Please confirm this cross-workspace coupling is acceptable for the experimental flag, and document it if so; otherwise keep the infra hash independent of a group another workspace created.
The principal backend stood down whenever the network was denied, which is the default, so opting into it left the restricted-token path doing all the work in normal use. The reason was that network denial is enforced by block filters keyed to the offline-marker SID, and a principal token cannot carry it: LogonUser builds a token from an account's real group memberships, and the marker is a synthetic capability SID. A real local group closes that gap. ZeroSandboxOffline is created by setup, the block filters name its SID alongside the marker, and a principal is denied the network by being a member. Each workspace therefore gets two accounts that differ only in that membership, and the command's network mode selects between them. A group rather than each principal's own SID because principals are per workspace: one filter set covers every offline principal on the machine instead of needing a filter per workspace. Both principals are provisioned together even though a given setup run sees one profile, because setup needs elevation and commands do not. Provisioning lazily would mean an unelevated command discovering it needs an account it cannot create. They also get identical filesystem access, so an approved network command sees the same filesystem as an ordinary one. Two orderings are load bearing. The network plan is now built AFTER provisioning, because the group it keys to is created there; planning first installed filters naming only the marker and left every offline principal with an open network while looking correctly set up. And the role tag sits before the workspace hash in the account name, so truncation at the 20 character limit eats hash characters rather than the tag, which would otherwise collide the two roles onto one account on exactly the workspaces most likely to truncate. Anything that is not an explicit allow maps to the offline principal, so an unrecognised mode loses the network rather than keeping it. The filter identity set is resolved rather than assumed, and stays absent until the group exists, so a machine that never provisions principals computes the same plan as before. That matters because the plan is hashed into the setup marker and re-derived on every command; an identity set that differed between setup and the command path would fail every command as out of date. Cost worth stating: this doubles the sandbox accounts on a machine, to two per workspace.
removeWindowsSandboxIdentity is called with a DERIVED name, so it could be pointed at a name that happens to belong to somebody else's local account. Deleting a user is not a recoverable mistake, and the only thing standing between the two cases was the name matching a pattern we generate ourselves. Raised by CodeRabbit against the test fixtures, but the production teardown path had the same hazard, so the guard belongs there rather than in the tests. The ownership check itself now lives on the base branch, which grew the same helper to stop provisioning ADOPTING a squatted account. This applies it to the other end: an account that is not ours is left alone rather than deleted. The gated tests get the protection for free, since their pre-clean goes through the same helper. Also appends the trimmed offline-group SID rather than the raw one. Worth noting the reported consequence does not hold: newWindowsWFPUserCondition canonicalises before converting, so a padded value would have been trimmed before reaching StringToSid. The resolver returns SID.String(), which never carries whitespace, so this is defensive tidying rather than a fix.
…ned account Two follow-ups from review, both on the same theme: a control that quietly does nothing looks identical to one that works. The network plan must be built AFTER provisioning, because provisioning creates the group the block filters name. Built first, the filters name only the offline marker and every offline principal has an open network while setup reports success. That ordering is invisible at the call site, so setup now checks the plan actually names the offline group before installing anything and refuses if it does not. A later refactor that moves the plan build back fails loudly instead of producing a security control that enforces nothing. The predicate is separate so it can be asserted directly: a plan carrying only the marker must read as uncovered, one carrying the group as covered, case differences must not read as missing, and a host with no group provisioned has no principal to miss and must not be refused. Making coverage always report true fails that test. Removal also reported plain success when it declined to delete an account Zero did not create. Leaving it alone is right, but telling an operator cleanup completed when a name they may care about was deliberately retained is not. That case is now a distinguishable sentinel, and teardown treats it as success, since "no principal of ours under this name" is the goal state either way.
Provisioning already declined to delete an account it had adopted, and the outer setup rollback then appended an unconditional removal for every role that got that far. With two roles that is the common case rather than an unlucky one: the offline role usually succeeds, so a failure in the online role or in ACL application destroyed a principal that was working before the run started. The removal closure is now only appended for a principal this run created. Threads the workspace key into the delete path as well. Ownership was proven from the account comment alone, which on a name collision belongs to a DIFFERENT workspace, so deleting it would have been the same unrecoverable mistake the check exists to prevent. Policy DenyWrite now reaches the principal ACL plan here too, matching the single-principal path. Fixes the mode-independence test, which required exactly one identity SID and so failed on any Windows host that already had ZeroSandboxOffline, where the plan legitimately carries two. CI never saw it because the Linux and macOS jobs leave the hook nil and a fresh Windows runner has no group. The hook is now pinned, and the test additionally asserts the property it is named for in the group-present case, including that the infra hash changes when the group appears, which is the cross-workspace coupling raised for a maintainer decision.
5db0e6e to
8c2eece
Compare
|
Both taken, in [P2] outer rollback scoped to what the run created. You were right that this is the same bug as #808's and that two roles make it bite more often. The removal closure is only appended for a principal this run created. I gave it a test rather than just the fix, and writing it corrected my own mental model. My first attempt failed the ACL apply on the first role, which aborts before the second is ever provisioned, so the rollback never had more than one principal to consider and the scenario you described was not being exercised at all. Failing the second role is what reaches it. Reverting the guard now fails with exactly the loss you predicted: That is the adopted offline principal being deleted because the online role failed. [P3] the mode-independence test. Fixed, and confirmed first: stubbing the hook to return a group SID reproduces the failure on a correct plan, two identity SIDs against an assertion demanding one. The hook is now pinned, and I extended the test to assert the property it is named for in the group-present case too, since pinning alone would have left that path untested. One more thing that came out of the rebase, related to your #808 P3 about name truncation. The ownership check on the delete path proved an account was Zero's from the comment marker alone. The comment now carries the full workspace key, so on a collision that account belongs to a DIFFERENT workspace, and deleting it would have been the same unrecoverable mistake the check exists to prevent. The key is threaded through Also carried the policy On the maintainer decisionNot resolving this one, but I have made it visible rather than leaving it implicit. @kevincodex1 that is the call: accept the cross-workspace coupling for an experimental flag and document it, or keep the infra hash independent of a group another workspace created. I do not think it should block review of the rest. |
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_online_offline_test.go`:
- Around line 102-104: Strengthen the assertions in the Windows sandbox test
around withGroupDeny.IdentitySIDs by verifying both plans’ identity lists
contain S-1-5-32-9999 in the second position, rather than checking only the
length. Preserve the existing count assertion if useful, and add a regression
assertion that detects duplicate or unrelated SIDs.
🪄 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: 7bd4492d-6a58-4e91-894a-001520dfac4f
📒 Files selected for processing (11)
internal/sandbox/windows_dualrole_rollback_windows_test.gointernal/sandbox/windows_identity_policy_windows_test.gointernal/sandbox/windows_identity_rollback_windows_test.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.gointernal/sandbox/windows_network.gointernal/sandbox/windows_network_test.gointernal/sandbox/windows_online_offline_test.gointernal/sandbox/windows_setup_windows.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sandbox/windows_network.go
- internal/sandbox/windows_network_test.go
- internal/sandbox/windows_identity_runtime_windows_test.go
- internal/sandbox/windows_identity_rollback_windows_test.go
- internal/sandbox/windows_identity_windows_test.go
- internal/sandbox/windows_setup_windows.go
- internal/sandbox/windows_identity_windows.go
- internal/sandbox/windows_identity_runtime_windows.go
| if len(withGroupDeny.IdentitySIDs) != 2 { | ||
| t.Fatalf("group present should add its SID, got %v", withGroupDeny.IdentitySIDs) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the resolved group SID, not just its count.
A duplicate offline-marker SID or unrelated SID would still pass this check. Verify both plans contain S-1-5-32-9999 as the second identity.
Proposed test strengthening
-if len(withGroupDeny.IdentitySIDs) != 2 {
- t.Fatalf("group present should add its SID, got %v", withGroupDeny.IdentitySIDs)
+if len(withGroupDeny.IdentitySIDs) != 2 || len(withGroupAllow.IdentitySIDs) != 2 {
+ t.Fatalf("group present should add its SID: deny=%v allow=%v", withGroupDeny.IdentitySIDs, withGroupAllow.IdentitySIDs)
+}
+if withGroupDeny.IdentitySIDs[1] != "S-1-5-32-9999" || withGroupAllow.IdentitySIDs[1] != "S-1-5-32-9999" {
+ t.Fatalf("group SID was not propagated: deny=%v allow=%v", withGroupDeny.IdentitySIDs, withGroupAllow.IdentitySIDs)
}As per coding guidelines, “add a regression test for behavior changes.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(withGroupDeny.IdentitySIDs) != 2 { | |
| t.Fatalf("group present should add its SID, got %v", withGroupDeny.IdentitySIDs) | |
| } | |
| if len(withGroupDeny.IdentitySIDs) != 2 || len(withGroupAllow.IdentitySIDs) != 2 { | |
| t.Fatalf("group present should add its SID: deny=%v allow=%v", withGroupDeny.IdentitySIDs, withGroupAllow.IdentitySIDs) | |
| } | |
| if withGroupDeny.IdentitySIDs[1] != "S-1-5-32-9999" || withGroupAllow.IdentitySIDs[1] != "S-1-5-32-9999" { | |
| t.Fatalf("group SID was not propagated: deny=%v allow=%v", withGroupDeny.IdentitySIDs, withGroupAllow.IdentitySIDs) | |
| } |
🤖 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_online_offline_test.go` around lines 102 - 104,
Strengthen the assertions in the Windows sandbox test around
withGroupDeny.IdentitySIDs by verifying both plans’ identity lists contain
S-1-5-32-9999 in the second position, rather than checking only the length.
Preserve the existing count assertion if useful, and add a regression assertion
that detects duplicate or unrelated SIDs.
Source: Coding guidelines
|
@coderabbitai review |
✅ Action performedReview finished.
|
Two review points on the tests added in the previous commit. The provisioning stub left resetWindowsSandboxUserPassword as a real call. Nothing under test reaches it any more, because rotation moved to the caller, but a test that resets a real managed account's password if the code ever moves back is not a risk worth carrying. It is now stubbed to fail the test instead, which also states the contract. The group-present assertion checked only that two identity SIDs were present. A duplicated offline marker or an unrelated SID would satisfy that while meaning something quite different, so it now pins both positions. Also drops a duplicated stub assignment left by the rebase.
|
Both taken in
The SID assertion is a fair catch and is now pinned by position rather than by count. Also dropped a duplicated stub assignment the rebase left behind. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@jatmn @anandh8x @gnanam1990 re-review please. Head is jatmn, both of yours are in, and writing the test for the first one corrected my own understanding: my initial attempt failed the ACL apply on the first role, which aborts before the second is provisioned, so the scenario you actually described was never being exercised. Failing the second role reaches it. Reverting the guard now gives Your maintainer-decision item is not resolved, deliberately. I made it visible instead: the test now asserts the infra hash changes when the offline group appears, so the cross-workspace coupling is pinned as intentional rather than something a later change could quietly drop. @kevincodex1 that one is yours, and I do not think it should block this review. This merges after #808, since its base is |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewed the latest stacked head f25ae08 against #808 at d67267f. The rollback hole from my previous review is fixed: offline-group attachment failures now return the role-specific username, and the injected failure test reaches that exact path. The dual-role outer rollback now removes only principals created by the current setup run, the mode-independent network test is host-independent and pins both SIDs, and the stale CodeRabbit thread is addressed by f25ae08.
The incremental offline/online design is good. This approval is for #812 itself; it should remain stacked and be refreshed after the two base-branch issues on #808 are fixed. The machine-global group changing every sandbox-home marker remains an explicit maintainer product decision, not a hidden logic defect in this patch.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Re-reviewed at f25ae089 on feat/windows-sandbox-offline-online, stacked on feat/windows-sandbox-identity. I still have issues that need to be addressed before this is ready.
What I withdraw from my earlier review
You are right that both of my earlier findings are in at f25ae089, and I verified them on the current head.
Outer dual-role rollback scoped to created. setupWindowsSandboxPrincipal now only appends removal for principals this run created, and TestDualRoleSetupRollbackSparesAdoptedPrincipals covers it. Your note about the test helped: failing the ACL apply on the first role never reached the scenario I cared about; failing the second role does, and reverting the guard reproduces removed [online offline], want [online] — exactly the adopted-principal loss I was trying to prevent.
Mode-independent infra-plan test. The offline-group hook is pinned, the group-present case is asserted, and the SID positions are pinned rather than just the count. That addresses my earlier concern.
I also verified the offline-group attachment failure path returns the role-specific username for rollback, and the CodeRabbit group-SID assertion you answered in f25ae089.
Merge blockers
- [P0] Rebase onto current
feat/windows-sandbox-identityand resolve conflicts
Since your re-review request, the base branch has moved again. GitHub now reportsmergeStateStatus: DIRTY/mergeable: CONFLICTINGagainstfeat/windows-sandbox-identityata1a0edc51ceaf24724cd9d09e5888124409808a1. That commit (fix(sandbox): keep an adopted principal's logon rights on rollback) is not in this branch's history.git merge-treeshows conflicts ininternal/sandbox/windows_identity_windows.goamong other hunks.
I am not going to treat green checks onf25ae089alone as enough here. We have already seen a clean-looking rebase produce a merged tree that did not build, and this is the same class of stacked-branch drift. Please rebase onto the current base, resolve the seam conflicts, and show the merged tree still cross-compiles for Windows before I sign off.
Remaining findings
-
[P2] The rebase needs to bring in
#808's adopted-principal logon-rights guard
internal/sandbox/windows_identity_runtime_windows.go:223-224
Onf25ae089,provisionWindowsSandboxPrincipalForSetup'sundo()still revokes logon rights wheneverrightsAttemptedis true, without checkingcreated. That is not new dual-role logic in this PR; it is a regression from being behind base.#808ata1a0edcalready fixed this withrightsAttempted && created, plus thegrantWindowsSandboxLogonRightsFn/revokeWindowsSandboxLogonRightsFnseams. Dual-role re-setup makes the missing guard bite more often, but the fix is to takea1a0edcduring the rebase. -
[P2] The rebase needs to bring in
#808's secret permission-denied fail-soft path
internal/sandbox/windows_identity_secret_windows.go:186-192
Onf25ae089,readWindowsSandboxSecretmaps onlyos.IsNotExisttoerrWindowsSandboxIdentityUnavailable. A permission error propagates as a hard failure throughwindowsSandboxPrincipalToken, so commands exit with "sandbox principal is provisioned but unusable" when setup ran under a different admin account than the command user. Again this is missinga1a0edc, not new work from dual-role provisioning. Please carry that fix and its test seam forward in the rebase. -
[P2] No migration path from
#808single-principal account names
internal/sandbox/windows_identity_windows.go:188-206
internal/sandbox/windows_identity_runtime_windows.go:106-111
#808derivedzero-sbx-<hash>. This branch deriveszero-sbx-d<hash>andzero-sbx-n<hash>. There is no adopt, rename, or cleanup of the legacy untagged name. Upgrading the stack without re-running elevated setup leaves the old account orphaned: role-tagged lookup misses, commands fail-soft to the restricted-token backend with no warning, and re-setup then creates two new accounts per workspace. I know this is still experimental and#808is not onmainyet, but anyone already testing the stack will hit this silently. Please add a migration path during provisioning or emit a stderr hint when a legacy account exists but the role-tagged lookup misses. -
[P3] Adopted offline principal can remain without
ZeroSandboxOfflinemembership after a failed group join
internal/sandbox/windows_identity_windows.go:599-602
internal/sandbox/windows_identity_runtime_windows.go:316-318
If offline provisioning adopts an existing managed account andaddWindowsSandboxUserToOfflineGroupthen fails, provisioning returnscreated=false, so neither the inner nor outer rollback removes the account. The narrow sequence is: adopt a principal that never had offline-group membership (for example a#808-era account) and have the first dual-role offline join refused by policy. If a readable secret still exists from a prior run, deny-mode commands can log on as that principal without offline-group membership, so WFP filters keyed to the offline-group SID may not block egress. Membership is the enforcement boundary in this design, so please either refuse to leave an offline-role principal without offline-group membership after provisioning, or fail closed at command time when the offline principal is not a member.
Needs maintainer decision
- Machine-global offline group invalidates other sandbox-home markers
internal/sandbox/windows_network.go:79-86
internal/sandbox/windows_setup.go:264-265
I am not asking you to resolve this in this PR. You said you made the coupling visible rather than implicit, and I agree that is the right move: the test now asserts the infra hash changes when the offline group appears, so a later change cannot drop the coupling quietly. I still think@kevincodex1needs to decide whether that cross-workspace marker invalidation is acceptable for the experimental flag and document it for operators, or whether the infra hash should stay independent of a group another workspace created. I do not think it should block review of the rest of this change.
4 participants
Stacked on #808, and targets that branch rather than main so the diff is only the new work. #808 should land first.
What this fixes
#808 shipped with the principal backend standing down whenever the network was denied. Deny is the default, so opting in left the restricted-token path doing all the work in ordinary use and the new identity only engaged for approved network commands. That was the honest choice at the time, but it made the feature close to inert.
The cause: network denial is enforced by block filters keyed to the offline-marker SID, and a principal token cannot carry it.
LogonUserbuilds a token from an account's real group memberships, and the marker is a synthetic capability SID with no account behind it.How
A real local group.
ZeroSandboxOfflineis created by setup, the block filters name its SID alongside the marker, and a principal is denied the network by being a member of it. Each workspace gets two accounts that differ only in that membership, and the command's network mode picks between them.A group rather than each principal's own SID because principals here are per workspace: one filter set covers every offline principal on the machine, instead of needing a filter per workspace and a setup re-run whenever a workspace appears.
Both principals are provisioned together even though a setup run only sees one profile. Setup needs elevation and commands do not, so provisioning lazily would mean an unelevated command discovering it needs an account it cannot create. They also get identical filesystem access, so an approved network command sees the same filesystem as an ordinary one.
Two orderings that are load bearing
The network plan is now built after provisioning. The group it keys to is created there. Planning first installed filters naming only the marker and left every offline principal with an open network while the machine looked correctly set up. That is the same class of silent-downgrade bug this PR exists to remove, so it is worth naming.
The role tag sits before the workspace hash in the account name. Truncation at the 20 character limit then eats hash characters rather than the tag. A lost tag would collide the two roles onto one account, on exactly the workspaces most likely to truncate, and the dangerous direction of that collision is an ordinary command silently getting the online principal.
Anything that is not an explicit allow maps to the offline principal, so an unrecognised mode loses the network rather than keeps it.
Setup-marker compatibility
The filter identity set is resolved, not assumed, and stays absent until the group exists. A machine that never provisions principals computes exactly the plan it computed before. This matters because the plan is hashed into the setup marker and re-derived on every command: an identity set that differed between setup and the command path would fail every command with "setup is out of date". There is a test for the hash changing when the group is present and for a lookup failure propagating rather than silently degrading to "no group".
Verification, and what is not verified
gofmt,go vet,GOOS=windows go vet, and builds for linux, darwin and windows are clean.Before I added the last three tests, the full
internal/sandboxsuite passed on this branch with the single expected failure: the test asserting the old stand-down, which this replaces.The three new tests have not been run locally. Smart App Control on this machine is enforcing and started refusing every freshly built unsigned test binary partway through, including via
go test, and turning it off is not something I am going to do to get a green local run. CI is the check for those. They areTestPrincipalRoleFollowsNetworkMode,TestWindowsSandboxUserNameSeparatesRoles, and the twoTestNetworkInfraPlan...cases, the last of which are portable and will run on the Linux and macOS jobs too.Also unchanged from #808: the privileged half is still unexercised here, so
LogonUserand the LSA rights need a clean elevated box.Cost worth stating
This doubles the sandbox accounts, to two per workspace. Given the concern already raised about local account creation being visible to endpoint protection and enterprise policy, that is a real increase and a deliberate trade for making network denial work under a separate identity.
Summary by CodeRabbit