age: fix YubiKey identity persistence via raw-append (ADR-0002) (#3399)

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

Author: dominikschulz

internal/backend/crypto/age/commands.go

@@ -173,9 +173,12 @@ func (l loader) Commands() []*cli.Command {
 								}
 								if len(recEncm) < 1 && !strings.HasPrefix(idS, "AGE-SECRET-KEY-1") {
 									recEncm, err = termio.AskForString(ctx, "Provide the corresponding age recipient", "")
-									if err != nil || recEncm == "" {
+									if err != nil {
 										return exit.Error(exit.Unknown, err, "failed to read corresponding age recipient")
 									}
+									if recEncm == "" {
+										return exit.Error(exit.Usage, nil, "recipient must not be empty for plugin identities")
+									}
 									if strings.HasPrefix(recEncm, "AGE-") {
 										out.Warning(ctx, "You have provided an identity as a recipient, recipients should start in 'age1', this might not be properly supported and might leak secret data in our identity recipient cache")
 									}

internal/backend/crypto/age/identities.go

@@ -339,11 +339,44 @@ func (a *Age) addIdentity(ctx context.Context, id age.Identity) error {
 		debug.Log("error invalidating age id recipient cache: %s", err)
 	}
 
-	ids, _ := a.Identities(ctx)
+	// Read existing identity file as raw text without parsing it.
+	// This avoids re-invoking external age plugins (e.g. age-plugin-yubikey) for
+	// identities already in the file, which would fail if the hardware token is
+	// unavailable or the plugin binary is missing.
+	existing, err := a.loadIdentityFile(ctx)
+	newFile := false
+	if err != nil {
+		if !errors.Is(err, os.ErrNotExist) {
+			return fmt.Errorf("failed to read identity file: %w", err)
+		}
+		newFile = true
+	}
+
+	// Append the new identity as a raw line, preserving all existing content.
+	newLine := fmt.Sprintf("%s", id)
+	var lines []string
+	if existing == "" {
+		lines = []string{newLine}
+	} else {
+		lines = append(strings.Split(strings.TrimRight(existing, "\n"), "\n"), newLine)
+	}
+
+	return a.saveIdentities(ctx, lines, newFile)
+}
+
+// loadIdentityFile decrypts and returns the raw text content of the identity file
+// without parsing individual identity lines. This avoids invoking external age
+// plugins (e.g. age-plugin-yubikey) merely to read existing file contents.
+func (a *Age) loadIdentityFile(ctx context.Context) (string, error) {
+	pwcb := a.effectivePwCallback(fmt.Sprintf("to read the age keyring from %s", a.identity))
+	ppcb := a.effectivePwPurgeCallback()
 
-	ids = append(ids, id)
+	buf, err := a.decryptFile(ctx, a.identity, pwcb, ppcb)
+	if err != nil {
+		return "", err
+	}
 
-	return a.saveIdentities(ctx, identitiesToString(ids), true)
+	return string(buf), nil
 }
 
 func (a *Age) saveIdentities(ctx context.Context, ids []string, newFile bool) error {
@@ -471,15 +504,6 @@ func recipientsToString(recps []age.Recipient) []string {
 	return r
 }
 
-func identitiesToString(ids []age.Identity) []string {
-	r := make([]string, 0, len(ids))
-	for _, id := range ids {
-		r = append(r, fmt.Sprintf("%s", id))
-	}
-
-	return r
-}
-
 func modTime(path string) time.Time {
 	fi, err := os.Stat(path)
 	if err != nil {

internal/backend/crypto/age/identities_test.go

@@ -2,6 +2,8 @@ package age
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"filippo.io/age"
@@ -107,3 +109,112 @@ func TestIdentityAndRecipient(t *testing.T) {
 		})
 	}
 }
+
+// newTestAge creates an Age instance whose identity file lives under a temp
+// directory and uses a fixed test passphrase so no interactive prompt is
+// needed during tests.
+func newTestAge(t *testing.T) *Age {
+	t.Helper()
+	td := t.TempDir()
+	t.Setenv("GOPASS_HOMEDIR", td)
+	ctx := t.Context()
+	a, err := New(ctx, "")
+	require.NoError(t, err)
+	// Override the identity path to a known temp location.
+	a.identity = filepath.Join(td, "age", "identities")
+	// Use a fixed passphrase so no UI interaction is needed.
+	a.pwCallback = func(_ string, _ bool) ([]byte, error) {
+		return []byte("test-passphrase"), nil
+	}
+	a.pwPurgeCallback = func(_ string) {}
+
+	return a
+}
+
+// TestAddIdentityToNewFile verifies that addIdentity works when no identity
+// file exists yet (i.e. creates a new file correctly).
+func TestAddIdentityToNewFile(t *testing.T) {
+	ctx := t.Context()
+	a := newTestAge(t)
+
+	id, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+
+	require.NoError(t, a.addIdentity(ctx, id))
+
+	// The file should now exist and contain exactly the one identity.
+	ids, err := a.Identities(ctx)
+	require.NoError(t, err)
+	require.Len(t, ids, 1)
+	rec := IdentityToRecipient(ids[0])
+	require.NotNil(t, rec)
+	assert.Equal(t, id.Recipient().String(), fmt.Sprintf("%s", rec))
+}
+
+// TestAddIdentityDoesNotParseExistingPluginLines verifies Option A: when a
+// plugin-format line is already present in the identity file, adding a new
+// native identity does NOT re-invoke the plugin binary (because we only do a
+// raw text append, not a parse-all-then-serialize cycle).
+//
+// We simulate this by writing a plugin-format raw line directly into the
+// encrypted identity file via saveIdentities, then adding a new key. If the
+// old code path were still active it would call parseIdentity on the plugin
+// line, which calls plugin.NewIdentity() and would fail without the binary.
+// With Option A, the plugin line is copied verbatim.
+func TestAddIdentityPreservesPluginLineWithoutInvokingPlugin(t *testing.T) {
+	ctx := t.Context()
+	a := newTestAge(t)
+
+	// Plant a plugin-format line (gopass custom format: identity|recipient).
+	// This is the serialized form that saveIdentities / identitiesToString
+	// would produce for a wrappedIdentity.
+	pluginRaw := "AGE-PLUGIN-YUBIKEY-1GKZKJQYZL98RLMC67F9PJ|age1yubikey1qt2r3tfk7wvlykudm7ew28dqqm3h8ln9zfsxsq4lcd2w8rh4n4hhz46ur24"
+	require.NoError(t, a.saveIdentities(ctx, []string{pluginRaw}, true))
+
+	// Now add a real native identity. Option A must NOT try to call
+	// plugin.NewIdentity() on the existing pluginRaw line.
+	newID, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+	require.NoError(t, a.addIdentity(ctx, newID))
+
+	// Read back the raw file and confirm both lines are present.
+	raw, err := a.loadIdentityFile(ctx)
+	require.NoError(t, err)
+
+	assert.Contains(t, raw, pluginRaw, "plugin line must be preserved verbatim")
+	assert.Contains(t, raw, newID.String(), "new native identity must be appended")
+}
+
+// TestAddMultipleIdentitiesAccumulate verifies that calling addIdentity
+// multiple times accumulates all identities in the file, each as its own line.
+func TestAddMultipleIdentitiesAccumulate(t *testing.T) {
+	ctx := t.Context()
+	a := newTestAge(t)
+
+	id1, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+	id2, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+	id3, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+
+	require.NoError(t, a.addIdentity(ctx, id1))
+	require.NoError(t, a.addIdentity(ctx, id2))
+	require.NoError(t, a.addIdentity(ctx, id3))
+
+	ids, err := a.Identities(ctx)
+	require.NoError(t, err)
+	require.Len(t, ids, 3)
+}
+
+// TestLoadIdentityFileNotExist verifies that loadIdentityFile returns an
+// os.ErrNotExist-wrapped error when the identity file has not been created yet.
+func TestLoadIdentityFileNotExist(t *testing.T) {
+	a := newTestAge(t)
+	ctx := t.Context()
+
+	_, err := a.loadIdentityFile(ctx)
+	require.Error(t, err)
+	assert.ErrorIs(t, err, os.ErrNotExist,
+		"loadIdentityFile should surface an os.ErrNotExist-compatible error")
+}