Skip to content

Commit 5a4af44

Browse files
ebourgeoisebourgeois
committed
Minor security fixes and added docs on CRD Attack surface
Signed-off-by: Erick Bourgeois <erick@jeb.ca>
1 parent c6f8eb3 commit 5a4af44

8 files changed

Lines changed: 259 additions & 3 deletions

File tree

‎.claude/CHANGELOG.md‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,78 @@ The format is based on the regulated environment requirements:
99

1010
---
1111

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

1486
**Author:** Erick Bourgeois

‎deploy/deployment/rbac/clusterrole.yaml‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,20 @@ rules:
6666
resources: ["secrets"]
6767
verbs: ["get", "list", "watch"]
6868

69-
# Nodes for drain operations
69+
# Nodes for drain operations and reclaim-agent label / taint management.
70+
#
71+
# `update` was removed in Phase 5 of the 2026-04-25 security audit
72+
# roadmap. Every Node touchpoint in the controller uses `.get` or
73+
# `.patch` (cordon, reclaim-agent label, applied taints) — no path
74+
# uses `Api::replace` or any other update-shaped call. Granting only
75+
# the verbs that are actually exercised reduces blast radius if the
76+
# controller's ServiceAccount token is ever compromised. If a future
77+
# change reintroduces a need for full-object replacement, the lint
78+
# rule in `deny.toml` will surface it before this rule has to be
79+
# widened.
7080
- apiGroups: [""]
7181
resources: ["nodes"]
72-
verbs: ["get", "list", "watch", "patch", "update"]
82+
verbs: ["get", "list", "watch", "patch"]
7383

7484
# Pods for eviction during drain
7585
- apiGroups: [""]
@@ -79,7 +89,14 @@ rules:
7989
resources: ["pods/eviction"]
8090
verbs: ["create"]
8191

82-
# Leases for leader election (no delete needed for HA)
92+
# Leases for leader election (no delete needed for HA).
93+
#
94+
# `update` is retained: `kube_lease_manager` (the leader-election
95+
# library used in src/main.rs) currently uses HTTP PUT semantics
96+
# (Api::replace) on the Lease object during renewal. Removing
97+
# `update` would break leader election. If the upstream library
98+
# switches to merge-patch in a future release we can drop the verb;
99+
# tracked alongside Phase 5 of the security audit roadmap.
83100
- apiGroups: ["coordination.k8s.io"]
84101
resources: ["leases"]
85102
verbs: ["get", "create", "update", "patch"]

‎docs/mkdocs.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ nav:
230230
- Admission Validation: security/admission-validation.md
231231
- Threat Model: security/threat-model.md
232232
- VEX (Vulnerability Exploitability eXchange): security/vex.md
233+
- CRD Attack Surface: security/crd-attack-surface.md
233234

234235
- Developer Guide:
235236
- Development Setup: development/setup.md

‎docs/src/concepts/scheduled-machine.md‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,9 +228,61 @@ The controller:
228228
4. When schedule ends: gracefully shuts down and cleans up all created resources
229229
5. Maintains owner references for automatic garbage collection
230230

231+
## Security: Provider payload pass-through
232+
233+
`spec.bootstrapSpec.spec` and `spec.infrastructureSpec.spec` are
234+
**forwarded unchanged** to the named provider. 5-Spot validates the
235+
provider name (allowlist of `bootstrap.cluster.x-k8s.io`,
236+
`infrastructure.cluster.x-k8s.io`, and `k0smotron.io` groups) and the
237+
envelope shape (`apiVersion`, `kind`, presence of `spec`) but
238+
**does not inspect the inner spec**. That is by design — the controller
239+
is provider-agnostic, and inspecting every possible provider's schema
240+
would couple the controller to every provider's release cycle.
241+
242+
The trust boundary is therefore the **provider**, not 5-Spot:
243+
244+
- **k0smotron `K0sWorkerConfig.spec.cloudInit`** is interpreted as
245+
cloud-init YAML and executed verbatim on the provisioned machine. A
246+
user with `create scheduledmachines` can run any cloud-init payload
247+
on the VMs they cause to be provisioned.
248+
- **k0smotron `RemoteMachine.spec.address`** is the SSH endpoint the
249+
infrastructure controller connects to. A user can point this at any
250+
reachable host or IP.
251+
- **CAPA / CAPM3 / other CAPI providers** carry their own attack
252+
surfaces in their inline specs (image, command, env-with-secret,
253+
etc.).
254+
255+
### Implications for multi-tenant operators
256+
257+
If different teams self-service `ScheduledMachine` CRs in their own
258+
namespaces, you MUST review provider documentation for fields that
259+
grant code execution or cross-tenant reachability **before** granting
260+
`create scheduledmachines` to a tenant. Options:
261+
262+
1. **Pre-stage approved bootstrap / infrastructure resources** in a
263+
platform-controlled namespace and expose only their `kind` + `name`
264+
to tenants via your own wrapper CR. This eliminates the inline spec
265+
entirely. (Out of scope for v1alpha1; tracked for a future
266+
v1alpha2.)
267+
2. **Layer policy on top of 5-Spot**: a CEL `ValidatingAdmissionPolicy`
268+
can reject specific provider fields (e.g. `cloudInit`,
269+
`address` outside an approved CIDR) at admission time. The 5-Spot
270+
`ValidatingAdmissionPolicy` validates structure but does not inspect
271+
provider payloads — that's a complementary policy.
272+
3. **Trust-but-verify**: scope `create` to trusted teams and audit the
273+
inline specs out of band.
274+
275+
The 5-Spot `ValidatingAdmissionPolicy` and the runtime validators in
276+
`src/reconcilers/helpers.rs` together cover everything that 5-Spot
277+
itself can decide is malformed (cluster name length, label prefixes,
278+
schedule format, kill-if-commands bounds, …). They do not — and cannot
279+
— validate provider-specific cloud-init, SSH targets, or container
280+
images.
281+
231282
## Related
232283

233284
- [API Reference](../reference/api.md) - Complete API documentation
234285
- [Machine Lifecycle](./machine-lifecycle.md) - Phase transitions
235286
- [Schedules](./schedules.md) - Schedule configuration details
236287
- [Emergency Reclaim](./emergency-reclaim.md) - `killIfCommands` and the process-match kill switch
288+
- [CRD Attack Surface](../security/crd-attack-surface.md) - per-field validation status and downstream sinks

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# CRD Attack Surface
2+
3+
Per-field validation status and downstream sinks for every
4+
attacker-controllable field in the `ScheduledMachine` CRD. The
5+
intended threat model is a **namespace-scoped tenant** with
6+
`create scheduledmachines.5spot.finos.org` in their own namespace —
7+
the most realistic adversary in a multi-tenant cluster.
8+
9+
> Source of truth for this table is `src/crd.rs` and
10+
> `deploy/admission/validatingadmissionpolicy.yaml`. If you add a new
11+
> field to the CRD, update this page in the same PR.
12+
13+
## Spec fields
14+
15+
| Field | Validation present | Flows to | Status |
16+
|---|---|---|---|
17+
| `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. |
18+
| `spec.schedule.enabled` (bool) | ✓ schema type | phase state machine (drives Active ↔ Inactive transitions) | Safe. Boolean enum, no injection surface. |
19+
| `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. |
20+
| `spec.schedule.hoursOfDay[]` (string array of `0`–`23` or ranges) | ✓ VAP rule 7 (regex), ✓ `parse_hour_ranges()` | schedule evaluator | Safe. Same shape as `daysOfWeek`. |
21+
| `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. |
22+
| `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. |
23+
| `spec.bootstrapSpec.apiVersion` (string) | ✓ VAP rules 8 / 9, ✓ `validate_api_group()` | dynamic resource creation (GVK construction) | Safe. Provider allowlist enforced. |
24+
| `spec.bootstrapSpec.kind` (string, non-empty) | ✓ VAP rule 10 | dynamic resource creation | Safe. Non-empty check; opaque to 5-Spot. |
25+
| **`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). |
26+
| `spec.infrastructureSpec.apiVersion` (string) | ✓ VAP rules 11 / 12, ✓ `validate_api_group()` | dynamic resource creation | Safe. Provider allowlist. |
27+
| `spec.infrastructureSpec.kind` (string, non-empty) | ✓ VAP rule 13 | dynamic resource creation | Safe. |
28+
| **`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`. |
29+
| `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/`). |
30+
| `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. |
31+
| `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)`. |
32+
| `spec.killSwitch` (bool) | ✓ schema type | emergency-termination phase | Safe. Boolean. |
33+
| `spec.gracefulShutdownTimeout` (string, e.g. `"5m"`) | ✓ VAP rule 2, ✓ `parse_duration()` (≤ 24h) | drain timeout | Safe. Format + 24h cap. |
34+
| `spec.nodeDrainTimeout` (string) | ✓ VAP rule 3, ✓ `parse_duration()` | drain timeout | Safe. Same shape. |
35+
| `spec.priority` (u8) | ✓ schema type (0–255) | consistent-hash assignment for multi-instance distribution | Safe. Range-bounded. |
36+
37+
## Status fields (writable by anyone with `patch scheduledmachines/status`)
38+
39+
| Field | How the controller treats it | Spoof risk |
40+
|---|---|---|
41+
| `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. |
42+
| `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. |
43+
| `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. |
44+
| `status.bootstrapRef.name`, `status.infrastructureRef.name` | Same as `machineRef`. | Low. |
45+
| `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. |
46+
| `status.observedGeneration` | Read for debugging only. | None. |
47+
48+
## Summary
49+
50+
- 16 of 18 spec fields have defence-in-depth validation (VAP at admission + runtime check at reconcile).
51+
- 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.
52+
- 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.
53+
54+
## Related
55+
56+
- [Admission Validation](./admission-validation.md) — the CEL `ValidatingAdmissionPolicy` that enforces these rules at the API server.
57+
- [Threat Model](./threat-model.md) — STRIDE analysis of the controller's trust boundaries.
58+
- [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‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ A STRIDE-based analysis of the threats facing 5-Spot and the mitigations in plac
2222

2323
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.
2424

25+
### [CRD Attack Surface](crd-attack-surface.md)
26+
27+
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.
28+
2529
---
2630

2731
## Security Posture at a Glance

‎src/crd.rs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,33 @@ impl ScheduleSpec {
199199
///
200200
/// Must contain at minimum `apiVersion` and `kind` fields. The controller
201201
/// will extract these to create the appropriate dynamic resource.
202+
///
203+
/// # Security — pass-through trust boundary
204+
///
205+
/// **The `spec` field of an `EmbeddedResource` is forwarded unchanged
206+
/// to the named provider.** The 5-Spot reconciler validates the
207+
/// envelope (`apiVersion` group allowlist, presence of `kind`, etc.)
208+
/// but **does not inspect the inner spec**. That is by design —
209+
/// 5-Spot is provider-agnostic and cannot ship a schema for every
210+
/// possible CAPI provider.
211+
///
212+
/// This means the trust boundary is the provider, not 5-Spot:
213+
///
214+
/// - `k0smotron.io/K0sWorkerConfig.spec.cloudInit` is interpreted as
215+
/// cloud-init YAML and executed verbatim on the provisioned VM.
216+
/// - `k0smotron.io/RemoteMachine.spec.address` is an SSH endpoint
217+
/// reached by the infrastructure controller.
218+
/// - Other providers carry their own code-execution / network-reach
219+
/// surfaces in their inline specs.
220+
///
221+
/// In multi-tenant clusters where different teams can `create
222+
/// scheduledmachines` in their own namespaces, operators **MUST**
223+
/// either pre-stage approved provider specs (out of scope for
224+
/// v1alpha1) or layer a complementary `ValidatingAdmissionPolicy`
225+
/// that inspects the provider payload — the 5-Spot VAP only
226+
/// validates structure. See `docs/src/concepts/scheduled-machine.md`
227+
/// (section "Security: Provider payload pass-through") for the full
228+
/// trade-off discussion.
202229
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
203230
#[schemars(schema_with = "embedded_resource_schema")]
204231
pub struct EmbeddedResource(pub Value);

0 commit comments

Comments
 (0)