diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 7b40758cce..c74b4562eb 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -354,18 +354,40 @@ func (tb *TransactionBuilder) ReplaceUnsignedTransaction( ) } + // The replacement's SignatureScript and Witness are both empty here + // because of the two refusals above, so the per-input restore below + // only has to decide what to copy *from* the previous input. if tb.sigHashArgs[i].witness { - if len(replacedInput.Witness) == 0 && len(previousInput.Witness) == 1 { + // Witness inputs may carry a single-element pre-signing witness + // that holds a P2WSH-style redeem script. Multi-element witnesses + // belong to P2TR script-path spends or other workflows that are + // not in scope for the current FROST migration, and silently + // dropping them produced malformed transactions later — refuse + // instead so the unsupported case fails loudly. Lifting this to + // support multi-element witnesses requires a per-input policy + // rather than a blanket copy because the replacement could + // legitimately differ in witness shape from the previous input. + switch len(previousInput.Witness) { + case 0: + // Nothing to restore (typical P2TR key-path or P2WPKH). + case 1: redeemScript := append([]byte{}, previousInput.Witness[0]...) replacedInput.Witness = wire.TxWitness{redeemScript} - } - } else { - if len(replacedInput.SignatureScript) == 0 && len(previousInput.SignatureScript) > 0 { - replacedInput.SignatureScript = append( - []byte{}, - previousInput.SignatureScript..., + default: + return fmt.Errorf( + "replacement transaction input [%d] previous witness has "+ + "[%d] elements; only zero- or single-element "+ + "pre-signing witnesses are currently supported for "+ + "restoration", + i, + len(previousInput.Witness), ) } + } else if len(previousInput.SignatureScript) > 0 { + replacedInput.SignatureScript = append( + []byte{}, + previousInput.SignatureScript..., + ) } } diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index 2720febb7d..8349946f6c 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -451,6 +451,58 @@ func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsNonEmptyReplacemen } } +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsMultiElementPreviousWitness( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + previousInput := wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil) + // Pre-signing witness that mimics a P2TR script-path spend: [script, + // controlBlock]. The restoration path supports only zero- or + // single-element previous witnesses today; the multi-element case must + // fail loudly rather than silently dropping data later in signing. + previousInput.Witness = wire.TxWitness{ + []byte{0x51, 0x52}, + []byte{0xc0, 0xab, 0xcd}, + } + builder.internal.AddTxIn(previousInput) + builder.sigHashArgs = append( + builder.sigHashArgs, + &inputSigHashArgs{value: 1, scriptCode: []byte{0x51}, witness: true}, + ) + + err := builder.ReplaceUnsignedTransaction( + &Transaction{ + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(txHash), + OutputIndex: 0, + }, + Sequence: 0xffffffff, + }, + }, + }, + ) + if err == nil { + t.Fatal("expected multi-element witness restoration error") + } + + if !strings.Contains( + err.Error(), + "previous witness has [2] elements", + ) { + t.Fatalf("unexpected error: [%v]", err) + } + if !strings.Contains( + err.Error(), + "only zero- or single-element", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + func TestTransactionBuilder_UnsignedTransactionIO(t *testing.T) { builder := NewTransactionBuilder(nil) diff --git a/pkg/frost/signing/native_ffi_primitive_registration.go b/pkg/frost/signing/native_ffi_primitive_registration.go index 516330a56f..a62a38b19f 100644 --- a/pkg/frost/signing/native_ffi_primitive_registration.go +++ b/pkg/frost/signing/native_ffi_primitive_registration.go @@ -9,6 +9,7 @@ import ( var ( registrationLogger = log.Logger("keep-frost-signing-registration") + protocolLogger = log.Logger("keep-frost-signing-protocol") registrationErrorMu sync.RWMutex lastRegistrationError error ) diff --git a/pkg/frost/signing/native_ffi_primitive_transitional_frost_native.go b/pkg/frost/signing/native_ffi_primitive_transitional_frost_native.go index e38b12eecb..6bdac5a854 100644 --- a/pkg/frost/signing/native_ffi_primitive_transitional_frost_native.go +++ b/pkg/frost/signing/native_ffi_primitive_transitional_frost_native.go @@ -3,6 +3,7 @@ package signing import ( + "bytes" "context" "crypto/sha256" "encoding/hex" @@ -466,6 +467,23 @@ func buildTaggedTBTCSignerRoundKeyGroup( if payload.KeyGroupSource == NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey { // Scaffold compatibility: legacy-wallet-pubkey key groups are // placeholder-only and expected to diverge from coarse RunDKG output. + // Refuse the substitution by default so a production deployment that + // somehow ended up with placeholder material does not silently route + // signing through whatever key group the Rust side happens to return. + // The operator must explicitly opt into the scaffold path via + // AcceptScaffoldKeyGroupEnvVar; the env-var check is per-call (not + // cached) so flipping it off recovers fail-closed behavior without a + // restart. + if !AcceptScaffoldKeyGroupEnabled() { + return "", false, fmt.Errorf( + "tbtc-signer key group source %q is scaffold-era placeholder "+ + "material and may not be silently substituted with the "+ + "RunDKG output; set %s=true to opt in for local/CI use "+ + "only, never in production", + NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey, + AcceptScaffoldKeyGroupEnvVar, + ) + } return dkgResult.KeyGroup, true, nil } @@ -902,13 +920,44 @@ func collectBuildTaggedTBTCSignerRoundContributionMessages( ) case message := <-messageChan: - receivedMessages[message.SenderID()] = message + // First-write-wins / equal-or-reject. A peer that retransmits the + // same contribution is idempotent; a peer that mutates its own + // contribution after the first send is a ROAST evidence concern + // and must not be allowed to overwrite the persisted view. + senderID := message.SenderID() + if existing, ok := receivedMessages[senderID]; ok { + if !buildTaggedTBTCSignerRoundContributionMessagesEqual( + existing, + message, + ) { + protocolLogger.Warnf( + "dropping conflicting tbtc-signer round contribution "+ + "from sender [%d]; first-write-wins keeps the "+ + "originally accepted contribution", + senderID, + ) + } + continue + } + receivedMessages[senderID] = message } } return receivedMessages, nil } +func buildTaggedTBTCSignerRoundContributionMessagesEqual( + left, right *buildTaggedTBTCSignerRoundContributionMessage, +) bool { + if left == nil || right == nil { + return left == right + } + return left.SenderIDValue == right.SenderIDValue && + left.SessionIDValue == right.SessionIDValue && + left.ContributionIdentifier == right.ContributionIdentifier && + bytes.Equal(left.ContributionData, right.ContributionData) +} + func buildTaggedTBTCSignerSyntheticRoundContributions( roundState *NativeTBTCSignerRoundState, includedMembersIndexes []group.MemberIndex, diff --git a/pkg/frost/signing/native_ffi_primitive_transitional_frost_native_test.go b/pkg/frost/signing/native_ffi_primitive_transitional_frost_native_test.go index 03a01951cc..3bcce62fc3 100644 --- a/pkg/frost/signing/native_ffi_primitive_transitional_frost_native_test.go +++ b/pkg/frost/signing/native_ffi_primitive_transitional_frost_native_test.go @@ -1289,12 +1289,14 @@ func TestExecuteBuildTaggedTBTCSignerBootstrapCoarseRound_FailsWhenRoundStateSig func TestBuildTaggedTBTCSignerRoundKeyGroup(t *testing.T) { testCases := []struct { - name string - payload *NativeTBTCSignerMaterialPayload - dkgResult *NativeTBTCSignerDKGResult - expected string - substituted bool - expectError bool + name string + payload *NativeTBTCSignerMaterialPayload + dkgResult *NativeTBTCSignerDKGResult + acceptScaffoldOptIn bool + expected string + substituted bool + expectError bool + expectScaffoldRefuse bool }{ { name: "exact match", @@ -1308,7 +1310,19 @@ func TestBuildTaggedTBTCSignerRoundKeyGroup(t *testing.T) { substituted: false, }, { - name: "legacy source mismatch uses dkg key group", + name: "legacy source mismatch refused without opt-in", + payload: &NativeTBTCSignerMaterialPayload{ + KeyGroup: "legacy-group", + KeyGroupSource: NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey, + }, + dkgResult: &NativeTBTCSignerDKGResult{ + KeyGroup: "dkg-group", + }, + expectError: true, + expectScaffoldRefuse: true, + }, + { + name: "legacy source mismatch uses dkg key group with opt-in", payload: &NativeTBTCSignerMaterialPayload{ KeyGroup: "legacy-group", KeyGroupSource: NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey, @@ -1316,8 +1330,9 @@ func TestBuildTaggedTBTCSignerRoundKeyGroup(t *testing.T) { dkgResult: &NativeTBTCSignerDKGResult{ KeyGroup: "dkg-group", }, - expected: "dkg-group", - substituted: true, + acceptScaffoldOptIn: true, + expected: "dkg-group", + substituted: true, }, { name: "non-legacy source mismatch rejects", @@ -1334,12 +1349,30 @@ func TestBuildTaggedTBTCSignerRoundKeyGroup(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + if tc.acceptScaffoldOptIn { + t.Setenv(AcceptScaffoldKeyGroupEnvVar, "true") + } else { + // Force the env to "" so a stray external value from a + // containing process cannot suppress the scaffold refusal + // during this test case. + t.Setenv(AcceptScaffoldKeyGroupEnvVar, "") + } + actual, substituted, err := buildTaggedTBTCSignerRoundKeyGroup(tc.payload, tc.dkgResult) if tc.expectError { if err == nil { t.Fatal("expected error") } + if tc.expectScaffoldRefuse && + !strings.Contains(err.Error(), AcceptScaffoldKeyGroupEnvVar) { + t.Fatalf( + "expected scaffold-refusal error referencing %s; got: [%v]", + AcceptScaffoldKeyGroupEnvVar, + err, + ) + } + return } @@ -1802,6 +1835,12 @@ func TestBuildTaggedLegacyCompatibleNativeExecutionFFISigningPrimitive_Sign_TBTC func TestBuildTaggedLegacyCompatibleNativeExecutionFFISigningPrimitive_Sign_TBTCSignerPath_BootstrapVersion_LegacyKeyGroupSourceUsesRunDKGResult( t *testing.T, ) { + // Scaffold-era path: legacy-wallet-pubkey signer material is refused by + // default; the operator opt-in via AcceptScaffoldKeyGroupEnvVar is what + // lets this test exercise the substitution. Production deployments must + // never set this. + t.Setenv(AcceptScaffoldKeyGroupEnvVar, "true") + engine := &mockBuildTaggedTBTCSignerEngine{ version: "tbtc-signer/0.1.0-bootstrap", runDKGResult: &NativeTBTCSignerDKGResult{ diff --git a/pkg/frost/signing/native_frost_protocol_frost_native.go b/pkg/frost/signing/native_frost_protocol_frost_native.go index e2c496be73..3dcc1af4d8 100644 --- a/pkg/frost/signing/native_frost_protocol_frost_native.go +++ b/pkg/frost/signing/native_frost_protocol_frost_native.go @@ -3,6 +3,7 @@ package signing import ( + "bytes" "context" "encoding/json" "errors" @@ -582,13 +583,39 @@ func collectNativeFROSTRoundOneMessages( ) case message := <-messageChan: - receivedMessages[message.SenderID()] = message + // First-write-wins / equal-or-reject. See the matching comment in + // native_ffi_primitive_transitional_frost_native.go. + senderID := message.SenderID() + if existing, ok := receivedMessages[senderID]; ok { + if !nativeFROSTRoundOneCommitmentMessagesEqual(existing, message) { + protocolLogger.Warnf( + "dropping conflicting native FROST round one "+ + "commitment from sender [%d]; first-write-wins "+ + "keeps the originally accepted commitment", + senderID, + ) + } + continue + } + receivedMessages[senderID] = message } } return receivedMessages, nil } +func nativeFROSTRoundOneCommitmentMessagesEqual( + left, right *nativeFROSTRoundOneCommitmentMessage, +) bool { + if left == nil || right == nil { + return left == right + } + return left.SenderIDValue == right.SenderIDValue && + left.SessionIDValue == right.SessionIDValue && + left.ParticipantIdentifier == right.ParticipantIdentifier && + bytes.Equal(left.CommitmentData, right.CommitmentData) +} + func collectNativeFROSTRoundTwoMessages( ctx context.Context, request *NativeExecutionFFISigningRequest, @@ -638,13 +665,41 @@ func collectNativeFROSTRoundTwoMessages( ) case message := <-messageChan: - receivedMessages[message.SenderID()] = message + // First-write-wins / equal-or-reject. See round one above. + senderID := message.SenderID() + if existing, ok := receivedMessages[senderID]; ok { + if !nativeFROSTRoundTwoSignatureShareMessagesEqual( + existing, + message, + ) { + protocolLogger.Warnf( + "dropping conflicting native FROST round two "+ + "signature share from sender [%d]; first-write-wins "+ + "keeps the originally accepted share", + senderID, + ) + } + continue + } + receivedMessages[senderID] = message } } return receivedMessages, nil } +func nativeFROSTRoundTwoSignatureShareMessagesEqual( + left, right *nativeFROSTRoundTwoSignatureShareMessage, +) bool { + if left == nil || right == nil { + return left == right + } + return left.SenderIDValue == right.SenderIDValue && + left.SessionIDValue == right.SessionIDValue && + left.ParticipantIdentifier == right.ParticipantIdentifier && + bytes.Equal(left.SignatureShareData, right.SignatureShareData) +} + func shouldAcceptNativeFROSTMessage( request *NativeExecutionFFISigningRequest, includedMembersSet map[group.MemberIndex]struct{}, diff --git a/pkg/frost/signing/native_tbtc_signer_material.go b/pkg/frost/signing/native_tbtc_signer_material.go index ad8b443ad9..3b5391e391 100644 --- a/pkg/frost/signing/native_tbtc_signer_material.go +++ b/pkg/frost/signing/native_tbtc_signer_material.go @@ -1,12 +1,27 @@ package signing +import ( + "os" + "strings" +) + const ( // NativeSignerMaterialFormatFrostTBTCSignerV1 carries signer material for // tbtc-signer coarse session APIs. NativeSignerMaterialFormatFrostTBTCSignerV1 = "frost-tbtc-signer-v1" // NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey marks scaffold-era - // key-group derivation from the legacy wallet public key. + // key-group derivation from the legacy wallet public key. Material built + // with this source is placeholder data, not the output of a real FROST DKG + // run, and is refused by default at signing time. See + // `AcceptScaffoldKeyGroupEnvVar` for the opt-in escape hatch. NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey = "legacy-wallet-pubkey" + + // AcceptScaffoldKeyGroupEnvVar is the operator-facing opt-in that allows + // the FROST tbtc-signer FFI path to accept signer material whose + // `KeyGroupSource` is `legacy-wallet-pubkey`. Production deployments must + // not set this; it exists for local dev, CI, and integration rehearsals + // where a real DKG hand-off is not yet wired. + AcceptScaffoldKeyGroupEnvVar = "KEEP_CORE_FROST_TBTC_SIGNER_ACCEPT_SCAFFOLD_KEY_GROUP" ) // NativeTBTCSignerMaterialPayload is the signer-material payload schema for @@ -16,3 +31,27 @@ type NativeTBTCSignerMaterialPayload struct { KeyGroupSource string `json:"keyGroupSource,omitempty"` LegacyPrivateKeyShareHex string `json:"legacyPrivateKeyShareHex,omitempty"` } + +// AcceptScaffoldKeyGroupEnabled reports whether the operator has opted into +// accepting scaffold-era (legacy-wallet-pubkey) key-group material. Without +// this, the signer material resolver and the FFI signing primitive both +// refuse legacy material rather than silently signing with placeholder +// cryptographic context. +// +// The env var is parsed identically to the bootstrap-mode flag in +// `pkg/frost/signing/backend.go`: case-insensitive `1`, `true`, `yes`, or +// `on`. Anything else (including missing/empty) is treated as disabled, so +// the safe-by-default behavior is to refuse. +func AcceptScaffoldKeyGroupEnabled() bool { + raw, ok := os.LookupEnv(AcceptScaffoldKeyGroupEnvVar) + if !ok { + return false + } + + switch strings.ToLower(strings.TrimSpace(raw)) { + case "1", "true", "yes", "on": + return true + default: + return false + } +} diff --git a/pkg/tbtc/signer_material_resolver_build_frost_native_tbtc_signer.go b/pkg/tbtc/signer_material_resolver_build_frost_native_tbtc_signer.go index ef6a07a252..268b53a521 100644 --- a/pkg/tbtc/signer_material_resolver_build_frost_native_tbtc_signer.go +++ b/pkg/tbtc/signer_material_resolver_build_frost_native_tbtc_signer.go @@ -58,6 +58,21 @@ func (btnsmr *buildTaggedNativeSignerMaterialResolver) ResolveSignerMaterial( keyGroupDigest := sha256.Sum256(walletPublicKeyBytes) + // Scaffold-era key-group derivation: the current value identifies + // placeholder material derived from the legacy wallet public-key hash, + // not the output of a real FROST DKG run. Refuse to surface that material + // at all unless the operator has explicitly opted in via + // AcceptScaffoldKeyGroupEnvVar — production deployments must never set + // this. See native_tbtc_signer_material.go for the env-var contract. + if !frostsigning.AcceptScaffoldKeyGroupEnabled() { + return nil, fmt.Errorf( + "refusing to build scaffold-era %q signer material; set %s=true to "+ + "opt in for local/CI use only, never in production", + frostsigning.NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey, + frostsigning.AcceptScaffoldKeyGroupEnvVar, + ) + } + // TODO: Replace this placeholder key-group derivation with Rust DKG output. // The current value identifies scaffold-era material only. payload, err := json.Marshal(frostsigning.NativeTBTCSignerMaterialPayload{ diff --git a/pkg/tbtc/signer_material_resolver_build_frost_native_test.go b/pkg/tbtc/signer_material_resolver_build_frost_native_test.go index 45680db2ad..23f4b36b8b 100644 --- a/pkg/tbtc/signer_material_resolver_build_frost_native_test.go +++ b/pkg/tbtc/signer_material_resolver_build_frost_native_test.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "encoding/json" "errors" + "strings" "testing" frostsigning "github.com/keep-network/keep-core/pkg/frost/signing" @@ -16,6 +17,10 @@ import ( func TestRegisterSignerMaterialResolverForBuild_UsesDefaultProvider( t *testing.T, ) { + // Default scaffold-era resolver builds legacy-wallet-pubkey signer + // material; production refuses it but local/CI tests can opt in. + t.Setenv(frostsigning.AcceptScaffoldKeyGroupEnvVar, "true") + UnregisterSignerMaterialResolver() UnregisterSignerMaterialResolverProviderForBuild() t.Cleanup(UnregisterSignerMaterialResolver) @@ -194,3 +199,44 @@ func TestRegisterSignerMaterialResolverForBuild_ProviderReturnsNilResolver( t.Fatal("expected build resolver registration error") } } + +func TestRegisterSignerMaterialResolverForBuild_DefaultProviderRefusesScaffoldWithoutOptIn( + t *testing.T, +) { + // Force the env var to "" so a stray external value cannot suppress the + // scaffold refusal during this regression test. + t.Setenv(frostsigning.AcceptScaffoldKeyGroupEnvVar, "") + + UnregisterSignerMaterialResolver() + UnregisterSignerMaterialResolverProviderForBuild() + t.Cleanup(UnregisterSignerMaterialResolver) + t.Cleanup(UnregisterSignerMaterialResolverProviderForBuild) + + err := RegisterSignerMaterialResolverForBuild() + if err != nil { + t.Fatalf("unexpected build resolver registration error: [%v]", err) + } + + privateKeyShare := createMockSigner(t).privateKeyShare + _, err = resolveSignerMaterial(privateKeyShare) + if err == nil { + t.Fatal( + "expected scaffold-refusal error from default resolver without opt-in", + ) + } + + if !strings.Contains(err.Error(), frostsigning.AcceptScaffoldKeyGroupEnvVar) { + t.Fatalf( + "expected scaffold-refusal error to reference %s; got: [%v]", + frostsigning.AcceptScaffoldKeyGroupEnvVar, + err, + ) + } + if !strings.Contains(err.Error(), frostsigning.NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey) { + t.Fatalf( + "expected scaffold-refusal error to reference %s; got: [%v]", + frostsigning.NativeTBTCSignerKeyGroupSourceLegacyWalletPubKey, + err, + ) + } +} diff --git a/pkg/tbtc/signing_test.go b/pkg/tbtc/signing_test.go index 5505e63c72..9ac73be7ee 100644 --- a/pkg/tbtc/signing_test.go +++ b/pkg/tbtc/signing_test.go @@ -110,6 +110,14 @@ func TestSigningExecutor_SignBatch(t *testing.T) { // setupSigningExecutor sets up an instance of the signing executor ready // to perform test signing. func setupSigningExecutor(t *testing.T) *signingExecutor { + // Tests in this suite exercise the keep-tbtc signing executor against + // in-process tECDSA fixtures. Under the `frost_native frost_tbtc_signer` + // build tags, the signer-material resolver refuses scaffold-era + // (legacy-wallet-pubkey) material by default; the fixtures here are + // inherently scaffold-era so the executor needs the operator opt-in to + // continue running. Production deployments must never set this env var. + t.Setenv("KEEP_CORE_FROST_TBTC_SIGNER_ACCEPT_SCAFFOLD_KEY_GROUP", "true") + groupParameters := &GroupParameters{ GroupSize: 5, GroupQuorum: 4,