mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-07-09 16:00:59 +00:00
Harden secure local key file handling
This commit is contained in:
parent
ccb2edc3b8
commit
70b91759d2
10 changed files with 341 additions and 146 deletions
|
|
@ -4240,6 +4240,8 @@
|
|||
"internal/api/security.go",
|
||||
"internal/api/security_tokens.go",
|
||||
"internal/api/system_settings.go",
|
||||
"internal/cloudcp/auth/magiclink.go",
|
||||
"internal/cloudcp/auth/magiclink_store.go",
|
||||
"internal/crypto/crypto.go",
|
||||
"internal/securityutil/secure_storage_dir.go",
|
||||
"internal/telemetry/telemetry.go",
|
||||
|
|
@ -4261,6 +4263,7 @@
|
|||
"internal/api/security_tokens_owner_binding_test.go",
|
||||
"internal/api/security_tokens_test.go",
|
||||
"internal/api/system_settings_telemetry_test.go",
|
||||
"internal/cloudcp/auth/magiclink_test.go",
|
||||
"internal/telemetry/telemetry_test.go",
|
||||
"scripts/tests/test_telemetry_adoption_report.py"
|
||||
],
|
||||
|
|
@ -4350,12 +4353,15 @@
|
|||
"label": "storage directory hardening proof",
|
||||
"match_prefixes": [],
|
||||
"match_files": [
|
||||
"internal/cloudcp/auth/magiclink.go",
|
||||
"internal/cloudcp/auth/magiclink_store.go",
|
||||
"internal/crypto/crypto.go",
|
||||
"internal/securityutil/secure_storage_dir.go"
|
||||
],
|
||||
"allow_same_subsystem_tests": false,
|
||||
"test_prefixes": [],
|
||||
"exact_files": [
|
||||
"internal/cloudcp/auth/magiclink_test.go",
|
||||
"internal/crypto/crypto_test.go",
|
||||
"internal/securityutil/secure_storage_dir_test.go"
|
||||
]
|
||||
|
|
|
|||
|
|
@ -51,7 +51,9 @@ visibility, and privacy controls to operators.
|
|||
25. `internal/api/router_routes_auth_security.go`
|
||||
26. `internal/crypto/crypto.go`
|
||||
27. `internal/securityutil/secure_storage_dir.go`
|
||||
28. `scripts/telemetry_adoption_report.py`
|
||||
28. `internal/cloudcp/auth/magiclink.go`
|
||||
29. `internal/cloudcp/auth/magiclink_store.go`
|
||||
30. `scripts/telemetry_adoption_report.py`
|
||||
|
||||
## Shared Boundaries
|
||||
|
||||
|
|
@ -75,7 +77,7 @@ visibility, and privacy controls to operators.
|
|||
4. Change security/auth/token transport behavior through the shared `frontend-modern/src/api/security.ts`, `frontend-modern/src/components/Settings/APITokenManager.tsx`, `frontend-modern/src/components/Settings/apiTokenManagerModel.ts`, `frontend-modern/src/components/Settings/useAPITokenManagerState.ts`, `internal/api/security.go`, `internal/api/security_tokens.go`, and `internal/api/system_settings.go` boundary.
|
||||
5. Change security/privacy settings presentation through the shared `frontend-modern/src/components/Settings/GeneralSettingsPanel.tsx`, `frontend-modern/src/components/Settings/SecurityAuthPanel.tsx`, `frontend-modern/src/components/Settings/SecurityOverviewPanel.tsx`, `frontend-modern/src/components/Settings/QuickSecuritySetup.tsx`, `frontend-modern/src/components/Settings/SecurityPostureSummary.tsx`, `frontend-modern/src/components/Settings/SSOProviderTypeIcon.tsx`, `frontend-modern/src/utils/securityAuthPresentation.ts`, `frontend-modern/src/utils/securityScorePresentation.ts`, `frontend-modern/src/utils/auditLogPresentation.ts`, and `frontend-modern/src/utils/auditWebhookPresentation.ts` boundary.
|
||||
6. Change operator-facing telemetry/adoption reporting through `scripts/telemetry_adoption_report.py` together with the privacy disclosure whenever release-identity interpretation changes.
|
||||
7. Change data-at-rest encryption-key storage-root hardening semantics through `internal/crypto/crypto.go` and `internal/securityutil/secure_storage_dir.go` together so writable-but-not-owned runtime storage mounts stay supported without weakening file-level secrecy.
|
||||
7. Change data-at-rest encryption-key or control-plane magic-link HMAC key and storage-root hardening semantics through `internal/crypto/crypto.go`, `internal/cloudcp/auth/magiclink.go`, `internal/cloudcp/auth/magiclink_store.go`, and `internal/securityutil/secure_storage_dir.go` together so writable-but-not-owned runtime storage mounts stay supported without weakening file-level secrecy.
|
||||
|
||||
## Forbidden Paths
|
||||
|
||||
|
|
@ -90,7 +92,7 @@ visibility, and privacy controls to operators.
|
|||
3. Keep shared frontend settings proof routing aligned whenever security/privacy presentation changes.
|
||||
4. Keep the checked-in telemetry adoption report aligned with the same release-identity rules used by the runtime telemetry payload.
|
||||
5. Update this contract whenever a new canonical security, token, auth, or privacy surface becomes part of the governed trust boundary.
|
||||
6. Keep the shared storage-directory hardening helper and the crypto manager aligned whenever runtime data-root ownership assumptions change.
|
||||
6. Keep the shared storage-directory and secure storage-file hardening helper aligned with the crypto manager plus control-plane magic-link key and store handling whenever runtime data-root ownership assumptions change.
|
||||
|
||||
## Current State
|
||||
|
||||
|
|
@ -105,6 +107,14 @@ adoption baselines. `scripts/telemetry_adoption_report.py` must emit
|
|||
windowed 24h, 72h, and 7d latest-install snapshots that split published
|
||||
versions from unpublished or development builds, so RC adoption reads stop
|
||||
depending on ad hoc SQL or one-off local helper scripts.
|
||||
That same storage hardening boundary now also owns secure regular-file
|
||||
handling for secret-bearing local trust material and the control-plane
|
||||
magic-link storage root. `internal/crypto/crypto.go`,
|
||||
`internal/cloudcp/auth/magiclink.go`, and
|
||||
`internal/cloudcp/auth/magiclink_store.go` must route encryption keys,
|
||||
magic-link HMAC keys, and the magic-link SQLite store path through the shared
|
||||
secure storage helpers so symlink, oversize, and non-regular file paths fail
|
||||
closed instead of slipping past directory-only hardening.
|
||||
|
||||
Security-facing settings remain intentionally shared with `frontend-primitives`
|
||||
because shell framing and presentation consistency still belong there, but the
|
||||
|
|
|
|||
|
|
@ -5,22 +5,24 @@ import (
|
|||
"crypto/rand"
|
||||
"crypto/sha256"
|
||||
"encoding/base64"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/url"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/rcourtman/pulse-go-rewrite/internal/securityutil"
|
||||
"github.com/rs/zerolog/log"
|
||||
)
|
||||
|
||||
const (
|
||||
magicLinkTTL = 15 * time.Minute
|
||||
magicLinkPrefix = "ml1_"
|
||||
hmacKeyFile = ".cp_magic_link_key"
|
||||
hmacKeySize = 32
|
||||
magicLinkTTL = 15 * time.Minute
|
||||
magicLinkPrefix = "ml1_"
|
||||
hmacKeyFile = ".cp_magic_link_key"
|
||||
hmacKeySize = 32
|
||||
maxHMACKeyFileSize = 64
|
||||
)
|
||||
|
||||
type MagicLinkTarget string
|
||||
|
|
@ -50,16 +52,25 @@ type Service struct {
|
|||
// NewService creates a Service backed by a SQLite store in cpDataDir.
|
||||
// It loads (or generates) an HMAC key from {cpDataDir}/.cp_magic_link_key.
|
||||
func NewService(cpDataDir string) (*Service, error) {
|
||||
if err := ensureOwnerOnlyDir(cpDataDir); err != nil {
|
||||
normalizedDir, err := securityutil.NormalizeStorageDir(cpDataDir)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("cp data dir is required: %w", err)
|
||||
}
|
||||
if err := ensureOwnerOnlyDir(normalizedDir); err != nil {
|
||||
return nil, fmt.Errorf("ensure cp data dir: %w", err)
|
||||
}
|
||||
|
||||
key, err := loadOrGenerateKey(filepath.Join(cpDataDir, hmacKeyFile))
|
||||
keyPath, err := securityutil.JoinStorageLeaf(normalizedDir, hmacKeyFile)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("resolve magic link hmac key path: %w", err)
|
||||
}
|
||||
|
||||
key, err := loadOrGenerateKey(keyPath)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("magic link hmac key: %w", err)
|
||||
}
|
||||
|
||||
store, err := NewStore(cpDataDir)
|
||||
store, err := NewStore(normalizedDir)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("magic link store: %w", err)
|
||||
}
|
||||
|
|
@ -178,11 +189,17 @@ func (s *Service) Close() {
|
|||
}
|
||||
|
||||
func loadOrGenerateKey(path string) ([]byte, error) {
|
||||
data, err := os.ReadFile(path)
|
||||
if err == nil && len(data) >= hmacKeySize {
|
||||
return data[:hmacKeySize], nil
|
||||
data, err := securityutil.ReadSecureStorageFile(path, maxHMACKeyFileSize)
|
||||
if err == nil {
|
||||
if len(data) != hmacKeySize {
|
||||
return nil, fmt.Errorf("invalid key file %s: expected %d bytes, got %d", path, hmacKeySize, len(data))
|
||||
}
|
||||
return append([]byte(nil), data...), nil
|
||||
}
|
||||
if err != nil && !os.IsNotExist(err) {
|
||||
if errors.Is(err, securityutil.ErrUnsafeStorageFile) {
|
||||
return nil, fmt.Errorf("unsafe key file %s: %w", path, err)
|
||||
}
|
||||
return nil, fmt.Errorf("read key file %s: %w", path, err)
|
||||
}
|
||||
|
||||
|
|
@ -190,7 +207,7 @@ func loadOrGenerateKey(path string) ([]byte, error) {
|
|||
if _, err := io.ReadFull(rand.Reader, key); err != nil {
|
||||
return nil, fmt.Errorf("generate key: %w", err)
|
||||
}
|
||||
if err := os.WriteFile(path, key, 0o600); err != nil {
|
||||
if err := securityutil.WriteSecureStorageFile(path, key, privateDirPerm, 0o600); err != nil {
|
||||
return nil, fmt.Errorf("write key file %s: %w", path, err)
|
||||
}
|
||||
log.Info().Str("path", path).Msg("Generated new magic link HMAC key")
|
||||
|
|
|
|||
|
|
@ -6,12 +6,11 @@ import (
|
|||
"errors"
|
||||
"fmt"
|
||||
"net/url"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/rcourtman/pulse-go-rewrite/internal/securityutil"
|
||||
"github.com/rs/zerolog/log"
|
||||
_ "modernc.org/sqlite"
|
||||
)
|
||||
|
|
@ -26,10 +25,7 @@ const storeCleanupInterval = 5 * time.Minute
|
|||
const privateDirPerm = 0o700
|
||||
|
||||
func ensureOwnerOnlyDir(dir string) error {
|
||||
if err := os.MkdirAll(dir, privateDirPerm); err != nil {
|
||||
return err
|
||||
}
|
||||
return os.Chmod(dir, privateDirPerm)
|
||||
return securityutil.EnsureSecureStorageDir(dir, privateDirPerm)
|
||||
}
|
||||
|
||||
// TokenRecord holds the data associated with a stored magic link token.
|
||||
|
|
@ -51,15 +47,18 @@ type Store struct {
|
|||
|
||||
// NewStore opens (or creates) the magic link token database in dir.
|
||||
func NewStore(dir string) (*Store, error) {
|
||||
dir = filepath.Clean(dir)
|
||||
if strings.TrimSpace(dir) == "" {
|
||||
return nil, fmt.Errorf("dir is required")
|
||||
normalizedDir, err := securityutil.NormalizeStorageDir(dir)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("dir is required: %w", err)
|
||||
}
|
||||
if err := ensureOwnerOnlyDir(dir); err != nil {
|
||||
if err := ensureOwnerOnlyDir(normalizedDir); err != nil {
|
||||
return nil, fmt.Errorf("create magic link store dir: %w", err)
|
||||
}
|
||||
|
||||
dbPath := filepath.Join(dir, "cp_magic_links.db")
|
||||
dbPath, err := securityutil.JoinStorageLeaf(normalizedDir, "cp_magic_links.db")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("resolve magic link db path: %w", err)
|
||||
}
|
||||
dsn := dbPath + "?" + url.Values{
|
||||
"_pragma": []string{
|
||||
"busy_timeout(30000)",
|
||||
|
|
|
|||
|
|
@ -225,6 +225,54 @@ func TestKeyPersistence(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestKeyPersistence_SecuresKeyFilePermissions(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
svc, err := NewService(dir)
|
||||
if err != nil {
|
||||
t.Fatalf("NewService: %v", err)
|
||||
}
|
||||
svc.Close()
|
||||
|
||||
info, err := os.Stat(filepath.Join(dir, hmacKeyFile))
|
||||
if err != nil {
|
||||
t.Fatalf("stat key file: %v", err)
|
||||
}
|
||||
if got := info.Mode().Perm(); got != 0o600 {
|
||||
t.Fatalf("key file perms = %o, want %o", got, 0o600)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewService_RejectsShortExistingKeyFile(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
if err := os.WriteFile(filepath.Join(dir, hmacKeyFile), make([]byte, hmacKeySize-1), 0o600); err != nil {
|
||||
t.Fatalf("write short key file: %v", err)
|
||||
}
|
||||
|
||||
_, err := NewService(dir)
|
||||
if err == nil || !strings.Contains(err.Error(), "expected 32 bytes") {
|
||||
t.Fatalf("expected invalid key length error, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewService_RejectsSymlinkedKeyFile(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
realKeyPath := filepath.Join(dir, "real.key")
|
||||
if err := os.WriteFile(realKeyPath, make([]byte, hmacKeySize), 0o600); err != nil {
|
||||
t.Fatalf("write real key: %v", err)
|
||||
}
|
||||
|
||||
keyPath := filepath.Join(dir, hmacKeyFile)
|
||||
if err := os.Symlink(realKeyPath, keyPath); err != nil {
|
||||
t.Skipf("symlink not supported on this platform: %v", err)
|
||||
}
|
||||
|
||||
_, err := NewService(dir)
|
||||
if err == nil || !strings.Contains(err.Error(), "unsafe key file") {
|
||||
t.Fatalf("expected unsafe key file error, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewService_SecuresDataDirPermissions(t *testing.T) {
|
||||
dir := filepath.Join(t.TempDir(), "cp-data")
|
||||
|
||||
|
|
|
|||
|
|
@ -59,35 +59,14 @@ func decodeEncryptionKey(data []byte) ([]byte, error) {
|
|||
return decoded[:n], nil
|
||||
}
|
||||
|
||||
func validateEncryptionKeyFile(path string, info os.FileInfo) error {
|
||||
if info.Mode()&os.ModeSymlink != 0 {
|
||||
return fmt.Errorf("%w: refusing symlink key path %q", errUnsafeKeyPath, path)
|
||||
}
|
||||
if !info.Mode().IsRegular() {
|
||||
return fmt.Errorf("%w: non-regular key path %q", errInvalidKeyMaterial, path)
|
||||
}
|
||||
if info.Size() > maxEncryptionKeyFileSize {
|
||||
return fmt.Errorf("%w: key file %q is too large (%d bytes)", errUnsafeKeyPath, path, info.Size())
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func loadKeyFromFile(path string) ([]byte, error) {
|
||||
info, err := os.Lstat(path)
|
||||
data, err := securityutil.ReadSecureStorageFile(path, maxEncryptionKeyFileSize)
|
||||
if err != nil {
|
||||
if errors.Is(err, securityutil.ErrUnsafeStorageFile) {
|
||||
return nil, fmt.Errorf("%w: %v", errUnsafeKeyPath, err)
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
if err := validateEncryptionKeyFile(path, info); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
data, err := os.ReadFile(path)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if len(data) > maxEncryptionKeyFileSize {
|
||||
return nil, fmt.Errorf("%w: key file %q exceeded size limit while reading", errUnsafeKeyPath, path)
|
||||
}
|
||||
return decodeEncryptionKey(data)
|
||||
}
|
||||
|
||||
|
|
@ -100,43 +79,8 @@ func writeKeyFile(path string, key []byte) error {
|
|||
return fmt.Errorf("refusing to write invalid key length %d", len(key))
|
||||
}
|
||||
|
||||
dir := filepath.Dir(path)
|
||||
if err := ensureOwnerOnlyDir(dir); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
tmpFile, err := os.CreateTemp(dir, ".encryption.key.*.tmp")
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
tmpPath := tmpFile.Name()
|
||||
cleanup := true
|
||||
defer func() {
|
||||
if cleanup {
|
||||
_ = os.Remove(tmpPath)
|
||||
}
|
||||
}()
|
||||
|
||||
if err := tmpFile.Chmod(encryptionKeyFilePerm); err != nil {
|
||||
_ = tmpFile.Close()
|
||||
return err
|
||||
}
|
||||
|
||||
encoded := base64.StdEncoding.EncodeToString(key)
|
||||
if _, err := tmpFile.WriteString(encoded); err != nil {
|
||||
_ = tmpFile.Close()
|
||||
return err
|
||||
}
|
||||
if err := tmpFile.Close(); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if err := os.Rename(tmpPath, path); err != nil {
|
||||
return err
|
||||
}
|
||||
cleanup = false
|
||||
|
||||
return os.Chmod(path, encryptionKeyFilePerm)
|
||||
return securityutil.WriteSecureStorageFile(path, []byte(encoded), encryptionKeyDirPerm, encryptionKeyFilePerm)
|
||||
}
|
||||
|
||||
// CryptoManager handles encryption/decryption of sensitive data
|
||||
|
|
@ -279,61 +223,48 @@ func getOrCreateKeyAt(dataDir string) ([]byte, error) {
|
|||
Str("oldKeyPath", oldKeyPath).
|
||||
Msg("checking for legacy encryption key migration")
|
||||
|
||||
if data, err := os.ReadFile(oldKeyPath); err == nil {
|
||||
decoded := make([]byte, base64.StdEncoding.DecodedLen(len(data)))
|
||||
n, decodeErr := base64.StdEncoding.Decode(decoded, data)
|
||||
if decodeErr != nil {
|
||||
if key, err := loadKeyFromFile(oldKeyPath); err == nil {
|
||||
// Migrate key to new location
|
||||
if err := writeKeyFile(keyPath, key); err != nil {
|
||||
// Migration failed, but we can still use the old key
|
||||
log.Warn().
|
||||
Err(decodeErr).
|
||||
Str("path", oldKeyPath).
|
||||
Msg("Failed to decode legacy encryption key during migration check")
|
||||
} else if n != 32 {
|
||||
log.Warn().
|
||||
Int("decodedBytes", n).
|
||||
Str("path", oldKeyPath).
|
||||
Msg("Legacy encryption key has invalid length during migration check")
|
||||
} else {
|
||||
key := decoded[:n]
|
||||
// Migrate key to new location
|
||||
if err := writeKeyFile(keyPath, key); err != nil {
|
||||
// Migration failed, but we can still use the old key
|
||||
log.Warn().
|
||||
Err(err).
|
||||
Str("from", oldKeyPath).
|
||||
Str("to", keyPath).
|
||||
Msg("Failed to migrate encryption key, using old location")
|
||||
return key, nil
|
||||
}
|
||||
log.Info().
|
||||
Err(err).
|
||||
Str("from", oldKeyPath).
|
||||
Str("to", keyPath).
|
||||
Msg("migrated encryption key to data directory")
|
||||
|
||||
// CRITICAL: This is the ONLY place in the codebase that deletes the encryption key!
|
||||
// BUG FIX: Disabling key deletion to prevent key loss.
|
||||
// Keeping both copies is safe - the old key at /etc/pulse will just be unused.
|
||||
log.Info().
|
||||
Str("oldKeyPath", oldKeyPath).
|
||||
Str("newKeyPath", keyPath).
|
||||
Str("dataDir", resolvedDataDir).
|
||||
Msg("Key migration complete - PRESERVING old key at original location for safety")
|
||||
|
||||
// DISABLED: Key deletion was causing mysterious key loss bugs.
|
||||
// The old key is now preserved. This is safe because:
|
||||
// 1. We just successfully wrote the key to the new location
|
||||
// 2. Future reads will use the new location (checked first)
|
||||
// 3. Keeping the backup prevents data loss if something goes wrong
|
||||
//
|
||||
// if err := os.Remove(oldKeyPath); err != nil {
|
||||
// log.Debug().Err(err).Msg("Could not remove old encryption key (may lack permissions)")
|
||||
// } else {
|
||||
// log.Error().
|
||||
// Str("deletedPath", oldKeyPath).
|
||||
// Msg("CRITICAL: ENCRYPTION KEY HAS BEEN DELETED")
|
||||
// }
|
||||
Msg("Failed to migrate encryption key, using old location")
|
||||
return key, nil
|
||||
}
|
||||
} else if !os.IsNotExist(err) {
|
||||
log.Info().
|
||||
Str("from", oldKeyPath).
|
||||
Str("to", keyPath).
|
||||
Msg("migrated encryption key to data directory")
|
||||
|
||||
// CRITICAL: This is the ONLY place in the codebase that deletes the encryption key!
|
||||
// BUG FIX: Disabling key deletion to prevent key loss.
|
||||
// Keeping both copies is safe - the old key at /etc/pulse will just be unused.
|
||||
log.Info().
|
||||
Str("oldKeyPath", oldKeyPath).
|
||||
Str("newKeyPath", keyPath).
|
||||
Str("dataDir", resolvedDataDir).
|
||||
Msg("Key migration complete - PRESERVING old key at original location for safety")
|
||||
|
||||
// DISABLED: Key deletion was causing mysterious key loss bugs.
|
||||
// The old key is now preserved. This is safe because:
|
||||
// 1. We just successfully wrote the key to the new location
|
||||
// 2. Future reads will use the new location (checked first)
|
||||
// 3. Keeping the backup prevents data loss if something goes wrong
|
||||
//
|
||||
// if err := os.Remove(oldKeyPath); err != nil {
|
||||
// log.Debug().Err(err).Msg("Could not remove old encryption key (may lack permissions)")
|
||||
// } else {
|
||||
// log.Error().
|
||||
// Str("deletedPath", oldKeyPath).
|
||||
// Msg("CRITICAL: ENCRYPTION KEY HAS BEEN DELETED")
|
||||
// }
|
||||
return key, nil
|
||||
} else if errors.Is(err, errUnsafeKeyPath) {
|
||||
return nil, fmt.Errorf("unsafe legacy encryption key path %q: %w", oldKeyPath, err)
|
||||
} else if !os.IsNotExist(err) && !errors.Is(err, errInvalidKeyMaterial) {
|
||||
log.Warn().
|
||||
Err(err).
|
||||
Str("path", oldKeyPath).
|
||||
|
|
|
|||
|
|
@ -586,6 +586,46 @@ func TestGetOrCreateKeyAt_MigrateSuccess(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateKeyAt_RejectsSymlinkedLegacyKeyPath(t *testing.T) {
|
||||
legacyDir := t.TempDir()
|
||||
legacyPath := filepath.Join(legacyDir, ".encryption.key")
|
||||
withLegacyKeyPath(t, legacyPath)
|
||||
|
||||
realKeyPath := filepath.Join(legacyDir, "real-encryption.key")
|
||||
oldKey := make([]byte, 32)
|
||||
for i := range oldKey {
|
||||
oldKey[i] = byte(i)
|
||||
}
|
||||
encoded := base64.StdEncoding.EncodeToString(oldKey)
|
||||
if err := os.WriteFile(realKeyPath, []byte(encoded), 0o600); err != nil {
|
||||
t.Fatalf("write real key: %v", err)
|
||||
}
|
||||
if err := os.Symlink(realKeyPath, legacyPath); err != nil {
|
||||
t.Skipf("symlink not supported on this platform: %v", err)
|
||||
}
|
||||
|
||||
_, err := getOrCreateKeyAt(t.TempDir())
|
||||
if err == nil || !strings.Contains(err.Error(), "unsafe legacy encryption key path") {
|
||||
t.Fatalf("expected unsafe legacy key path error, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateKeyAt_RejectsOversizedLegacyKeyFile(t *testing.T) {
|
||||
legacyDir := t.TempDir()
|
||||
legacyPath := filepath.Join(legacyDir, ".encryption.key")
|
||||
withLegacyKeyPath(t, legacyPath)
|
||||
|
||||
oversized := bytes.Repeat([]byte("A"), maxEncryptionKeyFileSize+1)
|
||||
if err := os.WriteFile(legacyPath, oversized, 0o600); err != nil {
|
||||
t.Fatalf("write oversized legacy key: %v", err)
|
||||
}
|
||||
|
||||
_, err := getOrCreateKeyAt(t.TempDir())
|
||||
if err == nil || !strings.Contains(err.Error(), "unsafe legacy encryption key path") {
|
||||
t.Fatalf("expected unsafe legacy key path error, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateKeyAt_IgnoresRelativeLegacyOverride(t *testing.T) {
|
||||
legacyDir := t.TempDir()
|
||||
legacyPath := filepath.Join(legacyDir, ".encryption.key")
|
||||
|
|
@ -647,7 +687,7 @@ func TestGetOrCreateKeyAt_MigrateMkdirError(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateKeyAt_MigrateWriteError(t *testing.T) {
|
||||
func TestGetOrCreateKeyAt_RejectsUnsafePrimaryKeyPathBeforeMigration(t *testing.T) {
|
||||
legacyDir := t.TempDir()
|
||||
legacyPath := filepath.Join(legacyDir, ".encryption.key")
|
||||
withLegacyKeyPath(t, legacyPath)
|
||||
|
|
@ -667,12 +707,9 @@ func TestGetOrCreateKeyAt_MigrateWriteError(t *testing.T) {
|
|||
t.Fatalf("Failed to create key path dir: %v", err)
|
||||
}
|
||||
|
||||
key, err := getOrCreateKeyAt(newDir)
|
||||
if err != nil {
|
||||
t.Fatalf("getOrCreateKeyAt() error: %v", err)
|
||||
}
|
||||
if !bytes.Equal(key, oldKey) {
|
||||
t.Fatalf("expected legacy key on write error")
|
||||
_, err := getOrCreateKeyAt(newDir)
|
||||
if err == nil || !strings.Contains(err.Error(), "unsafe encryption key path") {
|
||||
t.Fatalf("expected unsafe encryption key path error, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import (
|
|||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"syscall"
|
||||
)
|
||||
|
||||
|
|
@ -11,8 +12,16 @@ var (
|
|||
secureStorageDirMkdirAllFn = os.MkdirAll
|
||||
secureStorageDirChmodFn = os.Chmod
|
||||
secureStorageDirLstatFn = os.Lstat
|
||||
secureStorageFileLstatFn = os.Lstat
|
||||
secureStorageFileReadFn = os.ReadFile
|
||||
secureStorageFileTempFn = os.CreateTemp
|
||||
secureStorageFileRenameFn = os.Rename
|
||||
secureStorageFileRemoveFn = os.Remove
|
||||
secureStorageFileChmodFn = os.Chmod
|
||||
)
|
||||
|
||||
var ErrUnsafeStorageFile = errors.New("unsafe storage file")
|
||||
|
||||
// EnsureSecureStorageDir creates or hardens a storage directory to the desired
|
||||
// permissions when possible. For pre-mounted runtime storage roots such as
|
||||
// Kubernetes volume mounts, the running process may be able to write inside the
|
||||
|
|
@ -50,3 +59,75 @@ func isStorageDirPermissionError(err error) bool {
|
|||
errors.Is(err, syscall.EPERM) ||
|
||||
errors.Is(err, syscall.EACCES)
|
||||
}
|
||||
|
||||
// ReadSecureStorageFile reads a regular, non-symlink storage file while
|
||||
// enforcing a caller-provided size ceiling both before and after the read.
|
||||
func ReadSecureStorageFile(path string, maxSize int64) ([]byte, error) {
|
||||
if maxSize <= 0 {
|
||||
return nil, fmt.Errorf("max storage file size must be positive")
|
||||
}
|
||||
|
||||
info, err := secureStorageFileLstatFn(path)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if info.Mode()&os.ModeSymlink != 0 {
|
||||
return nil, fmt.Errorf("%w: refusing symlink file path %q", ErrUnsafeStorageFile, path)
|
||||
}
|
||||
if !info.Mode().IsRegular() {
|
||||
return nil, fmt.Errorf("%w: non-regular file path %q", ErrUnsafeStorageFile, path)
|
||||
}
|
||||
if info.Size() > maxSize {
|
||||
return nil, fmt.Errorf("%w: file %q is too large (%d bytes)", ErrUnsafeStorageFile, path, info.Size())
|
||||
}
|
||||
|
||||
data, err := secureStorageFileReadFn(path)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if int64(len(data)) > maxSize {
|
||||
return nil, fmt.Errorf("%w: file %q exceeded size limit while reading", ErrUnsafeStorageFile, path)
|
||||
}
|
||||
|
||||
return data, nil
|
||||
}
|
||||
|
||||
// WriteSecureStorageFile writes a file via a temp file + rename inside an
|
||||
// already validated storage directory so file creation stays owner-only and
|
||||
// does not follow pre-existing symlinks at the destination path.
|
||||
func WriteSecureStorageFile(path string, data []byte, dirPerm, filePerm os.FileMode) error {
|
||||
dir := filepath.Dir(path)
|
||||
if err := EnsureSecureStorageDir(dir, dirPerm); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
tmpFile, err := secureStorageFileTempFn(dir, "."+filepath.Base(path)+".*.tmp")
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
tmpPath := tmpFile.Name()
|
||||
cleanup := true
|
||||
defer func() {
|
||||
if cleanup {
|
||||
_ = secureStorageFileRemoveFn(tmpPath)
|
||||
}
|
||||
}()
|
||||
|
||||
if err := tmpFile.Chmod(filePerm); err != nil {
|
||||
_ = tmpFile.Close()
|
||||
return err
|
||||
}
|
||||
if _, err := tmpFile.Write(data); err != nil {
|
||||
_ = tmpFile.Close()
|
||||
return err
|
||||
}
|
||||
if err := tmpFile.Close(); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := secureStorageFileRenameFn(tmpPath, path); err != nil {
|
||||
return err
|
||||
}
|
||||
cleanup = false
|
||||
|
||||
return secureStorageFileChmodFn(path, filePerm)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,17 +1,20 @@
|
|||
package securityutil
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
type stubFileInfo struct {
|
||||
mode os.FileMode
|
||||
size int64
|
||||
}
|
||||
|
||||
func (s stubFileInfo) Name() string { return "stub" }
|
||||
func (s stubFileInfo) Size() int64 { return 0 }
|
||||
func (s stubFileInfo) Size() int64 { return s.size }
|
||||
func (s stubFileInfo) Mode() os.FileMode { return s.mode }
|
||||
func (s stubFileInfo) ModTime() time.Time { return time.Time{} }
|
||||
func (s stubFileInfo) IsDir() bool { return s.mode.IsDir() }
|
||||
|
|
@ -21,6 +24,12 @@ func resetSecureStorageDirFns() {
|
|||
secureStorageDirMkdirAllFn = os.MkdirAll
|
||||
secureStorageDirChmodFn = os.Chmod
|
||||
secureStorageDirLstatFn = os.Lstat
|
||||
secureStorageFileLstatFn = os.Lstat
|
||||
secureStorageFileReadFn = os.ReadFile
|
||||
secureStorageFileTempFn = os.CreateTemp
|
||||
secureStorageFileRenameFn = os.Rename
|
||||
secureStorageFileRemoveFn = os.Remove
|
||||
secureStorageFileChmodFn = os.Chmod
|
||||
}
|
||||
|
||||
func TestEnsureSecureStorageDir_CreatesOwnerOnlyDirectory(t *testing.T) {
|
||||
|
|
@ -94,3 +103,57 @@ func TestEnsureSecureStorageDir_RejectsNonDirectoryPaths(t *testing.T) {
|
|||
t.Fatal("expected non-directory path error")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadSecureStorageFile_RejectsSymlinkPaths(t *testing.T) {
|
||||
t.Cleanup(resetSecureStorageDirFns)
|
||||
|
||||
secureStorageFileLstatFn = func(string) (os.FileInfo, error) {
|
||||
return stubFileInfo{mode: os.ModeSymlink}, nil
|
||||
}
|
||||
|
||||
_, err := ReadSecureStorageFile("/data/key", 32)
|
||||
if !errors.Is(err, ErrUnsafeStorageFile) {
|
||||
t.Fatalf("expected ErrUnsafeStorageFile, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadSecureStorageFile_RejectsOversizedFiles(t *testing.T) {
|
||||
t.Cleanup(resetSecureStorageDirFns)
|
||||
|
||||
secureStorageFileLstatFn = func(string) (os.FileInfo, error) {
|
||||
return stubFileInfo{mode: 0o600, size: 33}, nil
|
||||
}
|
||||
|
||||
_, err := ReadSecureStorageFile("/data/key", 32)
|
||||
if !errors.Is(err, ErrUnsafeStorageFile) {
|
||||
t.Fatalf("expected ErrUnsafeStorageFile, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWriteSecureStorageFile_CreatesOwnerOnlyFile(t *testing.T) {
|
||||
t.Cleanup(resetSecureStorageDirFns)
|
||||
|
||||
root := t.TempDir()
|
||||
target := filepath.Join(root, "secure", "key.bin")
|
||||
want := []byte("secret")
|
||||
|
||||
if err := WriteSecureStorageFile(target, want, 0o700, 0o600); err != nil {
|
||||
t.Fatalf("WriteSecureStorageFile() error: %v", err)
|
||||
}
|
||||
|
||||
data, err := os.ReadFile(target)
|
||||
if err != nil {
|
||||
t.Fatalf("read target: %v", err)
|
||||
}
|
||||
if string(data) != string(want) {
|
||||
t.Fatalf("contents = %q, want %q", data, want)
|
||||
}
|
||||
|
||||
info, err := os.Stat(target)
|
||||
if err != nil {
|
||||
t.Fatalf("stat target: %v", err)
|
||||
}
|
||||
if got := info.Mode().Perm(); got != 0o600 {
|
||||
t.Fatalf("permissions = %o, want 600", got)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4428,9 +4428,11 @@ class SubsystemLookupTest(unittest.TestCase):
|
|||
["scripts/release_control/subsystem_lookup_test.py"],
|
||||
)
|
||||
|
||||
def test_lookup_paths_assigns_crypto_storage_hardening_to_security_privacy(self) -> None:
|
||||
def test_lookup_paths_assigns_secure_storage_hardening_to_security_privacy(self) -> None:
|
||||
result = lookup_paths(
|
||||
[
|
||||
"internal/cloudcp/auth/magiclink.go",
|
||||
"internal/cloudcp/auth/magiclink_store.go",
|
||||
"internal/crypto/crypto.go",
|
||||
"internal/securityutil/secure_storage_dir.go",
|
||||
]
|
||||
|
|
@ -4459,6 +4461,7 @@ class SubsystemLookupTest(unittest.TestCase):
|
|||
self.assertEqual(
|
||||
match["verification_requirement"]["exact_files"],
|
||||
[
|
||||
"internal/cloudcp/auth/magiclink_test.go",
|
||||
"internal/crypto/crypto_test.go",
|
||||
"internal/securityutil/secure_storage_dir_test.go",
|
||||
],
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue