diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index 211dda0404..ebe932cda5 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -574,6 +574,15 @@ pub fn interactive_aggregate( enforce_provenance_gate()?; validate_session_id(&request.session_id)?; let attempt_id = canonical_attempt_id(&request.attempt_id); + // The completion marker persists attempt_id, and the reload path rejects an + // empty key; reject an empty attempt_id here too so a malformed (or + // malicious) request cannot write durable state that fails to reload after + // a restart. + if attempt_id.is_empty() { + return Err(EngineError::Validation( + "InteractiveAggregate: attempt_id must not be empty".to_string(), + )); + } let mut signing_package_bytes = decode_hex_field( "InteractiveAggregate", @@ -612,6 +621,18 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; + // Reject a completed attempt: re-aggregation is not a recovery path (a + // lost signature is recovered with a fresh attempt), and the marker is + // durable so a completed attempt stays rejected across a restart. + if session + .aggregated_interactive_attempt_markers + .contains(&attempt_id) + { + return Err(EngineError::InteractiveAttemptAlreadyAggregated { + session_id: request.session_id.clone(), + attempt_id, + }); + } if session.dkg_result.is_none() { return Err(EngineError::DkgNotReady { session_id: request.session_id.clone(), @@ -675,6 +696,54 @@ pub fn interactive_aggregate( let signature_bytes = signature .serialize() .map_err(|e| EngineError::Internal(format!("failed to serialize aggregate: {e}")))?; + let signature_hex = hex::encode(signature_bytes); + + // Mark the attempt complete before reporting success, so a repeat + // InteractiveAggregate is rejected rather than recomputed (Phase 7.2b design + // section 6). The engine lock was dropped for the aggregation crypto above; + // re-acquire it, re-check the marker (a concurrent aggregate may have + // completed first), insert it, and persist before reporting success; on + // persist failure roll the marker back and fail closed. + let mut guard = state()? + .lock() + .map_err(|_| EngineError::Internal("engine lock poisoned".to_string()))?; + let session = guard.sessions.get_mut(&request.session_id).ok_or_else(|| { + EngineError::SessionNotFound { + session_id: request.session_id.clone(), + } + })?; + // A concurrent aggregate that raced past the pre-check may have completed + // this attempt first; if the marker is now present, reject this call's + // re-aggregation - the winner already produced the attempt's signature. + if session + .aggregated_interactive_attempt_markers + .contains(&attempt_id) + { + return Err(EngineError::InteractiveAttemptAlreadyAggregated { + session_id: request.session_id.clone(), + attempt_id, + }); + } + ensure_consumed_registry_insert_capacity( + &session.aggregated_interactive_attempt_markers, + &attempt_id, + "aggregated_interactive_attempt_markers", + &request.session_id, + )?; + session + .aggregated_interactive_attempt_markers + .insert(attempt_id.clone()); + if let Err(persist_error) = persist_engine_state_to_storage(&guard) { + let session = guard + .sessions + .get_mut(&request.session_id) + .expect("session existed under the held engine lock"); + session + .aggregated_interactive_attempt_markers + .remove(&attempt_id); + return Err(persist_error); + } + drop(guard); record_hardening_telemetry(|telemetry| { telemetry.interactive_aggregate_success_total = telemetry @@ -685,7 +754,7 @@ pub fn interactive_aggregate( Ok(InteractiveAggregateResult { session_id: request.session_id, attempt_id, - signature_hex: hex::encode(signature_bytes), + signature_hex, }) } diff --git a/pkg/tbtc/signer/src/engine/persistence.rs b/pkg/tbtc/signer/src/engine/persistence.rs index 9a6200345e..ce68b66094 100644 --- a/pkg/tbtc/signer/src/engine/persistence.rs +++ b/pkg/tbtc/signer/src/engine/persistence.rs @@ -44,6 +44,11 @@ pub(crate) struct PersistedSessionState { // interactive state, including nonces, never persists). #[serde(default, skip_serializing_if = "Vec::is_empty")] pub(crate) consumed_interactive_attempt_markers: Vec, + // Phase 7.2b InteractiveAggregate completion markers (see SessionState). + // serde(default) keeps state written before 7.2b loadable: an absent field + // deserializes to an empty set. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub(crate) aggregated_interactive_attempt_markers: Vec, } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -1286,6 +1291,26 @@ impl TryFrom for SessionState { consumed_interactive_attempt_markers.len(), "consumed_interactive_attempt_markers", )?; + + let mut aggregated_interactive_attempt_markers = HashSet::new(); + for attempt_marker in persisted.aggregated_interactive_attempt_markers { + if attempt_marker.is_empty() { + return Err(EngineError::Internal( + "persisted aggregated interactive attempt marker must be non-empty".to_string(), + )); + } + + if !aggregated_interactive_attempt_markers.insert(attempt_marker.clone()) { + return Err(EngineError::Internal(format!( + "duplicate persisted aggregated interactive attempt marker [{}]", + attempt_marker + ))); + } + } + ensure_consumed_registry_persisted_bound( + aggregated_interactive_attempt_markers.len(), + "aggregated_interactive_attempt_markers", + )?; if persisted.attempt_transition_records.len() > TBTC_SIGNER_MAX_ATTEMPT_TRANSITION_RECORDS_PER_SESSION { @@ -1345,6 +1370,7 @@ impl TryFrom for SessionState { // only the consumption markers survive. interactive_signing: None, consumed_interactive_attempt_markers, + aggregated_interactive_attempt_markers, }) } } @@ -1455,6 +1481,12 @@ impl TryFrom<&SessionState> for PersistedSessionState { .cloned() .collect::>(); consumed_interactive_attempt_markers.sort_unstable(); + let mut aggregated_interactive_attempt_markers = session_state + .aggregated_interactive_attempt_markers + .iter() + .cloned() + .collect::>(); + aggregated_interactive_attempt_markers.sort_unstable(); Ok(PersistedSessionState { dkg_request_fingerprint: session_state.dkg_request_fingerprint.clone(), @@ -1479,6 +1511,7 @@ impl TryFrom<&SessionState> for PersistedSessionState { refresh_history: session_state.refresh_history.clone(), emergency_rekey_event: session_state.emergency_rekey_event.clone(), consumed_interactive_attempt_markers, + aggregated_interactive_attempt_markers, }) } } diff --git a/pkg/tbtc/signer/src/engine/state.rs b/pkg/tbtc/signer/src/engine/state.rs index 43fe1dc3d0..ab52ab36e1 100644 --- a/pkg/tbtc/signer/src/engine/state.rs +++ b/pkg/tbtc/signer/src/engine/state.rs @@ -111,6 +111,14 @@ pub(crate) struct SessionState { pub(crate) emergency_rekey_event: Option, pub(crate) interactive_signing: Option, pub(crate) consumed_interactive_attempt_markers: HashSet, + // Phase 7.2b InteractiveAggregate completion markers: an attempt whose + // aggregate signature has been produced is recorded here so a repeat + // InteractiveAggregate is rejected (re-aggregation is not a recovery path; + // a lost signature is recovered with a fresh attempt). Durable like the + // consumed markers (markers-only durability) and bounded the same way; not + // security-load-bearing (the aggregate is deterministic over public data), + // but the frozen Phase 7 spec marks the session complete. + pub(crate) aggregated_interactive_attempt_markers: HashSet, } #[derive(Default)] diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 22b533ab7d..60f8a2d2cb 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -702,6 +702,7 @@ fn persisted_session_state_fixture() -> PersistedSessionState { refresh_history: vec![], emergency_rekey_event: None, consumed_interactive_attempt_markers: vec![], + aggregated_interactive_attempt_markers: vec![], } } @@ -12840,6 +12841,199 @@ fn interactive_aggregate_produces_and_self_verifies_bip340() { .expect("interactive aggregate yields a valid BIP-340 signature"); } +#[test] +fn interactive_aggregate_rejects_repeat_aggregate_of_completed_attempt() { + let _guard = lock_test_state(); + reset_for_tests(); + + let session_id = "interactive-aggregate-repeat"; + let key_group = "interactive-test-key-group"; + let message = [0x4eu8; 32]; + let included = [1u16, 2]; + let key_packages = ensure_interactive_dkg_session(session_id, key_group); + + let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2) + .expect("opens"); + let round1 = interactive_round1(InteractiveRound1Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + }) + .expect("round 1"); + let member2 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 nonces"); + let signing_package_hex = interactive_package_for_test( + &message, + vec![ + NativeFrostCommitment { + identifier: key_packages[&1].identifier.clone(), + data_hex: round1.commitments_hex, + }, + member2.commitment.clone(), + ], + ); + let round2 = interactive_round2(InteractiveRound2Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + signing_package_hex: signing_package_hex.clone(), + }) + .expect("round 2 share"); + let member2_share = sign_share(SignShareRequest { + signing_package_hex: signing_package_hex.clone(), + nonces_hex: member2.nonces_hex, + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 share"); + + let aggregate_request = InteractiveAggregateRequest { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + signing_package_hex, + signature_shares: vec![ + crate::api::NativeFrostSignatureShare { + identifier: key_packages[&1].identifier.clone(), + data_hex: round2.signature_share_hex, + }, + member2_share.signature_share, + ], + taproot_merkle_root_hex: None, + }; + + // First aggregate completes the attempt. + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + + // Re-aggregating a completed attempt is rejected by the durable completion + // marker rather than recomputed (re-aggregation is not a recovery path; a + // lost signature is recovered with a fresh attempt). Phase 7.2b design + // section 6. + let err = interactive_aggregate(aggregate_request) + .expect_err("re-aggregating a completed attempt must be rejected"); + assert!( + matches!( + err, + EngineError::InteractiveAttemptAlreadyAggregated { ref attempt_id, .. } + if *attempt_id == opened.attempt_id + ), + "unexpected error: {err:?}" + ); + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + assert_eq!(err.recovery_class(), "recoverable"); +} + +#[test] +fn interactive_aggregate_rejects_empty_attempt_id() { + let _guard = lock_test_state(); + reset_for_tests(); + + // An empty attempt_id must be rejected before anything is persisted: the + // completion record is keyed by attempt_id and the reload path rejects an + // empty key, so persisting one would brick restart. The guard runs before + // the signing-package/share decode, so the placeholder inputs are never + // reached. + let err = interactive_aggregate(InteractiveAggregateRequest { + session_id: "interactive-aggregate-empty-attempt".to_string(), + attempt_id: String::new(), + signing_package_hex: String::new(), + signature_shares: vec![], + taproot_merkle_root_hex: None, + }) + .expect_err("an empty attempt_id must be rejected"); + assert!( + matches!(err, EngineError::Validation(ref m) if m.contains("attempt_id must not be empty")), + "unexpected error: {err:?}" + ); +} + +#[test] +fn interactive_aggregate_completion_marker_survives_process_restart() { + let _guard = lock_test_state(); + let state_path = configure_test_state_path("interactive_aggregate_marker_restart"); + reset_for_tests(); + + let session_id = "interactive-aggregate-marker-restart"; + let key_group = "interactive-test-key-group"; + let message = [0x4fu8; 32]; + let included = [1u16, 2]; + let key_packages = ensure_interactive_dkg_session(session_id, key_group); + + let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2) + .expect("opens"); + let round1 = interactive_round1(InteractiveRound1Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + }) + .expect("round 1"); + let member2 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 nonces"); + let signing_package_hex = interactive_package_for_test( + &message, + vec![ + NativeFrostCommitment { + identifier: key_packages[&1].identifier.clone(), + data_hex: round1.commitments_hex, + }, + member2.commitment.clone(), + ], + ); + let round2 = interactive_round2(InteractiveRound2Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + signing_package_hex: signing_package_hex.clone(), + }) + .expect("round 2 share"); + let member2_share = sign_share(SignShareRequest { + signing_package_hex: signing_package_hex.clone(), + nonces_hex: member2.nonces_hex, + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 share"); + + let aggregate_request = InteractiveAggregateRequest { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + signing_package_hex, + signature_shares: vec![ + crate::api::NativeFrostSignatureShare { + identifier: key_packages[&1].identifier.clone(), + data_hex: round2.signature_share_hex, + }, + member2_share.signature_share, + ], + taproot_merkle_root_hex: None, + }; + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + + // The completion marker is the only durable interactive artifact (live + // nonce state is gone after restart by construction). It must survive a + // reload so a replayed aggregate is still rejected - this also exercises + // the marker's persistence round-trip (serialize + reload validation). + simulate_process_restart_for_tests(); + reload_state_from_storage_for_tests(); + + let err = interactive_aggregate(aggregate_request) + .expect_err("a completed attempt must stay completed across restart"); + assert!( + matches!(err, EngineError::InteractiveAttemptAlreadyAggregated { .. }), + "unexpected error: {err:?}" + ); + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + + reset_for_tests(); + cleanup_test_state_artifacts(&state_path); + clear_state_storage_policy_overrides(); +} + #[test] fn interactive_aggregate_rejects_invalid_share_fail_closed() { let _guard = lock_test_state(); diff --git a/pkg/tbtc/signer/src/errors.rs b/pkg/tbtc/signer/src/errors.rs index 636d9ccf82..34a0671ee7 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -82,6 +82,18 @@ pub enum EngineError { session_id: String, attempt_id: String, }, + /// Returned when InteractiveAggregate is invoked again for an attempt that + /// already produced an aggregate signature in this session. The per-attempt + /// "aggregated" marker is durable, so a completed attempt stays completed + /// across restart; re-aggregation is rejected rather than recomputed + /// (a lost signature is recovered with a fresh attempt, not by replay). + /// Distinct code so callers match on + /// `interactive_attempt_already_aggregated` rather than the message. + #[error("interactive attempt [{attempt_id}] already aggregated in session [{session_id}]")] + InteractiveAttemptAlreadyAggregated { + session_id: String, + attempt_id: String, + }, #[error("internal error: {0}")] Internal(String), } @@ -104,6 +116,9 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "consumed_attempt_replay", Self::ConsumedRoundReplay { .. } => "consumed_round_replay", Self::ConsumedNonceReplay { .. } => "consumed_nonce_replay", + Self::InteractiveAttemptAlreadyAggregated { .. } => { + "interactive_attempt_already_aggregated" + } Self::Internal(_) => "internal_error", } } @@ -128,6 +143,10 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "recoverable", Self::ConsumedRoundReplay { .. } => "recoverable", Self::ConsumedNonceReplay { .. } => "recoverable", + // The aggregate is deterministic over public data and the attempt + // is durably marked complete; a re-aggregation request is a benign + // duplicate the caller should not retry, not an engine fault. + Self::InteractiveAttemptAlreadyAggregated { .. } => "recoverable", Self::SessionFinalized { .. } => "terminal", Self::SessionNotFound { .. } => "terminal", Self::Internal(_) => "terminal", @@ -170,6 +189,20 @@ mod tests { ); } + #[test] + fn interactive_attempt_already_aggregated_has_stable_code_and_message_format() { + let err = EngineError::InteractiveAttemptAlreadyAggregated { + session_id: "session-a".to_string(), + attempt_id: "attempt-1".to_string(), + }; + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + assert_eq!(err.recovery_class(), "recoverable"); + assert_eq!( + err.to_string(), + "interactive attempt [attempt-1] already aggregated in session [session-a]", + ); + } + #[test] fn recovery_class_maps_retryable_and_terminal_errors() { assert_eq!(