From 7da008956409b3edd9b51da3567a3c15dce82710 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sat, 3 Oct 2026 12:17:04 +0000 Subject: [PATCH] Keep unlocker list working when unlocker metadata is corrupt (closes #42) PGPUnlocker.GetID() panicked when its metadata could not be read or parsed, which took down `secret unlocker list` for every unlocker. It now warns with the unlocker's directory and returns `pgp-unknown`; metadata with an empty GPG key ID counts as corrupt too. ListUnlockers now skips, with a warning, an unlocker whose metadata file cannot be checked for, read or parsed, as it already did for a missing one. The listing's ID lookup skips such a directory without warning again. This is the first half of the issue only. Passing the mnemonic in memory moved to https://git.eeqj.de/sneak/secret/issues/60. Model: opus-5-5 --- TODO.md | 8 +- internal/cli/unlockers.go | 10 +- internal/cli/unlockers_list_test.go | 153 +++++++++++++++++++++++++++- internal/secret/pgpunlocker.go | 16 ++- internal/vault/unlockers.go | 19 ++-- 5 files changed, 185 insertions(+), 21 deletions(-) diff --git a/TODO.md b/TODO.md index b919e96..e7896e3 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,12 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: A PGP unlocker whose metadata has no usable GPG key ID + no longer panics: `GetID()` warns with the unlocker's directory and + returns `pgp-unknown`. `ListUnlockers` skips, with a warning, an + unlocker whose metadata file cannot be checked for, read or parsed + instead of failing, so `secret unlocker list` still lists the others; + the listing's ID lookup no longer warns about that directory again. - 2026-10-03: `version rm`, `version promote` and `get --version` accept a version only if it is one of the versions `version list` lists for that secret, compared as typed before any path is built @@ -125,8 +131,6 @@ Bring the repo into policy compliance in one commit: - Timing attacks: bytes.Equal passphrase compare (cli/init.go: 209-216); non-constant-time public key compare (vault.go:95-100). - High priority: - - Return errors instead of panicking on corrupted metadata - (pgpunlocker.go:116, keychainunlocker.go:141). - Secure temporary file handling and cleanup. - Print cobra usage only for argument errors, not internal failures. diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 593f462..6d80265 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -349,6 +349,10 @@ func unlockerIDFromDir( // 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, @@ -371,9 +375,6 @@ func findUnlockerIDByMetadata( // Check if this is the right unlocker by comparing metadata metadataBytes, err := afero.ReadFile(fs, metadataPath) if err != nil { - secret.Warn("Could not read unlocker metadata file", - "path", metadataPath, "error", err) - continue } @@ -381,9 +382,6 @@ func findUnlockerIDByMetadata( err = json.Unmarshal(metadataBytes, &diskMetadata) if err != nil { - secret.Warn("Could not parse unlocker metadata file", - "path", metadataPath, "error", err) - continue } diff --git a/internal/cli/unlockers_list_test.go b/internal/cli/unlockers_list_test.go index bf2c564..20033d7 100644 --- a/internal/cli/unlockers_list_test.go +++ b/internal/cli/unlockers_list_test.go @@ -1,13 +1,19 @@ // Unlocker List Tests // -// Tests for `secret unlocker list` behavior when the unlockers.d directory -// cannot be read while the listing is being rendered: +// 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: // // - 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 @@ -22,6 +28,7 @@ import ( "bytes" "encoding/json" "errors" + "os" "path/filepath" "testing" "time" @@ -92,6 +99,49 @@ func (f *unlockersDirFailFs) Open(name string) (afero.File, error) { return f.Fs.Open(name) } +// errMetadataUnreadable is returned by the test filesystem in place of a +// successful open of one unlocker'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 + } + + //nolint:wrapcheck // test double must return the wrapped Fs error as-is + return f.Fs.Open(name) +} + +// errMetadataUncheckable is returned by the test filesystem in place of a +// successful check for one unlocker's metadata file. +var errMetadataUncheckable = errors.New("permission denied") + +// metadataStatFailFs fails every check for whether the file at +// uncheckablePath exists, as when its unlocker directory cannot be entered. +type metadataStatFailFs struct { + afero.Fs + + uncheckablePath string +} + +func (f *metadataStatFailFs) Stat(name string) (os.FileInfo, error) { + if name == f.uncheckablePath { + return nil, errMetadataUncheckable + } + + //nolint:wrapcheck // test double must return the wrapped Fs error as-is + return f.Fs.Stat(name) +} + // writePGPUnlocker writes a PGP unlocker directory with metadata that // yields the real ID "pgp-". func writePGPUnlocker( @@ -227,3 +277,102 @@ func TestUnlockersListReadableEntriesAreListed(t *testing.T) { 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. +func TestUnlockersListToleratesCorruptMetadata(t *testing.T) { + t.Parallel() + + healthyID := "pgp-" + listTestGPGKeyID + "A" + + tests := []struct { + name string + metadata string + wantIDs []string + }{ + { + name: "not JSON", + metadata: "not json", + wantIDs: []string{healthyID}, + }, + { + name: "GPG key ID of the wrong type", + metadata: `{"type": "pgp", "gpgKeyId": 42}`, + wantIDs: []string{healthyID, "pgp-unknown"}, + }, + { + name: "GPG key ID missing", + metadata: `{"type": "pgp"}`, + wantIDs: []string{healthyID, "pgp-unknown"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := newListTestVault(t, 2) + metadataPath := filepath.Join(listTestStateDir, "vaults.d", + listTestVaultName, listTestUnlockersDirName, + listTestUnlockerDirTwo, listTestMetadataFileName) + require.NoError(t, afero.WriteFile( + fs, metadataPath, []byte(tt.metadata), listTestFilePerm, + )) + + unlockers := listUnlockersJSON(t, fs) + require.Len(t, unlockers, len(tt.wantIDs)) + + for i, wantID := range tt.wantIDs { + assert.Equal(t, wantID, unlockers[i].ID) + } + }) + } +} + +// TestUnlockersListSkipsUnreadableMetadata asserts that an unlocker whose +// metadata file cannot be checked for or cannot be read is left out of the +// listing, and the other unlocker is still listed with its real ID. The +// failing one sorts first, so finding the other's ID has to step past it +// as well. +func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) { + t.Parallel() + + failingPath := filepath.Join(listTestStateDir, "vaults.d", + listTestVaultName, listTestUnlockersDirName, + listTestUnlockerDirOne, listTestMetadataFileName) + + tests := []struct { + name string + wrap func(base afero.Fs) afero.Fs + }{ + { + name: "checking for the file fails", + wrap: func(base afero.Fs) afero.Fs { + return &metadataStatFailFs{Fs: base, uncheckablePath: failingPath} + }, + }, + { + name: "reading the file fails", + wrap: func(base afero.Fs) afero.Fs { + return &metadataReadFailFs{Fs: base, unreadablePath: failingPath} + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := tt.wrap(newListTestVault(t, 2)) + + unlockers := listUnlockersJSON(t, fs) + + require.Len(t, unlockers, 1, + "only the unlocker with usable metadata may be listed") + assert.Equal(t, "pgp-"+listTestGPGKeyID+"B", unlockers[0].ID, + "the listed row must carry the real unlocker ID") + }) + } +} diff --git a/internal/secret/pgpunlocker.go b/internal/secret/pgpunlocker.go index c6fcbc3..cf80b35 100644 --- a/internal/secret/pgpunlocker.go +++ b/internal/secret/pgpunlocker.go @@ -155,14 +155,18 @@ func (p *PGPUnlocker) GetDirectory() string { return p.Directory } -// GetID implements Unlocker interface - generates ID from GPG key ID +// 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. func (p *PGPUnlocker) GetID() string { // Generate ID using GPG key ID: pgp- gpgKeyID, err := p.GetGPGKeyID() if err != nil { - // The vault metadata is corrupt - this is a fatal error - // We cannot continue with a fallback ID as that would mask data corruption - panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", err)) + Warn("PGP unlocker metadata is corrupt or missing its GPG key ID", + "directory", p.Directory, "error", err) + + return "pgp-unknown" } return "pgp-" + gpgKeyID @@ -197,6 +201,10 @@ func (p *PGPUnlocker) GetGPGKeyID() (string, error) { return "", fmt.Errorf("failed to parse PGP metadata: %w", err) } + if pgpMetadata.GPGKeyID == "" { + return "", fmt.Errorf("PGP metadata: %w", errGPGKeyIDEmpty) + } + return pgpMetadata.GPGKeyID, nil } diff --git a/internal/vault/unlockers.go b/internal/vault/unlockers.go index e2c562b..11eaca9 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -233,9 +233,10 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { exists, err := afero.Exists(v.fs, metadataPath) if err != nil { - return nil, fmt.Errorf( - "failed to check if metadata exists for unlocker %s: %w", - file.Name(), err) + secret.Warn("Skipping unlocker directory whose metadata file cannot be checked", + "directory", file.Name(), "error", err) + + continue } if !exists { @@ -247,16 +248,20 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { metadataBytes, err := afero.ReadFile(v.fs, metadataPath) if err != nil { - return nil, fmt.Errorf( - "failed to read metadata for unlocker %s: %w", file.Name(), err) + secret.Warn("Skipping unlocker directory with unreadable metadata file", + "directory", file.Name(), "error", err) + + continue } var metadata UnlockerMetadata err = json.Unmarshal(metadataBytes, &metadata) if err != nil { - return nil, fmt.Errorf( - "failed to parse metadata for unlocker %s: %w", file.Name(), err) + secret.Warn("Skipping unlocker directory with corrupt metadata file", + "directory", file.Name(), "error", err) + + continue } unlockers = append(unlockers, metadata)