refactor(execution): centralize management canister access control in validate_sender

[mraszyk]

Summary

Centralizes most sender-authorization checks for management canister methods targeting an existing canister in validate_sender. It decides, based on the sender, the method, the targeted canister, and the subnet's type, cost schedule, and subnet admins, whether the call is allowed. The ingress filter, replicated execution (including install_code and install_chunked_code), and non-replicated queries use it for the checks covered by this refactor.

The following controller checks remain outside validate_sender:

• When load_canister_snapshot loads a snapshot from another canister, it still checks the source canister's controllers directly. Only authorization for the destination canister goes through validate_sender.
• canister_metadata still checks the targeted canister's controllers directly when deciding whether the caller can read a private custom section.

Access control for methods such as create_canister, list_canisters, and the provisional methods also remains separate, as do restrictions on which call paths can invoke a method.

This prepares a follow-up that grants subnet admins on free cloud engines access to methods that controllers can call. Policies routed through validate_sender can be changed centrally; extending access to the controller-protected operations listed above will require separate changes.

Behavior changes

Access control in replicated execution and non-replicated queries is unchanged. The ingress filter admits a few more sender/method pairs than before, because it now uses the same exceptions as replicated execution, which already had them: a canister may call canister_status, upload_chunk, stored_chunks, and clear_chunk_store on itself, and the governance canister may call uninstall_code. The filter is now consistent with execution instead of stricter than it, so it admits nothing that execution then rejects. Real ingress messages cannot reach these pairs, because validate_user_id requires a non-anonymous sender to be self-authenticating, and a canister id is not. Only StateMachine::execute_ingress_as and PocketIC can reach them, since they accept an arbitrary sender principal.

Error messages change as follows:

• The ingress filter rejects non-controllers calling controller-only methods with the standard CanisterInvalidController message instead of "Only controllers of canister X can call ic00 method Y" (same error code).
• The snapshot access denied message of read_canister_snapshot_metadata names the method correctly (previously "read read_canister_snapshot_metadata snapshot metadata").

Testing

New unit tests check the senders accepted by validate_sender for every management canister method, on all subnet types and cost schedules, and that each method consults the right visibility setting (status, snapshot, or log). New integration tests cover the read_canister_snapshot_metadata error message and the rejection of a replicated fetch_canister_logs call from a non-controller.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

The broad security-sensitive authorization refactor spans ingress, query, replicated execution, and DTS replay paths and warrants final human review.

0 open findings

What changed in this PR

Centralizes management-canister authorization in validate_sender, consistently applying subnet-aware policy across execution, query, and DTS paths.

Changes:

• Introduces exhaustive, method-specific sender validation.
• Threads network topology through canister-management operations.
• Separates log authorization from response generation and adds policy tests.

File	Description
rs/​execution_environment/​src/​execution/​common.rs	Defines centralized authorization policy and tests.
rs/​execution_environment/​src/​canister_manager.rs	Replaces distributed authorization checks.
rs/​execution_environment/​src/​execution_environment.rs	Supplies topology and validates relevant execution paths.
rs/​execution_environment/​src/​execution/​install_code.rs	Adds method-aware DTS authorization.
rs/​execution_environment/​src/​execution/​install.rs	Passes topology during installation validation.
rs/​execution_environment/​src/​execution/​upgrade.rs	Passes topology during upgrade validation.
rs/​execution_environment/​src/​query_handler/​subnet_query.rs	Applies centralized authorization to queries.
rs/​execution_environment/​src/​canister_logs.rs	Moves log authorization to callers.
rs/​execution_environment/​src/​canister_manager/​tests.rs	Updates tests for topology-based APIs.
rs/​execution_environment/​benches/​management_canister/​canister_logging.rs	Updates benchmarks for the authorization-free response helper.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Copilot]

🔵 Needs a closer look

The broad, security-sensitive authorization refactor warrants final human validation despite no specific defects found.

0 open findings

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Copilot]

🔵 Needs a closer look

The self-store install_chunked_code authorization still bypasses the centralized validator.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Unconditionally validate store canister callers

rs/​execution_environment/​src/​execution_environment.rs:4179

This still bypasses validate_sender when the caller is the store canister, so part of install_chunked_code authorization remains outside the centralized policy and contradicts the new contract in common.rs:457-461 that every targeted canister is validated. Preserve the existing self-store behavior by adding that exception to validate_sender, then invoke it unconditionally for the store canister here.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Copilot]

🔵 Needs a closer look

The broad security-sensitive authorization refactor warrants final human review despite no concrete defect found.

0 open findings

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to 4d65de9.

Security Overview

• 🔎 Scanned files: 12 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=928672aa-1c0d-49d3-94a0-87f80629c693&codeScanTypes=PrScan&tab=issues&detectionTypes=traditional,businessLogic,customRule,IAC,SECRETS,CICD,EOL,uncategorized

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/execution_environment/src/canister_logs.rs      Add network topology validation for fetch_canister_logs and related functions; pass NetworkTopology and SubnetId through to fetch_canister_logs and related bench/testing helpers; remove anonymous sender usage in bench paths ► rs/execution_environment/src/canister_manager.rs      Replace direct controller/subnet admin checks with validate_sender_on_subnet using Ic00Method mappings; extend method validation across multiple ic00 methods; propagate network_topology and own_subnet_id through various management paths ► rs/execution_environment/src/canister_manager/types.rs      Introduce Method as Ic00Method field in InstallCodeContext to track management method type (InstallCode vs InstallChunkedCode)
Refactor	► rs/execution_environment/src/execution/common.rs      Import and utilize Ic00Method aliasing for validation; adjust related imports and usages accordingly
Enhancement	► rs/execution_environment/src/canister_manager/tests.rs      Update tests to pass Arc instead of subnet_admins and adapt call sites to new topology parameter passing; extend test fixtures to include network_topology
Enhancement	► rs/execution_environment/src/canister_manager/types.rs      Update InstallCodeContext to include method field; set default methods for contexts used in tests and context creation

[eichhorl]

🤖 validate_input is replayed here in a later round, so the replayed step is authorized against that round's network_topology, while the canister it validates is the pinned clean_canister. OriginalContext is documented as "context variables that remain the same throughout the entire deterministic time slicing execution", and the topology is now the one input to the verdict that does not.

Today this cannot change the outcome — InstallCode and InstallChunkedCode both land on validate_controller, which ignores subnet type, cost schedule and subnet admins — but it becomes observable with the follow-up this PR prepares: if the subnet admins change between slices, the replay can reject an install that was authorized when it started and abort it mid-way.

Pinning the topology in OriginalContext fixes it and removes the parameter, so install.rs and upgrade.rs need no change in this PR at all:

// install_code.rs
pub(crate) struct OriginalContext {
    /// The management canister method (`InstallCode` or `InstallChunkedCode`).
    pub method: Ic00Method,
    /// The network topology of the round in which the execution started. The
    /// sender is validated against it in every round the execution spans, so
    /// that replaying `InstallCodeStep::ValidateInput` in a later round cannot
    /// reach a different verdict than the original validation did.
    pub network_topology: Arc<NetworkTopology>,
    ...
}

pub fn validate_input(&mut self, original: &OriginalContext) -> Result<(), CanisterManagerError> {
    ...
    validate_sender_on_subnet(
        &original.sender,
        original.method,
        &self.canister,
        &original.network_topology,
        config.own_subnet_id,
    )?;

// replay_step
InstallCodeStep::ValidateInput => self.validate_input(original),

// canister_manager.rs, install_code_dts
let original: OriginalContext = OriginalContext {
    method: context.method,
    network_topology: Arc::clone(&network_topology),
    ...

(round.network_topology stays in use for HandleWasmExecution, so nothing becomes dead.)

[mraszyk]

Good catch, done in 4d65de9. OriginalContext now stores network_topology: Arc<NetworkTopology>, set in install_code_dts. validate_input validates against it, and replaying InstallCodeStep::ValidateInput no longer takes the round's topology. install.rs and upgrade.rs are back to matching master.

[eichhorl]

🤖 This clones the admin set on every validated call on a subnet that can have subnet admins, although only the error path needs an owned set. Since validate_status_visibility and validate_controller_or_subnet_admin are both private to this module now, they can take it by reference and clone only when constructing the error:

fn validate_status_visibility(
    canister: &CanisterState,
    subnet_admins: Option<&BTreeSet<PrincipalId>>,
    caller: &PrincipalId,
) -> Result<(), CanisterManagerError> {
    // Subnet admins always retain access to the canister status.
    if let Some(subnet_admins) = subnet_admins
        && subnet_admins.contains(caller)
    { ... }

fn validate_controller_or_subnet_admin(
    canister: &CanisterState,
    subnet_admins: Option<&BTreeSet<PrincipalId>>,
    sender: &PrincipalId,
) -> Result<(), CanisterManagerError> {
    ...
                    subnet_admins_expected: subnet_admins.clone(),

which turns this line into the suggestion below.

Suggested change

	let subnet_admins =
	can_have_subnet_admins(subnet_type, cost_schedule).then(|| subnet_admins.clone());
	let subnet_admins = can_have_subnet_admins(subnet_type, cost_schedule).then_some(subnet_admins);

[mraszyk]

Done in 4d65de9. validate_status_visibility and validate_controller_or_subnet_admin now take Option<&BTreeSet<PrincipalId>> and only clone the set when building the error. The line is now .then_some(subnet_admins).

[eichhorl]

🤖 method.to_string() allocates on the happy path even though only the error needs the name. Letting validate_snapshot_visibility take the method defers the allocation to the error branch — and, more to the point, makes the bug this PR fixes ("read read_canister_snapshot_metadata snapshot metadata") unconstructible, since there is no longer a free-form string to get wrong:

fn validate_snapshot_visibility(
    canister: &CanisterState,
    caller: &PrincipalId,
    method: Ic00Method,
) -> Result<(), CanisterManagerError> {
    if !crate::canister_settings::VisibilitySettings::from(canister.snapshot_visibility())
        .has_access(caller, canister.controllers())
    {
        return Err(CanisterManagerError::CanisterSnapshotAccessDenied {
            caller: *caller,
            method_name: method.to_string(),
        });
    }
    Ok(())
}

Suggested change

	validate_snapshot_visibility(canister, sender, &method.to_string())
	validate_snapshot_visibility(canister, sender, method)

[mraszyk]

Done in 4d65de9. validate_snapshot_visibility takes Ic00Method and formats the name only in the error branch.

[eichhorl]

🤖 The two arms differ only in the three values they pass, so the whole call can be written once, which also puts the safe defaults right next to the lookup they belong to. BTreeSet::new() is a const fn, so the empty set can be a static rather than a fresh allocation per fallback call.

Suggested change

	match network_topology.subnets().get(&subnet_id) {
	Some(subnet_topology) => validate_sender(
	sender,
	method,
	canister,
	subnet_topology.subnet_type,
	subnet_topology.cost_schedule,
	&subnet_topology.subnet_admins,
	),
	None => validate_sender(
	sender,
	method,
	canister,
	SubnetType::Application,
	CanisterCyclesCostSchedule::Normal,
	&BTreeSet::new(),
	),
	}
	static NO_SUBNET_ADMINS: BTreeSet<PrincipalId> = BTreeSet::new();
	let (subnet_type, cost_schedule, subnet_admins) =
	match network_topology.subnets().get(&subnet_id) {
	Some(subnet_topology) => (
	subnet_topology.subnet_type,
	subnet_topology.cost_schedule,
	&subnet_topology.subnet_admins,
	),
	None => (
	SubnetType::Application,
	CanisterCyclesCostSchedule::Normal,
	&NO_SUBNET_ADMINS,
	),
	};
	validate_sender(
	sender,
	method,
	canister,
	subnet_type,
	cost_schedule,
	subnet_admins,
	)

While you are here: it would also help to state the precondition on subnet_id, since callers now source it from two places (CanisterMgrConfig::own_subnet_id in the CanisterManager methods, SystemMetadata::own_subnet_id in the ingress filter, install_chunked_code and fetch_canister_logs), whereas the base always went through ReplicatedState::get_own_subnet_admins:

/// `subnet_id` must be the id of this subnet, i.e., it must agree with the
/// `own_subnet_id` of the state that `network_topology` was taken from. Both
/// `CanisterMgrConfig::own_subnet_id` and `SystemMetadata::own_subnet_id` hold
/// it, and callers pass whichever of the two is at hand.

[mraszyk]

Done in 4d65de9: a single validate_sender call with a static NO_SUBNET_ADMINS fallback. I also added the doc note on the subnet_id precondition.

[eichhorl]

🤖 The PR makes validate_controller, validate_snapshot_visibility, validate_status_visibility and validate_controller_or_subnet_admin private, but validate_sender keeps pub(crate) even though its only non-test caller is validate_sender_on_subnet right below it (the unit tests see it through use super::*). Making it private too enforces the invariant the PR establishes: validate_sender_on_subnet is the only place that derives the subnet type, cost schedule and admins consistently with ReplicatedState::get_own_subnet_admins, and nobody can bypass it with a hand-rolled triple.

Suggested change

	pub(crate) fn validate_sender(
	fn validate_sender(

[mraszyk]

Done in 4d65de9.

[eichhorl]

🤖 Two things in this paragraph do not match the code below it, and since this doc block is now the written specification of ic00 access control it is worth getting exactly right:

• canister_metadata's access control does depend on the targeted canister (its controllers plus the visibility of the requested custom section), so it is not an example of "does not depend on the targeted canister".
• canister_info and deposit_cycles are accepted for every sender rather than rejected, which the paragraph does not mention.

It would also help to name the owners of the orthogonal rules, because a reader of this function will look for them: whether a method may be called by ingress at all is decided by CanisterManager::should_accept_ingress_message (Ic00MethodPermissions::verify returns Ok(()) for every ingress message, so it is not the place to look), and the source-canister controller check of load_canister_snapshot stays outside.

Suggested change

	/// Methods that do not target a canister or whose access control does not
	/// depend on the targeted canister (e.g., `create_canister`, `list_canisters`,
	/// or `canister_metadata`) are rejected: their access control is implemented
	/// separately and calling this function for them is a bug.
	/// This function decides only *who* may call a method, not through which call
	/// path: whether a method may be called by an ingress message at all is
	/// decided by `CanisterManager::should_accept_ingress_message` (backed by the
	/// `reject_unexpected_ingress` arms of
	/// `ExecutionEnvironment::execute_subnet_message`), and which subnet an
	/// inter-canister caller may be on by
	/// `crate::ic00_permissions::Ic00MethodPermissions`.
	///
	/// Methods whose access control does not depend on the targeted canister are
	/// rejected: calling this function for them is a bug. Those are the methods
	/// that are not gated on the targeted canister at all (e.g., `create_canister`,
	/// `list_canisters`, or the provisional methods, which are gated on the subnet
	/// admins and the provisional whitelist, respectively) and `canister_metadata`,
	/// whose access control also depends on the visibility of the requested custom
	/// section. In contrast, `canister_info` and `deposit_cycles` are accepted for
	/// every sender: they target a canister but are subject to no access control.
	///
	/// A few checks on a method that this function does validate are implemented
	/// elsewhere, namely the controller check on the canister owning the snapshot
	/// loaded by `load_canister_snapshot`.

[mraszyk]

Applied your wording in 4d65de9, thanks.

[eichhorl]

🤖 Test gap opened by moving this check out of the shared helper. Before this PR the visibility check lived inside fetch_canister_logs_response, which both paths called, so the single negative test (test_log_visibility_of_fetch_canister_logs, which goes through env.query_as) guarded both by construction. Now the check sits at two call sites and only the non-replicated one is covered: every replicated-path test is a positive case, so deleting these six lines makes no test fail, even though they are the only thing stopping any canister on the subnet from reading any other canister's private logs via an inter-canister call (and the feature is Enabled by default).

Worth adding a negative case next to test_fetch_canister_logs_via_inter_canister_update_call_enabled: same setup, but with canister_b controlled by the user rather than by canister_a, asserting

let expected_message = format!(
    "Caller {} is not allowed to access canister logs",
    canister_a.get()
);
assert!(reject_message.contains(&expected_message), ...);

[mraszyk]

Added test_fetch_canister_logs_via_inter_canister_update_call_enabled_not_allowed in 4d65de9. It uses the same setup but with canister_b controlled by the user, and asserts the "Caller {canister_a} is not allowed to access canister logs" rejection.

[eichhorl]

🤖 The message fix this PR documents is asserted by no test. test_visibility_of_canister_snapshots does assert the full string, but it submits ingress messages, so the rejection comes from the ingress filter — which always passed the correct method_name — and the garbled "read read_canister_snapshot_metadata snapshot metadata" was only ever observable on this path. The only test that reaches it, read_canister_snapshot_metadata_fails_invalid_controller, checks error.code() alone, which was already true before.

One assertion there pins the fix:

assert!(
    error.description().contains(&format!(
        "Caller {} is not allowed to call read_canister_snapshot_metadata",
        user_test_id(42).get()
    )),
    "{error}"
);

[mraszyk]

Added the assertion to read_canister_snapshot_metadata_fails_invalid_controller in 4d65de9.

[eichhorl]

🤖 Nice test. Two things would make it harder to outgrow:

Close the table over Ic00Method::iter(). The match in validate_sender is exhaustive, so a new method forces a decision there — but if that decision is an Ok(()) or a visibility check, nothing makes anyone extend this table. Deriving the complement and requiring it to be rejected for every sender gets that for free, and also covers the CallerNotAuthorized arm and the canister_info/deposit_cycles arm that are untested today:

let senders = [controller, subnet_admin, stranger, canister_id.get(), GOVERNANCE_CANISTER_ID.get()];
let mut cases: Vec<(Vec<Ic00Method>, Vec<PrincipalId>, Vec<PrincipalId>)> = vec![
    // ... as today, but listing `controller` explicitly in the accepted sets ...
    // These methods are not subject to any access control.
    (
        vec![Ic00Method::CanisterInfo, Ic00Method::DepositCycles],
        senders.to_vec(),
        senders.to_vec(),
    ),
];
// `validate_sender` does not define the access control of any other method, so
// it must reject every sender for them. `rename_canister` is among them: none
// of the senders here is the migration canister (see the dedicated test below).
let validated: Vec<Ic00Method> = cases
    .iter()
    .flat_map(|(methods, _, _)| methods.iter().copied())
    .collect();
cases.push((
    Ic00Method::iter()
        .filter(|method| !validated.contains(method))
        .collect(),
    vec![],
    vec![],
));

(with is_accepted becoming plain accepted.contains(&sender)).

Cover the visibility routing. The canister here uses the default visibility for all three settings, and all three default to Controllers — so a method wired to the wrong visibility check (say read_canister_snapshot_data consulting the status visibility) passes this test unchanged. That is the main wiring risk of centralizing these checks, and a second test catches it cheaply: build three canisters, set status_visibility / snapshot_visibility / log_visibility to Public one at a time, and assert that a stranger is accepted for exactly the methods governed by that setting and for no other method.

[mraszyk]

Done in 4d65de9, both parts:

• The table test now runs over Ic00Method::iter(). The controller is listed explicitly in the accepted sets, canister_info and deposit_cycles are accepted for every sender, and every method not listed must be rejected for all senders.
• New test validate_sender_consults_the_right_visibility_setting: it sets status, snapshot and log visibility to Public one at a time. A stranger must get in for exactly the methods governed by that setting, plus canister_info and deposit_cycles.

[eichhorl]

🤖 Worth a line in the PR description: merging these arms makes the filter admit a few sender/method pairs it rejected before, because validate_sender carries exceptions the old filter arms did not have — the canister itself for canister_status, upload_chunk, stored_chunks and clear_chunk_store, and the governance canister for uninstall_code.

It is not a security change in any direction: all five exceptions already existed in replicated execution before this PR, so the filter was merely stricter than execution and is now consistent with it, and nothing can pass the filter that execution then rejects. It is also unreachable on a real ingress message, since validate_user_id requires a non-anonymous sender to be self-authenticating and a canister id is an opaque principal. It is reachable from StateMachine::execute_ingress_as and PocketIC, which accept an arbitrary sender principal.

Only flagging it because "Access control is unchanged" with exactly two listed error-message changes is the paragraph a security reviewer reads, and this is the one spot where the equivalence is not literal.

[mraszyk]

Agreed, I updated the PR description. The "Behavior changes" section now explains that the ingress filter uses the same exceptions as replicated execution. It also notes that real ingress messages can't reach these pairs, while StateMachine::execute_ingress_as and PocketIC can.

[Copilot]

🔵 Needs a closer look

The broad, security-sensitive authorization refactor warrants final human verification despite comprehensive tests and no identified defects.

0 open findings

🧠 Review effort: Balanced