From 90c04ecc88c9886b4358d882a2a23eb8566495f9 Mon Sep 17 00:00:00 2001 From: sam bacha <32783916+sambacha@users.noreply.github.com> Date: Wed, 30 Jul 2025 00:57:11 -0700 Subject: [PATCH 1/3] fix(dirk): condition seperation prevent failed/denied signing attempts from being processed as successful. --- crates/signer/src/manager/dirk.rs | 100 +++++++++++++++++++++++++----- 1 file changed, 85 insertions(+), 15 deletions(-) diff --git a/crates/signer/src/manager/dirk.rs b/crates/signer/src/manager/dirk.rs index 4c2d909f..839728a6 100644 --- a/crates/signer/src/manager/dirk.rs +++ b/crates/signer/src/manager/dirk.rs @@ -173,9 +173,22 @@ impl DirkManager { .proxy_accounts .values() .filter_map(|proxy| { - if proxy.module == *module && - proxy.consensus.public_key() == account.public_key() + let module_matches = proxy.module == *module; + let consensus_matches = proxy.consensus.public_key() == account.public_key(); + + // Debug assertion to ensure our logic is correct + #[cfg(debug_assertions)] { + if module_matches && !consensus_matches { + debug!("Found proxy with matching module but wrong consensus key"); + } + if !module_matches && consensus_matches { + debug!("Found proxy with matching consensus but wrong module"); + } + } + + // Only return if BOTH conditions are met + if module_matches && consensus_matches { Some(proxy.inner.public_key()) } else { None @@ -241,10 +254,29 @@ impl DirkManager { SignerModuleError::DirkCommunicationError(format!("Failed to sign object: {e}")) })?; - if response.get_ref().state() != ResponseState::Succeeded { - return Err(SignerModuleError::DirkCommunicationError( - "Failed to sign object, server responded error".to_string(), - )); + // Get state once to avoid multiple calls + let response_state = response.get_ref().state(); + + // Explicitly check for the success state + match response_state { + ResponseState::Succeeded => { + // Continue with signature parsing + }, + ResponseState::Denied => { + return Err(SignerModuleError::DirkCommunicationError( + "Signing request was denied".to_string() + )); + }, + ResponseState::Failed => { + return Err(SignerModuleError::DirkCommunicationError( + "Signing request failed".to_string() + )); + }, + ResponseState::Unknown => { + return Err(SignerModuleError::DirkCommunicationError( + "Unknown response state".to_string() + )); + }, } BlsSignature::try_from(response.into_inner().signature.as_slice()).map_err(|_| { @@ -286,9 +318,16 @@ impl DirkManager { } }; - if response.get_ref().state() != ResponseState::Succeeded { - warn!("Failed to sign object with participant {participant_id}"); - continue; + // Check response state explicitly + let response_state = response.get_ref().state(); + match response_state { + ResponseState::Succeeded => { + // Continue processing this response + }, + _ => { + warn!("Failed to sign object with participant {participant_id}, state: {:?}", response_state); + continue; + } } let signature = match BlsSignature::try_from(response.into_inner().signature.as_slice()) @@ -302,14 +341,25 @@ impl DirkManager { partials.push((signature, participant_id)); - if partials.len() >= account.threshold as usize { + // Store threshold as usize to avoid repeated casting + let required_threshold = account.threshold as usize; + let current_count = partials.len(); + + // Explicit check with no ambiguity + if current_count >= required_threshold { + debug!("Collected {} signatures, threshold {} met", current_count, required_threshold); break; } } - if partials.len() < account.threshold as usize { + // Final validation after loop with explicit comparison + let required_threshold = account.threshold as usize; + let final_count = partials.len(); + + if final_count < required_threshold { return Err(SignerModuleError::DirkCommunicationError( - "Failed to get enough partial signatures".to_string(), + format!("Failed to get enough partial signatures: got {}, need {}", + final_count, required_threshold) )); } @@ -705,9 +755,29 @@ fn random_password() -> String { /// Returns whether the name of an account has a proxy name format. /// /// i.e., `{wallet}/{consensus_proxy}/{module}/{uuid}` -fn name_matches_proxy(name: &str) -> bool { - name.split("/").count() > 3 && - name.rsplit_once("/").is_some_and(|(_, name)| uuid::Uuid::parse_str(name).is_ok()) +pub(crate) fn name_matches_proxy(name: &str) -> bool { + let parts: Vec<&str> = name.split('/').collect(); + + // Early return for wrong number of parts + if parts.len() != 4 { + return false; + } + + // Check each requirement separately + if parts[0].is_empty() { + return false; // wallet name cannot be empty + } + + if parts[1] != "consensus_proxy" { + return false; // must be exact string + } + + if parts[2].is_empty() { + return false; // module name cannot be empty + } + + // Final check: valid UUID + uuid::Uuid::parse_str(parts[3]).is_ok() } mod test { From 554bc6b9abd365aad4919c377024e9d1aed9643a Mon Sep 17 00:00:00 2001 From: sam bacha <32783916+sambacha@users.noreply.github.com> Date: Wed, 30 Jul 2025 01:04:08 -0700 Subject: [PATCH 2/3] tests(dirk): initial prop tests --- tests/tests/dirk_tests.rs | 592 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 592 insertions(+) create mode 100644 tests/tests/dirk_tests.rs diff --git a/tests/tests/dirk_tests.rs b/tests/tests/dirk_tests.rs new file mode 100644 index 00000000..20cb1af0 --- /dev/null +++ b/tests/tests/dirk_tests.rs @@ -0,0 +1,592 @@ +/// Dirk tests + +#[cfg(test)] +mod dirk_property_tests { + use super::*; + use proptest::prelude::*; + use proptest::collection::vec; + use proptest::string::string_regex; + + /// Name_matches_proxy + mod name_matching_tests { + use super::*; + + // Strategy for generating valid UUID strings + fn uuid_strategy() -> impl Strategy { + "[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}" + .prop_map(|s| s.to_lowercase()) + } + + // Strategy for generating valid wallet names (non-empty alphanumeric) + fn wallet_name_strategy() -> impl Strategy { + "[a-zA-Z][a-zA-Z0-9_]{0,31}" + } + + // Strategy for generating valid module names + fn module_name_strategy() -> impl Strategy { + "[a-zA-Z][a-zA-Z0-9_-]{0,31}" + } + + proptest! { + #[test] + fn test_valid_proxy_names_always_match( + wallet in wallet_name_strategy(), + module in module_name_strategy(), + uuid in uuid_strategy() + ) { + let name = format!("{}/consensus_proxy/{}/{}", wallet, module, uuid); + prop_assert!( + name_matches_proxy(&name), + "Valid proxy name should match: {}", + name + ); + } + + #[test] + fn test_wrong_part_count_never_matches( + parts in vec(string_regex("[^/]+").unwrap(), 0..10) + .prop_filter("not exactly 4 parts", |v| v.len() != 4) + ) { + let name = parts.join("/"); + prop_assert!( + !name_matches_proxy(&name), + "Name with {} parts should not match: {}", + parts.len(), + name + ); + } + + #[test] + fn test_empty_wallet_name_never_matches( + module in module_name_strategy(), + uuid in uuid_strategy() + ) { + let name = format!("/consensus_proxy/{}/{}", module, uuid); + prop_assert!( + !name_matches_proxy(&name), + "Empty wallet name should not match: {}", + name + ); + } + + #[test] + fn test_wrong_consensus_proxy_literal_never_matches( + wallet in wallet_name_strategy(), + wrong_literal in string_regex("[a-zA-Z_]+") + .prop_filter("not consensus_proxy", |s| s != "consensus_proxy"), + module in module_name_strategy(), + uuid in uuid_strategy() + ) { + let name = format!("{}/{}/{}/{}", wallet, wrong_literal, module, uuid); + prop_assert!( + !name_matches_proxy(&name), + "Wrong literal should not match: {}", + name + ); + } + + #[test] + fn test_invalid_uuid_never_matches( + wallet in wallet_name_strategy(), + module in module_name_strategy(), + invalid_uuid in string_regex("[a-zA-Z0-9-]{1,50}") + .prop_filter("not valid UUID", |s| uuid::Uuid::parse_str(s).is_err()) + ) { + let name = format!("{}/consensus_proxy/{}/{}", wallet, module, invalid_uuid); + prop_assert!( + !name_matches_proxy(&name), + "Invalid UUID should not match: {}", + name + ); + } + + #[test] + fn test_empty_module_name_never_matches( + wallet in wallet_name_strategy(), + uuid in uuid_strategy() + ) { + let name = format!("{}/consensus_proxy//{}", wallet, uuid); + prop_assert!( + !name_matches_proxy(&name), + "Empty module name should not match: {}", + name + ); + } + + #[test] + fn test_all_conditions_must_be_met( + wallet in proptest::option::of(wallet_name_strategy()), + literal in proptest::option::of(Just("consensus_proxy".to_string())), + module in proptest::option::of(module_name_strategy()), + uuid in proptest::option::of(uuid_strategy()) + ) { + let name = format!( + "{}/{}/{}/{}", + wallet.as_deref().unwrap_or(""), + literal.as_deref().unwrap_or("wrong"), + module.as_deref().unwrap_or(""), + uuid.as_deref().unwrap_or("not-a-uuid") + ); + + // Should only match if ALL conditions are met + let should_match = wallet.is_some() + && literal.is_some() + && module.is_some() + && uuid.is_some(); + + prop_assert_eq!( + name_matches_proxy(&name), + should_match, + "Name: {} should match: {}", + name, + should_match + ); + } + } + } + + /// Consensus proxy validation + mod consensus_proxy_validation_tests { + use super::*; + use std::collections::HashMap; + + // Mock types for testing + #[derive(Clone, Debug, PartialEq)] + struct MockModuleId(String); + + #[derive(Clone, Debug)] + struct MockProxyAccount { + module: MockModuleId, + consensus_pubkey: Vec, + proxy_pubkey: Vec, + } + + // Strategy for generating public keys + fn pubkey_strategy() -> impl Strategy> { + vec(any::(), 48) + } + + // Strategy for generating module IDs + fn module_id_strategy() -> impl Strategy { + "[a-zA-Z][a-zA-Z0-9_]{0,31}".prop_map(MockModuleId) + } + + // Helper function that mimics the fixed consensus proxy filtering logic + fn filter_proxies_for_consensus_and_module( + proxies: &[MockProxyAccount], + target_module: &MockModuleId, + target_consensus: &[u8], + ) -> Vec> { + proxies + .iter() + .filter_map(|proxy| { + let module_matches = proxy.module == *target_module; + let consensus_matches = proxy.consensus_pubkey == target_consensus; + + // Only return if BOTH conditions are met + if module_matches && consensus_matches { + Some(proxy.proxy_pubkey.clone()) + } else { + None + } + }) + .collect() + } + + proptest! { + #[test] + fn test_both_conditions_required( + target_module in module_id_strategy(), + target_consensus in pubkey_strategy(), + other_module in module_id_strategy() + .prop_filter("different module", |m| true), + other_consensus in pubkey_strategy() + .prop_filter("different consensus", |k| true), + proxy_key in pubkey_strategy() + ) { + prop_assume!(target_module != other_module); + prop_assume!(target_consensus != other_consensus); + + let proxies = vec![ + // Matching module, wrong consensus + MockProxyAccount { + module: target_module.clone(), + consensus_pubkey: other_consensus.clone(), + proxy_pubkey: proxy_key.clone(), + }, + // Wrong module, matching consensus + MockProxyAccount { + module: other_module.clone(), + consensus_pubkey: target_consensus.clone(), + proxy_pubkey: proxy_key.clone(), + }, + // Both match - this should be the only one returned + MockProxyAccount { + module: target_module.clone(), + consensus_pubkey: target_consensus.clone(), + proxy_pubkey: proxy_key.clone(), + }, + ]; + + let filtered = filter_proxies_for_consensus_and_module( + &proxies, + &target_module, + &target_consensus + ); + + prop_assert_eq!( + filtered.len(), + 1, + "Should only return proxy where both conditions match" + ); + prop_assert_eq!( + &filtered[0], + &proxy_key, + "Should return the correct proxy key" + ); + } + + #[test] + fn test_no_matches_returns_empty( + proxies in vec( + (module_id_strategy(), pubkey_strategy(), pubkey_strategy()), + 0..10 + ).prop_map(|v| v.into_iter().map(|(m, c, p)| MockProxyAccount { + module: m, + consensus_pubkey: c, + proxy_pubkey: p, + }).collect::>()), + target_module in module_id_strategy(), + target_consensus in pubkey_strategy() + ) { + // Ensure no proxy matches both conditions + let has_match = proxies.iter().any(|p| + p.module == target_module && p.consensus_pubkey == target_consensus + ); + prop_assume!(!has_match); + + let filtered = filter_proxies_for_consensus_and_module( + &proxies, + &target_module, + &target_consensus + ); + + prop_assert!( + filtered.is_empty(), + "Should return empty when no proxy matches both conditions" + ); + } + + #[test] + fn test_multiple_matching_proxies( + target_module in module_id_strategy(), + target_consensus in pubkey_strategy(), + proxy_keys in vec(pubkey_strategy(), 1..5) + ) { + let proxies: Vec<_> = proxy_keys + .iter() + .map(|key| MockProxyAccount { + module: target_module.clone(), + consensus_pubkey: target_consensus.clone(), + proxy_pubkey: key.clone(), + }) + .collect(); + + let filtered = filter_proxies_for_consensus_and_module( + &proxies, + &target_module, + &target_consensus + ); + + prop_assert_eq!( + filtered.len(), + proxy_keys.len(), + "Should return all matching proxies" + ); + + // Verify all keys are returned + for key in &proxy_keys { + prop_assert!( + filtered.contains(key), + "All matching proxy keys should be returned" + ); + } + } + } + } + + /// Response state validation + mod response_state_validation_tests { + use super::*; + + #[derive(Debug, Clone, Copy, PartialEq)] + enum MockResponseState { + Succeeded, + Denied, + Failed, + Unknown, + } + + // Helper function that mimics the fixed response validation logic + fn validate_response_state(state: MockResponseState) -> Result<(), String> { + match state { + MockResponseState::Succeeded => Ok(()), + MockResponseState::Denied => Err("Signing request was denied".to_string()), + MockResponseState::Failed => Err("Signing request failed".to_string()), + MockResponseState::Unknown => Err("Unknown response state".to_string()), + } + } + + proptest! { + #[test] + fn test_only_succeeded_is_accepted( + state_idx in 0..4usize + ) { + let states = [ + MockResponseState::Succeeded, + MockResponseState::Denied, + MockResponseState::Failed, + MockResponseState::Unknown, + ]; + + let state = states[state_idx]; + let result = validate_response_state(state); + + if state == MockResponseState::Succeeded { + prop_assert!(result.is_ok(), "Succeeded state should be accepted"); + } else { + prop_assert!(result.is_err(), "Non-succeeded state should be rejected"); + let err = result.unwrap_err(); + prop_assert!(!err.is_empty(), "Error message should not be empty"); + + // Verify specific error messages + match state { + MockResponseState::Denied => { + prop_assert!(err.contains("denied")); + } + MockResponseState::Failed => { + prop_assert!(err.contains("failed")); + } + MockResponseState::Unknown => { + prop_assert!(err.contains("Unknown")); + } + _ => {} + } + } + } + + #[test] + fn test_all_states_have_distinct_handling( + states in vec(0..4usize, 2..10) + ) { + let state_types = [ + MockResponseState::Succeeded, + MockResponseState::Denied, + MockResponseState::Failed, + MockResponseState::Unknown, + ]; + + let mut error_messages = std::collections::HashSet::new(); + let mut success_count = 0; + + for &idx in &states { + let state = state_types[idx % 4]; + match validate_response_state(state) { + Ok(()) => success_count += 1, + Err(msg) => { + error_messages.insert(msg); + } + } + } + + // Each error state should have a unique message + let error_state_count = states.iter() + .filter(|&&idx| state_types[idx % 4] != MockResponseState::Succeeded) + .count(); + let unique_error_types = std::cmp::min(3, error_state_count); // Max 3 error types + + prop_assert!( + error_messages.len() <= 3, + "Should have at most 3 distinct error messages" + ); + } + } + } + + /// Threshold comparison logic + /// Test for premature loop termination and potentially a + /// accepting fewer signatures than required for security. + mod threshold_comparison_tests { + use super::*; + + // Helper function that mimics the fixed threshold logic + fn collect_until_threshold( + available: usize, + threshold: usize, + failures: Vec, + ) -> Result { + let mut collected = 0; + + for i in 0..available { + if failures.contains(&i) { + continue; + } + + collected += 1; + + // Store threshold as usize to avoid repeated casting + let required_threshold = threshold; + let current_count = collected; + + // Explicit check with no ambiguity + if current_count >= required_threshold { + break; + } + } + + // Final validation after loop + let required_threshold = threshold; + let final_count = collected; + + if final_count < required_threshold { + Err(format!( + "Failed to get enough partial signatures: got {}, need {}", + final_count, required_threshold + )) + } else { + Ok(final_count) + } + } + + proptest! { + #[test] + fn test_threshold_exact_match( + threshold in 1..10usize, + extra_available in 0..5usize + ) { + let available = threshold + extra_available; + let result = collect_until_threshold(available, threshold, vec![]); + + prop_assert!(result.is_ok()); + let collected = result.unwrap(); + prop_assert_eq!( + collected, + threshold, + "Should collect exactly threshold signatures when all succeed" + ); + } + + #[test] + fn test_threshold_with_failures( + threshold in 2..10usize, + total in 2..20usize, + failure_indices in proptest::collection::vec(0..20usize, 0..10) + ) { + prop_assume!(total >= threshold); + + // Count how many non-failed participants we have + let available_count = (0..total) + .filter(|i| !failure_indices.contains(i)) + .count(); + + let result = collect_until_threshold(total, threshold, failure_indices); + + if available_count >= threshold { + prop_assert!( + result.is_ok(), + "Should succeed when enough participants available" + ); + let collected = result.unwrap(); + prop_assert!( + collected >= threshold, + "Should collect at least threshold signatures" + ); + prop_assert!( + collected <= available_count, + "Cannot collect more than available" + ); + } else { + prop_assert!( + result.is_err(), + "Should fail when not enough participants available" + ); + let err = result.unwrap_err(); + prop_assert!( + err.contains("got") && err.contains("need"), + "Error should show actual vs required" + ); + } + } + + #[test] + fn test_threshold_boundary_conditions( + available in 1..20usize + ) { + // Test threshold = available (exact match) + let result = collect_until_threshold(available, available, vec![]); + prop_assert!(result.is_ok()); + prop_assert_eq!(result.unwrap(), available); + + // Test threshold > available (impossible) + let result = collect_until_threshold(available, available + 1, vec![]); + prop_assert!(result.is_err()); + + // Test threshold = 1 (minimum) + let result = collect_until_threshold(available, 1, vec![]); + prop_assert!(result.is_ok()); + prop_assert_eq!(result.unwrap(), 1); + } + + #[test] + fn test_zero_threshold_edge_case( + available in 1..10usize + ) { + // In practice, threshold should never be 0, but let's test the logic + let result = collect_until_threshold(available, 0, vec![]); + prop_assert!( + result.is_ok(), + "Zero threshold should succeed immediately" + ); + prop_assert_eq!( + result.unwrap(), + 0, + "Should collect 0 signatures for 0 threshold" + ); + } + + #[test] + fn test_deterministic_collection_order( + threshold in 1..10usize, + total in 10..20usize, + failure_pattern in vec(bool, 20) + ) { + prop_assume!(total >= threshold); + + let failures: Vec = failure_pattern + .iter() + .take(total) + .enumerate() + .filter_map(|(i, &fail)| if fail { Some(i) } else { None }) + .collect(); + + let available_count = total - failures.len(); + prop_assume!(available_count >= threshold); + + // Run multiple times to ensure deterministic behavior + let results: Vec<_> = (0..5) + .map(|_| collect_until_threshold(total, threshold, failures.clone())) + .collect(); + + // All runs should have the same result + let first_result = &results[0]; + for result in &results[1..] { + prop_assert_eq!( + result, + first_result, + "Collection should be deterministic" + ); + } + } + } + } +} From f5b3bae85a46a03c453bd3dfd75a0e6a14b052d5 Mon Sep 17 00:00:00 2001 From: sam bacha <32783916+sambacha@users.noreply.github.com> Date: Wed, 30 Jul 2025 01:05:18 -0700 Subject: [PATCH 3/3] chore(cargo): add proptest dev-dep dirk_test.rs requires proptest --- crates/signer/Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/signer/Cargo.toml b/crates/signer/Cargo.toml index 569797ac..b06a637a 100644 --- a/crates/signer/Cargo.toml +++ b/crates/signer/Cargo.toml @@ -31,3 +31,6 @@ uuid.workspace = true [build-dependencies] tonic-build.workspace = true + +[dev-dependencies] +proptest = "1.4"