From a58f5c446e6ce828db207f201571471458de9974 Mon Sep 17 00:00:00 2001 From: matthew-pilot Date: Fri, 29 May 2026 20:42:01 +0000 Subject: [PATCH] fix: refuse to load identity file with group/other-readable permissions (PILOT-249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promote the slog.Warn in LoadIdentity to a hard error when the identity file mode has group/other bits set (mode & 0o077 != 0). An Ed25519 private key readable by other local users is a security boundary violation — loading it silently is not acceptable. Tests updated: - TestLoadIdentity_LoosePermissionsWarn → _Error (expects error) - TestLoadIdentity_ReadFileNonNotExistError → uses 0o700 subdir - TestLoadIdentity_PermsExactly600NoWarn → _OK (no slog check) --- crypto/identity.go | 20 ++++------- crypto/zz_coverage_test.go | 71 ++++++++++++++------------------------ 2 files changed, 33 insertions(+), 58 deletions(-) diff --git a/crypto/identity.go b/crypto/identity.go index ef5bbfc..1c414dd 100644 --- a/crypto/identity.go +++ b/crypto/identity.go @@ -8,7 +8,6 @@ import ( "encoding/base64" "encoding/json" "fmt" - "log/slog" "os" "path/filepath" ) @@ -104,21 +103,16 @@ func SaveIdentity(path string, id *Identity) error { // LoadIdentity reads an identity keypair from a JSON file. // Returns nil, nil if the file does not exist (first run). // -// Emits a WARN (does NOT refuse) when the file's mode permits group -// or other access. The identity file contains the Ed25519 private -// key; SaveIdentity always writes 0o600, but an operator who created -// the file by hand or restored from a permissive backup can end up -// with 0o644 — group/other readable. The warning gives them a signal -// to chmod 600 without breaking existing deployments. A future -// release can promote the warning to a refusal once the fleet has -// had a release cycle to tighten permissions. +// Refuses to load when the file's mode permits group or other access. +// The identity file contains the Ed25519 private key; SaveIdentity +// always writes 0o600, but an operator who created the file by hand +// or restored from a permissive backup can end up with 0o644. +// Remediation: chmod 600 . func LoadIdentity(path string) (*Identity, error) { if fi, statErr := os.Stat(path); statErr == nil { if fi.Mode().Perm()&0o077 != 0 { - slog.Warn("identity file has loose permissions; chmod 600 recommended", - "path", path, - "mode", fmt.Sprintf("%o", fi.Mode().Perm()), - ) + return nil, fmt.Errorf("identity file has loose permissions (mode %o); chmod 600 %s and retry", + fi.Mode().Perm(), path) } } diff --git a/crypto/zz_coverage_test.go b/crypto/zz_coverage_test.go index b32a46e..da57396 100644 --- a/crypto/zz_coverage_test.go +++ b/crypto/zz_coverage_test.go @@ -5,7 +5,6 @@ package crypto import ( "bytes" "io" - "log/slog" "os" "path/filepath" "runtime" @@ -14,14 +13,10 @@ import ( "testing" ) -// TestLoadIdentity_LoosePermissionsWarn covers the slog.Warn branch in -// LoadIdentity that fires when the on-disk file is group/other-readable. -// -// We swap the default slog handler for one that writes to a buffer so we -// can assert the warning was emitted, then restore it. -func TestLoadIdentity_LoosePermissionsWarn(t *testing.T) { - // NOTE: not t.Parallel() — we mutate the global slog default logger - // and need to restore it cleanly. +// TestLoadIdentity_LoosePermissionsError checks that LoadIdentity +// refuses to load an identity file with group/other-readable permissions. +func TestLoadIdentity_LoosePermissionsError(t *testing.T) { + t.Parallel() if runtime.GOOS == "windows" { t.Skip("unix file permissions only") } @@ -35,46 +30,39 @@ func TestLoadIdentity_LoosePermissionsWarn(t *testing.T) { if err := SaveIdentity(path, id); err != nil { t.Fatalf("SaveIdentity: %v", err) } - // Loosen permissions so the warn branch trips. + // Loosen permissions so the error branch trips. if err := os.Chmod(path, 0o644); err != nil { t.Fatalf("Chmod: %v", err) } - var buf bytes.Buffer - origLogger := slog.Default() - slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) - t.Cleanup(func() { slog.SetDefault(origLogger) }) - - loaded, err := LoadIdentity(path) - if err != nil { - t.Fatalf("LoadIdentity: %v", err) - } - if loaded == nil { - t.Fatal("LoadIdentity returned nil") + _, err = LoadIdentity(path) + if err == nil { + t.Fatal("expected error for loose permissions (0o644)") } - if !strings.Contains(buf.String(), "loose permissions") { - t.Fatalf("expected 'loose permissions' warning, got log: %q", buf.String()) + if !strings.Contains(err.Error(), "loose permissions") { + t.Fatalf("error %q missing 'loose permissions'", err) } - if !strings.Contains(buf.String(), "level=WARN") { - t.Fatalf("expected WARN level, got log: %q", buf.String()) + if !strings.Contains(err.Error(), "chmod 600") { + t.Fatalf("error %q missing 'chmod 600' instruction", err) } } // TestLoadIdentity_ReadFileNonNotExistError covers the os.ReadFile -// error branch where the error is *not* IsNotExist. Pointing the path -// at a directory triggers EISDIR from read(2), which os.IsNotExist -// returns false for, so the wrapped "read identity" path is exercised. +// error branch where the error is *not* IsNotExist. We create a +// directory with strict 0o700 perms (passes the permissions gate), +// then os.ReadFile on the directory fails with EISDIR. func TestLoadIdentity_ReadFileNonNotExistError(t *testing.T) { t.Parallel() if runtime.GOOS == "windows" { t.Skip("EISDIR is unix-specific") } dir := t.TempDir() - // `dir` itself is a directory; os.Stat succeeds, os.ReadFile fails - // with a non-NotExist error. The mode bits on a directory include - // the executable bit for group/other (0o755 typical for TempDir), - // which is harmless for this test — slog default discards. - _, err := LoadIdentity(dir) + // Create a subdirectory with strict perms that passes the gate. + subdir := filepath.Join(dir, "subdir") + if err := os.MkdirAll(subdir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + _, err := LoadIdentity(subdir) if err == nil { t.Fatal("expected read error against a directory path") } @@ -83,9 +71,10 @@ func TestLoadIdentity_ReadFileNonNotExistError(t *testing.T) { } } -// TestLoadIdentity_PermsExactly600NoWarn confirms the *negative* case: -// a strict 0o600 file does NOT trip the warn branch. -func TestLoadIdentity_PermsExactly600NoWarn(t *testing.T) { +// TestLoadIdentity_PermsExactly600OK confirms that a strict 0o600 file +// loads successfully (passes the permissions gate). +func TestLoadIdentity_PermsExactly600OK(t *testing.T) { + t.Parallel() if runtime.GOOS == "windows" { t.Skip("unix file permissions only") } @@ -100,16 +89,8 @@ func TestLoadIdentity_PermsExactly600NoWarn(t *testing.T) { t.Fatalf("Chmod: %v", err) } - var buf bytes.Buffer - origLogger := slog.Default() - slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) - t.Cleanup(func() { slog.SetDefault(origLogger) }) - if _, err := LoadIdentity(path); err != nil { - t.Fatalf("LoadIdentity: %v", err) - } - if strings.Contains(buf.String(), "loose permissions") { - t.Fatalf("did not expect warn for 0o600, got log: %q", buf.String()) + t.Fatalf("LoadIdentity with 0o600 should succeed: %v", err) } }