diff --git a/.claude/CHANGELOG.md b/.claude/CHANGELOG.md index a01b52e..5e3ec50 100644 --- a/.claude/CHANGELOG.md +++ b/.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 diff --git a/deploy/deployment/rbac/clusterrole.yaml b/deploy/deployment/rbac/clusterrole.yaml index a085846..2c6419c 100644 --- a/deploy/deployment/rbac/clusterrole.yaml +++ b/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"] diff --git a/docs/mkdocs.yml b/docs/mkdocs.yml index bdc9c86..a63fa5d 100644 --- a/docs/mkdocs.yml +++ b/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 diff --git a/docs/src/concepts/scheduled-machine.md b/docs/src/concepts/scheduled-machine.md index 7b3e2d6..663dc96 100644 --- a/docs/src/concepts/scheduled-machine.md +++ b/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 diff --git a/docs/src/security/crd-attack-surface.md b/docs/src/security/crd-attack-surface.md new file mode 100644 index 0000000..4834bd5 --- /dev/null +++ b/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`) | 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. diff --git a/docs/src/security/index.md b/docs/src/security/index.md index d784401..4ff7756 100644 --- a/docs/src/security/index.md +++ b/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 diff --git a/src/crd.rs b/src/crd.rs index 7689819..dd72a06 100644 --- a/src/crd.rs +++ b/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); diff --git a/src/reconcilers/helpers.rs b/src/reconcilers/helpers.rs index e677528..c4256ce 100644 --- a/src/reconcilers/helpers.rs +++ b/src/reconcilers/helpers.rs @@ -1029,12 +1029,37 @@ pub fn validate_api_group( /// 1. Creates the bootstrap resource from bootstrapSpec /// 2. Creates the infrastructure resource from infrastructureSpec /// 3. Creates the CAPI Machine referencing both +/// +/// # Invariants +/// +/// `namespace` MUST equal `resource.namespace()`. Every child resource +/// (bootstrap, infrastructure, Machine) carries an `ownerReference` back +/// to the `ScheduledMachine`. Kubernetes' garbage collector treats +/// owner-cascade as a same-namespace relationship: a child in +/// namespace `B` cannot be garbage-collected by an owner in namespace +/// `A` — the API server silently ignores the ownerRef. Mismatching +/// namespaces would therefore produce orphan resources on cascading +/// delete without any error surfaced at creation time. +/// +/// The `debug_assert_eq!` below makes the violation loud in test and +/// dev builds. Today every caller passes `resource.namespace()` so the +/// invariant holds; the assertion is a regression guard for future +/// refactors that might move bootstrap / infra creation to a different +/// namespace without updating the ownerRef contract. Filed as Phase 6b +/// of the 2026-04-25 security audit roadmap. #[allow(clippy::too_many_lines)] pub async fn add_machine_to_cluster( resource: &ScheduledMachine, client: &Client, namespace: &str, ) -> Result<(), ReconcilerError> { + debug_assert_eq!( + resource.namespace().as_deref(), + Some(namespace), + "ownerRef contract: bootstrap/infra/Machine MUST be created in the SM's own \ + namespace; cross-namespace ownerReferences are silently ignored by the GC" + ); + let name = resource.name_any(); let cluster_name = &resource.spec.cluster_name;