fix(windows): CI matrix, zip archives, and NTFS ACL for sensitive files

[lexfrei]

Summary

Close the Windows support loop. The original "template path with backslash" symptom was already fixed upstream of this branch (engine-side NormalizeTemplatePath in v0.22.0), but nothing proved it stayed fixed — CI only ran on Ubuntu, so the Windows-tagged tests never executed, and the release archive was a .tar.gz that stock Windows cannot open. This PR makes Windows a first-class target and closes the security gap that becomes visible once Windows users can actually run talm.

Closes #11

What changes

1. Windows CI runner

.github/workflows/pr.yml runs go test ./... on both ubuntu-latest and windows-latest with fail-fast: false. The existing engine_windows_test.go (build-tagged //go:build windows) now actually executes. Any future regression in Windows-specific path handling fails CI.

2. Windows zip archives

.goreleaser.yaml uses format_overrides so Windows binaries ship as .zip. Linux and macOS remain .tar.gz.

3. Regression tests for backslash paths

Users running talm from PowerShell pass paths with \ separators, which must be normalized before reaching the helm engine's forward-slash-only map keys. Both code paths that touch template arguments now have Windows-tagged tests:

• apply_windows_test.go::TestResolveTemplatePaths_BackslashInput covers talm apply via resolveTemplatePaths.
• template_windows_test.go::TestResolveEngineTemplatePaths_BackslashInput covers talm template via the newly-extracted resolveEngineTemplatePaths.

The two resolvers are intentionally different (apply resolves against rootDir, template against CWD with a templates/<basename> fallback), so they need separate coverage.

4. New pkg/secureperm package for sensitive files

A drop-in replacement for os.WriteFile(path, data, 0o600) and os.Chmod(path, 0o600). On Unix it delegates to those calls with an explicit follow-up Chmod (required because os.WriteFile preserves prior mode bits on overwrite). On Windows it installs a protected NTFS DACL with a single Allow ACE for the current user SID.

WriteFile is also atomic on both platforms via write-to-tmp + rename — a failure mid-write leaves the original file intact. Secrets files are not reconstructible (corrupting secrets.yaml forces a cluster PKI reissue), so non-atomic writes are a real hazard.

Call sites migrated:

• pkg/age/age.go — talm.key creation, decrypted secrets.yaml, generic decrypted YAML.
• pkg/commands/init.go — talosconfig, secrets.yaml bundle (via the new writeSecureToDestination helper).
• pkg/commands/rotate_ca_handler.go — rotated secrets.yaml.
• pkg/commands/talosconfig.go — regenerated talosconfig.
• pkg/commands/kubeconfig_handler.go — kubeconfig tighten-after-fetch.
• pkg/commands/template.go — rendered machine config via talm template --inplace (embeds certs + PKI keys + cluster join tokens).

Tests cover the atomic-write contract on both OSes (TestWriteFile_PreservesOriginalOnFailure_{Unix,Windows}), the DACL shape on Windows (TestWriteFile_*_DACL_Windows parse the SDDL string and assert D:P + exactly one Allow ACE for the current user SID), overwrite-downgrade behavior on Unix (TestWriteFile_OverwriteDowngrades_Unix), and the new-file happy path on both.

5. Collateral cleanups

Three small defects surfaced while working in this area:

• pkg/commands/kubeconfig_handler.go had a vacuous if err == nil { ... } wrapper around the entire 60-line post-write block, guarding a condition that was already checked with early-return one line above. The wrapper also shadowed err with inner filepath.Abs assignments, making the outer variable meaningless. Removed.
• pkg/commands/init.go::writeToDestination printed Created <path> unconditionally after the write, so a permission failure produced a confusing "Created foo.key" + error pair. Both writeToDestination and the new writeSecureToDestination now gate the message on err == nil. Pinned by TestWriteToDestination_SilentOnFailure and TestWriteToDestination_AnnouncesOnSuccess.
• writeSecretsBundleToFile called validateFileExists twice — the outer call was redundant with the same check inside writeSecureToDestination. Removed.

6. README

New "Windows" installation subsection pointing at the release zip and documenting the -t / --template separator equivalence.

Verification

• go test ./... on macOS — pass
• GOOS=windows GOARCH=amd64 go build ./... — clean
• GOOS=windows GOARCH=amd64 go vet ./... — clean
• GOOS=windows GOARCH=amd64 go test -c -o /dev/null ./... — all test packages compile for Windows
• golangci-lint run ./... — 0 issues
• CI matrix (ubuntu-latest + windows-latest) is exercised by this PR itself — watch for --- PASS on both runners

Summary by CodeRabbit

• New Features

  • Added full Windows platform support with signed binary releases and installation documentation.
  • Implemented secure file writing for sensitive outputs (private keys, configurations, decrypted secrets).
  • Enhanced Windows path handling with backslash support for template flags.

• Bug Fixes

  • Consistent file permission restrictions for sensitive files across all platforms.

• Tests

  • Expanded test coverage for Windows and Unix-specific behaviors.

• Chores

  • Updated system dependencies.

[coderabbitai]

Warning

Rate limit exceeded

@lexfrei has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 50 minutes and 4 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 4 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 92a3b782-a7e3-4ee3-8d3d-de49e96a6e7c

📥 Commits

Reviewing files that changed from the base of the PR and between 8d4d0a0 and 855b13b.

📒 Files selected for processing (3)

• pkg/commands/apply_test.go
• pkg/commands/template_windows_test.go
• pkg/secureperm/secureperm_windows_test.go

📝 Walkthrough

Walkthrough

The PR introduces a cross-platform secure file handling mechanism through a new secureperm package that atomically writes sensitive files (keys, configurations, secrets) with owner-only permissions on both Unix and Windows. Multiple command handlers are refactored to use this package instead of direct OS calls, Windows CI testing is added, and documentation and platform-specific tests are extended.

Changes

Cohort / File(s)	Summary
CI/Build Configuration .github/workflows/pr.yml, .goreleaser.yaml, go.mod	Added Windows test matrix in GitHub Actions workflow, configured Windows-specific archive format in goreleaser, and promoted golang.org/x/sys from indirect to direct dependency.
Documentation README.md	Added Windows installation section describing .zip archive extraction, talm.exe binary usage, and path separator handling (\ and / equivalence) for template flag.
Secure Permission Package pkg/secureperm/secureperm.go, pkg/secureperm/secureperm_unix.go, pkg/secureperm/secureperm_windows.go, pkg/secureperm/secureperm_test.go, pkg/secureperm/secureperm_unix_test.go, pkg/secureperm/secureperm_windows_test.go	New package implementing atomic secure file writing with owner-only permissions; Unix uses os.CreateTemp + chmod + rename, Windows uses protected DACL via Windows API. Includes cross-platform and platform-specific tests validating permission enforcement and atomicity on failure.
Age Command pkg/age/age.go, pkg/age/age_unix_test.go	Replaced os.WriteFile(..., 0o600) with secureperm.WriteFile(...) for key/secret outputs; added Unix test verifying 0o600 permission enforcement on generated key file.
Init Command pkg/commands/init.go, pkg/commands/init_test.go	Added writeSecureToDestination helper delegating to secureperm.WriteFile for sensitive outputs; introduced createdSink (io.Writer) for testable output routing; added three tests covering write/overwrite control flow and failure modes.
Template Command pkg/commands/template.go, pkg/commands/template_unix_test.go, pkg/commands/template_windows_test.go	Extracted resolveEngineTemplatePaths helper for cross-platform template path resolution; added writeInplaceRendered helper using secureperm.WriteFile for in-place config updates; added Unix tests for permission enforcement and Windows tests for backslash normalization and DACL protection.
Apply Command pkg/commands/apply_windows_test.go	Added Windows tests for resolveTemplatePaths validating backslash-to-forward-slash conversion and outside-root path handling.
Kubeconfig Handler pkg/commands/kubeconfig_handler.go	Replaced os.Chmod(absPath, 0o600) with secureperm.LockDown(absPath); restructured control flow to remove surrounding if err == nil gate, allowing subsequent operations to run independently.
Talosconfig Command pkg/commands/talosconfig.go	Replaced os.WriteFile(..., 0o600) with secureperm.WriteFile(...) in regenerateTalosconfig function.
Rotate CA Handler pkg/commands/rotate_ca_handler.go	Replaced os.WriteFile(secretsPath, data, 0o600) with secureperm.WriteFile(secretsPath, data) for secrets file writing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hops with glee through platform lands,
Secrets locked with steady hands,
Windows, Unix, both now blessed,
Permissions tight—the safest nest!
Keys and configs, secure and sound,
Cross-platform magic all around! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 79.07% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly summarizes the main changes: CI matrix setup, Windows zip archives, and NTFS ACL enforcement for sensitive files, directly reflecting the key modifications in the changeset.
Linked Issues check	✅ Passed	The PR comprehensively addresses issue #11 (Windows support) by enabling Windows CI, fixing archive formats, implementing cross-platform secure file handling, adding Windows-specific tests, and documenting Windows installation steps.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to enabling Windows support: CI configuration, archive formats, permission handling, path normalization tests, secure file writing, and documentation. No unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/windows-support

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, reopen this pull request to trigger a review.

[gemini-code-assist]

Code Review

This pull request implements cross-platform secure file handling by introducing the secureperm package, which ensures sensitive files like private keys and secrets are written with owner-only permissions on both Unix (0600) and Windows (protected DACLs). It also improves Windows support by refactoring template path resolution to handle backslashes and updating the release configuration. A security improvement was suggested to use more restrictive permissions (0700) when creating parent directories for sensitive files in the init command to prevent them from being world-readable.

[gemini-code-assist]

Since writeSecureToDestination is specifically designed for sensitive files (like talm.key and secrets.yaml), the parent directory should be created with restrictive permissions (0700) on Unix-like systems. Using os.ModePerm (0777) relies entirely on the system umask and may leave the directory world-readable or world-writable in some environments, which is inconsistent with the goal of this security-focused helper.

Suggested change

	if err := os.MkdirAll(parentDir, os.ModePerm); err != nil {
	if err := os.MkdirAll(parentDir, 0700); err != nil {

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (2)

pkg/commands/kubeconfig_handler.go (1)

104-162: LockDown swap looks correct; minor duplication in root-abs resolution.

secureperm.LockDown is the right replacement for os.Chmod here — the Windows branch now actually tightens the DACL instead of silently no-op'ing. The warning-on-failure behavior is preserved.

Minor: filepath.Abs(Config.RootDir) is computed twice (Line 111 and Line 139) for two independent "is this inside the project root?" checks. Hoisting it once keeps the two blocks in sync if the root ever changes and avoids a redundant syscall.

♻️ Proposed hoist

+		rootAbs, rootAbsErr := filepath.Abs(Config.RootDir)
+
 		// Set secure permissions (600) on kubeconfig file. On Windows
 		// this lays down an NTFS DACL; os.Chmod would have been a no-op.
 		if err := secureperm.LockDown(absPath); err != nil {
 			// Don't fail the command if the tighten fails, but log warning
 			fmt.Fprintf(os.Stderr, "Warning: failed to set permissions on kubeconfig: %v\n", err)
 		}
 
-		rootAbs, err := filepath.Abs(Config.RootDir)
-		if err == nil {
+		if rootAbsErr == nil {
 			relPath, err := filepath.Rel(rootAbs, absPath)
 			...
 		}
 		...
 		if !loginFlagValue {
-			// Get relative path from project root for encryption
-			rootAbs, err := filepath.Abs(Config.RootDir)
-			if err == nil {
+			if rootAbsErr == nil {
 				relKubeconfigPath, err := filepath.Rel(rootAbs, absPath)
 				...
 			}
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/commands/kubeconfig_handler.go` around lines 104 - 162, Hoist the
duplicate call to filepath.Abs(Config.RootDir) into a single rootAbs variable
computed once before the two "is inside project root?" checks so both blocks
reuse it; locate the code around secureperm.LockDown and the later block gated
by loginFlagValue that computes relPath and relKubeconfigPath and replace the
second filepath.Abs(Config.RootDir) with the previously computed rootAbs,
keeping the existing nil-checks and semantics (use the same err variable
handling for the initial abs calculation and then use rootAbs when calling
filepath.Rel for both absPath -> relPath and absPath -> relKubeconfigPath).

pkg/secureperm/secureperm_windows.go (1)

146-177: WriteFile atomicity and cleanup look correct.

Handle ownership is transferred cleanly to os.File via os.NewFile(uintptr(handle), …), the committed flag gates cleanup correctly, double-Close on the success path is a no-op on *os.File, and os.Remove(tmpPath) on any pre-rename error preserves the original destination — matching the contract documented in secureperm.go and exercised by TestWriteFile_PreservesOriginalOnFailure_Windows.

One optional hardening idea for later: consider f.Sync() before Close() on Windows for the same crash-durability reason you'd want it on Unix. Not a blocker; the atomicity contract here is about "no partial destination visible", not fsync durability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/secureperm/secureperm_windows.go` around lines 146 - 177, Add an optional
durability hardening by calling f.Sync() before closing the temporary file in
WriteFile to flush data to disk on Windows: after writing to f and before the
existing f.Close() call, invoke f.Sync() and handle/return any error (wrap with
the same "close tmp" style or a new "sync tmp" message), keeping the
tmpPath/handle/f/committed logic and cleanup via createSecureTmp intact so the
atomic rename behavior is unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/commands/template_unix_test.go`:
- Around line 56-59: The test currently seeds the file using os.WriteFile(path,
[]byte("old"), 0o644) which is subject to the process umask; to ensure the test
truly starts with mode 0644 before calling writeInplaceRendered, explicitly set
the file mode after creation using os.Chmod(path, 0o644). Update the test in
pkg/commands/template_unix_test.go (around the seed + writeInplaceRendered call)
to call os.Chmod(path, 0o644) after os.WriteFile and before calling
writeInplaceRendered so the test reliably verifies that writeInplaceRendered
tightens permissions.

In `@pkg/commands/template.go`:
- Around line 423-435: The check using strings.HasPrefix(relPath, "..") is too
broad and can misclassify paths whose first path element merely starts with
".."; change this to test the first path element exactly equals ".." by
splitting relPath with filepath.SplitList or strings.SplitN using
string(os.PathSeparator)/filepath.Separator (e.g., first :=
strings.SplitN(relPath, string(filepath.Separator), 2)[0]) and then use if first
== ".." { ... } so the rest of the logic (building possiblePath, stat check, and
setting resolved[i] = engine.NormalizeTemplatePath(...)) remains unchanged.

In `@pkg/secureperm/secureperm_unix_test.go`:
- Around line 48-75: The tests assume os.WriteFile(path, 0o644) yields lax perms
but umask can tighten them; make the precondition independent of umask by
explicitly forcing the lax mode after seeding: after the os.WriteFile calls in
TestWriteFile_OverwriteDowngrades_Unix and the other test that seeds a 0644
file, call os.Chmod(path, 0o644) (or equivalent) and check for errors before
invoking secureperm.WriteFile or secureperm.LockDown so the tests actually
verify that those functions tighten permissions.

In `@pkg/secureperm/secureperm_unix.go`:
- Around line 38-69: The WriteFile function must fsync the temporary file and
the containing directory to guarantee durability: after writing to f (and before
closing it) call f.Sync and handle/return any error; after os.Rename(tmpPath,
path) open the parent directory (using dir from filepath.Dir(path)), call Sync
on that directory file descriptor and return any error, ensuring committed is
only set true after both fsyncs succeed; keep existing cleanup/defer behavior
and error wrapping consistent with the surrounding error messages.

In `@pkg/secureperm/secureperm_windows_test.go`:
- Around line 30-38: Update the comment for
assertProtectedOwnerOnlyDACL/ownerOnlyDescriptor in secureperm_windows_test.go
to remove the incorrect `AI` inheritance flag: change the example SDDL from
`D:PAI(A;;FA;;;<USER-SID>)` to `D:P(A;;FA;;;<USER-SID>)` (or
`D:P(A;;FA;;;<current-user-SID>)`) and note that `AI` is stripped when
SE_DACL_PROTECTED is set so a protected owner-only DACL should not include `AI`.

---

Nitpick comments:
In `@pkg/commands/kubeconfig_handler.go`:
- Around line 104-162: Hoist the duplicate call to filepath.Abs(Config.RootDir)
into a single rootAbs variable computed once before the two "is inside project
root?" checks so both blocks reuse it; locate the code around
secureperm.LockDown and the later block gated by loginFlagValue that computes
relPath and relKubeconfigPath and replace the second
filepath.Abs(Config.RootDir) with the previously computed rootAbs, keeping the
existing nil-checks and semantics (use the same err variable handling for the
initial abs calculation and then use rootAbs when calling filepath.Rel for both
absPath -> relPath and absPath -> relKubeconfigPath).

In `@pkg/secureperm/secureperm_windows.go`:
- Around line 146-177: Add an optional durability hardening by calling f.Sync()
before closing the temporary file in WriteFile to flush data to disk on Windows:
after writing to f and before the existing f.Close() call, invoke f.Sync() and
handle/return any error (wrap with the same "close tmp" style or a new "sync
tmp" message), keeping the tmpPath/handle/f/committed logic and cleanup via
createSecureTmp intact so the atomic rename behavior is unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0cda9ea0-9793-4fa3-bacb-3440bb6a419d

📥 Commits

Reviewing files that changed from the base of the PR and between 00358fd and 8d4d0a0.

📒 Files selected for processing (21)

• .github/workflows/pr.yml
• .goreleaser.yaml
• README.md
• go.mod
• pkg/age/age.go
• pkg/age/age_unix_test.go
• pkg/commands/apply_windows_test.go
• pkg/commands/init.go
• pkg/commands/init_test.go
• pkg/commands/kubeconfig_handler.go
• pkg/commands/rotate_ca_handler.go
• pkg/commands/talosconfig.go
• pkg/commands/template.go
• pkg/commands/template_unix_test.go
• pkg/commands/template_windows_test.go
• pkg/secureperm/secureperm.go
• pkg/secureperm/secureperm_test.go
• pkg/secureperm/secureperm_unix.go
• pkg/secureperm/secureperm_unix_test.go
• pkg/secureperm/secureperm_windows.go
• pkg/secureperm/secureperm_windows_test.go

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Force the seed file to 0644 before testing the downgrade.

os.WriteFile(..., 0o644) is still filtered by the process umask, so this test may start from 0600 locally and pass without proving that writeInplaceRendered tightened a lax file.

🧪 Proposed test hardening

 	if err := os.WriteFile(path, []byte("old"), 0o644); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
+	if err := os.Chmod(path, 0o644); err != nil {
+		t.Fatalf("chmod seed: %v", err)
+	}
 	if err := writeInplaceRendered(path, "new"); err != nil {
 		t.Fatalf("writeInplaceRendered: %v", err)
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if err := os.WriteFile(path, []byte("old"), 0o644); err != nil {
	t.Fatalf("seed: %v", err)
	}
	if err := writeInplaceRendered(path, "new"); err != nil {
	if err := os.WriteFile(path, []byte("old"), 0o644); err != nil {
	t.Fatalf("seed: %v", err)
	}
	if err := os.Chmod(path, 0o644); err != nil {
	t.Fatalf("chmod seed: %v", err)
	}
	if err := writeInplaceRendered(path, "new"); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/commands/template_unix_test.go` around lines 56 - 59, The test currently
seeds the file using os.WriteFile(path, []byte("old"), 0o644) which is subject
to the process umask; to ensure the test truly starts with mode 0644 before
calling writeInplaceRendered, explicitly set the file mode after creation using
os.Chmod(path, 0o644). Update the test in pkg/commands/template_unix_test.go
(around the seed + writeInplaceRendered call) to call os.Chmod(path, 0o644)
after os.WriteFile and before calling writeInplaceRendered so the test reliably
verifies that writeInplaceRendered tightens permissions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Tighten the outside-root check to match a path element, not a prefix.

strings.HasPrefix(relPath, "..") misclassifies valid paths under rootDir whose first component starts with .., such as ..templates/controlplane.yaml, and may fall back to the wrong templates/<basename> file.

🐛 Proposed fix

 		}
 		relPath = filepath.Clean(relPath)
-		if strings.HasPrefix(relPath, "..") {
+		if relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) {
 			templateName := filepath.Base(templatePath)
 			possiblePath := filepath.Join("templates", templateName)
 			fullPath := filepath.Join(absRootDir, possiblePath)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/commands/template.go` around lines 423 - 435, The check using
strings.HasPrefix(relPath, "..") is too broad and can misclassify paths whose
first path element merely starts with ".."; change this to test the first path
element exactly equals ".." by splitting relPath with filepath.SplitList or
strings.SplitN using string(os.PathSeparator)/filepath.Separator (e.g., first :=
strings.SplitN(relPath, string(filepath.Separator), 2)[0]) and then use if first
== ".." { ... } so the rest of the logic (building possiblePath, stat check, and
setting resolved[i] = engine.NormalizeTemplatePath(...)) remains unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Make the lax-permission precondition independent of umask.

Both tests rely on os.WriteFile(..., 0o644) creating a lax file, but a restrictive umask can produce 0600, letting the tests pass without proving LockDown/WriteFile actually tightened permissions.

🧪 Proposed test hardening

 	if err := os.WriteFile(path, []byte("data"), 0o644); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
+	if err := os.Chmod(path, 0o644); err != nil {
+		t.Fatalf("chmod seed: %v", err)
+	}
 	if err := secureperm.LockDown(path); err != nil {
 		t.Fatalf("LockDown: %v", err)
 	}
@@
 	if err := os.WriteFile(path, []byte("old"), 0o644); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
+	if err := os.Chmod(path, 0o644); err != nil {
+		t.Fatalf("chmod seed: %v", err)
+	}
 	if err := secureperm.WriteFile(path, []byte("new")); err != nil {
 		t.Fatalf("WriteFile: %v", err)
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if err := os.WriteFile(path, []byte("data"), 0o644); err != nil {
	t.Fatalf("seed: %v", err)
	}
	if err := secureperm.LockDown(path); err != nil {
	t.Fatalf("LockDown: %v", err)
	}
	info, err := os.Stat(path)
	if err != nil {
	t.Fatalf("Stat: %v", err)
	}
	if got := info.Mode().Perm(); got != 0o600 {
	t.Errorf("mode = %o, want 0600", got)
	}
	}
	// TestWriteFile_OverwriteDowngrades_Unix pins the behavior that
	// rewriting an existing file with lax permissions tightens them. The
	// atomic tmp+rename strategy achieves this because the renamed tmp
	// file was created with 0o600.
	func TestWriteFile_OverwriteDowngrades_Unix(t *testing.T) {
	dir := t.TempDir()
	path := filepath.Join(dir, "lax.txt")
	if err := os.WriteFile(path, []byte("old"), 0o644); err != nil {
	t.Fatalf("seed: %v", err)
	}
	if err := secureperm.WriteFile(path, []byte("new")); err != nil {
	if err := os.WriteFile(path, []byte("data"), 0o644); err != nil {
	t.Fatalf("seed: %v", err)
	}
	if err := os.Chmod(path, 0o644); err != nil {
	t.Fatalf("chmod seed: %v", err)
	}
	if err := secureperm.LockDown(path); err != nil {
	t.Fatalf("LockDown: %v", err)
	}
	info, err := os.Stat(path)
	if err != nil {
	t.Fatalf("Stat: %v", err)
	}
	if got := info.Mode().Perm(); got != 0o600 {
	t.Errorf("mode = %o, want 0600", got)
	}
	}
	// TestWriteFile_OverwriteDowngrades_Unix pins the behavior that
	// rewriting an existing file with lax permissions tightens them. The
	// atomic tmp+rename strategy achieves this because the renamed tmp
	// file was created with 0o600.
	func TestWriteFile_OverwriteDowngrades_Unix(t *testing.T) {
	dir := t.TempDir()
	path := filepath.Join(dir, "lax.txt")
	if err := os.WriteFile(path, []byte("old"), 0o644); err != nil {
	t.Fatalf("seed: %v", err)
	}
	if err := os.Chmod(path, 0o644); err != nil {
	t.Fatalf("chmod seed: %v", err)
	}
	if err := secureperm.WriteFile(path, []byte("new")); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/secureperm/secureperm_unix_test.go` around lines 48 - 75, The tests
assume os.WriteFile(path, 0o644) yields lax perms but umask can tighten them;
make the precondition independent of umask by explicitly forcing the lax mode
after seeding: after the os.WriteFile calls in
TestWriteFile_OverwriteDowngrades_Unix and the other test that seeds a 0644
file, call os.Chmod(path, 0o644) (or equivalent) and check for errors before
invoking secureperm.WriteFile or secureperm.LockDown so the tests actually
verify that those functions tighten permissions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Consider f.Sync() before Close() for durability.

The package comment justifies the atomic tmp+rename on the grounds that "secrets are not reconstructible (a corrupted secrets.yaml forces a cluster PKI reissue)", but the Unix path omits f.Sync(). On Linux/ext4 with default mount options, a crash or power loss between os.Rename and the delayed disk flush can surface the renamed inode pointing at zero-length or stale data on reboot — the canonical failure mode this pattern is meant to avoid. For full durability, also fsync the parent directory after the rename so the rename entry itself is persisted.

This is a minor issue in practice (process crashes are handled correctly by the existing cleanup), but worth closing for the non-reconstructible-secrets guarantee.

🛡️ Proposed fix

 	if _, err := f.Write(data); err != nil {
 		return fmt.Errorf("write tmp: %w", err)
 	}
+	if err := f.Sync(); err != nil {
+		return fmt.Errorf("sync tmp: %w", err)
+	}
 	if err := f.Close(); err != nil {
 		return fmt.Errorf("close tmp: %w", err)
 	}
 	if err := os.Rename(tmpPath, path); err != nil {
 		return fmt.Errorf("rename tmp -> %s: %w", path, err)
 	}
 	committed = true
+	// Best-effort fsync of the parent directory so the rename entry is durable.
+	if d, err := os.Open(dir); err == nil {
+		_ = d.Sync()
+		_ = d.Close()
+	}
 	return nil

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	func WriteFile(path string, data []byte) error {
	dir := filepath.Dir(path)
	f, err := os.CreateTemp(dir, ".secureperm-*")
	if err != nil {
	return fmt.Errorf("create tmp in %s: %w", dir, err)
	}
	tmpPath := f.Name()
	committed := false
	defer func() {
	if !committed {
	_ = f.Close()
	_ = os.Remove(tmpPath)
	}
	}()
	// os.CreateTemp already uses 0o600 but enforce explicitly so the
	// contract survives any future stdlib change.
	if err := f.Chmod(0o600); err != nil {
	return fmt.Errorf("chmod tmp: %w", err)
	}
	if _, err := f.Write(data); err != nil {
	return fmt.Errorf("write tmp: %w", err)
	}
	if err := f.Close(); err != nil {
	return fmt.Errorf("close tmp: %w", err)
	}
	if err := os.Rename(tmpPath, path); err != nil {
	return fmt.Errorf("rename tmp -> %s: %w", path, err)
	}
	committed = true
	return nil
	}
	func WriteFile(path string, data []byte) error {
	dir := filepath.Dir(path)
	f, err := os.CreateTemp(dir, ".secureperm-*")
	if err != nil {
	return fmt.Errorf("create tmp in %s: %w", dir, err)
	}
	tmpPath := f.Name()
	committed := false
	defer func() {
	if !committed {
	_ = f.Close()
	_ = os.Remove(tmpPath)
	}
	}()
	// os.CreateTemp already uses 0o600 but enforce explicitly so the
	// contract survives any future stdlib change.
	if err := f.Chmod(0o600); err != nil {
	return fmt.Errorf("chmod tmp: %w", err)
	}
	if _, err := f.Write(data); err != nil {
	return fmt.Errorf("write tmp: %w", err)
	}
	if err := f.Sync(); err != nil {
	return fmt.Errorf("sync tmp: %w", err)
	}
	if err := f.Close(); err != nil {
	return fmt.Errorf("close tmp: %w", err)
	}
	if err := os.Rename(tmpPath, path); err != nil {
	return fmt.Errorf("rename tmp -> %s: %w", path, err)
	}
	committed = true
	// Best-effort fsync of the parent directory so the rename entry is durable.
	if d, err := os.Open(dir); err == nil {
	_ = d.Sync()
	_ = d.Close()
	}
	return nil
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/secureperm/secureperm_unix.go` around lines 38 - 69, The WriteFile
function must fsync the temporary file and the containing directory to guarantee
durability: after writing to f (and before closing it) call f.Sync and
handle/return any error; after os.Rename(tmpPath, path) open the parent
directory (using dir from filepath.Dir(path)), call Sync on that directory file
descriptor and return any error, ensuring committed is only set true after both
fsyncs succeed; keep existing cleanup/defer behavior and error wrapping
consistent with the surrounding error messages.