diff --git a/README.md b/README.md index 4651c36..5910d35 100644 --- a/README.md +++ b/README.md @@ -517,7 +517,9 @@ 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 or importing a mnemonic + fails, with an error naming the file, when any vault's `vault-metadata.json` + cannot be read or parsed, as that vault's index is then unknown - **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/TODO.md b/TODO.md index 857a05f..c048b36 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,13 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: Creating a vault, or importing a mnemonic into one, fails with an + error naming the file when any vault's `vault-metadata.json` exists but cannot + be read or parsed, and writes nothing + (https://git.eeqj.de/sneak/secret/issues/140). `vault.GetNextDerivationIndex` + passed over such a vault, so the next vault made from the same mnemonic could + be given its derivation index, and so its long-term key. A vault directory + with no `vault-metadata.json` is still passed over. - 2026-10-14: `secret generate mnemonic`, `secret vault list` and `secret unlocker list` print their output to stdout, as `secret list` does, so it can be piped or captured (https://git.eeqj.de/sneak/secret/issues/138). 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 f3d6206..3291dd7 100644 --- a/internal/vault/metadata_test.go +++ b/internal/vault/metadata_test.go @@ -6,6 +6,9 @@ import ( "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 +166,33 @@ func testGetNextDerivationIndex(t *testing.T, fs afero.Fs) { } } +// TestCreateVaultWithCorruptMetadataFails checks that creating a vault from +// a mnemonic fails, naming the file and writing nothing, when another +// vault's metadata cannot be parsed. 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() + + fs := afero.NewMemMapFs() + + _, err := vault.CreateVault(fs, testStateDir, "work", testMnemonicBuffer(t), nil) + require.NoError(t, err) + + vaultsDir := filepath.Join(testStateDir, "vaults.d") + metadataPath := filepath.Join(vaultsDir, "work", "vault-metadata.json") + + err = afero.WriteFile(fs, metadataPath, []byte("{"), secret.FilePerms) + require.NoError(t, err) + + _, err = vault.CreateVault(fs, testStateDir, "other", testMnemonicBuffer(t), nil) + require.ErrorContains(t, err, metadataPath) + + entries, err := afero.ReadDir(fs, 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()