feat(sandbox): give each workspace an offline and an online principal

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)
+		}
+	}
+}

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)
 	}
 }

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)
 	}
 }

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")
 	}