diff --git a/.github/workflows/deployment.yml b/.github/workflows/deployment.yml index ff35a0d2..200fa58b 100644 --- a/.github/workflows/deployment.yml +++ b/.github/workflows/deployment.yml @@ -149,7 +149,7 @@ jobs: cache-dependency-path: tools/tht/go.sum - name: Run native Windows retained-capability tests working-directory: tools/tht - run: go test ./internal/safeio ./internal/backup -count=1 + run: go test ./internal/safeio ./internal/backup ./internal/authstorage -count=1 - name: Verify Windows clone contract shell: pwsh run: ./scripts/test-windows-clone-contract.ps1 diff --git a/docs/superpowers/plans/2026-08-18-thothii-authentication-remediation.md b/docs/superpowers/plans/2026-08-18-thothii-authentication-remediation.md index 3bab5f93..347f2ec3 100644 --- a/docs/superpowers/plans/2026-08-18-thothii-authentication-remediation.md +++ b/docs/superpowers/plans/2026-08-18-thothii-authentication-remediation.md @@ -536,7 +536,7 @@ In the existing `windows-clone` job, after Go setup and before the clone-contrac ```yaml - name: Run native Windows retained-capability tests working-directory: tools/tht - run: go test ./internal/safeio ./internal/backup -count=1 + run: go test ./internal/safeio ./internal/backup ./internal/authstorage -count=1 ``` This job is the native execution authority. A Linux/macOS cross-compile only proves buildability. @@ -771,6 +771,7 @@ go vet ./... go build ./cmd/tht GOOS=windows GOARCH=amd64 go test -c ./internal/safeio -o /tmp/tht-safeio-final-windows.test.exe GOOS=windows GOARCH=amd64 go test -c ./internal/backup -o /tmp/tht-backup-final-windows.test.exe +GOOS=windows GOARCH=amd64 go test -c ./internal/authstorage -o /tmp/tht-authstorage-final-windows.test.exe GOOS=windows GOARCH=amd64 go build -o /tmp/tht-final-windows.exe ./cmd/tht ``` diff --git a/tools/tht/internal/backup/create_test.go b/tools/tht/internal/backup/create_test.go index d43f8f66..5e354e33 100644 --- a/tools/tht/internal/backup/create_test.go +++ b/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) diff --git a/tools/tht/internal/backup/fixture_security_unix_test.go b/tools/tht/internal/backup/fixture_security_unix_test.go new file mode 100644 index 00000000..830f85fe --- /dev/null +++ b/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() +} diff --git a/tools/tht/internal/backup/fixture_security_windows_test.go b/tools/tht/internal/backup/fixture_security_windows_test.go new file mode 100644 index 00000000..378dc912 --- /dev/null +++ b/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) + } + } +} diff --git a/tools/tht/internal/backup/preflight_test.go b/tools/tht/internal/backup/preflight_test.go index 76a1dcc1..e31b8993 100644 --- a/tools/tht/internal/backup/preflight_test.go +++ b/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) } diff --git a/tools/tht/internal/backup/restore_test.go b/tools/tht/internal/backup/restore_test.go index c9cd6bab..87075a0e 100644 --- a/tools/tht/internal/backup/restore_test.go +++ b/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)) diff --git a/tools/tht/internal/safeio/private_root_windows.go b/tools/tht/internal/safeio/private_root_windows.go index a5007d0b..abde8098 100644 --- a/tools/tht/internal/safeio/private_root_windows.go +++ b/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 diff --git a/tools/tht/internal/safeio/private_windows.go b/tools/tht/internal/safeio/private_windows.go index b8af531a..dad9f809 100644 --- a/tools/tht/internal/safeio/private_windows.go +++ b/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 { diff --git a/tools/tht/internal/safeio/private_windows_test.go b/tools/tht/internal/safeio/private_windows_test.go index 742c2bd6..a208fd42 100644 --- a/tools/tht/internal/safeio/private_windows_test.go +++ b/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 {