From ae44be6f9b133bc1a038bc32ab5a8fbe8e5062a3 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 12:51:44 +0000 Subject: [PATCH 01/12] feat: new canister setting status_visibility --- .../src/canister_manager.rs | 13 +- .../src/canister_settings.rs | 30 +++++ .../src/execution/common.rs | 29 +++++ rs/pocket_ic_server/src/pocket_ic.rs | 19 ++- .../v1/canister_state_bits.proto | 14 +++ .../gen/state/state.canister_state_bits.v1.rs | 25 ++++ rs/replica_tests/tests/canister_lifecycle.rs | 5 +- rs/replicated_state/src/canister_state.rs | 6 +- .../src/canister_state/system_state.rs | 9 +- rs/state_layout/src/state_layout.rs | 2 + rs/state_layout/src/state_layout/proto.rs | 9 ++ rs/state_layout/src/state_layout/tests.rs | 1 + rs/state_manager/src/checkpoint.rs | 1 + rs/state_manager/src/tip.rs | 1 + rs/types/management_canister_types/src/lib.rs | 111 ++++++++++++++++++ .../management_canister_types/tests/ic.did | 8 ++ 16 files changed, 276 insertions(+), 7 deletions(-) diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 706359abd06f..77970a75f8f1 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2,7 +2,7 @@ use crate::as_round_instructions; use crate::canister_settings::CanisterSettings; use crate::execution::common::{ validate_controller, validate_controller_or_subnet_admin, validate_snapshot_visibility, - validate_subnet_admin, + validate_status_visibility, validate_subnet_admin, }; use crate::execution::install_code::OriginalContext; use crate::execution::{install::execute_install, upgrade::execute_upgrade}; @@ -357,6 +357,11 @@ impl CanisterManager { canister.system_state.snapshot_visibility = snapshot_visibility.clone(); } + // Status visibility: apply. + if let Some(status_visibility) = settings.status_visibility() { + canister.system_state.status_visibility = status_visibility.clone(); + } + // Wasm memory threshold: apply. if let Some(wasm_memory_threshold) = settings.wasm_memory_threshold() { canister.system_state.wasm_memory_threshold = wasm_memory_threshold; @@ -1069,10 +1074,10 @@ impl CanisterManager { ready_for_migration: bool, subnet_admins: Option>, ) -> Result { - // Skip the controller check if the canister itself is requesting its + // Skip the visibility check if the canister itself is requesting its // own status, as the canister is considered in the same trust domain. if sender != canister.canister_id().get() { - validate_controller_or_subnet_admin(canister, subnet_admins, &sender)? + validate_status_visibility(canister, subnet_admins, &sender)? } let controller = canister.system_state.controller(); @@ -1103,6 +1108,7 @@ impl CanisterManager { canister.system_state.minimum_incoming_canister_call_cycles; let log_visibility = canister.system_state.log_visibility.clone(); let snapshot_visibility = canister.system_state.snapshot_visibility.clone(); + let status_visibility = canister.system_state.status_visibility.clone(); let log_memory_limit = canister.log_memory_limit().get(); let wasm_memory_limit = canister.system_state.wasm_memory_limit; let wasm_memory_threshold = canister.system_state.wasm_memory_threshold; @@ -1135,6 +1141,7 @@ impl CanisterManager { minimum_incoming_canister_call_cycles.get(), log_visibility, snapshot_visibility, + status_visibility, log_memory_limit, self.cycles_account_manager .idle_cycles_burned_rate( diff --git a/rs/execution_environment/src/canister_settings.rs b/rs/execution_environment/src/canister_settings.rs index 739c14a08ac6..d879e9c7d0c5 100644 --- a/rs/execution_environment/src/canister_settings.rs +++ b/rs/execution_environment/src/canister_settings.rs @@ -2,6 +2,7 @@ use ic_base_types::{EnvironmentVariables, NumBytes, NumSeconds}; use ic_error_types::{ErrorCode, UserError}; use ic_management_canister_types_private::{ BoundedAllowedViewers, CanisterSettingsArgs, LogVisibilityV2, SnapshotVisibility, + StatusVisibility, }; use ic_types::{ComputeAllocation, InvalidComputeAllocationError, MemoryAllocation, PrincipalId}; use ic_types_cycles::Cycles; @@ -29,12 +30,14 @@ pub(crate) struct CanisterSettings { pub(crate) minimum_incoming_canister_call_cycles: Option, pub(crate) log_visibility: Option, pub(crate) snapshot_visibility: Option, + pub(crate) status_visibility: Option, pub(crate) log_memory_limit: Option, pub(crate) wasm_memory_limit: Option, pub(crate) environment_variables: Option, } impl CanisterSettings { + #[allow(clippy::too_many_arguments)] pub fn new( controllers: Option>, compute_allocation: Option, @@ -45,6 +48,7 @@ impl CanisterSettings { minimum_incoming_canister_call_cycles: Option, log_visibility: Option, snapshot_visibility: Option, + status_visibility: Option, log_memory_limit: Option, wasm_memory_limit: Option, environment_variables: Option, @@ -59,6 +63,7 @@ impl CanisterSettings { minimum_incoming_canister_call_cycles, log_visibility, snapshot_visibility, + status_visibility, log_memory_limit, wasm_memory_limit, environment_variables, @@ -101,6 +106,10 @@ impl CanisterSettings { self.snapshot_visibility.as_ref() } + pub fn status_visibility(&self) -> Option<&StatusVisibility> { + self.status_visibility.as_ref() + } + pub fn log_memory_limit(&self) -> Option { self.log_memory_limit } @@ -223,6 +232,7 @@ impl TryFrom for CanisterSettings { minimum_incoming_canister_call_cycles, input.log_visibility, input.snapshot_visibility, + input.status_visibility, log_memory_limit, wasm_memory_limit, environment_variables, @@ -251,6 +261,7 @@ pub(crate) struct CanisterSettingsBuilder { minimum_incoming_canister_call_cycles: Option, log_visibility: Option, snapshot_visibility: Option, + status_visibility: Option, log_memory_limit: Option, wasm_memory_limit: Option, environment_variables: Option, @@ -269,6 +280,7 @@ impl CanisterSettingsBuilder { minimum_incoming_canister_call_cycles: None, log_visibility: None, snapshot_visibility: None, + status_visibility: None, log_memory_limit: None, wasm_memory_limit: None, environment_variables: None, @@ -286,6 +298,7 @@ impl CanisterSettingsBuilder { minimum_incoming_canister_call_cycles: self.minimum_incoming_canister_call_cycles, log_visibility: self.log_visibility, snapshot_visibility: self.snapshot_visibility, + status_visibility: self.status_visibility, log_memory_limit: self.log_memory_limit, wasm_memory_limit: self.wasm_memory_limit, environment_variables: self.environment_variables, @@ -358,6 +371,13 @@ impl CanisterSettingsBuilder { } } + pub fn with_status_visibility(self, status_visibility: StatusVisibility) -> Self { + Self { + status_visibility: Some(status_visibility), + ..self + } + } + pub fn with_log_memory_limit(self, log_memory_limit: NumBytes) -> Self { Self { log_memory_limit: Some(log_memory_limit), @@ -487,6 +507,16 @@ impl<'a> From<&'a SnapshotVisibility> for VisibilitySettings<'a> { } } +impl<'a> From<&'a StatusVisibility> for VisibilitySettings<'a> { + fn from(v: &'a StatusVisibility) -> Self { + match v { + StatusVisibility::Public => Self::Public, + StatusVisibility::Controllers => Self::Controllers, + StatusVisibility::AllowedViewers(principals) => Self::AllowedViewers(principals), + } + } +} + impl VisibilitySettings<'_> { pub(crate) fn has_access( &self, diff --git a/rs/execution_environment/src/execution/common.rs b/rs/execution_environment/src/execution/common.rs index 79b445711ec9..c062ae537ff9 100644 --- a/rs/execution_environment/src/execution/common.rs +++ b/rs/execution_environment/src/execution/common.rs @@ -372,6 +372,35 @@ pub(crate) fn validate_snapshot_visibility( Ok(()) } +/// Validates that the `caller` is allowed to read the status of the `canister` +/// according to its canister status visibility settings. +/// +/// Subnet admins always retain access (preserving the historical behavior of +/// `validate_controller_or_subnet_admin`); otherwise access is governed by the +/// status visibility setting, which grants access to the controllers plus any +/// additional allowed viewers (or everyone, if the status is public). +pub(crate) fn validate_status_visibility( + canister: &CanisterState, + subnet_admins: Option>, + caller: &PrincipalId, +) -> Result<(), CanisterManagerError> { + // Subnet admins always retain access to the canister status. + if let Some(subnet_admins) = &subnet_admins + && subnet_admins.contains(caller) + { + return Ok(()); + } + // Otherwise, access is governed by the status visibility setting. + if crate::canister_settings::VisibilitySettings::from(canister.status_visibility()) + .has_access(caller, canister.controllers()) + { + return Ok(()); + } + // Fall back to the legacy controller-or-subnet-admin error to preserve + // backward-compatible error reporting. + validate_controller_or_subnet_admin(canister, subnet_admins, caller) +} + pub(crate) fn validate_subnet_admin( subnet_admins: &BTreeSet, sender: &PrincipalId, diff --git a/rs/pocket_ic_server/src/pocket_ic.rs b/rs/pocket_ic_server/src/pocket_ic.rs index 2d76130d4af3..6ec46e7bf289 100644 --- a/rs/pocket_ic_server/src/pocket_ic.rs +++ b/rs/pocket_ic_server/src/pocket_ic.rs @@ -72,7 +72,8 @@ use ic_management_canister_types_private::{ MasterPublicKeyId, Method as Ic00Method, ProvisionalCreateCanisterWithCyclesArgs, ReadCanisterSnapshotDataArgs, ReadCanisterSnapshotMetadataArgs, ReadCanisterSnapshotMetadataResponse, SchnorrAlgorithm, SchnorrKeyId, SnapshotVisibility, - UploadCanisterSnapshotDataArgs, UploadCanisterSnapshotMetadataArgs, VetKdCurve, VetKdKeyId, + StatusVisibility, UploadCanisterSnapshotDataArgs, UploadCanisterSnapshotMetadataArgs, + VetKdCurve, VetKdKeyId, }; use ic_metrics::MetricsRegistry; use ic_nervous_system_common::ONE_YEAR_SECONDS; @@ -1177,6 +1178,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1273,6 +1275,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1445,6 +1448,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1528,6 +1532,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1609,6 +1614,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = ii_subnet.state_machine.create_canister_with_cycles( @@ -1675,6 +1681,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = ii_subnet.state_machine.create_canister_with_cycles( @@ -1747,6 +1754,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1826,6 +1834,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1895,6 +1904,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -1995,6 +2005,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = sns_subnet.state_machine.create_canister_with_cycles( @@ -2070,6 +2081,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = ii_subnet.state_machine.create_canister_with_cycles( @@ -2307,6 +2319,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }); @@ -2386,6 +2399,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( @@ -2485,6 +2499,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = btc_subnet.state_machine.create_canister_with_cycles( @@ -2561,6 +2576,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = btc_subnet.state_machine.create_canister_with_cycles( @@ -2629,6 +2645,7 @@ impl PocketIcSubnets { wasm_memory_threshold: Some(0_u64.into()), environment_variables: None, snapshot_visibility: Some(SnapshotVisibility::Controllers), + status_visibility: Some(StatusVisibility::Controllers), minimum_incoming_canister_call_cycles: None, }; let canister_id = nns_subnet.state_machine.create_canister_with_cycles( diff --git a/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto b/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto index 21a3d6119407..a616228166e9 100644 --- a/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto +++ b/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto @@ -397,6 +397,18 @@ message SnapshotVisibility { } } +message StatusVisibilityAllowedViewers { + repeated types.v1.PrincipalId principals = 1; +} + +message StatusVisibility { + oneof status_visibility { + int32 controllers = 1; + int32 public = 2; + StatusVisibilityAllowedViewers allowed_viewers = 3; + } +} + message CanisterLogRecord { uint64 idx = 1; uint64 timestamp_nanos = 2; @@ -522,4 +534,6 @@ message CanisterStateBits { map environment_variables = 55; // Snapshot visibility for the canister. SnapshotVisibility snapshot_visibility = 64; + // Status visibility for the canister. + StatusVisibility status_visibility = 69; } diff --git a/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs b/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs index c53d8f524e86..c775ca1468c0 100644 --- a/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs +++ b/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs @@ -619,6 +619,28 @@ pub mod snapshot_visibility { } } #[derive(Clone, PartialEq, ::prost::Message)] +pub struct StatusVisibilityAllowedViewers { + #[prost(message, repeated, tag = "1")] + pub principals: ::prost::alloc::vec::Vec, +} +#[derive(Clone, PartialEq, ::prost::Message)] +pub struct StatusVisibility { + #[prost(oneof = "status_visibility::StatusVisibility", tags = "1, 2, 3")] + pub status_visibility: ::core::option::Option, +} +/// Nested message and enum types in `StatusVisibility`. +pub mod status_visibility { + #[derive(Clone, PartialEq, ::prost::Oneof)] + pub enum StatusVisibility { + #[prost(int32, tag = "1")] + Controllers(i32), + #[prost(int32, tag = "2")] + Public(i32), + #[prost(message, tag = "3")] + AllowedViewers(super::StatusVisibilityAllowedViewers), + } +} +#[derive(Clone, PartialEq, ::prost::Message)] pub struct CanisterLogRecord { #[prost(uint64, tag = "1")] pub idx: u64, @@ -780,6 +802,9 @@ pub struct CanisterStateBits { /// Snapshot visibility for the canister. #[prost(message, optional, tag = "64")] pub snapshot_visibility: ::core::option::Option, + /// Status visibility for the canister. + #[prost(message, optional, tag = "69")] + pub status_visibility: ::core::option::Option, #[prost(oneof = "canister_state_bits::CanisterStatus", tags = "11, 12, 13")] pub canister_status: ::core::option::Option, } diff --git a/rs/replica_tests/tests/canister_lifecycle.rs b/rs/replica_tests/tests/canister_lifecycle.rs index a986c84ea9ce..f862e945d970 100644 --- a/rs/replica_tests/tests/canister_lifecycle.rs +++ b/rs/replica_tests/tests/canister_lifecycle.rs @@ -9,7 +9,8 @@ use ic_error_types::{ErrorCode, RejectCode}; use ic_management_canister_types_private::{ self as ic00, CanisterChange, CanisterIdRecord, CanisterInstallMode, CanisterSettingsArgsBuilder, CanisterStatusResultV2, CanisterStatusType, EmptyBlob, IC_00, - InstallCodeArgs, LogVisibilityV2, Method, Payload, SnapshotVisibility, UpdateSettingsArgs, + InstallCodeArgs, LogVisibilityV2, Method, Payload, SnapshotVisibility, StatusVisibility, + UpdateSettingsArgs, }; use ic_registry_provisional_whitelist::ProvisionalWhitelist; use ic_replica_tests as utils; @@ -729,6 +730,7 @@ fn can_get_canister_information() { 0_u128, LogVisibilityV2::default(), SnapshotVisibility::default(), + StatusVisibility::default(), TEST_DEFAULT_LOG_MEMORY_LIMIT, 0_u128, 0_u128, @@ -801,6 +803,7 @@ fn can_get_canister_information() { 0_u128, LogVisibilityV2::default(), SnapshotVisibility::default(), + StatusVisibility::default(), TEST_DEFAULT_LOG_MEMORY_LIMIT, 0_u128, 0_u128, diff --git a/rs/replicated_state/src/canister_state.rs b/rs/replicated_state/src/canister_state.rs index dfeb80c547f5..d28794b86003 100644 --- a/rs/replicated_state/src/canister_state.rs +++ b/rs/replicated_state/src/canister_state.rs @@ -19,7 +19,7 @@ use ic_interfaces::execution_environment::{ }; use ic_management_canister_types_private::{ CanisterChangeDetails, CanisterChangeOrigin, CanisterStatusType, LogVisibilityV2, - SnapshotVisibility, + SnapshotVisibility, StatusVisibility, }; use ic_registry_subnet_type::SubnetType; use ic_types::messages::{CallbackId, CanisterMessage, Ingress, RequestOrResponse, Response}; @@ -113,6 +113,10 @@ impl CanisterState { &self.system_state.snapshot_visibility } + pub fn status_visibility(&self) -> &StatusVisibility { + &self.system_state.status_visibility + } + /// Returns the difference in time since the canister was last charged for resource allocations. pub fn duration_since_last_allocation_charge(&self, current_time: Time) -> Duration { debug_assert!( diff --git a/rs/replicated_state/src/canister_state/system_state.rs b/rs/replicated_state/src/canister_state/system_state.rs index f3d556343a58..2592b7bb8c14 100644 --- a/rs/replicated_state/src/canister_state/system_state.rs +++ b/rs/replicated_state/src/canister_state/system_state.rs @@ -29,7 +29,7 @@ use ic_interfaces::execution_environment::{HypervisorError, MessageMemoryUsage}; use ic_logger::{ReplicaLogger, error}; use ic_management_canister_types_private::{ CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, CanisterStatusType, - LogVisibilityV2, SnapshotVisibility, + LogVisibilityV2, SnapshotVisibility, StatusVisibility, }; use ic_registry_subnet_type::SubnetType; use ic_types::batch::TotalQueryStats; @@ -587,6 +587,9 @@ pub struct SystemState { /// Snapshot visibility of the canister. pub snapshot_visibility: SnapshotVisibility, + /// Status visibility of the canister. + pub status_visibility: StatusVisibility, + /// Log records of the canister. #[validate_eq(CompareWithValidateEq)] pub canister_log: CanisterLog, @@ -777,6 +780,7 @@ impl SystemState { wasm_chunk_store, log_visibility: Default::default(), snapshot_visibility: Default::default(), + status_visibility: Default::default(), // TODO(EXC-2118): CanisterLog does not store log records efficiently, // therefore it should not scale to memory limit from above. // Remove this field after migration is done. @@ -814,6 +818,7 @@ impl SystemState { wasm_chunk_store_metadata: WasmChunkStoreMetadata, log_visibility: LogVisibilityV2, snapshot_visibility: SnapshotVisibility, + status_visibility: StatusVisibility, canister_log: CanisterLog, log_memory_store_data: Option, log_memory_store_persistent_next_idx: u64, @@ -851,6 +856,7 @@ impl SystemState { ), log_visibility, snapshot_visibility, + status_visibility, canister_log, log_memory_store: LogMemoryStore::from_checkpoint( log_memory_store_data, @@ -2688,6 +2694,7 @@ pub mod testing { wasm_chunk_store: WasmChunkStore::new_for_testing(), log_visibility: Default::default(), snapshot_visibility: Default::default(), + status_visibility: Default::default(), // TODO(EXC-2118): CanisterLog does not store log records efficiently, // therefore it should not scale to memory limit from above. // Remove this field after migration is done. diff --git a/rs/state_layout/src/state_layout.rs b/rs/state_layout/src/state_layout.rs index 452e76b65b47..30226b7615b4 100644 --- a/rs/state_layout/src/state_layout.rs +++ b/rs/state_layout/src/state_layout.rs @@ -2,6 +2,7 @@ use ic_base_types::{NumBytes, NumSeconds}; use ic_logger::{ReplicaLogger, error, info, warn}; use ic_management_canister_types_private::{ Global, LogVisibilityV2, OnLowWasmMemoryHookStatus, SnapshotSource, SnapshotVisibility, + StatusVisibility, }; use ic_metrics::{MetricsRegistry, buckets::decimal_buckets}; use ic_protobuf::state::{ @@ -203,6 +204,7 @@ pub struct CanisterStateBits { pub total_query_stats: TotalQueryStats, pub log_visibility: LogVisibilityV2, pub snapshot_visibility: SnapshotVisibility, + pub status_visibility: StatusVisibility, pub log_memory_limit: NumBytes, pub canister_log: CanisterLog, pub next_canister_log_record_idx: u64, diff --git a/rs/state_layout/src/state_layout/proto.rs b/rs/state_layout/src/state_layout/proto.rs index ff971bf347c1..83f368e89f09 100644 --- a/rs/state_layout/src/state_layout/proto.rs +++ b/rs/state_layout/src/state_layout/proto.rs @@ -69,6 +69,10 @@ impl From for pb_canister_state_bits::CanisterStateBits { &item.snapshot_visibility, ) .into(), + status_visibility: pb_canister_state_bits::StatusVisibility::from( + &item.status_visibility, + ) + .into(), log_memory_limit: item.log_memory_limit.get(), canister_log_records: item .canister_log @@ -225,6 +229,11 @@ impl TryFrom for CanisterStateBits { "CanisterStateBits::snapshot_visibility", ) .unwrap_or_default(), + status_visibility: try_from_option_field( + value.status_visibility, + "CanisterStateBits::status_visibility", + ) + .unwrap_or_default(), log_memory_limit: NumBytes::from(value.log_memory_limit), canister_log: CanisterLog::new_aggregate( value.next_canister_log_record_idx, diff --git a/rs/state_layout/src/state_layout/tests.rs b/rs/state_layout/src/state_layout/tests.rs index 825aad898146..fa7f8a48541d 100644 --- a/rs/state_layout/src/state_layout/tests.rs +++ b/rs/state_layout/src/state_layout/tests.rs @@ -60,6 +60,7 @@ fn default_canister_state_bits() -> CanisterStateBits { total_query_stats: TotalQueryStats::default(), log_visibility: Default::default(), snapshot_visibility: Default::default(), + status_visibility: Default::default(), log_memory_limit: NumBytes::from(0), canister_log: CanisterLog::default_aggregate(), next_canister_log_record_idx: 0, diff --git a/rs/state_manager/src/checkpoint.rs b/rs/state_manager/src/checkpoint.rs index 296693a14410..98796293a957 100644 --- a/rs/state_manager/src/checkpoint.rs +++ b/rs/state_manager/src/checkpoint.rs @@ -861,6 +861,7 @@ pub fn load_canister_state( canister_state_bits.wasm_chunk_store_metadata, canister_state_bits.log_visibility, canister_state_bits.snapshot_visibility, + canister_state_bits.status_visibility, canister_state_bits.canister_log, log_memory_store_data, canister_state_bits.log_memory_store_persistent_next_idx, diff --git a/rs/state_manager/src/tip.rs b/rs/state_manager/src/tip.rs index 5641a654f933..4b57ec85bea4 100644 --- a/rs/state_manager/src/tip.rs +++ b/rs/state_manager/src/tip.rs @@ -1282,6 +1282,7 @@ fn serialize_canister_protos_to_checkpoint_readwrite( total_query_stats: canister_state.system_state.total_query_stats.clone(), log_visibility: canister_state.system_state.log_visibility.clone(), snapshot_visibility: canister_state.system_state.snapshot_visibility.clone(), + status_visibility: canister_state.system_state.status_visibility.clone(), log_memory_limit: canister_state.log_memory_limit(), canister_log: canister_state.system_state.canister_log.clone(), next_canister_log_record_idx: canister_state.system_state.canister_log.next_idx(), diff --git a/rs/types/management_canister_types/src/lib.rs b/rs/types/management_canister_types/src/lib.rs index bea5cb354e86..983dca0ddb4e 100644 --- a/rs/types/management_canister_types/src/lib.rs +++ b/rs/types/management_canister_types/src/lib.rs @@ -1372,6 +1372,94 @@ impl TryFrom for SnapshotVisibility } } +/// Status visibility for a canister. +/// ```text +/// variant { +/// controllers; +/// public; +/// allowed_viewers : vec principal; +/// } +/// ``` +#[derive(Clone, Eq, PartialEq, Debug, Default, CandidType, Deserialize, EnumIter)] +pub enum StatusVisibility { + #[default] + #[serde(rename = "controllers")] + Controllers, + #[serde(rename = "public")] + Public, + #[serde(rename = "allowed_viewers")] + AllowedViewers(BoundedAllowedViewers), +} + +impl Payload<'_> for StatusVisibility {} + +impl From<&StatusVisibility> for pb_canister_state_bits::StatusVisibility { + fn from(item: &StatusVisibility) -> Self { + match item { + StatusVisibility::Controllers => pb_canister_state_bits::StatusVisibility { + status_visibility: Some( + pb_canister_state_bits::status_visibility::StatusVisibility::Controllers(1), + ), + }, + StatusVisibility::Public => pb_canister_state_bits::StatusVisibility { + status_visibility: Some( + pb_canister_state_bits::status_visibility::StatusVisibility::Public(2), + ), + }, + StatusVisibility::AllowedViewers(principals) => { + pb_canister_state_bits::StatusVisibility { + status_visibility: Some( + pb_canister_state_bits::status_visibility::StatusVisibility::AllowedViewers( + pb_canister_state_bits::StatusVisibilityAllowedViewers { + principals: principals + .get() + .iter() + .map(|c| (*c).into()) + .collect::>(), + }, + ), + ), + } + } + } + } +} + +impl TryFrom for StatusVisibility { + type Error = ProxyDecodeError; + + fn try_from(item: pb_canister_state_bits::StatusVisibility) -> Result { + let Some(status_visibility) = item.status_visibility else { + return Err(ProxyDecodeError::MissingField( + "StatusVisibility::status_visibility", + )); + }; + match status_visibility { + pb_canister_state_bits::status_visibility::StatusVisibility::Controllers(_) => { + Ok(Self::Controllers) + } + pb_canister_state_bits::status_visibility::StatusVisibility::Public(_) => { + Ok(Self::Public) + } + pb_canister_state_bits::status_visibility::StatusVisibility::AllowedViewers(data) => { + let principals = data + .principals + .iter() + .map(|p| { + PrincipalId::try_from(p.raw.clone()).map_err(|e| { + ProxyDecodeError::ValueOutOfRange { + typ: "PrincipalId", + err: e.to_string(), + } + }) + }) + .collect::, _>>()?; + Ok(Self::AllowedViewers(BoundedAllowedViewers::new(principals))) + } + } + } +} + /// Struct used for encoding/decoding /// ```text /// record { @@ -1384,6 +1472,7 @@ impl TryFrom for SnapshotVisibility /// minimum_incoming_canister_call_cycles : nat; /// log_visibility : log_visibility; /// snapshot_visibility : snapshot_visibility; +/// status_visibility : status_visibility; /// log_memory_limit : nat; /// wasm_memory_limit : nat; /// wasm_memory_threshold : nat; @@ -1401,6 +1490,7 @@ pub struct DefiniteCanisterSettingsArgs { minimum_incoming_canister_call_cycles: candid::Nat, log_visibility: LogVisibilityV2, snapshot_visibility: SnapshotVisibility, + status_visibility: StatusVisibility, log_memory_limit: candid::Nat, wasm_memory_limit: candid::Nat, wasm_memory_threshold: candid::Nat, @@ -1419,6 +1509,7 @@ impl DefiniteCanisterSettingsArgs { minimum_incoming_canister_call_cycles: u128, log_visibility: LogVisibilityV2, snapshot_visibility: SnapshotVisibility, + status_visibility: StatusVisibility, log_memory_limit: u64, wasm_memory_limit: Option, wasm_memory_threshold: u64, @@ -1446,6 +1537,7 @@ impl DefiniteCanisterSettingsArgs { minimum_incoming_canister_call_cycles, log_visibility, snapshot_visibility, + status_visibility, log_memory_limit: candid::Nat::from(log_memory_limit), wasm_memory_limit, wasm_memory_threshold: candid::Nat::from(wasm_memory_threshold), @@ -1473,6 +1565,10 @@ impl DefiniteCanisterSettingsArgs { &self.snapshot_visibility } + pub fn status_visibility(&self) -> &StatusVisibility { + &self.status_visibility + } + pub fn log_memory_limit(&self) -> candid::Nat { self.log_memory_limit.clone() } @@ -1605,6 +1701,7 @@ impl CanisterStatusResultV2 { minimum_incoming_canister_call_cycles: u128, log_visibility: LogVisibilityV2, snapshot_visibility: SnapshotVisibility, + status_visibility: StatusVisibility, log_memory_limit: u64, idle_cycles_burned_per_day: u128, reserved_cycles: u128, @@ -1648,6 +1745,7 @@ impl CanisterStatusResultV2 { minimum_incoming_canister_call_cycles, log_visibility, snapshot_visibility, + status_visibility, log_memory_limit, wasm_memory_limit, wasm_memory_threshold, @@ -2378,6 +2476,7 @@ pub struct EnvironmentVariable { /// minimum_incoming_canister_call_cycles : opt nat; /// log_visibility : opt log_visibility; /// snapshot_visibility : opt snapshot_visibility; +/// status_visibility : opt status_visibility; /// log_memory_limit : opt nat; /// wasm_memory_limit : opt nat; /// wasm_memory_threshold : opt nat; @@ -2394,6 +2493,7 @@ pub struct CanisterSettingsArgs { pub minimum_incoming_canister_call_cycles: Option, pub log_visibility: Option, pub snapshot_visibility: Option, + pub status_visibility: Option, pub log_memory_limit: Option, pub wasm_memory_limit: Option, pub wasm_memory_threshold: Option, @@ -2415,6 +2515,7 @@ impl CanisterSettingsArgs { minimum_incoming_canister_call_cycles: None, log_visibility: None, snapshot_visibility: None, + status_visibility: None, log_memory_limit: None, wasm_memory_limit: None, wasm_memory_threshold: None, @@ -2433,6 +2534,7 @@ pub struct CanisterSettingsArgsBuilder { minimum_incoming_canister_call_cycles: Option, log_visibility: Option, snapshot_visibility: Option, + status_visibility: Option, log_memory_limit: Option, wasm_memory_limit: Option, wasm_memory_threshold: Option, @@ -2455,6 +2557,7 @@ impl CanisterSettingsArgsBuilder { minimum_incoming_canister_call_cycles: self.minimum_incoming_canister_call_cycles, log_visibility: self.log_visibility, snapshot_visibility: self.snapshot_visibility, + status_visibility: self.status_visibility, log_memory_limit: self.log_memory_limit, wasm_memory_limit: self.wasm_memory_limit, wasm_memory_threshold: self.wasm_memory_threshold, @@ -2561,6 +2664,14 @@ impl CanisterSettingsArgsBuilder { } } + /// Sets the status visibility. + pub fn with_status_visibility(self, status_visibility: StatusVisibility) -> Self { + Self { + status_visibility: Some(status_visibility), + ..self + } + } + /// Sets the log capacity in bytes. pub fn with_log_memory_limit(self, log_memory_limit: u64) -> Self { Self { diff --git a/rs/types/management_canister_types/tests/ic.did b/rs/types/management_canister_types/tests/ic.did index 9c243aff4f8e..5d242892bc83 100644 --- a/rs/types/management_canister_types/tests/ic.did +++ b/rs/types/management_canister_types/tests/ic.did @@ -14,6 +14,12 @@ type snapshot_visibility = variant { allowed_viewers : vec principal; }; +type status_visibility = variant { + controllers; + public; + allowed_viewers : vec principal; +}; + type environment_variable = record { name: text; value: text; @@ -28,6 +34,7 @@ type canister_settings = record { minimum_incoming_canister_call_cycles : opt nat; log_visibility : opt log_visibility; snapshot_visibility : opt snapshot_visibility; + status_visibility : opt status_visibility; log_memory_limit : opt nat; wasm_memory_limit : opt nat; wasm_memory_threshold : opt nat; @@ -44,6 +51,7 @@ type definite_canister_settings = record { minimum_incoming_canister_call_cycles : nat; log_visibility : log_visibility; snapshot_visibility : snapshot_visibility; + status_visibility : status_visibility; log_memory_limit : nat; wasm_memory_limit : nat; wasm_memory_threshold: nat; From c077b463b2599f25b35d593a30a077a721e232e4 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 13:06:39 +0000 Subject: [PATCH 02/12] fixes and tests --- .../src/canister_manager.rs | 24 ++- .../tests/canister_status.rs | 161 ++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) create mode 100644 rs/execution_environment/tests/canister_status.rs diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 77970a75f8f1..39c738a1a2e4 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -180,11 +180,31 @@ impl CanisterManager { } } + // `CanisterStatus` is governed by the canister's status visibility + // settings: subnet admins always retain access; otherwise access is + // granted to the controllers plus any additional allowed viewers, or + // to everyone if the status is public. + Ok(Ic00Method::CanisterStatus) => { + match effective_canister_id { + Some(canister_id) => { + let canister = state.canister_state(&canister_id).ok_or_else(|| UserError::new( + ErrorCode::CanisterNotFound, + format!("Canister {canister_id} not found"), + ))?; + let subnet_admins = state.get_own_subnet_admins(); + validate_status_visibility(canister, subnet_admins, &sender.get()).map_err(|err| err.into()) + }, + None => Err(UserError::new( + ErrorCode::InvalidManagementPayload, + format!("Failed to decode payload for ic00 method: {method_name}"), + )), + } + }, + // These methods are only valid if they are sent by the controller // of the canister or a subnet admin. We assume that the canister // always wants to accept such messages. - Ok(Ic00Method::CanisterStatus) - | Ok(Ic00Method::StartCanister) + Ok(Ic00Method::StartCanister) | Ok(Ic00Method::UninstallCode) | Ok(Ic00Method::StopCanister) | Ok(Ic00Method::DeleteCanister) diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs new file mode 100644 index 000000000000..16d926541199 --- /dev/null +++ b/rs/execution_environment/tests/canister_status.rs @@ -0,0 +1,161 @@ +use ic_base_types::PrincipalId; +use ic_config::{execution_environment::Config as HypervisorConfig, subnet_config::SubnetConfig}; +use ic_management_canister_types_private::{ + BoundedAllowedViewers, CanisterSettingsArgsBuilder, CanisterStatusResultV2, StatusVisibility, +}; +use ic_registry_subnet_type::SubnetType; +use ic_state_machine_tests::{ + ErrorCode, StateMachine, StateMachineBuilder, StateMachineConfig, UserError, +}; +use ic_test_utilities_types::ids::user_test_id; +use ic_types::CanisterId; +use ic_types_cycles::{CanisterCyclesCostSchedule, Cycles}; + +/// Initial cycles balance for the created canister, big enough for a regular test. +const INITIAL_CYCLES_BALANCE: Cycles = Cycles::new(100 * 1_000_000_000_000); + +/// The call path used to invoke the `canister_status` management endpoint. +#[derive(Clone, Copy, Debug)] +enum CallPath { + /// Replicated update call via an ingress message. + Update, + /// Non-replicated query call. + Query, +} + +/// Sets up an application subnet with the given `subnet_admin`. +fn setup(subnet_admin: PrincipalId) -> StateMachine { + let subnet_config = SubnetConfig::new(SubnetType::Application); + StateMachineBuilder::new() + .with_config(Some(StateMachineConfig::new( + subnet_config, + HypervisorConfig::default(), + ))) + .with_subnet_type(SubnetType::Application) + .with_cost_schedule(CanisterCyclesCostSchedule::Free) + .with_subnet_admins(vec![subnet_admin]) + .build() +} + +/// Calls the `canister_status` endpoint via the given call path as `sender`. +fn canister_status( + env: &StateMachine, + call_path: CallPath, + sender: PrincipalId, + canister_id: CanisterId, +) -> Result, UserError> { + match call_path { + CallPath::Update => env.canister_status_as(sender, canister_id), + CallPath::Query => env.canister_status_query_as(sender, canister_id), + } +} + +#[test] +fn test_status_visibility_of_canister_status() { + // Test combinations of status_visibility, sender, and call path for the + // `canister_status` management endpoint. + let controller = user_test_id(1).get(); + let subnet_admin = user_test_id(100).get(); + let allowed_viewer = user_test_id(3).get(); + // A principal that is neither a controller, nor a subnet admin, nor an + // allowed viewer. + let other = user_test_id(4).get(); + let allowed_viewers = BoundedAllowedViewers::new(vec![allowed_viewer]); + + // (status_visibility, sender, sender_label, expected_allowed) + let test_cases = vec![ + // Controllers (default): only controllers and subnet admins have access. + ( + StatusVisibility::Controllers, + controller, + "controller", + true, + ), + ( + StatusVisibility::Controllers, + subnet_admin, + "subnet_admin", + true, + ), + ( + StatusVisibility::Controllers, + allowed_viewer, + "allowed_viewer", + false, + ), + (StatusVisibility::Controllers, other, "other", false), + // Public: everyone has access. + (StatusVisibility::Public, controller, "controller", true), + (StatusVisibility::Public, subnet_admin, "subnet_admin", true), + ( + StatusVisibility::Public, + allowed_viewer, + "allowed_viewer", + true, + ), + (StatusVisibility::Public, other, "other", true), + // AllowedViewers: controllers, subnet admins, and the listed viewers. + ( + StatusVisibility::AllowedViewers(allowed_viewers.clone()), + controller, + "controller", + true, + ), + ( + StatusVisibility::AllowedViewers(allowed_viewers.clone()), + subnet_admin, + "subnet_admin", + true, + ), + ( + StatusVisibility::AllowedViewers(allowed_viewers.clone()), + allowed_viewer, + "allowed_viewer", + true, + ), + ( + StatusVisibility::AllowedViewers(allowed_viewers.clone()), + other, + "other", + false, + ), + ]; + + for (status_visibility, sender, sender_label, expected_allowed) in test_cases { + let env = setup(subnet_admin); + let canister_id = env.create_canister_with_cycles( + None, + INITIAL_CYCLES_BALANCE, + Some( + CanisterSettingsArgsBuilder::new() + .with_controllers(vec![controller]) + .with_status_visibility(status_visibility.clone()) + .build(), + ), + ); + assert_ne!(sender, canister_id.get()); + + for call_path in [CallPath::Update, CallPath::Query] { + let result = canister_status(&env, call_path, sender, canister_id); + if expected_allowed { + assert!( + matches!(result, Ok(Ok(_))), + "expected access to be granted for status_visibility: \ + {status_visibility:?}, sender: {sender_label}, call path: {call_path:?}, \ + but got: {result:?}" + ); + } else { + let err = result.expect_err(&format!( + "expected access to be denied for status_visibility: \ + {status_visibility:?}, sender: {sender_label}, call path: {call_path:?}" + )); + assert_eq!( + err.code(), + ErrorCode::CanisterInvalidControllerOrSubnetAdmin, + "unexpected error for status_visibility: {status_visibility:?}, \ + sender: {sender_label}, call path: {call_path:?}" + ); + } + } + } +} From 2c0413f9361835197e7e469f914efcca339e706a Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 13:09:49 +0000 Subject: [PATCH 03/12] simplify --- .../tests/canister_status.rs | 9 ++ .../tests/execution_test.rs | 117 ------------------ 2 files changed, 9 insertions(+), 117 deletions(-) diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs index 16d926541199..6c809d73bff5 100644 --- a/rs/execution_environment/tests/canister_status.rs +++ b/rs/execution_environment/tests/canister_status.rs @@ -155,6 +155,15 @@ fn test_status_visibility_of_canister_status() { "unexpected error for status_visibility: {status_visibility:?}, \ sender: {sender_label}, call path: {call_path:?}" ); + assert!( + err.description().contains(&format!( + "Only the controllers of the canister {canister_id} \ + or subnet admins can perform certain actions" + )), + "unexpected error description for status_visibility: {status_visibility:?}, \ + sender: {sender_label}, call path: {call_path:?}, description: {}", + err.description() + ); } } } diff --git a/rs/execution_environment/tests/execution_test.rs b/rs/execution_environment/tests/execution_test.rs index 1f5cc81ce53c..10402105f037 100644 --- a/rs/execution_environment/tests/execution_test.rs +++ b/rs/execution_environment/tests/execution_test.rs @@ -2759,123 +2759,6 @@ fn get_canister_metadata() { ); } -fn canister_status_count(env: &StateMachine) -> u64 { - fetch_histogram_vec_stats( - env.metrics_registry(), - "execution_subnet_query_message_duration_seconds", - ) - .get(&labels(&[ - ("method_name", "query_ic00_canister_status"), - ("status", "success"), - ])) - .map_or(0, |stats| stats.count) -} - -#[test] -fn canister_status_via_query_call_by_controller_succeeds() { - let subnet_config = SubnetConfig::new(SubnetType::Application); - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - subnet_config, - HypervisorConfig::default(), - ))) - .build(); - let canister_id = create_universal_canister_with_cycles( - &env, - Some(CanisterSettingsArgsBuilder::new().build()), - INITIAL_CYCLES_BALANCE, - ); - - assert_eq!(canister_status_count(&env), 0); - - let result = env.query( - CanisterId::ic_00(), - "canister_status", - CanisterIdRecord::from(canister_id).encode(), - ); - - assert!(result.is_ok()); - assert_eq!(canister_status_count(&env), 1); -} - -#[test] -fn canister_status_via_query_call_by_subnet_admin_succeeds() { - let subnet_config = SubnetConfig::new(SubnetType::Application); - let subnet_admin = user_test_id(100); - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - subnet_config, - HypervisorConfig::default(), - ))) - .with_subnet_type(SubnetType::Application) - .with_cost_schedule(CanisterCyclesCostSchedule::Free) - .with_subnet_admins(vec![subnet_admin.get()]) - .build(); - let canister_id = create_universal_canister_with_cycles( - &env, - Some(CanisterSettingsArgsBuilder::new().build()), - INITIAL_CYCLES_BALANCE, - ); - // Get the first canister controller. - let controller = env.get_controllers(canister_id).unwrap()[0]; - - assert_eq!(canister_status_count(&env), 0); - assert_ne!(subnet_admin.get(), controller); - - let result = env.query_as( - subnet_admin.get(), - CanisterId::ic_00(), - "canister_status", - CanisterIdRecord::from(canister_id).encode(), - ); - assert!(result.is_ok()); - assert_eq!(canister_status_count(&env), 1); -} - -#[test] -fn canister_status_via_query_call_by_neither_controller_nor_subnet_admin_fails() { - let subnet_config = SubnetConfig::new(SubnetType::Application); - let subnet_admin = user_test_id(100); - let test_user = user_test_id(101); - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - subnet_config, - HypervisorConfig::default(), - ))) - .with_subnet_type(SubnetType::Application) - .with_cost_schedule(CanisterCyclesCostSchedule::Free) - .with_subnet_admins(vec![subnet_admin.get()]) - .build(); - let canister_id = create_universal_canister_with_cycles( - &env, - Some(CanisterSettingsArgsBuilder::new().build()), - INITIAL_CYCLES_BALANCE, - ); - // Get the first canister controller. - let controller = env.get_controllers(canister_id).unwrap()[0]; - - assert_eq!(canister_status_count(&env), 0); - assert_ne!(subnet_admin.get(), controller); - assert_ne!(test_user.get(), controller); - - let err = env - .query_as( - test_user.get(), - CanisterId::ic_00(), - "canister_status", - CanisterIdRecord::from(canister_id).encode(), - ) - .unwrap_err(); - assert_eq!( - err.code(), - ErrorCode::CanisterInvalidControllerOrSubnetAdmin - ); - assert!(err.description().contains(&format!( - "Only the controllers of the canister {canister_id} or subnet admins can perform certain actions" - ))); - assert_eq!(canister_status_count(&env), 0); -} - #[test] fn maximum_state_size() { let maximum_state_size = NumBytes::new(1 << 30); From 0f8a3aa54fc76835a3b159aa8f52859f9746b7c4 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 13:17:36 +0000 Subject: [PATCH 04/12] error messages --- rs/execution_environment/src/canister_manager.rs | 4 ++-- .../src/canister_manager/types.rs | 16 ++++++++++++++++ rs/execution_environment/src/execution/common.rs | 11 ++++++----- .../tests/canister_status.rs | 5 ++--- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 39c738a1a2e4..b867d81a5b15 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -192,7 +192,7 @@ impl CanisterManager { format!("Canister {canister_id} not found"), ))?; let subnet_admins = state.get_own_subnet_admins(); - validate_status_visibility(canister, subnet_admins, &sender.get()).map_err(|err| err.into()) + validate_status_visibility(canister, subnet_admins, &sender.get(), method_name).map_err(|err| err.into()) }, None => Err(UserError::new( ErrorCode::InvalidManagementPayload, @@ -1097,7 +1097,7 @@ impl CanisterManager { // Skip the visibility check if the canister itself is requesting its // own status, as the canister is considered in the same trust domain. if sender != canister.canister_id().get() { - validate_status_visibility(canister, subnet_admins, &sender)? + validate_status_visibility(canister, subnet_admins, &sender, "canister_status")? } let controller = canister.system_state.controller(); diff --git a/rs/execution_environment/src/canister_manager/types.rs b/rs/execution_environment/src/canister_manager/types.rs index 83b69be5b19f..11826e20e6c2 100644 --- a/rs/execution_environment/src/canister_manager/types.rs +++ b/rs/execution_environment/src/canister_manager/types.rs @@ -501,6 +501,10 @@ pub(crate) enum CanisterManagerError { caller: PrincipalId, method_name: String, }, + CanisterStatusAccessDenied { + caller: PrincipalId, + method_name: String, + }, FetchCanisterLogsNotEnoughCycles { sent: Cycles, required: Cycles, @@ -761,6 +765,11 @@ impl AsErrorHelp for CanisterManagerError { .to_string(), doc_link: doc_ref("invalid-controller"), }, + CanisterManagerError::CanisterStatusAccessDenied { .. } => ErrorHelp::UserError { + suggestion: "Execute this call from a principal with canister status read access." + .to_string(), + doc_link: doc_ref("invalid-controller"), + }, CanisterManagerError::FetchCanisterLogsAccessDenied { .. } => ErrorHelp::UserError { suggestion: "Execute this call from a controller of the target canister or \ a principal with log read access." @@ -1201,6 +1210,13 @@ impl From for UserError { ErrorCode::CanisterRejectedMessage, format!("Caller {caller} is not allowed to call {method_name}"), ), + CanisterStatusAccessDenied { + caller, + method_name, + } => Self::new( + ErrorCode::CanisterRejectedMessage, + format!("Caller {caller} is not allowed to call {method_name}"), + ), CanisterLogMemoryLimitIsTooHigh { bytes, limit } => Self::new( ErrorCode::CanisterRejectedMessage, format!( diff --git a/rs/execution_environment/src/execution/common.rs b/rs/execution_environment/src/execution/common.rs index c062ae537ff9..936630c1c208 100644 --- a/rs/execution_environment/src/execution/common.rs +++ b/rs/execution_environment/src/execution/common.rs @@ -375,14 +375,14 @@ pub(crate) fn validate_snapshot_visibility( /// Validates that the `caller` is allowed to read the status of the `canister` /// according to its canister status visibility settings. /// -/// Subnet admins always retain access (preserving the historical behavior of -/// `validate_controller_or_subnet_admin`); otherwise access is governed by the +/// Subnet admins always retain access; otherwise access is governed by the /// status visibility setting, which grants access to the controllers plus any /// additional allowed viewers (or everyone, if the status is public). pub(crate) fn validate_status_visibility( canister: &CanisterState, subnet_admins: Option>, caller: &PrincipalId, + method_name: &str, ) -> Result<(), CanisterManagerError> { // Subnet admins always retain access to the canister status. if let Some(subnet_admins) = &subnet_admins @@ -396,9 +396,10 @@ pub(crate) fn validate_status_visibility( { return Ok(()); } - // Fall back to the legacy controller-or-subnet-admin error to preserve - // backward-compatible error reporting. - validate_controller_or_subnet_admin(canister, subnet_admins, caller) + Err(CanisterManagerError::CanisterStatusAccessDenied { + caller: *caller, + method_name: method_name.to_string(), + }) } pub(crate) fn validate_subnet_admin( diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs index 6c809d73bff5..03788358b17c 100644 --- a/rs/execution_environment/tests/canister_status.rs +++ b/rs/execution_environment/tests/canister_status.rs @@ -151,14 +151,13 @@ fn test_status_visibility_of_canister_status() { )); assert_eq!( err.code(), - ErrorCode::CanisterInvalidControllerOrSubnetAdmin, + ErrorCode::CanisterRejectedMessage, "unexpected error for status_visibility: {status_visibility:?}, \ sender: {sender_label}, call path: {call_path:?}" ); assert!( err.description().contains(&format!( - "Only the controllers of the canister {canister_id} \ - or subnet admins can perform certain actions" + "Caller {sender} is not allowed to call canister_status" )), "unexpected error description for status_visibility: {status_visibility:?}, \ sender: {sender_label}, call path: {call_path:?}, description: {}", From b8e90fa1466a46fb2b3b9011f0d9243bcb2c354e Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 13:18:00 +0000 Subject: [PATCH 05/12] docref --- rs/execution_environment/src/canister_manager/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/execution_environment/src/canister_manager/types.rs b/rs/execution_environment/src/canister_manager/types.rs index 11826e20e6c2..ad49698176e0 100644 --- a/rs/execution_environment/src/canister_manager/types.rs +++ b/rs/execution_environment/src/canister_manager/types.rs @@ -768,7 +768,7 @@ impl AsErrorHelp for CanisterManagerError { CanisterManagerError::CanisterStatusAccessDenied { .. } => ErrorHelp::UserError { suggestion: "Execute this call from a principal with canister status read access." .to_string(), - doc_link: doc_ref("invalid-controller"), + doc_link: "".to_string(), }, CanisterManagerError::FetchCanisterLogsAccessDenied { .. } => ErrorHelp::UserError { suggestion: "Execute this call from a controller of the target canister or \ From c6b11fa344c4c9fd49aedaaaed1ad75bec9babea Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 14:21:12 +0000 Subject: [PATCH 06/12] fix --- rs/nervous_system/clients/src/update_settings.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rs/nervous_system/clients/src/update_settings.rs b/rs/nervous_system/clients/src/update_settings.rs index d5c4d8a2a3c7..a3c0fab644a7 100644 --- a/rs/nervous_system/clients/src/update_settings.rs +++ b/rs/nervous_system/clients/src/update_settings.rs @@ -88,6 +88,7 @@ impl From for management_canister::CanisterSettingsArgs { log_visibility: log_visibility.map(management_canister::LogVisibilityV2::from), snapshot_visibility: snapshot_visibility .map(management_canister::SnapshotVisibility::from), + status_visibility: None, log_memory_limit: None, wasm_memory_limit, wasm_memory_threshold, From 6991f22b2ecd4004cb1a4c4eb6c8cd158de43847 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 16:02:03 +0000 Subject: [PATCH 07/12] new error code --- packages/ic-error-types/src/lib.rs | 5 ++++- packages/pocket-ic/src/lib.rs | 2 ++ .../src/cycles_account_manager.rs | 2 +- .../src/canister_manager/tests.rs | 11 ++++------- .../src/canister_manager/types.rs | 7 ++++++- .../src/execution_environment/tests.rs | 7 ++----- rs/execution_environment/src/history.rs | 1 + rs/execution_environment/tests/canister_status.rs | 2 +- rs/protobuf/def/state/ingress/v1/ingress.proto | 1 + rs/protobuf/src/gen/state/state.ingress.v1.rs | 3 +++ rs/protobuf/src/gen/types/state.ingress.v1.rs | 3 +++ rs/protobuf/src/state/ingress/mod.rs | 6 ++++++ rs/rust_canisters/tests/test/canister_management.rs | 2 +- 13 files changed, 35 insertions(+), 17 deletions(-) diff --git a/packages/ic-error-types/src/lib.rs b/packages/ic-error-types/src/lib.rs index d9b88c927187..c07cb36ae4ae 100644 --- a/packages/ic-error-types/src/lib.rs +++ b/packages/ic-error-types/src/lib.rs @@ -97,6 +97,7 @@ impl From for RejectCode { // Canister errors. CanisterInvalidController => CanisterError, CanisterInvalidControllerOrSubnetAdmin => CanisterError, + CanisterStatusAccessDenied => CanisterError, CanisterFunctionNotFound => CanisterError, CanisterNonEmpty => CanisterError, CanisterTrapped => CanisterError, @@ -222,6 +223,7 @@ pub enum ErrorCode { CanisterWasmMemoryLimitExceeded = 539, ReservedCyclesLimitIsTooLow = 540, CanisterInvalidControllerOrSubnetAdmin = 541, + CanisterStatusAccessDenied = 542, // 6xx -- `RejectCode::SysUnknown` DeadlineExpired = 601, ResponseDropped = 602, @@ -337,6 +339,7 @@ impl UserError { | ErrorCode::CanisterStoppingCancelled | ErrorCode::CanisterInvalidController | ErrorCode::CanisterInvalidControllerOrSubnetAdmin + | ErrorCode::CanisterStatusAccessDenied | ErrorCode::CanisterFunctionNotFound | ErrorCode::CanisterNonEmpty | ErrorCode::QueryCallGraphLoopDetected @@ -442,7 +445,7 @@ mod tests { 402, 403, 404, 405, 406, 407, 408, 409, 410, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514, 517, 520, 521, 522, 524, 525, 526, 527, 528, 529, 530, 531, 532, - 533, 534, 535, 536, 537, 538, 539, 540, 541, + 533, 534, 535, 536, 537, 538, 539, 540, 541, 542, 601, 602, ] ); diff --git a/packages/pocket-ic/src/lib.rs b/packages/pocket-ic/src/lib.rs index 5d38f4ff19a9..ba4bb9953eb3 100644 --- a/packages/pocket-ic/src/lib.rs +++ b/packages/pocket-ic/src/lib.rs @@ -1947,6 +1947,7 @@ pub enum ErrorCode { CanisterWasmMemoryLimitExceeded = 539, ReservedCyclesLimitIsTooLow = 540, CanisterInvalidControllerOrSubnetAdmin = 541, + CanisterStatusAccessDenied = 542, // 6xx -- `RejectCode::SysUnknown` DeadlineExpired = 601, ResponseDropped = 602, @@ -2019,6 +2020,7 @@ impl TryFrom for ErrorCode { 539 => Ok(ErrorCode::CanisterWasmMemoryLimitExceeded), 540 => Ok(ErrorCode::ReservedCyclesLimitIsTooLow), 541 => Ok(ErrorCode::CanisterInvalidControllerOrSubnetAdmin), + 542 => Ok(ErrorCode::CanisterStatusAccessDenied), // 6xx -- `RejectCode::SysUnknown` 601 => Ok(ErrorCode::DeadlineExpired), 602 => Ok(ErrorCode::ResponseDropped), diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index f0d943a7d3c3..a127048e2671 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -42,7 +42,7 @@ const DAY: Duration = Duration::from_secs(SECONDS_PER_DAY as u64); /// Maximum payload size of a management call to update_settings /// overriding the canister's freezing threshold. -const MAX_DELAYED_INGRESS_COST_PAYLOAD_SIZE: usize = 352; +const MAX_DELAYED_INGRESS_COST_PAYLOAD_SIZE: usize = 366; struct CyclesBurnedRate { memory: CompoundCycles, diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index f0dbcbfbaf5d..1b4654ab2d26 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -1391,9 +1391,9 @@ fn get_canister_status_with_incorrect_controller_fails() { let err = test.canister_status(canister_id).unwrap_err(); - assert_eq!(err.code(), ErrorCode::CanisterInvalidController); + assert_eq!(err.code(), ErrorCode::CanisterStatusAccessDenied); assert!(err.description().contains(&format!( - "Only the controllers of the canister {canister_id} can control it" + "Caller {test_user} is not allowed to call canister_status" ))); } @@ -8603,12 +8603,9 @@ fn non_controller_and_non_subnet_admin_cannot_perform_subnet_admin_actions_on_ca // ...or status cannot be checked... let err = test.canister_status(canister_id).unwrap_err(); - assert_eq!( - err.code(), - ErrorCode::CanisterInvalidControllerOrSubnetAdmin - ); + assert_eq!(err.code(), ErrorCode::CanisterStatusAccessDenied); assert!(err.description().contains(&format!( - "Only the controllers of the canister {canister_id} or subnet admins can perform certain actions" + "Caller {test_user} is not allowed to call canister_status" ))); // ...or canister metrics cannot be retrieved... let err = test.canister_metrics(canister_id).unwrap_err(); diff --git a/rs/execution_environment/src/canister_manager/types.rs b/rs/execution_environment/src/canister_manager/types.rs index ad49698176e0..ad746bcab7e8 100644 --- a/rs/execution_environment/src/canister_manager/types.rs +++ b/rs/execution_environment/src/canister_manager/types.rs @@ -1214,7 +1214,12 @@ impl From for UserError { caller, method_name, } => Self::new( - ErrorCode::CanisterRejectedMessage, + // `CanisterStatusAccessDenied` is a dedicated error code that is + // mapped to the same reject code (`CanisterError`) as the + // `CanisterInvalidController` error code that governed access to + // `canister_status` before the status visibility feature was + // introduced. + ErrorCode::CanisterStatusAccessDenied, format!("Caller {caller} is not allowed to call {method_name}"), ), CanisterLogMemoryLimitIsTooHigh { bytes, limit } => Self::new( diff --git a/rs/execution_environment/src/execution_environment/tests.rs b/rs/execution_environment/src/execution_environment/tests.rs index 6f76dcd5e6e9..35f94a9328e1 100644 --- a/rs/execution_environment/src/execution_environment/tests.rs +++ b/rs/execution_environment/src/execution_environment/tests.rs @@ -3051,7 +3051,7 @@ fn management_message_with_invalid_sender_is_not_accepted_without_subnet_admins( let err = test .should_accept_ingress_message(IC_00, "canister_status", Encode!(&arg).unwrap()) .unwrap_err(); - assert_eq!(ErrorCode::CanisterInvalidController, err.code()); + assert_eq!(ErrorCode::CanisterStatusAccessDenied, err.code()); } #[test] @@ -3071,10 +3071,7 @@ fn management_message_with_invalid_sender_is_not_accepted_with_subnet_admins() { let err = test .should_accept_ingress_message(IC_00, "canister_status", Encode!(&arg).unwrap()) .unwrap_err(); - assert_eq!( - ErrorCode::CanisterInvalidControllerOrSubnetAdmin, - err.code() - ); + assert_eq!(ErrorCode::CanisterStatusAccessDenied, err.code()); } #[test] diff --git a/rs/execution_environment/src/history.rs b/rs/execution_environment/src/history.rs index 0f195afbce25..070058b5ab24 100644 --- a/rs/execution_environment/src/history.rs +++ b/rs/execution_environment/src/history.rs @@ -396,6 +396,7 @@ fn dashboard_label_value_from(code: ErrorCode) -> &'static str { CanisterStoppingCancelled => "Canister Stopping Cancelled", CanisterInvalidController => "Canister Invalid Controller", CanisterInvalidControllerOrSubnetAdmin => "Canister Invalid Controller Or Subnet Admin", + CanisterStatusAccessDenied => "Canister Status Access Denied", CanisterFunctionNotFound => "Canister Function Not Found", CanisterNonEmpty => "Canister Non-Empty", QueryCallGraphLoopDetected => "Loop in inter-canister query call graph", diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs index 03788358b17c..02272bbf503b 100644 --- a/rs/execution_environment/tests/canister_status.rs +++ b/rs/execution_environment/tests/canister_status.rs @@ -151,7 +151,7 @@ fn test_status_visibility_of_canister_status() { )); assert_eq!( err.code(), - ErrorCode::CanisterRejectedMessage, + ErrorCode::CanisterStatusAccessDenied, "unexpected error for status_visibility: {status_visibility:?}, \ sender: {sender_label}, call path: {call_path:?}" ); diff --git a/rs/protobuf/def/state/ingress/v1/ingress.proto b/rs/protobuf/def/state/ingress/v1/ingress.proto index 31f76b28a826..ee936355ce7d 100644 --- a/rs/protobuf/def/state/ingress/v1/ingress.proto +++ b/rs/protobuf/def/state/ingress/v1/ingress.proto @@ -97,6 +97,7 @@ enum ErrorCode { ERROR_CODE_CANISTER_WASM_MEMORY_LIMIT_EXCEEDED = 539; ERROR_CODE_RESERVED_CYCLES_LIMIT_IS_TOO_LOW = 540; ERROR_CODE_CANISTER_INVALID_CONTROLLER_OR_SUBNET_ADMIN = 541; + ERROR_CODE_CANISTER_STATUS_ACCESS_DENIED = 542; // 6xx -- `RejectCode::SysUnknown` ERROR_CODE_DEADLINE_EXPIRED = 601; ERROR_CODE_RESPONSE_DROPPED = 602; diff --git a/rs/protobuf/src/gen/state/state.ingress.v1.rs b/rs/protobuf/src/gen/state/state.ingress.v1.rs index 7e2cacfd2647..4e8ccfeb95c7 100644 --- a/rs/protobuf/src/gen/state/state.ingress.v1.rs +++ b/rs/protobuf/src/gen/state/state.ingress.v1.rs @@ -204,6 +204,7 @@ pub enum ErrorCode { CanisterWasmMemoryLimitExceeded = 539, ReservedCyclesLimitIsTooLow = 540, CanisterInvalidControllerOrSubnetAdmin = 541, + CanisterStatusAccessDenied = 542, /// 6xx -- `RejectCode::SysUnknown` DeadlineExpired = 601, ResponseDropped = 602, @@ -298,6 +299,7 @@ impl ErrorCode { Self::CanisterInvalidControllerOrSubnetAdmin => { "ERROR_CODE_CANISTER_INVALID_CONTROLLER_OR_SUBNET_ADMIN" } + Self::CanisterStatusAccessDenied => "ERROR_CODE_CANISTER_STATUS_ACCESS_DENIED", Self::DeadlineExpired => "ERROR_CODE_DEADLINE_EXPIRED", Self::ResponseDropped => "ERROR_CODE_RESPONSE_DROPPED", } @@ -396,6 +398,7 @@ impl ErrorCode { "ERROR_CODE_CANISTER_INVALID_CONTROLLER_OR_SUBNET_ADMIN" => { Some(Self::CanisterInvalidControllerOrSubnetAdmin) } + "ERROR_CODE_CANISTER_STATUS_ACCESS_DENIED" => Some(Self::CanisterStatusAccessDenied), "ERROR_CODE_DEADLINE_EXPIRED" => Some(Self::DeadlineExpired), "ERROR_CODE_RESPONSE_DROPPED" => Some(Self::ResponseDropped), _ => None, diff --git a/rs/protobuf/src/gen/types/state.ingress.v1.rs b/rs/protobuf/src/gen/types/state.ingress.v1.rs index 7e2cacfd2647..4e8ccfeb95c7 100644 --- a/rs/protobuf/src/gen/types/state.ingress.v1.rs +++ b/rs/protobuf/src/gen/types/state.ingress.v1.rs @@ -204,6 +204,7 @@ pub enum ErrorCode { CanisterWasmMemoryLimitExceeded = 539, ReservedCyclesLimitIsTooLow = 540, CanisterInvalidControllerOrSubnetAdmin = 541, + CanisterStatusAccessDenied = 542, /// 6xx -- `RejectCode::SysUnknown` DeadlineExpired = 601, ResponseDropped = 602, @@ -298,6 +299,7 @@ impl ErrorCode { Self::CanisterInvalidControllerOrSubnetAdmin => { "ERROR_CODE_CANISTER_INVALID_CONTROLLER_OR_SUBNET_ADMIN" } + Self::CanisterStatusAccessDenied => "ERROR_CODE_CANISTER_STATUS_ACCESS_DENIED", Self::DeadlineExpired => "ERROR_CODE_DEADLINE_EXPIRED", Self::ResponseDropped => "ERROR_CODE_RESPONSE_DROPPED", } @@ -396,6 +398,7 @@ impl ErrorCode { "ERROR_CODE_CANISTER_INVALID_CONTROLLER_OR_SUBNET_ADMIN" => { Some(Self::CanisterInvalidControllerOrSubnetAdmin) } + "ERROR_CODE_CANISTER_STATUS_ACCESS_DENIED" => Some(Self::CanisterStatusAccessDenied), "ERROR_CODE_DEADLINE_EXPIRED" => Some(Self::DeadlineExpired), "ERROR_CODE_RESPONSE_DROPPED" => Some(Self::ResponseDropped), _ => None, diff --git a/rs/protobuf/src/state/ingress/mod.rs b/rs/protobuf/src/state/ingress/mod.rs index 0b33f239152d..c0d22d9a459f 100644 --- a/rs/protobuf/src/state/ingress/mod.rs +++ b/rs/protobuf/src/state/ingress/mod.rs @@ -57,6 +57,9 @@ pub mod v1 { ErrorCodePublic::CanisterInvalidControllerOrSubnetAdmin => { ErrorCode::CanisterInvalidControllerOrSubnetAdmin } + ErrorCodePublic::CanisterStatusAccessDenied => { + ErrorCode::CanisterStatusAccessDenied + } ErrorCodePublic::CanisterFunctionNotFound => ErrorCode::CanisterFunctionNotFound, ErrorCodePublic::CanisterNonEmpty => ErrorCode::CanisterNonEmpty, ErrorCodePublic::QueryCallGraphLoopDetected => { @@ -189,6 +192,9 @@ pub mod v1 { ErrorCode::CanisterInvalidControllerOrSubnetAdmin => { Ok(ErrorCodePublic::CanisterInvalidControllerOrSubnetAdmin) } + ErrorCode::CanisterStatusAccessDenied => { + Ok(ErrorCodePublic::CanisterStatusAccessDenied) + } ErrorCode::CanisterFunctionNotFound => { Ok(ErrorCodePublic::CanisterFunctionNotFound) } diff --git a/rs/rust_canisters/tests/test/canister_management.rs b/rs/rust_canisters/tests/test/canister_management.rs index a7cf1e1544fc..93fb3b10076e 100644 --- a/rs/rust_canisters/tests/test/canister_management.rs +++ b/rs/rust_canisters/tests/test/canister_management.rs @@ -45,7 +45,7 @@ fn test_set_controller() { ) .await; - assert_matches!(res, Err(msg) if msg.contains(&ErrorCode::CanisterInvalidController.to_string())); + assert_matches!(res, Err(msg) if msg.contains(&ErrorCode::CanisterStatusAccessDenied.to_string())); // Now call canister_status from the controller let arg = universal_canister_argument_builder() From 496d4e87447af4dbc1c490abc0797ee77bdfa6f8 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 6 Jul 2026 20:39:43 +0000 Subject: [PATCH 08/12] fix --- rs/canonical_state/tests/hash_tree.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rs/canonical_state/tests/hash_tree.rs b/rs/canonical_state/tests/hash_tree.rs index 3308dcb2093b..5073018a05b7 100644 --- a/rs/canonical_state/tests/hash_tree.rs +++ b/rs/canonical_state/tests/hash_tree.rs @@ -208,8 +208,8 @@ fn error_code_change_guard() { // the name. assert_eq!( [ - 252, 204, 185, 141, 145, 243, 70, 19, 233, 148, 60, 55, 116, 136, 68, 22, 127, 255, 85, - 15, 252, 219, 68, 239, 166, 87, 109, 22, 28, 238, 47, 129 + 58, 36, 133, 223, 16, 218, 168, 51, 248, 11, 191, 177, 232, 246, 109, 148, 95, 219, 30, + 17, 111, 165, 156, 98, 245, 151, 12, 34, 140, 243, 215, 7 ], hasher.finish() ); From f9b2fb63ed07548c583f4ed2f0a5a77bde23c92d Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Tue, 7 Jul 2026 05:40:54 +0000 Subject: [PATCH 09/12] misc --- rs/execution_environment/src/canister_manager.rs | 4 ++-- .../src/canister_manager/tests.rs | 4 ++-- .../src/canister_manager/types.rs | 8 ++------ rs/execution_environment/src/execution/common.rs | 6 +----- rs/execution_environment/tests/canister_status.rs | 15 +++++++++------ .../v1/canister_state_bits.proto | 2 +- .../src/gen/state/state.canister_state_bits.v1.rs | 2 +- rs/types/management_canister_types/src/lib.rs | 4 ++-- 8 files changed, 20 insertions(+), 25 deletions(-) diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index b867d81a5b15..39c738a1a2e4 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -192,7 +192,7 @@ impl CanisterManager { format!("Canister {canister_id} not found"), ))?; let subnet_admins = state.get_own_subnet_admins(); - validate_status_visibility(canister, subnet_admins, &sender.get(), method_name).map_err(|err| err.into()) + validate_status_visibility(canister, subnet_admins, &sender.get()).map_err(|err| err.into()) }, None => Err(UserError::new( ErrorCode::InvalidManagementPayload, @@ -1097,7 +1097,7 @@ impl CanisterManager { // Skip the visibility check if the canister itself is requesting its // own status, as the canister is considered in the same trust domain. if sender != canister.canister_id().get() { - validate_status_visibility(canister, subnet_admins, &sender, "canister_status")? + validate_status_visibility(canister, subnet_admins, &sender)? } let controller = canister.system_state.controller(); diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index 1b4654ab2d26..d4d7d018d056 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -1393,7 +1393,7 @@ fn get_canister_status_with_incorrect_controller_fails() { assert_eq!(err.code(), ErrorCode::CanisterStatusAccessDenied); assert!(err.description().contains(&format!( - "Caller {test_user} is not allowed to call canister_status" + "Caller {test_user} is not allowed to read the canister status" ))); } @@ -8605,7 +8605,7 @@ fn non_controller_and_non_subnet_admin_cannot_perform_subnet_admin_actions_on_ca let err = test.canister_status(canister_id).unwrap_err(); assert_eq!(err.code(), ErrorCode::CanisterStatusAccessDenied); assert!(err.description().contains(&format!( - "Caller {test_user} is not allowed to call canister_status" + "Caller {test_user} is not allowed to read the canister status" ))); // ...or canister metrics cannot be retrieved... let err = test.canister_metrics(canister_id).unwrap_err(); diff --git a/rs/execution_environment/src/canister_manager/types.rs b/rs/execution_environment/src/canister_manager/types.rs index ad746bcab7e8..0dea8faf8208 100644 --- a/rs/execution_environment/src/canister_manager/types.rs +++ b/rs/execution_environment/src/canister_manager/types.rs @@ -503,7 +503,6 @@ pub(crate) enum CanisterManagerError { }, CanisterStatusAccessDenied { caller: PrincipalId, - method_name: String, }, FetchCanisterLogsNotEnoughCycles { sent: Cycles, @@ -1210,17 +1209,14 @@ impl From for UserError { ErrorCode::CanisterRejectedMessage, format!("Caller {caller} is not allowed to call {method_name}"), ), - CanisterStatusAccessDenied { - caller, - method_name, - } => Self::new( + CanisterStatusAccessDenied { caller } => Self::new( // `CanisterStatusAccessDenied` is a dedicated error code that is // mapped to the same reject code (`CanisterError`) as the // `CanisterInvalidController` error code that governed access to // `canister_status` before the status visibility feature was // introduced. ErrorCode::CanisterStatusAccessDenied, - format!("Caller {caller} is not allowed to call {method_name}"), + format!("Caller {caller} is not allowed to read the canister status"), ), CanisterLogMemoryLimitIsTooHigh { bytes, limit } => Self::new( ErrorCode::CanisterRejectedMessage, diff --git a/rs/execution_environment/src/execution/common.rs b/rs/execution_environment/src/execution/common.rs index 936630c1c208..5e2387966159 100644 --- a/rs/execution_environment/src/execution/common.rs +++ b/rs/execution_environment/src/execution/common.rs @@ -382,7 +382,6 @@ pub(crate) fn validate_status_visibility( canister: &CanisterState, subnet_admins: Option>, caller: &PrincipalId, - method_name: &str, ) -> Result<(), CanisterManagerError> { // Subnet admins always retain access to the canister status. if let Some(subnet_admins) = &subnet_admins @@ -396,10 +395,7 @@ pub(crate) fn validate_status_visibility( { return Ok(()); } - Err(CanisterManagerError::CanisterStatusAccessDenied { - caller: *caller, - method_name: method_name.to_string(), - }) + Err(CanisterManagerError::CanisterStatusAccessDenied { caller: *caller }) } pub(crate) fn validate_subnet_admin( diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs index 02272bbf503b..4dd62b809c0b 100644 --- a/rs/execution_environment/tests/canister_status.rs +++ b/rs/execution_environment/tests/canister_status.rs @@ -138,11 +138,14 @@ fn test_status_visibility_of_canister_status() { for call_path in [CallPath::Update, CallPath::Query] { let result = canister_status(&env, call_path, sender, canister_id); if expected_allowed { - assert!( - matches!(result, Ok(Ok(_))), - "expected access to be granted for status_visibility: \ - {status_visibility:?}, sender: {sender_label}, call path: {call_path:?}, \ - but got: {result:?}" + let status = result + .expect("expected a successful response") + .expect("expected access to be granted"); + assert_eq!( + status.settings().status_visibility(), + &status_visibility, + "unexpected status_visibility for status_visibility: \ + {status_visibility:?}, sender: {sender_label}, call path: {call_path:?}" ); } else { let err = result.expect_err(&format!( @@ -157,7 +160,7 @@ fn test_status_visibility_of_canister_status() { ); assert!( err.description().contains(&format!( - "Caller {sender} is not allowed to call canister_status" + "Caller {sender} is not allowed to read the canister status" )), "unexpected error description for status_visibility: {status_visibility:?}, \ sender: {sender_label}, call path: {call_path:?}, description: {}", diff --git a/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto b/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto index a616228166e9..e2260aa83b68 100644 --- a/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto +++ b/rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto @@ -442,7 +442,7 @@ message TaskQueue { repeated ExecutionTask queue = 3; } -// Next ID: 69 +// Next ID: 70 message CanisterStateBits { reserved 1, 2, 3, 5, 6, 9, 10, 14, 16, 17, 24, 30, 35, 42, 47, 48, 49, 52, 53; diff --git a/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs b/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs index c775ca1468c0..885ec4ccce43 100644 --- a/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs +++ b/rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs @@ -667,7 +667,7 @@ pub struct TaskQueue { #[prost(message, repeated, tag = "3")] pub queue: ::prost::alloc::vec::Vec, } -/// Next ID: 69 +/// Next ID: 70 #[derive(Clone, PartialEq, ::prost::Message)] pub struct CanisterStateBits { #[prost(uint64, tag = "4")] diff --git a/rs/types/management_canister_types/src/lib.rs b/rs/types/management_canister_types/src/lib.rs index 983dca0ddb4e..097910e0abdf 100644 --- a/rs/types/management_canister_types/src/lib.rs +++ b/rs/types/management_canister_types/src/lib.rs @@ -1444,9 +1444,9 @@ impl TryFrom for StatusVisibility { pb_canister_state_bits::status_visibility::StatusVisibility::AllowedViewers(data) => { let principals = data .principals - .iter() + .into_iter() .map(|p| { - PrincipalId::try_from(p.raw.clone()).map_err(|e| { + PrincipalId::try_from(p.raw).map_err(|e| { ProxyDecodeError::ValueOutOfRange { typ: "PrincipalId", err: e.to_string(), From 784917782dd44258dc7548f1858dc351d0f5a614 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 13 Jul 2026 15:13:17 +0000 Subject: [PATCH 10/12] test: remove redundant canister_status query-call tests from execution_test.rs Commit 2c0413f9 ("simplify") deliberately removed the three canister_status_via_query_call_* tests and the canister_status_count helper from execution_test.rs, as they are fully covered by the table-driven test_status_visibility_of_canister_status in canister_status.rs (which exercises every visibility x sender combination over both the query and update call paths). The merge of master (6ab53deb43) accidentally resurrected these tests during conflict resolution, reintroducing the stale negative test that still asserted the pre-status-visibility error code (CanisterInvalidControllerOrSubnetAdmin instead of the new CanisterStatusAccessDenied). Re-remove them to restore the intended state. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/execution_test.rs | 117 ------------------ 1 file changed, 117 deletions(-) diff --git a/rs/execution_environment/tests/execution_test.rs b/rs/execution_environment/tests/execution_test.rs index 03f04d7f12fb..867c7157c585 100644 --- a/rs/execution_environment/tests/execution_test.rs +++ b/rs/execution_environment/tests/execution_test.rs @@ -2759,79 +2759,6 @@ fn get_canister_metadata() { ); } -fn canister_status_count(env: &StateMachine) -> u64 { - fetch_histogram_vec_stats( - env.metrics_registry(), - "execution_subnet_query_message_duration_seconds", - ) - .get(&labels(&[ - ("method_name", "query_ic00_canister_status"), - ("status", "success"), - ])) - .map_or(0, |stats| stats.count) -} - -#[test] -fn canister_status_via_query_call_by_controller_succeeds() { - let subnet_config = SubnetConfig::new(SubnetType::Application); - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - subnet_config, - HypervisorConfig::default(), - ))) - .build(); - let canister_id = create_universal_canister_with_cycles( - &env, - Some(CanisterSettingsArgsBuilder::new().build()), - INITIAL_CYCLES_BALANCE, - ); - - assert_eq!(canister_status_count(&env), 0); - - let result = env.query( - CanisterId::ic_00(), - "canister_status", - CanisterIdRecord::from(canister_id).encode(), - ); - - assert!(result.is_ok()); - assert_eq!(canister_status_count(&env), 1); -} - -#[test] -fn canister_status_via_query_call_by_subnet_admin_succeeds() { - let subnet_config = SubnetConfig::new(SubnetType::Application); - let subnet_admin = user_test_id(100); - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - subnet_config, - HypervisorConfig::default(), - ))) - .with_subnet_type(SubnetType::Application) - .with_cost_schedule(CanisterCyclesCostSchedule::Free) - .with_subnet_admins(vec![subnet_admin.get()]) - .build(); - let canister_id = create_universal_canister_with_cycles( - &env, - Some(CanisterSettingsArgsBuilder::new().build()), - INITIAL_CYCLES_BALANCE, - ); - // Get the first canister controller. - let controller = env.get_controllers(canister_id).unwrap()[0]; - - assert_eq!(canister_status_count(&env), 0); - assert_ne!(subnet_admin.get(), controller); - - let result = env.query_as( - subnet_admin.get(), - CanisterId::ic_00(), - "canister_status", - CanisterIdRecord::from(canister_id).encode(), - ); - assert!(result.is_ok()); - assert_eq!(canister_status_count(&env), 1); -} - fn list_canisters_count(env: &StateMachine) -> u64 { fetch_histogram_vec_stats( env.metrics_registry(), @@ -3033,50 +2960,6 @@ fn list_canisters_via_inter_canister_call_rejected_for_non_admin() { assert_eq!(env.subnet_message_instructions(), instructions_baseline); } -#[test] -fn canister_status_via_query_call_by_neither_controller_nor_subnet_admin_fails() { - let subnet_config = SubnetConfig::new(SubnetType::Application); - let subnet_admin = user_test_id(100); - let test_user = user_test_id(101); - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - subnet_config, - HypervisorConfig::default(), - ))) - .with_subnet_type(SubnetType::Application) - .with_cost_schedule(CanisterCyclesCostSchedule::Free) - .with_subnet_admins(vec![subnet_admin.get()]) - .build(); - let canister_id = create_universal_canister_with_cycles( - &env, - Some(CanisterSettingsArgsBuilder::new().build()), - INITIAL_CYCLES_BALANCE, - ); - // Get the first canister controller. - let controller = env.get_controllers(canister_id).unwrap()[0]; - - assert_eq!(canister_status_count(&env), 0); - assert_ne!(subnet_admin.get(), controller); - assert_ne!(test_user.get(), controller); - - let err = env - .query_as( - test_user.get(), - CanisterId::ic_00(), - "canister_status", - CanisterIdRecord::from(canister_id).encode(), - ) - .unwrap_err(); - assert_eq!( - err.code(), - ErrorCode::CanisterInvalidControllerOrSubnetAdmin - ); - assert!(err.description().contains(&format!( - "Only the controllers of the canister {canister_id} or subnet admins can perform certain actions" - ))); - assert_eq!(canister_status_count(&env), 0); -} - #[test] fn maximum_state_size() { let maximum_state_size = NumBytes::new(1 << 30); From ad38874bf66f859a7ad6a654dff9a06434c07389 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Wed, 15 Jul 2026 07:18:50 +0000 Subject: [PATCH 11/12] test: assert canister_status query success metric Re-add the query success-metric coverage lost when the canister_status_via_query_call_* tests were removed from execution_test.rs, folding it into the status_visibility matrix test: the successful canister_status query metric must increment exactly once for an allowed query call and never for a denied query or an update call. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/canister_status.rs | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs index 4dd62b809c0b..91bba28bae56 100644 --- a/rs/execution_environment/tests/canister_status.rs +++ b/rs/execution_environment/tests/canister_status.rs @@ -7,6 +7,7 @@ use ic_registry_subnet_type::SubnetType; use ic_state_machine_tests::{ ErrorCode, StateMachine, StateMachineBuilder, StateMachineConfig, UserError, }; +use ic_test_utilities_metrics::{fetch_histogram_vec_stats, labels}; use ic_test_utilities_types::ids::user_test_id; use ic_types::CanisterId; use ic_types_cycles::{CanisterCyclesCostSchedule, Cycles}; @@ -50,6 +51,20 @@ fn canister_status( } } +/// Returns the number of successful `canister_status` query calls recorded in +/// the subnet query message duration metrics. +fn canister_status_query_success_count(env: &StateMachine) -> u64 { + fetch_histogram_vec_stats( + env.metrics_registry(), + "execution_subnet_query_message_duration_seconds", + ) + .get(&labels(&[ + ("method_name", "query_ic00_canister_status"), + ("status", "success"), + ])) + .map_or(0, |stats| stats.count) +} + #[test] fn test_status_visibility_of_canister_status() { // Test combinations of status_visibility, sender, and call path for the @@ -136,6 +151,7 @@ fn test_status_visibility_of_canister_status() { assert_ne!(sender, canister_id.get()); for call_path in [CallPath::Update, CallPath::Query] { + let query_success_before = canister_status_query_success_count(&env); let result = canister_status(&env, call_path, sender, canister_id); if expected_allowed { let status = result @@ -167,6 +183,21 @@ fn test_status_visibility_of_canister_status() { err.description() ); } + + // The successful `canister_status` query metric must increment + // exactly once for an allowed query call, and must not increment for + // a denied query call or for an update call (which is not recorded + // as a query message). + let expected_query_success_delta = match (call_path, expected_allowed) { + (CallPath::Query, true) => 1, + _ => 0, + }; + assert_eq!( + canister_status_query_success_count(&env) - query_success_before, + expected_query_success_delta, + "unexpected successful query metric delta for status_visibility: \ + {status_visibility:?}, sender: {sender_label}, call path: {call_path:?}" + ); } } } From f092104d3269c4c3729bd9c7a50190e50492cbb6 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Wed, 15 Jul 2026 07:37:53 +0000 Subject: [PATCH 12/12] test: simplify canister_status test setup and clarify expectations - Create the test canister with zero cycles: the subnet uses a free cost schedule, so no cycles are needed (removes the INITIAL_CYCLES_BALANCE constant that was carried over from the original tests). - Fix the two `.expect` messages on the success path so each matches the layer it unwraps: the outer Result is the access-control gate, the inner Result is the reply/reject layer (unreachable via ingress/query). Co-Authored-By: Claude Opus 4.8 (1M context) --- rs/execution_environment/tests/canister_status.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/rs/execution_environment/tests/canister_status.rs b/rs/execution_environment/tests/canister_status.rs index 91bba28bae56..01b462871610 100644 --- a/rs/execution_environment/tests/canister_status.rs +++ b/rs/execution_environment/tests/canister_status.rs @@ -12,9 +12,6 @@ use ic_test_utilities_types::ids::user_test_id; use ic_types::CanisterId; use ic_types_cycles::{CanisterCyclesCostSchedule, Cycles}; -/// Initial cycles balance for the created canister, big enough for a regular test. -const INITIAL_CYCLES_BALANCE: Cycles = Cycles::new(100 * 1_000_000_000_000); - /// The call path used to invoke the `canister_status` management endpoint. #[derive(Clone, Copy, Debug)] enum CallPath { @@ -138,9 +135,10 @@ fn test_status_visibility_of_canister_status() { for (status_visibility, sender, sender_label, expected_allowed) in test_cases { let env = setup(subnet_admin); + // The subnet uses a free cost schedule, so no cycles are needed. let canister_id = env.create_canister_with_cycles( None, - INITIAL_CYCLES_BALANCE, + Cycles::zero(), Some( CanisterSettingsArgsBuilder::new() .with_controllers(vec![controller]) @@ -155,8 +153,11 @@ fn test_status_visibility_of_canister_status() { let result = canister_status(&env, call_path, sender, canister_id); if expected_allowed { let status = result - .expect("expected a successful response") - .expect("expected access to be granted"); + .expect("expected access to be granted") + // `canister_status` invoked via an ingress or query call never + // produces a reject response (a reject is only possible for an + // inter-canister call), so this layer is expected to be unreachable. + .expect("unexpected reject response"); assert_eq!( status.settings().status_visibility(), &status_visibility,