diff --git a/rs/http_endpoints/fuzz/BUILD.bazel b/rs/http_endpoints/fuzz/BUILD.bazel index 9be7bcdf3fc0..16ff738daaa2 100644 --- a/rs/http_endpoints/fuzz/BUILD.bazel +++ b/rs/http_endpoints/fuzz/BUILD.bazel @@ -54,6 +54,7 @@ DEV_DEPENDENCIES = [ "//rs/crypto/tls_interfaces", "//rs/crypto/tls_interfaces/mocks", "//rs/crypto/tree_hash", + "//rs/http_endpoints/test_agent", "//rs/interfaces/mocks", "//rs/interfaces/state_manager", "//rs/interfaces/state_manager/mocks", diff --git a/rs/http_endpoints/public/src/call.rs b/rs/http_endpoints/public/src/call.rs index ee3f63fc5748..13285117ad26 100644 --- a/rs/http_endpoints/public/src/call.rs +++ b/rs/http_endpoints/public/src/call.rs @@ -119,6 +119,11 @@ pub(crate) enum IngressError { UserError(UserError), } +pub(crate) enum EffectiveDestination { + Canister(CanisterId), + Subnet(SubnetId), +} + impl From for IngressError { fn from(err: HttpError) -> Self { IngressError::HttpError(err) @@ -206,7 +211,7 @@ impl IngressValidator { pub(crate) async fn validate_ingress_message( self, request: HttpRequestEnvelope, - effective_canister_id: CanisterId, + effective_destination: EffectiveDestination, ) -> Result { let Self { log, @@ -243,20 +248,50 @@ impl IngressValidator { message: format!("Could not parse body as call message: {e}"), })?; - // Reject requests where `canister_id` != `effective_canister_id` for non mgmt canister calls. - // This needs to be enforced because boundary nodes block access based on the `effective_canister_id` - // in the url and the replica processes the request based on the `canister_id`. - // If this is not enforced, a blocked canisters can still be accessed by specifying - // a non-blocked `effective_canister_id` and a blocked `canister_id`. - if msg.canister_id() != CanisterId::ic_00() && msg.canister_id() != effective_canister_id { - Err(HttpError { - status: StatusCode::BAD_REQUEST, - message: format!( - "Specified CanisterId {} does not match effective canister id in URL {}", - msg.canister_id(), - effective_canister_id - ), - })?; + match effective_destination { + // Reject requests where `canister_id` != `effective_canister_id` for non mgmt canister calls. + // This needs to be enforced because boundary nodes block access based on the `effective_canister_id` + // in the url and the replica processes the request based on the `canister_id`. + // If this is not enforced, a blocked canisters can still be accessed by specifying + // a non-blocked `effective_canister_id` and a blocked `canister_id`. + EffectiveDestination::Canister(effective_canister_id) => { + if msg.canister_id() != CanisterId::ic_00() + && msg.canister_id() != effective_canister_id + { + Err(HttpError { + status: StatusCode::BAD_REQUEST, + message: format!( + "Specified CanisterId {} does not match effective canister id in URL {}", + msg.canister_id(), + effective_canister_id + ), + })?; + } + } + EffectiveDestination::Subnet(effective_subnet_id) => { + if effective_subnet_id != subnet_id { + Err(HttpError { + status: StatusCode::BAD_REQUEST, + message: format!( + "Specified SubnetId {} does not match the subnet id of this node {}", + effective_subnet_id, subnet_id + ), + })?; + } + if msg.canister_id() != CanisterId::ic_00() + || msg.method_name() != "create_canister" + { + Err(HttpError { + status: StatusCode::BAD_REQUEST, + message: format!( + "Subnet call endpoint only accepts calls to the management canister ({}) 'create_canister' method, got canister_id={} method_name='{}'", + CanisterId::ic_00(), + msg.canister_id(), + msg.method_name() + ), + })?; + } + } } let message_id = msg.id(); diff --git a/rs/http_endpoints/public/src/call/call_async.rs b/rs/http_endpoints/public/src/call/call_async.rs index 72f822269358..0ae0c585da51 100644 --- a/rs/http_endpoints/public/src/call/call_async.rs +++ b/rs/http_endpoints/public/src/call/call_async.rs @@ -1,6 +1,6 @@ //! Module that deals with requests to /api/v2/canister/.../call -use super::{IngressError, IngressValidator, IngressWatcherHandle}; +use super::{EffectiveDestination, IngressError, IngressValidator, IngressWatcherHandle}; use crate::{ HttpError, common::{Cbor, CborUserError, WithTimeout}, @@ -112,7 +112,10 @@ async fn handler( let logger = ingress_validator.log.clone(); let ingress_submitter = ingress_validator - .validate_ingress_message(request, effective_canister_id) + .validate_ingress_message( + request, + EffectiveDestination::Canister(effective_canister_id), + ) .await?; let message_id = ingress_submitter.message_id(); diff --git a/rs/http_endpoints/public/src/call/call_sync.rs b/rs/http_endpoints/public/src/call/call_sync.rs index be4cbd42b0c9..e2900dc5aaaf 100644 --- a/rs/http_endpoints/public/src/call/call_sync.rs +++ b/rs/http_endpoints/public/src/call/call_sync.rs @@ -1,7 +1,7 @@ //! Module that deals with requests to /api/{v3,v4}/canister/.../call. use super::{ - IngressError, IngressValidator, + EffectiveDestination, IngressError, IngressValidator, ingress_watcher::{IngressWatcherHandle, SubscriptionError}, }; use crate::{ @@ -33,7 +33,7 @@ use ic_logger::{error, warn}; use ic_nns_delegation_manager::{CanisterRangesFilter, NNSDelegationReader}; use ic_replicated_state::ReplicatedState; use ic_types::{ - CanisterId, + CanisterId, PrincipalId, SubnetId, consensus::certification::Certification, messages::{Blob, Certificate, HttpCallContent, HttpRequestEnvelope, MessageId}, }; @@ -54,6 +54,8 @@ pub enum Version { V3, // Synchronous endpoint with the NNS delegation using the tree format of the canister ranges. V4, + // Synchronous subnet endpoint with no canister ranges in the NNS delegation. + SubnetV4, } enum SyncCallResponse { @@ -133,6 +135,7 @@ pub(crate) fn route(version: Version) -> &'static str { match version { Version::V3 => "/api/v3/canister/{effective_canister_id}/call", Version::V4 => "/api/v4/canister/{effective_canister_id}/call", + Version::SubnetV4 => "/api/v4/subnet/{effective_subnet_id}/call", } } @@ -184,9 +187,9 @@ pub fn new_service( BoxCloneService::new(router.into_service()) } -/// Handles a call to /api/{v3,v4}/canister/../call +/// Handles a call to /api/{v3,v4}/canister/../call and /api/v4/subnet/../call async fn call_sync( - axum::extract::Path(effective_canister_id): axum::extract::Path, + axum::extract::Path(id): axum::extract::Path, State(SynchronousCallHandlerState { call_handler, ingress_watcher_handle, @@ -198,10 +201,30 @@ async fn call_sync( }): State, WithTimeout(Cbor(request)): WithTimeout>>, ) -> SyncCallResponse { + let (effective_destination, delegation_filter) = match version { + Version::V3 => { + let canister_id = CanisterId::unchecked_from_principal(id); + ( + EffectiveDestination::Canister(canister_id), + CanisterRangesFilter::Flat, + ) + } + Version::V4 => { + let canister_id = CanisterId::unchecked_from_principal(id); + ( + EffectiveDestination::Canister(canister_id), + CanisterRangesFilter::Tree(canister_id), + ) + } + Version::SubnetV4 => ( + EffectiveDestination::Subnet(SubnetId::from(id)), + CanisterRangesFilter::None, + ), + }; let log = call_handler.log.clone(); let ingress_submitter = match call_handler - .validate_ingress_message(request, effective_canister_id) + .validate_ingress_message(request, effective_destination) .await { Ok(ingress_submitter) => ingress_submitter, @@ -217,11 +240,6 @@ async fn call_sync( tree_and_certificate_for_message(state_reader.clone(), message_id.clone()).await && let ParsedMessageStatus::Known(_) = parsed_message_status(&tree, &message_id) { - let delegation_from_nns = match version { - Version::V3 => nns_delegation_reader.get_delegation(CanisterRangesFilter::Flat), - Version::V4 => nns_delegation_reader - .get_delegation(CanisterRangesFilter::Tree(effective_canister_id)), - }; let signature = certification.signed.signature.signature.get().0; metrics @@ -232,7 +250,7 @@ async fn call_sync( return SyncCallResponse::Certificate(Certificate { tree, signature: Blob(signature), - delegation: delegation_from_nns, + delegation: nns_delegation_reader.get_delegation(delegation_filter), }); }; @@ -335,18 +353,12 @@ async fn call_sync( ); } - let delegation_from_nns = match version { - Version::V3 => nns_delegation_reader.get_delegation(CanisterRangesFilter::Flat), - Version::V4 => { - nns_delegation_reader.get_delegation(CanisterRangesFilter::Tree(effective_canister_id)) - } - }; let signature = certification.signed.signature.signature.get().0; SyncCallResponse::Certificate(Certificate { tree, signature: Blob(signature), - delegation: delegation_from_nns, + delegation: nns_delegation_reader.get_delegation(delegation_filter), }) } diff --git a/rs/http_endpoints/public/src/lib.rs b/rs/http_endpoints/public/src/lib.rs index e5b23c77933e..a2f55ab26782 100644 --- a/rs/http_endpoints/public/src/lib.rs +++ b/rs/http_endpoints/public/src/lib.rs @@ -131,6 +131,7 @@ struct HttpHandler { call_v2_router: Router, call_v3_router: Router, call_v4_router: Router, + subnet_call_v4_router: Router, query_v2_router: Router, query_v3_router: Router, catchup_router: Router, @@ -328,6 +329,8 @@ pub fn start_server( let call_v3_router = call_sync_router(call_sync::Version::V3); let call_v4_router = call_sync_router(call_sync::Version::V4); + let subnet_call_v4_router = call_sync_router(call_sync::Version::SubnetV4); + let query_router = |version| { QueryServiceBuilder::builder( log.clone(), @@ -420,6 +423,7 @@ pub fn start_server( call_v2_router, call_v3_router, call_v4_router, + subnet_call_v4_router, query_v2_router, query_v3_router, status_router, @@ -607,6 +611,7 @@ fn make_router( // TODO(CON-1574): see if there is any reasonable explicit concurrency limit we could use here. .merge(http_handler.call_v3_router) .merge(http_handler.call_v4_router) + .merge(http_handler.subnet_call_v4_router) .merge(http_handler.query_v2_router.layer(service_builder( GlobalConcurrencyLimitLayer::new(config.max_query_concurrent_requests), ))) @@ -809,6 +814,10 @@ pub(crate) mod tests { call_sync::route(call_sync::Version::V4), axum::routing::post(dummy), ), + subnet_call_v4_router: Router::new().route( + call_sync::route(call_sync::Version::SubnetV4), + axum::routing::post(dummy), + ), query_v2_router: Router::new().route( QueryService::route(query::Version::V2), axum::routing::post(dummy_cbor), diff --git a/rs/http_endpoints/public/tests/common/mod.rs b/rs/http_endpoints/public/tests/common/mod.rs index 9b2e12330515..fdf11cc0c8f4 100644 --- a/rs/http_endpoints/public/tests/common/mod.rs +++ b/rs/http_endpoints/public/tests/common/mod.rs @@ -48,7 +48,7 @@ use ic_replicated_state::{ }; use ic_test_utilities_types::ids::{node_test_id, subnet_test_id}; use ic_types::{ - CryptoHashOfPartialState, Height, RegistryVersion, + CanisterId, CryptoHashOfPartialState, Height, RegistryVersion, artifact::UnvalidatedArtifactMutation, batch::RawQueryStats, consensus::certification::{Certification, CertificationContent}, @@ -87,6 +87,36 @@ use tower_test::mock::Handle; pub type IngressFilterHandle = Handle; pub type QueryExecutionHandle = Handle; +/// Unified endpoint type used to parameterise tests that cover both the canister and subnet +/// synchronous call paths. +#[derive(Copy, Clone, Debug)] +pub enum UpdateEndpoint { + Canister(ic_http_endpoints_test_agent::Call), + Subnet(ic_http_endpoints_test_agent::CallSubnet), +} + +impl UpdateEndpoint { + pub async fn call( + self, + addr: SocketAddr, + message: ic_http_endpoints_test_agent::IngressMessage, + ) -> reqwest::Response { + match self { + UpdateEndpoint::Canister(c) => c.call(addr, message).await, + UpdateEndpoint::Subnet(s) => s.call(addr, message).await, + } + } + + pub fn default_ingress_message(&self) -> ic_http_endpoints_test_agent::IngressMessage { + match self { + UpdateEndpoint::Canister(_) => ic_http_endpoints_test_agent::IngressMessage::default(), + UpdateEndpoint::Subnet(_) => ic_http_endpoints_test_agent::IngressMessage::default() + .with_canister_id(CanisterId::ic_00().get(), CanisterId::ic_00().get()) + .with_method_name("create_canister".to_string()), + } + } +} + fn setup_query_execution_mock() -> (QueryExecutionService, QueryExecutionHandle) { let (service, handle) = tower_test::mock::pair::(); diff --git a/rs/http_endpoints/public/tests/load_shed_test.rs b/rs/http_endpoints/public/tests/load_shed_test.rs index 065b155aa4cf..e7709506d8d2 100644 --- a/rs/http_endpoints/public/tests/load_shed_test.rs +++ b/rs/http_endpoints/public/tests/load_shed_test.rs @@ -1,7 +1,7 @@ pub mod common; use crate::common::{ - HttpEndpointBuilder, MockIngressPoolThrottler, default_certified_state_reader, + HttpEndpointBuilder, MockIngressPoolThrottler, UpdateEndpoint, default_certified_state_reader, default_get_latest_state, default_read_certified_state, get_free_localhost_socket_addr, }; use async_trait::async_trait; @@ -13,8 +13,10 @@ use ic_crypto_tree_hash::{Label, Path}; use ic_http_endpoints_public::query; use ic_http_endpoints_public::read_state; use ic_http_endpoints_test_agent::{ - self, Call, CanisterReadState, IngressMessage, Query, wait_for_status_healthy, + self, Call, CallSubnet, CanisterReadState, IngressMessage, Query, wait_for_status_healthy, }; +use ic_test_utilities_types::ids::subnet_test_id; + use ic_interfaces_state_manager_mocks::MockStateManager; use ic_pprof::{Error, PprofCollector}; use ic_types::PrincipalId; @@ -407,10 +409,11 @@ fn test_load_shedding_update_call() { /// Test that the call endpoints load shed requests when the ingress pool is full. #[rstest] -#[case::v2_endpoint(Call::V2)] -#[case::v3_endpoint(Call::V3)] -#[case::v4_endpoint(Call::V4)] -fn test_load_shedding_update_call_when_ingress_pool_is_full(#[case] endpoint: Call) { +#[case::v2_endpoint(UpdateEndpoint::Canister(Call::V2))] +#[case::v3_endpoint(UpdateEndpoint::Canister(Call::V3))] +#[case::v4_endpoint(UpdateEndpoint::Canister(Call::V4))] +#[case::v4_subnet_endpoint(UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())))] +fn test_load_shedding_update_call_when_ingress_pool_is_full(#[case] endpoint: UpdateEndpoint) { use std::sync::RwLock; let rt = Runtime::new().unwrap(); @@ -445,10 +448,11 @@ fn test_load_shedding_update_call_when_ingress_pool_is_full(#[case] endpoint: Ca /// Test that the call endpoints load shed requests when the ingress channel is full. #[rstest] -#[case::v2_endpoint(Call::V2)] -#[case::v3_endpoint(Call::V3)] -#[case::v4_endpoint(Call::V4)] -fn test_load_shedding_update_call_when_ingress_channel_is_full(#[case] endpoint: Call) { +#[case::v2_endpoint(UpdateEndpoint::Canister(Call::V2))] +#[case::v3_endpoint(UpdateEndpoint::Canister(Call::V3))] +#[case::v4_endpoint(UpdateEndpoint::Canister(Call::V4))] +#[case::v4_subnet_endpoint(UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())))] +fn test_load_shedding_update_call_when_ingress_channel_is_full(#[case] endpoint: UpdateEndpoint) { let rt = Runtime::new().unwrap(); let addr = get_free_localhost_socket_addr(); @@ -473,7 +477,7 @@ fn test_load_shedding_update_call_when_ingress_channel_is_full(#[case] endpoint: rt.block_on(async move { wait_for_status_healthy(&addr).await.unwrap(); for _ in 0..capacity { - let message = Default::default(); + let message = endpoint.default_ingress_message(); let call_response = endpoint.call(addr, message).await; assert_eq!( call_response.status(), @@ -482,7 +486,7 @@ fn test_load_shedding_update_call_when_ingress_channel_is_full(#[case] endpoint: call_response.text().await.unwrap() ); } - let message = Default::default(); + let message = endpoint.default_ingress_message(); let call_response = endpoint.call(addr, message).await; assert_eq!( call_response.status(), diff --git a/rs/http_endpoints/public/tests/test.rs b/rs/http_endpoints/public/tests/test.rs index 8dee019809e3..094bd6b62a52 100644 --- a/rs/http_endpoints/public/tests/test.rs +++ b/rs/http_endpoints/public/tests/test.rs @@ -4,7 +4,7 @@ pub mod common; use crate::common::{ - HttpEndpointBuilder, basic_state_manager_mock, create_conn_and_send_request, + HttpEndpointBuilder, UpdateEndpoint, basic_state_manager_mock, create_conn_and_send_request, default_get_latest_state, default_read_certified_state, get_free_localhost_socket_addr, }; use axum::body::{Body, to_bytes}; @@ -28,7 +28,8 @@ use ic_crypto_tree_hash::{ use ic_error_types::{ErrorCode, RejectCode, UserError}; use ic_http_endpoints_public::{query, read_state}; use ic_http_endpoints_test_agent::{ - self, APPLICATION_CBOR, Call, CanisterReadState, IngressMessage, Query, wait_for_status_healthy, + self, APPLICATION_CBOR, Call, CallSubnet, CanisterReadState, IngressMessage, Query, + wait_for_status_healthy, }; use ic_interfaces::execution_environment::QueryExecutionError; use ic_interfaces_mocks::consensus_pool::MockConsensusPoolCache; @@ -45,7 +46,7 @@ use ic_replicated_state::ReplicatedState; use ic_test_utilities_state::ReplicatedStateBuilder; use ic_test_utilities_types::ids::{NODE_1, canister_test_id, subnet_test_id, user_test_id}; use ic_types::{ - CryptoHashOfPartialState, Height, NumBytes, PrincipalId, RegistryVersion, + CanisterId, CryptoHashOfPartialState, Height, NumBytes, PrincipalId, RegistryVersion, artifact::UnvalidatedArtifactMutation, consensus::certification::{Certification, CertificationContent}, crypto::{ @@ -522,7 +523,7 @@ fn test_request_too_slow() { } #[rstest] -#[case(Call::V2, CBOR::Map(BTreeMap::from([ +#[case(UpdateEndpoint::Canister(Call::V2), CBOR::Map(BTreeMap::from([ ( CBOR::Text("error_code".to_string()), CBOR::Text("IC0204".to_string()), @@ -536,7 +537,7 @@ fn test_request_too_slow() { CBOR::Integer(RejectCode::SysTransient as i128), ), ])))] -#[case(Call::V3, CBOR::Map(BTreeMap::from([ +#[case(UpdateEndpoint::Canister(Call::V3), CBOR::Map(BTreeMap::from([ ( CBOR::Text("status".to_string()), CBOR::Text("non_replicated_rejection".to_string()), @@ -554,7 +555,25 @@ fn test_request_too_slow() { CBOR::Integer(RejectCode::SysTransient as i128), ), ])))] -#[case(Call::V4, CBOR::Map(BTreeMap::from([ +#[case(UpdateEndpoint::Canister(Call::V4), CBOR::Map(BTreeMap::from([ + ( + CBOR::Text("status".to_string()), + CBOR::Text("non_replicated_rejection".to_string()), + ), + ( + CBOR::Text("error_code".to_string()), + CBOR::Text("IC0204".to_string()), + ), + ( + CBOR::Text("reject_message".to_string()), + CBOR::Text("Test reject message".to_string()), + ), + ( + CBOR::Text("reject_code".to_string()), + CBOR::Integer(RejectCode::SysTransient as i128), + ), +])))] +#[case(UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), CBOR::Map(BTreeMap::from([ ( CBOR::Text("status".to_string()), CBOR::Text("non_replicated_rejection".to_string()), @@ -573,7 +592,7 @@ fn test_request_too_slow() { ), ])))] fn test_status_code_when_ingress_filter_fails( - #[case] endpoint: Call, + #[case] endpoint: UpdateEndpoint, #[case] expected_response: CBOR, ) { let rt = Runtime::new().unwrap(); @@ -596,7 +615,7 @@ fn test_status_code_when_ingress_filter_fails( rt.block_on(async move { wait_for_status_healthy(&addr).await.unwrap(); - let message = Default::default(); + let message = endpoint.default_ingress_message(); let call_response = endpoint.call(addr, message).await; assert_eq!( call_response.status(), @@ -1122,7 +1141,12 @@ fn test_http_1_requests_are_accepted() { /// return the certificate in the response with a 200 status code. #[rstest] fn test_call_handler_returns_early_for_ingress_message_already_in_certified_state( - #[values(Call::V3, Call::V4)] endpoint: Call, + #[values( + UpdateEndpoint::Canister(Call::V3), + UpdateEndpoint::Canister(Call::V4), + UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), + )] + endpoint: UpdateEndpoint, ) { use ic_crypto_tree_hash::MatchPatternPath; @@ -1250,7 +1274,7 @@ fn test_call_handler_returns_early_for_ingress_message_already_in_certified_stat rt.block_on(async { wait_for_status_healthy(&addr).await.unwrap(); - let message = IngressMessage::default(); + let message = endpoint.default_ingress_message(); let response = endpoint.call(addr, message).await; @@ -1297,7 +1321,14 @@ fn test_call_handler_returns_early_for_ingress_message_already_in_certified_stat /// Test that the sync call endpoints handle multiple requests with the same ingress message, /// by returning `202` for subsequent concurrent requests. #[rstest] -fn test_duplicate_concurrent_requests_return_early(#[values(Call::V3, Call::V4)] endpoint: Call) { +fn test_duplicate_concurrent_requests_return_early( + #[values( + UpdateEndpoint::Canister(Call::V3), + UpdateEndpoint::Canister(Call::V4), + UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), + )] + endpoint: UpdateEndpoint, +) { let rt = Runtime::new().unwrap(); let addr = get_free_localhost_socket_addr(); let config = Config { @@ -1310,7 +1341,7 @@ fn test_duplicate_concurrent_requests_return_early(#[values(Call::V3, Call::V4)] let mut handlers = HttpEndpointBuilder::new(rt.handle().clone(), config) .with_state_manager(state_manager) .run(); - let message = IngressMessage::default(); + let message = endpoint.default_ingress_message(); // Mock ingress filter to always accept the message. rt.spawn(async move { @@ -1409,7 +1440,12 @@ fn test_duplicate_concurrent_requests_return_early(#[values(Call::V3, Call::V4)] #[case(Height::from(1), None, Height::from(1))] #[case(Height::from(1), Some(Height::from(0)), Height::from(1))] fn test_sync_call_endpoint_responds_with_certificate( - #[values(Call::V3, Call::V4)] endpoint: Call, + #[values( + UpdateEndpoint::Canister(Call::V3), + UpdateEndpoint::Canister(Call::V4), + UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), + )] + endpoint: UpdateEndpoint, #[case] initial_certified_height: Height, #[case] transitioned_certified_height: Option, #[case] message_finalization_height: Height, @@ -1428,7 +1464,7 @@ fn test_sync_call_endpoint_responds_with_certificate( .with_state_manager(state_manager) .run(); - let message = IngressMessage::default(); + let message = endpoint.default_ingress_message(); // Mock ingress filter to always accept the message. rt.spawn(async move { @@ -1520,7 +1556,14 @@ fn test_sync_call_endpoint_responds_with_certificate( /// ingress messages that complete execution, but its height never /// gets certified. #[rstest] -fn test_synchronous_call_endpoint_no_certification(#[values(Call::V3, Call::V4)] endpoint: Call) { +fn test_synchronous_call_endpoint_no_certification( + #[values( + UpdateEndpoint::Canister(Call::V3), + UpdateEndpoint::Canister(Call::V4), + UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), + )] + endpoint: UpdateEndpoint, +) { let rt = Runtime::new().unwrap(); let addr = get_free_localhost_socket_addr(); let config = Config { @@ -1533,7 +1576,7 @@ fn test_synchronous_call_endpoint_no_certification(#[values(Call::V3, Call::V4)] .with_certified_height(Height::from(0)) .run(); - let message = IngressMessage::default(); + let message = endpoint.default_ingress_message(); // Mock ingress filter to always accept the message. rt.spawn(async move { @@ -1598,7 +1641,12 @@ impl CertifiedStateSnapshot for FakeCertifiedStateSnapshot { #[case::certified_state_snapshot_unavailable(None)] #[case::reading_certified_state_fails(Some(Box::new(FakeCertifiedStateSnapshot) as _))] fn test_call_v3_response_when_state_reader_fails( - #[values(Call::V3, Call::V4)] endpoint: Call, + #[values( + UpdateEndpoint::Canister(Call::V3), + UpdateEndpoint::Canister(Call::V4), + UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), + )] + endpoint: UpdateEndpoint, #[case] certified_state_snapshot: Option< Box>, >, @@ -1641,7 +1689,7 @@ fn test_call_v3_response_when_state_reader_fails( .with_state_manager(mock_state_manager) .run(); - let message = IngressMessage::default(); + let message = endpoint.default_ingress_message(); // Mock ingress filter to always accept the message. rt.spawn(async move { @@ -1688,7 +1736,13 @@ fn test_call_v3_response_when_state_reader_fails( /// P2P. #[rstest] fn test_call_response_when_p2p_not_running( - #[values(Call::V2, Call::V3, Call::V4)] call_agent: Call, + #[values( + UpdateEndpoint::Canister(Call::V2), + UpdateEndpoint::Canister(Call::V3), + UpdateEndpoint::Canister(Call::V4), + UpdateEndpoint::Subnet(CallSubnet::V4(subnet_test_id(1).get())), + )] + call_agent: UpdateEndpoint, ) { let rt = Runtime::new().unwrap(); let addr = get_free_localhost_socket_addr(); @@ -1715,7 +1769,9 @@ fn test_call_response_when_p2p_not_running( // Drop the P2P receiver to simulate P2P not running. drop(handlers.ingress_rx); - let response = call_agent.call(addr, IngressMessage::default()).await; + let response = call_agent + .call(addr, call_agent.default_ingress_message()) + .await; assert_eq!( StatusCode::INTERNAL_SERVER_ERROR, @@ -1730,3 +1786,141 @@ fn test_call_response_when_p2p_not_running( ); }); } + +/// Tests that /api/v4/subnet/../call rejects a request whose URL subnet ID does not match +/// the node's own subnet ID. +#[test] +fn test_call_v4_subnet_wrong_subnet_id() { + let rt = Runtime::new().unwrap(); + let addr = get_free_localhost_socket_addr(); + let config = Config { + listen_addr: addr, + ingress_message_certificate_timeout_seconds: 0, + ..Default::default() + }; + + // HttpEndpointBuilder configures the node with subnet_test_id(1). + let node_subnet_id = subnet_test_id(1).get(); + let wrong_subnet_id = subnet_test_id(2).get(); + + // Ingress filter is never reached; spawn a dummy handler so the loop doesn't hang. + let mut handlers = HttpEndpointBuilder::new(rt.handle().clone(), config).run(); + rt.spawn(async move { + loop { + let (_, resp) = handlers.ingress_filter.next_request().await.unwrap(); + resp.send_response(Ok(Ok(()))) + } + }); + + rt.block_on(async { + wait_for_status_healthy(&addr).await.unwrap(); + let response = CallSubnet::V4(wrong_subnet_id) + .call(addr, IngressMessage::default()) + .await; + + assert_eq!(StatusCode::BAD_REQUEST, response.status()); + assert_eq!( + format!( + "Specified SubnetId {wrong_subnet_id} does not match the subnet id of this node {node_subnet_id}" + ), + response.text().await.unwrap() + ); + }); +} + +/// Tests that /api/v4/subnet/../call accepts a request whose URL subnet ID matches +/// the node's own subnet ID. +#[test] +fn test_call_v4_subnet_correct_subnet_id() { + let rt = Runtime::new().unwrap(); + let addr = get_free_localhost_socket_addr(); + let config = Config { + listen_addr: addr, + ingress_message_certificate_timeout_seconds: 0, + ..Default::default() + }; + + let subnet_id = subnet_test_id(1).get(); + + let mut handlers = HttpEndpointBuilder::new(rt.handle().clone(), config).run(); + rt.spawn(async move { + loop { + let (_, resp) = handlers.ingress_filter.next_request().await.unwrap(); + resp.send_response(Ok(Ok(()))) + } + }); + + rt.block_on(async { + wait_for_status_healthy(&addr).await.unwrap(); + let response = CallSubnet::V4(subnet_id) + .call( + addr, + IngressMessage::default() + .with_canister_id( + ic_types::CanisterId::ic_00().get(), + ic_types::CanisterId::ic_00().get(), + ) + .with_method_name("create_canister".to_string()), + ) + .await; + + assert_eq!( + StatusCode::ACCEPTED, + response.status(), + "{:?}", + response.text().await + ); + }); +} + +/// Tests that /api/v4/subnet/../call rejects calls to non-management canisters (even with +/// method "create_canister") and calls to IC_00 methods other than "create_canister". +#[rstest] +#[case::wrong_canister_id(canister_test_id(1).get(), "create_canister")] +#[case::wrong_method_name(CanisterId::ic_00().get(), "install_code")] +fn test_call_v4_subnet_wrong_canister_or_method( + #[case] canister_id: PrincipalId, + #[case] method_name: &str, +) { + let rt = Runtime::new().unwrap(); + let addr = get_free_localhost_socket_addr(); + let config = Config { + listen_addr: addr, + ingress_message_certificate_timeout_seconds: 0, + ..Default::default() + }; + + let subnet_id = subnet_test_id(1).get(); + + let mut handlers = HttpEndpointBuilder::new(rt.handle().clone(), config).run(); + rt.spawn(async move { + loop { + let (_, resp) = handlers.ingress_filter.next_request().await.unwrap(); + resp.send_response(Ok(Ok(()))) + } + }); + + rt.block_on(async { + wait_for_status_healthy(&addr).await.unwrap(); + let response = CallSubnet::V4(subnet_id) + .call( + addr, + IngressMessage::default() + .with_canister_id(canister_id, canister_id) + .with_method_name(method_name.to_string()), + ) + .await; + + assert_eq!(StatusCode::BAD_REQUEST, response.status()); + let body = response.text().await.unwrap(); + assert_eq!( + body, + format!( + "Subnet call endpoint only accepts calls to the management canister ({}) 'create_canister' method, got canister_id={} method_name='{}'", + CanisterId::ic_00(), + CanisterId::unchecked_from_principal(canister_id), + method_name, + ) + ); + }); +} diff --git a/rs/http_endpoints/test_agent/BUILD.bazel b/rs/http_endpoints/test_agent/BUILD.bazel index a12308f962a4..9bd2cb58931e 100644 --- a/rs/http_endpoints/test_agent/BUILD.bazel +++ b/rs/http_endpoints/test_agent/BUILD.bazel @@ -1,6 +1,7 @@ load("@rules_rust//rust:defs.bzl", "rust_library") package(default_visibility = [ + "//rs/http_endpoints/fuzz:__subpackages__", "//rs/http_endpoints/public:__subpackages__", "//rs/tests/networking:__subpackages__", ]) diff --git a/rs/http_endpoints/test_agent/src/lib.rs b/rs/http_endpoints/test_agent/src/lib.rs index 7fb4294199be..6318ad461007 100644 --- a/rs/http_endpoints/test_agent/src/lib.rs +++ b/rs/http_endpoints/test_agent/src/lib.rs @@ -176,6 +176,30 @@ impl Call { } } +#[derive(Copy, Clone, Debug)] +pub enum CallSubnet { + V4(PrincipalId), +} + +impl CallSubnet { + pub async fn call( + self, + addr: SocketAddr, + ingress_message: IngressMessage, + ) -> reqwest::Response { + let CallSubnet::V4(subnet_id) = self; + let body = serde_cbor::to_vec(&ingress_message.envelope()).unwrap(); + let url = format!("http://{addr}/api/v4/subnet/{subnet_id}/call"); + reqwest::Client::new() + .post(url) + .body(body) + .header(CONTENT_TYPE, APPLICATION_CBOR) + .send() + .await + .unwrap() + } +} + pub struct Query { canister_id: PrincipalId, effective_canister_id: PrincipalId, diff --git a/rs/tests/networking/http_endpoints_public_spec_test.rs b/rs/tests/networking/http_endpoints_public_spec_test.rs index 4f1828c6f4ad..aa3b944ac741 100644 --- a/rs/tests/networking/http_endpoints_public_spec_test.rs +++ b/rs/tests/networking/http_endpoints_public_spec_test.rs @@ -61,7 +61,7 @@ use ic_system_test_driver::{ systest, util::{UniversalCanister, block_on}, }; -use ic_types::{CanisterId, PrincipalId}; +use ic_types::{CanisterId, PrincipalId, SubnetId}; use ic_universal_canister::wasm; use ic_utils::interfaces::ManagementCanister; use ic_utils::interfaces::management_canister::builders::CanisterInstallMode; @@ -760,6 +760,11 @@ fn get_api_bn_url(snapshot: &TopologySnapshot) -> Url { api_bn.get_public_url() } +fn get_subnet_ids(snapshot: &TopologySnapshot) -> (SubnetId, SubnetId) { + let (sys_subnet, app_subnet) = get_subnets(snapshot); + (sys_subnet.subnet_id, app_subnet.subnet_id) +} + fn get_canister_test_ids(snapshot: &TopologySnapshot) -> (CanisterId, [CanisterId; 5]) { let (primary, sys_uc, app_uc) = get_canister_ids(snapshot); ( @@ -779,6 +784,60 @@ fn get_canister_test_ids(snapshot: &TopologySnapshot) -> (CanisterId, [CanisterI ) } +fn update_calls_subnet_v4(env: TestEnv) { + let logger = env.logger(); + let snapshot = env.topology_snapshot(); + let socket = get_socket_addr(&snapshot); + let (sys_subnet_id, app_subnet_id) = get_subnet_ids(&snapshot); + + let mgmt_message = || { + IngressMessage::default() + .with_canister_id(CanisterId::ic_00().get(), CanisterId::ic_00().get()) + .with_method_name("create_canister".to_string()) + }; + + block_on(async { + // Correct subnet ID → accepted + let response = CallSubnet::V4(sys_subnet_id.get()) + .call(socket, mgmt_message()) + .await; + let status = inspect_response(response, "CallSubnet", &logger).await; + assert_2xx(&status); + + // Wrong subnet ID → rejected + let response = CallSubnet::V4(app_subnet_id.get()) + .call(socket, mgmt_message()) + .await; + let status = inspect_response(response, "CallSubnet", &logger).await; + assert_4xx(&status); + + // Non-management canister with "create_canister" → rejected + let (non_mgmt_canister, _) = get_canister_test_ids(&snapshot); + let response = CallSubnet::V4(sys_subnet_id.get()) + .call( + socket, + IngressMessage::default() + .with_canister_id(non_mgmt_canister.get(), non_mgmt_canister.get()) + .with_method_name("create_canister".to_string()), + ) + .await; + let status = inspect_response(response, "CallSubnet", &logger).await; + assert_4xx(&status); + + // IC_00 with wrong method → rejected + let response = CallSubnet::V4(sys_subnet_id.get()) + .call( + socket, + IngressMessage::default() + .with_canister_id(CanisterId::ic_00().get(), CanisterId::ic_00().get()) + .with_method_name("install_code".to_string()), + ) + .await; + let status = inspect_response(response, "CallSubnet", &logger).await; + assert_4xx(&status); + }); +} + fn assert_2xx(status: &u16) { assert!( (200..300).contains(status), @@ -803,6 +862,7 @@ fn main() -> Result<()> { .add_test(systest!(update_calls; Call::V2)) .add_test(systest!(update_calls; Call::V3)) .add_test(systest!(update_calls; Call::V4)) + .add_test(systest!(update_calls_subnet_v4)) .add_test(systest!(read_state_valid_succeeds; read_state::canister::Version::V2)) .add_test(systest!(read_state_valid_succeeds; read_state::canister::Version::V3)) .add_test(