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..12f38b0 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -219,6 +219,15 @@ func (v *Vault) addVersion( name, secretDir string, value *memguard.LockedBuffer, previousVersion *secret.Version, ) error { + // 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 + } + + // Taken after the unlock, which can wait for the user to type the + // passphrase, so that the new version starts when it is created now := time.Now() // Create the new version and save the encrypted value @@ -632,8 +641,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 +677,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/secrets_version_test.go b/internal/vault/secrets_version_test.go index 7fcded0..81b64f0 100644 --- a/internal/vault/secrets_version_test.go +++ b/internal/vault/secrets_version_test.go @@ -10,6 +10,8 @@ // version // - TestVaultVersionTimestamps: Tests timestamp logic (notBefore/notAfter) // across versions +// - TestVaultVersionTimestampsAfterUnlock: Tests that those timestamps are +// taken after AddSecret unlocks the vault // - TestVaultGetNonExistentVersion: Tests error handling for invalid versions // - TestUpdateVersionMetadata: Tests metadata update functionality // @@ -295,6 +297,70 @@ func TestVaultVersionTimestamps(t *testing.T) { assert.Nil(t, secondVersion.Metadata.NotAfter) // Current version } +// longTermKeyReadFs records when longterm.age, the last file an unlock +// reads, was last opened. +type longTermKeyReadFs struct { + afero.Fs + + readAt time.Time +} + +//nolint:ireturn // afero.File is the interface required by afero.Fs +func (f *longTermKeyReadFs) Open(name string) (afero.File, error) { + if filepath.Base(name) == "longterm.age" { + f.readAt = time.Now() + } + + return f.Fs.Open(name) +} + +// TestVaultVersionTimestampsAfterUnlock checks that when AddSecret has to +// unlock the vault to replace a version, the new version's notBefore and the +// replaced version's notAfter are taken after the unlock, which can wait for +// the user to type the passphrase. +func TestVaultVersionTimestampsAfterUnlock(t *testing.T) { + t.Parallel() + + fs := &longTermKeyReadFs{Fs: afero.NewMemMapFs()} + + mnemonic := memguard.NewBufferFromBytes([]byte(testMnemonic)) + defer mnemonic.Destroy() + + passphrase := memguard.NewBufferFromBytes([]byte("test-passphrase")) + defer passphrase.Destroy() + + created, err := CreateVault(fs, testStateDir, "test", mnemonic, passphrase) + require.NoError(t, err) + + addTestSecretToVault(t, created, testSecretPath, []byte("version-1"), false) + + // A new instance is locked; AddSecret unlocks it with the passphrase + vault := NewVault(fs, testStateDir, "test") + vault.SetUnlockPassphrase(passphrase) + + addTestSecretToVault(t, vault, testSecretPath, []byte("version-2"), true) + require.False(t, fs.readAt.IsZero(), "AddSecret did not unlock the vault") + + ltIdentity, err := vault.GetOrDeriveLongTermKey() + require.NoError(t, err) + + vaultDir, _ := vault.GetDirectory() + secretDir := filepath.Join(vaultDir, "secrets.d", "test%secret") + + versions, err := secret.ListVersions(fs, secretDir) + require.NoError(t, err) + require.Len(t, versions, 2) + + newVersion := secret.NewVersion(vault, testSecretPath, versions[0]) + require.NoError(t, newVersion.LoadMetadata(ltIdentity)) + + replacedVersion := secret.NewVersion(vault, testSecretPath, versions[1]) + require.NoError(t, replacedVersion.LoadMetadata(ltIdentity)) + + assert.False(t, newVersion.Metadata.NotBefore.Before(fs.readAt)) + assert.False(t, replacedVersion.Metadata.NotAfter.Before(fs.readAt)) +} + func TestVaultGetNonExistentVersion(t *testing.T) { t.Parallel() 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 +}