diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index faa95a82c57a..e23fb161827e 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -1,6 +1,7 @@ use crate::as_round_instructions; use crate::execution::common::{ - validate_controller, validate_controller_or_subnet_admin, validate_subnet_admin, + validate_controller, validate_controller_or_subnet_admin, validate_snapshot_visibility, + validate_subnet_admin, }; use crate::execution::install_code::OriginalContext; use crate::execution::{install::execute_install, upgrade::execute_upgrade}; @@ -229,10 +230,7 @@ impl CanisterManager { | Ok(Ic00Method::ClearChunkStore) | Ok(Ic00Method::TakeCanisterSnapshot) | Ok(Ic00Method::LoadCanisterSnapshot) - | Ok(Ic00Method::ListCanisterSnapshots) | Ok(Ic00Method::DeleteCanisterSnapshot) - | Ok(Ic00Method::ReadCanisterSnapshotMetadata) - | Ok(Ic00Method::ReadCanisterSnapshotData) | Ok(Ic00Method::UploadCanisterSnapshotMetadata) | Ok(Ic00Method::UploadCanisterSnapshotData) => { match effective_canister_id { @@ -258,6 +256,25 @@ impl CanisterManager { } }, + Ok(Ic00Method::ListCanisterSnapshots) + | Ok(Ic00Method::ReadCanisterSnapshotMetadata) + | Ok(Ic00Method::ReadCanisterSnapshotData) => { + match effective_canister_id { + Some(canister_id) => { + let canister_state = state.canister_state(&canister_id).ok_or_else(|| UserError::new( + ErrorCode::CanisterNotFound, + format!("Canister {canister_id} not found"), + ))?; + validate_snapshot_visibility(canister_state, &sender.get(), method_name)?; + Ok(()) + }, + None => Err(UserError::new( + ErrorCode::InvalidManagementPayload, + format!("Failed to decode payload for ic00 method: {method_name}"), + )), + } + }, + Ok(Ic00Method::FetchCanisterLogs) => Err(UserError::new( ErrorCode::CanisterRejectedMessage, format!( @@ -2493,14 +2510,14 @@ impl CanisterManager { /// Returns the canister snapshots list, or /// an error if it failed to retrieve the information. /// - /// Retrieving the canister snapshots list can only be initiated by the controllers. + /// Retrieving the canister snapshots list can only be initiated by a principal + /// allowed by the canister snapshot visibility settings. pub(crate) fn list_canister_snapshot( &self, sender: PrincipalId, canister: &CanisterState, - ) -> Result, CanisterManagerError> { - // Check sender is a controller. - validate_controller(canister, &sender)?; + ) -> Result, UserError> { + validate_snapshot_visibility(canister, &sender, "list_canister_snapshots")?; let mut responses = vec![]; for (snapshot_id, snapshot) in canister.canister_snapshots.list_snapshots() { @@ -2570,10 +2587,15 @@ impl CanisterManager { sender: PrincipalId, snapshot_id: SnapshotId, canister: &CanisterState, - ) -> Result { - // Check sender is a controller. - validate_controller(canister, &sender)?; - let snapshot = self.get_snapshot(canister, snapshot_id)?; + ) -> Result { + validate_snapshot_visibility( + canister, + &sender, + "read read_canister_snapshot_metadata snapshot metadata", + )?; + let snapshot = self + .get_snapshot(canister, snapshot_id) + .map_err(UserError::from)?; // A snapshot also contains the instruction counter as the last global // (because it is *appended* during WASM instrumentation). // We pop that last global (which is merely an implementation detail) @@ -2618,13 +2640,14 @@ impl CanisterManager { state: &ReplicatedState, subnet_size: usize, round_limits: &mut RoundLimits, - ) -> Result { - // Check sender is a controller. - validate_controller(canister, &sender)?; - let snapshot = self.get_snapshot(canister, snapshot_id)?; + ) -> Result { + validate_snapshot_visibility(canister, &sender, "read_canister_snapshot_data")?; + let snapshot = self + .get_snapshot(canister, snapshot_id) + .map_err(UserError::from)?; // Charge upfront for the baseline plus the maximum possible size of the returned slice or fail. - let num_response_bytes = get_response_size(&kind)?; + let num_response_bytes = get_response_size(&kind).map_err(UserError::from)?; let num_instructions = self .config .canister_snapshot_data_baseline_instructions @@ -2639,10 +2662,12 @@ impl CanisterManager { state.get_own_cost_schedule(), ) { - return Err(CanisterManagerError::CanisterSnapshotNotEnoughCycles(err)); + return Err(UserError::from( + CanisterManagerError::CanisterSnapshotNotEnoughCycles(err), + )); }; round_limits.instructions -= as_round_instructions(num_instructions); - let res = match kind { + let res: Result, CanisterManagerError> = match kind { CanisterSnapshotDataKind::StableMemory { offset, size } => { let stable_memory = snapshot.execution_snapshot().stable_memory.clone(); match CanisterSnapshot::get_memory_chunk(stable_memory, offset, size) { @@ -2665,19 +2690,21 @@ impl CanisterManager { } CanisterSnapshotDataKind::WasmChunk { hash } => { let Ok(hash) = ::try_from(hash.clone()) else { - return Err(CanisterManagerError::WasmChunkStoreError { + return Err(UserError::from(CanisterManagerError::WasmChunkStoreError { message: format!("Bytes {hash:02x?} are not a valid WasmChunkHash."), - }); + })); }; let Some(chunk) = snapshot.chunk_store().get_chunk_complete(&hash) else { - return Err(CanisterManagerError::WasmChunkStoreError { + return Err(UserError::from(CanisterManagerError::WasmChunkStoreError { message: format!("WasmChunkHash {hash:02x?} not found."), - }); + })); }; Ok(chunk) } }; + res.map(ReadCanisterSnapshotDataResponse::new) + .map_err(UserError::from) } /// Creates a new snapshot based on the provided metadata and returns the new snapshot ID. diff --git a/rs/execution_environment/src/execution/common.rs b/rs/execution_environment/src/execution/common.rs index 2a6d8e5dec1c..b31a46752e66 100644 --- a/rs/execution_environment/src/execution/common.rs +++ b/rs/execution_environment/src/execution/common.rs @@ -355,6 +355,22 @@ pub(crate) fn validate_controller( Ok(()) } +pub(crate) fn validate_snapshot_visibility( + canister: &CanisterState, + caller: &PrincipalId, + method_name: &str, +) -> Result<(), UserError> { + if !crate::canister_settings::VisibilitySettings::from(canister.snapshot_visibility()) + .has_access(caller, canister.controllers()) + { + return Err(UserError::new( + ErrorCode::CanisterRejectedMessage, + format!("Caller {caller} is not allowed to call {method_name}"), + )); + } + Ok(()) +} + pub(crate) fn validate_subnet_admin( subnet_admins: &BTreeSet, sender: &PrincipalId, diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index 978bab5aa2a6..c5c98120f971 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -2622,12 +2622,9 @@ impl ExecutionEnvironment { ) -> Result, UserError> { let canister = get_canister(args.get_canister_id(), state)?; - let result = self - .canister_manager + self.canister_manager .list_canister_snapshot(sender, canister) - .map_err(UserError::from)?; - - Ok(Encode!(&result).unwrap()) + .map(|result| Encode!(&result).unwrap()) } /// Deletes the specified canister snapshot if it exists. @@ -2691,18 +2688,18 @@ impl ExecutionEnvironment { Some(canister) => canister, }; - let result = match self.canister_manager.read_snapshot_data( - sender, - Arc::make_mut(&mut canister), - args.get_snapshot_id(), - args.kind, - state, - subnet_size, - round_limits, - ) { - Ok(result) => Ok(Encode!(&result).unwrap()), - Err(err) => Err(UserError::from(err)), - }; + let result = self + .canister_manager + .read_snapshot_data( + sender, + Arc::make_mut(&mut canister), + args.get_snapshot_id(), + args.kind, + state, + subnet_size, + round_limits, + ) + .map(|response| Encode!(&response).unwrap()); // Put canister back. state.put_canister_state(canister); @@ -2764,13 +2761,9 @@ impl ExecutionEnvironment { ) -> Result, UserError> { let canister = get_canister(args.get_canister_id(), state)?; let snapshot_id = args.get_snapshot_id(); - match self - .canister_manager + self.canister_manager .read_snapshot_metadata(sender, snapshot_id, canister) - { - Ok(response) => Ok(Encode!(&response).unwrap()), - Err(e) => Err(UserError::from(e)), - } + .map(|response| Encode!(&response).unwrap()) } fn create_snapshot_from_metadata( diff --git a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs index 6e5fa13b1cd1..5012b7a735fd 100644 --- a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs @@ -1155,13 +1155,9 @@ fn list_canister_snapshot_fails_invalid_controller() { if let RequestOrResponse::Response(res) = response { assert_eq!(res.originator, *receiver); res.response_payload.assert_contains_reject( - RejectCode::CanisterError, + RejectCode::CanisterReject, &format!( - "Only the controllers of the canister {} can control it.\n\ - Canister's controllers: {}\n\ - Sender's ID: {}", - canister_id, - test.user_id().get(), + "Caller {} is not allowed to call list_canister_snapshots", caller_canister.get(), ), ); @@ -2103,7 +2099,7 @@ fn read_canister_snapshot_metadata_fails_invalid_controller() { let error = test .subnet_message("read_canister_snapshot_metadata", args.encode()) .unwrap_err(); - assert_eq!(error.code(), ErrorCode::CanisterInvalidController); + assert_eq!(error.code(), ErrorCode::CanisterRejectedMessage); } fn read_canister_snapshot_data( @@ -2473,7 +2469,7 @@ fn read_canister_snapshot_data_fails_invalid_controller() { let error = test .subnet_message("read_canister_snapshot_data", args.encode()) .unwrap_err(); - assert_eq!(error.code(), ErrorCode::CanisterInvalidController); + assert_eq!(error.code(), ErrorCode::CanisterRejectedMessage); } #[test] diff --git a/rs/execution_environment/tests/canister_snapshots.rs b/rs/execution_environment/tests/canister_snapshots.rs index 9f50126862ba..f0f48beaaf31 100644 --- a/rs/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/tests/canister_snapshots.rs @@ -1067,3 +1067,206 @@ fn canister_snapshots_and_memory_allocation() { env.take_canister_snapshot(args).unwrap(); } } + +mod snapshot_visibility { + use ic_base_types::{CanisterId, PrincipalId}; + use ic_error_types::{ErrorCode, UserError}; + use ic_management_canister_types_private::{ + BoundedAllowedViewers, CanisterSettingsArgsBuilder, CanisterSnapshotDataKind, + CanisterSnapshotResponse, ListCanisterSnapshotArgs, ReadCanisterSnapshotDataArgs, + ReadCanisterSnapshotMetadataArgs, SnapshotVisibility, TakeCanisterSnapshotArgs, + }; + use ic_state_machine_tests::{StateMachine, StateMachineBuilder}; + use ic_universal_canister::UNIVERSAL_CANISTER_WASM; + + #[test] + fn test_visibility_of_canister_snapshots() { + let env = StateMachineBuilder::new().build(); + let controller = PrincipalId::new_user_test_id(1); + let not_a_controller = PrincipalId::new_user_test_id(2); + let allowed_viewer = PrincipalId::new_user_test_id(3); + let not_allowed_viewer = PrincipalId::new_user_test_id(4); + let allowed_viewers = BoundedAllowedViewers::new(vec![allowed_viewer]); + let test_cases = vec![ + TestCase { + snapshot_visibility: SnapshotVisibility::Public, + sender: controller, + expected_result: Ok(()), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Public, + sender: not_a_controller, + expected_result: Ok(()), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Public, + sender: allowed_viewer, + expected_result: Ok(()), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Public, + sender: not_allowed_viewer, + expected_result: Ok(()), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Controllers, + sender: controller, + expected_result: Ok(()), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Controllers, + sender: not_a_controller, + expected_result: Err(not_a_controller), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Controllers, + sender: allowed_viewer, + expected_result: Err(allowed_viewer), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::Controllers, + sender: not_allowed_viewer, + expected_result: Err(not_allowed_viewer), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::AllowedViewers(allowed_viewers.clone()), + sender: allowed_viewer, + expected_result: Ok(()), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::AllowedViewers(allowed_viewers.clone()), + sender: not_a_controller, + expected_result: Err(not_a_controller), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::AllowedViewers(allowed_viewers.clone()), + sender: not_allowed_viewer, + expected_result: Err(not_allowed_viewer), + }, + TestCase { + snapshot_visibility: SnapshotVisibility::AllowedViewers(allowed_viewers), + sender: controller, + expected_result: Ok(()), + }, + ]; + + for test_case in &test_cases { + let canister_id = env + .install_canister( + UNIVERSAL_CANISTER_WASM.to_vec(), + vec![], + Some( + CanisterSettingsArgsBuilder::new() + .with_snapshot_visibility(test_case.snapshot_visibility.clone()) + .with_controllers(vec![controller]) + .build(), + ), + ) + .unwrap(); + + test_case.check_list_canister_snapshot(canister_id, &[], &env); + + let snapshot = env + .take_canister_snapshot(TakeCanisterSnapshotArgs { + canister_id: canister_id.get(), + replace_snapshot: None, + uninstall_code: None, + sender_canister_version: None, + }) + .unwrap(); + let snapshot_id = snapshot.snapshot_id(); + + test_case.check_list_canister_snapshot(canister_id, &[snapshot], &env); + test_case.check_read_canister_snapshot_metadata(canister_id, snapshot_id, &env); + test_case.check_read_canister_snapshot_data(canister_id, snapshot_id, &env); + } + } + + #[derive(Debug)] + struct TestCase { + snapshot_visibility: SnapshotVisibility, + sender: PrincipalId, + expected_result: Result<(), PrincipalId>, + } + + impl TestCase { + fn check_list_canister_snapshot( + &self, + canister_id: CanisterId, + snapshots: &[CanisterSnapshotResponse], + env: &StateMachine, + ) { + let result = env.list_canister_snapshots_as( + ListCanisterSnapshotArgs::new(canister_id), + self.sender, + ); + match &self.expected_result { + Err(caller) => { + assert_eq!( + result.as_ref().err(), + Some(&expected_error(caller, "list_canister_snapshots")), + "{self:?}" + ); + } + Ok(()) => { + assert_eq!(result, Ok(snapshots.to_vec()), "{self:?}"); + } + } + } + + fn check_read_canister_snapshot_metadata( + &self, + canister_id: CanisterId, + snapshot_id: ic_base_types::SnapshotId, + env: &StateMachine, + ) { + let args = ReadCanisterSnapshotMetadataArgs::new(canister_id, snapshot_id); + let result = env.read_canister_snapshot_metadata_as(&args, self.sender); + match &self.expected_result { + Err(caller) => { + assert_eq!( + result.as_ref().err(), + Some(&expected_error(caller, "read_canister_snapshot_metadata")), + "{self:?}" + ); + } + Ok(()) => { + assert!(result.is_ok(), "{self:?}: {}", result.unwrap_err()); + } + } + } + + fn check_read_canister_snapshot_data( + &self, + canister_id: CanisterId, + snapshot_id: ic_base_types::SnapshotId, + env: &StateMachine, + ) { + let args = ReadCanisterSnapshotDataArgs::new( + canister_id, + snapshot_id, + CanisterSnapshotDataKind::StableMemory { offset: 0, size: 0 }, + ); + let result = env.read_canister_snapshot_data_as(&args, self.sender); + match &self.expected_result { + Err(caller) => { + assert_eq!( + result.as_ref().err(), + Some(&expected_error(caller, "read_canister_snapshot_data")), + "{self:?}" + ); + } + Ok(()) => { + assert!(result.is_ok(), "{self:?}: {}", result.unwrap_err()); + } + } + } + } + + fn expected_error(caller: &PrincipalId, method: &str) -> UserError { + UserError::new( + ErrorCode::CanisterRejectedMessage, + format!("Caller {caller} is not allowed to call {method}"), + ) + } +} diff --git a/rs/state_machine_tests/src/lib.rs b/rs/state_machine_tests/src/lib.rs index 8c7f0364af29..1899e98d2a09 100644 --- a/rs/state_machine_tests/src/lib.rs +++ b/rs/state_machine_tests/src/lib.rs @@ -60,10 +60,11 @@ use ic_logger::replica_logger::test_logger; use ic_logger::{ReplicaLogger, error}; use ic_management_canister_types_private::{ self as ic00, CanisterIdRecord, CanisterSnapshotDataKind, CanisterSnapshotDataOffset, - InstallCodeArgs, MasterPublicKeyId, Method, Payload, ReadCanisterSnapshotDataArgs, - ReadCanisterSnapshotDataResponse, ReadCanisterSnapshotMetadataArgs, - ReadCanisterSnapshotMetadataResponse, UploadCanisterSnapshotDataArgs, - UploadCanisterSnapshotMetadataArgs, UploadCanisterSnapshotMetadataResponse, + InstallCodeArgs, ListCanisterSnapshotArgs, ListCanisterSnapshotResponse, MasterPublicKeyId, + Method, Payload, ReadCanisterSnapshotDataArgs, ReadCanisterSnapshotDataResponse, + ReadCanisterSnapshotMetadataArgs, ReadCanisterSnapshotMetadataResponse, + UploadCanisterSnapshotDataArgs, UploadCanisterSnapshotMetadataArgs, + UploadCanisterSnapshotMetadataResponse, }; use ic_management_canister_types_private::{ CanisterHttpResponsePayload, CanisterInstallMode, CanisterSettingsArgs, @@ -3917,6 +3918,14 @@ impl StateMachine { args: &ReadCanisterSnapshotMetadataArgs, ) -> Result { let sender = self.get_controller(&args.get_canister_id()); + self.read_canister_snapshot_metadata_as(args, sender) + } + + pub fn read_canister_snapshot_metadata_as( + &self, + args: &ReadCanisterSnapshotMetadataArgs, + sender: PrincipalId, + ) -> Result { self.execute_ingress_as( sender, ic00::IC_00, @@ -3936,6 +3945,14 @@ impl StateMachine { args: &ReadCanisterSnapshotDataArgs, ) -> Result { let sender = self.get_controller(&args.get_canister_id()); + self.read_canister_snapshot_data_as(args, sender) + } + + pub fn read_canister_snapshot_data_as( + &self, + args: &ReadCanisterSnapshotDataArgs, + sender: PrincipalId, + ) -> Result { self.execute_ingress_as( sender, ic00::IC_00, @@ -3950,6 +3967,35 @@ impl StateMachine { })? } + /// List canister snapshots. + pub fn list_canister_snapshots( + &self, + args: ListCanisterSnapshotArgs, + ) -> Result { + let sender = self.get_controller(&args.get_canister_id()); + self.list_canister_snapshots_as(args, sender) + } + + /// List canister snapshots. + pub fn list_canister_snapshots_as( + &self, + args: ListCanisterSnapshotArgs, + sender: PrincipalId, + ) -> Result { + self.execute_ingress_as( + sender, + ic00::IC_00, + Method::ListCanisterSnapshots, + args.encode(), + ) + .map(|res| match res { + WasmResult::Reply(data) => ListCanisterSnapshotResponse::decode(&data), + WasmResult::Reject(reason) => { + panic!("list_canister_snapshots call rejected: {reason}") + } + })? + } + /// Helper to download the whole snapshot chunk store. pub fn get_snapshot_chunk_store( &self, diff --git a/rs/types/management_canister_types/src/lib.rs b/rs/types/management_canister_types/src/lib.rs index aa4ac96690f3..90390b1f38d6 100644 --- a/rs/types/management_canister_types/src/lib.rs +++ b/rs/types/management_canister_types/src/lib.rs @@ -4257,6 +4257,10 @@ impl ListCanisterSnapshotArgs { impl Payload<'_> for ListCanisterSnapshotArgs {} +pub type ListCanisterSnapshotResponse = Vec; + +impl Payload<'_> for ListCanisterSnapshotResponse {} + /// An enum representing the possible values of a global variable. #[derive(Copy, Clone, Debug, Deserialize, Serialize, EnumIter, CandidType)] pub enum Global {