From eb1f8e6bf5a5e34222959f717ca4d5b086ef32bd Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Wed, 18 Dec 2024 13:48:58 -0500 Subject: [PATCH 1/6] chore: Log HTTP request body on signature verification failure There have been reports both internally and in support channels of ingress messages failing signature verification. So far these have not been reproducible, and the cause is unknown. To assist debugging, if verification fails log the HTTP request body. --- rs/http_endpoints/public/src/call.rs | 2 +- rs/http_endpoints/public/src/common.rs | 14 ++++++++++++-- rs/http_endpoints/public/src/query.rs | 2 +- .../public/src/read_state/canister.rs | 3 ++- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/rs/http_endpoints/public/src/call.rs b/rs/http_endpoints/public/src/call.rs index a02984f6a5d..0cd865df34d 100644 --- a/rs/http_endpoints/public/src/call.rs +++ b/rs/http_endpoints/public/src/call.rs @@ -251,7 +251,7 @@ impl IngressValidator { message: "".into(), })? .map_err(|validation_error| { - validation_error_to_http_error(message_id, validation_error, &log) + validation_error_to_http_error(msg.signed(), message_id, validation_error, &log) })?; let ingress_filter = ingress_filter.lock().unwrap().clone(); diff --git a/rs/http_endpoints/public/src/common.rs b/rs/http_endpoints/public/src/common.rs index 1f40f4c7e35..de0854c366c 100644 --- a/rs/http_endpoints/public/src/common.rs +++ b/rs/http_endpoints/public/src/common.rs @@ -246,12 +246,22 @@ impl IntoResponse for CborUserError { } } -pub(crate) fn validation_error_to_http_error( +pub(crate) fn validation_error_to_http_error( + request: &HttpRequest, message_id: MessageId, err: RequestValidationError, log: &ReplicaLogger, ) -> HttpError { - info!(log, "msg_id: {}, err: {}", message_id, err); + match err { + RequestValidationError::InvalidSignature(_) => { + info!( + log, + "msg_id: {}, err: {}, request: {:?}", message_id, err, request.content() + ) + } + _ => info!(log, "msg_id: {}, err: {}", message_id, err), + } + HttpError { status: StatusCode::BAD_REQUEST, message: format!("{err}"), diff --git a/rs/http_endpoints/public/src/query.rs b/rs/http_endpoints/public/src/query.rs index a68bdcceeb4..603a59447a9 100644 --- a/rs/http_endpoints/public/src/query.rs +++ b/rs/http_endpoints/public/src/query.rs @@ -194,7 +194,7 @@ pub(crate) async fn query( { Ok(Ok(_)) => {} Ok(Err(err)) => { - let http_err = validation_error_to_http_error(request.id(), err, &log); + let http_err = validation_error_to_http_error(&request, request.id(), err, &log); return (http_err.status, http_err.message).into_response(); } Err(_) => { diff --git a/rs/http_endpoints/public/src/read_state/canister.rs b/rs/http_endpoints/public/src/read_state/canister.rs index 1a491c8f036..11dfaf8243b 100644 --- a/rs/http_endpoints/public/src/read_state/canister.rs +++ b/rs/http_endpoints/public/src/read_state/canister.rs @@ -169,7 +169,8 @@ pub(crate) async fn canister_read_state( match validator.validate_request(&request_c, current_time(), &root_of_trust_provider) { Ok(targets) => targets, Err(err) => { - let http_err = validation_error_to_http_error(request.id(), err, &log); + let http_err = + validation_error_to_http_error(&request, request.id(), err, &log); return (http_err.status, http_err.message).into_response(); } }; From 6895f6b3ae28418267835976b5d0f0cd19f09ceb Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Wed, 18 Dec 2024 14:22:31 -0500 Subject: [PATCH 2/6] fmt --- rs/http_endpoints/public/src/common.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rs/http_endpoints/public/src/common.rs b/rs/http_endpoints/public/src/common.rs index de0854c366c..6e7cd825689 100644 --- a/rs/http_endpoints/public/src/common.rs +++ b/rs/http_endpoints/public/src/common.rs @@ -256,7 +256,10 @@ pub(crate) fn validation_error_to_http_error( RequestValidationError::InvalidSignature(_) => { info!( log, - "msg_id: {}, err: {}, request: {:?}", message_id, err, request.content() + "msg_id: {}, err: {}, request: {:?}", + message_id, + err, + request.content() ) } _ => info!(log, "msg_id: {}, err: {}", message_id, err), From 6350684695c4739160e342fd6c2832ba6c5cba21 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Wed, 18 Dec 2024 14:22:36 -0500 Subject: [PATCH 3/6] missing add --- rs/types/types/src/messages/ingress_messages.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rs/types/types/src/messages/ingress_messages.rs b/rs/types/types/src/messages/ingress_messages.rs index 212bb2d285b..5dc6cd2e854 100644 --- a/rs/types/types/src/messages/ingress_messages.rs +++ b/rs/types/types/src/messages/ingress_messages.rs @@ -282,6 +282,10 @@ impl SignedIngress { self.signed.content() } + pub fn signed(&self) -> &HttpRequest { + &self.signed + } + pub fn authentication(&self) -> &Authentication { self.signed.authentication() } From 3726c62adedb724d72d348ac07478a8d18503b86 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Fri, 10 Jan 2025 12:23:00 -0500 Subject: [PATCH 4/6] Address review comments --- rs/http_endpoints/public/src/call.rs | 2 +- rs/http_endpoints/public/src/common.rs | 6 +++--- rs/http_endpoints/public/src/query.rs | 2 +- rs/http_endpoints/public/src/read_state/canister.rs | 3 +-- rs/types/types/src/messages/ingress_messages.rs | 4 ---- 5 files changed, 6 insertions(+), 11 deletions(-) diff --git a/rs/http_endpoints/public/src/call.rs b/rs/http_endpoints/public/src/call.rs index 0cd865df34d..d2a3a1bc3f1 100644 --- a/rs/http_endpoints/public/src/call.rs +++ b/rs/http_endpoints/public/src/call.rs @@ -251,7 +251,7 @@ impl IngressValidator { message: "".into(), })? .map_err(|validation_error| { - validation_error_to_http_error(msg.signed(), message_id, validation_error, &log) + validation_error_to_http_error(msg.as_ref(), validation_error, &log) })?; let ingress_filter = ingress_filter.lock().unwrap().clone(); diff --git a/rs/http_endpoints/public/src/common.rs b/rs/http_endpoints/public/src/common.rs index 6e7cd825689..d83b39ed5cd 100644 --- a/rs/http_endpoints/public/src/common.rs +++ b/rs/http_endpoints/public/src/common.rs @@ -20,7 +20,7 @@ use ic_replicated_state::ReplicatedState; use ic_types::{ crypto::threshold_sig::ThresholdSigPublicKey, malicious_flags::MaliciousFlags, - messages::{HttpRequest, HttpRequestContent, MessageId}, + messages::{HttpRequest, HttpRequestContent}, RegistryVersion, SubnetId, Time, }; use ic_validator::{ @@ -246,12 +246,12 @@ impl IntoResponse for CborUserError { } } -pub(crate) fn validation_error_to_http_error( +pub(crate) fn validation_error_to_http_error( request: &HttpRequest, - message_id: MessageId, err: RequestValidationError, log: &ReplicaLogger, ) -> HttpError { + let message_id = request.id(); match err { RequestValidationError::InvalidSignature(_) => { info!( diff --git a/rs/http_endpoints/public/src/query.rs b/rs/http_endpoints/public/src/query.rs index 603a59447a9..27bc9b465b1 100644 --- a/rs/http_endpoints/public/src/query.rs +++ b/rs/http_endpoints/public/src/query.rs @@ -194,7 +194,7 @@ pub(crate) async fn query( { Ok(Ok(_)) => {} Ok(Err(err)) => { - let http_err = validation_error_to_http_error(&request, request.id(), err, &log); + let http_err = validation_error_to_http_error(&request, err, &log); return (http_err.status, http_err.message).into_response(); } Err(_) => { diff --git a/rs/http_endpoints/public/src/read_state/canister.rs b/rs/http_endpoints/public/src/read_state/canister.rs index 11dfaf8243b..1a9f191695b 100644 --- a/rs/http_endpoints/public/src/read_state/canister.rs +++ b/rs/http_endpoints/public/src/read_state/canister.rs @@ -169,8 +169,7 @@ pub(crate) async fn canister_read_state( match validator.validate_request(&request_c, current_time(), &root_of_trust_provider) { Ok(targets) => targets, Err(err) => { - let http_err = - validation_error_to_http_error(&request, request.id(), err, &log); + let http_err = validation_error_to_http_error(&request, err, &log); return (http_err.status, http_err.message).into_response(); } }; diff --git a/rs/types/types/src/messages/ingress_messages.rs b/rs/types/types/src/messages/ingress_messages.rs index 5dc6cd2e854..212bb2d285b 100644 --- a/rs/types/types/src/messages/ingress_messages.rs +++ b/rs/types/types/src/messages/ingress_messages.rs @@ -282,10 +282,6 @@ impl SignedIngress { self.signed.content() } - pub fn signed(&self) -> &HttpRequest { - &self.signed - } - pub fn authentication(&self) -> &Authentication { self.signed.authentication() } From 0f839b683aaa1d3a37b0c5e014152f4a607c715b Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Fri, 10 Jan 2025 13:51:40 -0500 Subject: [PATCH 5/6] Log the entire request rather than just the body --- rs/http_endpoints/public/src/common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/http_endpoints/public/src/common.rs b/rs/http_endpoints/public/src/common.rs index d83b39ed5cd..f73a034a40c 100644 --- a/rs/http_endpoints/public/src/common.rs +++ b/rs/http_endpoints/public/src/common.rs @@ -259,7 +259,7 @@ pub(crate) fn validation_error_to_http_error info!(log, "msg_id: {}, err: {}", message_id, err), From 52ac9b6e8b28f4ae16ff097a98c806ba568322be Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Fri, 10 Jan 2025 14:02:40 -0500 Subject: [PATCH 6/6] fmt --- rs/http_endpoints/public/src/common.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rs/http_endpoints/public/src/common.rs b/rs/http_endpoints/public/src/common.rs index f73a034a40c..60d34d92d35 100644 --- a/rs/http_endpoints/public/src/common.rs +++ b/rs/http_endpoints/public/src/common.rs @@ -256,10 +256,7 @@ pub(crate) fn validation_error_to_http_error { info!( log, - "msg_id: {}, err: {}, request: {:?}", - message_id, - err, - request + "msg_id: {}, err: {}, request: {:?}", message_id, err, request ) } _ => info!(log, "msg_id: {}, err: {}", message_id, err),