diff --git a/.claude/CHANGELOG.md b/.claude/CHANGELOG.md index 8977a2f..c739e24 100644 --- a/.claude/CHANGELOG.md +++ b/.claude/CHANGELOG.md @@ -9,6 +9,71 @@ The format is based on the regulated environment requirements: --- +## [2026-04-25 22:00] - Phase 3 of security audit: route Node→SM via canonical CAPI Machine + +**Author:** Erick Bourgeois + +### Changed +- `src/reconcilers/helpers.rs`: new + `node_to_scheduled_machines_via_machine(node, machines)` mapper. Routes + Node events to owning ScheduledMachines via the canonical CAPI Machine + ownership chain — `Machine.status.nodeRef.name` (written only by the + CAPI controller) plus the controller-stamped + `LABEL_SCHEDULED_MACHINE` label on the Machine. Both legs are + write-controlled by trusted controllers, not by the tenant. +- `src/reconcilers/helpers.rs`: `node_to_scheduled_machines` (the + pre-existing mapper that trusted `SM.status.nodeRef.name`) marked + `#[deprecated(since = "0.1.2")]` with a note pointing at the + replacement. Kept exported and tested for one release so any external + caller (none expected) gets a compile-time pointer. +- `src/reconcilers/mod.rs`: re-export the new symbol; gate the legacy + re-export with `#[allow(deprecated)]`. +- `src/main.rs`: wire a standalone `kube::runtime::reflector` for CAPI + Machines (filtered by `LABEL_SCHEDULED_MACHINE`). Spawn its watcher in + the background; pass the reader handle into the Node watch closure. + Switch the Node mapper from `node_to_scheduled_machines` to + `node_to_scheduled_machines_via_machine`. The + `Controller::watches_with` Machine watch keeps its own internal + reflector for the existing Machine→SM route — kube-rs does not expose + that internal store, so the secondary watch is intentional. Doubled + watch traffic is minor (same kind, same label selector). +- `src/reconcilers/helpers_tests.rs`: 9 new tests covering the + canonical-Machine mapper: + - happy-path single-Machine match returns owning SM + - Node with no owning Machine → NOT enqueued (closes spoof window) + - Machine for a different node → NOT enqueued (canonical wins over + spoofed SM status) + - unlabelled / namespace-less / no-nodeRef Machine → ignored + - Node without metadata.name → ignored + - whitespace-only label value → ignored (mirrors + `machine_to_scheduled_machine`'s defensive trim) + - multiple Machines for same Node → both owning SMs enqueued +- Existing `test_node_to_sms_*` tests for the deprecated mapper kept + with per-test `#[allow(deprecated)]` annotations so the legacy + surface stays covered until removal. + +### Why +Phase 3 of the 2026-04-25 security audit roadmap. The pre-existing +`node_to_scheduled_machines` mapper filtered SMs by +`status.nodeRef.name`, which is writable by anyone with `patch +scheduledmachines/status` on the resource. A tenant could write a +fast-changing node name (e.g. a busy worker that's frequently +updating) into status to amplify how many SM reconciliations they +caused per Node update — a CPU DoS amplifier. The drain target itself +was never affected (the reconciler always reads canonical +`Machine.status.nodeRef` via `get_node_from_machine`), so this was +verified as Low severity, but the routing change closes the +amplification regardless of how the reconciler evolves. + +### Impact +- [ ] Breaking change (deprecated symbol still exported) +- [ ] Requires cluster rollout (no schema or RBAC changes) +- [x] Config change only (in-binary; one extra Machine watch on the + same selector that already exists) +- [ ] Documentation only + +--- + ## [2026-04-25 18:00] - Phase 2 of security audit: finalizer cleanup force-remove on PDB stall **Author:** Erick Bourgeois diff --git a/src/main.rs b/src/main.rs index f6e537f..9326dfd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -28,15 +28,19 @@ use five_spot::health::{start_health_server, HealthState}; use five_spot::labels::LABEL_SCHEDULED_MACHINE; use five_spot::metrics::init_controller_info; use five_spot::reconcilers::{ - error_policy, machine_to_scheduled_machine, node_to_scheduled_machines, + error_policy, machine_to_scheduled_machine, node_to_scheduled_machines_via_machine, reconcile_scheduled_machine, Context, }; -use futures::StreamExt; +use futures::{StreamExt, TryStreamExt}; use k8s_openapi::api::core::v1::Node; use kube::{ api::{ApiResource, GroupVersionKind, ListParams}, core::DynamicObject, - runtime::{watcher::Config, Controller}, + runtime::{ + reflector, + watcher::{self, Config}, + Controller, WatchStreamExt, + }, Api, Client, }; use tracing::{error, info, warn}; @@ -276,10 +280,21 @@ async fn main() -> Result<()> { info!("Starting controller for ScheduledMachine resources"); // Secondary watches — event-driven reactivity without polling. - // 1. CAPI Machine (dynamic GVK) — filtered by the scheduled-machine label we - // already stamp on every Machine we create. Reverse-mapped via that label. - // 2. Kubernetes Node — name-matched against every SM's status.nodeRef.name - // using a snapshot of the controller's own primary-resource Store. + // + // 1. CAPI Machine (dynamic GVK), filtered by the scheduled-machine label + // we stamp on every Machine we create. Reverse-mapped via that label + // (`machine_to_scheduled_machine`). The `Controller::watches_with` + // call below maintains its own internal reflector for this watch. + // + // 2. Kubernetes Node, mapped to owning ScheduledMachines via the + // canonical CAPI Machine ownership chain + // (`node_to_scheduled_machines_via_machine`). This requires its own + // Machine reflector store, separate from #1's internal one — kube-rs + // `watches_with` does not expose its internal store. The doubled + // watch traffic is minor: same kind, same label selector. Routing + // via the canonical Machine instead of the tenant-writable + // `SM.status.nodeRef.name` closes the watch-amplification vector + // documented in Phase 3 of the 2026-04-25 security audit roadmap. let machine_ar = ApiResource::from_gvk_with_plural( &GroupVersionKind::gvk(CAPI_GROUP, CAPI_MACHINE_API_VERSION, "Machine"), CAPI_RESOURCE_MACHINES, @@ -287,8 +302,29 @@ async fn main() -> Result<()> { let machines_api: Api = Api::all_with(client.clone(), &machine_ar); let nodes_api: Api = Api::all(client.clone()); + // Standalone Machine reflector for the Node→SM mapper. We build its + // store here, spawn the watcher in the background, and clone the + // reader handle into the Node closure below. The store starts empty + // and fills as Machine watch events arrive — Node events that fire + // during the warmup window will see an empty store and enqueue + // nothing, which is correct (no Machine yet ⇒ no SM owns this Node). + let machine_writer: reflector::store::Writer = + reflector::store::Writer::new(machine_ar.clone()); + let machine_store = machine_writer.as_reader(); + let machine_reflector_machines_api = machines_api.clone(); + tokio::spawn(async move { + let stream = watcher::watcher( + machine_reflector_machines_api, + watcher::Config::default().labels(LABEL_SCHEDULED_MACHINE), + ) + .reflect(machine_writer) + .applied_objects(); + if let Err(e) = stream.try_for_each(|_| async { Ok(()) }).await { + error!(error = %e, "Machine reflector for Node→SM mapping terminated"); + } + }); + let controller = Controller::new(scheduled_machines, Config::default()); - let sm_store = controller.store(); controller .watches_with( @@ -298,8 +334,14 @@ async fn main() -> Result<()> { |machine: DynamicObject| machine_to_scheduled_machine(&machine), ) .watches(nodes_api, Config::default(), move |node: Node| { - let snapshot = sm_store.state(); - node_to_scheduled_machines(&node, snapshot.iter().map(std::convert::AsRef::as_ref)) + // Snapshot the standalone Machine reflector's store. Each + // entry is an Arc; deref through AsRef to feed + // the pure mapper. + let machines_snapshot = machine_store.state(); + node_to_scheduled_machines_via_machine( + &node, + machines_snapshot.iter().map(std::convert::AsRef::as_ref), + ) }) .shutdown_on_signal() .run(reconcile_scheduled_machine, error_policy, context) diff --git a/src/reconcilers/helpers.rs b/src/reconcilers/helpers.rs index 9b54f47..e677528 100644 --- a/src/reconcilers/helpers.rs +++ b/src/reconcilers/helpers.rs @@ -1600,10 +1600,32 @@ pub fn machine_to_scheduled_machine( /// Map a `Node` event to all `ScheduledMachine`s whose /// `status.nodeRef.name == node.metadata.name`. /// +/// **DEPRECATED — replaced by [`node_to_scheduled_machines_via_machine`].** +/// This function trusts the SM's own `status.nodeRef.name`, which is +/// writable by anyone with `patch scheduledmachines/status` on the +/// resource. A tenant could write a fast-changing node name into +/// status to amplify how many SM reconciliations they trigger from +/// unrelated Node events (CPU DoS amplifier — see Phase 3 of the +/// 2026-04-25 security audit roadmap). +/// +/// The replacement routes Node → Machine (canonical +/// `Machine.status.nodeRef.name` written by CAPI) → SM (via the +/// controller-stamped `LABEL_SCHEDULED_MACHINE` label on the Machine), +/// closing the spoof window. +/// +/// Kept exported with `#[deprecated]` for one release so external +/// callers (none expected) get a compile-time pointer at the +/// replacement. +/// /// Runs `O(N)` over the supplied SM iterator. Fine at small scale (tens to /// hundreds of SMs); if the cluster ever hosts thousands, swap in a reverse /// index keyed by the last-observed `nodeRef.name`. #[must_use] +#[deprecated( + since = "0.1.2", + note = "Trusts tenant-writable SM.status.nodeRef.name; use \ + node_to_scheduled_machines_via_machine for canonical-Machine routing." +)] pub fn node_to_scheduled_machines<'a, I>( node: &k8s_openapi::api::core::v1::Node, scheduled_machines: I, @@ -1634,6 +1656,95 @@ where .collect() } +/// Map a `Node` event to all `ScheduledMachine`s whose **owning CAPI Machine** +/// has `status.nodeRef.name == node.metadata.name`. +/// +/// This is the canonical-Machine replacement for +/// [`node_to_scheduled_machines`]. Routing via the CAPI Machine instead of the +/// SM's own status closes the watch-amplification vector where a tenant with +/// `patch scheduledmachines/status` could spoof `SM.status.nodeRef.name` to +/// pin controller CPU on unrelated Node updates: only the CAPI controller +/// writes `Machine.status.nodeRef`, and only the 5-Spot controller stamps +/// the [`crate::labels::LABEL_SCHEDULED_MACHINE`] label that walks back to +/// the SM. Both legs are write-controlled by trusted controllers, not by +/// the tenant. +/// +/// Algorithm (per Machine in the supplied iterator): +/// 1. Read `metadata.namespace` — required to build a namespaced +/// `ObjectRef`. Skip Machines without one. +/// 2. Read `status.nodeRef.name` from the Machine's dynamic `data` field +/// (`DynamicObject` because CAPI types are not statically modelled in +/// this codebase). Skip Machines whose `nodeRef` is absent or doesn't +/// match the node's name. +/// 3. Read `metadata.labels[LABEL_SCHEDULED_MACHINE]` to identify the +/// owning SM. Skip if the label is missing, empty, or whitespace-only +/// (mirrors the same defensive trim+empty check in +/// [`machine_to_scheduled_machine`]). +/// 4. Emit one [`kube::runtime::reflector::ObjectRef`] per match. +/// +/// Returns an empty `Vec` for the degenerate case where the Node itself +/// has no `metadata.name` — there is nothing meaningful to match against. +/// +/// Runs `O(N)` over the supplied Machine iterator. With one Machine per +/// SM and tens-to-hundreds of SMs per cluster this is comfortably cheap; +/// at thousands of SMs, build a reverse index keyed by node name. +/// +/// # Threat model context +/// Filed as Phase 3 of the 2026-04-25 security audit roadmap (severity: +/// Low — CPU amplification, not a drain pivot). The reconciler always +/// reads canonical `Machine.status.nodeRef` via `get_node_from_machine` +/// before draining, so the spoof never reached the drain target — but +/// it still amplified per-SM CPU cost. This routing change makes the +/// amplification impossible regardless of how the reconciler evolves. +#[must_use] +pub fn node_to_scheduled_machines_via_machine<'a, M>( + node: &k8s_openapi::api::core::v1::Node, + machines: M, +) -> Vec> +where + M: IntoIterator, +{ + let Some(node_name) = node.metadata.name.as_deref() else { + return Vec::new(); + }; + if node_name.is_empty() { + return Vec::new(); + } + + machines + .into_iter() + .filter_map(|machine| { + // Canonical nodeRef from Machine.status — written only by the + // CAPI controller, not by the tenant. + let machine_node_name = machine + .data + .get("status")? + .get("nodeRef")? + .get("name")? + .as_str()?; + if machine_node_name.is_empty() || machine_node_name != node_name { + return None; + } + + // Walk to the owning SM via the controller-stamped label — + // same primitive used by machine_to_scheduled_machine. + let labels = machine.metadata.labels.as_ref()?; + let raw_sm_name = labels.get(crate::labels::LABEL_SCHEDULED_MACHINE)?; + let sm_name = raw_sm_name.trim(); + if sm_name.is_empty() { + return None; + } + + let sm_namespace = machine.metadata.namespace.as_deref()?; + + Some( + kube::runtime::reflector::ObjectRef::::new(sm_name) + .within(sm_namespace), + ) + }) + .collect() +} + /// Cordon a Kubernetes Node (mark as unschedulable) /// /// # Errors diff --git a/src/reconcilers/helpers_tests.rs b/src/reconcilers/helpers_tests.rs index 2b39032..28c99b8 100644 --- a/src/reconcilers/helpers_tests.rs +++ b/src/reconcilers/helpers_tests.rs @@ -1750,6 +1750,12 @@ mod tests { // node_to_scheduled_machines — name-lookup mapper (Phase 4) // A Node event → ObjectRef for each SM whose // status.nodeRef.name matches node.metadata.name. + // + // DEPRECATED in 0.1.2 — superseded by + // node_to_scheduled_machines_via_machine (Phase 3 of the 2026-04-25 + // security audit). Tests retained to verify legacy behaviour for + // one release before the symbol is removed; the intentional + // deprecated-call sites are gated by allow(deprecated) below. // ======================================================================== fn sm_with_node_ref( @@ -1819,6 +1825,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_single_match_returns_one_ref() { let node = node_named("worker-1"); let sms = [ @@ -1832,6 +1839,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_multiple_matches_returns_all() { // Defensive: if two SMs claim the same Node (misconfiguration), reconcile BOTH // so the operator can surface the conflict via status. @@ -1848,6 +1856,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_no_match_returns_empty() { let node = node_named("worker-999"); let sms = [ @@ -1858,6 +1867,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_empty_list_returns_empty() { let node = node_named("worker-1"); let sms: Vec = Vec::new(); @@ -1865,6 +1875,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_sm_without_status_is_skipped() { let node = node_named("worker-1"); let sms = [sm_with_node_ref("sm-a", "default", None)]; @@ -1875,6 +1886,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_node_with_no_name_returns_empty() { // Defensive: Node without metadata.name should not match anything — especially // not SMs that also happen to have an empty/missing nodeRef.name. @@ -1884,6 +1896,7 @@ mod tests { } #[test] + #[allow(deprecated)] // exercises the legacy mapper kept for one release fn test_node_to_sms_empty_node_ref_name_is_not_matched() { // SM with an empty-string nodeRef.name must never match anything. let node = node_named("worker-1"); @@ -3701,4 +3714,187 @@ mod tests { "with_force_finalizer_on_timeout(false) must opt into strict-cleanup mode" ); } + + // ======================================================================== + // node_to_scheduled_machines_via_machine — Phase 3 hardening + // + // Routes Node events through the canonical CAPI Machine ownership chain + // (Node → Machine via Machine.status.nodeRef.name → SM via the + // controller-stamped LABEL_SCHEDULED_MACHINE label) instead of the + // tenant-writable SM.status.nodeRef.name. Closes the watch-amplification + // vector where a tenant with `patch scheduledmachines/status` could pin + // controller CPU by writing a fast-changing node name into status. + // ======================================================================== + + use crate::reconcilers::helpers::node_to_scheduled_machines_via_machine; + + /// Build a CAPI Machine `DynamicObject` carrying `metadata.labels`, + /// `metadata.namespace`, and an optional `status.nodeRef.name` so the + /// new mapper has something realistic to consume. + fn machine_with_label_and_node_ref( + sm_label: Option<&str>, + namespace: Option<&str>, + node_ref_name: Option<&str>, + ) -> kube::core::DynamicObject { + let mut meta = kube::core::ObjectMeta::default(); + if let Some(sm) = sm_label { + let mut m = std::collections::BTreeMap::new(); + m.insert( + crate::labels::LABEL_SCHEDULED_MACHINE.to_string(), + sm.to_string(), + ); + meta.labels = Some(m); + } + meta.namespace = namespace.map(std::string::ToString::to_string); + let data = match node_ref_name { + Some(n) => serde_json::json!({ "status": { "nodeRef": { "name": n } } }), + None => serde_json::json!({}), + }; + kube::core::DynamicObject { + types: None, + metadata: meta, + data, + } + } + + #[test] + fn test_via_machine_match_returns_owning_sm() { + let node = node_named("worker-1"); + let machines = [machine_with_label_and_node_ref( + Some("sm-a"), + Some("team-ns"), + Some("worker-1"), + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert_eq!(refs.len(), 1); + assert_eq!(refs[0].name, "sm-a"); + assert_eq!(refs[0].namespace.as_deref(), Some("team-ns")); + } + + #[test] + fn test_via_machine_node_with_no_owning_machine_returns_empty() { + // Node update for a host that no Machine claims — must NOT enqueue + // any SM, even if some SM has a stale or spoofed status.nodeRef. + // (No Machine vouches for the relationship, so nothing to enqueue.) + let node = node_named("victim-node"); + let machines: Vec = Vec::new(); + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!( + refs.is_empty(), + "no Machine vouches for victim-node ⇒ no SM enqueued" + ); + } + + #[test] + fn test_via_machine_machine_for_different_node_is_ignored() { + // The closing-the-status-spoof case: there's an SM with an owning + // Machine, but the Machine's canonical nodeRef points at a DIFFERENT + // node than the one in the event. Even if a malicious actor wrote + // SM.status.nodeRef = "victim-node" via patch status, this mapper + // looks at the canonical Machine.status.nodeRef and ignores the SM. + let node = node_named("victim-node"); + let machines = [machine_with_label_and_node_ref( + Some("sm-a"), + Some("ns"), + Some("worker-1"), // canonical: NOT victim-node + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!( + refs.is_empty(), + "Machine for a different node must not enqueue the SM on victim-node updates" + ); + } + + #[test] + fn test_via_machine_unlabelled_machine_is_ignored() { + // A Machine that matches the node but has no LABEL_SCHEDULED_MACHINE + // is not one of ours — ignore it (do NOT enqueue anything). + let node = node_named("worker-1"); + let machines = [machine_with_label_and_node_ref( + None, + Some("ns"), + Some("worker-1"), + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!( + refs.is_empty(), + "unlabelled Machine must not enqueue any SM" + ); + } + + #[test] + fn test_via_machine_machine_without_namespace_is_ignored() { + // Defensive: ObjectRef requires namespace. A Machine without one is + // malformed and must not produce a reconcile request. + let node = node_named("worker-1"); + let machines = [machine_with_label_and_node_ref( + Some("sm-a"), + None, + Some("worker-1"), + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!(refs.is_empty()); + } + + #[test] + fn test_via_machine_machine_without_node_ref_is_ignored() { + // Machine has no status.nodeRef yet (CAPI hasn't bound a Node). + // Cannot match a Node event — ignore. + let node = node_named("worker-1"); + let machines = [machine_with_label_and_node_ref( + Some("sm-a"), + Some("ns"), + None, + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!(refs.is_empty()); + } + + #[test] + fn test_via_machine_multiple_machines_for_same_node_returns_all_owning_sms() { + // Defensive: if two Machines claim the same Node (misconfiguration + // or replay race), reconcile BOTH owning SMs so the operator can + // surface the conflict via status. + let node = node_named("worker-1"); + let machines = [ + machine_with_label_and_node_ref(Some("sm-a"), Some("ns-1"), Some("worker-1")), + machine_with_label_and_node_ref(Some("sm-b"), Some("ns-2"), Some("worker-1")), + ]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert_eq!(refs.len(), 2, "both conflicting SMs must be enqueued"); + let names: std::collections::HashSet<_> = refs.iter().map(|r| r.name.as_str()).collect(); + assert!(names.contains("sm-a")); + assert!(names.contains("sm-b")); + } + + #[test] + fn test_via_machine_node_without_name_returns_empty() { + // A Node event with no metadata.name is degenerate — never enqueue. + use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; + let node = k8s_openapi::api::core::v1::Node { + metadata: ObjectMeta::default(), + ..Default::default() + }; + let machines = [machine_with_label_and_node_ref( + Some("sm-a"), + Some("ns"), + Some(""), + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!(refs.is_empty()); + } + + #[test] + fn test_via_machine_label_with_whitespace_is_rejected() { + // Mirrors machine_to_scheduled_machine's defensive trim+empty check + // — a whitespace-only label value is invalid and must not enqueue. + let node = node_named("worker-1"); + let machines = [machine_with_label_and_node_ref( + Some(" "), + Some("ns"), + Some("worker-1"), + )]; + let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); + assert!(refs.is_empty()); + } } diff --git a/src/reconcilers/mod.rs b/src/reconcilers/mod.rs index 2080a90..e1a1c3b 100644 --- a/src/reconcilers/mod.rs +++ b/src/reconcilers/mod.rs @@ -18,9 +18,11 @@ mod helpers; pub mod scheduled_machine; // Re-export main types and functions +#[allow(deprecated)] // re-export of legacy node_to_scheduled_machines for one release pub use helpers::{ error_policy, evaluate_schedule, machine_to_scheduled_machine, node_to_scheduled_machines, - parse_duration, reconcile_node_taints, should_process_resource, validate_cluster_name, - validate_kill_if_commands, NodeTaintReconcileOutcome, ReconcileNodeTaintsInput, + node_to_scheduled_machines_via_machine, parse_duration, reconcile_node_taints, + should_process_resource, validate_cluster_name, validate_kill_if_commands, + NodeTaintReconcileOutcome, ReconcileNodeTaintsInput, }; pub use scheduled_machine::{reconcile_scheduled_machine, Context, ReconcilerError};