diff --git a/internal/sandbox/windows_dualrole_rollback_windows_test.go b/internal/sandbox/windows_dualrole_rollback_windows_test.go new file mode 100644 index 000000000..381c06364 --- /dev/null +++ b/internal/sandbox/windows_dualrole_rollback_windows_test.go @@ -0,0 +1,161 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "testing" + + "golang.org/x/sys/windows" +) + +// The outer rollback must only delete principals this run created. +// +// Dual-role setup provisions offline then online. If the offline role adopts an +// account that already existed and anything afterwards fails, the outer rollback +// used to delete it anyway, turning a partial re-setup failure into the loss of +// a principal that was working before the run started. Two roles make that the +// common case rather than an unlucky one: the first role usually succeeds, so +// there is nearly always something for a later failure to destroy. +func TestDualRoleSetupRollbackSparesAdoptedPrincipals(t *testing.T) { + for name, testCase := range map[string]struct { + createdByRole map[windowsSandboxRole]bool + wantRemoved []windowsSandboxRole + }{ + "offline adopted, online created": { + createdByRole: map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: false, + windowsSandboxRoleOnline: true, + }, + // Only the one this run made. Deleting the adopted offline principal + // is the data loss this guards against. + wantRemoved: []windowsSandboxRole{windowsSandboxRoleOnline}, + }, + "both adopted": { + createdByRole: map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: false, + windowsSandboxRoleOnline: false, + }, + wantRemoved: nil, + }, + "both created": { + createdByRole: map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: true, + windowsSandboxRoleOnline: true, + }, + wantRemoved: []windowsSandboxRole{windowsSandboxRoleOffline, windowsSandboxRoleOnline}, + }, + } { + t.Run(name, func(t *testing.T) { + var removed []windowsSandboxRole + restoreDualRoleSeams(t, testCase.createdByRole, &removed) + + // Fail on the SECOND role, not the first. ACL application happens per + // role inside the loop, so failing the first aborts before the second + // is provisioned at all and the rollback never has more than one + // principal to consider. The case worth testing is the one review + // described: the first role succeeds, the second fails, and the + // question is whether the first gets destroyed on the way out. + // Count GRANT plans, not ACL calls. applyWindowsPrincipalACLs revokes + // this trustee's existing ACEs before applying the new plan, so there + // are two calls per role now. The contract under test is "the second + // ROLE fails"; counting raw calls would fail the first role's grant + // instead and prove something else entirely. + grants := 0 + applyWindowsACLPlanFn = func(plan WindowsACLPlan) (func() error, error) { + if len(plan.Entries) > 0 && plan.Entries[0].Action == windowsACLRevoke { + return func() error { return nil }, nil + } + grants++ + if grants < 2 { + return func() error { return nil }, nil + } + return nil, errors.New("ACL apply refused") + } + + if _, err := setupWindowsSandboxPrincipal(windowsSandboxTestConfig()); err == nil { + t.Fatal("setup reported success despite an injected ACL failure") + } + assertRolesEqual(t, removed, testCase.wantRemoved) + }) + } +} + +// The same contract on the success path: the rollback handed back to the caller +// for a LATER setup step to invoke must be just as reluctant. +func TestDualRoleSetupReturnedRollbackSparesAdoptedPrincipals(t *testing.T) { + var removed []windowsSandboxRole + restoreDualRoleSeams(t, map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: false, + windowsSandboxRoleOnline: true, + }, &removed) + + rollback, err := setupWindowsSandboxPrincipal(windowsSandboxTestConfig()) + if err != nil { + t.Fatalf("setup: %v", err) + } + if len(removed) != 0 { + t.Fatalf("setup removed principals before anything failed: %v", removed) + } + if err := rollback(); err != nil { + t.Fatalf("rollback: %v", err) + } + assertRolesEqual(t, removed, []windowsSandboxRole{windowsSandboxRoleOnline}) +} + +func restoreDualRoleSeams(t *testing.T, createdByRole map[windowsSandboxRole]bool, removed *[]windowsSandboxRole) { + t.Helper() + prevProvision := provisionWindowsSandboxPrincipalForSetupFn + prevApply := applyWindowsACLPlanFn + prevRemove := removeWindowsSandboxPrincipalForSetupFn + t.Cleanup(func() { + provisionWindowsSandboxPrincipalForSetupFn = prevProvision + applyWindowsACLPlanFn = prevApply + removeWindowsSandboxPrincipalForSetupFn = prevRemove + }) + + provisionWindowsSandboxPrincipalForSetupFn = func(_ WindowsSandboxCommandConfig, role windowsSandboxRole) (windowsSandboxIdentity, bool, error) { + sid, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + return windowsSandboxIdentity{}, false, err + } + return windowsSandboxIdentity{Username: "zero-sbx-test", SID: sid}, createdByRole[role], nil + } + applyWindowsACLPlanFn = func(WindowsACLPlan) (func() error, error) { + return func() error { return nil }, nil + } + removeWindowsSandboxPrincipalForSetupFn = func(_ WindowsSandboxCommandConfig, role windowsSandboxRole) error { + *removed = append(*removed, role) + return nil + } +} + +func windowsSandboxTestConfig() WindowsSandboxCommandConfig { + return WindowsSandboxCommandConfig{ + SandboxHome: `C:\sandboxhome`, + CommandCWD: `C:\ws`, + WorkspaceRoots: []string{`C:\ws`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\ws`}}, + }, + }, + } +} + +func assertRolesEqual(t *testing.T, got []windowsSandboxRole, want []windowsSandboxRole) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("removed %v, want %v", got, want) + } + seen := map[windowsSandboxRole]bool{} + for _, role := range got { + seen[role] = true + } + for _, role := range want { + if !seen[role] { + t.Fatalf("removed %v, want %v", got, want) + } + } +} diff --git a/internal/sandbox/windows_identity_policy_windows_test.go b/internal/sandbox/windows_identity_policy_windows_test.go index f1137cebd..3a7dcd42a 100644 --- a/internal/sandbox/windows_identity_policy_windows_test.go +++ b/internal/sandbox/windows_identity_policy_windows_test.go @@ -115,7 +115,7 @@ func findPrincipalACLEntry(plan WindowsACLPlan, action WindowsACLAction, path st // read the absent secret as "not provisioned" and quietly fell back to the // weaker backend. func TestProvisionWindowsSandboxIdentityDefersPasswordRotation(t *testing.T) { - stubWindowsProvisioning(t, true, nil, nil) + stubWindowsProvisioning(t, true, nil, nil, nil) rotated := false previous := resetWindowsSandboxUserPasswordFn @@ -125,7 +125,7 @@ func TestProvisionWindowsSandboxIdentityDefersPasswordRotation(t *testing.T) { return nil } - if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey"); err != nil { + if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline); err != nil { t.Fatalf("provisioning an adopted account: %v", err) } if rotated { @@ -147,7 +147,7 @@ func TestWindowsSandboxUserCommentDistinguishesWorkspaces(t *testing.T) { } // The names DO collide, which is the whole reason the comment has to carry // the key. If this stops being true the test is no longer covering anything. - if windowsSandboxUserName("aaaaaaaaaaaabbbbbbbb") != windowsSandboxUserName("aaaaaaaaaaaacccccccc") { + if windowsSandboxUserName("aaaaaaaaaaaabbbbbbbb", windowsSandboxRoleOffline) != windowsSandboxUserName("aaaaaaaaaaaacccccccc", windowsSandboxRoleOffline) { t.Skip("account names no longer collide for these keys; revisit what this test is for") } } @@ -168,7 +168,7 @@ func TestSetupRollbackRevokesRightsOnlyForCreatedPrincipals(t *testing.T) { "created principal": {existed: false, wantRevoked: true}, } { t.Run(name, func(t *testing.T) { - stubWindowsProvisioning(t, testCase.existed, nil, nil) + stubWindowsProvisioning(t, testCase.existed, nil, nil, nil) revoked := false prevGrant, prevRevoke := grantWindowsSandboxLogonRightsFn, revokeWindowsSandboxLogonRightsFn @@ -189,7 +189,7 @@ func TestSetupRollbackRevokesRightsOnlyForCreatedPrincipals(t *testing.T) { SandboxHome: t.TempDir(), WorkspaceRoots: []string{`C:\ws`}, } - if _, _, err := provisionWindowsSandboxPrincipalForSetup(config); err == nil { + if _, _, err := provisionWindowsSandboxPrincipalForSetup(config, windowsSandboxRoleOffline); err == nil { t.Fatal("provisioning reported success despite an injected grant failure") } if revoked != testCase.wantRevoked { @@ -246,7 +246,7 @@ func TestLookupWindowsSandboxIdentityRejectsForeignWorkspace(t *testing.T) { askedKey = workspaceKey return false, nil } - _, err := lookupWindowsSandboxIdentity("workspacekey") + _, err := lookupWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if err == nil { t.Fatal("lookup accepted an account belonging to another workspace") } @@ -267,7 +267,7 @@ func TestLookupWindowsSandboxIdentityAbsentAccountIsUnavailable(t *testing.T) { t.Fatal("ownership must not be consulted for an account that does not resolve") return false, nil } - if _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey"); !errors.Is(err, errWindowsSandboxIdentityUnavailable) { + if _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey", windowsSandboxRoleOffline); !errors.Is(err, errWindowsSandboxIdentityUnavailable) { t.Fatalf("absent account returned %v, want errWindowsSandboxIdentityUnavailable", err) } } @@ -362,13 +362,13 @@ func TestSetupRuntimeRootWithoutWorkspaceIsNotAnError(t *testing.T) { // exists to withhold: rewriting the ACLs confining it, reading the secret locked // to the invoking user, and stopping Zero. func TestProvisionWindowsSandboxIdentityRefusesPrivilegedAccount(t *testing.T) { - stubWindowsProvisioning(t, true, nil, nil) + stubWindowsProvisioning(t, true, nil, nil, nil) previous := windowsSandboxUserIsPrivilegedFn t.Cleanup(func() { windowsSandboxUserIsPrivilegedFn = previous }) windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return true, nil } - _, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + _, _, created, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if !errors.Is(err, errWindowsSandboxPrivilegedAccount) { t.Fatalf("provisioning adopted a privileged account, err = %v", err) } @@ -379,13 +379,13 @@ func TestProvisionWindowsSandboxIdentityRefusesPrivilegedAccount(t *testing.T) { // The ordinary adopted account is unaffected. func TestProvisionWindowsSandboxIdentityAdoptsUnprivilegedAccount(t *testing.T) { - stubWindowsProvisioning(t, true, nil, nil) + stubWindowsProvisioning(t, true, nil, nil, nil) previous := windowsSandboxUserIsPrivilegedFn t.Cleanup(func() { windowsSandboxUserIsPrivilegedFn = previous }) windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return false, nil } - if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey"); err != nil { + if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline); err != nil { t.Fatalf("an unprivileged managed account must still be adopted: %v", err) } } diff --git a/internal/sandbox/windows_identity_privilege_recheck_windows_test.go b/internal/sandbox/windows_identity_privilege_recheck_windows_test.go index 9544b867a..796c80831 100644 --- a/internal/sandbox/windows_identity_privilege_recheck_windows_test.go +++ b/internal/sandbox/windows_identity_privilege_recheck_windows_test.go @@ -26,7 +26,7 @@ func TestLookupPrincipalForCommandRefusesAnAccountThatBecamePrivileged(t *testin return true, nil } - _, err = lookupWindowsSandboxPrincipalForCommand("workspace-key") + _, err = lookupWindowsSandboxPrincipalForCommand("workspace-key", windowsSandboxRoleOnline) if !errors.Is(err, errWindowsSandboxPrivilegedAccount) { t.Fatalf("err = %v, want errWindowsSandboxPrivilegedAccount", err) } @@ -49,7 +49,7 @@ func TestLookupPrincipalForCommandAcceptsAnUnprivilegedAccount(t *testing.T) { restoreLookupSeams(t, sid) windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return false, nil } - identity, err := lookupWindowsSandboxPrincipalForCommand("workspace-key") + identity, err := lookupWindowsSandboxPrincipalForCommand("workspace-key", windowsSandboxRoleOnline) if err != nil { t.Fatalf("lookupWindowsSandboxPrincipalForCommand: %v", err) } @@ -72,7 +72,7 @@ func TestLookupIdentityItselfDoesNotConsultPrivilege(t *testing.T) { return true, nil } - if _, err := lookupWindowsSandboxIdentity("workspace-key"); err != nil { + if _, err := lookupWindowsSandboxIdentity("workspace-key", windowsSandboxRoleOnline); err != nil { t.Fatalf("lookupWindowsSandboxIdentity: %v", err) } } diff --git a/internal/sandbox/windows_identity_rollback_windows_test.go b/internal/sandbox/windows_identity_rollback_windows_test.go index e8593b816..25210b76d 100644 --- a/internal/sandbox/windows_identity_rollback_windows_test.go +++ b/internal/sandbox/windows_identity_rollback_windows_test.go @@ -13,23 +13,39 @@ import ( // function can run on an ordinary machine. Every one of them needs an elevated // caller and a real local account, so without this the test would stop at the // first call and never reach the behaviour it is named for. -func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, sidErr error) { +func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, offlineErr error, sidErr error) { t.Helper() prevGroup, prevUser := ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn + prevOfflineGroup := ensureWindowsSandboxOfflineGroupFn prevAdd, prevSID := addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn prevManaged := windowsSandboxUserIsManagedFn + prevOffline := addWindowsSandboxUserToOfflineGroupFn + prevReset := resetWindowsSandboxUserPasswordFn t.Cleanup(func() { ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn = prevGroup, prevUser + ensureWindowsSandboxOfflineGroupFn = prevOfflineGroup addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn = prevAdd, prevSID windowsSandboxUserIsManagedFn = prevManaged + addWindowsSandboxUserToOfflineGroupFn = prevOffline + resetWindowsSandboxUserPasswordFn = prevReset }) ensureWindowsSandboxGroupFn = func() error { return nil } + ensureWindowsSandboxOfflineGroupFn = func() error { return nil } // Adopted accounts are ours in these tests; the ownership check is a real // syscall and would otherwise refuse before the code under test runs. windowsSandboxUserIsManagedFn = func(string, string) (bool, error) { return true, nil } + // Stubbed even though provisioning no longer rotates: rotation moved to the + // caller, so nothing under test reaches this today. Left in so that if it + // ever moves back, these tests fail rather than resetting the password of a + // real managed account that happens to exist on the machine running them. + resetWindowsSandboxUserPasswordFn = func(string, string) error { + t.Fatal("provisioning must not rotate an account password; rotation belongs to the caller, immediately before the secret is written") + return nil + } ensureWindowsSandboxUserFn = func(string, string, string) (bool, error) { return existed, nil } addWindowsSandboxUserToGroupFn = func(string) error { return groupErr } + addWindowsSandboxUserToOfflineGroupFn = func(string) error { return offlineErr } resolveWindowsSandboxSIDFn = func(username string) (*windows.SID, error) { if sidErr != nil { return nil, sidErr @@ -46,19 +62,22 @@ func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, sidErr // most: it is the enforcement boundary and it can fail under local policy. func TestProvisionWindowsSandboxIdentityReturnsNameForRollback(t *testing.T) { groupFailure := errors.New("group attachment refused by policy") + offlineFailure := errors.New("offline group attachment refused by policy") sidFailure := errors.New("sid lookup failed") for name, testCase := range map[string]struct { - groupErr error - sidErr error + groupErr error + offlineErr error + sidErr error }{ - "group attachment fails": {groupErr: groupFailure}, - "sid resolution fails": {sidErr: sidFailure}, + "group attachment fails": {groupErr: groupFailure}, + "offline group attachment fails": {offlineErr: offlineFailure}, + "sid resolution fails": {sidErr: sidFailure}, } { t.Run(name, func(t *testing.T) { - stubWindowsProvisioning(t, false, testCase.groupErr, testCase.sidErr) + stubWindowsProvisioning(t, false, testCase.groupErr, testCase.offlineErr, testCase.sidErr) - identity, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + identity, _, created, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if err == nil { t.Fatal("provisioning reported success despite an injected failure") } @@ -69,7 +88,7 @@ func TestProvisionWindowsSandboxIdentityReturnsNameForRollback(t *testing.T) { if identity.Username == "" { t.Fatal("identity carries no username, so the rollback deletes \"\" and strands the account") } - if want := windowsSandboxUserName("workspacekey"); identity.Username != want { + if want := windowsSandboxUserName("workspacekey", windowsSandboxRoleOffline); identity.Username != want { t.Fatalf("username = %q, want %q", identity.Username, want) } }) @@ -80,9 +99,9 @@ func TestProvisionWindowsSandboxIdentityReturnsNameForRollback(t *testing.T) { // failed. created=false is what stops the rollback turning a partial failure // into the loss of a working principal from an earlier setup. func TestProvisionWindowsSandboxIdentityDoesNotClaimPreexistingAccount(t *testing.T) { - stubWindowsProvisioning(t, true, errors.New("group attachment refused"), nil) + stubWindowsProvisioning(t, true, errors.New("group attachment refused"), nil, nil) - _, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + _, _, created, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if err == nil { t.Fatal("provisioning reported success despite an injected failure") } diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 7b1ca58a4..644961a25 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -65,17 +65,26 @@ func windowsSandboxWorkspaceKey(workspaceRoots []string) string { // machine with nothing provisioned the lookup declines anyway, which would let a // missing guard here pass unnoticed. func windowsSandboxPrincipalEligible(config WindowsSandboxCommandConfig) bool { - if !windowsSandboxIdentityEnabled(config.Env) { - return false - } - // Network denial is enforced by WFP filters keyed to the offline-marker SID, - // which the restricted token carries and a principal token cannot: LogonUser - // mints a token for the account, not for a synthetic capability SID. Using a - // principal here would leave those filters matching nothing and silently drop - // network enforcement, which is a worse trade than the read confinement it - // buys. Fall back to the restricted token, which still enforces the network, - // until the filters are also keyed to the principal's own SID. - return config.PermissionProfile.Network.Mode != NetworkDeny + return windowsSandboxIdentityEnabled(config.Env) +} + +// windowsSandboxRoleForNetwork maps a command's network mode onto the principal +// that enforces it. +// +// The two roles differ only in membership of the offline group, which the block +// filters are keyed to. That indirection exists because network denial cannot be +// expressed on a logon token the way it is on a restricted token: the restricted +// token carries the offline-marker SID as a restricting SID, and LogonUser has +// no equivalent, since it builds a token from the account's real memberships. +// +// Defaulting anything that is not an explicit allow to the offline principal is +// deliberate. A mode this does not recognise should lose the network, not keep +// it. +func windowsSandboxRoleForNetwork(mode NetworkMode) windowsSandboxRole { + if mode == NetworkAllow { + return windowsSandboxRoleOnline + } + return windowsSandboxRoleOffline } // windowsSandboxPrincipalToken returns a token for this workspace's sandbox @@ -91,7 +100,11 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T return 0, false, nil } key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) - identity, err := lookupWindowsSandboxPrincipalForCommand(key) + // The network mode picks the principal. Both are provisioned by setup; the + // offline one is in the group the block filters match, so choosing here is + // what enforces the mode. + role := windowsSandboxRoleForNetwork(config.PermissionProfile.Network.Mode) + identity, err := lookupWindowsSandboxPrincipalForCommandFn(key, role) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // Not provisioned: fall back quietly, this is the default state. @@ -102,11 +115,31 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T // operator has to see, not a reason to pretend setup never ran. return 0, false, err } + // Selecting the offline account is only half of what denies it the network. + // The WFP filters match the OFFLINE GROUP'S SID, so an account that has + // drifted out of that group — local policy, an administrator, a re-setup that + // could not re-add it — still logs on from a stored secret and its token no + // longer satisfies the filter condition. It would get full egress under a + // profile that asked for none. Re-check the membership the mode depends on + // rather than trusting the marker written when setup last succeeded. + if role == windowsSandboxRoleOffline { + member, err := windowsSandboxUserInLocalGroupFn(identity.Username, windowsSandboxOfflineGroupName) + if err != nil { + return 0, false, err + } + if !member { + // Fail closed toward the weaker-but-still-denied path: the restricted + // token carries the offline marker the same filters match, so egress + // stays blocked. Handing back this principal token would not. + warnWindowsSandboxOfflineMembershipMissing(identity.Username) + return 0, false, nil + } + } secretPath, err := windowsSandboxSecretPath(config.SandboxHome, identity.Username) if err != nil { return 0, false, err } - password, err := readWindowsSandboxSecret(secretPath) + password, err := readWindowsSandboxSecretFn(secretPath) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // The account exists but its password does not. Setup was interrupted @@ -151,6 +184,19 @@ var warnWindowsSandboxPrincipalUnavailable = func(username string) { var windowsSandboxPrincipalWarnOnce sync.Once +// Seamed so the dual-role setup rollback can be exercised without an elevated +// machine. The contract under test is which principals the outer rollback is +// willing to delete, which is a data-loss decision and the one thing here worth +// proving rather than reasoning about. +var ( + provisionWindowsSandboxPrincipalForSetupFn = provisionWindowsSandboxPrincipalForSetup + removeWindowsSandboxPrincipalForSetupFn = removeWindowsSandboxPrincipalForSetup + // applyWindowsACLPlanFn lives with the other identity seams in + // windows_identity_windows.go — the ACL-ordering guarantee it exists for + // predates the dual-role split, and two declarations of the same seam would + // let a test stub one while production used the other. +) + // provisionWindowsSandboxPrincipalForSetup does the elevated half: create the // account, grant it the batch logon right, and store its password locked to the // invoking user. Called from `zero sandbox setup`. @@ -158,9 +204,9 @@ var windowsSandboxPrincipalWarnOnce sync.Once // The password is written BEFORE the caller applies any ACL plan, so a setup // that fails partway leaves a principal that can at least be logged on and // therefore cleaned up, rather than an account nothing holds the secret for. -func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) (windowsSandboxIdentity, bool, error) { +func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig, role windowsSandboxRole) (windowsSandboxIdentity, bool, error) { key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) - identity, password, created, err := provisionWindowsSandboxIdentityFn(key) + identity, password, created, err := provisionWindowsSandboxIdentityFn(key, role) // Undo whatever this run actually did, in reverse, on any failure after the // account exists. Without it a failure between creating the account and @@ -176,7 +222,7 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig rotated := false // Resolved from the account name rather than the identity, so it is known // before anything can fail. - secretPath, secretPathErr := windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key)) + secretPath, secretPathErr := windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key, role)) undo := func() error { // Only when this run invalidated it. The secret is removed if this run // created the account, or if it rotated an existing account's password, @@ -225,7 +271,7 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig _ = revokeWindowsSandboxLogonRightsFn(identity.SID) } if created { - _ = removeWindowsSandboxIdentity(identity.Username) + _ = removeWindowsSandboxIdentity(identity.Username, key) } return cleanupErr } @@ -274,71 +320,95 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig // naming a SID that no longer resolves, which is the orphaned-entry residue this // model exists to avoid. func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() error, error) { - identity, created, err := provisionWindowsSandboxPrincipalForSetup(config) - if err != nil { - return nil, err - } - // Scoped to what this run created, the same contract provisioning already - // applies to its own rollback. - // - // Unconditional removal here meant a transient ACL failure during a re-run of - // elevated setup deleted a principal that was working before the run started, - // taking its secret and logon rights with it. Provisioning was careful not to - // do that and then this undid the care one level up. A pre-existing principal - // is left alone: its ACEs are still reverted, since this run applied them, - // but the account itself is not this run's to destroy. - removePrincipal := func() error { - if !created { - return nil + // Both roles are provisioned regardless of the profile this setup ran with. + // Setup needs elevation and a command does not, so a command whose network + // mode differs from the one setup happened to see must still find a principal + // waiting; provisioning lazily would mean an unelevated command discovering it + // needs an account it cannot create. + roles := []windowsSandboxRole{windowsSandboxRoleOffline, windowsSandboxRoleOnline} + + var undo []func() error + rollback := func() error { + // Unwind in reverse, and keep going after a failure so one broken step + // cannot strand everything behind it. The first error is reported. + var firstErr error + for i := len(undo) - 1; i >= 0; i-- { + if err := undo[i](); err != nil && firstErr == nil { + firstErr = err + } } - return removeWindowsSandboxPrincipalForSetup(config) + return firstErr } filesystem := config.PermissionProfile.FileSystem - writeRoots := filesystem.WriteRoots - // The runtime tree has to be granted here, at setup, because nothing grants it - // later. + // Resolved once, before the loop: both principals need write access to the + // same runtime tree. // - // permissionProfileWithRuntime appends the per-workspace runtime root to - // WriteRoots on every COMMAND, and redirects HOME, GOCACHE, npm_config_cache - // and friends into it. That root lives under the user cache, not the - // workspace, so the profile setup sees never contains it. On the - // restricted-token path that costs nothing, since the child still runs as the - // caller and already has rights there. A principal is a separate local account - // with none, so without this every npm install, go build or pip install fails - // on a cache write with a bare ACCESS_DENIED and nothing pointing at the - // sandbox as the cause. - if runtimeRoot, err := setupWindowsSandboxRuntimeRoot(config); err != nil { - _ = removePrincipal() - return nil, err - } else if runtimeRoot != "" { - writeRoots = append(append([]WritableRoot{}, writeRoots...), WritableRoot{Root: runtimeRoot}) - } - revertACL, err := applyWindowsPrincipalACLs(identity.SID.String(), filesystem, writeRoots) + // permissionProfileWithRuntime appends this root to WriteRoots on every + // COMMAND and redirects HOME, GOCACHE, npm_config_cache and friends into it, + // but it lives under the user cache rather than the workspace, so the profile + // setup sees never contains it. On the restricted-token path that costs + // nothing, since the child still runs as the caller. A principal is a separate + // local account with none of those rights, so without this every npm install, + // go build or pip install fails on a cache write with a bare ACCESS_DENIED and + // nothing pointing at the sandbox as the cause. + runtimeRoot, err := setupWindowsSandboxRuntimeRoot(config) if err != nil { - _ = removePrincipal() return nil, err } - return func() error { - aclErr := revertACL() - // Remove the principal even when the ACL revert failed, so a broken - // rollback does not also strand an account; report the ACL error since it - // is the one that leaves state behind. - removeErr := removePrincipal() - if aclErr != nil { - return aclErr + + for _, role := range roles { + identity, created, err := provisionWindowsSandboxPrincipalForSetupFn(config, role) + if err != nil { + _ = rollback() + return nil, err } - return removeErr - }, nil + // Only for a principal this run created. + // + // Appending unconditionally meant that if the offline role adopted an + // account that already existed and the online role, or any later step, + // then failed, the outer rollback deleted a principal that was working + // before setup started. Dual-role provisioning makes that likely rather + // than unlucky: the first role usually succeeds, so there is almost + // always something for a later failure to destroy. This mirrors the + // contract provisioning already applies to its own inner rollback. + if created { + undo = append(undo, func() error { return removeWindowsSandboxPrincipalForSetupFn(config, role) }) + } + + // Both principals get the same filesystem access. They differ only in + // network reach, so granting the offline one less would make an approved + // network command see a different filesystem than an ordinary one. + writeRoots := filesystem.WriteRoots + if runtimeRoot != "" { + writeRoots = append(append([]WritableRoot{}, writeRoots...), WritableRoot{Root: runtimeRoot}) + } + // Revokes this trustee's existing ACEs on the paths the plan touches + // before applying it, so a re-run after narrowing a root does not leave + // the wider grant behind. Per role: the two principals are separate + // trustees and revoking one must not disturb the other. + revertACL, err := applyWindowsPrincipalACLs(identity.SID.String(), filesystem, writeRoots) + if err != nil { + _ = rollback() + return nil, err + } + // ACEs are reverted before the account they name is deleted, because + // removing the account first leaves ACEs naming a SID that no longer + // resolves, which is the orphaned residue this model exists to avoid. + // Appending after the removal closure puts it earlier in the reverse + // unwind, which is what gets that ordering. + undo = append(undo, revertACL) + } + return rollback, nil } -// removeWindowsSandboxPrincipalForSetup retires a workspace's principal in the +// removeWindowsSandboxPrincipalForSetup retires one role's principal in the // order that leaves nothing behind: secret, then ACEs, then LSA logon rights, // then the account itself. Everything keyed to the SID has to go while the SID // still resolves. -func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) error { +func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig, role windowsSandboxRole) error { key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) - username := windowsSandboxUserName(key) + username := windowsSandboxUserName(key, role) secretPath, err := windowsSandboxSecretPath(config.SandboxHome, username) if err != nil { return err @@ -351,12 +421,14 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e // which is the same orphaned residue the trustee-keyed ACE revocation exists // to avoid. A principal that was never provisioned has no SID to resolve and // nothing to revoke, so that case is not an error. - if identity, err := lookupWindowsSandboxIdentity(windowsSandboxWorkspaceKey(config.WorkspaceRoots)); err == nil { + if identity, err := lookupWindowsSandboxIdentity(key, role); err == nil { // ACEs first, for the same reason: once the account is gone its SID stops // resolving and every ACE naming it becomes an orphaned raw-SID entry on // the user's own tree, which is precisely the residue the capability-SID // model left behind and this one exists to avoid. Revocation is by - // trustee, so it clears grants written by older versions too. + // trustee, so it clears grants written by older versions too, and it is + // scoped to THIS role's principal — the other role is a separate trustee + // whose ACEs must survive one role being retired. // // Failing to revoke is not fatal. A path the user has since deleted or // renamed cannot be cleaned, and refusing to remove the account over it @@ -375,7 +447,14 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e } else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) { return err } - return removeWindowsSandboxIdentity(username) + // A foreign account under our derived name is left in place deliberately, and + // that is the goal state for teardown: no principal of ours exists under it. + // Surfacing it as a teardown failure would make setup rollback report an + // error for having correctly declined to delete somebody else's account. + if err := removeWindowsSandboxIdentity(username, key); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { + return err + } + return nil } // setupWindowsSandboxRuntimeRoot resolves this workspace's runtime root and @@ -405,13 +484,31 @@ func windowsSandboxRuntimeRootPath(config WindowsSandboxCommandConfig) (string, if err != nil { return "", fmt.Errorf("resolve user cache directory for sandbox runtime: %w", err) } - // Same canonicalization as the workspace root above: sandboxRuntimeRootFor - // compares them, so they have to be the same spelling of the same path. + // Same canonicalization as the workspace root above: the derivation compares + // them, so they have to be the same spelling of the same path. cacheRoot = canonicalSandboxWorkspaceRoot(cacheRoot) if cacheRoot == "" || cacheRoot == "." { return "", errors.New("user cache directory is unavailable for sandbox runtime") } - return sandboxRuntimeRootFor(workspaceRoot, cacheRoot) + // Deliberately the deterministic derivation and NOT sandboxRuntimeRootFor. + // + // sandboxRuntimeRootFor falls back to os.MkdirTemp when the cache lives + // inside the workspace, and memoizes that only in-process. Elevated setup is + // its own process, so it would grant the principals an ACE on temp root A, + // and the next command — a new process — would derive temp root B and hit + // ACCESS_DENIED on ordinary cache writes with nothing pointing at the cause. + // Teardown, being a third process, would clean a third directory. + // + // When the deterministic root is unusable there is no name both sides can + // agree on, so this reports "no runtime root" rather than inventing one. The + // principal then simply gets no ACE here: it loses the runtime tree, which is + // a degraded sandbox rather than a broken one, and it is the same answer + // teardown already gives. + root, ok := deterministicSandboxRuntimeRoot(workspaceRoot, cacheRoot) + if !ok { + return "", nil + } + return root, nil } // windowsSandboxDeterministicRuntimeRootPath names the cache-derived runtime @@ -584,3 +681,31 @@ var ( removeWindowsSandboxSecretFn = removeWindowsSandboxSecret writeWindowsSandboxSecretFn = writeWindowsSandboxSecret ) + +// warnWindowsSandboxOfflineMembershipMissing reports the one drift that would +// otherwise silently hand a no-network profile full egress. +// +// Separate from the missing-secret warning because the remedy differs and +// because this one is a containment failure rather than a setup gap: the +// account is fine, its group membership is not. Once per process, for the same +// reason as its sibling — this sits on the command path. +var warnWindowsSandboxOfflineMembershipMissing = func(username string) { + windowsSandboxOfflineMembershipWarnOnce.Do(func() { + fmt.Fprintf(os.Stderr, + "[zero] sandbox principal %q is no longer a member of %q, which is the group the network "+ + "block filters match. Falling back to the restricted-token sandbox, which still denies "+ + "the network but does not confine reads. "+ + "Re-run `zero sandbox setup` from an elevated terminal to restore it.\n", + username, windowsSandboxOfflineGroupName) + }) +} + +var windowsSandboxOfflineMembershipWarnOnce sync.Once + +// Seam for the principal lookup, so the command path's mode-enforcement checks +// are reachable in tests without a provisioned machine. +var lookupWindowsSandboxPrincipalForCommandFn = lookupWindowsSandboxPrincipalForCommand + +// Seam for the secret read on the command path, so the mode-enforcement gates +// ahead of it can be exercised without a provisioned secret on disk. +var readWindowsSandboxSecretFn = readWindowsSandboxSecret diff --git a/internal/sandbox/windows_identity_runtime_windows_test.go b/internal/sandbox/windows_identity_runtime_windows_test.go index 26f2852a8..c88f797e0 100644 --- a/internal/sandbox/windows_identity_runtime_windows_test.go +++ b/internal/sandbox/windows_identity_runtime_windows_test.go @@ -108,10 +108,10 @@ func TestWindowsSandboxWorkspaceKeyIsStableAndDistinct(t *testing.T) { // filters matching nothing, so the principal backend must stand down whenever // the network is denied rather than silently trading network enforcement for // read confinement. -// The eligibility predicate is asserted rather than the token lookup, because on -// a machine with no principal provisioned the lookup declines for its own reasons -// and would report success here whether or not the guard existed. -func TestPrincipalBackendDefersToRestrictedTokenWhenNetworkDenied(t *testing.T) { +// The backend now runs under both network modes, so the opt-in is the only thing +// eligibility turns on. The mode selects which principal is used, not whether one +// is used at all. +func TestPrincipalBackendEligibilityTurnsOnlyOnTheOptIn(t *testing.T) { eligible := func(mode NetworkMode, optIn string) bool { config := WindowsSandboxCommandConfig{ SandboxHome: t.TempDir(), @@ -122,15 +122,33 @@ func TestPrincipalBackendDefersToRestrictedTokenWhenNetworkDenied(t *testing.T) return windowsSandboxPrincipalEligible(config) } - if eligible(NetworkDeny, "1") { - t.Fatal("principal backend eligible with the network denied; the WFP filters key on the offline-marker SID, which a logon token does not carry, so egress would be unenforced") + for _, mode := range []NetworkMode{NetworkDeny, NetworkAllow} { + if !eligible(mode, "1") { + t.Fatalf("principal backend refused with network %q; the mode should choose a principal, not disable the backend", mode) + } + if eligible(mode, "0") { + t.Fatalf("principal backend eligible without the opt-in for network %q", mode) + } + } +} + +// The network mode is enforced by WHICH principal runs, because the offline one +// is a member of the group the block filters match. Picking the wrong role is +// therefore a silent loss of network enforcement, so the mapping is asserted +// directly rather than inferred from a token that only exists on a provisioned +// machine. +func TestPrincipalRoleFollowsNetworkMode(t *testing.T) { + if got := windowsSandboxRoleForNetwork(NetworkDeny); got != windowsSandboxRoleOffline { + t.Fatalf("deny selected %q, want the offline principal; the online one is not in the blocked group and would have the network", got) } - // The guard must be specific to denial, not a blanket disable that would make - // the whole backend dead code. - if !eligible(NetworkAllow, "1") { - t.Fatal("principal backend refused with the network allowed; the guard is over-broad and disables the backend entirely") + if got := windowsSandboxRoleForNetwork(NetworkAllow); got != windowsSandboxRoleOnline { + t.Fatalf("allow selected %q, want the online principal", got) } - if eligible(NetworkAllow, "0") { - t.Fatal("principal backend eligible without the opt-in") + // Fail closed. An unrecognised mode must lose the network rather than keep it, + // so anything that is not an explicit allow maps to the offline principal. + for _, mode := range []NetworkMode{NetworkMode(""), NetworkMode("bogus"), NetworkMode("ALLOW")} { + if got := windowsSandboxRoleForNetwork(mode); got != windowsSandboxRoleOffline { + t.Fatalf("unrecognised mode %q selected %q, want the offline principal so an unknown mode fails closed", mode, got) + } } } diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 03504b135..25f5d672a 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -54,13 +54,54 @@ const ( windowsSandboxUserComment = "Zero sandbox principal (managed)" windowsSandboxUserCommentKey = windowsSandboxUserComment + " key=" windowsSandboxUserNameMax = 20 + + // windowsSandboxOfflineGroupName is what the network block filters are keyed + // to. Network denial cannot be expressed on a principal's own token the way + // it is on a restricted token: the block filters match the offline-marker + // SID, which is a synthetic capability SID, and LogonUser mints a token from + // an account's real group memberships rather than from arbitrary SIDs. A + // principal is therefore denied the network by being a MEMBER of this group, + // whose SID the filters also match. + // + // A group rather than the offline 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. + windowsSandboxOfflineGroupName = "ZeroSandboxOffline" + windowsSandboxOfflineGroupComment = "Zero sandbox principals denied network access (managed)" +) + +// windowsSandboxRole distinguishes the two principals a workspace gets. They are +// separate accounts rather than one account reconfigured per command, because +// the network decision is baked into group membership at setup time and setup +// needs elevation; flipping it per command would need an elevated hop on every +// command. +type windowsSandboxRole string + +const ( + // windowsSandboxRoleOffline is a member of the offline group, so the block + // filters match its token and it has no network. + windowsSandboxRoleOffline windowsSandboxRole = "offline" + // windowsSandboxRoleOnline is not, so an approved network command reaches the + // network while still being read-confined by having its own identity. + windowsSandboxRoleOnline windowsSandboxRole = "online" ) +// roleTag is the single character that distinguishes the two accounts for a +// workspace. One character because the 20-character account-name limit is +// already tight and every character spent here is a bit of workspace hash lost. +func (role windowsSandboxRole) roleTag() string { + if role == windowsSandboxRoleOnline { + return "n" + } + return "d" +} + // Win32 status codes that mean "already there". Treated as success so // provisioning converges instead of failing on a second run. const ( nerrSuccess = 0 nerrGroupExists = 2223 + nerrGroupNotFound = 2220 nerrUserExists = 2224 errorAliasExists = 1379 errorMemberInAlias = 1378 @@ -81,6 +122,7 @@ var ( netapi32 = windows.NewLazySystemDLL("netapi32.dll") procNetUserAdd = netapi32.NewProc("NetUserAdd") procNetLocalGroupAdd = netapi32.NewProc("NetLocalGroupAdd") + procNetLocalGroupGetInfo = netapi32.NewProc("NetLocalGroupGetInfo") procNetLocalGroupAddMembers = netapi32.NewProc("NetLocalGroupAddMembers") procNetUserDel = netapi32.NewProc("NetUserDel") procNetUserSetInfo = netapi32.NewProc("NetUserSetInfo") @@ -136,12 +178,17 @@ func (identity windowsSandboxIdentity) String() string { return identity.Username + " (" + identity.SID.String() + ")" } -// windowsSandboxUserName derives a stable account name for a workspace key. The -// key is hashed by the caller (see windowsSandboxWorkspaceKey) so the name reveals no -// path, and it is truncated to the 20-character local-account limit. The same -// workspace always maps to the same account, so re-running setup reuses the -// principal instead of accumulating accounts. -func windowsSandboxUserName(workspaceKey string) string { +// windowsSandboxUserName derives a stable account name for a workspace key and +// role. The key is hashed by the caller (see windowsSandboxWorkspaceKey) so the +// name reveals no path, and it is truncated to the 20-character local-account +// limit. The same workspace and role always map to the same account, so +// re-running setup reuses the principals instead of accumulating accounts. +// +// The role tag sits before the hash rather than after it, so truncation eats +// hash characters and never the tag. A name that lost its tag would collide the +// two roles onto one account, which would silently put an online principal in +// the offline group or the reverse. +func windowsSandboxUserName(workspaceKey string, role windowsSandboxRole) string { cleaned := strings.Map(func(r rune) rune { switch { case r >= 'a' && r <= 'z', r >= '0' && r <= '9': @@ -155,7 +202,7 @@ func windowsSandboxUserName(workspaceKey string) string { if cleaned == "" { cleaned = "default" } - name := windowsSandboxUserPrefix + cleaned + name := windowsSandboxUserPrefix + role.roleTag() + cleaned if len(name) > windowsSandboxUserNameMax { name = name[:windowsSandboxUserNameMax] } @@ -208,14 +255,91 @@ func netAPIStatus(call string, status uintptr, okStatuses ...uintptr) error { // ensureWindowsSandboxGroup creates the managed local group, or leaves it alone // when it already exists. func ensureWindowsSandboxGroup() error { - name, err := windows.UTF16PtrFromString(windowsSandboxGroupName) + return ensureWindowsLocalGroup(windowsSandboxGroupName, windowsSandboxGroupComment) +} + +// ensureWindowsSandboxOfflineGroup creates the group the network block filters +// are keyed to. Membership of it is what denies a principal the network, so it +// has to exist before any offline principal is created. +func ensureWindowsSandboxOfflineGroup() error { + return ensureWindowsLocalGroup(windowsSandboxOfflineGroupName, windowsSandboxOfflineGroupComment) +} + +// Wires the portable network planner to the Windows group lookup. Done here so +// windows_network.go can stay free of Win32 calls while still folding the group +// into the filter identity set. +func init() { + resolveWindowsSandboxOfflineGroupSIDHook = resolveWindowsSandboxOfflineGroupSID +} + +// resolveWindowsSandboxOfflineGroupSID returns the SID the block filters key to. +// +// Reports ("", nil) when the group does not exist, which is the state before +// setup has ever run. Callers fold that into "no extra identity", so the plan +// they compute matches what an un-provisioned machine would produce rather than +// failing to build at all. +func resolveWindowsSandboxOfflineGroupSID() (string, error) { + sid, _, accountType, err := windows.LookupSID("", windowsSandboxOfflineGroupName) if err != nil { - return err + if errors.Is(err, windows.ERROR_NONE_MAPPED) { + return "", nil + } + return "", fmt.Errorf("look up %s: %w", windowsSandboxOfflineGroupName, err) + } + // A local group is an alias. Anything else means the name has been taken by + // something that is not ours, and keying network filters to it would be both + // wrong and a way to affect unrelated accounts. + if accountType != windows.SidTypeAlias && accountType != windows.SidTypeGroup { + return "", fmt.Errorf("%s resolves to a non-group account (type %d)", windowsSandboxOfflineGroupName, accountType) } - comment, err := windows.UTF16PtrFromString(windowsSandboxGroupComment) + return sid.String(), nil +} + +// ensureWindowsLocalGroup creates a local group, or leaves it alone when it +// already exists. +func ensureWindowsLocalGroup(groupName string, groupComment string) error { + status, err := addWindowsLocalGroupFn(groupName, groupComment) if err != nil { return err } + if status == nerrGroupExists || status == errorAliasExists { + // A group with this name already exists, which is the normal re-run case + // — but only if it is OURS. The offline group's SID is installed on the + // persistent WFP deny filters and the sandbox principal is made a member + // of it, so silently adopting a same-named group created by some other + // tool or by policy would cut off every existing member's outbound + // traffic and hand our principal that group's permissions. + owned, err := windowsLocalGroupOwnedByZeroFn(groupName, groupComment) + if err != nil { + return err + } + if !owned { + return fmt.Errorf("local group %q already exists and is not managed by zero; "+ + "rename or remove it before running sandbox setup", groupName) + } + return nil + } + return netAPIStatus("NetLocalGroupAdd", status) +} + +// Seams for the two Win32 calls behind group creation, so the already-exists +// branch is reachable in tests without an elevated machine. +var ( + addWindowsLocalGroupFn = addWindowsLocalGroup + windowsLocalGroupOwnedByZeroFn = windowsLocalGroupOwnedByZero +) + +// addWindowsLocalGroup issues NetLocalGroupAdd and hands back its raw status so +// the caller can distinguish "already exists" from a real failure. +func addWindowsLocalGroup(groupName string, groupComment string) (uintptr, error) { + name, err := windows.UTF16PtrFromString(groupName) + if err != nil { + return 0, err + } + comment, err := windows.UTF16PtrFromString(groupComment) + if err != nil { + return 0, err + } info := localGroupInfo1{Name: name, Comment: comment} status, _, _ := procNetLocalGroupAdd.Call( 0, // local machine @@ -228,7 +352,42 @@ func ensureWindowsSandboxGroup() error { runtime.KeepAlive(info) runtime.KeepAlive(name) runtime.KeepAlive(comment) - return netAPIStatus("NetLocalGroupAdd", status, nerrGroupExists, errorAliasExists) + return status, nil +} + +// windowsLocalGroupOwnedByZero reports whether an existing local group carries +// the managed-group marker this setup writes, so a foreign group that merely +// shares the name is never adopted. +func windowsLocalGroupOwnedByZero(groupName string, wantComment string) (bool, error) { + name, err := windows.UTF16PtrFromString(groupName) + if err != nil { + return false, err + } + var buffer *byte + status, _, _ := procNetLocalGroupGetInfo.Call( + 0, // local machine + uintptr(unsafe.Pointer(name)), + 1, // level: LOCALGROUP_INFO_1 + uintptr(unsafe.Pointer(&buffer)), + ) + runtime.KeepAlive(name) + if status == nerrGroupNotFound { + // It existed a moment ago and does not now. Treat that as not-ours + // rather than guessing; the next setup run recreates it cleanly. + return false, nil + } + if err := netAPIStatus("NetLocalGroupGetInfo", status); err != nil { + return false, err + } + if buffer == nil { + return false, nil + } + defer procNetApiBufferFree.Call(uintptr(unsafe.Pointer(buffer))) + info := (*localGroupInfo1)(unsafe.Pointer(buffer)) + if info.Comment == nil { + return false, nil + } + return windows.UTF16PtrToString(info.Comment) == wantComment, nil } // ensureWindowsSandboxUser creates a sandbox account with the supplied password. @@ -311,7 +470,20 @@ func resetWindowsSandboxUserPassword(username string, password string) error { // addWindowsSandboxUserToGroup puts a principal in the managed group, ignoring // the status that means it is already a member. func addWindowsSandboxUserToGroup(username string) error { - group, err := windows.UTF16PtrFromString(windowsSandboxGroupName) + return addWindowsUserToLocalGroup(windowsSandboxGroupName, username) +} + +// addWindowsSandboxUserToOfflineGroup is what actually denies a principal the +// network: the block filters match this group's SID, and LogonUser puts the +// group into the token it mints for a member. +func addWindowsSandboxUserToOfflineGroup(username string) error { + return addWindowsUserToLocalGroup(windowsSandboxOfflineGroupName, username) +} + +// addWindowsUserToLocalGroup puts an account in a local group, ignoring the +// status that means it is already a member. +func addWindowsUserToLocalGroup(groupName string, username string) error { + group, err := windows.UTF16PtrFromString(groupName) if err != nil { return err } @@ -501,19 +673,21 @@ func resolveWindowsSandboxSID(username string) (*windows.SID, error) { // post-creation pair would never get past ensureWindowsSandboxGroup on an // ordinary machine and would pass without reaching the code it names. var ( - ensureWindowsSandboxGroupFn = ensureWindowsSandboxGroup - ensureWindowsSandboxUserFn = ensureWindowsSandboxUser - addWindowsSandboxUserToGroupFn = addWindowsSandboxUserToGroup - resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID - resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword - windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged - windowsSandboxUserIsPrivilegedFn = windowsSandboxUserIsPrivileged - grantWindowsSandboxLogonRightsFn = grantWindowsSandboxLogonRights - revokeWindowsSandboxLogonRightsFn = revokeWindowsSandboxLogonRights + ensureWindowsSandboxGroupFn = ensureWindowsSandboxGroup + ensureWindowsSandboxOfflineGroupFn = ensureWindowsSandboxOfflineGroup + ensureWindowsSandboxUserFn = ensureWindowsSandboxUser + addWindowsSandboxUserToGroupFn = addWindowsSandboxUserToGroup + addWindowsSandboxUserToOfflineGroupFn = addWindowsSandboxUserToOfflineGroup + resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID + resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword + windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged + windowsSandboxUserIsPrivilegedFn = windowsSandboxUserIsPrivileged + grantWindowsSandboxLogonRightsFn = grantWindowsSandboxLogonRights + revokeWindowsSandboxLogonRightsFn = revokeWindowsSandboxLogonRights // applyWindowsACLPlanFn is a seam so a test can pin the ORDER of setup's ACL - // work. The revocation below only prevents a stale grant if it runs before - // the plan that re-adds the current one; a test that exercised the revoke - // helper on its own would pass just as happily with the call site deleted. + // work. The revocation only prevents a stale grant if it runs before the plan + // that re-adds the current one; a test that exercised the revoke helper on its + // own would pass just as happily with the call site deleted. applyWindowsACLPlanFn = applyWindowsACLPlan ) @@ -527,11 +701,17 @@ var ( // live. The returned value is always the account's actual password, including // when the account already existed, because that case is reset explicitly // below. -func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, string, bool, error) { +func provisionWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRole) (windowsSandboxIdentity, string, bool, error) { if err := ensureWindowsSandboxGroupFn(); err != nil { return windowsSandboxIdentity{}, "", false, err } - username := windowsSandboxUserName(workspaceKey) + // The offline group has to exist before an offline principal joins it, and it + // is created unconditionally so the network filters can be keyed to its SID + // whether or not an offline principal has been provisioned yet. + if err := ensureWindowsSandboxOfflineGroupFn(); err != nil { + return windowsSandboxIdentity{}, "", false, err + } + username := windowsSandboxUserName(workspaceKey, role) password, err := newWindowsSandboxPassword() if err != nil { return windowsSandboxIdentity{}, "", false, err @@ -596,6 +776,19 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit if err := addWindowsSandboxUserToGroupFn(username); err != nil { return windowsSandboxIdentity{Username: username}, "", !existed, err } + // Membership of the offline group IS the network denial, so it is applied + // here rather than left to a later step: a principal that reached the runner + // without it would look correctly provisioned and quietly have the network. + // + // This is the failure most worth surviving cleanly. It happens after the + // account exists, and local policy can refuse a group join, so the name has + // to come back with the error or the rollback deletes "" and strands a + // principal that has network access and no offline membership. + if role == windowsSandboxRoleOffline { + if err := addWindowsSandboxUserToOfflineGroupFn(username); err != nil { + return windowsSandboxIdentity{Username: username}, "", !existed, err + } + } sid, err := resolveWindowsSandboxSIDFn(username) if err != nil { return windowsSandboxIdentity{Username: username}, "", !existed, err @@ -611,15 +804,42 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit // // A missing account is success, so teardown converges the same way provisioning // does. Requires an elevated caller. -func removeWindowsSandboxIdentity(username string) error { +func removeWindowsSandboxIdentity(username string, workspaceKey string) error { + // Account names here are derived, not discovered, so this could be pointed at + // a name that happens to belong to somebody else's local account. Deleting a + // user is not a recoverable mistake, so ownership is proven before deleting + // rather than inferred from the name matching a pattern we generate. + // Keyed to the workspace as well as to the marker: on a name collision the + // account belongs to a DIFFERENT workspace, and deleting it would be the + // same unrecoverable mistake as deleting a stranger's account. + managed, err := windowsSandboxUserIsManagedFn(username, workspaceKey) + if err != nil { + return err + } + if !managed { + // Distinguishable rather than a bare nil. Leaving the account alone is + // right, and callers that only want the goal state ("no principal of ours + // under this name") can treat it as success by matching this sentinel. But + // reporting a plain success would tell an operator that cleanup completed + // when a name they may care about was deliberately left in place, which is + // the one detail worth surfacing here. + return fmt.Errorf("%w: %q", errWindowsSandboxForeignAccountRetained, username) + } name, err := windows.UTF16PtrFromString(username) if err != nil { return err } status, _, _ := procNetUserDel.Call(0, uintptr(unsafe.Pointer(name))) + runtime.KeepAlive(name) return netAPIStatus("NetUserDel", status, nerrUserNotFound) } +// errWindowsSandboxForeignAccountRetained reports that removal left an account +// in place because it is not one Zero created. It is not a failure to clean up, +// it is a refusal to delete somebody else's account, and teardown paths treat it +// as success. +var errWindowsSandboxForeignAccountRetained = errors.New("left a local account in place because Zero did not create it") + // errWindowsSandboxIdentityUnavailable reports that no sandbox principal has // been provisioned yet, so callers can fall back to the restricted-token // backend instead of failing the command. @@ -629,21 +849,8 @@ var errWindowsSandboxIdentityUnavailable = errors.New("no Zero sandbox principal // creating anything, so the unelevated command path can discover whether an // identity exists. It returns errWindowsSandboxIdentityUnavailable when setup // has not run. -func lookupWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, error) { - username := windowsSandboxUserName(workspaceKey) - // Ownership is checked here as well as at provisioning, because the account - // NAME cannot carry the whole workspace key. - // - // The name keeps 11 characters of the digest; the comment holds all of it. - // Provisioning refuses a name whose comment names a different workspace, and - // without the same check here the workspace that LOST that race would still - // resolve the name to a SID and quietly use the other workspace's principal, - // its secret and its ACL identity. Setup would have failed for it, so this is - // the path that decides whether the refusal actually holds. - // - // A collision is very unlikely with real keys, roughly 2^-44 per pair, but the - // cost of being wrong is one workspace running as another's identity, and the - // check is one syscall on a path that is already doing several. +func lookupWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRole) (windowsSandboxIdentity, error) { + username := windowsSandboxUserName(workspaceKey, role) // SID resolution runs FIRST so "no such account" stays the unavailable // sentinel. windowsSandboxUserIsManaged answers false for both an absent // account and one belonging to someone else, so checking it before this would @@ -678,8 +885,8 @@ func lookupWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, // stay able to clean up an account that has become privileged rather than // refusing to touch it. Refusing there would leave the very account this guards // against permanently undeletable by Zero. -func lookupWindowsSandboxPrincipalForCommand(workspaceKey string) (windowsSandboxIdentity, error) { - identity, err := lookupWindowsSandboxIdentity(workspaceKey) +func lookupWindowsSandboxPrincipalForCommand(workspaceKey string, role windowsSandboxRole) (windowsSandboxIdentity, error) { + identity, err := lookupWindowsSandboxIdentity(workspaceKey, role) if err != nil { return windowsSandboxIdentity{}, err } @@ -715,3 +922,56 @@ func classifyWindowsSandboxLookupError(err error) error { } return err } + +// windowsSandboxUserInLocalGroup reports direct membership of one named local +// group. Network denial is keyed to the offline group's SID on the WFP filters, +// so the command path uses this to confirm the account it is about to log on +// still carries the membership that makes those filters apply to it. +// +// Indirected through a var so the command path can be tested without an +// elevated machine. +var windowsSandboxUserInLocalGroupFn = windowsSandboxUserInLocalGroup + +func windowsSandboxUserInLocalGroup(username string, groupName string) (bool, error) { + name, err := windows.UTF16PtrFromString(username) + if err != nil { + return false, err + } + var ( + buffer *byte + entries uint32 + total uint32 + ) + status, _, _ := procNetUserGetLocalGroups.Call( + 0, // local machine + uintptr(unsafe.Pointer(name)), + 0, // level: LOCALGROUP_USERS_INFO_0 + 0, // flags: direct membership only + uintptr(unsafe.Pointer(&buffer)), + uintptr(^uint32(0)), // MAX_PREFERRED_LENGTH + uintptr(unsafe.Pointer(&entries)), + uintptr(unsafe.Pointer(&total)), + ) + runtime.KeepAlive(name) + if status == nerrUserNotFound { + return false, nil + } + if err := netAPIStatus("NetUserGetLocalGroups", status); err != nil { + return false, err + } + if buffer == nil || entries == 0 { + return false, nil + } + defer procNetApiBufferFree.Call(uintptr(unsafe.Pointer(buffer))) + want := strings.ToLower(groupName) + groups := unsafe.Slice((*localGroupUsersInfo0)(unsafe.Pointer(buffer)), entries) + for _, group := range groups { + if group.Name == nil { + continue + } + if strings.ToLower(windows.UTF16PtrToString(group.Name)) == want { + return true, nil + } + } + return false, nil +} diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 11d64c741..9c8cdde0f 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -16,7 +16,7 @@ import ( // A local Windows account name is capped at 20 characters, so the derived name // must truncate rather than produce a name NetUserAdd rejects. func TestWindowsSandboxUserNameRespectsLengthLimit(t *testing.T) { - name := windowsSandboxUserName(strings.Repeat("a", 64)) + name := windowsSandboxUserName(strings.Repeat("a", 64), windowsSandboxRoleOffline) if len(name) > windowsSandboxUserNameMax { t.Fatalf("name %q is %d chars, want at most %d", name, len(name), windowsSandboxUserNameMax) } @@ -28,12 +28,12 @@ func TestWindowsSandboxUserNameRespectsLengthLimit(t *testing.T) { // The same workspace must map to the same principal, otherwise re-running setup // would accumulate a new local account every time. func TestWindowsSandboxUserNameIsStable(t *testing.T) { - first := windowsSandboxUserName("abc123") - second := windowsSandboxUserName("abc123") + first := windowsSandboxUserName("abc123", windowsSandboxRoleOffline) + second := windowsSandboxUserName("abc123", windowsSandboxRoleOffline) if first != second { t.Fatalf("name is not stable: %q vs %q", first, second) } - if other := windowsSandboxUserName("def456"); other == first { + if other := windowsSandboxUserName("def456", windowsSandboxRoleOffline); other == first { t.Fatalf("different workspaces produced the same principal %q", first) } } @@ -41,7 +41,7 @@ func TestWindowsSandboxUserNameIsStable(t *testing.T) { // The key is sanitised to characters a local account name accepts, so a hash or // path fragment cannot smuggle a separator or a space into the name. func TestWindowsSandboxUserNameRejectsUnsafeCharacters(t *testing.T) { - name := windowsSandboxUserName(`C:\Users\me\proj ect`) + name := windowsSandboxUserName(`C:\Users\me\proj ect`, windowsSandboxRoleOffline) for _, r := range strings.TrimPrefix(name, windowsSandboxUserPrefix) { isLower := r >= 'a' && r <= 'z' isDigit := r >= '0' && r <= '9' @@ -58,7 +58,7 @@ func TestWindowsSandboxUserNameRejectsUnsafeCharacters(t *testing.T) { // than the bare prefix. func TestWindowsSandboxUserNameHandlesEmptyKey(t *testing.T) { for _, key := range []string{"", "///", " "} { - if got := windowsSandboxUserName(key); got == windowsSandboxUserPrefix { + if got := windowsSandboxUserName(key, windowsSandboxRoleOffline); got == windowsSandboxUserPrefix { t.Fatalf("key %q produced a bare prefix", key) } } @@ -213,9 +213,9 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { // previous run. Provisioning no longer resets an adopted account's password, // so a leftover account would otherwise be adopted with a password this test // never learns. - _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01")) + _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01", windowsSandboxRoleOffline), "ziptest01") - identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01") + identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("provision: %v", err) } @@ -228,7 +228,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { t.Errorf("cleanup: revoke logon rights: %v", err) } - if err := removeWindowsSandboxIdentity(identity.Username); err != nil { + if err := removeWindowsSandboxIdentity(identity.Username, "ziptest01"); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %v", err) } }) @@ -240,7 +240,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { } // Re-running must converge on the same principal rather than failing or // creating a second account. - again, secondPassword, _, err := provisionWindowsSandboxIdentity("ziptest01") + again, secondPassword, _, err := provisionWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("second provision: %v", err) } @@ -265,7 +265,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { SandboxHome: t.TempDir(), WorkspaceRoots: []string{`C:\ziptest01`}, } - setupIdentity, _, err := provisionWindowsSandboxPrincipalForSetup(config) + setupIdentity, _, err := provisionWindowsSandboxPrincipalForSetup(config, windowsSandboxRoleOffline) if err != nil { t.Fatalf("setup provision: %v", err) } @@ -284,10 +284,10 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { _ = token.Close() t.Cleanup(func() { _ = revokeWindowsSandboxLogonRights(setupIdentity.SID) - _ = removeWindowsSandboxIdentity(setupIdentity.Username) + _ = removeWindowsSandboxIdentity(setupIdentity.Username, "ziptest01") }) // Lookup must find what provisioning created. - found, err := lookupWindowsSandboxIdentity("ziptest01") + found, err := lookupWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("lookup after provision: %v", err) } @@ -318,9 +318,9 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { const key = "ziplogon01" // A leftover account from an interrupted run would keep its old password, // which the freshly generated one will not match, so start from a clean slate. - _ = removeWindowsSandboxIdentity(windowsSandboxUserName(key)) + _ = removeWindowsSandboxIdentity(windowsSandboxUserName(key, windowsSandboxRoleOffline), key) - identity, password, _, err := provisionWindowsSandboxIdentity(key) + identity, password, _, err := provisionWindowsSandboxIdentity(key, windowsSandboxRoleOffline) if err != nil { t.Fatalf("provision: %v", err) } @@ -330,7 +330,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { t.Errorf("cleanup: revoke logon rights: %v", err) } - if err := removeWindowsSandboxIdentity(identity.Username); err != nil { + if err := removeWindowsSandboxIdentity(identity.Username, key); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %v", err) } }) @@ -367,7 +367,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { // "run setup" error rather than a raw lookup failure, so the command path can // fall back instead of surfacing a Win32 code. func TestLookupWindowsSandboxIdentityUnprovisioned(t *testing.T) { - _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey9z") + _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey9z", windowsSandboxRoleOffline) if err == nil { t.Skip("a principal for this key unexpectedly exists on this machine") } @@ -459,3 +459,40 @@ func TestWindowsSandboxNameCollisionIsTyped(t *testing.T) { t.Fatalf("collision message = %q, want it to say the account is not ours", wrapped.Error()) } } + +// The two roles must never collide onto one account. If they did, the online +// principal would be whatever the offline one is, which means either an approved +// network command silently has no network, or an ordinary command silently has +// one. The second is the dangerous direction. +func TestWindowsSandboxUserNameSeparatesRoles(t *testing.T) { + const key = "abc123" + offline := windowsSandboxUserName(key, windowsSandboxRoleOffline) + online := windowsSandboxUserName(key, windowsSandboxRoleOnline) + if offline == online { + t.Fatalf("both roles produced %q", offline) + } + // Still stable per role, or setup would create a new account every run. + if again := windowsSandboxUserName(key, windowsSandboxRoleOnline); again != online { + t.Fatalf("online name is not stable: %q then %q", online, again) + } + + // The role tag has to survive truncation. A very long key is exactly when the + // name is cut to the 20-character limit, and losing the tag there is what + // would collide the roles on precisely the workspaces most likely to hit it. + longKey := strings.Repeat("f", 200) + longOffline := windowsSandboxUserName(longKey, windowsSandboxRoleOffline) + longOnline := windowsSandboxUserName(longKey, windowsSandboxRoleOnline) + if longOffline == longOnline { + t.Fatalf("truncation collided the roles onto %q", longOffline) + } + for _, name := range []string{longOffline, longOnline} { + if len(name) > windowsSandboxUserNameMax { + t.Fatalf("name %q is %d chars, want at most %d", name, len(name), windowsSandboxUserNameMax) + } + } + + // Different workspaces still get different accounts within a role. + if other := windowsSandboxUserName("def456", windowsSandboxRoleOffline); other == offline { + t.Fatalf("two workspaces produced the same offline account %q", other) + } +} diff --git a/internal/sandbox/windows_network.go b/internal/sandbox/windows_network.go index 0d614a0fc..fbf62341f 100644 --- a/internal/sandbox/windows_network.go +++ b/internal/sandbox/windows_network.go @@ -62,15 +62,71 @@ func BuildWindowsNetworkInfraPlan(config WindowsSandboxCommandConfig) (WindowsNe if err != nil { return WindowsNetworkPlan{}, err } + // The offline-marker SID stays first: the setup marker records + // IdentitySIDs[0] as the offline filter identity. + identitySIDs := []string{offlineSID} + // A sandbox principal cannot carry the offline-marker SID, because LogonUser + // builds a token from an account's real group memberships and the marker is a + // synthetic capability SID. So the same filters additionally match a real + // local group that network-denied principals belong to. + // + // Resolved rather than assumed, and absent until the group exists, so a + // machine that has never provisioned principals computes exactly the plan it + // computed before this existed. That matters because the plan is hashed into + // the setup marker and compared on every command: an identity set that + // differed between setup and the command path would fail every command with + // "setup is out of date". + if resolveWindowsSandboxOfflineGroupSIDHook != nil { + groupSID, err := resolveWindowsSandboxOfflineGroupSIDHook() + if err != nil { + return WindowsNetworkPlan{}, err + } + if trimmed := strings.TrimSpace(groupSID); trimmed != "" { + identitySIDs = append(identitySIDs, trimmed) + } + } return WindowsNetworkPlan{ Mode: NetworkDeny, ProviderKey: windowsWFPProviderKey, SubLayerKey: windowsWFPSubLayerKey, - IdentitySIDs: []string{offlineSID}, + IdentitySIDs: identitySIDs, Filters: windowsDenyWFPFilterSpecs(), }, nil } +// WindowsNetworkPlanCoversPrincipals reports whether a plan's block filters name +// the offline group, which is the only thing that makes them apply to a sandbox +// principal. +// +// This exists to be asserted, not consulted. The plan must be built AFTER +// provisioning, because provisioning is what creates the group it resolves; a +// plan built first names only the offline marker and leaves every offline +// principal with an open network while setup reports success. That is a control +// that enforces nothing while claiming to work, and the ordering which prevents +// it is invisible at the call site, so a later refactor can undo it silently. +func WindowsNetworkPlanCoversPrincipals(plan WindowsNetworkPlan, offlineGroupSID string) bool { + offlineGroupSID = strings.TrimSpace(offlineGroupSID) + if offlineGroupSID == "" { + // No group provisioned on this host, so there is no principal for the + // filters to miss and nothing to assert. + return true + } + for _, sid := range plan.IdentitySIDs { + if strings.EqualFold(strings.TrimSpace(sid), offlineGroupSID) { + return true + } + } + return false +} + +// resolveWindowsSandboxOfflineGroupSIDHook resolves the local group that +// network-denied principals belong to. It is wired up on Windows only, so this +// file stays free of Win32 calls and the plan on other platforms is unchanged. +// +// Returning ("", nil) means the group does not exist yet, which is the state +// before principals have ever been provisioned. +var resolveWindowsSandboxOfflineGroupSIDHook func() (string, error) + // WindowsNetworkInfraHash fingerprints the provisioned (mode-independent) network // infrastructure so the setup marker validates against the same setup for BOTH // command modes. It never folds in the per-command network mode. diff --git a/internal/sandbox/windows_network_test.go b/internal/sandbox/windows_network_test.go index 4253916f8..50f66d960 100644 --- a/internal/sandbox/windows_network_test.go +++ b/internal/sandbox/windows_network_test.go @@ -118,3 +118,108 @@ func assertWindowsWFPCommonFilter(t *testing.T, specs map[string]WindowsWFPFilte // Coverage for the network infra plan + hash and the per-mode token-SID // composition lives in windows_online_offline_test.go. + +// The block filters have to name the offline group as well as the offline-marker +// SID, or a sandbox principal is not covered by them at all: LogonUser builds a +// token from real group memberships and cannot carry the synthetic marker. This +// is the single property that makes network denial work for the principal +// backend, so assert it on the plan rather than trusting the wiring. +func TestNetworkInfraPlanIncludesOfflineGroupIdentity(t *testing.T) { + previous := resolveWindowsSandboxOfflineGroupSIDHook + t.Cleanup(func() { resolveWindowsSandboxOfflineGroupSIDHook = previous }) + + config := WindowsSandboxCommandConfig{SandboxHome: t.TempDir()} + + // Before principals have ever been provisioned the group does not exist, and + // the plan must be exactly what it was before this feature. The plan is hashed + // into the setup marker and re-derived on every command, so an identity set + // that appeared out of nowhere would fail every command with "setup is out of + // date". + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { return "", nil } + base, err := BuildWindowsNetworkInfraPlan(config) + if err != nil { + t.Fatalf("build plan without the group: %v", err) + } + if len(base.IdentitySIDs) != 1 { + t.Fatalf("identity SIDs = %v, want only the offline marker when the group is absent", base.IdentitySIDs) + } + + const groupSID = "S-1-5-32-9999" + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { return groupSID, nil } + withGroup, err := BuildWindowsNetworkInfraPlan(config) + if err != nil { + t.Fatalf("build plan with the group: %v", err) + } + if len(withGroup.IdentitySIDs) != 2 || withGroup.IdentitySIDs[1] != groupSID { + t.Fatalf("identity SIDs = %v, want the offline marker plus %s", withGroup.IdentitySIDs, groupSID) + } + // The marker records IdentitySIDs[0] as the offline filter identity, so the + // marker SID has to stay first. + if withGroup.IdentitySIDs[0] != base.IdentitySIDs[0] { + t.Fatalf("offline marker moved from position 0: %v", withGroup.IdentitySIDs) + } + // Adding the group must change the fingerprint, or setup and the command path + // could disagree about the installed filters without anything noticing. + baseHash, err := WindowsNetworkInfraHash(base) + if err != nil { + t.Fatalf("hash base: %v", err) + } + groupHash, err := WindowsNetworkInfraHash(withGroup) + if err != nil { + t.Fatalf("hash with group: %v", err) + } + if baseHash == groupHash { + t.Fatal("the infra hash ignored the offline group identity") + } +} + +// A lookup failure must not be swallowed into "no group", because that silently +// produces a plan whose filters do not cover sandbox principals. +func TestNetworkInfraPlanPropagatesOfflineGroupLookupFailure(t *testing.T) { + previous := resolveWindowsSandboxOfflineGroupSIDHook + t.Cleanup(func() { resolveWindowsSandboxOfflineGroupSIDHook = previous }) + + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { + return "", errors.New("boom") + } + if _, err := BuildWindowsNetworkInfraPlan(WindowsSandboxCommandConfig{SandboxHome: t.TempDir()}); err == nil { + t.Fatal("a failed offline-group lookup produced a plan; the filters would not cover any principal") + } +} + +// The ordering that makes the block filters apply to sandbox principals is +// invisible at the call site: the plan must be built AFTER provisioning, +// because provisioning creates the group the filters name. A plan built first +// names only the offline marker, and a machine would then report a successful +// setup while every offline principal had an open network. +// +// Asserted on the predicate setup checks before installing anything, so moving +// the plan build back ahead of provisioning fails loudly instead of silently +// producing a control that enforces nothing. +func TestNetworkPlanCoverageDetectsPrincipalsLeftUncovered(t *testing.T) { + const groupSID = "S-1-5-32-4242" + + // What a plan built BEFORE provisioning looks like: marker only. + tooEarly := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-15-3-1111"}} + if WindowsNetworkPlanCoversPrincipals(tooEarly, groupSID) { + t.Fatal("a plan naming only the offline marker was reported as covering principals; the ordering guard would not fire") + } + + // And after: marker plus the group. + correct := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-15-3-1111", groupSID}} + if !WindowsNetworkPlanCoversPrincipals(correct, groupSID) { + t.Fatal("a plan naming the offline group was reported as not covering principals; setup would refuse a correct plan") + } + // SID comparison is case-insensitive, so a differently-cased resolve does not + // read as a missing group and block a valid setup. + mixed := WindowsNetworkPlan{IdentitySIDs: []string{strings.ToLower(groupSID)}} + if !WindowsNetworkPlanCoversPrincipals(mixed, groupSID) { + t.Fatal("case difference read as a missing group") + } + + // A host with no group provisioned has no principal to miss, so there is + // nothing to assert and setup must not refuse. + if !WindowsNetworkPlanCoversPrincipals(tooEarly, "") { + t.Fatal("an unprovisioned host was treated as a coverage failure") + } +} diff --git a/internal/sandbox/windows_offline_group_ownership_windows_test.go b/internal/sandbox/windows_offline_group_ownership_windows_test.go new file mode 100644 index 000000000..4ca357ba7 --- /dev/null +++ b/internal/sandbox/windows_offline_group_ownership_windows_test.go @@ -0,0 +1,96 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "strings" + "testing" +) + +// The offline group's SID goes onto the persistent WFP deny filters and the +// sandbox principal is made a member of it. Adopting a same-named group that +// something else owns therefore cuts off every existing member's outbound +// traffic and hands our principal that group's permissions, so an unmarked +// group has to be refused rather than reused. +func TestEnsureWindowsLocalGroupRefusesForeignSameNameGroup(t *testing.T) { + for name, testCase := range map[string]struct { + status uintptr + owned bool + ownedErr error + wantError string + }{ + "foreign group with our name": { + status: nerrGroupExists, owned: false, + wantError: "not managed by zero", + }, + "foreign group reported via ERROR_ALIAS_EXISTS": { + status: errorAliasExists, owned: false, + wantError: "not managed by zero", + }, + "our own group on a re-run": { + status: nerrGroupExists, owned: true, + }, + "ownership lookup fails": { + status: nerrGroupExists, ownedErr: errors.New("lookup refused"), + wantError: "lookup refused", + }, + "group did not exist": { + status: nerrSuccess, + }, + } { + t.Run(name, func(t *testing.T) { + prevAdd, prevOwned := addWindowsLocalGroupFn, windowsLocalGroupOwnedByZeroFn + t.Cleanup(func() { + addWindowsLocalGroupFn = prevAdd + windowsLocalGroupOwnedByZeroFn = prevOwned + }) + addWindowsLocalGroupFn = func(string, string) (uintptr, error) { return testCase.status, nil } + lookups := 0 + windowsLocalGroupOwnedByZeroFn = func(string, string) (bool, error) { + lookups++ + return testCase.owned, testCase.ownedErr + } + + err := ensureWindowsSandboxOfflineGroup() + if testCase.wantError == "" { + if err != nil { + t.Fatalf("ensureWindowsSandboxOfflineGroup: %v", err) + } + return + } + if err == nil { + t.Fatalf("adopted a group it should have refused (lookups=%d)", lookups) + } + if !strings.Contains(err.Error(), testCase.wantError) { + t.Fatalf("error = %q, want it to mention %q", err, testCase.wantError) + } + }) + } +} + +// The marker compared against is the offline group's own comment, not the +// principals group's: two managed groups with different markers must not be +// mistaken for each other. +func TestEnsureWindowsLocalGroupChecksTheGroupsOwnMarker(t *testing.T) { + prevAdd, prevOwned := addWindowsLocalGroupFn, windowsLocalGroupOwnedByZeroFn + t.Cleanup(func() { + addWindowsLocalGroupFn = prevAdd + windowsLocalGroupOwnedByZeroFn = prevOwned + }) + addWindowsLocalGroupFn = func(string, string) (uintptr, error) { return nerrGroupExists, nil } + var gotName, gotComment string + windowsLocalGroupOwnedByZeroFn = func(name, comment string) (bool, error) { + gotName, gotComment = name, comment + return true, nil + } + if err := ensureWindowsSandboxOfflineGroup(); err != nil { + t.Fatalf("ensureWindowsSandboxOfflineGroup: %v", err) + } + if gotName != windowsSandboxOfflineGroupName { + t.Fatalf("checked group %q, want %q", gotName, windowsSandboxOfflineGroupName) + } + if gotComment != windowsSandboxOfflineGroupComment { + t.Fatalf("compared marker %q, want %q", gotComment, windowsSandboxOfflineGroupComment) + } +} diff --git a/internal/sandbox/windows_offline_membership_windows_test.go b/internal/sandbox/windows_offline_membership_windows_test.go new file mode 100644 index 000000000..e41fe09c1 --- /dev/null +++ b/internal/sandbox/windows_offline_membership_windows_test.go @@ -0,0 +1,102 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "testing" + + "golang.org/x/sys/windows" +) + +// Choosing the offline account is only half of what denies it the network: the +// WFP filters match the offline GROUP'S SID. An account that has drifted out of +// that group still logs on from its stored secret, and its token no longer +// satisfies the filter condition — so a no-network profile would get full +// egress. The command path has to re-check the membership, not trust the marker +// setup wrote when it last succeeded. +func TestPrincipalTokenRechecksOfflineGroupMembership(t *testing.T) { + for name, testCase := range map[string]struct { + mode NetworkMode + member bool + memberErr error + wantChecked bool + wantReached bool + wantErr bool + }{ + "offline principal drifted out of the group": { + mode: NetworkDeny, member: false, + wantChecked: true, wantReached: false, + }, + "offline principal still a member": { + mode: NetworkDeny, member: true, + wantChecked: true, wantReached: true, + }, + "membership lookup fails": { + mode: NetworkDeny, memberErr: errors.New("group lookup refused"), + wantChecked: true, wantReached: false, wantErr: true, + }, + // An allow-network command uses the online principal, which is not in the + // offline group by design. Checking it there would refuse every approved + // network command. + "online principal is not checked": { + mode: NetworkAllow, member: false, + wantChecked: false, wantReached: true, + }, + } { + t.Run(name, func(t *testing.T) { + prevLookup := lookupWindowsSandboxPrincipalForCommandFn + prevMember := windowsSandboxUserInLocalGroupFn + prevSecret := readWindowsSandboxSecretFn + prevWarn := warnWindowsSandboxOfflineMembershipMissing + t.Cleanup(func() { + lookupWindowsSandboxPrincipalForCommandFn = prevLookup + windowsSandboxUserInLocalGroupFn = prevMember + readWindowsSandboxSecretFn = prevSecret + warnWindowsSandboxOfflineMembershipMissing = prevWarn + }) + + sid, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + t.Fatal(err) + } + lookupWindowsSandboxPrincipalForCommandFn = func(string, windowsSandboxRole) (windowsSandboxIdentity, error) { + return windowsSandboxIdentity{Username: "zero-sbx-test", SID: sid}, nil + } + checkedGroup := "" + windowsSandboxUserInLocalGroupFn = func(_ string, group string) (bool, error) { + checkedGroup = group + return testCase.member, testCase.memberErr + } + secretRead := false + readWindowsSandboxSecretFn = func(string) (string, error) { + secretRead = true + return "", errWindowsSandboxIdentityUnavailable + } + warnWindowsSandboxOfflineMembershipMissing = func(string) {} + + config := windowsSandboxTestConfig() + config.Env = map[string]string{windowsSandboxIdentityEnv: "1"} + config.PermissionProfile.Network.Mode = testCase.mode + + _, ok, err := windowsSandboxPrincipalToken(config) + if testCase.wantErr { + if err == nil { + t.Fatal("a failed membership lookup was swallowed") + } + } else if err != nil { + t.Fatalf("windowsSandboxPrincipalToken: %v", err) + } + _ = ok + if secretRead != testCase.wantReached { + t.Fatalf("reached the secret read = %v, want %v (gate must short-circuit before it)", secretRead, testCase.wantReached) + } + if checked := checkedGroup != ""; checked != testCase.wantChecked { + t.Fatalf("membership checked = %v, want %v", checked, testCase.wantChecked) + } + if testCase.wantChecked && checkedGroup != windowsSandboxOfflineGroupName { + t.Fatalf("checked group %q, want %q", checkedGroup, windowsSandboxOfflineGroupName) + } + }) + } +} diff --git a/internal/sandbox/windows_online_offline_test.go b/internal/sandbox/windows_online_offline_test.go index 370abee73..f49d9258c 100644 --- a/internal/sandbox/windows_online_offline_test.go +++ b/internal/sandbox/windows_online_offline_test.go @@ -37,6 +37,18 @@ func TestWindowsRuntimeTokenSIDs(t *testing.T) { // setup serves both modes (and its fingerprint is stable across modes). func TestBuildWindowsNetworkInfraPlanIsModeIndependent(t *testing.T) { home := t.TempDir() + // Pinned, because the count below depends on whether this machine happens to + // have the offline group already. BuildWindowsNetworkInfraPlan folds that + // group's SID in when it resolves, so on a Windows host where an earlier + // elevated setup created ZeroSandboxOffline the plan legitimately carries two + // identity SIDs and this test failed on a correct plan. CI never saw it: the + // Linux and macOS jobs leave the hook nil, and a fresh Windows runner has no + // group yet. Stubbing it makes the assertion about the plan rather than about + // the machine it runs on. + previousHook := resolveWindowsSandboxOfflineGroupSIDHook + t.Cleanup(func() { resolveWindowsSandboxOfflineGroupSIDHook = previousHook }) + resolveWindowsSandboxOfflineGroupSIDHook = nil + mk := func(mode NetworkMode) WindowsSandboxCommandConfig { return WindowsSandboxCommandConfig{ SandboxHome: home, @@ -72,6 +84,40 @@ func TestBuildWindowsNetworkInfraPlanIsModeIndependent(t *testing.T) { if denyPlan.IdentitySIDs[0] != offline { t.Errorf("infra plan SID = %q, want offline-marker %q", denyPlan.IdentitySIDs[0], offline) } + + // Mode independence has to hold on a machine that already has the offline + // group too, which is where the pinned counts above would otherwise be + // asserting something about the host rather than the plan. The group SID is + // folded in for both modes, so the hashes must still agree; only the count + // changes. + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { return "S-1-5-32-9999", nil } + withGroupDeny, err := BuildWindowsNetworkInfraPlan(mk(NetworkDeny)) + if err != nil { + t.Fatalf("deny infra plan with the group present: %v", err) + } + withGroupAllow, err := BuildWindowsNetworkInfraPlan(mk(NetworkAllow)) + if err != nil { + t.Fatalf("allow infra plan with the group present: %v", err) + } + // Assert the SIDs themselves, not the count. A duplicate offline marker or an + // unrelated SID would satisfy a length check while meaning something quite + // different. + if len(withGroupDeny.IdentitySIDs) != 2 || + withGroupDeny.IdentitySIDs[0] != offline || + withGroupDeny.IdentitySIDs[1] != "S-1-5-32-9999" { + t.Fatalf("group present should add its SID after the offline marker, got %v (offline marker %q)", withGroupDeny.IdentitySIDs, offline) + } + groupDenyHash, _ := WindowsNetworkInfraHash(withGroupDeny) + groupAllowHash, _ := WindowsNetworkInfraHash(withGroupAllow) + if groupDenyHash != groupAllowHash || groupDenyHash == "" { + t.Fatalf("infra hash must stay mode-independent with the group present: deny=%q allow=%q", groupDenyHash, groupAllowHash) + } + // And it must differ from the no-group hash, which is the cross-workspace + // coupling called out for a maintainer decision: one workspace creating the + // group changes every other sandbox home's expected fingerprint. + if groupDenyHash == denyHash { + t.Error("group presence did not change the infra hash; the marker staleness this causes is a deliberate property and should be visible here") + } } // A pre-existing schema-1 capability file (no Offline SID) is upgraded in place: diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index ac8dc1243..2d895a1f2 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -3,6 +3,7 @@ package sandbox import ( + "errors" "fmt" "io" @@ -22,14 +23,6 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) return 1 } - // Always provision the mode-INDEPENDENT infrastructure: the outbound block - // filters scoped to the offline-marker SID. Runtime gates network per command - // by whether the token carries that SID, so one setup serves both modes. - networkPlan, err := BuildWindowsNetworkInfraPlan(config.commandConfig()) - if err != nil { - fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) - return 1 - } rollback, err := applyWindowsACLPlan(plan) if err != nil { fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) @@ -62,6 +55,39 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) return aclErr } } + // Built AFTER any principal provisioning, not before. The block filters are + // keyed to the offline group as well as the offline-marker SID, and that group + // is created as part of provisioning: planning first would install filters + // that name only the marker, leaving every offline principal with an open + // network while looking correctly set up. + // + // Mode-INDEPENDENT by design. Which identity a command runs under is what + // selects allow or deny, so one setup serves both modes. + networkPlan, err := BuildWindowsNetworkInfraPlan(config.commandConfig()) + if err != nil { + if rollbackErr := rollback(); rollbackErr != nil { + fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) + return 1 + } + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } + // Refuse to install filters that would not cover the principals just + // provisioned. The ordering above is what makes them cover it, and nothing at + // this call site shows that, so assert it rather than trusting it to survive + // a later refactor. Installing anyway would leave a machine reporting a + // successful setup while every offline principal had an open network. + if groupSID, groupErr := resolveWindowsSandboxOfflineGroupSID(); groupErr == nil { + if !WindowsNetworkPlanCoversPrincipals(networkPlan, groupSID) { + err := errors.New("network block filters do not name the sandbox offline group, so they would not apply to any sandbox principal; the network plan must be built after principals are provisioned") + if rollbackErr := rollback(); rollbackErr != nil { + fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) + return 1 + } + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } + } if err := applyWindowsNetworkPlan(networkPlan); err != nil { if rollbackErr := rollback(); rollbackErr != nil { fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) diff --git a/internal/sandbox/windows_stale_secret_windows_test.go b/internal/sandbox/windows_stale_secret_windows_test.go index 7e4e3ebef..222b8fde3 100644 --- a/internal/sandbox/windows_stale_secret_windows_test.go +++ b/internal/sandbox/windows_stale_secret_windows_test.go @@ -52,7 +52,7 @@ func TestProvisionSurfacesFailedStaleSecretCleanup(t *testing.T) { t.Fatal(err) } // created=false so the run ADOPTS an account and rotation applies. - provisionWindowsSandboxIdentityFn = func(string) (windowsSandboxIdentity, string, bool, error) { + provisionWindowsSandboxIdentityFn = func(string, windowsSandboxRole) (windowsSandboxIdentity, string, bool, error) { return windowsSandboxIdentity{Username: "zero-sbx-test", SID: sid}, "pw", false, nil } grantWindowsSandboxLogonRightsFn = func(*windows.SID) error { return nil } @@ -80,7 +80,7 @@ func TestProvisionSurfacesFailedStaleSecretCleanup(t *testing.T) { CommandCWD: `C:\ws`, WorkspaceRoots: []string{`C:\ws`}, } - _, _, err = provisionWindowsSandboxPrincipalForSetup(config) + _, _, err = provisionWindowsSandboxPrincipalForSetup(config, windowsSandboxRoleOffline) if removed != testCase.wantRemove { t.Fatalf("stale secret removal attempted = %v, want %v", removed, testCase.wantRemove) diff --git a/internal/sandbox/windows_workspace_canonical_windows_test.go b/internal/sandbox/windows_workspace_canonical_windows_test.go index 0997c3113..e986e2f31 100644 --- a/internal/sandbox/windows_workspace_canonical_windows_test.go +++ b/internal/sandbox/windows_workspace_canonical_windows_test.go @@ -202,16 +202,58 @@ func TestTeardownPathDerivationCreatesNothing(t *testing.T) { t.Errorf("temp directory gained %d entries; naming the paths must not create one", after-before) } - // And the setup resolver, which is allowed to create, still does. - created, err := windowsSandboxRuntimeRootPath(WindowsSandboxCommandConfig{ + // Setup's resolver must agree with teardown's, and create nothing either. + // + // This used to assert the opposite — that setup falls back to a usable tree — + // on the reasoning that some runtime root beats none. That was wrong: the + // fallback is os.MkdirTemp memoized only in-process, so elevated setup would + // grant the principal an ACE on temp root A, the next command would derive + // root B and fail ACCESS_DENIED on ordinary cache writes, and teardown would + // clean a third. A root only setup can name is worse than no root, because + // the sandbox looks provisioned and is not. + beforeSetup := tempDirEntryCount(t) + setupRoot, err := windowsSandboxRuntimeRootPath(WindowsSandboxCommandConfig{ WorkspaceRoots: []string{workspace}, CommandCWD: workspace, }) if err != nil { t.Fatalf("windowsSandboxRuntimeRootPath: %v", err) } - if created == "" { - t.Error("setup's resolver should still fall back to a usable tree") + if setupRoot != "" { + t.Errorf("setup named %q for a workspace whose runtime root is underivable; it must report none rather than invent one", setupRoot) + } + if after := tempDirEntryCount(t); after != beforeSetup { + t.Errorf("setup's resolver created %d temp entries; it must create nothing", after-beforeSetup) + } +} + +// Setup and teardown must derive the SAME root in the ordinary case, since one +// grants the ACE the other revokes. This is the case the fix above must not +// break: reporting "no root" is only correct when the root is underivable. +func TestSetupAndTeardownDeriveTheSameRuntimeRoot(t *testing.T) { + workspace := t.TempDir() + cacheRoot := t.TempDir() // outside the workspace, so the derivation is usable + previous := sandboxUserCacheDir + sandboxUserCacheDir = func() (string, error) { return cacheRoot, nil } + t.Cleanup(func() { sandboxUserCacheDir = previous }) + + setupRoot, err := windowsSandboxRuntimeRootPath(WindowsSandboxCommandConfig{ + WorkspaceRoots: []string{workspace}, + CommandCWD: workspace, + }) + if err != nil { + t.Fatalf("windowsSandboxRuntimeRootPath: %v", err) + } + if setupRoot == "" { + t.Fatal("setup named no runtime root for a derivable workspace") + } + teardownRoot, ok := deterministicSandboxRuntimeRoot( + canonicalSandboxWorkspaceRoot(workspace), canonicalSandboxWorkspaceRoot(cacheRoot)) + if !ok { + t.Fatal("precondition: the deterministic root should be usable here") + } + if setupRoot != teardownRoot { + t.Errorf("setup grants on %q but teardown revokes %q", setupRoot, teardownRoot) } }