diff --git a/cmd/add.go b/cmd/add.go index 215683d..05dc4d7 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -114,11 +114,8 @@ func runAdd(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err) } - // Smart sync pull before unlock to get latest version - syncPullBeforeUnlock(vaultService) - - // Unlock vault - if err := unlockVault(vaultService); err != nil { + // Pull from remote and unlock, overlapping the pull with the password prompt (#103). + if err := unlockVaultWithSync(vaultService); err != nil { return err } defer vaultService.Lock() diff --git a/cmd/concurrent_unlock_test.go b/cmd/concurrent_unlock_test.go new file mode 100644 index 0000000..4a6c210 --- /dev/null +++ b/cmd/concurrent_unlock_test.go @@ -0,0 +1,82 @@ +package cmd + +import ( + "io" + "os" + "path/filepath" + "testing" + + "github.com/arimxyer/pass-cli/internal/vault" +) + +// feedStdin redirects os.Stdin to a pipe carrying input (one line) for the +// duration of the test, and enables PASS_CLI_TEST so readPassword reads from it +// via the shared test scanner instead of a TTY. Only one password read per test +// process is safe (the scanner initializes once), so keep a single reader here. +func feedStdin(t *testing.T, input string) { + t.Helper() + t.Setenv("PASS_CLI_TEST", "1") + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig; _ = r.Close() }) + + go func() { + _, _ = io.WriteString(w, input) + _ = w.Close() + }() +} + +// unlockVaultWithSync's password branch overlaps the network pull (goroutine) +// with the master-password prompt (main thread), then joins before decrypting. +// This drives that branch end-to-end against a real, sync-enabled, initialized +// vault with no keychain — exercising the concurrency so `go test -race` proves +// the goroutine + join is data-race free and the vault still unlocks correctly. +func TestUnlockVaultWithSync_PasswordBranchConcurrent(t *testing.T) { + const masterPassword = "TestPass!1234" + + tmpDir := t.TempDir() + vaultPath := filepath.Join(tmpDir, "vault.enc") + cfgPath := filepath.Join(tmpDir, "config.yml") + + // Sync enabled with a bogus remote: SyncPull fails fast / no-ops (offline- + // friendly) and returns quickly, so the goroutine joins without real network. + cfg := "vault_path: " + vaultPath + "\nsync:\n enabled: true\n remote: \"mock-remote:bucket\"\n" + if err := os.WriteFile(cfgPath, []byte(cfg), 0600); err != nil { + t.Fatalf("write config: %v", err) + } + t.Setenv("PASS_CLI_CONFIG", cfgPath) + + // Initialize the vault (no keychain) with a first service. + initVS, err := vault.New(vaultPath) + if err != nil { + t.Fatalf("vault.New (init): %v", err) + } + if err := initVS.Initialize([]byte(masterPassword), false, "", ""); err != nil { + t.Fatalf("Initialize: %v", err) + } + + // A fresh, locked service is what a command would unlock. + vs, err := vault.New(vaultPath) + if err != nil { + t.Fatalf("vault.New (locked): %v", err) + } + if !vs.IsSyncEnabled() { + t.Fatal("expected sync to be enabled for the concurrent branch") + } + + setOffline(t, false) + setVerbose(t, false) + feedStdin(t, masterPassword+"\n") + + if err := unlockVaultWithSync(vs); err != nil { + t.Fatalf("unlockVaultWithSync: %v", err) + } + if !vs.IsUnlocked() { + t.Error("expected vault to be unlocked after the concurrent password branch") + } +} diff --git a/cmd/delete.go b/cmd/delete.go index cf50a58..014d1d1 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -61,11 +61,8 @@ func runDelete(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err) } - // Smart sync pull before unlock to get latest version - syncPullBeforeUnlock(vaultService) - - // Unlock vault - if err := unlockVault(vaultService); err != nil { + // Pull from remote and unlock, overlapping the pull with the password prompt (#103). + if err := unlockVaultWithSync(vaultService); err != nil { return err } defer vaultService.Lock() diff --git a/cmd/exec.go b/cmd/exec.go index 57dd85f..b3e8320 100644 --- a/cmd/exec.go +++ b/cmd/exec.go @@ -193,11 +193,9 @@ func runExec(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err) } - // Smart sync pull before unlock to get latest version (read-only: no push after) - syncPullBeforeUnlock(vaultService) - - // Unlock vault - if err := unlockVault(vaultService); err != nil { + // Pull from remote and unlock, overlapping the pull with the password prompt + // (#103; read-only: no push after). + if err := unlockVaultWithSync(vaultService); err != nil { return err } defer vaultService.Lock() diff --git a/cmd/get.go b/cmd/get.go index 0688fbb..4b2a06f 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -101,11 +101,8 @@ func runGet(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err) } - // Smart sync pull before unlock to get latest version - syncPullBeforeUnlock(vaultService) - - // Unlock vault - if err := unlockVault(vaultService); err != nil { + // Pull from remote and unlock, overlapping the pull with the password prompt (#103). + if err := unlockVaultWithSync(vaultService); err != nil { return err } defer vaultService.Lock() diff --git a/cmd/helpers.go b/cmd/helpers.go index 4e88340..2b6945a 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -3,6 +3,7 @@ package cmd import ( "bufio" "fmt" + "github.com/arimxyer/pass-cli/internal/crypto" "github.com/arimxyer/pass-cli/internal/recovery" "github.com/arimxyer/pass-cli/internal/vault" "os" @@ -397,6 +398,82 @@ func unlockVault(vaultService *vault.VaultService) error { return nil } +// unlockVaultWithSync pulls from the remote and unlocks, overlapping the network +// pull with the master-password prompt to hide the pre-unlock probe latency (#103, +// Tier 1). It replaces the previous sequential `syncPullBeforeUnlock(vs)` + +// `unlockVault(vs)` pair at command entry points. +// +// Ordering constraint: Unlock reads and decrypts vault.enc in one step, and a pull +// replaces that file — so the decrypt must run strictly AFTER the pull finishes. +// Only work that does not touch vault.enc may overlap the pull: +// - the master-password prompt touches stdin only → safe to overlap (the win); +// - keychain unlock decrypts vault.enc, and keychain users have no prompt to hide +// behind, so that path stays sequential (Tier 2, overlapping the Argon2 KDF, is +// a deferred follow-up). +func unlockVaultWithSync(vaultService *vault.VaultService) error { + // No pull to overlap (sync disabled or --offline): today's exact behavior. + if IsOffline() || !vaultService.IsSyncEnabled() { + syncPullBeforeUnlock(vaultService) // no-op in these cases + return unlockVault(vaultService) + } + + // Keychain is resolved BEFORE the pull: retrieving the keychain password reads + // metadata + the OS keyring but never decrypts vault.enc. There's no human wait + // to hide behind, so pull then decrypt sequentially. + if password, err := vaultService.RetrieveKeychainPassword(); err == nil { + defer crypto.ClearBytes(password) + syncPullBeforeUnlock(vaultService) + if err := vaultService.Unlock(password); err != nil { + return fmt.Errorf("failed to unlock vault: %w", err) + } + if IsVerbose() { + fmt.Fprintln(os.Stderr, "🔓 Unlocked vault using keychain") + } + return nil + } + + // Password path: run the pull concurrently with the prompt, then join before + // decrypting against the now-current file. + pullDone := make(chan struct{}) + var pullErr error + go func() { + defer close(pullDone) + pullErr = vaultService.SyncPull() + }() + + if IsVerbose() { + // Full line (terminated by newline) so it cannot corrupt the prompt below. + fmt.Fprintln(os.Stderr, "🔄 Checking remote for vault changes...") + } + // No transient "Checking remote..." indicator here: it would garble the prompt + // line. The prompt itself signals that work is in flight; the pull runs quietly + // in the background and any warnings are re-surfaced cleanly after the join. + fmt.Fprint(os.Stderr, "Master password: ") + password, readErr := readPassword() + defer crypto.ClearBytes(password) + fmt.Fprintln(os.Stderr) // newline after password input + + <-pullDone // join on ALL paths before returning, so rclone is never orphaned + + if readErr != nil { + return fmt.Errorf("failed to read password: %w", readErr) + } + + // Re-surface a sync conflict (or pull error) cleanly below the prompt. SyncPull + // may have printed its own warning mid-prompt (garbled), and read commands never + // re-echo it (only writes do, via SyncPush) — so a failed signal would be lost. + if vaultService.SyncConflictDetected() { + fmt.Fprintln(os.Stderr, "Warning: sync conflict — local and remote vault both changed. Use `pass-cli sync resolve` to choose which version to keep.") + } else if pullErr != nil { + fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", pullErr) + } + + if err := vaultService.Unlock(password); err != nil { + return fmt.Errorf("failed to unlock vault: %w", err) + } + return nil +} + // syncPullBeforeUnlock performs a smart sync pull before vault unlock. // This ensures we have the latest version from remote before reading. // diff --git a/cmd/list.go b/cmd/list.go index b35e0c2..3671306 100644 --- a/cmd/list.go +++ b/cmd/list.go @@ -116,11 +116,8 @@ func runList(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err) } - // Smart sync pull before unlock to get latest version - syncPullBeforeUnlock(vaultService) - - // Unlock vault - if err := unlockVault(vaultService); err != nil { + // Pull from remote and unlock, overlapping the pull with the password prompt (#103). + if err := unlockVaultWithSync(vaultService); err != nil { return err } defer vaultService.Lock() diff --git a/cmd/update.go b/cmd/update.go index 3cf7c67..22a89a4 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -139,11 +139,8 @@ func runUpdate(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err) } - // Smart sync pull before unlock to get latest version - syncPullBeforeUnlock(vaultService) - - // Unlock vault - if err := unlockVault(vaultService); err != nil { + // Pull from remote and unlock, overlapping the pull with the password prompt (#103). + if err := unlockVaultWithSync(vaultService); err != nil { return err } defer vaultService.Lock() diff --git a/internal/vault/concurrent_unlock_test.go b/internal/vault/concurrent_unlock_test.go new file mode 100644 index 0000000..af45f4e --- /dev/null +++ b/internal/vault/concurrent_unlock_test.go @@ -0,0 +1,44 @@ +package vault + +import ( + "errors" + "path/filepath" + "testing" +) + +// RetrieveKeychainPassword must report ErrKeychainNotEnabled (and return no +// bytes) when keychain unlock isn't configured — the signal the concurrent +// unlock path uses to fall through to the password prompt. +func TestRetrieveKeychainPassword_NotEnabled(t *testing.T) { + tempDir := t.TempDir() + vaultPath := filepath.Join(tempDir, "vault.enc") + + v, err := New(vaultPath) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := v.Initialize([]byte("TestPass!1234"), false, "", ""); err != nil { + t.Fatalf("Initialize: %v", err) + } + + pw, err := v.RetrieveKeychainPassword() + if !errors.Is(err, ErrKeychainNotEnabled) { + t.Errorf("expected ErrKeychainNotEnabled, got err=%v", err) + } + if pw != nil { + t.Errorf("expected nil password on error, got %d bytes", len(pw)) + } +} + +// The conflict getter defaults to false on a fresh service and is what the +// concurrent unlock path reads after the join to re-surface a swallowed conflict. +func TestSyncConflictDetected_DefaultsFalse(t *testing.T) { + tempDir := t.TempDir() + v, err := New(filepath.Join(tempDir, "vault.enc")) + if err != nil { + t.Fatalf("New: %v", err) + } + if v.SyncConflictDetected() { + t.Error("expected SyncConflictDetected() == false on a fresh service") + } +} diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 4db3654..c2f291d 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -228,6 +228,14 @@ func (v *VaultService) IsSyncEnabled() bool { return v.syncService != nil && v.syncService.IsEnabled() } +// SyncConflictDetected reports whether the most recent SyncPull detected a +// conflict (both local and remote changed). Used by the concurrent-unlock path +// to re-surface the conflict cleanly after the password prompt, since SyncPull's +// own warning may print mid-prompt and read commands never re-echo it (#103). +func (v *VaultService) SyncConflictDetected() bool { + return v.syncConflictDetected +} + // SyncPush performs a smart sync push if sync is enabled. // Should be called once at the end of a command, not per-save. // Returns true if a push was actually performed. @@ -921,14 +929,30 @@ func (v *VaultService) UnlockWithKey(vaultKey []byte) error { // UnlockWithKeychain attempts to unlock using keychain-stored password func (v *VaultService) UnlockWithKeychain() error { + password, err := v.RetrieveKeychainPassword() + if err != nil { + return err + } + return v.Unlock(password) +} + +// RetrieveKeychainPassword returns the master password from the OS keychain +// without decrypting the vault. It reads metadata and the keyring (and performs +// best-effort migration from a legacy global entry) but never touches the +// vault.enc ciphertext — so callers may run it concurrently with a sync pull +// that replaces vault.enc, then decrypt with the returned password afterwards +// (see the concurrent-unlock path in cmd, #103). Returns ErrKeychainNotEnabled +// when keychain unlock is not configured. The caller owns the returned bytes and +// must crypto.ClearBytes them. +func (v *VaultService) RetrieveKeychainPassword() ([]byte, error) { // T018: Check metadata to see if keychain is enabled (FR-007) metadata, err := v.LoadMetadata() if err != nil { - return fmt.Errorf("failed to load metadata: %w", err) + return nil, fmt.Errorf("failed to load metadata: %w", err) } if !metadata.KeychainEnabled { - return ErrKeychainNotEnabled + return nil, ErrKeychainNotEnabled } // Attempt to retrieve password from vault-specific keychain entry @@ -954,10 +978,10 @@ func (v *VaultService) UnlockWithKeychain() error { } if err != nil { - return fmt.Errorf("failed to retrieve password from keychain: %w", err) + return nil, fmt.Errorf("failed to retrieve password from keychain: %w", err) } - return v.Unlock([]byte(password)) + return []byte(password), nil } // Lock clears in-memory credentials and password