diff --git a/internal/oauth/encrypt.go b/internal/oauth/encrypt.go deleted file mode 100644 index ed821924c..000000000 --- a/internal/oauth/encrypt.go +++ /dev/null @@ -1,191 +0,0 @@ -package oauth - -import ( - "crypto/aes" - "crypto/cipher" - "crypto/rand" - "errors" - "fmt" - "io" - "os" - "path/filepath" - "time" -) - -const ( - // secretBytes is the AES-256 key length kept in the per-user secret file. - secretBytes = 32 - - secretRetryAttempts = 500 - secretRetryDelay = 2 * time.Millisecond -) - -// aesGCMCrypter encrypts the token file at rest with AES-256-GCM under a -// per-user random secret persisted (0600) beside the token file. The on-disk -// blob is nonce || ciphertext; GCM provides confidentiality AND tamper -// detection, so a corrupted/forged file fails closed on open. This is the -// opt-in "encrypted-file" storage backend; the default backend writes the -// 0600 plaintext JSON unchanged. -type aesGCMCrypter struct { - secretPath string -} - -func newAESGCMCrypter(secretPath string) *aesGCMCrypter { - return &aesGCMCrypter{secretPath: secretPath} -} - -// aead loads (or, when create is set, generates) the secret and returns the GCM -// AEAD. open passes create=false so a missing secret is a hard error rather than -// silently minting a new key that could never decrypt the existing file. -func (c *aesGCMCrypter) aead(create bool) (cipher.AEAD, error) { - secret, err := loadOrCreateSecret(c.secretPath, create) - if err != nil { - return nil, err - } - block, err := aes.NewCipher(secret) - if err != nil { - return nil, fmt.Errorf("oauth: build cipher: %w", err) - } - return cipher.NewGCM(block) -} - -// seal encrypts plaintext, prefixing a fresh random nonce. It creates the secret -// on first use. -func (c *aesGCMCrypter) seal(plaintext []byte) ([]byte, error) { - gcm, err := c.aead(true) - if err != nil { - return nil, err - } - nonce := make([]byte, gcm.NonceSize()) - if _, err := io.ReadFull(rand.Reader, nonce); err != nil { - return nil, fmt.Errorf("oauth: generate nonce: %w", err) - } - return gcm.Seal(nonce, nonce, plaintext, nil), nil -} - -// open decrypts a nonce||ciphertext blob, failing closed on a missing secret, -// a short blob, or a failed authentication tag (tampering / wrong key). -func (c *aesGCMCrypter) open(blob []byte) ([]byte, error) { - gcm, err := c.aead(false) - if err != nil { - return nil, err - } - if len(blob) < gcm.NonceSize() { - return nil, errors.New("oauth: encrypted token file is too short") - } - nonce, ciphertext := blob[:gcm.NonceSize()], blob[gcm.NonceSize():] - plaintext, err := gcm.Open(nil, nonce, ciphertext, nil) - if err != nil { - return nil, fmt.Errorf("oauth: decrypt token file (wrong secret or tampered): %w", err) - } - return plaintext, nil -} - -// loadOrCreateSecret reads the 32-byte secret at path. When create is set and -// the file is absent, it generates a random secret and creates the file -// atomically (0600). A wrong-sized existing secret fails closed (corruption). -func loadOrCreateSecret(path string, create bool) ([]byte, error) { - if data, err := readSecretFileRetry(path); err == nil { - return data, nil - } else if !errors.Is(err, os.ErrNotExist) { - return nil, err - } - if !create { - return nil, fmt.Errorf("oauth: token secret %s is missing; cannot decrypt the token file", path) - } - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return nil, err - } - return createSecretFile(path) -} - -func createSecretFile(path string) ([]byte, error) { - lockPath := path + ".lock" - var lastErr error - for attempt := 0; attempt < secretRetryAttempts; attempt++ { - lock, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) - if err == nil { - _ = lock.Close() - defer os.Remove(lockPath) - if data, rerr := readSecretFileRetry(path); rerr == nil { - return data, nil - } else if !errors.Is(rerr, os.ErrNotExist) { - return nil, rerr - } - return writeNewSecretFile(path) - } - // On Windows a concurrent holder's os.Remove leaves the lock file in a - // "delete pending" state, so an O_EXCL create races it with - // ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that - // as contention and retry too — mirroring acquireFileLock in lock.go — - // otherwise concurrent secret creation spuriously fails on Windows. - if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { - return nil, fmt.Errorf("oauth: create token secret lock: %w", err) - } - // Remember the lock-creation error itself rather than a subsequent - // "secret file doesn't exist yet" read error -- otherwise a real - // contention/ACL problem is masked by the expected-while-waiting - // ErrNotExist once the retries are exhausted. - lastErr = err - if data, rerr := readSecretFileRetry(path); rerr == nil { - return data, nil - } - time.Sleep(secretRetryDelay) - } - return nil, fmt.Errorf("oauth: timed out waiting for token secret %s: %w", path, lastErr) -} - -func writeNewSecretFile(path string) ([]byte, error) { - secret := make([]byte, secretBytes) - if _, err := io.ReadFull(rand.Reader, secret); err != nil { - return nil, fmt.Errorf("oauth: generate token secret: %w", err) - } - // The encryption key is published exactly like the token blob: through the - // protected per-store directory, under a random name a sandboxed process - // cannot guess, wait for, or rename away. - tmp, tmpPath, err := createPublicationFile(path) - if err != nil { - return nil, fmt.Errorf("oauth: create token secret temp file: %w", err) - } - defer os.Remove(tmpPath) - if _, werr := tmp.Write(secret); werr != nil { - _ = tmp.Close() - return nil, fmt.Errorf("oauth: write token secret: %w", werr) - } - if cerr := tmp.Close(); cerr != nil { - return nil, fmt.Errorf("oauth: write token secret: %w", cerr) - } - if err := os.Rename(tmpPath, path); err != nil { - return nil, fmt.Errorf("oauth: publish token secret: %w", err) - } - return secret, nil -} - -func readSecretFileRetry(path string) ([]byte, error) { - var lastErr error - for attempt := 0; attempt < secretRetryAttempts; attempt++ { - data, err := readSecretFile(path) - if !isTransientSecretAccessError(err) { - return data, err - } - lastErr = err - time.Sleep(secretRetryDelay) - } - return nil, fmt.Errorf("oauth: timed out waiting for token secret %s: %w", path, lastErr) -} - -// readSecretFile reads and validates the secret at path, returning a wrapped -// os.ErrNotExist when it is absent so callers can branch on creation. -func readSecretFile(path string) ([]byte, error) { - data, err := os.ReadFile(path) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return nil, err - } - return nil, fmt.Errorf("oauth: read token secret: %w", err) - } - if len(data) != secretBytes { - return nil, fmt.Errorf("oauth: token secret at %s has unexpected size %d", path, len(data)) - } - return data, nil -} diff --git a/internal/oauth/encrypt_test.go b/internal/oauth/encrypt_test.go index 701afbc0f..2ab91e8e8 100644 --- a/internal/oauth/encrypt_test.go +++ b/internal/oauth/encrypt_test.go @@ -1,103 +1,13 @@ package oauth import ( - "bytes" "os" "path/filepath" - "runtime" "strings" - "sync" "testing" + "time" ) -func TestLoadOrCreateSecretConcurrentConverges(t *testing.T) { - path := filepath.Join(t.TempDir(), "tok.json.secret") - const n = 16 - secrets := make([][]byte, n) - errs := make([]error, n) - var wg sync.WaitGroup - for i := 0; i < n; i++ { - wg.Add(1) - go func(i int) { - defer wg.Done() - secrets[i], errs[i] = loadOrCreateSecret(path, true) - }(i) - } - wg.Wait() - // Exactly one creator wins; every racer must converge on the same on-disk - // secret rather than reading a half-published file or orphaning its own. - for i := 0; i < n; i++ { - if errs[i] != nil { - t.Fatalf("goroutine %d: %v", i, errs[i]) - } - if !bytes.Equal(secrets[i], secrets[0]) { - t.Fatalf("goroutine %d got a divergent secret; concurrent create did not converge", i) - } - } -} - -func TestAESGCMCrypterRoundTripAndTamper(t *testing.T) { - secretPath := filepath.Join(t.TempDir(), "tok.json.secret") - c := newAESGCMCrypter(secretPath) - plaintext := []byte(`{"schemaVersion":1,"tokens":{}}`) - blob, err := c.seal(plaintext) - if err != nil { - t.Fatalf("seal: %v", err) - } - if bytes.Contains(blob, plaintext) { - t.Fatal("sealed blob must not contain the plaintext") - } - got, err := c.open(blob) - if err != nil { - t.Fatalf("open: %v", err) - } - if !bytes.Equal(got, plaintext) { - t.Fatalf("round-trip mismatch: %q", got) - } - // Tamper: flipping any byte breaks the GCM tag. - tampered := append([]byte(nil), blob...) - tampered[len(tampered)-1] ^= 0xff - if _, err := c.open(tampered); err == nil { - t.Fatal("open must reject a tampered blob (GCM auth)") - } -} - -func TestLoadOrCreateSecret(t *testing.T) { - path := filepath.Join(t.TempDir(), "tok.json.secret") - // Missing + create=false => fail closed (can't decrypt without the secret). - if _, err := loadOrCreateSecret(path, false); err == nil { - t.Fatal("missing secret with create=false must error") - } - secret, err := loadOrCreateSecret(path, true) - if err != nil { - t.Fatalf("create: %v", err) - } - if len(secret) != secretBytes { - t.Fatalf("secret length = %d, want %d", len(secret), secretBytes) - } - // Stable across reads. - again, err := loadOrCreateSecret(path, false) - if err != nil || !bytes.Equal(again, secret) { - t.Fatalf("secret not stable: %v", err) - } - if runtime.GOOS != "windows" { - info, err := os.Stat(path) - if err != nil { - t.Fatalf("stat secret: %v", err) - } - if perm := info.Mode().Perm(); perm != 0o600 { - t.Fatalf("secret file mode = %o, want 600", perm) - } - } - // Wrong-sized secret fails closed. - if err := os.WriteFile(path, []byte("short"), 0o600); err != nil { - t.Fatalf("corrupt secret: %v", err) - } - if _, err := loadOrCreateSecret(path, true); err == nil { - t.Fatal("wrong-sized secret must error") - } -} - func newEncryptedStore(t *testing.T) (*Store, string) { t.Helper() path := filepath.Join(t.TempDir(), "oauth-tokens.json") @@ -202,3 +112,36 @@ func TestEncryptedStoreMissingSecretFailsClosed(t *testing.T) { t.Fatal("missing secret must fail closed, not return empty") } } + +// TestEncryptedStoreReclaimsStaleSecretLock reproduces a crash while the +// encrypted-file backend was creating its AES key. The key-creation lock is an +// O_EXCL ownership marker, not a kernel lock, so a process killed between +// creating and removing it leaves a file that every later Save waits on and +// then rejects forever. A stale marker must be reclaimed instead. +func TestEncryptedStoreReclaimsStaleSecretLock(t *testing.T) { + s, path := newEncryptedStore(t) + lockPath := path + ".secret.lock" + if err := os.WriteFile(lockPath, []byte("crashed-holder"), 0o600); err != nil { + t.Fatalf("write stale secret lock: %v", err) + } + stale := time.Now().Add(-2 * time.Minute) + if err := os.Chtimes(lockPath, stale, stale); err != nil { + t.Fatalf("backdate stale secret lock: %v", err) + } + + if err := s.Save(ProviderKey("demo"), Token{AccessToken: "after-crash"}); err != nil { + t.Fatalf("Save must reclaim a stale secret lock, got: %v", err) + } + if _, err := os.Stat(lockPath); err == nil { + t.Fatal("stale secret lock was not cleaned up") + } + + s2, err := NewStore(StoreOptions{FilePath: path, Encrypted: true}) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + token, ok, err := s2.Load(ProviderKey("demo")) + if err != nil || !ok || token.AccessToken != "after-crash" { + t.Fatalf("Load after stale-lock recovery = %+v ok=%v err=%v", token, ok, err) + } +} diff --git a/internal/oauth/publication_test.go b/internal/oauth/publication_test.go new file mode 100644 index 000000000..6ffcae205 --- /dev/null +++ b/internal/oauth/publication_test.go @@ -0,0 +1,98 @@ +package oauth + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestFileBlobWriteSyncsFileAndDirectory(t *testing.T) { + path := filepath.Join(t.TempDir(), "oauth-tokens.json") + blob := fileBlob{path: path} + origFile, origDir := syncOAuthFileFn, syncOAuthDirFn + t.Cleanup(func() { + syncOAuthFileFn, syncOAuthDirFn = origFile, origDir + }) + fileSynced, dirSynced := false, false + syncOAuthFileFn = func(file *os.File) error { + fileSynced = true + return origFile(file) + } + syncOAuthDirFn = func(dir string) error { + dirSynced = true + return origDir(dir) + } + + if err := blob.write([]byte("payload")); err != nil { + t.Fatalf("write: %v", err) + } + if !fileSynced { + t.Fatal("token blob was not synced before rename") + } + if !dirSynced { + t.Fatal("token store directory was not synced after rename") + } + data, ok, err := blob.read() + if err != nil || !ok || !bytes.Equal(data, []byte("payload")) { + t.Fatalf("read = %q ok=%v err=%v", data, ok, err) + } +} + +// TestFileBlobWriteDirectorySyncFailureLeavesPublishedBlob pins the post-rename +// path: the directory barrier runs after the atomic replace, so a barrier +// failure reports an error but must not hide the already-published token. +func TestFileBlobWriteDirectorySyncFailureLeavesPublishedBlob(t *testing.T) { + path := filepath.Join(t.TempDir(), "oauth-tokens.json") + blob := fileBlob{path: path} + if err := blob.write([]byte("old")); err != nil { + t.Fatalf("seed write: %v", err) + } + + origDir := syncOAuthDirFn + t.Cleanup(func() { syncOAuthDirFn = origDir }) + syncErr := errors.New("directory sync failed") + syncOAuthDirFn = func(string) error { return syncErr } + + err := blob.write([]byte("new")) + if !errors.Is(err, syncErr) { + t.Fatalf("write error = %v, want it to wrap %v", err, syncErr) + } + data, ok, err := blob.read() + if err != nil || !ok || !bytes.Equal(data, []byte("new")) { + t.Fatalf("published blob was not retained: data=%q ok=%v err=%v", data, ok, err) + } +} + +func TestFileBlobWriteSyncFailurePreservesPrevious(t *testing.T) { + path := filepath.Join(t.TempDir(), "oauth-tokens.json") + blob := fileBlob{path: path} + if err := blob.write([]byte("old")); err != nil { + t.Fatalf("seed write: %v", err) + } + + origFile := syncOAuthFileFn + t.Cleanup(func() { syncOAuthFileFn = origFile }) + syncErr := errors.New("sync failed") + syncOAuthFileFn = func(*os.File) error { return syncErr } + + err := blob.write([]byte("new")) + if !errors.Is(err, syncErr) { + t.Fatalf("write error = %v, want it to wrap %v", err, syncErr) + } + data, ok, err := blob.read() + if err != nil || !ok || !bytes.Equal(data, []byte("old")) { + t.Fatalf("previous blob was not preserved: data=%q ok=%v err=%v", data, ok, err) + } + entries, err := os.ReadDir(PublicationDir(path)) + if err != nil { + t.Fatalf("ReadDir publication dir: %v", err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), "publish-") { + t.Fatalf("failed write left publication temp %s", entry.Name()) + } + } +} diff --git a/internal/oauth/secret_access_other.go b/internal/oauth/secret_access_other.go deleted file mode 100644 index 380f4cd60..000000000 --- a/internal/oauth/secret_access_other.go +++ /dev/null @@ -1,7 +0,0 @@ -//go:build !windows - -package oauth - -func isTransientSecretAccessError(error) bool { - return false -} diff --git a/internal/oauth/secret_access_windows.go b/internal/oauth/secret_access_windows.go deleted file mode 100644 index b2db602c3..000000000 --- a/internal/oauth/secret_access_windows.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build windows - -package oauth - -import ( - "errors" - - "golang.org/x/sys/windows" -) - -func isTransientSecretAccessError(err error) bool { - return errors.Is(err, windows.ERROR_SHARING_VIOLATION) || - errors.Is(err, windows.ERROR_LOCK_VIOLATION) -} diff --git a/internal/oauth/store.go b/internal/oauth/store.go index ec30e380a..ae066acef 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -18,7 +18,9 @@ import ( "sync" "time" + "github.com/Twigpine/zero/internal/fsutil" "github.com/Twigpine/zero/internal/keyring" + "github.com/Twigpine/zero/internal/securefile" ) const ( @@ -155,7 +157,7 @@ const ( // is AES-256-GCM ciphertext at rest. type Store struct { blob blobStore - crypter *aesGCMCrypter // nil => plaintext blob + crypter *securefile.Crypter // nil => plaintext blob now func() time.Time mu sync.Mutex } @@ -244,8 +246,15 @@ func NewStore(options StoreOptions) (*Store, error) { return nil, err } // The file blob holds AES-256-GCM ciphertext; the per-user secret lives in - // a sibling ".secret" file (see encrypt.go). - return &Store{blob: fileBlob{path: path}, crypter: newAESGCMCrypter(path + ".secret"), now: now}, nil + // a sibling ".secret" file. The secret publishes through the sandbox-denied + // ".secret.publish" directory so a same-user sandboxed process cannot race + // the plaintext key name. + secretPath := path + ".secret" + return &Store{ + blob: fileBlob{path: path}, + crypter: securefile.NewCrypterWithPublicationDir(secretPath, PublicationDir(secretPath)), + now: now, + }, nil case "keyring": kr := options.Keyring if kr == nil { @@ -405,7 +414,7 @@ func (s *Store) readState() (storeFile, error) { } if s.crypter != nil { // Encrypted backend: the blob is AES-256-GCM ciphertext, not JSON. - data, err = s.crypter.open(data) + data, err = s.crypter.Open(data) if err != nil { return storeFile{}, fmt.Errorf("oauth: decrypt token store at %s: %w", s.blob.location(), err) } @@ -437,9 +446,9 @@ func (s *Store) writeState(state storeFile) error { // writes opaque ciphertext instead. payload := append(data, '\n') if s.crypter != nil { - payload, err = s.crypter.seal(data) + payload, err = s.crypter.Seal(data) if err != nil { - return err + return fmt.Errorf("oauth: encrypt token store: %w", err) } } return s.blob.write(payload) @@ -511,6 +520,31 @@ func (b fileBlob) read() ([]byte, bool, error) { return data, true, nil } +// Sync seams allow tests to inject persistence failures at each barrier. +var ( + syncOAuthFileFn = (*os.File).Sync + syncOAuthDirFn = syncOAuthDir +) + +// syncOAuthDir fsyncs the directory that now holds the published token store. +// Windows cannot sync directory handles, so the rename is best-effort durable +// there. A directory the user can write but not read cannot be opened for +// syncing either; that is also best effort, because the write it follows +// already succeeded. Every other failure is reported. +func syncOAuthDir(dir string) error { + if runtime.GOOS == "windows" { + return nil + } + d, err := os.Open(dir) + if err != nil { + if errors.Is(err, os.ErrPermission) { + return nil + } + return err + } + return errors.Join(d.Sync(), d.Close()) +} + func (b fileBlob) write(data []byte) error { if err := os.MkdirAll(filepath.Dir(b.path), 0o700); err != nil { return err @@ -524,10 +558,20 @@ func (b fileBlob) write(data []byte) error { _ = temp.Close() return err } + if err := syncOAuthFileFn(temp); err != nil { + _ = temp.Close() + return fmt.Errorf("oauth: sync token store: %w", err) + } if err := temp.Close(); err != nil { - return err + return fmt.Errorf("oauth: write token store: %w", err) + } + if err := fsutil.RenameWithRetry(tempPath, b.path, nil); err != nil { + return fmt.Errorf("oauth: publish token store: %w", err) } - return os.Rename(tempPath, b.path) + if err := syncOAuthDirFn(filepath.Dir(b.path)); err != nil { + return fmt.Errorf("oauth: sync token store directory: %w", err) + } + return nil } // PublicationDirSuffix names the per-store directory a token store publishes new diff --git a/internal/securefile/securefile.go b/internal/securefile/securefile.go index 054ac8bcc..6e9190eb0 100644 --- a/internal/securefile/securefile.go +++ b/internal/securefile/securefile.go @@ -15,6 +15,7 @@ import ( "io" "os" "path/filepath" + "runtime" "sync/atomic" "time" ) @@ -30,13 +31,35 @@ const ( var ( openSecretLockFile = os.OpenFile + syncSecretFileFn = (*os.File).Sync + syncSecretDirFn = syncSecretDir secureLockSeq atomic.Uint64 ) +// syncSecretDir fsyncs the directory that now holds the published secret. +// Windows cannot sync directory handles, so the rename is best-effort durable +// there. A directory the user can write but not read cannot be opened for +// syncing either; that is also best effort, because the write it follows +// already succeeded. Every other failure is reported. +func syncSecretDir(dir string) error { + if runtime.GOOS == "windows" { + return nil + } + d, err := os.Open(dir) + if err != nil { + if errors.Is(err, os.ErrPermission) { + return nil + } + return err + } + return errors.Join(d.Sync(), d.Close()) +} + // Crypter encrypts a blob at rest with AES-256-GCM under a per-user random secret // persisted (0600) at secretPath. type Crypter struct { - secretPath string + secretPath string + publicationDir string } // NewCrypter returns a Crypter whose key lives at secretPath (created on first @@ -45,11 +68,20 @@ func NewCrypter(secretPath string) *Crypter { return &Crypter{secretPath: secretPath} } +// NewCrypterWithPublicationDir is NewCrypter with secret temp files published +// through publicationDir instead of beside the secret. Callers that protect a +// sandboxed process from racing the plaintext key use this to put the temp file +// in a directory their sandbox profile denies. The directory must be on the +// same filesystem as secretPath; the final rename remains atomic. +func NewCrypterWithPublicationDir(secretPath, publicationDir string) *Crypter { + return &Crypter{secretPath: secretPath, publicationDir: publicationDir} +} + // aead loads (or, when create is set, generates) the secret and returns the GCM // AEAD. Open passes create=false so a missing secret is a hard error rather than // silently minting a new key that could never decrypt the existing file. func (c *Crypter) aead(create bool) (cipher.AEAD, error) { - secret, err := loadOrCreateSecret(c.secretPath, create) + secret, err := loadOrCreateSecret(c.secretPath, c.publicationDir, create) if err != nil { return nil, err } @@ -95,7 +127,7 @@ func (c *Crypter) Open(blob []byte) ([]byte, error) { // loadOrCreateSecret reads the 32-byte secret at path. When create is set and the // file is absent, it generates a random secret and creates the file atomically // (0600). A wrong-sized existing secret fails closed (corruption). -func loadOrCreateSecret(path string, create bool) ([]byte, error) { +func loadOrCreateSecret(path, publicationDir string, create bool) ([]byte, error) { if data, err := readSecretFileRetry(path); err == nil { return data, nil } else if !errors.Is(err, os.ErrNotExist) { @@ -107,10 +139,10 @@ func loadOrCreateSecret(path string, create bool) ([]byte, error) { if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return nil, err } - return createSecretFile(path) + return createSecretFile(path, publicationDir) } -func createSecretFile(path string) ([]byte, error) { +func createSecretFile(path, publicationDir string) ([]byte, error) { lockPath := path + ".lock" token := fmt.Sprintf("%d-%d-%d", os.Getpid(), time.Now().UnixNano(), secureLockSeq.Add(1)) var lastErr error @@ -136,13 +168,13 @@ func createSecretFile(path string) ([]byte, error) { } else if !errors.Is(rerr, os.ErrNotExist) { return nil, rerr } - return writeNewSecretFile(path) + return writeNewSecretFile(path, publicationDir) } // On Windows a concurrent holder's os.Remove leaves the lock file in a // "delete pending" state, so an O_EXCL create races it with // ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that - // as contention and retry too -- mirroring oauth's createSecretFile -- - // otherwise concurrent secret creation spuriously fails on Windows. + // as contention and retry, otherwise concurrent secret creation + // spuriously fails on Windows. if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { return nil, fmt.Errorf("securefile: create secret lock: %w", err) } @@ -173,20 +205,63 @@ func reclaimStaleLock(lockPath, token string, staleAfter time.Duration) bool { return false } if info, err := os.Stat(reclaimed); err == nil && time.Since(info.ModTime()) <= staleAfter { - _ = os.Rename(reclaimed, lockPath) + // The lock was replaced between our stat and rename, so put the fresh + // lock back without clobbering a third holder that acquired lockPath in + // the meantime. + _ = restoreReclaimedLock(reclaimed, lockPath) return false } _ = os.Remove(reclaimed) return true } -func writeNewSecretFile(path string) ([]byte, error) { +// restoreReclaimedLock restores a quarantined lock only if lockPath is still +// free. The destination is created exclusively; a newer holder is never +// replaced. The quarantined copy is discarded either way. +func restoreReclaimedLock(reclaimed, lockPath string) error { + data, err := os.ReadFile(reclaimed) + if err != nil { + _ = os.Remove(reclaimed) + return err + } + lock, err := openSecretLockFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + _ = os.Remove(reclaimed) + return err + } + if _, err := lock.Write(data); err != nil { + _ = lock.Close() + _ = os.Remove(lockPath) + _ = os.Remove(reclaimed) + return err + } + if err := lock.Close(); err != nil { + _ = os.Remove(lockPath) + _ = os.Remove(reclaimed) + return err + } + _ = os.Remove(reclaimed) + return nil +} + +func writeNewSecretFile(path, publicationDir string) ([]byte, error) { secret := make([]byte, secretBytes) if _, err := io.ReadFull(rand.Reader, secret); err != nil { return nil, fmt.Errorf("securefile: generate secret: %w", err) } - dir := filepath.Dir(path) - tmp, err := os.CreateTemp(dir, filepath.Base(path)+".*.tmp") + dir := publicationDir + prefix := filepath.Base(path) + ".*.tmp" + if dir == "" { + dir = filepath.Dir(path) + } else { + // Keep the prefix oauth's store reset uses to clean crash residue from a + // publication directory. + prefix = "publish-*" + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("securefile: create secret publication dir: %w", err) + } + tmp, err := os.CreateTemp(dir, prefix) if err != nil { return nil, fmt.Errorf("securefile: create secret temp file: %w", err) } @@ -200,7 +275,7 @@ func writeNewSecretFile(path string) ([]byte, error) { _ = tmp.Close() return nil, fmt.Errorf("securefile: write secret: %w", werr) } - if serr := tmp.Sync(); serr != nil { + if serr := syncSecretFileFn(tmp); serr != nil { _ = tmp.Close() return nil, fmt.Errorf("securefile: sync secret: %w", serr) } @@ -210,6 +285,12 @@ func writeNewSecretFile(path string) ([]byte, error) { if err := os.Rename(tmpPath, path); err != nil { return nil, fmt.Errorf("securefile: publish secret: %w", err) } + // The key is now visible at its final name; sync the destination directory + // so the rename itself survives a crash. The source publication directory + // only held an unlinked temp entry and does not need a barrier. + if err := syncSecretDirFn(filepath.Dir(path)); err != nil { + return nil, fmt.Errorf("securefile: sync secret directory: %w", err) + } return secret, nil } diff --git a/internal/securefile/securefile_test.go b/internal/securefile/securefile_test.go index 9edb5486d..7d7aec433 100644 --- a/internal/securefile/securefile_test.go +++ b/internal/securefile/securefile_test.go @@ -1,9 +1,12 @@ package securefile import ( + "bytes" + "errors" "os" "path/filepath" "runtime" + "sync" "testing" "time" ) @@ -23,7 +26,7 @@ func TestCreateSecretFileRetriesOnPermissionContention(t *testing.T) { openSecretLockFile = origOpen }) - secret, err := createSecretFile(secretPath) + secret, err := createSecretFile(secretPath, "") if err != nil { t.Fatalf("createSecretFile should retry on permission contention: %v", err) } @@ -125,7 +128,7 @@ func TestCreateSecretFileReclaimsStaleLock(t *testing.T) { } // Attempt to create the secret file; it should detect the stale lock, reclaim it, and succeed - secret, err := createSecretFile(secretPath) + secret, err := createSecretFile(secretPath, "") if err != nil { t.Fatalf("createSecretFile failed to reclaim stale lock: %v", err) } @@ -138,3 +141,198 @@ func TestCreateSecretFileReclaimsStaleLock(t *testing.T) { t.Fatal("expected lock file to be cleaned up after successful acquisition") } } + +func TestSecretCreationConcurrentConverges(t *testing.T) { + secretPath := filepath.Join(t.TempDir(), "k.secret") + const n = 16 + blobs := make([][]byte, n) + errs := make([]error, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + blobs[i], errs[i] = NewCrypter(secretPath).Seal([]byte("payload")) + }(i) + } + wg.Wait() + + c := NewCrypter(secretPath) + for i := 0; i < n; i++ { + if errs[i] != nil { + t.Fatalf("goroutine %d: %v", i, errs[i]) + } + got, err := c.Open(blobs[i]) + if err != nil { + t.Fatalf("goroutine %d blob does not decrypt under the converged secret: %v", i, err) + } + if !bytes.Equal(got, []byte("payload")) { + t.Fatalf("goroutine %d plaintext = %q", i, got) + } + } +} + +func TestWrongSizedSecretFailsClosed(t *testing.T) { + secretPath := filepath.Join(t.TempDir(), "k.secret") + if err := os.WriteFile(secretPath, []byte("short"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := NewCrypter(secretPath).Seal([]byte("payload")); err == nil { + t.Fatal("wrong-sized secret must error, not mint a new key") + } +} + +func TestNewCrypterWithPublicationDirPublishesThroughIt(t *testing.T) { + dir := t.TempDir() + secretPath := filepath.Join(dir, "tokens.json.secret") + pubDir := secretPath + ".publish" + c := NewCrypterWithPublicationDir(secretPath, pubDir) + if _, err := c.Seal([]byte("payload")); err != nil { + t.Fatalf("Seal: %v", err) + } + if info, err := os.Stat(secretPath); err != nil || info.Size() != secretBytes { + t.Fatalf("secret = %v err=%v", info, err) + } + entries, err := os.ReadDir(pubDir) + if err != nil { + t.Fatalf("publication dir: %v", err) + } + if len(entries) != 0 { + t.Fatalf("publication dir left temp files: %v", entries) + } + if entries, err := os.ReadDir(dir); err != nil { + t.Fatalf("parent dir: %v", err) + } else { + for _, entry := range entries { + if entry.Name() != "tokens.json.secret" && entry.Name() != filepath.Base(pubDir) { + t.Fatalf("unexpected parent-dir entry %q; secret must publish through the protected dir", entry.Name()) + } + } + } +} + +func TestSecretCreationSyncsFileAndDirectory(t *testing.T) { + secretPath := filepath.Join(t.TempDir(), "k.secret") + origFile, origDir := syncSecretFileFn, syncSecretDirFn + t.Cleanup(func() { + syncSecretFileFn, syncSecretDirFn = origFile, origDir + }) + fileSynced, dirSynced := false, false + syncSecretFileFn = func(file *os.File) error { + fileSynced = true + return origFile(file) + } + syncSecretDirFn = func(dir string) error { + dirSynced = true + return origDir(dir) + } + if _, err := createSecretFile(secretPath, ""); err != nil { + t.Fatalf("createSecretFile: %v", err) + } + if !fileSynced { + t.Fatal("secret temp file was not synced before rename") + } + if !dirSynced { + t.Fatal("secret destination directory was not synced after rename") + } +} + +// TestSecretDirectorySyncFailureLeavesPublishedSecret pins the post-rename +// path: a destination-directory barrier failure reports an error but the +// renamed secret is already there and a later caller can load it. +func TestSecretDirectorySyncFailureLeavesPublishedSecret(t *testing.T) { + secretPath := filepath.Join(t.TempDir(), "k.secret") + origDir := syncSecretDirFn + t.Cleanup(func() { syncSecretDirFn = origDir }) + syncErr := errors.New("directory sync failed") + syncSecretDirFn = func(string) error { return syncErr } + + if _, err := createSecretFile(secretPath, ""); !errors.Is(err, syncErr) { + t.Fatalf("createSecretFile error = %v, want %v", err, syncErr) + } + published, err := os.ReadFile(secretPath) + if err != nil { + t.Fatalf("read published secret: %v", err) + } + if len(published) != secretBytes { + t.Fatalf("published secret size = %d, want %d", len(published), secretBytes) + } + loaded, err := createSecretFile(secretPath, "") + if err != nil { + t.Fatalf("later caller could not load the published secret: %v", err) + } + if !bytes.Equal(loaded, published) { + t.Fatal("later caller did not reuse the published secret") + } +} + +// TestRestoreReclaimedLockDoesNotReplaceNewerLock covers the reclaim race: a +// lock that looked stale can be replaced before the reclaimer renames it. If a +// third holder then creates lockPath, restoring the quarantined lock must not +// replace that newer lock. +func TestRestoreReclaimedLockDoesNotReplaceNewerLock(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, "k.secret.lock") + reclaimed := lockPath + ".stale.crashed" + if err := os.WriteFile(reclaimed, []byte("quarantined-token"), 0o600); err != nil { + t.Fatalf("write quarantined lock: %v", err) + } + if err := os.WriteFile(lockPath, []byte("new-holder-token"), 0o600); err != nil { + t.Fatalf("write newer lock: %v", err) + } + + if err := restoreReclaimedLock(reclaimed, lockPath); !errors.Is(err, os.ErrExist) { + t.Fatalf("restoreReclaimedLock error = %v, want %v", err, os.ErrExist) + } + data, err := os.ReadFile(lockPath) + if err != nil { + t.Fatalf("read newer lock: %v", err) + } + if string(data) != "new-holder-token" { + t.Fatalf("newer lock was replaced: %q", data) + } + if _, err := os.Stat(reclaimed); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("quarantined lock was not discarded: %v", err) + } +} + +func TestRestoreReclaimedLockRestoresWhenDestinationIsFree(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, "k.secret.lock") + reclaimed := lockPath + ".stale.crashed" + if err := os.WriteFile(reclaimed, []byte("fresh-token"), 0o600); err != nil { + t.Fatalf("write quarantined lock: %v", err) + } + + if err := restoreReclaimedLock(reclaimed, lockPath); err != nil { + t.Fatalf("restoreReclaimedLock: %v", err) + } + data, err := os.ReadFile(lockPath) + if err != nil { + t.Fatalf("read restored lock: %v", err) + } + if string(data) != "fresh-token" { + t.Fatalf("restored lock = %q", data) + } + if _, err := os.Stat(reclaimed); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("quarantined lock was not discarded: %v", err) + } +} + +func TestSecretSyncFailureLeavesNoSecretAndRetrySucceeds(t *testing.T) { + secretPath := filepath.Join(t.TempDir(), "k.secret") + origFile := syncSecretFileFn + t.Cleanup(func() { syncSecretFileFn = origFile }) + syncErr := errors.New("sync failed") + syncSecretFileFn = func(*os.File) error { return syncErr } + if _, err := createSecretFile(secretPath, ""); !errors.Is(err, syncErr) { + t.Fatalf("createSecretFile error = %v, want %v", err, syncErr) + } + if _, err := os.Stat(secretPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("failed publish left a secret: %v", err) + } + syncSecretFileFn = origFile + if _, err := createSecretFile(secretPath, ""); err != nil { + t.Fatalf("retry after sync failure: %v", err) + } +}