diff --git a/README.md b/README.md index 40ee8df..46d6d88 100644 --- a/README.md +++ b/README.md @@ -211,7 +211,9 @@ Generates and stores a random secret. #### `secret unlocker list [--json]` / `secret unlocker ls` -Lists all unlockers in the current vault with their metadata. +Lists all unlockers in the current vault with their metadata. An unlocker's ID, +which `secret unlocker select` and `secret unlocker remove` take, is the name of +its directory in `unlockers.d`. #### `secret unlocker add [options]` diff --git a/TODO.md b/TODO.md index e09672e..ca7c3d7 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,24 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-04: An unlocker's ID is the name of its directory in `unlockers.d`, + so no two unlockers of a vault share one + (https://git.eeqj.de/sneak/secret/issues/98). Before, a keychain or Secure + Enclave unlocker's ID was its creation time to the minute and the host name, + and a passphrase unlocker's the time to the minute, so two created within a + minute shared an ID, and `unlocker select`, `unlocker remove` and the + selection `unlocker add` makes acted on the older one. A PGP unlocker's ID + was `pgp-` and its key's fingerprint; a second PGP unlocker for a key is + still refused, now by comparing the fingerprint in the other unlockers' + metadata. `unlocker list` and the shell completion of `unlocker select` and + `unlocker remove` take each ID from the directory the unlocker was read + from, no longer by matching metadata, so two unlockers with the same + metadata are listed apart; an unlocker of an unknown type is listed under + its directory name, and completion now offers Secure Enclave unlockers too. + The keychain and Secure Enclave code was type-checked by + `script/lint-darwin`, never run; a test on Linux lists, completes, selects + and removes each of two passphrase unlockers with the same metadata by its + own ID. - 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 diff --git a/internal/cli/completions.go b/internal/cli/completions.go index e1ecb8c..ac4eb3c 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -1,10 +1,10 @@ package cli import ( - "path/filepath" + "maps" + "slices" "strings" - "git.eeqj.de/sneak/secret/internal/secret" "git.eeqj.de/sneak/secret/internal/vault" "github.com/spf13/afero" "github.com/spf13/cobra" @@ -44,7 +44,7 @@ func getSecretNamesCompletionFunc(fs afero.Fs, stateDir string) func( } // getUnlockerIDsCompletionFunc returns a completion function that provides -// unlocker IDs +// unlocker IDs, the names of the unlockers' directories in unlockers.d func getUnlockerIDsCompletionFunc(fs afero.Fs, stateDir string) func( cmd *cobra.Command, args []string, toComplete string, ) ([]string, cobra.ShellCompDirective) { @@ -57,38 +57,15 @@ func getUnlockerIDsCompletionFunc(fs afero.Fs, stateDir string) func( return nil, cobra.ShellCompDirectiveNoFileComp } - // Get unlocker metadata list - unlockerMetadataList, err := vlt.ListUnlockers() + unlockerMetadata, err := vlt.ListUnlockers() if err != nil { return nil, cobra.ShellCompDirectiveNoFileComp } - // Get vault directory - vaultDir, err := vlt.GetDirectory() - if err != nil { - return nil, cobra.ShellCompDirectiveNoFileComp - } - - // Collect unlocker IDs var completions []string - unlockersDir := filepath.Join(vaultDir, "unlockers.d") - - for _, metadata := range unlockerMetadataList { - // Get the actual unlocker ID by creating the unlocker instance - id, err := findUnlockerIDByMetadata( - fs, unlockersDir, metadata, false, - ) - if err != nil { - secret.Warn( - "Could not read unlockers directory during completion, "+ - "skipping unlocker", - "unlockers_dir", unlockersDir, "error", err) - - continue - } - - if id != "" && strings.HasPrefix(id, toComplete) { + for _, id := range slices.Sorted(maps.Keys(unlockerMetadata)) { + if strings.HasPrefix(id, toComplete) { completions = append(completions, id) } } diff --git a/internal/cli/confirm_test.go b/internal/cli/confirm_test.go index 9385580..dc86541 100644 --- a/internal/cli/confirm_test.go +++ b/internal/cli/confirm_test.go @@ -101,7 +101,8 @@ func newRemoval(t *testing.T, command string) removal { } fs, workDir, older := newConfirmTestVaults(t, unlockers) - unlockerID := "pgp-" + listTestGPGKeyID + "A" + // The first unlocker's directory name, written by newConfirmTestVaults + unlockerID := "pgp-0" removeFirstUnlocker := func(cli *Instance, cmd *cobra.Command, force bool) error { return cli.UnlockersRemove(unlockerID, force, cmd) @@ -142,7 +143,7 @@ func newRemoval(t *testing.T, command string) removal { return removal{ fs: fs, run: removeFirstUnlocker, - removed: filepath.Join(workDir, "unlockers.d", "pgp-0"), + removed: filepath.Join(workDir, "unlockers.d", unlockerID), question: "Permanently remove unlocker '" + unlockerID + "' from vault 'work'? It is not the vault's last unlocker.", } @@ -150,7 +151,7 @@ func newRemoval(t *testing.T, command string) removal { return removal{ fs: fs, run: removeFirstUnlocker, - removed: filepath.Join(workDir, "unlockers.d", "pgp-0"), + removed: filepath.Join(workDir, "unlockers.d", unlockerID), question: "Permanently remove unlocker '" + unlockerID + "', the last unlocker of vault 'work', which holds 1 " + "secret(s)? Without an unlocker the vault opens only " + diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index bcb417c..1c07dd5 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log" + "maps" "os" "os/exec" "path/filepath" @@ -313,91 +314,8 @@ func newUnlockerSelectCmd() *cobra.Command { } } -// unlockerIDFromDir constructs an unlocker of the given metadata type -// rooted at unlockerDir and returns its ID. Returns "" for unknown types -// and, when includeSecureEnclave is false, for secure enclave unlockers. -func unlockerIDFromDir( - fs afero.Fs, unlockerDir string, metadata secret.UnlockerMetadata, - includeSecureEnclave bool, -) string { - // Create the appropriate unlocker instance - var unlocker secret.Unlocker - - switch metadata.Type { - case unlockerTypePassphrase: - unlocker = secret.NewPassphraseUnlocker(fs, unlockerDir, metadata) - case unlockerTypeKeychain: - unlocker = secret.NewKeychainUnlocker(fs, unlockerDir, metadata) - case unlockerTypePGP: - unlocker = secret.NewPGPUnlocker(fs, unlockerDir, metadata) - case unlockerTypeSecureEnclave: - if includeSecureEnclave { - unlocker = secret.NewSecureEnclaveUnlocker(fs, unlockerDir, metadata) - } - } - - if unlocker == nil { - return "" - } - - return unlocker.GetID() -} - -// findUnlockerIDByMetadata scans unlockersDir for the directory whose -// stored metadata matches the given type and creation time and returns -// the matching unlocker's ID. It returns ("", nil) when the directory is -// readable but holds no match, and a non-nil error when the directory -// itself cannot be read. Callers must distinguish the two: an unreadable -// directory means the unlocker's real ID is unknowable, so the entry has -// to be skipped rather than reported under a synthesized ID. -// -// A metadata file that cannot be read or parsed is skipped without a -// warning: every caller gets metadata from vault.ListUnlockers first, -// which has already warned about that directory. -func findUnlockerIDByMetadata( - fs afero.Fs, unlockersDir string, metadata secret.UnlockerMetadata, - includeSecureEnclave bool, -) (string, error) { - files, err := afero.ReadDir(fs, unlockersDir) - if err != nil { - return "", fmt.Errorf( - "failed to read unlockers directory %s: %w", unlockersDir, err, - ) - } - - for _, file := range files { - if !file.IsDir() { - continue - } - - unlockerDir := filepath.Join(unlockersDir, file.Name()) - metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json") - - // Check if this is the right unlocker by comparing metadata - metadataBytes, err := afero.ReadFile(fs, metadataPath) - if err != nil { - continue - } - - var diskMetadata secret.UnlockerMetadata - - err = json.Unmarshal(metadataBytes, &diskMetadata) - if err != nil { - continue - } - - // Match by type and creation time - if diskMetadata.Type == metadata.Type && - diskMetadata.CreatedAt.Equal(metadata.CreatedAt) { - return unlockerIDFromDir(fs, unlockerDir, diskMetadata, - includeSecureEnclave), nil - } - } - - return "", nil -} - -// UnlockersList lists unlockers in the current vault +// UnlockersList lists unlockers in the current vault, each under its ID, +// the name of its directory in unlockers.d func (cli *Instance) UnlockersList(jsonOutput bool) error { // Get current vault vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) @@ -413,58 +331,23 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error { currentUnlockerID = currentUnlocker.GetID() } - // Get the metadata first - unlockerMetadataList, err := vlt.ListUnlockers() + unlockerMetadata, err := vlt.ListUnlockers() if err != nil { return err } - // Load actual unlocker objects to get the proper IDs var unlockers []UnlockerInfo - for _, metadata := range unlockerMetadataList { - // Create unlocker instance to get the proper ID - vaultDir, err := vlt.GetDirectory() - if err != nil { - secret.Warn("Could not get vault directory while listing unlockers", - "error", err) + for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) { + metadata := unlockerMetadata[unlockerID] - continue - } - - // Find the unlocker directory by type and created time - unlockersDir := filepath.Join(vaultDir, "unlockers.d") - - unlockerID, err := findUnlockerIDByMetadata( - cli.fs, unlockersDir, metadata, true, - ) - if err != nil { - secret.Warn("Could not read unlockers directory, skipping unlocker", - "unlockers_dir", unlockersDir, "error", err) - - continue - } - - // Get the proper ID using the unlocker's ID() method - var properID string - if unlockerID != "" { - properID = unlockerID - } else { - // Generate ID as fallback - properID = fmt.Sprintf("%s-%s", - metadata.CreatedAt.Format("2006-01-02.15.04"), metadata.Type) - secret.Warn("Could not create unlocker instance, using fallback ID", - "fallback_id", properID, "type", metadata.Type) - } - - unlockerInfo := UnlockerInfo{ - ID: properID, + unlockers = append(unlockers, UnlockerInfo{ + ID: unlockerID, Type: metadata.Type, CreatedAt: metadata.CreatedAt, Flags: metadata.Flags, - IsCurrent: properID == currentUnlockerID, - } - unlockers = append(unlockers, unlockerInfo) + IsCurrent: unlockerID == currentUnlockerID, + }) } if jsonOutput { @@ -697,9 +580,7 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { } // Check if this GPG key is already added - expectedID := "pgp-" + fingerprint - - exists, err := cli.checkUnlockerExists(vlt, expectedID) + exists, err := cli.pgpUnlockerExists(vlt, fingerprint) if err != nil { return fmt.Errorf( "could not check whether GPG key %s is already an unlocker: %w", @@ -804,13 +685,7 @@ func (cli *Instance) findUnlockerToRemove( } if len(unlockers) == 1 { - lastID, err := findUnlockerIDByMetadata( - cli.fs, unlockersDir, unlockers[0], true) - if err != nil { - return unlockerToRemove{}, err - } - - found.last = lastID == unlockerID + _, found.last = unlockers[unlockerID] } // unlockerID may instead name a directory left out of the list. If its @@ -889,16 +764,16 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error { return vlt.SelectUnlocker(unlockerID) } -// checkUnlockerExists reports whether the vault already has an unlocker -// with the given ID. It returns an error, and no answer, when unlockers.d -// or an unlocker's metadata file cannot be read; the caller must then not -// create the unlocker. It reads unlockers.d itself because -// vault.ListUnlockers skips an unlocker it cannot read, which suits +// pgpUnlockerExists reports whether the vault already has a PGP unlocker +// for the GPG key with the given fingerprint. It returns an error, and no +// answer, when unlockers.d or an unlocker's metadata file cannot be read; +// the caller must then not create the unlocker. It reads unlockers.d itself +// because vault.ListUnlockers skips an unlocker it cannot read, which suits // `unlocker list` but not this check: the skipped unlocker may be the // duplicate. A directory whose metadata file is missing or corrupt is not // a working unlocker and is passed over. -func (cli *Instance) checkUnlockerExists( - vlt *vault.Vault, unlockerID string, +func (cli *Instance) pgpUnlockerExists( + vlt *vault.Vault, fingerprint string, ) (bool, error) { vaultDir, err := vlt.GetDirectory() if err != nil { @@ -937,14 +812,14 @@ func (cli *Instance) checkUnlockerExists( ) } - var metadata secret.UnlockerMetadata + var metadata secret.PGPUnlockerMetadata err = json.Unmarshal(metadataBytes, &metadata) if err != nil { continue } - if unlockerIDFromDir(cli.fs, unlockerDir, metadata, true) == unlockerID { + if metadata.Type == unlockerTypePGP && metadata.GPGKeyID == fingerprint { return true, nil } } diff --git a/internal/cli/unlockers_corrupt_test.go b/internal/cli/unlockers_corrupt_test.go index 1ea6939..1dbb9f4 100644 --- a/internal/cli/unlockers_corrupt_test.go +++ b/internal/cli/unlockers_corrupt_test.go @@ -46,7 +46,7 @@ func TestUnlockerSelectSkipsCorruptUnlocker(t *testing.T) { fs := newCorruptUnlockerVault(t) instance, _ := newTestInstance(fs) - require.NoError(t, instance.UnlockerSelect("pgp-"+listTestGPGKeyID+"B")) + require.NoError(t, instance.UnlockerSelect(listTestUnlockerDirTwo)) current, err := afero.ReadFile(fs, filepath.Join(testVaultDir(listTestVaultName), "current-unlocker")) @@ -72,7 +72,7 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { }{ { name: "the other unlocker", - unlockerID: "pgp-" + listTestGPGKeyID + "B", + unlockerID: listTestUnlockerDirTwo, wantLast: true, wantEntries: []string{listTestUnlockerDirOne}, }, diff --git a/internal/cli/unlockers_ids_test.go b/internal/cli/unlockers_ids_test.go new file mode 100644 index 0000000..31d3979 --- /dev/null +++ b/internal/cli/unlockers_ids_test.go @@ -0,0 +1,79 @@ +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "encoding/json" + "path/filepath" + "testing" + "time" + + "git.eeqj.de/sneak/secret/internal/secret" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSameMetadataUnlockersHaveTheirOwnIDs writes two passphrase unlockers +// side by side whose metadata is the same, creation time included, as +// copying an unlocker directory leaves them. It asserts that `unlocker +// list` and the shell completion of `unlocker select` and `unlocker remove` +// give each its own ID, and that each is selected and removed by its ID +// alone. Keychain and Secure Enclave unlockers, which only macOS can add, +// get their IDs the same way. +func TestSameMetadataUnlockersHaveTheirOwnIDs(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + _, err := vault.CreateVault(fs, listTestStateDir, listTestVaultName, + testMnemonicBuffer(t), nil) + require.NoError(t, err) + + vaultDir := testVaultDir(listTestVaultName) + unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName) + dirNames := []string{ + "passphrase-2026-10-04.12.30.00.000000000", + "passphrase-2026-10-04.12.30.00.000000000-copy", + } + + metadata, err := json.Marshal(secret.UnlockerMetadata{ + Type: unlockerTypePassphrase, + CreatedAt: time.Date(2026, time.October, 4, 12, 30, 0, 0, time.UTC), + }) + require.NoError(t, err) + + for _, dirName := range dirNames { + dir := filepath.Join(unlockersDir, dirName) + require.NoError(t, fs.MkdirAll(dir, listTestDirPerm)) + require.NoError(t, afero.WriteFile(fs, + filepath.Join(dir, listTestMetadataFileName), metadata, + listTestFilePerm)) + } + + listed := listUnlockersJSON(t, fs) + require.Len(t, listed, len(dirNames)) + + completed, _ := getUnlockerIDsCompletionFunc(fs, listTestStateDir)( + nil, nil, "") + assert.Equal(t, dirNames, completed) + + instance, cmd := newTestInstance(fs) + + for i, unlocker := range listed { + assert.Equal(t, dirNames[i], unlocker.ID) + + require.NoError(t, instance.UnlockerSelect(unlocker.ID)) + + current, err := afero.ReadFile(fs, + filepath.Join(vaultDir, "current-unlocker")) + require.NoError(t, err) + assert.Equal(t, dirNames[i], string(current)) + } + + // The second one first: an ID both shared would remove the first one + require.NoError(t, instance.UnlockersRemove(listed[1].ID, true, cmd)) + assertDirEntries(t, fs, unlockersDir, dirNames[0]) + + require.NoError(t, instance.UnlockersRemove(listed[0].ID, true, cmd)) + assertDirEntries(t, fs, unlockersDir) +} diff --git a/internal/cli/unlockers_list_test.go b/internal/cli/unlockers_list_test.go index 20033d7..62177bc 100644 --- a/internal/cli/unlockers_list_test.go +++ b/internal/cli/unlockers_list_test.go @@ -1,25 +1,13 @@ // Unlocker List Tests // -// Tests for `secret unlocker list` behavior when the unlockers.d directory, -// or an unlocker's metadata in it, cannot be read while the listing is -// being rendered: +// Tests for `secret unlocker list` behavior when an unlocker's metadata +// cannot be read or used: // -// - TestUnlockersListSkipsUnreadableUnlockersDir: an unreadable -// unlockers.d yields no rows rather than rows bearing synthesized IDs. -// - TestUnlockersListSkipsOnlyUnreadableEntries: a readable entry is -// still listed, with its real ID and its current-unlocker marker, -// when a later entry's scan fails. // - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt // metadata does not stop the others from being listed. // - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata // file cannot be checked for or read is left out, and the other is // still listed. -// -// The listing resolves each unlocker's real ID by rescanning unlockers.d -// after the vault has already enumerated it. If that rescan fails the ID -// is unknowable, so the entry must be skipped: a synthesized ID matches -// no `unlocker remove` or `unlocker select` argument and would also -// suppress the current-unlocker marker. //nolint:testpackage // white-box test of unexported internals package cli @@ -48,18 +36,16 @@ const ( // listTestVaultName is the name of that synthetic vault. listTestVaultName = "default" - // listTestGPGKeyID is the GPG key ID recorded in the readable PGP - // unlocker's metadata. The unlocker's real ID is derived from it, and - // differs from the timestamp-derived fallback ID. + // listTestGPGKeyID is the GPG key ID recorded, with a letter appended, + // in the PGP unlockers' metadata. listTestGPGKeyID = "DEADBEEFDEADBEEF" // listTestUnlockerDirOne and listTestUnlockerDirTwo are the unlocker - // directory names under unlockers.d. + // directory names under unlockers.d, and so the unlockers' IDs. listTestUnlockerDirOne = "host-pgp-2026-08-09" listTestUnlockerDirTwo = "host-pgp-2026-08-10" - // listTestUnlockersDirName is the directory the listing rescans to - // resolve unlocker IDs. + // listTestUnlockersDirName is the directory holding the unlockers. listTestUnlockersDirName = "unlockers.d" // listTestMetadataFileName is the per-unlocker metadata file name. @@ -74,25 +60,16 @@ const ( // a successful open of unlockers.d. var errUnlockersDirUnreadable = errors.New("permission denied") -// unlockersDirFailFs makes unlockers.d unreadable once it has been opened -// successfully openBudget times. This reproduces the directory becoming -// unreadable (permission change, partially restored backup, EIO) between -// the vault's own enumeration and the per-entry rescan that resolves -// unlocker IDs. +// unlockersDirFailFs fails every open of unlockers.d, as when the +// directory cannot be read. type unlockersDirFailFs struct { afero.Fs - - openBudget int - opens int } //nolint:ireturn // afero.File is the interface required by afero.Fs func (f *unlockersDirFailFs) Open(name string) (afero.File, error) { if filepath.Base(name) == listTestUnlockersDirName { - f.opens++ - if f.opens > f.openBudget { - return nil, errUnlockersDirUnreadable - } + return nil, errUnlockersDirUnreadable } //nolint:wrapcheck // test double must return the wrapped Fs error as-is @@ -142,8 +119,8 @@ func (f *metadataStatFailFs) Stat(name string) (os.FileInfo, error) { return f.Fs.Stat(name) } -// writePGPUnlocker writes a PGP unlocker directory with metadata that -// yields the real ID "pgp-". +// writePGPUnlocker writes a PGP unlocker directory named dirName, with +// metadata recording the GPG key ID keyID. func writePGPUnlocker( t *testing.T, fs afero.Fs, unlockersDir, dirName string, createdAt time.Time, keyID string, @@ -224,44 +201,6 @@ func listUnlockersJSON(t *testing.T, fs afero.Fs) []UnlockerInfo { return decoded.Unlockers } -// TestUnlockersListSkipsUnreadableUnlockersDir asserts that an unlockers.d -// which becomes unreadable after the vault enumerated it produces no rows, -// rather than rows carrying fabricated fallback IDs. -func TestUnlockersListSkipsUnreadableUnlockersDir(t *testing.T) { - t.Parallel() - - base := newListTestVault(t, 1) - // Budget of one: the vault's own ListUnlockers scan succeeds, the - // per-entry rescan that resolves the ID fails. - fs := &unlockersDirFailFs{Fs: base, openBudget: 1} - - unlockers := listUnlockersJSON(t, fs) - - assert.Empty(t, unlockers, - "an unreadable unlockers.d must yield no rows, not fabricated IDs") -} - -// TestUnlockersListSkipsOnlyUnreadableEntries asserts that a readable -// entry survives with its real ID and current-unlocker marker when a later -// entry's rescan fails. -func TestUnlockersListSkipsOnlyUnreadableEntries(t *testing.T) { - t.Parallel() - - base := newListTestVault(t, 2) - // Budget of two: ListUnlockers plus the first entry's rescan succeed, - // the second entry's rescan fails. - fs := &unlockersDirFailFs{Fs: base, openBudget: 2} - - unlockers := listUnlockersJSON(t, fs) - - require.Len(t, unlockers, 1, - "only the entry whose directory was readable may be listed") - assert.Equal(t, "pgp-"+listTestGPGKeyID+"A", unlockers[0].ID, - "the surviving row must carry the real unlocker ID") - assert.True(t, unlockers[0].IsCurrent, - "the current-unlocker marker must survive the skip") -} - // TestUnlockersListReadableEntriesAreListed is the control case: with a // fully readable unlockers.d every entry is listed with its real ID. func TestUnlockersListReadableEntriesAreListed(t *testing.T) { @@ -272,20 +211,21 @@ func TestUnlockersListReadableEntriesAreListed(t *testing.T) { unlockers := listUnlockersJSON(t, base) require.Len(t, unlockers, 2) - assert.Equal(t, "pgp-"+listTestGPGKeyID+"A", unlockers[0].ID) - assert.Equal(t, "pgp-"+listTestGPGKeyID+"B", unlockers[1].ID) + assert.Equal(t, listTestUnlockerDirOne, unlockers[0].ID) + assert.Equal(t, listTestUnlockerDirTwo, unlockers[1].ID) assert.True(t, unlockers[0].IsCurrent) assert.False(t, unlockers[1].IsCurrent) } // TestUnlockersListToleratesCorruptMetadata asserts that one unlocker with // corrupt metadata does not stop the listing. Metadata that is not JSON -// leaves that unlocker out; PGP metadata without a usable GPG key ID lists -// it as "pgp-unknown". The healthy unlocker is listed with its real ID. +// leaves that unlocker out; PGP metadata without a usable GPG key ID, and +// metadata of an unknown type, are still listed, under the directory name +// like any other. The healthy unlocker is listed with its real ID. func TestUnlockersListToleratesCorruptMetadata(t *testing.T) { t.Parallel() - healthyID := "pgp-" + listTestGPGKeyID + "A" + healthyID := listTestUnlockerDirOne tests := []struct { name string @@ -300,12 +240,17 @@ func TestUnlockersListToleratesCorruptMetadata(t *testing.T) { { name: "GPG key ID of the wrong type", metadata: `{"type": "pgp", "gpgKeyId": 42}`, - wantIDs: []string{healthyID, "pgp-unknown"}, + wantIDs: []string{healthyID, listTestUnlockerDirTwo}, }, { name: "GPG key ID missing", metadata: `{"type": "pgp"}`, - wantIDs: []string{healthyID, "pgp-unknown"}, + wantIDs: []string{healthyID, listTestUnlockerDirTwo}, + }, + { + name: "unknown type", + metadata: `{"type": "unknown"}`, + wantIDs: []string{healthyID, listTestUnlockerDirTwo}, }, } @@ -371,7 +316,7 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) { require.Len(t, unlockers, 1, "only the unlocker with usable metadata may be listed") - assert.Equal(t, "pgp-"+listTestGPGKeyID+"B", unlockers[0].ID, + assert.Equal(t, listTestUnlockerDirTwo, unlockers[0].ID, "the listed row must carry the real unlocker ID") }) } diff --git a/internal/cli/unreadable_dir_test.go b/internal/cli/unreadable_dir_test.go index fb8be6f..ac500c1 100644 --- a/internal/cli/unreadable_dir_test.go +++ b/internal/cli/unreadable_dir_test.go @@ -290,7 +290,7 @@ func TestRemoveLastUnlockerAbortsWhenSecretsUnreadable(t *testing.T) { writeTestSecret(t, base, vaultDir) instance, _ := newTestInstance(&statFailFs{Fs: base, path: path}) - _, err := instance.findUnlockerToRemove("pgp-" + listTestGPGKeyID + "A") + _, err := instance.findUnlockerToRemove(listTestUnlockerDirOne) require.ErrorIs(t, err, errStatFailed) assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne) diff --git a/internal/secret/keychainunlocker.go b/internal/secret/keychainunlocker.go index c5bb5da..2a62102 100644 --- a/internal/secret/keychainunlocker.go +++ b/internal/secret/keychainunlocker.go @@ -156,20 +156,9 @@ func (k *KeychainUnlocker) GetDirectory() string { return k.Directory } -// GetID implements Unlocker interface - generates ID from keychain item name +// GetID implements Unlocker interface: the name of the unlocker's directory func (k *KeychainUnlocker) GetID() string { - // Generate ID in the format YYYY-MM-DD.HH.mm-hostname-keychain - // This matches the passphrase unlocker format - hostname, err := os.Hostname() - if err != nil { - hostname = "unknown" - } - - // Use the creation timestamp from metadata - createdAt := k.Metadata.CreatedAt - timestamp := createdAt.Format("2006-01-02.15.04") - - return fmt.Sprintf("%s-%s-keychain", timestamp, hostname) + return filepath.Base(k.Directory) } // Remove implements Unlocker interface - removes the keychain unlocker diff --git a/internal/secret/keychainunlocker_stub.go b/internal/secret/keychainunlocker_stub.go index 950f13a..6e3f3f9 100644 --- a/internal/secret/keychainunlocker_stub.go +++ b/internal/secret/keychainunlocker_stub.go @@ -4,6 +4,7 @@ package secret import ( "errors" + "path/filepath" "filippo.io/age" "github.com/awnumar/memguard" @@ -60,9 +61,9 @@ func (k *KeychainUnlocker) GetDirectory() string { return k.Directory } -// GetID returns the unlocker ID +// GetID returns the unlocker ID, the name of the unlocker's directory func (k *KeychainUnlocker) GetID() string { - return k.Metadata.CreatedAt.Format("2006-01-02.15.04") + "-keychain" + return filepath.Base(k.Directory) } // GetKeychainItemName returns an error on non-Darwin platforms diff --git a/internal/secret/passphraseunlocker.go b/internal/secret/passphraseunlocker.go index 730d7c5..2ed41f2 100644 --- a/internal/secret/passphraseunlocker.go +++ b/internal/secret/passphraseunlocker.go @@ -109,12 +109,9 @@ func (p *PassphraseUnlocker) GetDirectory() string { return p.Directory } -// GetID implements Unlocker interface - generates ID from creation timestamp +// GetID implements Unlocker interface: the name of the unlocker's directory func (p *PassphraseUnlocker) GetID() string { - // Generate ID using creation timestamp: YYYY-MM-DD.HH.mm-passphrase - createdAt := p.Metadata.CreatedAt - - return createdAt.Format("2006-01-02.15.04") + "-passphrase" + return filepath.Base(p.Directory) } // Remove implements Unlocker interface - removes the passphrase unlocker diff --git a/internal/secret/pgpunlock_test.go b/internal/secret/pgpunlock_test.go index 064a174..3fa4be5 100644 --- a/internal/secret/pgpunlock_test.go +++ b/internal/secret/pgpunlock_test.go @@ -297,11 +297,6 @@ func TestPGPUnlockerWithRealFS(t *testing.T) { // Create a PGP unlocker for the remaining tests unlocker := secret.NewPGPUnlocker(fs, unlockerDir, metadata) - // Test getting GPG key ID - t.Run("GetGPGKeyID", func(t *testing.T) { - testGetGPGKeyID(t, fs, unlocker, unlockerDir, metadata, fingerprint) - }) - // Test getting identity from PGP unlocker t.Run("GetIdentity", func(t *testing.T) { testPGPUnlockerGetIdentity(t, fs, unlocker, unlockerDir, keyID) @@ -396,10 +391,10 @@ func testCreatePGPUnlocker( t.Errorf("Expected PGP unlock key type 'pgp', got '%s'", pgpUnlocker.GetType()) } - // Check if the key ID includes the GPG fingerprint - if !strings.Contains(pgpUnlocker.GetID(), fingerprint) { - t.Errorf("PGP unlock key ID '%s' does not contain GPG fingerprint '%s'", - pgpUnlocker.GetID(), fingerprint) + // Check that the ID is the name of the unlocker's directory + if pgpUnlocker.GetID() != filepath.Base(pgpUnlocker.GetDirectory()) { + t.Errorf("PGP unlock key ID '%s' is not its directory name '%s'", + pgpUnlocker.GetID(), filepath.Base(pgpUnlocker.GetDirectory())) } checkPGPUnlockerFiles(t, fs, pgpUnlocker.GetDirectory()) @@ -504,51 +499,6 @@ func checkPGPUnlockerMetadata( } } -// testGetGPGKeyID writes PGP unlocker metadata holding the GPG fingerprint -// into unlockerDir and checks that unlocker reads it back. -func testGetGPGKeyID( - t *testing.T, fs afero.Fs, unlocker *secret.PGPUnlocker, - unlockerDir string, metadata secret.UnlockerMetadata, fingerprint string, -) { - t.Helper() - - // Create PGP metadata with GPG key ID - type PGPUnlockerMetadata struct { - secret.UnlockerMetadata - - GPGKeyID string `json:"gpgKeyId"` - } - - pgpMetadata := PGPUnlockerMetadata{ - UnlockerMetadata: metadata, - GPGKeyID: fingerprint, - } - - // Write metadata file - metadataPath := filepath.Join(unlockerDir, unlockerMetadataFile) - - metadataBytes, err := json.MarshalIndent(pgpMetadata, "", " ") - if err != nil { - t.Fatalf("Failed to marshal metadata: %v", err) - } - - err = afero.WriteFile(fs, metadataPath, metadataBytes, secret.FilePerms) - if err != nil { - t.Fatalf("Failed to write metadata: %v", err) - } - - // Get GPG key ID - retrievedKeyID, err := unlocker.GetGPGKeyID() - if err != nil { - t.Fatalf("Failed to get GPG key ID: %v", err) - } - - // Verify key ID (should be the fingerprint) - if retrievedKeyID != fingerprint { - t.Errorf("Expected GPG fingerprint '%s', got '%s'", fingerprint, retrievedKeyID) - } -} - // testPGPUnlockerGetIdentity writes an age identity encrypted to the GPG key // keyID into unlockerDir and checks that unlocker decrypts it. func testPGPUnlockerGetIdentity( diff --git a/internal/secret/pgpunlocker.go b/internal/secret/pgpunlocker.go index f1e17c6..5484646 100644 --- a/internal/secret/pgpunlocker.go +++ b/internal/secret/pgpunlocker.go @@ -155,21 +155,9 @@ func (p *PGPUnlocker) GetDirectory() string { return p.Directory } -// GetID implements Unlocker interface - generates ID from GPG key ID. -// If the metadata has no usable GPG key ID, it warns with the unlocker's -// directory and returns "pgp-unknown", so listing the other unlockers -// still works. +// GetID implements Unlocker interface: the name of the unlocker's directory func (p *PGPUnlocker) GetID() string { - // Generate ID using GPG key ID: pgp- - gpgKeyID, err := p.GetGPGKeyID() - if err != nil { - Warn("PGP unlocker metadata is corrupt or missing its GPG key ID", - "directory", p.Directory, "error", err) - - return "pgp-unknown" - } - - return "pgp-" + gpgKeyID + return filepath.Base(p.Directory) } // Remove implements Unlocker interface - removes the PGP unlocker @@ -184,30 +172,6 @@ func (p *PGPUnlocker) Remove() error { return nil } -// GetGPGKeyID returns the GPG key ID from metadata -func (p *PGPUnlocker) GetGPGKeyID() (string, error) { - // Load the metadata - metadataPath := filepath.Join(p.Directory, "unlocker-metadata.json") - - metadataData, err := afero.ReadFile(p.fs, metadataPath) - if err != nil { - return "", fmt.Errorf("failed to read PGP metadata: %w", err) - } - - var pgpMetadata PGPUnlockerMetadata - - err = json.Unmarshal(metadataData, &pgpMetadata) - if err != nil { - return "", fmt.Errorf("failed to parse PGP metadata: %w", err) - } - - if pgpMetadata.GPGKeyID == "" { - return "", fmt.Errorf("PGP metadata: %w", errGPGKeyIDEmpty) - } - - return pgpMetadata.GPGKeyID, nil -} - // generatePGPUnlockerName generates a unique name for the PGP unlocker // based on hostname and time func generatePGPUnlockerName() (string, error) { diff --git a/internal/secret/seunlocker_darwin.go b/internal/secret/seunlocker_darwin.go index d25ce61..f90ed30 100644 --- a/internal/secret/seunlocker_darwin.go +++ b/internal/secret/seunlocker_darwin.go @@ -130,17 +130,9 @@ func (s *SecureEnclaveUnlocker) GetDirectory() string { return s.Directory } -// GetID implements Unlocker interface. +// GetID implements Unlocker interface: the name of the unlocker's directory. func (s *SecureEnclaveUnlocker) GetID() string { - hostname, err := os.Hostname() - if err != nil { - hostname = "unknown" - } - - createdAt := s.Metadata.CreatedAt - timestamp := createdAt.Format("2006-01-02.15.04") - - return fmt.Sprintf("%s-%s-%s", timestamp, hostname, seUnlockerType) + return filepath.Base(s.Directory) } // Remove implements Unlocker interface. diff --git a/internal/secret/seunlocker_stub.go b/internal/secret/seunlocker_stub.go index eaae974..14f4456 100644 --- a/internal/secret/seunlocker_stub.go +++ b/internal/secret/seunlocker_stub.go @@ -4,6 +4,7 @@ package secret import ( "errors" + "path/filepath" "filippo.io/age" "github.com/awnumar/memguard" @@ -67,9 +68,9 @@ func (s *SecureEnclaveUnlocker) GetDirectory() string { return s.Directory } -// GetID returns the unlocker ID. +// GetID returns the unlocker ID, the name of the unlocker's directory. func (s *SecureEnclaveUnlocker) GetID() string { - return s.Metadata.CreatedAt.Format("2006-01-02.15.04") + "-" + seUnlockerType + return filepath.Base(s.Directory) } // Remove returns an error on non-Darwin platforms. diff --git a/internal/secret/seunlocker_stub_test.go b/internal/secret/seunlocker_stub_test.go index e6ff9db..4cae318 100644 --- a/internal/secret/seunlocker_stub_test.go +++ b/internal/secret/seunlocker_stub_test.go @@ -35,9 +35,8 @@ func TestNewSecureEnclaveUnlocker(t *testing.T) { // Test GetDirectory returns the directory we passed in assert.Equal(t, dir, unlocker.GetDirectory()) - // Test GetID returns a formatted string with the creation timestamp - expectedID := "2026-01-15.10.30-secure-enclave" - assert.Equal(t, expectedID, unlocker.GetID()) + // Test GetID returns the name of the unlocker's directory + assert.Equal(t, "test-se-unlocker", unlocker.GetID()) } func TestSecureEnclaveUnlockerGetIdentityReturnsError(t *testing.T) { diff --git a/internal/secret/seunlocker_test.go b/internal/secret/seunlocker_test.go index 47c5063..a737999 100644 --- a/internal/secret/seunlocker_test.go +++ b/internal/secret/seunlocker_test.go @@ -61,11 +61,9 @@ func TestSecureEnclaveUnlockerGetIDFormat(t *testing.T) { } unlocker := NewSecureEnclaveUnlocker(fs, "/tmp/test", metadata) - id := unlocker.GetID() - // ID should contain the timestamp and "secure-enclave" type - assert.Contains(t, id, "2026-03-10.14.30") - assert.Contains(t, id, seUnlockerType) + // The ID is the name of the unlocker's directory + assert.Equal(t, "test", unlocker.GetID()) } func TestGenerateSEKeyLabel(t *testing.T) { diff --git a/internal/secret/unlocker.go b/internal/secret/unlocker.go index 9cd41ea..338f8a2 100644 --- a/internal/secret/unlocker.go +++ b/internal/secret/unlocker.go @@ -10,6 +10,6 @@ type Unlocker interface { GetType() string GetMetadata() UnlockerMetadata GetDirectory() string - GetID() string // Generate ID based on unlocker type and data + GetID() string // The name of the unlocker's directory, unique in its vault Remove() error // Remove the unlocker and any associated resources } diff --git a/internal/vault/unlockers.go b/internal/vault/unlockers.go index b9df96d..b4b419b 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -188,8 +188,9 @@ func (v *Vault) findUnlockerByID( return nil, skippedDirPath, nil } -// ListUnlockers returns a list of available unlockers for this vault -func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { +// ListUnlockers returns the metadata of each unlocker of this vault, keyed +// by the unlocker's ID, the name of its directory in unlockers.d +func (v *Vault) ListUnlockers() (map[string]UnlockerMetadata, error) { vaultDir, err := v.GetDirectory() if err != nil { return nil, err @@ -204,7 +205,7 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { } if !exists { - return []UnlockerMetadata{}, nil + return map[string]UnlockerMetadata{}, nil } // List directories in unlockers.d @@ -213,7 +214,7 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { return nil, fmt.Errorf("failed to read unlockers directory: %w", err) } - var unlockers []UnlockerMetadata + unlockers := map[string]UnlockerMetadata{} for _, file := range files { if !file.IsDir() { @@ -222,7 +223,7 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name()) if ok { - unlockers = append(unlockers, metadata) + unlockers[file.Name()] = metadata } } @@ -453,8 +454,7 @@ func writePassphraseUnlocker( return nil, err } - // Select the new unlocker by its directory, not by its ID: an old - // passphrase unlocker created in the same minute has the same ID. + // Make the new unlocker the current one currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker") err = secret.WriteFileAtomic(fs, currentUnlockerPath,