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 is unreadable or not JSON, as it already did for a missing one.

This is the first half of the issue only. Passing the mnemonic in
memory moved to #60.

Model: opus-5-5

TODO.md

@@ -25,9 +25,14 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 2026-10-03: 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 read or parsed instead of
+  failing, so `secret unlocker list` still lists the others.
 - 2026-10-02: A plain `docker build .` builds again: the size tests
-  skip a case that needs more locked memory than the limit allows,
+  skip a case that needs more locked memory than the process can
-  and run every case under `script/cibuild`. The image stamps the
+  lock, and run every case under `script/cibuild`. The image stamps the
   `VERSION` build argument, else `git describe --tags --always`, into
   `Version`, and fails if `.git` is present but yields no version;
   `make build` stamps `git describe` too, not a fixed `0.1.0`.
@@ -101,8 +106,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.

internal/cli/secrets_size_test.go

@@ -28,24 +28,37 @@ const testVaultName = "test-vault"
 // size, and they are then copied into one more buffer of its size.
 const lockedBytesPerSecretByte = 3
 
-// skipIfLockedMemoryTooLow skips the test when the locked-memory limit
+// skipIfLockedMemoryTooLow skips the test when this process cannot lock
-// (RLIMIT_MEMLOCK) cannot hold a secret of size bytes. memguard panics,
+// the memory a secret of size bytes needs, found by locking a buffer of
-// ending the whole test run, when it cannot lock a buffer, and a plain
+// that size and releasing it. memguard panics, ending the whole test run,
-// `docker build .` runs the tests under an 8 MiB limit.
+// when it cannot lock a buffer, and a plain `docker build .` runs the
+// tests under an 8 MiB locked-memory limit (RLIMIT_MEMLOCK). A process
+// allowed to lock past that limit runs every case.
 func skipIfLockedMemoryTooLow(t *testing.T, size int) {
 	t.Helper()
 
-	var limit unix.Rlimit
+	need := lockedBytesPerSecretByte * size
 
-	err := unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
+	buf, err := unix.Mmap(-1, 0, need,
+		unix.PROT_READ|unix.PROT_WRITE, unix.MAP_PRIVATE|unix.MAP_ANON)
 	require.NoError(t, err)
 
-	//nolint:gosec // test sizes are never negative
+	lockErr := unix.Mlock(buf)
-	need := lockedBytesPerSecretByte * uint64(size)
+
-	if limit.Cur < need {
+	// Unmapping the buffer also unlocks it.
+	err = unix.Munmap(buf)
+	require.NoError(t, err)
+
+	if lockErr != nil {
+		var limit unix.Rlimit
+
+		err = unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
+		require.NoError(t, err)
+
 		t.Skipf("a %d-byte secret needs up to %d bytes of locked memory, "+
-			"more than the locked-memory limit (RLIMIT_MEMLOCK) of %d bytes",
+			"which could not be locked under the locked-memory limit "+
-			size, need, limit.Cur)
+			"(RLIMIT_MEMLOCK) of %d bytes: %v",
+			size, need, limit.Cur, lockErr)
 	}
 }
 

internal/cli/unlockers_list_test.go

@@ -1,13 +1,16 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when the unlockers.d directory
+// Tests for `secret unlocker list` behavior when the unlockers.d directory,
-// cannot be read while the listing is being rendered:
+// 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.
 //
 // 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
@@ -227,3 +230,56 @@ 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)
+			}
+		})
+	}
+}

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-<keyid>
 	gpgKeyID, err := p.GetGPGKeyID()
 	if err != nil {
-		// The vault metadata is corrupt - this is a fatal error
+		Warn("PGP unlocker metadata is corrupt or missing its GPG key ID",
-		// We cannot continue with a fallback ID as that would mask data corruption
+			"directory", p.Directory, "error", err)
-		panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", 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
 }
 

internal/vault/unlockers.go

@@ -247,16 +247,20 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
 			if err != nil {
-				return nil, fmt.Errorf(
+				secret.Warn("Skipping unlocker directory with unreadable metadata file",
-					"failed to read metadata for unlocker %s: %w", file.Name(), err)
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			var metadata UnlockerMetadata
 
 			err = json.Unmarshal(metadataBytes, &metadata)
 			if err != nil {
-				return nil, fmt.Errorf(
+				secret.Warn("Skipping unlocker directory with corrupt metadata file",
-					"failed to parse metadata for unlocker %s: %w", file.Name(), err)
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			unlockers = append(unlockers, metadata)