From b62bd6e9c7309dc39b31fbf1513a234d2af7589e Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 20 Oct 2026 16:30:49 +0530 Subject: [PATCH 1/3] fix(oauth): use the crash-safe secret store and sync token publications The encrypted-file OAuth backend carried its own AES-GCM secret implementation that had drifted from internal/securefile: a crash during first login left .lock behind and every later Save timed out instead of reclaiming it, and the secret and token blobs were published without fsync.\n\nUse securefile.Crypter with an opt-in publication directory so the secret still publishes through the sandbox-denied .secret.publish directory, add file and directory barriers to the encrypted token blob, and extend securefile with the publication directory plus destination-directory sync.\n\nMove the concurrent-creation and wrong-sized-secret coverage to securefile, add a stale-lock regression through the OAuth store, and test publication-dir use, sync ordering, sync-failure preservation, and retry after a failed secret sync. The stale-lock test fails on the previous code. --- internal/oauth/encrypt.go | 191 ------------------------ internal/oauth/encrypt_test.go | 125 +++++----------- internal/oauth/publication_test.go | 73 +++++++++ internal/oauth/secret_access_other.go | 7 - internal/oauth/secret_access_windows.go | 14 -- internal/oauth/store.go | 60 +++++++- internal/securefile/securefile.go | 73 +++++++-- internal/securefile/securefile_test.go | 120 ++++++++++++++- 8 files changed, 338 insertions(+), 325 deletions(-) delete mode 100644 internal/oauth/encrypt.go create mode 100644 internal/oauth/publication_test.go delete mode 100644 internal/oauth/secret_access_other.go delete mode 100644 internal/oauth/secret_access_windows.go 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..653cf2ca2 --- /dev/null +++ b/internal/oauth/publication_test.go @@ -0,0 +1,73 @@ +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) + } +} + +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..088f52027 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) } @@ -180,13 +212,24 @@ func reclaimStaleLock(lockPath, token string, staleAfter time.Duration) bool { return true } -func writeNewSecretFile(path string) ([]byte, error) { +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 +243,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 +253,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..74d698291 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,116 @@ 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") + } +} + +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) + } +} From fd76ce0521d527dbc68e93ae1918c5d23d35607d Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 20 Oct 2026 16:52:16 +0530 Subject: [PATCH 2/3] test(oauth): cover post-rename directory-sync failures The existing failure tests inject a file sync error before the rename. Add coverage for a destination-directory barrier failure after the token or secret has been atomically replaced: the write must report the error and the newly published bytes must remain readable. Also cover the corresponding securefile secret path, including its expected size. --- internal/oauth/publication_test.go | 25 +++++++++++++++++++++++++ internal/securefile/securefile_test.go | 22 ++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/internal/oauth/publication_test.go b/internal/oauth/publication_test.go index 653cf2ca2..6ffcae205 100644 --- a/internal/oauth/publication_test.go +++ b/internal/oauth/publication_test.go @@ -41,6 +41,31 @@ func TestFileBlobWriteSyncsFileAndDirectory(t *testing.T) { } } +// 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} diff --git a/internal/securefile/securefile_test.go b/internal/securefile/securefile_test.go index 74d698291..3a4bf6727 100644 --- a/internal/securefile/securefile_test.go +++ b/internal/securefile/securefile_test.go @@ -237,6 +237,28 @@ func TestSecretCreationSyncsFileAndDirectory(t *testing.T) { } } +// 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) + } + info, err := os.Stat(secretPath) + if err != nil { + t.Fatalf("published secret: %v", err) + } + if info.Size() != secretBytes { + t.Fatalf("published secret size = %d, want %d", info.Size(), secretBytes) + } +} + func TestSecretSyncFailureLeavesNoSecretAndRetrySucceeds(t *testing.T) { secretPath := filepath.Join(t.TempDir(), "k.secret") origFile := syncSecretFileFn From e7b0e6fd861b4180a2a6decdf168cc5aed7665c0 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 20 Oct 2026 17:08:55 +0530 Subject: [PATCH 3/3] fix(securefile): never clobber a newer lock when restoring a reclaimed one reclaimStaleLock could rename a lock that had just been replaced, then restore the quarantined copy with os.Rename, replacing a third holder's new lock. Both holders could then publish different secrets for the same file. Restore with O_CREATE|O_EXCL and discard the quarantined copy when the destination is already held. Extend the directory-sync failure test to prove a later caller actually loads the published secret, not just its size. Add restore tests for the free and already-held cases. --- internal/securefile/securefile.go | 34 ++++++++++++- internal/securefile/securefile_test.go | 68 ++++++++++++++++++++++++-- 2 files changed, 97 insertions(+), 5 deletions(-) diff --git a/internal/securefile/securefile.go b/internal/securefile/securefile.go index 088f52027..6e9190eb0 100644 --- a/internal/securefile/securefile.go +++ b/internal/securefile/securefile.go @@ -205,13 +205,45 @@ 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 } +// 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 { diff --git a/internal/securefile/securefile_test.go b/internal/securefile/securefile_test.go index 3a4bf6727..7d7aec433 100644 --- a/internal/securefile/securefile_test.go +++ b/internal/securefile/securefile_test.go @@ -250,12 +250,72 @@ func TestSecretDirectorySyncFailureLeavesPublishedSecret(t *testing.T) { if _, err := createSecretFile(secretPath, ""); !errors.Is(err, syncErr) { t.Fatalf("createSecretFile error = %v, want %v", err, syncErr) } - info, err := os.Stat(secretPath) + published, err := os.ReadFile(secretPath) if err != nil { - t.Fatalf("published secret: %v", err) + t.Fatalf("read published secret: %v", err) } - if info.Size() != secretBytes { - t.Fatalf("published secret size = %d, want %d", info.Size(), secretBytes) + 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) } }