Check existing recipients before trying to add a new one (#2487)

* Check existing recipients before trying to add a new one

Fixes #1918

RELEASE_NOTES=[ENHANCEMENT] Check recipients before adding a new one.

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

* Add test for CheckRecipients with an invalid key.

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

* Add custom error type and a better error message.

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

* Initialize InvalidRecipientsError

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

* Skip CheckRecipients tests on Windows

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

docs/faq.md

@@ -48,6 +48,13 @@ This can be fixed by setting `export TMPDIR=/tmp` (or any other suiteable locati
 
 Old version of `gpg` may fail to decode message encrypted with newer version without any message. The encrypted secret in such case is just empty and gopass will warn you about this. One case of such behaviour we have seen so far is when the encryption key generated with `gpg` version 2.3.x encrypt a password that is then decrypted on `gpg` version 2.2.x (default on Ubuntu 18.04). In this particular case old `gpg` does not understand `AEAD` encryption extension, and it fails without any error.  If it is your case then follw the instructions in listed in #2283.
 
+## Expired recipients
+
+`gopass` will refuse to add new recipients when any invalid (e.g. expired) recipients are present in a password store.
+In such cases manual intervention is required. Expired keys can either be removed or extended. Unknown keys that
+can not be automatically imported need to be obtained and manually imported first. These are restrictions from the underlying
+crypto implementation (GPG) and we can not easily work around these.
+
 ## API Stability
 
 gopass is provided as an CLI program, not as a library. While we try to make the packages usable as libraries we make no guarantees whatsoever with respect to the API stability. The gopass version only reflects changes in the CLI commands.

internal/action/recipients.go

@@ -86,6 +86,12 @@ func (s *Action) RecipientsAdd(c *cli.Context) error {
 		store = cui.AskForStore(ctx, s.Store)
 	}
 
+	if err := s.Store.CheckRecipients(ctx, store); err != nil && !force {
+		out.Errorf(ctx, "%s. Please remove expired keys or extend their validity. See https://go.gopass.pw/faq#expired-recipients", err.Error())
+
+		return exit.Error(exit.Recipients, err, "recipients invalid: %q", err)
+	}
+
 	crypto := s.Store.Crypto(ctx, store)
 
 	// select recipient.

internal/action/recipients_test.go

@@ -147,7 +147,7 @@ func TestRecipientsGpg(t *testing.T) {
 
 	t.Run("add recipient 0xBEEFFEED", func(t *testing.T) {
 		defer buf.Reset()
-		assert.NoError(t, act.RecipientsAdd(gptest.CliCtx(ctx, t, "0xBEEFFEED")))
+		assert.NoError(t, act.RecipientsAdd(gptest.CliCtxWithFlags(ctx, t, map[string]string{"force": "true"}, "0xBEEFFEED")))
 	})
 
 	t.Run("remove recipient 0x82EBD945BE73F104", func(t *testing.T) {

internal/backend/crypto/gpg/cli/recipients.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"fmt"
 	"os"
 	"os/exec"
 	"strings"
@@ -33,9 +34,12 @@ func (g *GPG) ListRecipients(ctx context.Context) ([]string, error) {
 // FindRecipients searches for the given public keys.
 func (g *GPG) FindRecipients(ctx context.Context, search ...string) ([]string, error) {
 	kl, err := g.listKeys(ctx, "public", search...)
-	if err != nil || kl == nil {
+	if err != nil {
 		return nil, err
 	}
+	if kl == nil {
+		return nil, fmt.Errorf("no keys found for %v", search)
+	}
 
 	recp := kl.UseableKeys(gpg.IsAlwaysTrust(ctx)).Recipients()
 	if gpg.IsAlwaysTrust(ctx) {

internal/recipients/recipients.go

@@ -26,6 +26,15 @@ func New() *Recipients {
 	}
 }
 
+// Len returns the number of recipients.
+func (r *Recipients) Len() int {
+	if r == nil {
+		return 0
+	}
+
+	return len(r.r)
+}
+
 // IDs returns the key IDs.
 func (r *Recipients) IDs() []string {
 	res := maps.Keys(r.r)

internal/store/leaf/context_test.go

@@ -43,7 +43,7 @@ func TestFsckFunc(t *testing.T) {
 	assert.True(t, HasFsckFunc(WithFsckFunc(ctx, ffunc)))
 }
 
-func TestCheckRecipients(t *testing.T) {
+func TestCheckRecipientsCtx(t *testing.T) {
 	t.Parallel()
 
 	ctx := context.Background()

internal/store/leaf/recipients.go

@@ -12,6 +12,7 @@ import (
 	"github.com/gopasspw/gopass/internal/config"
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/internal/recipients"
+	"github.com/gopasspw/gopass/internal/set"
 	"github.com/gopasspw/gopass/internal/store"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
 	"github.com/gopasspw/gopass/pkg/debug"
@@ -26,6 +27,31 @@ const (
 // ErrInvalidHash indicates an outdated value of `recipients.hash`.
 var ErrInvalidHash = fmt.Errorf("recipients.hash invalid")
 
+// InvalidRecipientsError is a custom error type that contains a
+// list of invalid recipients with their check failures.
+type InvalidRecipientsError struct {
+	Invalid map[string]error
+}
+
+func (e InvalidRecipientsError) Error() string {
+	var sb strings.Builder
+
+	sb.WriteString("Invalid Recipients: ")
+	for _, k := range set.SortedKeys(e.Invalid) {
+		sb.WriteString(k)
+		sb.WriteString(": ")
+		sb.WriteString(e.Invalid[k].Error())
+		sb.WriteString(", ")
+	}
+
+	return sb.String()
+}
+
+// IsError returns true if this multi error contains any underlying errors.
+func (e InvalidRecipientsError) IsError() bool {
+	return len(e.Invalid) > 0
+}
+
 // Recipients returns the list of recipients of this store.
 func (s *Store) Recipients(ctx context.Context) []string {
 	rs, err := s.GetRecipients(ctx, "")
@@ -71,6 +97,42 @@ func (s *Store) RecipientsTree(ctx context.Context) map[string][]string {
 	return out
 }
 
+// CheckRecipients makes sure all existing recipients are valid.
+func (s *Store) CheckRecipients(ctx context.Context) error {
+	rs, err := s.GetRecipients(ctx, "")
+	if err != nil {
+		return fmt.Errorf("failed to read recipient list: %w", err)
+	}
+
+	er := InvalidRecipientsError{
+		Invalid: make(map[string]error, len(rs.IDs())),
+	}
+	for _, k := range rs.IDs() {
+		validKeys, err := s.crypto.FindRecipients(ctx, k)
+		if err != nil {
+			debug.Log("no GPG key info (unexpected) for %s: %s", k, err)
+			er.Invalid[k] = err
+
+			continue
+		}
+
+		if len(validKeys) < 1 {
+			debug.Log("no valid keys (expired?) for %s", k)
+			er.Invalid[k] = fmt.Errorf("no valid keys (expired?)")
+
+			continue
+		}
+
+		debug.Log("valid keys found for %s", k)
+	}
+
+	if er.IsError() {
+		return er
+	}
+
+	return nil
+}
+
 // AddRecipient adds a new recipient to the list.
 func (s *Store) AddRecipient(ctx context.Context, id string) error {
 	rs, err := s.GetRecipients(ctx, "")

internal/store/leaf/recipients_test.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"runtime"
 	"sort"
 	"strings"
 	"testing"
@@ -16,6 +17,7 @@ import (
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/internal/recipients"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
+	"github.com/gopasspw/gopass/tests/gptest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -250,3 +252,30 @@ func TestListRecipients(t *testing.T) {
 
 	assert.Equal(t, "0xDEADBEEF", s.OurKeyID(ctx))
 }
+
+func TestCheckRecipients(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("test setup not supported on Windows")
+	}
+
+	u := gptest.NewGUnitTester(t)
+
+	ctx := context.Background()
+	ctx = ctxutil.WithTerminal(ctx, false)
+	ctx = backend.WithCryptoBackend(ctx, backend.GPGCLI)
+
+	obuf := &bytes.Buffer{}
+	out.Stdout = obuf
+
+	defer func() {
+		out.Stdout = os.Stdout
+	}()
+
+	s, err := New(ctx, "", u.StoreDir(""))
+	require.NoError(t, err)
+
+	assert.NoError(t, s.CheckRecipients(ctx))
+
+	u.AddExpiredRecipient()
+	assert.Error(t, s.CheckRecipients(ctx))
+}

internal/store/root/recipients.go

@@ -20,6 +20,14 @@ func (r *Store) ListRecipients(ctx context.Context, store string) []string {
 	return sub.Recipients(ctx)
 }
 
+// CheckRecipients checks all current recipients to make sure that they are
+// valid, e.g. not expired.
+func (r *Store) CheckRecipients(ctx context.Context, store string) error {
+	sub, _ := r.getStore(store)
+
+	return sub.CheckRecipients(ctx)
+}
+
 // AddRecipient adds a single recipient to the given store.
 func (r *Store) AddRecipient(ctx context.Context, store, rec string) error {
 	sub, _ := r.getStore(store)

tests/gptest/gunit.go

@@ -6,9 +6,11 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/armor"
+	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	aclip "github.com/atotto/clipboard"
 	"github.com/gopasspw/gopass/tests/can"
 	"github.com/stretchr/testify/assert"
@@ -100,8 +102,7 @@ func (u GUnit) recipients() []byte {
 	return []byte(strings.Join(u.Recipients, "\n"))
 }
 
-// InitStore initializes the test store.
-func (u GUnit) InitStore(name string) error {
+func (u GUnit) writeRecipients(name string) error {
 	dir := u.StoreDir(name)
 	if err := os.MkdirAll(dir, 0o700); err != nil {
 		return fmt.Errorf("failed to create store dir %s: %w", dir, err)
@@ -114,10 +115,21 @@ func (u GUnit) InitStore(name string) error {
 		return fmt.Errorf("failed to write IDFile %s: %w", fn, err)
 	}
 
+	return nil
+}
+
+// InitStore initializes the test store.
+func (u GUnit) InitStore(name string) error {
+	if err := u.writeRecipients(name); err != nil {
+		return fmt.Errorf("failed to write recipients: %w", err)
+	}
+
 	if err := can.WriteTo(u.GPGHome()); err != nil {
 		return err
 	}
 
+	dir := u.StoreDir(name)
+
 	// write embedded public keys to the store so we can import them
 	el := can.EmbeddedKeyRing()
 	for _, e := range el {
@@ -145,3 +157,42 @@ func (u GUnit) InitStore(name string) error {
 
 	return nil
 }
+
+func (u *GUnit) AddExpiredRecipient() string {
+	u.t.Helper()
+
+	e, err := openpgp.NewEntity("Expired", "", "expired@example.com", &packet.Config{
+		RSABits: 4096,
+	})
+	require.NoError(u.t, err)
+
+	for _, id := range e.Identities {
+		err := id.SelfSignature.SignUserId(id.UserId.Id, e.PrimaryKey, e.PrivateKey, &packet.Config{
+			SigLifetimeSecs: 1, // we can not use negative or zero here
+		})
+		require.NoError(u.t, err)
+	}
+
+	el := can.EmbeddedKeyRing()
+	el = append(el, e)
+
+	fn := filepath.Join(u.GPGHome(), "pubring.gpg")
+	fh, err := os.Create(fn)
+	require.NoError(u.t, err)
+
+	for _, e := range el {
+		require.NoError(u.t, e.Serialize(fh))
+		// u.t.Logf("wrote %X to %s", e.PrimaryKey.Fingerprint, fn)
+	}
+	require.NoError(u.t, fh.Close())
+
+	// wait for the key to expire
+	time.Sleep(time.Second)
+
+	id := fmt.Sprintf("%X", e.PrimaryKey.Fingerprint)
+	u.Recipients = append(u.Recipients, id)
+
+	require.NoError(u.t, u.writeRecipients(""))
+
+	return id
+}