From 6bdc92ebfc4464fb79115c3134028e508ef0d1c9 Mon Sep 17 00:00:00 2001 From: William Smith Date: Tue, 5 Nov 2024 09:54:46 -0800 Subject: [PATCH] address review comment --- crates/sui-core/src/authority_server.rs | 39 ++++++++++++------- crates/sui-core/src/traffic_controller/mod.rs | 9 ++--- .../tests/traffic_control_tests.rs | 35 ++++++++++------- 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/crates/sui-core/src/authority_server.rs b/crates/sui-core/src/authority_server.rs index 6c8bdc956c391..af4d80ae342d4 100644 --- a/crates/sui-core/src/authority_server.rs +++ b/crates/sui-core/src/authority_server.rs @@ -198,6 +198,7 @@ pub struct ValidatorServiceMetrics { forwarded_header_parse_error: IntCounter, forwarded_header_invalid: IntCounter, forwarded_header_not_included: IntCounter, + client_id_source_config_mismatch: IntCounter, } impl ValidatorServiceMetrics { @@ -329,6 +330,12 @@ impl ValidatorServiceMetrics { registry, ) .unwrap(), + client_id_source_config_mismatch: register_int_counter_with_registry!( + "validator_service_client_id_source_config_mismatch", + "Number of times detected that client id source config doesn't agree with x-forwarded-for header", + registry, + ) + .unwrap(), } } @@ -1225,19 +1232,19 @@ impl ValidatorService { return None; } let contents_len = header_contents.len(); - // Network topology should not be very dynamic, therefore if it changes and the above - // invariant is violated, we should fail loudly so that the node config can be updated. - assert!( - contents_len >= *num_hops, - "x-forwarded-for header value of {:?} contains {} values, but {} hops were specified. \ - Expected at least {} values. Please correctly set the `x-forwarded-for` value under \ - `client-id-source` in the node config.", - header_contents, - contents_len, - num_hops, - contents_len, - ); - let contents_len = header_contents.len(); + if contents_len < *num_hops { + error!( + "x-forwarded-for header value of {:?} contains {} values, but {} hops were specified. \ + Expected at least {} values. Please correctly set the `x-forwarded-for` value under \ + `client-id-source` in the node config.", + header_contents, + contents_len, + num_hops, + contents_len, + ); + self.metrics.client_id_source_config_mismatch.inc(); + return None; + } let Some(client_ip) = header_contents.get(contents_len - num_hops) else { error!( @@ -1337,8 +1344,10 @@ fn make_tonic_request_for_testing(message: T) -> tonic::Request { // TODO: refine error matching here fn normalize(err: SuiError) -> Weight { match err { - SuiError::UserInputError { .. } - | SuiError::InvalidSignature { .. } + SuiError::UserInputError { + error: UserInputError::IncorrectUserSignature { .. }, + } => Weight::one(), + SuiError::InvalidSignature { .. } | SuiError::SignerSignatureAbsent { .. } | SuiError::SignerSignatureNumberMismatch { .. } | SuiError::IncorrectSigner { .. } diff --git a/crates/sui-core/src/traffic_controller/mod.rs b/crates/sui-core/src/traffic_controller/mod.rs index f759e1d58ee75..8d1dbbe0a8c31 100644 --- a/crates/sui-core/src/traffic_controller/mod.rs +++ b/crates/sui-core/src/traffic_controller/mod.rs @@ -284,7 +284,6 @@ async fn run_clear_blocklists_loop(blocklists: Blocklists, metrics: Arc Result<(), anyhow::Error> { @@ -225,7 +228,7 @@ async fn test_validator_traffic_control_error_blocked() -> Result<(), anyhow::Er .with_policy_config(Some(policy_config)) .build(); let committee = network_config.committee_with_network(); - let _test_cluster = TestClusterBuilder::new() + let test_cluster = TestClusterBuilder::new() .set_network_config(network_config) .build() .await; @@ -235,12 +238,13 @@ async fn test_validator_traffic_control_error_blocked() -> Result<(), anyhow::Er ); let (_, auth_client) = local_clients.first_key_value().unwrap(); - // transaction signed using user wallet from a different chain/genesis, - // therefore we should fail with UserInputError - let other_cluster = TestClusterBuilder::new().build().await; - - let mut txns = batch_make_transfer_transactions(&other_cluster.wallet, n as usize).await; - let tx = txns.swap_remove(0); + let mut txns = batch_make_transfer_transactions(&test_cluster.wallet, n as usize).await; + let mut tx = txns.swap_remove(0); + let signatures = tx.tx_signatures_mut_for_testing(); + signatures.pop(); + signatures.push(GenericSignature::Signature( + sui_types::crypto::Signature::Ed25519SuiSignature(Ed25519SuiSignature::default()), + )); // it should take no more than 4 requests to be added to the blocklist for _ in 0..n { @@ -251,7 +255,7 @@ async fn test_validator_traffic_control_error_blocked() -> Result<(), anyhow::Er } } } - panic!("Expected spam policy to trigger within {n} requests"); + panic!("Expected error policy to trigger within {n} requests"); } #[tokio::test] @@ -406,7 +410,7 @@ async fn test_validator_traffic_control_error_delegated() -> Result<(), anyhow:: .with_firewall_config(Some(firewall_config)) .build(); let committee = network_config.committee_with_network(); - let _test_cluster = TestClusterBuilder::new() + let test_cluster = TestClusterBuilder::new() .set_network_config(network_config) .build() .await; @@ -416,12 +420,13 @@ async fn test_validator_traffic_control_error_delegated() -> Result<(), anyhow:: ); let (_, auth_client) = local_clients.first_key_value().unwrap(); - // transaction signed using user wallet from a different chain/genesis, - // therefore we should fail with UserInputError - let other_cluster = TestClusterBuilder::new().build().await; - - let mut txns = batch_make_transfer_transactions(&other_cluster.wallet, n as usize).await; - let tx = txns.swap_remove(0); + let mut txns = batch_make_transfer_transactions(&test_cluster.wallet, n as usize).await; + let mut tx = txns.swap_remove(0); + let signatures = tx.tx_signatures_mut_for_testing(); + signatures.pop(); + signatures.push(GenericSignature::Signature( + sui_types::crypto::Signature::Ed25519SuiSignature(Ed25519SuiSignature::default()), + )); // start test firewall server let mut server = NodeFwTestServer::new();