Skip to content

Commit 8af9cd2

Browse files
authored
fix: F-2026-18190 | [Dual Defense] Obsolete TSS Keyshares Survive Quorum Change Without Deletion or Retirement (#305)
* fix: delete keyshares superseded by quorum change or key refresh (F-2026-18190) * fix: resolve keyshare pubkeys per share instead of paging full key history (F-2026-18190) * refactor: rename keysharegc to keysharesweeper, sweep hourly (F-2026-18190) * refactor: move keyshare sweeper into keyshare package, guard Start with sync.Once (F-2026-18190) * refactor: sweep keyshares daily and run once at start (F-2026-18190) * test: cover GetKeyByID and guard nil query response (F-2026-18190) * refactor: drop redundant pending-process and fund-migration guards from keyshare sweeper (F-2026-18190)
1 parent d28d095 commit 8af9cd2

7 files changed

Lines changed: 654 additions & 5 deletions

File tree

universalClient/pushcore/pushCore.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,27 @@ func (c *Client) GetCurrentKey(ctx context.Context) (*utsstypes.TssKey, error) {
211211
)
212212
}
213213

214+
// GetKeyByID retrieves a single TSS key from the on-chain key history.
215+
// Returns an error if the key ID is not in the history.
216+
func (c *Client) GetKeyByID(ctx context.Context, keyID string) (*utsstypes.TssKey, error) {
217+
return retryWithRoundRobin(
218+
len(c.utssClients),
219+
&c.rr,
220+
func(idx int) (*utsstypes.TssKey, error) {
221+
resp, err := c.utssClients[idx].KeyById(ctx, &utsstypes.QueryKeyByIdRequest{KeyId: keyID})
222+
if err != nil {
223+
return nil, err
224+
}
225+
if resp == nil || resp.Key == nil {
226+
return nil, fmt.Errorf("pushcore: TSS key %s not found", keyID)
227+
}
228+
return resp.Key, nil
229+
},
230+
"GetKeyByID",
231+
c.logger,
232+
)
233+
}
234+
214235
// GetGasPrice retrieves the median gas price for a specific chain from the on-chain oracle.
215236
func (c *Client) GetGasPrice(ctx context.Context, chainID string) (*big.Int, error) {
216237
if chainID == "" {

universalClient/pushcore/pushCore_test.go

Lines changed: 70 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package pushcore
22

33
import (
44
"context"
5+
"errors"
56
"math/big"
67
"testing"
78

@@ -965,10 +966,11 @@ func (m *mockUValidatorQueryClient) UniversalValidator(ctx context.Context, req
965966

966967
type mockUTSSQueryClient struct {
967968
utsstypes.QueryClient
968-
currentKeyResp *utsstypes.QueryCurrentKeyResponse
969-
pendingTssEventsResp *utsstypes.QueryAllPendingTssEventsResponse
970-
pendingFundMigrationsResp *utsstypes.QueryPendingFundMigrationsResponse
971-
err error
969+
currentKeyResp *utsstypes.QueryCurrentKeyResponse
970+
keyByIdResp *utsstypes.QueryKeyByIdResponse
971+
pendingTssEventsResp *utsstypes.QueryAllPendingTssEventsResponse
972+
pendingFundMigrationsResp *utsstypes.QueryPendingFundMigrationsResponse
973+
err error
972974
}
973975

974976
func (m *mockUTSSQueryClient) CurrentKey(ctx context.Context, req *utsstypes.QueryCurrentKeyRequest, opts ...grpc.CallOption) (*utsstypes.QueryCurrentKeyResponse, error) {
@@ -993,7 +995,10 @@ func (m *mockUTSSQueryClient) PendingFundMigrations(ctx context.Context, req *ut
993995
}
994996

995997
func (m *mockUTSSQueryClient) KeyById(ctx context.Context, req *utsstypes.QueryKeyByIdRequest, opts ...grpc.CallOption) (*utsstypes.QueryKeyByIdResponse, error) {
996-
return nil, nil
998+
if m.err != nil {
999+
return nil, m.err
1000+
}
1001+
return m.keyByIdResp, nil
9971002
}
9981003

9991004
type mockTxServiceClient struct {
@@ -1084,3 +1089,63 @@ func (m *mockAuthAccountQueryClient) Account(ctx context.Context, req *authtypes
10841089
}
10851090
return m.accountResp, nil
10861091
}
1092+
1093+
func TestClient_GetKeyByID(t *testing.T) {
1094+
logger := zerolog.Nop()
1095+
1096+
t.Run("no endpoints configured", func(t *testing.T) {
1097+
client := &Client{logger: logger, utssClients: []utsstypes.QueryClient{}}
1098+
1099+
key, err := client.GetKeyByID(context.Background(), "key-123")
1100+
require.Error(t, err)
1101+
assert.Contains(t, err.Error(), "no endpoints configured")
1102+
assert.Nil(t, key)
1103+
})
1104+
1105+
t.Run("successful query returns key", func(t *testing.T) {
1106+
mockClient := &mockUTSSQueryClient{
1107+
keyByIdResp: &utsstypes.QueryKeyByIdResponse{
1108+
Key: &utsstypes.TssKey{KeyId: "key-123", TssPubkey: "0xpub"},
1109+
},
1110+
}
1111+
client := &Client{logger: logger, utssClients: []utsstypes.QueryClient{mockClient}}
1112+
1113+
key, err := client.GetKeyByID(context.Background(), "key-123")
1114+
require.NoError(t, err)
1115+
require.NotNil(t, key)
1116+
assert.Equal(t, "key-123", key.KeyId)
1117+
assert.Equal(t, "0xpub", key.TssPubkey)
1118+
})
1119+
1120+
t.Run("unknown key id errors", func(t *testing.T) {
1121+
mockClient := &mockUTSSQueryClient{
1122+
keyByIdResp: &utsstypes.QueryKeyByIdResponse{Key: nil},
1123+
}
1124+
client := &Client{logger: logger, utssClients: []utsstypes.QueryClient{mockClient}}
1125+
1126+
key, err := client.GetKeyByID(context.Background(), "missing")
1127+
require.Error(t, err)
1128+
assert.Contains(t, err.Error(), "not found")
1129+
assert.Nil(t, key)
1130+
})
1131+
1132+
// A nil response with a nil error must not panic.
1133+
t.Run("nil response errors", func(t *testing.T) {
1134+
mockClient := &mockUTSSQueryClient{keyByIdResp: nil}
1135+
client := &Client{logger: logger, utssClients: []utsstypes.QueryClient{mockClient}}
1136+
1137+
key, err := client.GetKeyByID(context.Background(), "key-123")
1138+
require.Error(t, err)
1139+
assert.Contains(t, err.Error(), "not found")
1140+
assert.Nil(t, key)
1141+
})
1142+
1143+
t.Run("query error propagates", func(t *testing.T) {
1144+
mockClient := &mockUTSSQueryClient{err: errors.New("rpc down")}
1145+
client := &Client{logger: logger, utssClients: []utsstypes.QueryClient{mockClient}}
1146+
1147+
key, err := client.GetKeyByID(context.Background(), "key-123")
1148+
require.Error(t, err)
1149+
assert.Nil(t, key)
1150+
})
1151+
}

universalClient/tss/keyshare/manager.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,59 @@ func (m *Manager) Exists(id string) (bool, error) {
144144
return true, nil
145145
}
146146

147+
// List returns the IDs of all stored keyshares.
148+
func (m *Manager) List() ([]string, error) {
149+
entries, err := os.ReadDir(m.keysharesDir)
150+
if err != nil {
151+
if os.IsNotExist(err) {
152+
return nil, nil
153+
}
154+
return nil, fmt.Errorf("failed to read keyshares directory: %w", err)
155+
}
156+
157+
ids := make([]string, 0, len(entries))
158+
for _, e := range entries {
159+
if !e.IsDir() {
160+
ids = append(ids, e.Name())
161+
}
162+
}
163+
return ids, nil
164+
}
165+
166+
// Delete removes a stored keyshare. It overwrites the file with random bytes
167+
// before unlinking; on SSD/COW filesystems that is best-effort, so the real
168+
// protection remains the at-rest encryption. Deleting a missing ID is a no-op.
169+
func (m *Manager) Delete(id string) error {
170+
if id == "" {
171+
return ErrInvalidID
172+
}
173+
174+
if strings.Contains(id, "/") || strings.Contains(id, "\\") || strings.Contains(id, "..") {
175+
return fmt.Errorf("%w: id contains invalid characters", ErrInvalidID)
176+
}
177+
178+
filePath := filepath.Join(m.keysharesDir, id)
179+
info, err := os.Stat(filePath)
180+
if err != nil {
181+
if os.IsNotExist(err) {
182+
return nil
183+
}
184+
return fmt.Errorf("failed to stat keyshare file: %w", err)
185+
}
186+
187+
if info.Mode().IsRegular() && info.Size() > 0 {
188+
scratch := make([]byte, info.Size())
189+
if _, rerr := rand.Read(scratch); rerr == nil {
190+
_ = os.WriteFile(filePath, scratch, filePerms)
191+
}
192+
}
193+
194+
if err := os.Remove(filePath); err != nil && !os.IsNotExist(err) {
195+
return fmt.Errorf("failed to remove keyshare file: %w", err)
196+
}
197+
return nil
198+
}
199+
147200
// encrypt encrypts keyshare data using AES-256-GCM with a password-derived key.
148201
// Returns encrypted data in format: [salt(32) || nonce(12) || ciphertext || tag(16)]
149202
func (m *Manager) encrypt(keyshareData []byte) ([]byte, error) {

universalClient/tss/keyshare/manager_test.go

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,3 +516,133 @@ func TestManager_EncryptDecrypt(t *testing.T) {
516516
}
517517
})
518518
}
519+
520+
func TestList(t *testing.T) {
521+
t.Run("empty directory", func(t *testing.T) {
522+
mgr, err := NewManager(t.TempDir(), "pw")
523+
if err != nil {
524+
t.Fatalf("NewManager() error = %v", err)
525+
}
526+
ids, err := mgr.List()
527+
if err != nil {
528+
t.Fatalf("List() error = %v", err)
529+
}
530+
if len(ids) != 0 {
531+
t.Errorf("List() = %v, want empty", ids)
532+
}
533+
})
534+
535+
t.Run("returns stored ids", func(t *testing.T) {
536+
mgr, err := NewManager(t.TempDir(), "pw")
537+
if err != nil {
538+
t.Fatalf("NewManager() error = %v", err)
539+
}
540+
for _, id := range []string{"key-a", "key-b"} {
541+
if err := mgr.Store([]byte("share-"+id), id); err != nil {
542+
t.Fatalf("Store(%s) error = %v", id, err)
543+
}
544+
}
545+
ids, err := mgr.List()
546+
if err != nil {
547+
t.Fatalf("List() error = %v", err)
548+
}
549+
if len(ids) != 2 {
550+
t.Fatalf("List() returned %d ids, want 2", len(ids))
551+
}
552+
found := map[string]bool{}
553+
for _, id := range ids {
554+
found[id] = true
555+
}
556+
if !found["key-a"] || !found["key-b"] {
557+
t.Errorf("List() = %v, want key-a and key-b", ids)
558+
}
559+
})
560+
561+
t.Run("ignores subdirectories", func(t *testing.T) {
562+
tmpDir := t.TempDir()
563+
mgr, err := NewManager(tmpDir, "pw")
564+
if err != nil {
565+
t.Fatalf("NewManager() error = %v", err)
566+
}
567+
if err := os.MkdirAll(filepath.Join(mgr.keysharesDir, "nested"), dirPerms); err != nil {
568+
t.Fatalf("MkdirAll() error = %v", err)
569+
}
570+
ids, err := mgr.List()
571+
if err != nil {
572+
t.Fatalf("List() error = %v", err)
573+
}
574+
if len(ids) != 0 {
575+
t.Errorf("List() = %v, want empty (dirs ignored)", ids)
576+
}
577+
})
578+
}
579+
580+
func TestDelete(t *testing.T) {
581+
t.Run("removes stored keyshare", func(t *testing.T) {
582+
mgr, err := NewManager(t.TempDir(), "pw")
583+
if err != nil {
584+
t.Fatalf("NewManager() error = %v", err)
585+
}
586+
if err := mgr.Store([]byte("secret-share"), "key-1"); err != nil {
587+
t.Fatalf("Store() error = %v", err)
588+
}
589+
if err := mgr.Delete("key-1"); err != nil {
590+
t.Fatalf("Delete() error = %v", err)
591+
}
592+
if _, err := mgr.Get("key-1"); !errors.Is(err, ErrKeyshareNotFound) {
593+
t.Errorf("Get() after Delete error = %v, want ErrKeyshareNotFound", err)
594+
}
595+
exists, err := mgr.Exists("key-1")
596+
if err != nil {
597+
t.Fatalf("Exists() error = %v", err)
598+
}
599+
if exists {
600+
t.Error("Exists() = true after Delete, want false")
601+
}
602+
})
603+
604+
t.Run("missing id is a no-op", func(t *testing.T) {
605+
mgr, err := NewManager(t.TempDir(), "pw")
606+
if err != nil {
607+
t.Fatalf("NewManager() error = %v", err)
608+
}
609+
if err := mgr.Delete("never-stored"); err != nil {
610+
t.Errorf("Delete() on missing id error = %v, want nil", err)
611+
}
612+
})
613+
614+
t.Run("rejects invalid ids", func(t *testing.T) {
615+
mgr, err := NewManager(t.TempDir(), "pw")
616+
if err != nil {
617+
t.Fatalf("NewManager() error = %v", err)
618+
}
619+
for _, id := range []string{"", "../escape", "sub/dir", "back\\slash"} {
620+
if err := mgr.Delete(id); !errors.Is(err, ErrInvalidID) {
621+
t.Errorf("Delete(%q) error = %v, want ErrInvalidID", id, err)
622+
}
623+
}
624+
})
625+
626+
t.Run("leaves other keyshares intact", func(t *testing.T) {
627+
mgr, err := NewManager(t.TempDir(), "pw")
628+
if err != nil {
629+
t.Fatalf("NewManager() error = %v", err)
630+
}
631+
if err := mgr.Store([]byte("share-a"), "key-a"); err != nil {
632+
t.Fatalf("Store() error = %v", err)
633+
}
634+
if err := mgr.Store([]byte("share-b"), "key-b"); err != nil {
635+
t.Fatalf("Store() error = %v", err)
636+
}
637+
if err := mgr.Delete("key-a"); err != nil {
638+
t.Fatalf("Delete() error = %v", err)
639+
}
640+
got, err := mgr.Get("key-b")
641+
if err != nil {
642+
t.Fatalf("Get(key-b) error = %v", err)
643+
}
644+
if string(got) != "share-b" {
645+
t.Errorf("Get(key-b) = %q, want %q", got, "share-b")
646+
}
647+
})
648+
}

0 commit comments

Comments
 (0)