fix(sns_aggregator): restrict reconfigure to controllers and clamp short intervals

CHANGELOG-Sns_Aggregator.md

@@ -14,6 +14,10 @@ The SNS Aggregator is released through proposals in the Network Nervous System.
 ### Removed
 ### Fixed
 ### Security
+- Only a controller can call the `reconfigure` method. The method exists in the
+  development build. Before, any principal could replace the configuration.
+- Raise an update interval below 100 ms to 100 ms. A shorter interval makes the
+  canister collect data continuously and burn cycles.
 
 ## [Proposal 137283](https://dashboard.internetcomputer.org/proposal/137283)
 ### Added

rs/sns_aggregator/src/auth.rs

@@ -0,0 +1,32 @@
+//! Caller checks for the methods that only a controller may call.
+
+#[cfg(test)]
+pub mod test_api;
+
+#[cfg(test)]
+use test_api::{caller_is_controller, deny};
+
+/// The message that a controller-only method traps with.
+pub const CALLER_IS_NOT_A_CONTROLLER: &str = "Only a controller of this canister may call this method.";
+
+/// Traps unless a controller of this canister made the current call.
+pub fn assert_caller_is_controller() {
+    if !caller_is_controller() {
+        deny(CALLER_IS_NOT_A_CONTROLLER);
+    }
+}
+
+/// Returns `true` if a controller of this canister made the current call.
+///
+/// A caller gets the list of controllers from
+/// `agent.read_state_canister_info(canister_id, "controllers")`.
+#[cfg(not(test))]
+fn caller_is_controller() -> bool {
+    ic_cdk::api::is_controller(&ic_cdk::api::msg_caller())
+}
+
+/// Stops the current call with an error message.
+#[cfg(not(test))]
+fn deny(message: &str) -> ! {
+    ic_cdk::api::trap(message)
+}

rs/sns_aggregator/src/auth/test_api.rs

@@ -0,0 +1,33 @@
+//! Test doubles for the system API that the caller checks use.
+//!
+//! The system API traps outside a canister, so a test provides the caller and
+//! the controllers itself.
+#![allow(clippy::panic)]
+
+use candid::Principal;
+use std::cell::RefCell;
+
+thread_local! {
+    /// The principal that the test presents as the caller.
+    static CALLER: RefCell<Principal> = RefCell::new(Principal::anonymous());
+    /// The principals that the test presents as the controllers of the canister.
+    static CONTROLLERS: RefCell<Vec<Principal>> = RefCell::new(Vec::new());
+}
+
+/// Sets the caller and the controllers for the current test.
+pub fn set_caller_and_controllers(caller: Principal, controllers: &[Principal]) {
+    CALLER.with(|value| *value.borrow_mut() = caller);
+    CONTROLLERS.with(|value| *value.borrow_mut() = controllers.to_vec());
+}
+
+/// Returns `true` if the caller of the current test is one of its controllers.
+pub(super) fn caller_is_controller() -> bool {
+    CALLER.with(|caller| CONTROLLERS.with(|controllers| controllers.borrow().contains(&caller.borrow())))
+}
+
+/// Stops the current test call with an error message.
+///
+/// A test has no trap, so the message arrives as a panic.
+pub(super) fn deny(message: &str) -> ! {
+    panic!("{message}")
+}

rs/sns_aggregator/src/lib.rs

@@ -5,13 +5,18 @@
 #![deny(clippy::expect_used)]
 #![deny(clippy::unwrap_used)]
 pub mod assets;
+#[cfg(any(test, feature = "reconfigurable"))]
+mod auth;
 mod conversion;
 mod state;
 mod types;
 mod upstream;
 
 mod fast_scheduler;
 
+#[cfg(test)]
+mod tests;
+
 use std::collections::VecDeque;
 use std::time::Duration;
 
@@ -218,15 +223,32 @@ fn post_upgrade(config: Option<Config>) {
 
 /// Method to allow reconfiguration without a WASM change.
 ///
-/// Note: This _could_ be exposed in production if limited to the controllers
-///  - Controllers can be obtained by the async call: `agent.read_state_canister_info(canister_id, "controllers")`
-#[cfg(feature = "reconfigurable")]
+/// Only a controller of this canister may call this method.  Every other caller
+/// gets a trap and the configuration stays as it is.
+#[cfg(any(test, feature = "reconfigurable"))]
 #[ic_cdk::update]
 #[candid_method(update)]
 fn reconfigure(config: Option<Config>) {
+    crate::auth::assert_caller_is_controller();
     setup(config);
 }
 
+/// Stores the given `Config`, then raises every interval that is too short.
+///
+/// A timer with a very short interval runs the data collection continuously and burns
+/// cycles.  A `Config` reaches this canister from a caller, or from the stable memory of
+/// an older version, so both need the same limit.
+///
+/// Returns `true` if it raised an interval.
+fn apply_config(config: Option<Config>) -> bool {
+    if let Some(config) = config {
+        STATE.with(|state| {
+            *state.stable.borrow().config.borrow_mut() = config;
+        });
+    }
+    STATE.with(|state| state.stable.borrow().config.borrow_mut().raise_short_intervals())
+}
+
 /// Code that needs to be run on `init` and after every upgrade.
 fn setup(config: Option<Config>) {
     // Note: This is intentionally highly visible in logs.
@@ -240,13 +262,15 @@ fn setup(config: Option<Config>) {
         ///////////////////////////\n"
     ));
     // Set configuration, if provided
-    if let Some(config) = config {
-        crate::state::log(format!("Setting config to: {config:?}"));
-        STATE.with(|state| {
-            *state.stable.borrow().config.borrow_mut() = config;
-        });
-    } else {
-        crate::state::log("Using existing config.".to_string());
+    match &config {
+        Some(config) => crate::state::log(format!("Setting config to: {config:?}")),
+        None => crate::state::log("Using existing config.".to_string()),
+    }
+    if apply_config(config) {
+        crate::state::log(format!(
+            "Raised the intervals to the minimum of {} ms.",
+            Config::MIN_INTERVAL_MS
+        ));
     }
     // Browsers complain if they don't get pretty pictures.  So do I.
     insert_favicon();

rs/sns_aggregator/src/state.rs

@@ -300,6 +300,23 @@ pub struct Config {
     /// The fast update interval, in milliseconds
     pub fast_interval_ms: u64,
 }
+impl Config {
+    /// The shortest interval that the canister uses between two data collection runs.
+    ///
+    /// Data collection makes several inter-canister calls, so a shorter interval burns
+    /// cycles without collecting more data.
+    pub const MIN_INTERVAL_MS: u64 = 100;
+
+    /// Raises every interval that is shorter than `MIN_INTERVAL_MS` to `MIN_INTERVAL_MS`.
+    ///
+    /// Returns `true` if it changed an interval.
+    pub fn raise_short_intervals(&mut self) -> bool {
+        let original = (self.update_interval_ms, self.fast_interval_ms);
+        self.update_interval_ms = self.update_interval_ms.max(Self::MIN_INTERVAL_MS);
+        self.fast_interval_ms = self.fast_interval_ms.max(Self::MIN_INTERVAL_MS);
+        original != (self.update_interval_ms, self.fast_interval_ms)
+    }
+}
 impl Default for Config {
     fn default() -> Self {
         Config {

rs/sns_aggregator/src/state/tests.rs

@@ -104,3 +104,34 @@ fn nat_candid_works() {
         assert_eq!(number, parsed);
     }
 }
+
+#[test]
+fn raise_short_intervals_raises_an_interval_that_is_too_short() {
+    let mut config = Config {
+        update_interval_ms: 0,
+        fast_interval_ms: 0,
+    };
+    assert!(config.raise_short_intervals(), "The intervals should have changed");
+    assert_eq!(config.update_interval_ms, Config::MIN_INTERVAL_MS);
+    assert_eq!(config.fast_interval_ms, Config::MIN_INTERVAL_MS);
+}
+
+#[test]
+fn raise_short_intervals_keeps_an_interval_that_is_long_enough() {
+    let mut config = Config {
+        update_interval_ms: Config::MIN_INTERVAL_MS,
+        fast_interval_ms: 1_000_000,
+    };
+    assert!(!config.raise_short_intervals(), "The intervals should not have changed");
+    assert_eq!(config.update_interval_ms, Config::MIN_INTERVAL_MS);
+    assert_eq!(config.fast_interval_ms, 1_000_000);
+}
+
+#[test]
+fn raise_short_intervals_keeps_the_default_config() {
+    let mut config = Config::default();
+    let original = config.clone();
+    assert!(!config.raise_short_intervals(), "The intervals should not have changed");
+    assert_eq!(config.update_interval_ms, original.update_interval_ms);
+    assert_eq!(config.fast_interval_ms, original.fast_interval_ms);
+}

rs/sns_aggregator/src/tests.rs

@@ -0,0 +1,93 @@
+//! Tests for the API methods of the aggregator canister.
+#![allow(clippy::unwrap_used)]
+
+use crate::auth::assert_caller_is_controller;
+use crate::auth::test_api::set_caller_and_controllers;
+use crate::state::Config;
+use candid::Principal;
+
+/// A principal that controls the canister in a test.
+fn controller() -> Principal {
+    Principal::from_text("qsgjb-riaaa-aaaaa-aaaga-cai").unwrap()
+}
+
+/// A principal that does not control the canister in a test.
+fn other_principal() -> Principal {
+    Principal::from_slice(&[1, 2, 3, 4])
+}
+
+#[test]
+fn assert_caller_is_controller_accepts_a_controller() {
+    set_caller_and_controllers(controller(), &[controller()]);
+    assert_caller_is_controller();
+}
+
+#[test]
+#[should_panic(expected = "Only a controller of this canister may call this method.")]
+fn assert_caller_is_controller_rejects_another_principal() {
+    set_caller_and_controllers(other_principal(), &[controller()]);
+    assert_caller_is_controller();
+}
+
+#[test]
+#[should_panic(expected = "Only a controller of this canister may call this method.")]
+fn assert_caller_is_controller_rejects_the_anonymous_principal() {
+    set_caller_and_controllers(Principal::anonymous(), &[controller()]);
+    assert_caller_is_controller();
+}
+
+/// The trap message proves that `reconfigure` stops at the caller check, before
+/// it reaches `setup` and the timers.
+#[test]
+#[should_panic(expected = "Only a controller of this canister may call this method.")]
+fn reconfigure_rejects_another_principal() {
+    set_caller_and_controllers(other_principal(), &[controller()]);
+    crate::reconfigure(Some(Config {
+        update_interval_ms: 0,
+        fast_interval_ms: 0,
+    }));
+}
+
+#[test]
+#[should_panic(expected = "Only a controller of this canister may call this method.")]
+fn reconfigure_rejects_the_anonymous_principal() {
+    set_caller_and_controllers(Principal::anonymous(), &[controller()]);
+    crate::reconfigure(None);
+}
+
+/// Returns the `Config` that the canister stores now.
+fn stored_config() -> Config {
+    crate::STATE.with(|state| state.stable.borrow().config.borrow().clone())
+}
+
+#[test]
+fn apply_config_raises_an_interval_that_the_caller_set_too_short() {
+    let raised = crate::apply_config(Some(Config {
+        update_interval_ms: 0,
+        fast_interval_ms: 0,
+    }));
+    assert!(raised, "The intervals should have been raised");
+    assert_eq!(stored_config().update_interval_ms, Config::MIN_INTERVAL_MS);
+    assert_eq!(stored_config().fast_interval_ms, Config::MIN_INTERVAL_MS);
+}
+
+#[test]
+fn apply_config_keeps_an_interval_that_is_long_enough() {
+    let raised = crate::apply_config(Some(Config {
+        update_interval_ms: 1_000,
+        fast_interval_ms: 2_000,
+    }));
+    assert!(!raised, "The intervals should have been kept");
+    assert_eq!(stored_config().update_interval_ms, 1_000);
+    assert_eq!(stored_config().fast_interval_ms, 2_000);
+}
+
+#[test]
+fn apply_config_raises_a_short_interval_that_is_already_stored() {
+    crate::STATE.with(|state| {
+        state.stable.borrow().config.borrow_mut().update_interval_ms = 0;
+    });
+    let raised = crate::apply_config(None);
+    assert!(raised, "The stored interval should have been raised");
+    assert_eq!(stored_config().update_interval_ms, Config::MIN_INTERVAL_MS);
+}