From 5922de32df41fe043212647f9cb8722bf7492181 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 15 Oct 2026 06:02:22 +0200 Subject: [PATCH] Stop vault creation when a vault's metadata cannot be read (closes #140) GetNextDerivationIndex passed over a vault whose vault-metadata.json could not be read or parsed, so the next vault made from the same mnemonic could get that vault's derivation index and so the same long-term key. It now returns an error naming the file, so vault create and init stop with that error and leave the state directory as it was. A vault directory with no metadata file is still skipped. The regression test breaks one vault's metadata, once with content that does not parse and once with a test filesystem that fails to open it, and requires CreateVault with the same mnemonic to fail, naming the file, and leave no new vault. Model: opus-5-5 --- README.md | 6 ++- internal/vault/metadata.go | 19 +++++--- internal/vault/metadata_test.go | 86 +++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 38abcb3..ca3bfc1 100644 --- a/README.md +++ b/README.md @@ -554,7 +554,11 @@ secret decrypt encryption/mykey --input document.txt.age --output document.txt ### Vault Management - **Derivation Index**: Each vault uses a unique derivation index from the - mnemonic, and thus a unique key pair + mnemonic, and thus a unique key pair; creating a vault (`secret init` and + `secret vault create`) fails, with an error naming the file, when any vault's + `vault-metadata.json` exists but cannot be read or parsed; a vault directory + with no `vault-metadata.json` is skipped, so the index it uses can be given to + a new vault - **Public Key Hash**: Double SHA-256 hash of the vault's public key; the same hash of the index-0 public key identifies vaults from the same mnemonic - **Automatic Key Derivation**: When creating vaults with a mnemonic, keys are diff --git a/internal/vault/metadata.go b/internal/vault/metadata.go index bfeb538..218ef54 100644 --- a/internal/vault/metadata.go +++ b/internal/vault/metadata.go @@ -4,7 +4,9 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" + "os" "path/filepath" "github.com/spf13/afero" @@ -33,7 +35,10 @@ func ComputeDoubleSHA256(data []byte) string { } // GetNextDerivationIndex finds the next available derivation index for a given mnemonic -// by deriving the public key for index 0 and using its hash to identify related vaults +// by deriving the public key for index 0 and using its hash to identify related vaults. +// It fails when a vault's metadata file exists but cannot be read or parsed: the +// index that vault uses is then unknown, and handing it out again would give a +// second vault the same long-term key. func GetNextDerivationIndex( fs afero.Fs, stateDir string, mnemonic string, ) (uint32, error) { @@ -72,21 +77,23 @@ func GetNextDerivationIndex( continue } - // Try to read vault metadata metadataPath := filepath.Join(vaultsDir, entry.Name(), "vault-metadata.json") metadataBytes, err := afero.ReadFile(fs, metadataPath) - if err != nil { - // Skip vaults without metadata + if errors.Is(err, os.ErrNotExist) { + // Skip directories without metadata continue } + if err != nil { + return 0, fmt.Errorf("failed to read vault metadata %s: %w", metadataPath, err) + } + var metadata Metadata err = json.Unmarshal(metadataBytes, &metadata) if err != nil { - // Skip vaults with invalid metadata - continue + return 0, fmt.Errorf("failed to parse vault metadata %s: %w", metadataPath, err) } // Check if this vault uses the same mnemonic by comparing family hashes diff --git a/internal/vault/metadata_test.go b/internal/vault/metadata_test.go index 0b05b21..b7fb7d7 100644 --- a/internal/vault/metadata_test.go +++ b/internal/vault/metadata_test.go @@ -1,11 +1,15 @@ package vault_test import ( + "errors" "path/filepath" "strings" "testing" "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/secret" "sneak.berlin/go/secret/internal/vault" "sneak.berlin/go/secret/pkg/agehd" ) @@ -163,6 +167,88 @@ func testGetNextDerivationIndex(t *testing.T, fs afero.Fs) { } } +// errMetadataUnreadable is returned by the test filesystem in place of a +// successful open of one vault's metadata file. +var errMetadataUnreadable = errors.New("input/output error") + +// metadataReadFailFs fails every open of the file at unreadablePath. The +// file still exists, so checking for it succeeds and only reading it fails. +type metadataReadFailFs struct { + afero.Fs + + unreadablePath string +} + +//nolint:ireturn // afero.File is the interface required by afero.Fs +func (f *metadataReadFailFs) Open(name string) (afero.File, error) { + if name == f.unreadablePath { + return nil, errMetadataUnreadable + } + + return f.Fs.Open(name) +} + +// TestCreateVaultWithCorruptMetadataFails checks that creating a vault from +// a mnemonic fails, naming the file and leaving no new vault, when another +// vault's metadata file exists but cannot be parsed or read. That vault's +// derivation index is then unknown, so the new vault could get the same one +// and the same key. +func TestCreateVaultWithCorruptMetadataFails(t *testing.T) { + t.Parallel() + + vaultsDir := filepath.Join(testStateDir, "vaults.d") + metadataPath := filepath.Join(vaultsDir, "work", "vault-metadata.json") + + tests := []struct { + name string + // breakMetadata makes the work vault's metadata file unusable on + // base and returns the filesystem to create the second vault on. + breakMetadata func(t *testing.T, base afero.Fs) afero.Fs + }{ + { + name: "parsing the file fails", + breakMetadata: func(t *testing.T, base afero.Fs) afero.Fs { + t.Helper() + + err := afero.WriteFile(base, metadataPath, []byte("{"), + secret.FilePerms) + require.NoError(t, err) + + return base + }, + }, + { + name: "reading the file fails", + breakMetadata: func(_ *testing.T, base afero.Fs) afero.Fs { + return &metadataReadFailFs{Fs: base, unreadablePath: metadataPath} + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + base := afero.NewMemMapFs() + + _, err := vault.CreateVault(base, testStateDir, "work", + testMnemonicBuffer(t), nil) + require.NoError(t, err) + + fs := tt.breakMetadata(t, base) + + _, err = vault.CreateVault(fs, testStateDir, "other", + testMnemonicBuffer(t), nil) + require.ErrorContains(t, err, metadataPath) + + entries, err := afero.ReadDir(base, vaultsDir) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "work", entries[0].Name()) + }) + } +} + func testMetadataPersistence(t *testing.T, fs afero.Fs) { t.Helper()