diff --git a/TODO.md b/TODO.md index d659129..42302ae 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,14 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the + prompt, gives the error `wrong passphrase for unlocker ` + (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no + identity matched any of the recipients", followed by advice to give the vault + a new unlocker from its mnemonic, which would have replaced an unlocker that + works. The error now wraps `secret.ErrWrongPassphrase` and carries no advice + to use the mnemonic. A `priv.age` that is not an age file still gets that + advice. - 2026-10-14: The passphrase and mnemonic prompts ask on the controlling terminal (`/dev/tty`) instead of stdin, so `echo v | secret add s1 --force`, `cat file | secret encrypt key` and `cat file.age | secret decrypt key` ask diff --git a/internal/cli/unlock_failure_test.go b/internal/cli/unlock_failure_test.go index 7bc9574..6c3859b 100644 --- a/internal/cli/unlock_failure_test.go +++ b/internal/cli/unlock_failure_test.go @@ -1,12 +1,12 @@ // Unlock Failure Tests // // When a vault cannot be opened through its current unlocker, because a -// file the unlocker needs is missing or the passphrase is wrong, the error -// keeps its cause and ends by saying that the mnemonic still opens that -// vault, but only for a vault that the mnemonic does open, and not when the -// passphrase could not be read at all. When the current unlocker does not -// exist and the vault has another that `secret unlocker select` accepts, the -// error says to select one instead. +// file the unlocker needs is missing, the error keeps its cause and ends by +// saying that the mnemonic still opens that vault, but only for a vault that +// the mnemonic does open, and not when the passphrase could not be read at +// all. A wrong passphrase gives an error that says so, without that advice. +// When the current unlocker does not exist and the vault has another that +// `secret unlocker select` accepts, the error says to select one instead. // When a secret's current file is missing, the error says how to make a // version current again. Each test that pins such advice also follows it. @@ -21,7 +21,6 @@ import ( "syscall" "testing" - "filippo.io/age" "github.com/awnumar/memguard" "github.com/spf13/afero" "github.com/spf13/cobra" @@ -165,26 +164,48 @@ func TestUnlockFailureNamesMnemonic(t *testing.T) { } } -// TestWrongPassphraseNamesMnemonic checks the error of `secret get` given a -// passphrase that does not decrypt the passphrase unlocker: it keeps age's -// error and ends with the advice that the mnemonic still opens the vault. -func TestWrongPassphraseNamesMnemonic(t *testing.T) { +// TestWrongPassphraseNamesPassphrase checks the error of `secret get` given +// a passphrase that does not decrypt the passphrase unlocker: it says that +// the passphrase is wrong, without the advice to use the mnemonic, since the +// unlocker still opens the vault with the right passphrase. The test then +// gives the right passphrase. +func TestWrongPassphraseNamesPassphrase(t *testing.T) { t.Parallel() - _, c := newUnlockTestCLI(t, newTwoVaultFs(t)) + unlockerDir, c := newUnlockTestCLI(t, newTwoVaultFs(t)) c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte("wrong passphrase")) t.Cleanup(c.UnlockPassphrase.Destroy) err := c.GetSecret(discardCmd(), "x") - var noMatch *age.NoIdentityMatchError - - require.ErrorAs(t, err, &noMatch) - + require.ErrorIs(t, err, secret.ErrWrongPassphrase) + assert.NotContains(t, err.Error(), mnemonicAdvice) require.EqualError(t, err, "failed to unlock vault: "+ "failed to get long-term key: failed to get unlocker identity: "+ - "failed to decrypt unlocker private key: failed to create decryptor: "+ - noMatch.Error()+mnemonicAdvice) + "wrong passphrase for unlocker "+filepath.Base(unlockerDir)) + + c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte(testPassphrase)) + t.Cleanup(c.UnlockPassphrase.Destroy) + assert.Equal(t, "value", getSecret(t, c, "x")) +} + +// TestDamagedUnlockerNamesMnemonic checks the error of `secret get` when the +// passphrase unlocker's priv.age is not an age file: the passphrase is not +// blamed, and the error ends with the advice that the mnemonic still opens +// the vault. +func TestDamagedUnlockerNamesMnemonic(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + unlockerDir, c := newUnlockTestCLI(t, fs) + + require.NoError(t, afero.WriteFile(fs, filepath.Join(unlockerDir, "priv.age"), + []byte("damaged"), secret.FilePerms)) + + err := c.GetSecret(discardCmd(), "x") + + require.NotErrorIs(t, err, secret.ErrWrongPassphrase) + require.ErrorContains(t, err, mnemonicAdvice) } // TestMoveUnlockFailureNamesVault checks the error of `secret move` into diff --git a/internal/secret/passphraseunlocker.go b/internal/secret/passphraseunlocker.go index 6d38520..91d47b3 100644 --- a/internal/secret/passphraseunlocker.go +++ b/internal/secret/passphraseunlocker.go @@ -16,6 +16,10 @@ import ( // empty. A passphrase unlocker that fails with it was not tried. var ErrPassphraseNotRead = errors.New("failed to read passphrase") +// ErrWrongPassphrase is wrapped in the error of GetIdentity when the +// passphrase does not decrypt the unlocker's private key. +var ErrWrongPassphrase = errors.New("wrong passphrase") + // PassphraseUnlocker represents a passphrase-protected unlocker type PassphraseUnlocker struct { Directory string @@ -73,6 +77,12 @@ func (p *PassphraseUnlocker) GetIdentity() (*age.X25519Identity, error) { Debug("Failed to decrypt unlocker private key", "error", err, "unlocker_id", p.GetID()) + // age says that no identity matched when the passphrase is wrong + var noMatch *age.NoIdentityMatchError + if errors.As(err, &noMatch) { + return nil, fmt.Errorf("%w for unlocker %s", ErrWrongPassphrase, p.GetID()) + } + return nil, fmt.Errorf("failed to decrypt unlocker private key: %w", err) } defer privKeyBuffer.Destroy() diff --git a/internal/vault/vault.go b/internal/vault/vault.go index d289bef..f43614a 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -291,10 +291,13 @@ func (v *Vault) unlockLongTermKey( // without a mnemonic records none, and without its metadata the key cannot // be derived. It is not added when the passphrase could not be read: the // unlocker was not tried, and adding one would need a passphrase read the -// same way. Nor is it added when the current unlocker does not exist and the -// vault has others: the error already says to select one of them. +// same way. Nor is it added when the passphrase is wrong: the unlocker still +// opens the vault with the right one. Nor is it added when the current +// unlocker does not exist and the vault has others: the error already says +// to select one of them. func (v *Vault) withMnemonicAdvice(err error) error { if errors.Is(err, secret.ErrPassphraseNotRead) || + errors.Is(err, secret.ErrWrongPassphrase) || errors.Is(err, ErrUnlockerNotFound) { return err } diff --git a/internal/vault/vault_error_test.go b/internal/vault/vault_error_test.go index b28378a..5b0f9ef 100644 --- a/internal/vault/vault_error_test.go +++ b/internal/vault/vault_error_test.go @@ -5,7 +5,6 @@ import ( "path/filepath" "testing" - "filippo.io/age" "github.com/awnumar/memguard" "github.com/spf13/afero" "github.com/stretchr/testify/assert" @@ -140,9 +139,7 @@ func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) { err = vlt.AddSecret(testSecretName, second, true) - var noMatch *age.NoIdentityMatchError - - require.ErrorAs(t, err, &noMatch) + require.ErrorIs(t, err, secret.ErrWrongPassphrase) assert.Equal(t, before, dirContents(t, fs, secretDir)) }