diff --git a/README.md b/README.md index 7d3d10b..3ce2183 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 e4bf515..998e9f1 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,18 @@ 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. The keychain and Secure Enclave code was type-checked by + `script/lint-darwin`, never run; a test on Linux lists, selects and removes + each of two passphrase unlockers created in one minute by its own ID. - 2026-10-04: A failed `secret unlocker add keychain` or `secret unlocker add secure-enclave` no longer leaves its keychain item or Secure Enclave key behind (https://git.eeqj.de/sneak/secret/issues/89). diff --git a/internal/cli/confirm_test.go b/internal/cli/confirm_test.go index 8569e70..3796dcf 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..b77fe4f 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -697,9 +697,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", @@ -889,16 +887,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 +935,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..6dc273a --- /dev/null +++ b/internal/cli/unlockers_ids_test.go @@ -0,0 +1,74 @@ +//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" +) + +// TestSameMinuteUnlockersHaveTheirOwnIDs writes two passphrase unlockers +// created in the same minute side by side, as an `unlocker add passphrase` +// that fails before removing the old one leaves them, and asserts that +// `unlocker list` gives 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 TestSameMinuteUnlockersHaveTheirOwnIDs(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + _, err := vault.CreateVault(fs, listTestStateDir, listTestVaultName, + testMnemonicBuffer(t)) + 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.30.000000000", + } + + for i, dirName := range dirNames { + metadata, err := json.Marshal(secret.UnlockerMetadata{ + Type: unlockerTypePassphrase, + CreatedAt: time.Date(2026, time.October, 4, 12, 30, 30*i, 0, time.UTC), + }) + require.NoError(t, err) + + 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)) + + 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 newer one first: an ID both shared would remove the older 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..b3eea1e 100644 --- a/internal/cli/unlockers_list_test.go +++ b/internal/cli/unlockers_list_test.go @@ -48,13 +48,12 @@ 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" @@ -142,8 +141,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, @@ -256,7 +255,7 @@ func TestUnlockersListSkipsOnlyUnreadableEntries(t *testing.T) { require.Len(t, unlockers, 1, "only the entry whose directory was readable may be listed") - assert.Equal(t, "pgp-"+listTestGPGKeyID+"A", unlockers[0].ID, + assert.Equal(t, listTestUnlockerDirOne, 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") @@ -272,20 +271,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 is +// still listed, under its 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 +300,12 @@ 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}, }, } @@ -371,7 +371,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 164948a..80ab6f6 100644 --- a/internal/secret/pgpunlock_test.go +++ b/internal/secret/pgpunlock_test.go @@ -396,10 +396,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()) diff --git a/internal/secret/pgpunlocker.go b/internal/secret/pgpunlocker.go index f1e17c6..7469dbc 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 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 4075066..e7828b8 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -430,8 +430,7 @@ func (v *Vault) CreatePassphraseUnlocker( 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(v.fs, currentUnlockerPath,