keys: verify the threshold a generated key actually has, and fail closed

GenerateValidatorKeys validated req.Threshold (>=2, parties >= threshold) and then never sent it — mpc.KeygenRequest has no threshold field, because the MPC ring's own --threshold decides the CGGMP21 polynomial degree. The request was advisory; the result was trusted blind. That was not a theoretical gap. mpcd's KeygenResult carried neither threshold nor participants, so `Threshold: blsResult.Threshold` and `Parties: len(blsResult.Participants)` both evaluated to zero. Every validator key set on record reads 0-of-0 while its caller requested 3-of-5 and passed validation. The unit tests missed it because the mock server returned threshold:3 and five participants — it told the truth the real ring never did. Add verifyThreshold: one function, called for both the BLS and Corona keys. It refuses a mismatch in either direction, and refuses just as firmly when the ring declines to state what it did — silently trusting an unstated threshold is the exact failure being guarded. Nothing is written to the store on refusal, so a rejected key leaves behind no record asserting a property never established. DEPLOY ORDER: requires mpcd >= v1.17.15 (luxfi/mpc 81be6b9), which is what began reporting these fields. Against an older ring every keygen now fails closed with an explicit "upgrade the MPC ring" error instead of silently recording 0-of-0. Roll the MPC quorums first. Tests: 5 refusal cases (unreported, weaker, degree-0, fewer parties, stronger) each asserting nothing is stored, plus a positive control proving a matching 3-of-5 is accepted and recorded as 3-of-5. Co-authored-by: Hanzo Dev <dev@hanzo.ai>
2026-07-27 03:38:31 +00:00 · 2026-07-25 16:42:10 -07:00
parent 0c3eca807e
commit 56debeba46
2 changed files with 195 additions and 0 deletions
@@ -118,6 +118,17 @@ func (m *Manager) GenerateValidatorKeys(ctx context.Context, req GenerateRequest
 		return nil, fmt.Errorf("keys: corona keygen failed (orphaned bls wallet %s): %w", blsResult.WalletID, err)
 	}

+	// Verify we got the threshold we asked for. Keygen takes no per-request
+	// threshold — the MPC ring's own --threshold governs — so requesting
+	// 3-of-5 and receiving something weaker is silent unless we check.
+	// Fail closed: an explicit error beats recording an unverified key.
+	if err := verifyThreshold("bls", req, blsResult.Threshold, blsResult.Participants); err != nil {
+		return nil, err
+	}
+	if err := verifyThreshold("corona", req, coronaResult.Threshold, coronaResult.Participants); err != nil {
+		return nil, err
+	}
+
 	blsPub := ""
 	if blsResult.ECDSAPubkey != nil {
 		blsPub = *blsResult.ECDSAPubkey
@@ -148,6 +159,37 @@ func (m *Manager) GenerateValidatorKeys(ctx context.Context, req GenerateRequest
 	return ks, nil
 }

+// verifyThreshold checks that a freshly generated key actually has the t-of-n
+// the caller asked for, and refuses the key otherwise.
+//
+// Why this must exist: mpc.KeygenRequest has no threshold field. The MPC ring's
+// own --threshold decides the CGGMP21 polynomial degree, so GenerateValidatorKeys
+// validating req.Threshold proves nothing about the key it gets back — the
+// request is advisory at best. Before mpcd reported these values it emitted
+// neither field, so every key set was stored as Threshold:0 Parties:0 while its
+// caller had asked for 3-of-5 and passed validation.
+//
+// Fail closed in both directions: a mismatch is an error, and so is a ring that
+// declines to state what it did. Silently trusting an unstated threshold is the
+// exact failure this guards.
+func verifyThreshold(keyName string, req GenerateRequest, gotThreshold int, gotParticipants []string) error {
+	if gotThreshold == 0 && len(gotParticipants) == 0 {
+		return fmt.Errorf(
+			"keys: %s keygen did not report its threshold or party set, so the requested %d-of-%d cannot be verified; "+
+				"upgrade the MPC ring to mpcd >= v1.17.15 (older builds omit both fields and this silently recorded 0-of-0)",
+			keyName, req.Threshold, req.Parties)
+	}
+	if gotThreshold != req.Threshold {
+		return fmt.Errorf("keys: %s keygen produced a %d-of-%d key but %d-of-%d was requested; refusing to record it",
+			keyName, gotThreshold, len(gotParticipants), req.Threshold, req.Parties)
+	}
+	if len(gotParticipants) != req.Parties {
+		return fmt.Errorf("keys: %s keygen produced a %d-of-%d key but %d-of-%d was requested; refusing to record it",
+			keyName, gotThreshold, len(gotParticipants), req.Threshold, req.Parties)
+	}
+	return nil
+}
+
 // SignWithBLS signs a message using the validator's BLS key via MPC threshold signing.
 func (m *Manager) SignWithBLS(ctx context.Context, validatorID string, message []byte) (*SignResponse, error) {
 	ks, err := m.store.Get(validatorID)
@@ -0,0 +1,153 @@
+// Copyright (C) 2026, Lux Industries Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package keys
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// mpcServerReporting returns a mock MPC server whose keygen response reports the
+// given threshold and participant set — i.e. it lets a test say "the ring
+// actually produced THIS" independently of what the caller requested.
+//
+// A nil participants slice with threshold 0 models a pre-v1.17.15 mpcd, which
+// omitted both fields entirely. That is the shape that made every real
+// validator key set record 0-of-0 while its caller asked for 3-of-5.
+func mpcServerReporting(t *testing.T, threshold int, participants []string) *httptest.Server {
+	t.Helper()
+	n := 0
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodPost || !strings.Contains(r.URL.Path, "/wallets") {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		n++
+		body := map[string]interface{}{
+			"wallet_id":     "wallet-" + string(rune('0'+n)),
+			"vault_id":      "vault-1",
+			"ecdsa_pub_key": "04pubkey",
+			"eddsa_pub_key": "edpub",
+		}
+		if threshold != 0 {
+			body["threshold"] = threshold
+		}
+		if participants != nil {
+			body["participants"] = participants
+		}
+		w.WriteHeader(http.StatusCreated)
+		_ = json.NewEncoder(w).Encode(body)
+	}))
+}
+
+func nodes(n int) []string {
+	out := make([]string, n)
+	for i := range out {
+		out[i] = "node" + string(rune('0'+i))
+	}
+	return out
+}
+
+// TestGenerateRefusesUnverifiableThreshold is the regression guard for the KMS
+// half of the 1-of-n incident.
+//
+// GenerateValidatorKeys validates req.Threshold and then never sends it —
+// mpc.KeygenRequest has no threshold field, because the MPC ring's own
+// --threshold decides the CGGMP21 degree. So the request is advisory, and the
+// only real check is comparing what came back. Each case below is a way that
+// comparison can fail; every one must refuse the key rather than record it.
+func TestGenerateRefusesUnverifiableThreshold(t *testing.T) {
+	cases := []struct {
+		name         string
+		threshold    int
+		participants []string
+		wantErr      string
+	}{
+		{
+			name:         "ring reports nothing (pre-v1.17.15 mpcd)",
+			threshold:    0,
+			participants: nil,
+			wantErr:      "cannot be verified",
+		},
+		{
+			name:         "weaker threshold than requested",
+			threshold:    2,
+			participants: nodes(5),
+			wantErr:      "refusing to record it",
+		},
+		{
+			name:         "degree-0: a single node can sign",
+			threshold:    1,
+			participants: nodes(5),
+			wantErr:      "refusing to record it",
+		},
+		{
+			name:         "fewer parties than requested",
+			threshold:    3,
+			participants: nodes(3),
+			wantErr:      "refusing to record it",
+		},
+		{
+			name:         "stronger than requested is still a mismatch",
+			threshold:    4,
+			participants: nodes(5),
+			wantErr:      "refusing to record it",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			srv := mpcServerReporting(t, tc.threshold, tc.participants)
+			defer srv.Close()
+
+			store := newMemStore()
+			mgr := NewManager(newTestMPCClient(srv.URL), store, "vault-1")
+
+			ks, err := mgr.GenerateValidatorKeys(context.Background(), GenerateRequest{
+				ValidatorID: "val-1",
+				Threshold:   3,
+				Parties:     5,
+			})
+			if err == nil {
+				t.Fatalf("expected refusal for %d-of-%d, got key set %+v",
+					tc.threshold, len(tc.participants), ks)
+			}
+			if !strings.Contains(err.Error(), tc.wantErr) {
+				t.Fatalf("error %q does not contain %q", err.Error(), tc.wantErr)
+			}
+			// A refused key must leave nothing behind — a stored record would
+			// assert a security property that was never established.
+			if _, gerr := store.Get("val-1"); gerr == nil {
+				t.Fatal("refused key set was still written to the store")
+			}
+		})
+	}
+}
+
+// TestGenerateAcceptsMatchingThreshold is the positive control: when the ring
+// reports exactly what was asked for, the key is accepted and the recorded
+// t-of-n reflects reality rather than a zero value.
+func TestGenerateAcceptsMatchingThreshold(t *testing.T) {
+	srv := mpcServerReporting(t, 3, nodes(5))
+	defer srv.Close()
+
+	store := newMemStore()
+	mgr := NewManager(newTestMPCClient(srv.URL), store, "vault-1")
+
+	ks, err := mgr.GenerateValidatorKeys(context.Background(), GenerateRequest{
+		ValidatorID: "val-1",
+		Threshold:   3,
+		Parties:     5,
+	})
+	if err != nil {
+		t.Fatalf("generate: %v", err)
+	}
+	if ks.Threshold != 3 || ks.Parties != 5 {
+		t.Fatalf("recorded %d-of-%d, want 3-of-5", ks.Threshold, ks.Parties)
+	}
+}