Skip to content

Commit

Permalink
chore: [EXC-1792] Guard node_metrics_history against ingress (#2549)
Browse files Browse the repository at this point in the history 
This MR addresses these small issues: 

The node_metrics_history endpoint [is not
guarded](https://github.com/dfinity/ic/pull/2549/files#diff-3e6adef731ba785a0d6eef87d1174c48f9c242a148d279dadf5db47cdeef1cccR1375)
against ingress messages in the management canister code. Although this
is checked in the ingress filter, that code runs on a single node, so
malicious nodes could get the management canister to execute such
messages without payment.

Also, the tests for node_metrics_history are system tests, which take
more time and resources than necessary.

---------

Co-authored-by: Michael Weigelt <michael.weigelt@dfinity.com>
  • Loading branch information
@michael-weigelt
michael-weigelt and Michael Weigelt authored Nov 13, 2024
1 parent 132c245 commit 6034537
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 147 deletions.
102 changes: 99 additions & 3 deletions rs/execution_environment/src/canister_manager/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ use ic_management_canister_types::{
CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, CanisterIdRecord,
CanisterInstallMode, CanisterInstallModeV2, CanisterSettingsArgsBuilder,
CanisterStatusResultV2, CanisterStatusType, CanisterUpgradeOptions, ChunkHash,
ClearChunkStoreArgs, CreateCanisterArgs, EmptyBlob, InstallCodeArgsV2, Method, Payload,
StoredChunksArgs, StoredChunksReply, SubnetInfoArgs, SubnetInfoResponse, UpdateSettingsArgs,
UploadChunkArgs, UploadChunkReply, WasmMemoryPersistence,
ClearChunkStoreArgs, CreateCanisterArgs, EmptyBlob, InstallCodeArgsV2, Method,
NodeMetricsHistoryArgs, NodeMetricsHistoryResponse, Payload, StoredChunksArgs,
StoredChunksReply, SubnetInfoArgs, SubnetInfoResponse, UpdateSettingsArgs, UploadChunkArgs,
UploadChunkReply, WasmMemoryPersistence,
};
use ic_metrics::MetricsRegistry;
use ic_registry_provisional_whitelist::ProvisionalWhitelist;
Expand Down Expand Up @@ -7970,3 +7971,98 @@ fn subnet_info_ingress_fails() {
"cannot be called by a user",
);
}

#[test]
fn node_metrics_history_update_succeeds() {
let own_subnet_id = subnet_test_id(1);
let mut test = ExecutionTestBuilder::new()
.with_own_subnet_id(own_subnet_id)
.build();
let uni_canister = test
.universal_canister_with_cycles(Cycles::new(1_000_000_000_000))
.unwrap();
let update = wasm()
.call_simple(
CanisterId::ic_00(),
Method::NodeMetricsHistory,
call_args().other_side(
NodeMetricsHistoryArgs {
subnet_id: own_subnet_id.get(),
start_at_timestamp_nanos: 0,
}
.encode(),
),
)
.build();
let result = test.ingress(uni_canister, "update", update);
let bytes = get_reply(result);
let _ = Decode!(&bytes, Vec<NodeMetricsHistoryResponse>).unwrap();
}

#[test]
fn node_metrics_history_query_fails() {
let own_subnet_id = subnet_test_id(1);
let mut test = ExecutionTestBuilder::new()
.with_own_subnet_id(own_subnet_id)
.build();
let uni_canister = test
.universal_canister_with_cycles(Cycles::new(1_000_000_000_000))
.unwrap();
let query = wasm()
.call_simple(
CanisterId::ic_00(),
Method::NodeMetricsHistory,
call_args().other_side(
NodeMetricsHistoryArgs {
subnet_id: own_subnet_id.get(),
start_at_timestamp_nanos: 0,
}
.encode(),
),
)
.build();
test.ingress(uni_canister, "query", query)
.unwrap_err()
.assert_contains(
ErrorCode::CanisterContractViolation,
"cannot be executed in replicated query mode",
)
}

#[test]
fn node_metrics_history_ingress_update_fails() {
let own_subnet_id = subnet_test_id(1);
let mut test = ExecutionTestBuilder::new()
.with_own_subnet_id(own_subnet_id)
.build();
let payload = NodeMetricsHistoryArgs {
subnet_id: own_subnet_id.get(),
start_at_timestamp_nanos: 0,
}
.encode();
test.subnet_message(Method::NodeMetricsHistory, payload)
.unwrap_err()
.assert_contains(
ErrorCode::CanisterContractViolation,
"cannot be called by a user",
);
}

#[test]
fn node_metrics_history_ingress_query_fails() {
let own_subnet_id = subnet_test_id(1);
let mut test = ExecutionTestBuilder::new()
.with_own_subnet_id(own_subnet_id)
.build();
let payload = NodeMetricsHistoryArgs {
subnet_id: own_subnet_id.get(),
start_at_timestamp_nanos: 0,
}
.encode();
test.non_replicated_query(CanisterId::ic_00(), "node_metrics_history", payload)
.unwrap_err()
.assert_contains(
ErrorCode::CanisterMethodNotFound,
"Query method node_metrics_history not found.",
);
}
19 changes: 12 additions & 7 deletions rs/execution_environment/src/execution_environment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1371,14 +1371,19 @@ impl ExecutionEnvironment {
}
}

Ok(Ic00Method::NodeMetricsHistory) => {
let res = NodeMetricsHistoryArgs::decode(payload)
.and_then(|args| self.node_metrics_history(&state, args));
ExecuteSubnetMessageResult::Finished {
response: res,
refund: msg.take_cycles(),
Ok(Ic00Method::NodeMetricsHistory) => match &msg {
CanisterCall::Ingress(_) => {
self.reject_unexpected_ingress(Ic00Method::NodeMetricsHistory)
}
}
CanisterCall::Request(_) => {
let res = NodeMetricsHistoryArgs::decode(payload)
.and_then(|args| self.node_metrics_history(&state, args));
ExecuteSubnetMessageResult::Finished {
response: res,
refund: msg.take_cycles(),
}
}
},

Ok(Ic00Method::SubnetInfo) => match &msg {
CanisterCall::Ingress(_) => self.reject_unexpected_ingress(Ic00Method::SubnetInfo),
Expand Down
8 changes: 0 additions & 8 deletions rs/tests/execution/general_execution_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ mod general_execution_tests;

use anyhow::Result;
use general_execution_tests::api_tests::node_metrics_history_another_subnet_succeeds;
use general_execution_tests::api_tests::node_metrics_history_ingress_query_fails;
use general_execution_tests::api_tests::node_metrics_history_ingress_update_fails;
use general_execution_tests::api_tests::node_metrics_history_non_existing_subnet_fails;
use general_execution_tests::api_tests::node_metrics_history_query_fails;
use general_execution_tests::api_tests::node_metrics_history_update_succeeds;
use general_execution_tests::api_tests::test_controller;
use general_execution_tests::api_tests::test_cycles_burn;
use general_execution_tests::api_tests::test_in_replicated_execution;
Expand Down Expand Up @@ -39,12 +35,8 @@ fn main() -> Result<()> {
.add_test(systest!(test_controller))
.add_test(systest!(test_in_replicated_execution))
.add_test(systest!(test_cycles_burn))
.add_test(systest!(node_metrics_history_update_succeeds))
.add_test(systest!(node_metrics_history_query_fails))
.add_test(systest!(node_metrics_history_another_subnet_succeeds))
.add_test(systest!(node_metrics_history_non_existing_subnet_fails))
.add_test(systest!(node_metrics_history_ingress_update_fails))
.add_test(systest!(node_metrics_history_ingress_query_fails))
.add_test(systest!(can_access_big_heap_and_big_stable_memory))
.add_test(systest!(can_access_big_stable_memory))
.add_test(systest!(can_handle_overflows_when_indexing_stable_memory))
Expand Down
130 changes: 1 addition & 129 deletions rs/tests/execution/general_execution_tests/api_tests.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/* tag::catalog[]
end::catalog[] */

use candid::Principal;
use ic_agent::{agent::RejectCode, Agent, AgentError};
use ic_agent::{agent::RejectCode, Agent};
use ic_base_types::PrincipalId;
use ic_management_canister_types::{self as ic00, EmptyBlob, Method, Payload};
use ic_system_test_driver::driver::test_env::TestEnv;
Expand Down Expand Up @@ -208,81 +207,6 @@ pub fn test_cycles_burn(env: TestEnv) {
})
}

pub fn node_metrics_history_update_succeeds(env: TestEnv) {
// Arrange.
let (app_node, agent) = setup_app_node_and_agent(&env);
let logger = env.logger();
let subnet_id = app_node.subnet_id().unwrap().get();
block_on({
async move {
let canister = UniversalCanister::new_with_retries(
&agent,
app_node.effective_canister_id(),
&logger,
)
.await;
// Act.
let result = canister
.update(
wasm().call_simple(
ic00::IC_00,
Method::NodeMetricsHistory,
call_args().other_side(
ic00::NodeMetricsHistoryArgs {
subnet_id,
start_at_timestamp_nanos: 0,
}
.encode(),
),
),
)
.await;
// Assert.
assert!(result.is_ok());
assert!(!result.ok().unwrap().is_empty()); // Assert it has some non zero data.
}
})
}

pub fn node_metrics_history_query_fails(env: TestEnv) {
// Arrange.
let (app_node, agent) = setup_app_node_and_agent(&env);
let logger = env.logger();
let subnet_id = app_node.subnet_id().unwrap().get();
block_on({
async move {
let canister = UniversalCanister::new_with_retries(
&agent,
app_node.effective_canister_id(),
&logger,
)
.await;
// Act.
let result = canister
.query(
wasm().call_simple(
ic00::IC_00,
Method::NodeMetricsHistory,
call_args().other_side(
ic00::NodeMetricsHistoryArgs {
subnet_id,
start_at_timestamp_nanos: 0,
}
.encode(),
),
),
)
.await;
// Assert.
assert_reject_msg(
result,
RejectCode::CanisterError,
"cannot be executed in non replicated query mode",
);
}
})
}

pub fn node_metrics_history_another_subnet_succeeds(env: TestEnv) {
// Arrange.
let (app_node_1, agent_1) = setup_app_node_and_agent(&env);
Expand Down Expand Up @@ -356,55 +280,3 @@ pub fn node_metrics_history_non_existing_subnet_fails(env: TestEnv) {
}
})
}

pub fn node_metrics_history_ingress_update_fails(env: TestEnv) {
// Arrange.
let (app_node, agent) = setup_app_node_and_agent(&env);
let subnet_id = app_node.subnet_id().unwrap().get();
block_on({
async move {
// Act.
let result = agent
.update(&Principal::management_canister(), "node_metrics_history")
.with_arg(
ic00::NodeMetricsHistoryArgs {
subnet_id,
start_at_timestamp_nanos: 0,
}
.encode(),
)
.call_and_wait()
.await;
// Assert.
assert_reject_msg(
result,
RejectCode::CanisterReject,
"ic00 method node_metrics_history can not be called via ingress messages",
);
}
})
}

pub fn node_metrics_history_ingress_query_fails(env: TestEnv) {
// Arrange.
let (app_node, agent) = setup_app_node_and_agent(&env);
let subnet_id = app_node.subnet_id().unwrap().get();
block_on({
async move {
// Act.
let result = agent
.query(&Principal::management_canister(), "node_metrics_history")
.with_arg(
ic00::NodeMetricsHistoryArgs {
subnet_id,
start_at_timestamp_nanos: 0,
}
.encode(),
)
.call()
.await;
// Assert.
assert_eq!(result, Err(AgentError::CertificateNotAuthorized()));
}
})
}

0 comments on commit 6034537

Please sign in to comment.