From 877ed48c1cb1709649ef860d0769ecb66c315010 Mon Sep 17 00:00:00 2001 From: maclane Date: Sun, 14 Jun 2026 10:43:01 -0400 Subject: [PATCH 1/4] feat(frost/roast): domain-separate evidence-snapshot + transition-message signatures The node's operator key signs LocalEvidenceSnapshot, TransitionMessage, and (on #4056) the signing package, and their bodies are wire-compatible, so a signature over one body could be replayed as a signature over another. Each signed body now prepends a UNIQUE domain tag to the bytes it signs and verifies, while the body that travels on the wire stays the bare serialized body. - Add localEvidenceSnapshotSignatureDomain + transitionMessageSignatureDomain, each beginning with byte 0x00 (an illegal protobuf tag, field 0) so the signed payload is undecodable as any protobuf message. This separates the domains in both directions without relying on field layout: a signature over one body cannot be accepted on another envelope (its decoder rejects the 0x00-leading body), and a genuine protobuf body starts >= 0x08 so its signature can never verify against domain||body. Matches the signing-package fix on #4056. - Split each type into bodyBytes() (the bare wire body) and SignableBytes() (domain || body). wireEnvelopeBytes / Marshal now embed bodyBytes(), and both Unmarshal reset the signable-bytes cache so a reused receiver never verifies against stale bytes. Rename signedBody -> bodyCache; add signaturePayloadCache. Sign and verify both flow through SignableBytes(), so the change is transparent to the sign sites and the wire envelope structure is unchanged. It DOES change the bytes signed, so signatures are not compatible with the pre-change protocol - all nodes must run the new code together (acceptable pre-mainnet; the external-audit gate stands). Tests pin the domain tagging, undecodability, prefix-free distinctness, bare wire body, and the reused-receiver cache reset. Co-Authored-By: Claude Opus 4.8 --- pkg/frost/roast/domain_separation_test.go | 160 ++++++++++++++++++++++ pkg/frost/roast/equivocation_test.go | 3 +- pkg/frost/roast/transition_message.go | 46 ++++--- pkg/frost/roast/wire.go | 120 ++++++++++++---- pkg/frost/roast/wire_test.go | 5 +- 5 files changed, 292 insertions(+), 42 deletions(-) create mode 100644 pkg/frost/roast/domain_separation_test.go diff --git a/pkg/frost/roast/domain_separation_test.go b/pkg/frost/roast/domain_separation_test.go new file mode 100644 index 0000000000..9b0695a2b5 --- /dev/null +++ b/pkg/frost/roast/domain_separation_test.go @@ -0,0 +1,160 @@ +package roast + +import ( + "bytes" + "testing" + + "google.golang.org/protobuf/proto" + + "github.com/keep-network/keep-core/pkg/frost/roast/gen/pb" +) + +// These pin the cross-protocol signature-confusion defense for the operator-key +// signed bodies: the signed bytes are domain-tagged and undecodable as +// protobuf, the bytes that travel on the wire stay the bare body, and the two +// body types use distinct tags so a signature over one can never verify as a +// signature over the other. + +func TestSnapshotSignableBytes_DomainSeparated(t *testing.T) { + snap := signedTestSnapshot(t, 7) + signable, err := snap.SignableBytes() + if err != nil { + t.Fatalf("signable: %v", err) + } + body, err := snap.bodyBytes() + if err != nil { + t.Fatalf("body: %v", err) + } + + // SignableBytes = snapshot domain tag || bare body. + if !bytes.HasPrefix(signable, localEvidenceSnapshotSignatureDomain) { + t.Fatal("snapshot signed bytes must carry the snapshot domain tag") + } + if !bytes.Equal(signable[len(localEvidenceSnapshotSignatureDomain):], body) { + t.Fatal("snapshot signed bytes must be the domain tag followed by the bare body") + } + + // The signed bytes begin with an illegal protobuf tag (field 0) and so are + // undecodable as any protobuf message - a snapshot signature can never be + // accepted on a transition (or other) envelope whose decoder parses the + // forged body. + if signable[0] != 0x00 { + t.Fatal("snapshot signed bytes must begin with an illegal protobuf tag (0x00)") + } + if err := proto.Unmarshal(signable, &pb.LocalEvidenceSnapshotBody{}); err == nil { + t.Fatal("snapshot signed bytes must not decode as a protobuf message") + } + + // The bare wire body, by contrast, carries no tag and IS a valid protobuf + // body - the domain tag never travels on the wire. + if bytes.HasPrefix(body, localEvidenceSnapshotSignatureDomain) { + t.Fatal("the wire body must not carry the domain tag") + } + if err := proto.Unmarshal(body, &pb.LocalEvidenceSnapshotBody{}); err != nil { + t.Fatalf("the bare wire body must be a valid protobuf body: %v", err) + } +} + +func TestTransitionSignableBytes_DomainSeparated(t *testing.T) { + msg := buildValidTransitionMessage() + signable, err := msg.SignableBytes() + if err != nil { + t.Fatalf("signable: %v", err) + } + body, err := msg.bodyBytes() + if err != nil { + t.Fatalf("body: %v", err) + } + + if !bytes.HasPrefix(signable, transitionMessageSignatureDomain) { + t.Fatal("transition signed bytes must carry the transition domain tag") + } + if !bytes.Equal(signable[len(transitionMessageSignatureDomain):], body) { + t.Fatal("transition signed bytes must be the domain tag followed by the bare body") + } + if signable[0] != 0x00 { + t.Fatal("transition signed bytes must begin with an illegal protobuf tag (0x00)") + } + if err := proto.Unmarshal(signable, &pb.TransitionMessageBody{}); err == nil { + t.Fatal("transition signed bytes must not decode as a protobuf message") + } + if bytes.HasPrefix(body, transitionMessageSignatureDomain) { + t.Fatal("the wire body must not carry the domain tag") + } + if err := proto.Unmarshal(body, &pb.TransitionMessageBody{}); err != nil { + t.Fatalf("the bare wire body must be a valid protobuf body: %v", err) + } +} + +func TestSignedBodyDomains_AreDistinctAndPrefixFree(t *testing.T) { + // Distinct, prefix-free tags make the signed-byte spaces of the two body + // types disjoint: domain_a || body_a == domain_b || body_b is impossible + // unless one tag is a prefix of the other, so a signature over one body can + // never verify as a signature over the other. + a := localEvidenceSnapshotSignatureDomain + b := transitionMessageSignatureDomain + if bytes.Equal(a, b) { + t.Fatal("each signed body type must use a distinct domain tag") + } + if bytes.HasPrefix(a, b) || bytes.HasPrefix(b, a) { + t.Fatal("no domain tag may be a prefix of another") + } + for _, tag := range [][]byte{a, b} { + if len(tag) == 0 || tag[0] != 0x00 { + t.Fatalf("domain tag %q must begin with an illegal protobuf tag (0x00)", tag) + } + } +} + +func TestSnapshotUnmarshal_ResetsSignableCache(t *testing.T) { + // A snapshot value reused across a SignableBytes call and then an Unmarshal + // must authenticate the newly decoded snapshot against the bytes it just + // received, never the stale cached payload. + reused := signedTestSnapshot(t, 7) + if _, err := reused.SignableBytes(); err != nil { // prime the cache + t.Fatalf("prime cache: %v", err) + } + + genuine := signedTestSnapshot(t, 9) + wire, err := genuine.Marshal() + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := reused.Unmarshal(wire); err != nil { + t.Fatalf("unmarshal into reused value: %v", err) + } + + got, _ := reused.SignableBytes() + want, _ := genuine.SignableBytes() + if !bytes.Equal(got, want) { + t.Fatal("Unmarshal must reset the snapshot signable-bytes cache") + } + if err := verifySnapshotSignature(fakeVerifier{}, reused); err != nil { + t.Fatalf("authenticate reused-decoded snapshot: %v", err) + } +} + +func TestTransitionUnmarshal_ResetsSignableCache(t *testing.T) { + reused := buildValidTransitionMessage() + if _, err := reused.SignableBytes(); err != nil { // prime the cache + t.Fatalf("prime cache: %v", err) + } + + // Decode a structurally different genuine bundle into the SAME value. + other := buildValidTransitionMessage() + other.CoordinatorIDValue = 2 + other.CoordinatorSignature = bytes.Repeat([]byte{0xcd}, 64) + wire, err := other.Marshal() + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := reused.Unmarshal(wire); err != nil { + t.Fatalf("unmarshal into reused value: %v", err) + } + + got, _ := reused.SignableBytes() + want, _ := other.SignableBytes() + if !bytes.Equal(got, want) { + t.Fatal("Unmarshal must reset the transition signable-bytes cache") + } +} diff --git a/pkg/frost/roast/equivocation_test.go b/pkg/frost/roast/equivocation_test.go index e4568ac47b..03de8c4972 100644 --- a/pkg/frost/roast/equivocation_test.go +++ b/pkg/frost/roast/equivocation_test.go @@ -94,7 +94,8 @@ func TestOwnSnapshotMutatedInBundle_RetainsBothSignedEnvelopes(t *testing.T) { mutated := *selfSubmission mutated.OperatorSignature = bytes.Repeat([]byte{0xff}, 64) // Fresh caches: the mutated copy is a distinct signed object. - mutated.signedBody = nil + mutated.bodyCache = nil + mutated.signaturePayloadCache = nil mutated.wireEnvelope = nil bundle := &TransitionMessage{ diff --git a/pkg/frost/roast/transition_message.go b/pkg/frost/roast/transition_message.go index f8499ad91f..11de4d1b5b 100644 --- a/pkg/frost/roast/transition_message.go +++ b/pkg/frost/roast/transition_message.go @@ -101,15 +101,20 @@ type LocalEvidenceSnapshot struct { // observed. Conflicts []ConflictEntry // OperatorSignature is the signer's operator-key signature over - // SignableBytes(): the serialized protobuf body of (senderID, - // attemptContextHash, overflows, rejects, conflicts). + // SignableBytes(): the snapshot domain tag followed by the serialized + // protobuf body of (senderID, attemptContextHash, overflows, rejects, + // conflicts). OperatorSignature []byte - // signedBody caches the exact serialized body bytes the - // OperatorSignature covers: marshaled once at signing time for - // self-authored snapshots, or the received bytes verbatim for - // parsed ones. Evidence fields must not be mutated once set. - signedBody []byte + // bodyCache caches the exact serialized body bytes carried on the wire: + // marshaled once at signing time for self-authored snapshots, or the + // received bytes verbatim for parsed ones. Evidence fields must not be + // mutated once set. + bodyCache []byte + // signaturePayloadCache caches the domain-tagged bytes the + // OperatorSignature covers (localEvidenceSnapshotSignatureDomain || + // bodyCache); rebuilt from bodyCache and never carried on the wire. + signaturePayloadCache []byte // wireEnvelope caches the exact on-wire envelope (body + // signature): the received bytes verbatim for parsed snapshots, // or built once after signing for self-authored ones. @@ -246,8 +251,11 @@ func (s *LocalEvidenceSnapshot) Unmarshal(data []byte) error { } snapshotFieldsFromBody(s, &body) s.OperatorSignature = append([]byte(nil), envelope.OperatorSignature...) - s.signedBody = append([]byte(nil), envelope.Body...) + s.bodyCache = append([]byte(nil), envelope.Body...) s.wireEnvelope = append([]byte(nil), data...) + // Clear any signable-bytes cache a prior SignableBytes call left on a + // reused receiver, so the next call rebuilds it from the received body. + s.signaturePayloadCache = nil return s.Validate() } @@ -326,15 +334,16 @@ type TransitionMessage struct { // Bundle is the canonical sorted-by-SenderID list of signed // evidence snapshots aggregated by the coordinator. Bundle []LocalEvidenceSnapshot - // CoordinatorSignature is the coordinator's operator-key - // signature over SignableBytes(): the serialized protobuf body - // embedding every snapshot's signed envelope verbatim. + // CoordinatorSignature is the coordinator's operator-key signature over + // SignableBytes(): the transition domain tag followed by the serialized + // protobuf body embedding every snapshot's signed envelope verbatim. CoordinatorSignature []byte - // signedBody and wireEnvelope cache exact bytes with the same - // semantics as the LocalEvidenceSnapshot caches. - signedBody []byte - wireEnvelope []byte + // bodyCache, signaturePayloadCache, and wireEnvelope cache exact bytes + // with the same semantics as the LocalEvidenceSnapshot caches. + bodyCache []byte + signaturePayloadCache []byte + wireEnvelope []byte } // CoordinatorID returns the coordinator member index as a @@ -373,7 +382,7 @@ func (m *TransitionMessage) Marshal() ([]byte, error) { "transition message: must be signed before wire encoding", ) } - body, err := m.SignableBytes() + body, err := m.bodyBytes() if err != nil { return nil, err } @@ -426,8 +435,11 @@ func (m *TransitionMessage) Unmarshal(data []byte) error { m.Bundle = append(m.Bundle, snapshot) } m.CoordinatorSignature = append([]byte(nil), envelope.CoordinatorSignature...) - m.signedBody = append([]byte(nil), envelope.Body...) + m.bodyCache = append([]byte(nil), envelope.Body...) m.wireEnvelope = append([]byte(nil), data...) + // Clear any signable-bytes cache left on a reused receiver (see + // LocalEvidenceSnapshot.Unmarshal). + m.signaturePayloadCache = nil return m.Validate() } diff --git a/pkg/frost/roast/wire.go b/pkg/frost/roast/wire.go index a7932aae49..08021d0811 100644 --- a/pkg/frost/roast/wire.go +++ b/pkg/frost/roast/wire.go @@ -19,6 +19,29 @@ import ( // on any serializer's canonical form, across protobuf library versions or // across languages. Producers marshal a body exactly once (at signing // time) and cache it; parsed messages cache the received bytes. +// +// Domain separation. Several signed bodies share the node's operator key and +// have wire-compatible layouts (a LocalEvidenceSnapshotBody, a +// TransitionMessageBody, and the signing-package body all begin with a field-1 +// tag and an attempt-context binding), so a signature over one body must not +// be acceptable as a signature over another. Each signed-body type therefore +// prepends a UNIQUE domain tag to the bytes it signs and verifies +// (SignableBytes), while the body that travels on the wire stays the bare +// serialized body (bodyBytes). +// +// Each tag BEGINS with byte 0x00 - an illegal protobuf tag (field number 0) - +// so the signed payload is undecodable as any protobuf message. That separates +// the domains in both directions without relying on field layout: a signature +// over one body cannot be replayed onto another envelope (whose decoder +// proto.Unmarshals and rejects the 0x00-leading body), and a genuine +// serialized protobuf body always begins with a valid tag (>= 0x08), so its +// signature can never verify against domain || body. The tags are NOT carried +// on the wire; signer and verifier prepend the same constant. (The signed +// signing-package envelope follows the same scheme with its own tag.) +var ( + localEvidenceSnapshotSignatureDomain = []byte("\x00roast/signed-evidence-snapshot/v1\x00") + transitionMessageSignatureDomain = []byte("\x00roast/signed-transition-message/v1\x00") +) func snapshotBodyMessage(s *LocalEvidenceSnapshot) *pb.LocalEvidenceSnapshotBody { body := &pb.LocalEvidenceSnapshotBody{ @@ -74,28 +97,54 @@ func snapshotFieldsFromBody(s *LocalEvidenceSnapshot, body *pb.LocalEvidenceSnap } } -// SignableBytes returns the exact byte stream the OperatorSignature covers: -// the serialized LocalEvidenceSnapshotBody. For a self-authored snapshot -// the body is marshaled once and cached - sign exactly what will be -// transmitted. For a snapshot parsed off the wire this returns the -// received body bytes verbatim - verify exactly what was received. The -// snapshot's evidence fields must not be mutated afterwards, and the -// returned slice is the internal cache - callers must not mutate it. -func (s *LocalEvidenceSnapshot) SignableBytes() ([]byte, error) { +// bodyBytes returns the exact serialized LocalEvidenceSnapshotBody - the body +// carried verbatim in the SignedLocalEvidenceSnapshot envelope. Marshaled once +// and cached for a self-authored snapshot; the received bytes verbatim for a +// parsed one. The returned slice is the internal cache - callers must not +// mutate it. +func (s *LocalEvidenceSnapshot) bodyBytes() ([]byte, error) { if s == nil { return nil, errors.New("roast: cannot encode a nil snapshot") } - if s.signedBody != nil { - return s.signedBody, nil + if s.bodyCache != nil { + return s.bodyCache, nil } body, err := proto.Marshal(snapshotBodyMessage(s)) if err != nil { return nil, fmt.Errorf("roast: marshal snapshot body: %w", err) } - s.signedBody = body + s.bodyCache = body return body, nil } +// SignableBytes returns the exact byte stream the OperatorSignature covers: +// the snapshot domain tag (localEvidenceSnapshotSignatureDomain) followed by +// the serialized LocalEvidenceSnapshotBody. The domain tag is a fixed constant +// prepended by both signer and verifier and is NOT carried on the wire - it +// domain-separates this signature from the node's other signed bodies (see the +// package comment). The body half is the bytes that travel: marshaled once for +// a self-authored snapshot, or the received body verbatim for a parsed one +// (verify exactly what was received). Evidence fields must not be mutated +// afterwards, and the returned slice is the internal cache - callers must not +// mutate it. +func (s *LocalEvidenceSnapshot) SignableBytes() ([]byte, error) { + if s == nil { + return nil, errors.New("roast: cannot encode a nil snapshot") + } + if s.signaturePayloadCache != nil { + return s.signaturePayloadCache, nil + } + body, err := s.bodyBytes() + if err != nil { + return nil, err + } + payload := make([]byte, 0, len(localEvidenceSnapshotSignatureDomain)+len(body)) + payload = append(payload, localEvidenceSnapshotSignatureDomain...) + payload = append(payload, body...) + s.signaturePayloadCache = payload + return payload, nil +} + // wireEnvelopeBytes returns the exact on-wire SignedLocalEvidenceSnapshot // envelope. For parsed snapshots this is the received envelope verbatim; // for self-authored snapshots it is built once (after signing) and cached, @@ -110,7 +159,7 @@ func (s *LocalEvidenceSnapshot) wireEnvelopeBytes() ([]byte, error) { "roast: snapshot must be signed before wire encoding", ) } - body, err := s.SignableBytes() + body, err := s.bodyBytes() if err != nil { return nil, err } @@ -125,19 +174,17 @@ func (s *LocalEvidenceSnapshot) wireEnvelopeBytes() ([]byte, error) { return envelope, nil } -// SignableBytes returns the exact byte stream the CoordinatorSignature -// covers: the serialized TransitionMessageBody, which embeds every -// snapshot's signed envelope verbatim. The coordinator's signature -// attests that these specific signed snapshots were assembled in this -// specific order. For a message parsed off the wire this returns the -// received body bytes verbatim. The returned slice is the internal -// cache - callers must not mutate it. -func (m *TransitionMessage) SignableBytes() ([]byte, error) { +// bodyBytes returns the exact serialized TransitionMessageBody, which embeds +// every snapshot's signed envelope verbatim - the body carried in the +// SignedTransitionMessage envelope. Built and cached once for a self-authored +// message; the received bytes verbatim for a parsed one. The returned slice is +// the internal cache - callers must not mutate it. +func (m *TransitionMessage) bodyBytes() ([]byte, error) { if m == nil { return nil, errors.New("roast: cannot encode a nil transition message") } - if m.signedBody != nil { - return m.signedBody, nil + if m.bodyCache != nil { + return m.bodyCache, nil } body := &pb.TransitionMessageBody{ AttemptContextHash: m.AttemptContextHash, @@ -154,6 +201,33 @@ func (m *TransitionMessage) SignableBytes() ([]byte, error) { if err != nil { return nil, fmt.Errorf("roast: marshal transition body: %w", err) } - m.signedBody = bodyBytes + m.bodyCache = bodyBytes return bodyBytes, nil } + +// SignableBytes returns the exact byte stream the CoordinatorSignature covers: +// the transition domain tag (transitionMessageSignatureDomain) followed by the +// serialized TransitionMessageBody. The domain tag is a fixed constant +// prepended by both signer and verifier and is NOT carried on the wire - it +// domain-separates the coordinator's bundle signature from its other signed +// bodies (see the package comment). The coordinator's signature attests that +// these specific signed snapshots were assembled in this specific order; for a +// message parsed off the wire the body half is the received bytes verbatim. +// The returned slice is the internal cache - callers must not mutate it. +func (m *TransitionMessage) SignableBytes() ([]byte, error) { + if m == nil { + return nil, errors.New("roast: cannot encode a nil transition message") + } + if m.signaturePayloadCache != nil { + return m.signaturePayloadCache, nil + } + body, err := m.bodyBytes() + if err != nil { + return nil, err + } + payload := make([]byte, 0, len(transitionMessageSignatureDomain)+len(body)) + payload = append(payload, transitionMessageSignatureDomain...) + payload = append(payload, body...) + m.signaturePayloadCache = payload + return payload, nil +} diff --git a/pkg/frost/roast/wire_test.go b/pkg/frost/roast/wire_test.go index e9ddf2e3e8..d20eb879a2 100644 --- a/pkg/frost/roast/wire_test.go +++ b/pkg/frost/roast/wire_test.go @@ -66,7 +66,10 @@ func TestSnapshotWire_ReceivedBytesPreservedVerbatim(t *testing.T) { func TestSnapshotWire_NonCanonicalEnvelopeEncodingSurvives(t *testing.T) { original := signedTestSnapshot(t, 7) - body, _ := original.SignableBytes() + // The wire body is the bare serialized body (NOT the domain-tagged + // SignableBytes); the signature over SignableBytes still verifies against + // it after decode. + body, _ := original.bodyBytes() // Handcraft an envelope with the fields in REVERSE tag order // (operator_signature before body) - a wire-legal but non-canonical From 27c06c923e434859df7ca99d822310258de36dfc Mon Sep 17 00:00:00 2001 From: maclane Date: Sun, 14 Jun 2026 10:47:54 -0400 Subject: [PATCH 2/4] docs(frost/roast): clarify domain-separation rationale (self-review) The package comment called all three signed bodies wire-compatible; in fact only TransitionMessageBody and the signing-package body share field-1 = attempt_context_hash. LocalEvidenceSnapshotBody's field 1 is sender_id (a varint), so it is only INCIDENTALLY separated - a difference a later proto change could erase, which is precisely why the domain tag makes the separation intentional. Comment-only. Co-Authored-By: Claude Opus 4.8 --- pkg/frost/roast/wire.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/frost/roast/wire.go b/pkg/frost/roast/wire.go index 08021d0811..5da542c134 100644 --- a/pkg/frost/roast/wire.go +++ b/pkg/frost/roast/wire.go @@ -21,13 +21,17 @@ import ( // time) and cache it; parsed messages cache the received bytes. // // Domain separation. Several signed bodies share the node's operator key and -// have wire-compatible layouts (a LocalEvidenceSnapshotBody, a -// TransitionMessageBody, and the signing-package body all begin with a field-1 -// tag and an attempt-context binding), so a signature over one body must not -// be acceptable as a signature over another. Each signed-body type therefore +// are structurally similar - each carries an attempt-context hash and a member +// index. The TransitionMessageBody and the signing-package body are outright +// wire-compatible (both have attempt_context_hash as a length-delimited field +// 1). The LocalEvidenceSnapshotBody is only INCIDENTALLY distinguished - its +// field 1 is sender_id (a varint), not a length-delimited field - a difference +// a later proto change could erase. So a signature over one body must not be +// acceptable as a signature over another. Each signed-body type therefore // prepends a UNIQUE domain tag to the bytes it signs and verifies -// (SignableBytes), while the body that travels on the wire stays the bare -// serialized body (bodyBytes). +// (SignableBytes), making the separation intentional rather than incidental, +// while the body that travels on the wire stays the bare serialized body +// (bodyBytes). // // Each tag BEGINS with byte 0x00 - an illegal protobuf tag (field number 0) - // so the signed payload is undecodable as any protobuf message. That separates From 4a47cb6935dc1a4e5baf000247bb288118bb948f Mon Sep 17 00:00:00 2001 From: maclane Date: Sun, 14 Jun 2026 11:27:54 -0400 Subject: [PATCH 3/4] docs(frost/roast): align evidence proto + RFC-21 with tagged signatures (review) Codex (PR #4057) flagged that the cross-language contract still documented snapshot and transition signatures as covering the bare body, while this PR makes Go sign/verify domain || body - so a non-Go implementation built from evidence.proto or RFC-21 would sign/verify different bytes and reject Go's evidence messages. Document the signed payload as `domain_tag || body` in evidence.proto (a file-level SIGNED PAYLOAD note with both exact tags + the four message comments) and in the RFC-21 wire-format decision. Regenerated evidence.pb.go is comment-only (descriptor and symbols unchanged). Same fix class as the signing-package proto contract on #4056. Co-Authored-By: Claude Opus 4.8 --- ...dinator-retry-and-transition-evidence.adoc | 36 ++++++++----- pkg/frost/roast/gen/pb/evidence.pb.go | 27 ++++++---- pkg/frost/roast/gen/pb/evidence.proto | 51 ++++++++++++------- 3 files changed, 73 insertions(+), 41 deletions(-) diff --git a/docs/rfc/rfc-21-roast-coordinator-retry-and-transition-evidence.adoc b/docs/rfc/rfc-21-roast-coordinator-retry-and-transition-evidence.adoc index 04f5084fb9..d66bfd2218 100644 --- a/docs/rfc/rfc-21-roast-coordinator-retry-and-transition-evidence.adoc +++ b/docs/rfc/rfc-21-roast-coordinator-retry-and-transition-evidence.adoc @@ -669,20 +669,30 @@ optimization given the current key model. (`pkg/frost/roast/gen/pb/evidence.proto`), routed via the `net.Message` interface.* -Evidence signatures cover exact serialized body bytes, and those -bytes travel verbatim: a snapshot is +Evidence signatures cover a domain-separated byte stream -- a +fixed per-message-type tag followed by the exact serialized body -- +while the body itself travels verbatim. A snapshot is `SignedLocalEvidenceSnapshot{body, operator_signature}` where -`body` is the serialized `LocalEvidenceSnapshotBody`, and a -transition message is `SignedTransitionMessage{body, -coordinator_signature}` whose body embeds every member's signed -snapshot envelope verbatim (`repeated bytes signed_snapshots`). -A verifier checks each signature over the bytes it received and -only then parses them; nothing in the evidence chain is ever -re-encoded. Signature validity therefore never depends on any -serializer's canonical form -- across protobuf library versions -or across languages (the Phase 7 Rust signer verifies and parses -these same bytes). Producers marshal a body exactly once, at -signing time, and transmit those bytes. +`body` is the serialized `LocalEvidenceSnapshotBody` and the +operator signature covers `domain_tag || body`; a transition +message is `SignedTransitionMessage{body, coordinator_signature}` +whose body embeds every member's signed snapshot envelope verbatim +(`repeated bytes signed_snapshots`) and whose coordinator signature +covers `domain_tag || body`. Each `domain_tag` begins with byte +0x00 -- an illegal protobuf tag (field 0) -- so the signed payload +is undecodable as any protobuf message, and each message type uses +a distinct tag, so a signature over one body can never be accepted +for another (snapshot: `0x00 "roast/signed-evidence-snapshot/v1" +0x00`; transition: `0x00 "roast/signed-transition-message/v1" +0x00`; the Phase 7 signing package uses its own tag). The tag is +not carried on the wire -- only the body is. A verifier re-derives +the tag, checks the signature over the tag plus the body bytes it +received, and only then parses the body; nothing in the evidence +chain is ever re-encoded. Signature validity therefore never +depends on any serializer's canonical form -- across protobuf +library versions or across languages (the Phase 7 Rust signer +verifies and parses these same bytes). Producers marshal a body +exactly once, at signing time, and transmit those bytes. The original Phase 3 implementation used canonical JSON (`json.Marshal` over field-order-stable structs) as the signed diff --git a/pkg/frost/roast/gen/pb/evidence.pb.go b/pkg/frost/roast/gen/pb/evidence.pb.go index e026531675..d53f4e7d5d 100644 --- a/pkg/frost/roast/gen/pb/evidence.pb.go +++ b/pkg/frost/roast/gen/pb/evidence.pb.go @@ -20,8 +20,9 @@ const ( _ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20) ) -// The byte stream a signer's operator key signs. Carried as exact bytes in -// SignedLocalEvidenceSnapshot.body. +// The evidence-snapshot body. Carried verbatim as +// SignedLocalEvidenceSnapshot.body; the operator signature covers the +// domain-tagged form of these bytes (domain_tag || body), not the bare bytes. type LocalEvidenceSnapshotBody struct { state protoimpl.MessageState `protogen:"open.v1"` SenderId uint32 `protobuf:"varint,1,opt,name=sender_id,json=senderId,proto3" json:"sender_id,omitempty"` @@ -267,8 +268,9 @@ func (x *ConflictEntry) GetCount() uint64 { return 0 } -// The on-wire snapshot message: exact signed body bytes plus the operator -// signature over them. +// The on-wire snapshot message: the exact serialized LocalEvidenceSnapshotBody +// bytes plus the operator signature, which covers the domain-tagged body +// (domain_tag || body), not the bare body field. type SignedLocalEvidenceSnapshot struct { state protoimpl.MessageState `protogen:"open.v1"` Body []byte `protobuf:"bytes,1,opt,name=body,proto3" json:"body,omitempty"` @@ -321,11 +323,13 @@ func (x *SignedLocalEvidenceSnapshot) GetOperatorSignature() []byte { return nil } -// The byte stream the elected coordinator signs. signed_snapshots carries -// each member's SignedLocalEvidenceSnapshot envelope verbatim as received, -// so the coordinator attests to the exact signed snapshots it assembled, -// in order, and downstream verifiers re-check the operator signatures over -// those same exact bytes. +// The transition-message body. signed_snapshots carries each member's +// SignedLocalEvidenceSnapshot envelope verbatim as received, so the +// coordinator attests to the exact signed snapshots it assembled, in order, +// and downstream verifiers re-check the operator signatures over those same +// exact bytes. Carried verbatim as SignedTransitionMessage.body; the +// coordinator signature covers the domain-tagged form of these bytes +// (domain_tag || body), not the bare bytes. type TransitionMessageBody struct { state protoimpl.MessageState `protogen:"open.v1"` AttemptContextHash []byte `protobuf:"bytes,1,opt,name=attempt_context_hash,json=attemptContextHash,proto3" json:"attempt_context_hash,omitempty"` @@ -386,8 +390,9 @@ func (x *TransitionMessageBody) GetSignedSnapshots() [][]byte { return nil } -// The on-wire transition message: exact signed body bytes plus the -// coordinator signature over them. +// The on-wire transition message: the exact serialized TransitionMessageBody +// bytes plus the coordinator signature, which covers the domain-tagged body +// (domain_tag || body), not the bare body field. type SignedTransitionMessage struct { state protoimpl.MessageState `protogen:"open.v1"` Body []byte `protobuf:"bytes,1,opt,name=body,proto3" json:"body,omitempty"` diff --git a/pkg/frost/roast/gen/pb/evidence.proto b/pkg/frost/roast/gen/pb/evidence.proto index ac78895eb0..3b8075e459 100644 --- a/pkg/frost/roast/gen/pb/evidence.proto +++ b/pkg/frost/roast/gen/pb/evidence.proto @@ -5,15 +5,28 @@ package roast; // Evidence wire format (RFC-21 Layer B). // -// Signatures cover exact serialized body bytes, and those bytes travel -// verbatim: a verifier checks the signature over the bytes it received and -// only then parses them. Nothing in the evidence chain is ever re-encoded, -// so signature validity never depends on any serializer's canonical form - -// across protobuf library versions or across languages (the Phase 7 Rust -// signer verifies and parses these same bytes). +// The body bytes travel verbatim: a verifier checks the signature over the +// bytes it received and only then parses them. Nothing in the evidence chain +// is ever re-encoded, so signature validity never depends on any serializer's +// canonical form - across protobuf library versions or across languages (the +// Phase 7 Rust signer verifies and parses these same bytes). +// +// SIGNED PAYLOAD (cross-language contract). Operator and coordinator +// signatures do NOT cover the bare body; they cover a domain-separated byte +// stream, domain_tag || serialized body, where domain_tag is, in order: a +// single 0x00 byte, a fixed ASCII string, and a trailing 0x00 byte. The +// leading 0x00 is an illegal protobuf tag (field number 0), so the signed +// payload is undecodable as any protobuf message and a signature over one body +// type can never be accepted for another. The tag is NOT carried on the wire - +// only the body is, in the .body field. The ASCII strings are +// "roast/signed-evidence-snapshot/v1" (operator_signature) and +// "roast/signed-transition-message/v1" (coordinator_signature). Any +// implementation that signs or verifies the bare body (without the exact tag) +// will fail to interoperate. -// The byte stream a signer's operator key signs. Carried as exact bytes in -// SignedLocalEvidenceSnapshot.body. +// The evidence-snapshot body. Carried verbatim as +// SignedLocalEvidenceSnapshot.body; the operator signature covers the +// domain-tagged form of these bytes (domain_tag || body), not the bare bytes. message LocalEvidenceSnapshotBody { uint32 sender_id = 1; // 32-byte attempt context hash binding the evidence to one attempt. @@ -43,26 +56,30 @@ message ConflictEntry { uint64 count = 2; } -// The on-wire snapshot message: exact signed body bytes plus the operator -// signature over them. +// The on-wire snapshot message: the exact serialized LocalEvidenceSnapshotBody +// bytes plus the operator signature, which covers the domain-tagged body +// (domain_tag || body), not the bare body field. message SignedLocalEvidenceSnapshot { bytes body = 1; bytes operator_signature = 2; } -// The byte stream the elected coordinator signs. signed_snapshots carries -// each member's SignedLocalEvidenceSnapshot envelope verbatim as received, -// so the coordinator attests to the exact signed snapshots it assembled, -// in order, and downstream verifiers re-check the operator signatures over -// those same exact bytes. +// The transition-message body. signed_snapshots carries each member's +// SignedLocalEvidenceSnapshot envelope verbatim as received, so the +// coordinator attests to the exact signed snapshots it assembled, in order, +// and downstream verifiers re-check the operator signatures over those same +// exact bytes. Carried verbatim as SignedTransitionMessage.body; the +// coordinator signature covers the domain-tagged form of these bytes +// (domain_tag || body), not the bare bytes. message TransitionMessageBody { bytes attempt_context_hash = 1; uint32 coordinator_id = 2; repeated bytes signed_snapshots = 3; } -// The on-wire transition message: exact signed body bytes plus the -// coordinator signature over them. +// The on-wire transition message: the exact serialized TransitionMessageBody +// bytes plus the coordinator signature, which covers the domain-tagged body +// (domain_tag || body), not the bare body field. message SignedTransitionMessage { bytes body = 1; bytes coordinator_signature = 2; From fa5b4dc9b31ed61922eb7a9bdafd4a9ed7e32e1a Mon Sep 17 00:00:00 2001 From: maclane Date: Sun, 14 Jun 2026 11:54:57 -0400 Subject: [PATCH 4/4] fix(frost/roast): prime signable-bytes cache in Unmarshal to avoid verify race (P1) Gemini (PR #4057) flagged a data race the lazy-cache change introduced: setting signaturePayloadCache = nil in Unmarshal means the first SignableBytes call on a PARSED message writes the cache, so concurrent signature verification of one received message races on that write. This regressed a prior property - the pre-PR code primed the cache at Unmarshal because the signed payload equaled the received body, making a parsed message's SignableBytes a pure read. Prime the cache in both Unmarshal paths (clear it, then call SignableBytes once), restoring the race-free read on the verification path. This still discards any stale cache on a reused value. Tests: end-to-end cross-protocol rejection in both directions (a transition coordinator signature does not verify as a snapshot operator signature and vice versa, even with matching id/attempt), plus a -race regression guard that verifies a parsed snapshot concurrently. Full pkg/frost/roast suite passes under -race. Co-Authored-By: Claude Opus 4.8 --- pkg/frost/roast/domain_separation_test.go | 82 +++++++++++++++++++++++ pkg/frost/roast/transition_message.go | 16 ++++- 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/pkg/frost/roast/domain_separation_test.go b/pkg/frost/roast/domain_separation_test.go index 9b0695a2b5..f4847469b5 100644 --- a/pkg/frost/roast/domain_separation_test.go +++ b/pkg/frost/roast/domain_separation_test.go @@ -2,11 +2,15 @@ package roast import ( "bytes" + "errors" + "sync" "testing" "google.golang.org/protobuf/proto" + "github.com/keep-network/keep-core/pkg/frost/roast/attempt" "github.com/keep-network/keep-core/pkg/frost/roast/gen/pb" + "github.com/keep-network/keep-core/pkg/protocol/group" ) // These pin the cross-protocol signature-confusion defense for the operator-key @@ -158,3 +162,81 @@ func TestTransitionUnmarshal_ResetsSignableCache(t *testing.T) { t.Fatal("Unmarshal must reset the transition signable-bytes cache") } } + +func TestCrossProtocol_TransitionSignatureRejectedAsSnapshot(t *testing.T) { + // End-to-end: a coordinator signature over a transition message must not + // verify as an operator signature over a snapshot, even when the snapshot + // shares the signer id and attempt context. The distinct domain tags (and + // bodies) make the signed preimages disjoint. + const id group.MemberIndex = 7 + + transition := buildValidTransitionMessage() + transition.CoordinatorIDValue = uint32(id) + transition.CoordinatorSignature = nil + tPayload, err := transition.SignableBytes() + if err != nil { + t.Fatalf("transition signable: %v", err) + } + tSig, err := (&fakeSigner{id: id}).Sign(tPayload) + if err != nil { + t.Fatalf("sign transition: %v", err) + } + transition.CoordinatorSignature = tSig + // Control: the signature really is a valid bundle signature. + if err := verifyBundleSignature(fakeVerifier{}, transition, id); err != nil { + t.Fatalf("control: genuine transition signature must verify: %v", err) + } + + // Paste it onto a snapshot from the same signer + attempt. + snap := NewLocalEvidenceSnapshot(id, pinnedContextHash, attempt.Evidence{}) + snap.OperatorSignature = tSig + if err := verifySnapshotSignature(fakeVerifier{}, snap); !errors.Is(err, ErrSignatureInvalid) { + t.Fatalf("transition signature must not verify as a snapshot signature; got %v", err) + } +} + +func TestCrossProtocol_SnapshotSignatureRejectedAsTransition(t *testing.T) { + // The reverse direction: an operator signature over a snapshot must not + // verify as a coordinator signature over a transition message. + const id group.MemberIndex = 7 + snap := signedTestSnapshot(t, id) + // Control: it verifies as a snapshot signature. + if err := verifySnapshotSignature(fakeVerifier{}, snap); err != nil { + t.Fatalf("control: genuine snapshot signature must verify: %v", err) + } + + transition := buildValidTransitionMessage() + transition.CoordinatorIDValue = uint32(id) + transition.CoordinatorSignature = snap.OperatorSignature + if err := verifyBundleSignature(fakeVerifier{}, transition, id); !errors.Is(err, ErrSignatureInvalid) { + t.Fatalf("snapshot signature must not verify as a transition signature; got %v", err) + } +} + +func TestSnapshotSignableBytes_ConcurrentAfterUnmarshalIsRaceFree(t *testing.T) { + // Regression guard (run under -race): a parsed snapshot must carry a primed + // signable-bytes cache so concurrent signature verification reads a ready + // cache instead of racing on lazy initialization. Without priming in + // Unmarshal, the concurrent first SignableBytes calls below race on the + // cache write. + wire, err := signedTestSnapshot(t, 7).Marshal() + if err != nil { + t.Fatalf("marshal: %v", err) + } + var decoded LocalEvidenceSnapshot + if err := decoded.Unmarshal(wire); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if _, err := decoded.SignableBytes(); err != nil { + t.Errorf("signable: %v", err) + } + }() + } + wg.Wait() +} diff --git a/pkg/frost/roast/transition_message.go b/pkg/frost/roast/transition_message.go index 11de4d1b5b..4a63ff2550 100644 --- a/pkg/frost/roast/transition_message.go +++ b/pkg/frost/roast/transition_message.go @@ -253,9 +253,15 @@ func (s *LocalEvidenceSnapshot) Unmarshal(data []byte) error { s.OperatorSignature = append([]byte(nil), envelope.OperatorSignature...) s.bodyCache = append([]byte(nil), envelope.Body...) s.wireEnvelope = append([]byte(nil), data...) - // Clear any signable-bytes cache a prior SignableBytes call left on a - // reused receiver, so the next call rebuilds it from the received body. + // Prime the signable-bytes cache from the body just received, discarding any + // cache a prior SignableBytes call left on a reused value. Priming here - + // rather than lazily in SignableBytes - keeps concurrent signature + // verification of a parsed snapshot race-free: verifiers read a ready cache + // instead of racing on lazy initialization. s.signaturePayloadCache = nil + if _, err := s.SignableBytes(); err != nil { + return err + } return s.Validate() } @@ -437,9 +443,13 @@ func (m *TransitionMessage) Unmarshal(data []byte) error { m.CoordinatorSignature = append([]byte(nil), envelope.CoordinatorSignature...) m.bodyCache = append([]byte(nil), envelope.Body...) m.wireEnvelope = append([]byte(nil), data...) - // Clear any signable-bytes cache left on a reused receiver (see + // Prime the signable-bytes cache so concurrent verification of a parsed + // bundle is race-free, and discard any stale cache on a reused value (see // LocalEvidenceSnapshot.Unmarshal). m.signaturePayloadCache = nil + if _, err := m.SignableBytes(); err != nil { + return err + } return m.Validate() }