fix(auth): close Windows remediation review findings

tools/tht/internal/backup/create_test.go

@@ -171,6 +171,7 @@ func TestCreateReferencesAuthFilesByDefaultAndArchivesThemOnlyWithSecretCustody(
 	if err := os.WriteFile(usersPath, []byte("users:\n  - passwordHash: must-not-be-archived-by-default\n"), 0o600); err != nil {
 		t.Fatal(err)
 	}
+	prepareBackupFixturePrivatePaths(t, []string{authDirectory}, []string{authPath, usersPath})
 	fixture.installation.Authentication.ConfigDirectory = authDirectory
 
 	defaultOutput := filepath.Join(t.TempDir(), "default.zip")
@@ -781,6 +782,7 @@ func newBackupFixture(t *testing.T, profile string) *backupFixture {
 		},
 	}
 	fixture.writeEnvironment(t)
+	prepareBackupFixturePrivatePaths(t, []string{descriptorDirectory, secretDirectory}, []string{environment, secretPath})
 	currentImage := fixture.installation.CurrentImageOverridePath()
 	if err := os.MkdirAll(filepath.Dir(currentImage), 0o700); err != nil {
 		t.Fatal(err)

tools/tht/internal/backup/fixture_security_unix_test.go

@@ -0,0 +1,9 @@
+//go:build !windows
+
+package backup
+
+import "testing"
+
+func prepareBackupFixturePrivatePaths(t *testing.T, directories, files []string) {
+	t.Helper()
+}

tools/tht/internal/backup/fixture_security_windows_test.go

@@ -0,0 +1,23 @@
+//go:build windows
+
+package backup
+
+import (
+	"testing"
+
+	"github.com/aritmolab/thothii/tools/tht/internal/safeio"
+)
+
+func prepareBackupFixturePrivatePaths(t *testing.T, directories, files []string) {
+	t.Helper()
+	for _, path := range directories {
+		if err := safeio.ProtectPrivateDirectory(path); err != nil {
+			t.Fatalf("ProtectPrivateDirectory(%q): %v", path, err)
+		}
+	}
+	for _, path := range files {
+		if err := safeio.ProtectPrivateRegular(path); err != nil {
+			t.Fatalf("ProtectPrivateRegular(%q): %v", path, err)
+		}
+	}
+}

tools/tht/internal/backup/preflight_test.go

@@ -10,6 +10,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -43,6 +44,7 @@ func TestPreflightReturnsValidatedMetadataAndCallsAllTargetChecksWithoutExtracti
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer result.CloseArchive()
 	if result.Manifest.InstallationID != "local-dev" || result.Manifest.Entries[0].Path != "configuration/operator.env" {
 		t.Fatalf("validated metadata = %#v", result)
 	}
@@ -197,10 +199,11 @@ func TestPreflightRequiresExplicitProtectionForExternalSecretPayloadsWithoutLeak
 	if err == nil || strings.Contains(err.Error(), string(secret)) || !strings.Contains(err.Error(), "confirmation") {
 		t.Fatalf("secret policy error = %v", err)
 	}
-	_, err = Preflight(context.Background(), installation, PreflightRequest{Archive: archive, Confirm: true, AllowExternalSecrets: true}, permissivePreflightDependencies())
+	result, err := Preflight(context.Background(), installation, PreflightRequest{Archive: archive, Confirm: true, AllowExternalSecrets: true}, permissivePreflightDependencies())
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer result.CloseArchive()
 }
 
 func TestPreflightRejectsInsufficientDiskAndEachTargetCompatibilityFailure(t *testing.T) {
@@ -392,6 +395,17 @@ func TestPreflightRevalidationRefusesAnArchivePathThatWasReplaced(t *testing.T)
 	writePreflightArchive(t, replacement, preflightArchiveSpec{
 		entries: []preflightArchiveEntry{{path: "configuration/operator.env", body: []byte("unchecked")}},
 	})
+	if runtime.GOOS == "windows" {
+		// The retained validated handle denies replacement on Windows; that is the
+		// stronger invariant, so revalidation must still succeed for the unchanged path.
+		if err := os.Rename(replacement, archive); err == nil {
+			t.Fatal("Windows replaced a retained archive path")
+		}
+		if _, err := result.RevalidateArchive(); err != nil {
+			t.Fatalf("RevalidateArchive() on retained Windows archive = %v", err)
+		}
+		return
+	}
 	if err := os.Rename(replacement, archive); err != nil {
 		t.Fatal(err)
 	}

tools/tht/internal/backup/restore_test.go

@@ -308,6 +308,64 @@ func TestRestoreStagesArchiveAfterCheckpointAndRejectsMutation(t *testing.T) {
 	}
 }
 
+type restoreLifecycleTestOutcome struct {
+	result RestoreResult
+	err    error
+}
+
+func releaseLifecycleStage(ctx context.Context, release chan<- struct{}, done <-chan restoreLifecycleTestOutcome) (restoreLifecycleTestOutcome, bool, error) {
+	select {
+	case outcome := <-done:
+		return outcome, true, nil
+	default:
+	}
+	select {
+	case release <- struct{}{}:
+		return restoreLifecycleTestOutcome{}, false, nil
+	default:
+	}
+
+	timer := time.NewTimer(2 * time.Second)
+	defer timer.Stop()
+	var ctxErr error
+	select {
+	case outcome := <-done:
+		return outcome, true, nil
+	case release <- struct{}{}:
+		return restoreLifecycleTestOutcome{}, false, nil
+	case <-ctx.Done():
+		ctxErr = ctx.Err()
+		ctx = nil
+	case <-timer.C:
+		if ctxErr != nil {
+			return restoreLifecycleTestOutcome{}, false, ctxErr
+		}
+		return restoreLifecycleTestOutcome{}, false, errors.New("timed out releasing lifecycle stage")
+	}
+	select {
+	case outcome := <-done:
+		return outcome, true, nil
+	case release <- struct{}{}:
+		return restoreLifecycleTestOutcome{}, false, nil
+	case <-timer.C:
+		return restoreLifecycleTestOutcome{}, false, ctxErr
+	}
+}
+
+func TestReleaseLifecycleStageReturnsPrematureWorkerOutcome(t *testing.T) {
+	want := errors.New("worker ended before the next lifecycle stage")
+	done := make(chan restoreLifecycleTestOutcome, 1)
+	done <- restoreLifecycleTestOutcome{err: want}
+
+	got, terminal, err := releaseLifecycleStage(context.Background(), make(chan struct{}), done)
+	if err != nil {
+		t.Fatalf("releaseLifecycleStage() error = %v", err)
+	}
+	if !terminal || !errors.Is(got.err, want) {
+		t.Fatalf("releaseLifecycleStage() = outcome %#v, terminal %t; want original worker error", got, terminal)
+	}
+}
+
 func TestRestoreLifecycleLockExcludesCompetingTransactionsUntilTerminalCleanup(t *testing.T) {
 	targetFailure := errors.New("target restore failed")
 	recoveryFailure := errors.New("recovery restore failed")
@@ -402,15 +460,15 @@ func TestRestoreLifecycleLockExcludesCompetingTransactionsUntilTerminalCleanup(t
 				installationID: fixture.installationID,
 				entries:        []preflightArchiveEntry{{path: "configuration/operator.env", body: []byte("checkpoint")}},
 			})
-			stages := make(chan string)
-			continueStage := make(chan struct{})
+			stages := make(chan string, 1)
+			continueStage := make(chan struct{}, 1)
+			caller, cancel := context.WithCancel(context.Background())
+			defer cancel()
 			gate := func(stage string) {
 				stages <- stage
 				<-continueStage
 			}
 			runner.beforeFinalMaintenanceRelease = func() { gate("final-barrier-release") }
-			caller, cancel := context.WithCancel(context.Background())
-			defer cancel()
 			deps := restoreTestDependencies(t, runner)
 			deps.prepareRecovery = func(ctx context.Context, target config.Installation, _ string) (PreflightResult, error) {
 				return Preflight(ctx, target, PreflightRequest{Archive: recoveryArchive, Confirm: true, AllowExternalSecrets: true}, permissivePreflightDependencies())
@@ -426,32 +484,66 @@ func TestRestoreLifecycleLockExcludesCompetingTransactionsUntilTerminalCleanup(t
 			}
 			scenario.configure(&deps, runner, gate, cancel)
 
-			type outcome struct {
-				result RestoreResult
-				err    error
-			}
-			done := make(chan outcome, 1)
+			done := make(chan restoreLifecycleTestOutcome, 1)
 			go func() {
 				result, err := restoreWithDependencies(caller, installation, RestoreRequest{Archive: archive, Confirm: true}, deps)
-				done <- outcome{result: result, err: err}
+				done <- restoreLifecycleTestOutcome{result: result, err: err}
 			}()
 
 			var failures []error
-			for _, wantStage := range scenario.stages {
+			var terminal *restoreLifecycleTestOutcome
+			for stageIndex, wantStage := range scenario.stages {
+				receivedStage := false
 				select {
 				case stage := <-stages:
+					receivedStage = true
 					if stage != wantStage {
 						failures = append(failures, fmt.Errorf("lifecycle stage = %q, want %q", stage, wantStage))
 					}
+				case outcome := <-done:
+					terminal = &outcome
+					failures = append(failures, fmt.Errorf("restore ended before lifecycle stage %q: %w", wantStage, outcome.err))
 				case <-time.After(2 * time.Second):
-					failures = append(failures, fmt.Errorf("timed out waiting for lifecycle stage %q", wantStage))
+					select {
+					case outcome := <-done:
+						terminal = &outcome
+						failures = append(failures, fmt.Errorf("restore ended before lifecycle stage %q: %w", wantStage, outcome.err))
+					default:
+						failures = append(failures, fmt.Errorf("timed out waiting for lifecycle stage %q", wantStage))
+						cancel()
+					}
+				}
+				if terminal != nil || !receivedStage {
+					break
 				}
 				if err := competingRestoreAndBackupEntry(installation, archive, t); err != nil {
 					failures = append(failures, fmt.Errorf("%s: %w", wantStage, err))
 				}
-				continueStage <- struct{}{}
+				outcome, workerDone, err := releaseLifecycleStage(caller, continueStage, done)
+				if err != nil {
+					failures = append(failures, fmt.Errorf("release lifecycle stage %q: %w", wantStage, err))
+					cancel()
+					break
+				}
+				if workerDone {
+					terminal = &outcome
+					if stageIndex+1 < len(scenario.stages) {
+						failures = append(failures, fmt.Errorf("restore ended before lifecycle stage %q: %w", scenario.stages[stageIndex+1], outcome.err))
+					}
+					break
+				}
+			}
+			var result restoreLifecycleTestOutcome
+			if terminal != nil {
+				result = *terminal
+			} else {
+				select {
+				case result = <-done:
+				case <-time.After(2 * time.Second):
+					failures = append(failures, errors.New("timed out waiting for restore worker outcome"))
+					cancel()
+				}
 			}
-			result := <-done
 			for _, wantErr := range scenario.wantErrors {
 				if !errors.Is(result.err, wantErr) {
 					failures = append(failures, fmt.Errorf("restore error = %v, want %v", result.err, wantErr))

tools/tht/internal/safeio/private_root_windows.go

@@ -539,13 +539,30 @@ func markWindowsHandleForDelete(handle windows.Handle) error {
 	return nil
 }
 
-func closeAndDeleteWindowsPrivateRegular(value *windowsPrivateRegularAt) error {
-	if value == nil || value.handle == 0 || markWindowsHandleForDelete(value.handle) != nil || value.Close() != nil {
+func finishWindowsPrivateRegularCleanup(mark func() error, close func() error) error {
+	failed := false
+	if mark == nil || mark() != nil {
+		failed = true
+	}
+	if close == nil || close() != nil {
+		failed = true
+	}
+	if failed {
 		return ErrUnsafeFile
 	}
 	return nil
 }
 
+func closeAndDeleteWindowsPrivateRegular(value *windowsPrivateRegularAt) error {
+	if value == nil || value.handle == 0 {
+		return ErrUnsafeFile
+	}
+	return finishWindowsPrivateRegularCleanup(
+		func() error { return markWindowsHandleForDelete(value.handle) },
+		value.Close,
+	)
+}
+
 func (directory *windowsPrivateDirectory) CreateRegular(name string, contents []byte) (bool, error) {
 	if directory.Validate() != nil || !validPrivateLeafName(name) || len(contents) == 0 {
 		return false, ErrUnsafeFile
@@ -554,7 +571,11 @@ func (directory *windowsPrivateDirectory) CreateRegular(name string, contents []
 	if err != nil {
 		existing, existingErr := openWindowsPrivateRegularAt(directory.handle, name, windows.FILE_GENERIC_READ, 1)
 		if existingErr == nil {
-			_ = existing.Close()
+			closeErr := existing.Close()
+			validateErr := directory.Validate()
+			if closeErr != nil || validateErr != nil {
+				return false, ErrUnsafeFile
+			}
 			return false, nil
 		}
 		return false, ErrUnsafeFile
@@ -720,15 +741,27 @@ func (directory *windowsPrivateDirectory) ReplaceRegular(name string, contents [
 		}
 	}()
 	current, err := openWindowsPrivateRegularAt(directory.handle, name, windows.FILE_GENERIC_READ, 1)
-	if err != nil || current.Close() != nil || directory.Validate() != nil {
+	if err != nil {
 		return ErrUnsafeFile
 	}
-	if renameWindowsPrivateRegularAt(temporary.handle, directory.handle, name) != nil || temporary.Close() != nil {
+	currentCloseErr := current.Close()
+	currentValidateErr := directory.Validate()
+	if currentCloseErr != nil || currentValidateErr != nil {
+		return ErrUnsafeFile
+	}
+	renameErr := renameWindowsPrivateRegularAt(temporary.handle, directory.handle, name)
+	temporaryCloseErr := temporary.Close()
+	if renameErr != nil || temporaryCloseErr != nil {
 		return ErrUnsafeFile
 	}
 	renamed = true
 	replaced, err := openWindowsPrivateRegularAt(directory.handle, name, windows.FILE_GENERIC_READ, 1)
-	if err != nil || replaced.Close() != nil || directory.Validate() != nil {
+	if err != nil {
+		return ErrUnsafeFile
+	}
+	replacedCloseErr := replaced.Close()
+	replacedValidateErr := directory.Validate()
+	if replacedCloseErr != nil || replacedValidateErr != nil {
 		return ErrUnsafeFile
 	}
 	return nil
@@ -745,7 +778,9 @@ func (directory *windowsPrivateDirectory) RemoveRegular(name string) (bool, erro
 	if err != nil {
 		return false, ErrUnsafeFile
 	}
-	if closeAndDeleteWindowsPrivateRegular(value) != nil || directory.Validate() != nil {
+	cleanupErr := closeAndDeleteWindowsPrivateRegular(value)
+	validateErr := directory.Validate()
+	if cleanupErr != nil || validateErr != nil {
 		return false, ErrUnsafeFile
 	}
 	return true, nil
@@ -839,6 +874,19 @@ func sameWindowsRelativeClaim(source, claim *windowsPrivateRegularAt) bool {
 	return source != nil && claim != nil && sameWindowsPrivateFile(source.info, claim.info)
 }
 
+func finishWindowsClaimCleanup(closeClaim, deleteSource, deleteClaim, validate func() error) error {
+	failed := false
+	for _, operation := range []func() error{closeClaim, deleteSource, deleteClaim, validate} {
+		if operation == nil || operation() != nil {
+			failed = true
+		}
+	}
+	if failed {
+		return ErrUnsafeFile
+	}
+	return nil
+}
+
 func windowsRelativeClaimPairExists(directory *windowsPrivateDirectory, source, claim string) (bool, error) {
 	left, err := openWindowsPrivateRegularAt(directory.handle, source, windows.FILE_GENERIC_READ, 2)
 	if isWindowsRelativeNotFound(err) {
@@ -981,21 +1029,37 @@ func (directory *windowsPrivateDirectory) RemoveClaim(source, claim string) (boo
 		2,
 	)
 	if isWindowsRelativeNotFound(err) {
-		_ = value.Close()
+		closeErr := value.Close()
+		validateErr := directory.Validate()
+		if closeErr != nil || validateErr != nil {
+			return false, ErrUnsafeFile
+		}
 		return false, nil
 	}
 	if err != nil || !sameWindowsRelativeClaim(value, claimed) {
-		_ = value.Close()
+		valueCloseErr := value.Close()
+		claimedCloseErr := error(nil)
 		if claimed != nil {
-			_ = claimed.Close()
+			claimedCloseErr = claimed.Close()
+		}
+		if valueCloseErr != nil || claimedCloseErr != nil || directory.Validate() != nil {
+			return false, ErrUnsafeFile
 		}
 		return false, ErrUnsafeFile
 	}
-	if claimed.Close() != nil || closeAndDeleteWindowsPrivateRegular(value) != nil {
-		return false, ErrUnsafeFile
-	}
-	remaining, err := openWindowsPrivateRegularAt(directory.handle, claim, windows.FILE_GENERIC_READ|windows.DELETE, 1)
-	if err != nil || closeAndDeleteWindowsPrivateRegular(remaining) != nil || directory.Validate() != nil {
+	cleanupErr := finishWindowsClaimCleanup(
+		claimed.Close,
+		func() error { return closeAndDeleteWindowsPrivateRegular(value) },
+		func() error {
+			remaining, remainingErr := openWindowsPrivateRegularAt(directory.handle, claim, windows.FILE_GENERIC_READ|windows.DELETE, 1)
+			if remainingErr != nil {
+				return ErrUnsafeFile
+			}
+			return closeAndDeleteWindowsPrivateRegular(remaining)
+		},
+		directory.Validate,
+	)
+	if cleanupErr != nil {
 		return false, ErrUnsafeFile
 	}
 	return true, nil

tools/tht/internal/safeio/private_windows.go

@@ -316,7 +316,7 @@ func validateOwnerOnlyDACL(handle windows.Handle) error {
 			return ErrUnsafeFile
 		}
 		var ace *windows.ACCESS_ALLOWED_ACE
-		if err := windows.GetAce(dacl, 0, &ace); err != nil || ace == nil || ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE || ace.Header.AceFlags != 0 || ace.Mask != windows.GENERIC_ALL {
+		if err := windows.GetAce(dacl, 0, &ace); err != nil || ace == nil || ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE || ace.Header.AceFlags != 0 || !isOwnerOnlyFullControlMask(uint32(ace.Mask)) {
 			return ErrUnsafeFile
 		}
 		aceSID := (*windows.SID)(unsafe.Pointer(&ace.SidStart))
@@ -327,6 +327,15 @@ func validateOwnerOnlyDACL(handle windows.Handle) error {
 	})
 }
 
+func isOwnerOnlyFullControlMask(mask uint32) bool {
+	// Windows may persist GENERIC_ALL in the ACE or expand it to the file-object
+	// full-control mask (including FILE_DELETE_CHILD). Both are the same semantic
+	// authority; any additional bit remains unsafe.
+	const fileDeleteChild = uint32(0x40)
+	effective := uint32(windows.FILE_GENERIC_READ|windows.FILE_GENERIC_WRITE|windows.FILE_GENERIC_EXECUTE|windows.DELETE) | fileDeleteChild
+	return mask == uint32(windows.GENERIC_ALL) || mask == effective
+}
+
 // withWindowsSecurityDescriptor confines inspection to x/sys's Go-owned descriptor copy. Its
 // GetSecurityInfo wrapper releases the native LocalAlloc result with LocalFree before returning.
 func withWindowsSecurityDescriptor(handle windows.Handle, inspect func(*windows.SECURITY_DESCRIPTOR) error) error {

tools/tht/internal/safeio/private_windows_test.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 
 	"golang.org/x/sys/windows"
@@ -53,6 +54,68 @@ func TestPrivateWindowsDACLRejectsPermissiveDirectoryAndRegularFile(t *testing.T
 	}
 }
 
+func TestOwnerOnlyDACLAcceptsWindowsFullControlMask(t *testing.T) {
+	const fileDeleteChild = uint32(0x40)
+	effectiveFullControl := uint32(windows.FILE_GENERIC_READ|windows.FILE_GENERIC_WRITE|windows.FILE_GENERIC_EXECUTE|windows.DELETE) | fileDeleteChild
+	if !isOwnerOnlyFullControlMask(effectiveFullControl) {
+		t.Fatalf("effective Windows full-control mask %#x was rejected", effectiveFullControl)
+	}
+	if !isOwnerOnlyFullControlMask(uint32(windows.GENERIC_ALL)) {
+		t.Fatal("generic full-control mask was rejected")
+	}
+	if isOwnerOnlyFullControlMask(effectiveFullControl | uint32(windows.ACCESS_SYSTEM_SECURITY)) {
+		t.Fatal("full-control mask with an extra right was accepted")
+	}
+}
+
+func TestWindowsPrivateRegularCleanupClosesAfterDeleteDispositionFailure(t *testing.T) {
+	var calls []string
+	err := finishWindowsPrivateRegularCleanup(
+		func() error {
+			calls = append(calls, "delete")
+			return ErrUnsafeFile
+		},
+		func() error {
+			calls = append(calls, "close")
+			return nil
+		},
+	)
+	if !errors.Is(err, ErrUnsafeFile) {
+		t.Fatalf("finishWindowsPrivateRegularCleanup() error = %v, want ErrUnsafeFile", err)
+	}
+	if got, want := strings.Join(calls, ","), "delete,close"; got != want {
+		t.Fatalf("cleanup order = %q, want %q", got, want)
+	}
+}
+
+func TestWindowsClaimCleanupAttemptsLaterOperationsAfterEarlierFailure(t *testing.T) {
+	var calls []string
+	err := finishWindowsClaimCleanup(
+		func() error {
+			calls = append(calls, "claim-close")
+			return ErrUnsafeFile
+		},
+		func() error {
+			calls = append(calls, "source-delete")
+			return nil
+		},
+		func() error {
+			calls = append(calls, "claim-delete")
+			return nil
+		},
+		func() error {
+			calls = append(calls, "validate")
+			return nil
+		},
+	)
+	if !errors.Is(err, ErrUnsafeFile) {
+		t.Fatalf("finishWindowsClaimCleanup() error = %v, want ErrUnsafeFile", err)
+	}
+	if got, want := strings.Join(calls, ","), "claim-close,source-delete,claim-delete,validate"; got != want {
+		t.Fatalf("cleanup order = %q, want %q", got, want)
+	}
+}
+
 func TestCreateCanonicalNewPrivateFileInstallsOwnerOnlyDACLAtCreation(t *testing.T) {
 	directory := filepath.Join(t.TempDir(), "auth")
 	if err := os.Mkdir(directory, 0o700); err != nil {