diff --git a/TODO.md b/TODO.md index 54fcc47..cfc97da 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 `secret add --force` that cannot unlock the vault, given a wrong + passphrase or mnemonic, leaves the secret as it was + (https://git.eeqj.de/sneak/secret/issues/136). The new version used to be + written before the vault was unlocked to set `notAfter` on the version it + replaces, so each failed add left one more version that `secret version list` + showed. The vault is now unlocked, and that version's metadata read, before + anything is written. `generate secret --force` and `import --force` take the + same path. - 2026-10-12: A mnemonic typed with extra spaces, tabs or line breaks gives the same long-term key as its words separated by single spaces (https://git.eeqj.de/sneak/secret/issues/135). The key was derived from the diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index 6da0ef1..30940e0 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -221,6 +221,13 @@ func (v *Vault) addVersion( ) error { now := time.Now() + // Unlock the vault before writing anything, so that a wrong passphrase + // or mnemonic leaves the secret as it was + err := v.loadPreviousVersionMetadata(previousVersion) + if err != nil { + return err + } + // Create the new version and save the encrypted value versionName, err := v.createAndSaveVersion( name, secretDir, value, previousVersion, &now) @@ -632,8 +639,35 @@ func (v *Vault) checkExistingSecret( return true, previousVersion, nil } +// loadPreviousVersionMetadata unlocks the vault and loads the metadata of +// the version being superseded. It is a no-op when previousVersion is nil. +func (v *Vault) loadPreviousVersionMetadata( + previousVersion *secret.Version, +) error { + if previousVersion == nil { + return nil + } + + ltIdentity, err := v.GetOrDeriveLongTermKey() + if err != nil { + secret.Debug("Failed to get long-term key for previous version", "error", err) + + return fmt.Errorf("failed to get long-term key: %w", err) + } + + err = previousVersion.LoadMetadata(ltIdentity) + if err != nil { + secret.Debug("Failed to load previous version metadata", "error", err) + + return fmt.Errorf("failed to load previous version metadata: %w", err) + } + + return nil +} + // updatePreviousVersion sets the notAfter timestamp on the version being -// superseded. It is a no-op when previousVersion is nil. +// superseded, whose metadata loadPreviousVersionMetadata has loaded. It is +// a no-op when previousVersion is nil. func (v *Vault) updatePreviousVersion( previousVersion *secret.Version, now *time.Time, ) error { @@ -641,22 +675,13 @@ func (v *Vault) updatePreviousVersion( return nil } - // Get long-term key to decrypt/encrypt metadata + // loadPreviousVersionMetadata unlocked the vault, so this returns the + // key it keeps in memory ltIdentity, err := v.GetOrDeriveLongTermKey() if err != nil { - secret.Debug("Failed to get long-term key for metadata update", "error", err) - return fmt.Errorf("failed to get long-term key: %w", err) } - // Load previous version metadata - err = previousVersion.LoadMetadata(ltIdentity) - if err != nil { - secret.Debug("Failed to load previous version metadata", "error", err) - - return fmt.Errorf("failed to load previous version metadata: %w", err) - } - // Update notAfter timestamp previousVersion.Metadata.NotAfter = now diff --git a/internal/vault/vault_error_test.go b/internal/vault/vault_error_test.go index 7cec289..b28378a 100644 --- a/internal/vault/vault_error_test.go +++ b/internal/vault/vault_error_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "filippo.io/age" "github.com/awnumar/memguard" "github.com/spf13/afero" "github.com/stretchr/testify/assert" @@ -103,3 +104,78 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) { require.Len(t, entries, 1) assert.Equal(t, "pub.age", entries[0].Name()) } + +// TestAddSecretForceWithWrongPassphraseChangesNothing checks that adding a +// version to an existing secret, with a passphrase that does not open the +// vault, fails and leaves the secret's directory as it was. +func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + + passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase)) + t.Cleanup(passphrase.Destroy) + + created, err := vault.CreateVault(fs, testStateDir, testVaultName, + testMnemonicBuffer(t), passphrase) + require.NoError(t, err) + + first := memguard.NewBufferFromBytes([]byte("first")) + t.Cleanup(first.Destroy) + + require.NoError(t, created.AddSecret(testSecretName, first, false)) + + wrong := memguard.NewBufferFromBytes([]byte("wrong passphrase")) + t.Cleanup(wrong.Destroy) + + vlt := vault.NewVault(fs, testStateDir, testVaultName) + vlt.SetUnlockPassphrase(wrong) + + secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName, + "secrets.d", testSecretName) + before := dirContents(t, fs, secretDir) + + second := memguard.NewBufferFromBytes([]byte("second")) + t.Cleanup(second.Destroy) + + err = vlt.AddSecret(testSecretName, second, true) + + var noMatch *age.NoIdentityMatchError + + require.ErrorAs(t, err, &noMatch) + assert.Equal(t, before, dirContents(t, fs, secretDir)) +} + +// dirContents maps every file under dir to its contents, and every +// directory, written with a trailing "/", to "". +func dirContents(t *testing.T, fs afero.Fs, dir string) map[string]string { + t.Helper() + + contents := map[string]string{} + + err := afero.Walk(fs, dir, func( + path string, info os.FileInfo, err error, + ) error { + if err != nil { + return err + } + + if info.IsDir() { + contents[path+"/"] = "" + + return nil + } + + content, err := afero.ReadFile(fs, path) + if err != nil { + return err + } + + contents[path] = string(content) + + return nil + }) + require.NoError(t, err) + + return contents +}