diff --git a/TODO.md b/TODO.md index e09672e..2e15fe9 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,21 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-04: When the vault cannot be opened through its current unlocker, + because a file the unlocker needs is missing or damaged, its keychain item + or Secure Enclave key is gone, or the passphrase is wrong, the error now + ends by saying that the vault still opens with its mnemonic, and that + `secret unlocker add passphrase`, run with `SB_SECRET_MNEMONIC` set to it, + gives the vault a new unlocker + (https://git.eeqj.de/sneak/secret/issues/47). Before, it ended with the + bare cause. The advice is given only when the vault metadata records the + key the mnemonic derives, so not for a vault created without a mnemonic. + `secret vault import` is not named: it refuses a vault that has a + long-term key. `secret encrypt` and `secret decrypt` now read the key + secret through `vault.GetSecret`, as `secret get` does, so they give the + same advice. When a secret's `current` file cannot be read, the error says + that `secret version list` lists its versions and `secret version promote` + makes one current. The causes stay wrapped. - 2026-10-04: README's Storage Architecture, `secret version promote`, Technical Details and Testing text matches the code (https://git.eeqj.de/sneak/secret/issues/102). `current` and @@ -388,8 +403,6 @@ https://git.eeqj.de/sneak/secret/milestone/12 (`secret.IdentityToLockedBuffer` overwrites only the string itself). - Medium priority: - Standardize error messages; stop leaking internals. - - Graceful handling of corrupted or missing key files with recovery - suggestions. - Split oversized CLI functions. - Cleanups: read statedir from environment or default instead of passing it around. diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index d6da3d6..b420ac3 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -130,7 +130,7 @@ func (cli *Instance) resolveEncryptionKey( } // Secret exists, get the age secret key from it - secretBuffer, err := cli.getSecretValue(vlt, secretObj) + secretBuffer, err := vlt.GetSecret(secretName) if err != nil { return nil, fmt.Errorf("failed to get secret value: %w", err) } @@ -249,7 +249,7 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { } // Get the age secret key from the secret - secretBuffer, err := cli.getSecretValue(vlt, secretObj) + secretBuffer, err := vlt.GetSecret(secretName) if err != nil { return fmt.Errorf("failed to get secret value: %w", err) } @@ -313,20 +313,3 @@ func isValidAgeSecretKey(key string) bool { return err == nil } - -// getSecretValue retrieves the value of a secret with the vault's mnemonic -// when it has one, else with the current unlocker -func (cli *Instance) getSecretValue( - vlt *vault.Vault, secretObj *secret.Secret, -) (*memguard.LockedBuffer, error) { - if vlt.Mnemonic != nil { - return secretObj.GetValue(nil, vlt.Mnemonic) - } - - unlocker, err := vlt.GetCurrentUnlocker() - if err != nil { - return nil, fmt.Errorf("failed to get current unlocker: %w", err) - } - - return secretObj.GetValue(unlocker, nil) -} diff --git a/internal/cli/unlock_failure_test.go b/internal/cli/unlock_failure_test.go new file mode 100644 index 0000000..c7eb331 --- /dev/null +++ b/internal/cli/unlock_failure_test.go @@ -0,0 +1,282 @@ +// Unlock Failure Tests +// +// When the 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 the +// vault, but only for a vault that the mnemonic does open. 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. + +package cli_test + +import ( + "bytes" + "io" + "os" + "path/filepath" + "testing" + + "filippo.io/age" + "git.eeqj.de/sneak/secret/internal/cli" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/awnumar/memguard" + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // mnemonicAdvice ends the error when a vault that its mnemonic opens + // cannot be opened through its current unlocker. + mnemonicAdvice = "; the vault still opens with its mnemonic: run " + + "'secret unlocker add passphrase' with SB_SECRET_MNEMONIC set to " + + "the mnemonic to give it a new unlocker" + + // versionAdvice ends the error when a secret's current file cannot be + // read. + versionAdvice = "; this file only names the current version: " + + "'secret version list' lists the secret's versions, and " + + "'secret version promote' makes one of them current" + + // unlockTestVaultDir is the directory of the vault "default" of + // newTwoVaultFs, the current vault, whose secret "x" is "value". + unlockTestVaultDir = testStateDir + "/vaults.d/default" +) + +// newUnlockTestCLI returns the directory of the current unlocker of the +// vault "default" on fs, a copy of the vaults of newTwoVaultFs, and a CLI +// instance on fs that has the unlock passphrase, as from the environment, +// but not the mnemonic. +func newUnlockTestCLI(t *testing.T, fs afero.Fs) (string, *cli.Instance) { + t.Helper() + + unlockerName, err := afero.ReadFile(fs, + filepath.Join(unlockTestVaultDir, "current-unlocker")) + require.NoError(t, err) + + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte(testPassphrase)) + t.Cleanup(c.UnlockPassphrase.Destroy) + + return filepath.Join(unlockTestVaultDir, "unlockers.d", string(unlockerName)), c +} + +// discardCmd returns a command whose output is discarded. +func discardCmd() *cobra.Command { + cmd := &cobra.Command{} + cmd.SetOut(io.Discard) + + return cmd +} + +// getSecret returns what `secret get name` prints. +func getSecret(t *testing.T, c *cli.Instance, name string) string { + t.Helper() + + var out bytes.Buffer + + cmd := &cobra.Command{} + cmd.SetOut(&out) + require.NoError(t, c.GetSecret(cmd, name)) + + return out.String() +} + +// TestUnlockFailureNamesMnemonic checks the error of `secret get` when a +// file that opening the vault through its current unlocker needs is +// missing: it keeps the cause, which names the file, and ends with the +// advice that the mnemonic still opens the vault. The test then follows +// that advice: `secret unlocker add passphrase`, with the mnemonic, gives +// the vault a new unlocker, which opens it. +func TestUnlockFailureNamesMnemonic(t *testing.T) { + t.Parallel() + + tests := []struct { + file string // the file removed + inVaultDir bool // the file is the vault's, not the unlocker's + want string // the message before the cause + }{ + { + file: "current-unlocker", + inVaultDir: true, + want: "failed to unlock vault: failed to get long-term key: " + + "failed to get current unlocker: " + + "failed to read current unlocker: ", + }, + { + file: "priv.age", + want: "failed to unlock vault: failed to get long-term key: " + + "failed to get unlocker identity: " + + "failed to read unlocker private key: ", + }, + { + file: "longterm.age", + want: "failed to unlock vault: failed to get long-term key: " + + "failed to read encrypted long-term private key: ", + }, + } + + for _, tt := range tests { + t.Run(tt.file, func(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + unlockerDir, c := newUnlockTestCLI(t, fs) + + path := filepath.Join(unlockerDir, tt.file) + + if tt.inVaultDir { + path = filepath.Join(unlockTestVaultDir, tt.file) + } + + require.NoError(t, fs.Remove(path)) + + err := c.GetSecret(discardCmd(), "x") + + var cause *os.PathError + + require.ErrorAs(t, err, &cause) + require.ErrorIs(t, err, os.ErrNotExist) + assert.Equal(t, path, cause.Path) + + require.EqualError(t, err, tt.want+cause.Error()+mnemonicAdvice) + + c.Mnemonic = testMnemonicBuffer(t) + require.NoError(t, c.UnlockersAdd("passphrase", discardCmd())) + + c.Mnemonic = nil + assert.Equal(t, "value", getSecret(t, c, "x")) + }) + } +} + +// 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) { + t.Parallel() + + _, 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.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) +} + +// TestCryptoUnlockFailureNamesMnemonic checks that `secret encrypt` and +// `secret decrypt`, reading the key secret, end with the same advice as +// `secret get` when the vault cannot be opened through its current +// unlocker. +func TestCryptoUnlockFailureNamesMnemonic(t *testing.T) { + t.Parallel() + + tests := []struct { + command string + run func(c *cli.Instance) error + }{ + {"encrypt", func(c *cli.Instance) error { return c.Encrypt("x", "", "") }}, + {"decrypt", func(c *cli.Instance) error { return c.Decrypt("x", "", "") }}, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + unlockerDir, c := newUnlockTestCLI(t, fs) + + path := filepath.Join(unlockerDir, "priv.age") + require.NoError(t, fs.Remove(path)) + + err := tt.run(c) + + var cause *os.PathError + + require.ErrorAs(t, err, &cause) + assert.Equal(t, path, cause.Path) + + require.EqualError(t, err, "failed to get secret value: "+ + "failed to unlock vault: failed to get long-term key: "+ + "failed to get unlocker identity: "+ + "failed to read unlocker private key: "+cause.Error()+ + mnemonicAdvice) + }) + } +} + +// TestMissingCurrentFileNamesVersionCommands checks the error of `secret +// get` when the secret's current file is missing: it keeps the cause, which +// names the file, and ends with the advice that says how to make a version +// current again. The test then follows that advice. +func TestMissingCurrentFileNamesVersionCommands(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + _, c := newUnlockTestCLI(t, fs) + + secretDir := filepath.Join(unlockTestVaultDir, "secrets.d", "x") + path := filepath.Join(secretDir, "current") + require.NoError(t, fs.Remove(path)) + + err := c.GetSecret(discardCmd(), "x") + + var cause *os.PathError + + require.ErrorAs(t, err, &cause) + require.ErrorIs(t, err, os.ErrNotExist) + assert.Equal(t, path, cause.Path) + + require.EqualError(t, err, "failed to get current version: "+ + "failed to read current version file: "+cause.Error()+versionAdvice) + + versions, err := afero.ReadDir(fs, filepath.Join(secretDir, "versions")) + require.NoError(t, err) + require.Len(t, versions, 1) + + var out bytes.Buffer + + cmd := &cobra.Command{} + cmd.SetOut(&out) + require.NoError(t, c.ListVersions(cmd, "x")) + assert.Contains(t, out.String(), versions[0].Name()) + + require.NoError(t, c.PromoteVersion(cmd, "x", versions[0].Name())) + assert.Equal(t, "value", getSecret(t, c, "x")) +} + +// TestUnlockFailureWithoutLongTermKeyNamesNoMnemonic checks that a vault +// created without a mnemonic, which no mnemonic opens, gets no advice to +// use one: `secret unlocker add passphrase` there fails with the cause +// alone. +func TestUnlockFailureWithoutLongTermKeyNamesNoMnemonic(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + + _, err := vault.CreateVault(fs, testStateDir, "keyless", nil, nil) + require.NoError(t, err) + + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte(testPassphrase)) + t.Cleanup(c.UnlockPassphrase.Destroy) + + err = c.UnlockersAdd("passphrase", discardCmd()) + + var cause *os.PathError + + require.ErrorAs(t, err, &cause) + + require.EqualError(t, err, "failed to get long-term key: "+ + "failed to get current unlocker: failed to read current unlocker: "+ + cause.Error()) +} diff --git a/internal/secret/version.go b/internal/secret/version.go index 17cca2b..fe9fd0a 100644 --- a/internal/secret/version.go +++ b/internal/secret/version.go @@ -557,13 +557,18 @@ func VersionExists(fs afero.Fs, secretDir string, version string) (bool, error) } // GetCurrentVersion returns the version that the "current" file points to -// The file contains just the version name (e.g., "20231215.001") +// The file contains just the version name (e.g., "20231215.001"). If it +// cannot be read, the error says how to make a version current again: the +// versions themselves are not in the file. func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) { currentPath := filepath.Join(secretDir, "current") fileData, err := afero.ReadFile(fs, currentPath) if err != nil { - return "", fmt.Errorf("failed to read current version file: %w", err) + return "", fmt.Errorf("failed to read current version file: %w; "+ + "this file only names the current version: 'secret version list' "+ + "lists the secret's versions, and 'secret version promote' makes "+ + "one of them current", err) } version := strings.TrimSpace(string(fileData)) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index a4f3fe1..84bab08 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -98,7 +98,8 @@ func (v *Vault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) { if err != nil { secret.Debug("Failed to get current unlocker", "error", err, "vault_name", v.Name) - return nil, fmt.Errorf("failed to get current unlocker: %w", err) + return nil, v.withMnemonicAdvice( + fmt.Errorf("failed to get current unlocker: %w", err)) } secret.DebugWith("Retrieved current unlocker for vault unlock", @@ -112,7 +113,7 @@ func (v *Vault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) { // Other unlockers return their own identity, used to decrypt longterm.age. ltIdentity, err := v.unlockLongTermKey(unlocker) if err != nil { - return nil, err + return nil, v.withMnemonicAdvice(err) } secret.DebugWith("Successfully obtained long-term identity via unlocker", @@ -295,3 +296,22 @@ func (v *Vault) unlockLongTermKey( return ltIdentity, nil } + +// withMnemonicAdvice returns err, a failure to get the long-term key through +// the current unlocker, with advice added: that the mnemonic still opens the +// vault, and how to give it a new unlocker. The advice is added only when the +// vault metadata records the key that the mnemonic derives; a vault created +// without a mnemonic records none, and without its metadata the key cannot +// be derived. +func (v *Vault) withMnemonicAdvice(err error) error { + vaultDir, _ := v.GetDirectory() + + metadata, metadataErr := LoadVaultMetadata(v.fs, vaultDir) + if metadataErr != nil || metadata.PublicKeyHash == "" { + return err + } + + return fmt.Errorf("%w; the vault still opens with its mnemonic: run "+ + "'secret unlocker add passphrase' with %s set to the mnemonic "+ + "to give it a new unlocker", err, secret.EnvMnemonic) +}