From 5d5e83d789e0d61a3d43ee50e093a5eb6fd57e07 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Sat, 24 Dec 2022 19:16:41 +0100 Subject: [PATCH] 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 * Add test for CheckRecipients with an invalid key. Signed-off-by: Dominik Schulz * Add custom error type and a better error message. Signed-off-by: Dominik Schulz * Initialize InvalidRecipientsError Signed-off-by: Dominik Schulz * Skip CheckRecipients tests on Windows Signed-off-by: Dominik Schulz --- docs/faq.md | 7 +++ internal/action/recipients.go | 6 ++ internal/action/recipients_test.go | 2 +- internal/backend/crypto/gpg/cli/recipients.go | 6 +- internal/recipients/recipients.go | 9 +++ internal/store/leaf/context_test.go | 2 +- internal/store/leaf/recipients.go | 62 +++++++++++++++++++ internal/store/leaf/recipients_test.go | 29 +++++++++ internal/store/root/recipients.go | 8 +++ tests/gptest/gunit.go | 55 +++++++++++++++- 10 files changed, 181 insertions(+), 5 deletions(-) diff --git a/docs/faq.md b/docs/faq.md index c9353f0bf4..c7dcade457 100644 --- a/docs/faq.md +++ b/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. diff --git a/internal/action/recipients.go b/internal/action/recipients.go index 74aa5c8d66..8a09fd7d47 100644 --- a/internal/action/recipients.go +++ b/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. diff --git a/internal/action/recipients_test.go b/internal/action/recipients_test.go index 8ac8e90405..3306361e67 100644 --- a/internal/action/recipients_test.go +++ b/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) { diff --git a/internal/backend/crypto/gpg/cli/recipients.go b/internal/backend/crypto/gpg/cli/recipients.go index cb8953e06d..6930dc2580 100644 --- a/internal/backend/crypto/gpg/cli/recipients.go +++ b/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) { diff --git a/internal/recipients/recipients.go b/internal/recipients/recipients.go index 40212be49a..8c6e52456e 100644 --- a/internal/recipients/recipients.go +++ b/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) diff --git a/internal/store/leaf/context_test.go b/internal/store/leaf/context_test.go index 2f70f383ea..b67fc571a6 100644 --- a/internal/store/leaf/context_test.go +++ b/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() diff --git a/internal/store/leaf/recipients.go b/internal/store/leaf/recipients.go index 405c92183c..cdb91524ba 100644 --- a/internal/store/leaf/recipients.go +++ b/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, "") diff --git a/internal/store/leaf/recipients_test.go b/internal/store/leaf/recipients_test.go index 1c2a8f65bc..2d422014cf 100644 --- a/internal/store/leaf/recipients_test.go +++ b/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)) +} diff --git a/internal/store/root/recipients.go b/internal/store/root/recipients.go index 15cb046011..8832556645 100644 --- a/internal/store/root/recipients.go +++ b/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) diff --git a/tests/gptest/gunit.go b/tests/gptest/gunit.go index 07d051fe9b..e3b3562a2c 100644 --- a/tests/gptest/gunit.go +++ b/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 +}