Minor security fixes and added docs on CRD Attack surface

.claude/CHANGELOG.md

@@ -9,6 +9,78 @@ The format is based on the regulated environment requirements:
 
 ---
 
+## [2026-04-30 14:00] - Phases 5 + 6 of security audit: RBAC tightening + defence-in-depth docs
+
+**Author:** Erick Bourgeois
+
+### Changed (Phase 5 — RBAC tightening)
+- `deploy/deployment/rbac/clusterrole.yaml`: removed `update` verb from
+  the `nodes` rule. Audit of `src/reconcilers/` confirmed every Node
+  touchpoint uses `.get` or `.patch` only — cordon, reclaim-agent
+  label, applied taints, drain-related reads. Granting only the verbs
+  actually exercised reduces blast radius if the controller's
+  ServiceAccount token is compromised.
+- `deploy/deployment/rbac/clusterrole.yaml`: documented why the
+  `update` verb is **retained** on `coordination.k8s.io/leases` —
+  `kube_lease_manager` (the upstream leader-election library used in
+  `src/main.rs`) currently uses HTTP PUT semantics on lease renewal.
+  Dropping `update` would break leader election. Tracked alongside the
+  Phase 5 entry in the security audit roadmap; will be removed when
+  the upstream library switches to merge-patch.
+
+### Changed (Phase 6 — defence-in-depth + documentation)
+- `src/reconcilers/helpers.rs` (`add_machine_to_cluster`): added a
+  `debug_assert_eq!` at the top guarding the same-namespace ownerRef
+  contract (bootstrap/infra/Machine MUST be created in the SM's own
+  namespace; Kubernetes' GC silently ignores cross-namespace
+  ownerReferences). Rustdoc gained a new `# Invariants` section
+  explaining why the assertion exists and what would break without it.
+- `src/crd.rs` (`EmbeddedResource` rustdoc): expanded the doc comment
+  to document the pass-through trust boundary explicitly — the
+  reconciler validates the envelope (`apiVersion` group allowlist,
+  `kind`) but does NOT inspect provider-specific fields like
+  k0smotron's `cloudInit` or `RemoteMachine.address`. Operators in
+  multi-tenant clusters must layer a complementary policy.
+- `docs/src/concepts/scheduled-machine.md`: new "Security: Provider
+  payload pass-through" section before the "Related" links. Spells
+  out the trust boundary, names the high-risk fields per provider
+  (k0smotron cloudInit, RemoteMachine address), and gives three
+  concrete remediation options for multi-tenant operators (pre-stage
+  approved specs, layer CEL policy, scope `create` to trusted teams).
+- `docs/src/security/crd-attack-surface.md`: new page. Per-field
+  validation status and downstream sinks for every attacker-controllable
+  field on the CRD (spec + status), framed against the "namespace-scoped
+  tenant with `create scheduledmachines`" threat model. Marks
+  `bootstrapSpec.spec` and `infrastructureSpec.spec` as intentional
+  pass-throughs and points at the narrative section. Notes that the
+  status fields are no longer used for security-critical routing as of
+  Phase 3 of this audit.
+- `docs/src/security/index.md`: added the new page to the Documents
+  list.
+- `docs/mkdocs.yml`: registered the new page in the Security nav.
+
+### Why
+Phase 5 of the 2026-04-25 security audit roadmap closes the over-broad
+`update` verb on `nodes` (a long-standing finding from the cluster-role
+audit). Phase 6 surfaces two latent invariants — the same-namespace
+ownerRef contract and the provider-payload pass-through trust
+boundary — that today are correct by convention but were not
+documented anywhere a future refactor would notice. Both now have a
+loud failure mode: the `debug_assert!` for the ownerRef, and a
+discoverable docs page for the pass-through.
+
+### Impact
+- [ ] Breaking change
+- [x] Requires cluster rollout (the controller's ClusterRole no
+      longer requests `update` on `nodes` — existing deployments
+      should `kubectl apply` the new manifest. No code-side change is
+      required; the controller already only uses verbs it is now
+      granted.)
+- [ ] Config change only
+- [ ] Documentation only
+
+---
+
 ## [2026-04-26 00:30] - Phase 4 of security audit: reclaim-agent host-identity verification
 
 **Author:** Erick Bourgeois

deploy/deployment/rbac/clusterrole.yaml

@@ -66,10 +66,20 @@ rules:
     resources: ["secrets"]
     verbs: ["get", "list", "watch"]
 
-  # Nodes for drain operations
+  # Nodes for drain operations and reclaim-agent label / taint management.
+  #
+  # `update` was removed in Phase 5 of the 2026-04-25 security audit
+  # roadmap. Every Node touchpoint in the controller uses `.get` or
+  # `.patch` (cordon, reclaim-agent label, applied taints) — no path
+  # uses `Api::replace` or any other update-shaped call. Granting only
+  # the verbs that are actually exercised reduces blast radius if the
+  # controller's ServiceAccount token is ever compromised. If a future
+  # change reintroduces a need for full-object replacement, the lint
+  # rule in `deny.toml` will surface it before this rule has to be
+  # widened.
   - apiGroups: [""]
     resources: ["nodes"]
-    verbs: ["get", "list", "watch", "patch", "update"]
+    verbs: ["get", "list", "watch", "patch"]
 
   # Pods for eviction during drain
   - apiGroups: [""]
@@ -79,7 +89,14 @@ rules:
     resources: ["pods/eviction"]
     verbs: ["create"]
 
-  # Leases for leader election (no delete needed for HA)
+  # Leases for leader election (no delete needed for HA).
+  #
+  # `update` is retained: `kube_lease_manager` (the leader-election
+  # library used in src/main.rs) currently uses HTTP PUT semantics
+  # (Api::replace) on the Lease object during renewal. Removing
+  # `update` would break leader election. If the upstream library
+  # switches to merge-patch in a future release we can drop the verb;
+  # tracked alongside Phase 5 of the security audit roadmap.
   - apiGroups: ["coordination.k8s.io"]
     resources: ["leases"]
     verbs: ["get", "create", "update", "patch"]

docs/mkdocs.yml

@@ -230,6 +230,7 @@ nav:
       - Admission Validation: security/admission-validation.md
       - Threat Model: security/threat-model.md
       - VEX (Vulnerability Exploitability eXchange): security/vex.md
+      - CRD Attack Surface: security/crd-attack-surface.md
 
   - Developer Guide:
       - Development Setup: development/setup.md

docs/src/concepts/scheduled-machine.md

@@ -228,9 +228,61 @@ The controller:
 4. When schedule ends: gracefully shuts down and cleans up all created resources
 5. Maintains owner references for automatic garbage collection
 
+## Security: Provider payload pass-through
+
+`spec.bootstrapSpec.spec` and `spec.infrastructureSpec.spec` are
+**forwarded unchanged** to the named provider. 5-Spot validates the
+provider name (allowlist of `bootstrap.cluster.x-k8s.io`,
+`infrastructure.cluster.x-k8s.io`, and `k0smotron.io` groups) and the
+envelope shape (`apiVersion`, `kind`, presence of `spec`) but
+**does not inspect the inner spec**. That is by design — the controller
+is provider-agnostic, and inspecting every possible provider's schema
+would couple the controller to every provider's release cycle.
+
+The trust boundary is therefore the **provider**, not 5-Spot:
+
+- **k0smotron `K0sWorkerConfig.spec.cloudInit`** is interpreted as
+  cloud-init YAML and executed verbatim on the provisioned machine. A
+  user with `create scheduledmachines` can run any cloud-init payload
+  on the VMs they cause to be provisioned.
+- **k0smotron `RemoteMachine.spec.address`** is the SSH endpoint the
+  infrastructure controller connects to. A user can point this at any
+  reachable host or IP.
+- **CAPA / CAPM3 / other CAPI providers** carry their own attack
+  surfaces in their inline specs (image, command, env-with-secret,
+  etc.).
+
+### Implications for multi-tenant operators
+
+If different teams self-service `ScheduledMachine` CRs in their own
+namespaces, you MUST review provider documentation for fields that
+grant code execution or cross-tenant reachability **before** granting
+`create scheduledmachines` to a tenant. Options:
+
+1. **Pre-stage approved bootstrap / infrastructure resources** in a
+   platform-controlled namespace and expose only their `kind` + `name`
+   to tenants via your own wrapper CR. This eliminates the inline spec
+   entirely. (Out of scope for v1alpha1; tracked for a future
+   v1alpha2.)
+2. **Layer policy on top of 5-Spot**: a CEL `ValidatingAdmissionPolicy`
+   can reject specific provider fields (e.g. `cloudInit`,
+   `address` outside an approved CIDR) at admission time. The 5-Spot
+   `ValidatingAdmissionPolicy` validates structure but does not inspect
+   provider payloads — that's a complementary policy.
+3. **Trust-but-verify**: scope `create` to trusted teams and audit the
+   inline specs out of band.
+
+The 5-Spot `ValidatingAdmissionPolicy` and the runtime validators in
+`src/reconcilers/helpers.rs` together cover everything that 5-Spot
+itself can decide is malformed (cluster name length, label prefixes,
+schedule format, kill-if-commands bounds, …). They do not — and cannot
+— validate provider-specific cloud-init, SSH targets, or container
+images.
+
 ## Related
 
 - [API Reference](../reference/api.md) - Complete API documentation
 - [Machine Lifecycle](./machine-lifecycle.md) - Phase transitions
 - [Schedules](./schedules.md) - Schedule configuration details
 - [Emergency Reclaim](./emergency-reclaim.md) - `killIfCommands` and the process-match kill switch
+- [CRD Attack Surface](../security/crd-attack-surface.md) - per-field validation status and downstream sinks

docs/src/security/crd-attack-surface.md

@@ -0,0 +1,58 @@
+# CRD Attack Surface
+
+Per-field validation status and downstream sinks for every
+attacker-controllable field in the `ScheduledMachine` CRD. The
+intended threat model is a **namespace-scoped tenant** with
+`create scheduledmachines.5spot.finos.org` in their own namespace —
+the most realistic adversary in a multi-tenant cluster.
+
+> Source of truth for this table is `src/crd.rs` and
+> `deploy/admission/validatingadmissionpolicy.yaml`. If you add a new
+> field to the CRD, update this page in the same PR.
+
+## Spec fields
+
+| Field | Validation present | Flows to | Status |
+|---|---|---|---|
+| `spec.clusterName` (string, ≤ 63 chars, ASCII alphanumerics + `-._`) | ✓ VAP rule 1b/1c, ✓ `validate_cluster_name()` | `cluster.x-k8s.io/cluster-name` label on every owned Machine; Prometheus metric labels; log lines | **Bounded**. Phase 1 of the 2026-04-25 audit roadmap is open: add a per-namespace allowlist binding so tenant A cannot join machines to tenant B's cluster. |
+| `spec.schedule.enabled` (bool) | ✓ schema type | phase state machine (drives Active ↔ Inactive transitions) | Safe. Boolean enum, no injection surface. |
+| `spec.schedule.daysOfWeek[]` (string array of `mon`–`sun` or ranges) | ✓ VAP rule 6 (regex), ✓ `parse_day_ranges()` | schedule evaluator (parsed into a `HashSet<u8>`) | Safe. Bounded grammar; parsed into a typed set. |
+| `spec.schedule.hoursOfDay[]` (string array of `0`–`23` or ranges) | ✓ VAP rule 7 (regex), ✓ `parse_hour_ranges()` | schedule evaluator | Safe. Same shape as `daysOfWeek`. |
+| `spec.schedule.timezone` (string, ≤ 64 chars, IANA-shape) | ✓ CRD schema regex, ✓ `chrono_tz::Tz::parse()` at runtime | schedule time conversion | Safe. Bounded charset; runtime parse rejects invalid IANA identifiers. |
+| `spec.schedule.cron` (string, optional) | ✓ VAP rule 4 / 5 (XOR with day/hour windows), ✓ `parse_cron_expression()` | schedule evaluator | Safe. Mutually exclusive with day/hour windows. |
+| `spec.bootstrapSpec.apiVersion` (string) | ✓ VAP rules 8 / 9, ✓ `validate_api_group()` | dynamic resource creation (GVK construction) | Safe. Provider allowlist enforced. |
+| `spec.bootstrapSpec.kind` (string, non-empty) | ✓ VAP rule 10 | dynamic resource creation | Safe. Non-empty check; opaque to 5-Spot. |
+| **`spec.bootstrapSpec.spec` (arbitrary JSON)** | ✗ **none — by design** | provider-specific bootstrap controller (e.g. k0smotron `K0sWorkerConfig.cloudInit`) | **Pass-through.** Trust boundary is the provider, not 5-Spot. See [Provider payload pass-through](../concepts/scheduled-machine.md#security-provider-payload-pass-through). |
+| `spec.infrastructureSpec.apiVersion` (string) | ✓ VAP rules 11 / 12, ✓ `validate_api_group()` | dynamic resource creation | Safe. Provider allowlist. |
+| `spec.infrastructureSpec.kind` (string, non-empty) | ✓ VAP rule 13 | dynamic resource creation | Safe. |
+| **`spec.infrastructureSpec.spec` (arbitrary JSON)** | ✗ **none — by design** | provider-specific infrastructure controller (e.g. k0smotron `RemoteMachine.address` SSH endpoint) | **Pass-through.** Same trust-boundary argument as `bootstrapSpec.spec`. |
+| `spec.machineTemplate.labels` / `.annotations` (string→string maps) | ✓ runtime `validate_labels()` | metadata on the generated CAPI Machine | Safe. Reserved-prefix rejection (`kubernetes.io/`, `k8s.io/`, `cluster.x-k8s.io/`, `5spot.finos.org/`). |
+| `spec.killIfCommands` (string array, ≤ 100 items × ≤ 256 chars each) | ✓ VAP rules 1d / 1e, ✓ `validate_kill_if_commands()` | per-node `ConfigMap` (`reclaim.toml`) projected for the reclaim agent | Safe. Both list size and per-entry length bounded. |
+| `spec.nodeTaints[]` ({ key, value, effect }) | ✓ VAP rules 14–19, ✓ `validate_node_taints()` | Node `spec.taints` (one per declared entry) | Safe. RFC-1123 qualified names, reserved-prefix rejection (`5spot.finos.org/`, `kubernetes.io/`, `node.kubernetes.io/`, `node-role.kubernetes.io/`), unique on `(key, effect)`. |
+| `spec.killSwitch` (bool) | ✓ schema type | emergency-termination phase | Safe. Boolean. |
+| `spec.gracefulShutdownTimeout` (string, e.g. `"5m"`) | ✓ VAP rule 2, ✓ `parse_duration()` (≤ 24h) | drain timeout | Safe. Format + 24h cap. |
+| `spec.nodeDrainTimeout` (string) | ✓ VAP rule 3, ✓ `parse_duration()` | drain timeout | Safe. Same shape. |
+| `spec.priority` (u8) | ✓ schema type (0–255) | consistent-hash assignment for multi-instance distribution | Safe. Range-bounded. |
+
+## Status fields (writable by anyone with `patch scheduledmachines/status`)
+
+| Field | How the controller treats it | Spoof risk |
+|---|---|---|
+| `status.phase` | Read at the top of every reconcile to dispatch to a phase handler. The reconciler also writes it, so any tampered value is overwritten on the next pass. | Low — phase handlers are idempotent and re-derive their actions from `spec` + canonical CAPI state. |
+| `status.nodeRef.name` | **No longer used for routing** as of Phase 3 of the 2026-04-25 audit. The Node→SM watch mapper (`node_to_scheduled_machines_via_machine`) walks the canonical CAPI Machine ownership chain instead. The drain target is read from the canonical `Machine.status.nodeRef` via `get_node_from_machine`. | Low — both the routing and the drain target are now derived from controller-written state. The legacy `node_to_scheduled_machines` symbol is `#[deprecated]` and exported for one release only. |
+| `status.machineRef.name` | Used when the controller needs the Machine name for fetch / delete. Always recomputed from the canonical Machine on the same reconcile, so a spoofed value cannot redirect a delete to an unowned Machine. | Low. |
+| `status.bootstrapRef.name`, `status.infrastructureRef.name` | Same as `machineRef`. | Low. |
+| `status.appliedNodeTaints[]` | Owner-tracking record of which taints the controller has applied. Read on subsequent reconciles to compute "should remove" deltas. | A tenant who patches this could induce taint churn (controller removes taints it didn't apply, or fails to remove ones it did). Confined to the SM's own bound Node — does not pivot to other Nodes. |
+| `status.observedGeneration` | Read for debugging only. | None. |
+
+## Summary
+
+- 16 of 18 spec fields have defence-in-depth validation (VAP at admission + runtime check at reconcile).
+- The two unvalidated fields (`spec.bootstrapSpec.spec`, `spec.infrastructureSpec.spec`) are **intentional pass-throughs**; the trust boundary is the provider. See the [Provider payload pass-through](../concepts/scheduled-machine.md#security-provider-payload-pass-through) section for the rationale and recommended layered policy.
+- Status fields are no longer used for security-critical routing or drain decisions (Phase 3 of the 2026-04-25 audit). A tenant with `patch scheduledmachines/status` can induce small reconcile-loop noise but cannot pivot the controller to act on resources the tenant does not own.
+
+## Related
+
+- [Admission Validation](./admission-validation.md) — the CEL `ValidatingAdmissionPolicy` that enforces these rules at the API server.
+- [Threat Model](./threat-model.md) — STRIDE analysis of the controller's trust boundaries.
+- [ScheduledMachine — Security: Provider payload pass-through](../concepts/scheduled-machine.md#security-provider-payload-pass-through) — the inline-spec trust boundary in narrative form.

docs/src/security/index.md

@@ -22,6 +22,10 @@ A STRIDE-based analysis of the threats facing 5-Spot and the mitigations in plac
 
 How 5-Spot publishes a signed OpenVEX document with every release so downstream scanners can suppress CVEs we have already triaged as non-exploitable. Covers the `.vex/` authoring workflow, the Cosign attestation chain, and `grype --vex` / `trivy --vex` consumer usage.
 
+### [CRD Attack Surface](crd-attack-surface.md)
+
+Per-field validation status and downstream sinks for every attacker-controllable field on the `ScheduledMachine` CRD. Generated alongside the schema; useful for security reviews and for understanding which spec / status fields a namespace tenant can influence.
+
 ---
 
 ## Security Posture at a Glance

src/crd.rs

@@ -199,6 +199,33 @@ impl ScheduleSpec {
 ///
 /// Must contain at minimum `apiVersion` and `kind` fields. The controller
 /// will extract these to create the appropriate dynamic resource.
+///
+/// # Security — pass-through trust boundary
+///
+/// **The `spec` field of an `EmbeddedResource` is forwarded unchanged
+/// to the named provider.** The 5-Spot reconciler validates the
+/// envelope (`apiVersion` group allowlist, presence of `kind`, etc.)
+/// but **does not inspect the inner spec**. That is by design —
+/// 5-Spot is provider-agnostic and cannot ship a schema for every
+/// possible CAPI provider.
+///
+/// This means the trust boundary is the provider, not 5-Spot:
+///
+/// - `k0smotron.io/K0sWorkerConfig.spec.cloudInit` is interpreted as
+///   cloud-init YAML and executed verbatim on the provisioned VM.
+/// - `k0smotron.io/RemoteMachine.spec.address` is an SSH endpoint
+///   reached by the infrastructure controller.
+/// - Other providers carry their own code-execution / network-reach
+///   surfaces in their inline specs.
+///
+/// In multi-tenant clusters where different teams can `create
+/// scheduledmachines` in their own namespaces, operators **MUST**
+/// either pre-stage approved provider specs (out of scope for
+/// v1alpha1) or layer a complementary `ValidatingAdmissionPolicy`
+/// that inspects the provider payload — the 5-Spot VAP only
+/// validates structure. See `docs/src/concepts/scheduled-machine.md`
+/// (section "Security: Provider payload pass-through") for the full
+/// trade-off discussion.
 #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
 #[schemars(schema_with = "embedded_resource_schema")]
 pub struct EmbeddedResource(pub Value);