Minor fixes to address some vulnerabilities I found

Signed-off-by: Erick Bourgeois <erick@jeb.ca>

Author: ebourgeois

.claude/CHANGELOG.md

@@ -9,6 +9,77 @@ The format is based on the regulated environment requirements:
 
 ---
 
+## [2026-04-24 15:00] - Security audit remediation: grace period, retry-count poisoning, input bounds
+
+**Author:** Erick Bourgeois
+
+### Changed
+- `src/reconcilers/helpers.rs` (`check_grace_period_elapsed`): guard
+  against negative `elapsed` durations produced by clock-skew (NTP step,
+  VM freeze-thaw, backward wall-clock adjustment). The prior comparison
+  `elapsed.num_seconds() >= timeout.as_secs() as i64` silently returned
+  `false` when `elapsed` was negative, potentially bypassing the
+  graceful-drain window entirely. Negative elapsed is now treated as
+  "timeout reached" so the reconciler forces progress rather than
+  stalling on a misbehaving clock.
+- `src/reconcilers/helpers.rs` (`error_policy`): replace
+  `ctx.retry_counts.lock().unwrap_or_else(PoisonError::into_inner)`
+  with an explicit `match` that aborts the error-policy pass and
+  requeues after `ERROR_REQUEUE_SECS` on poison. The prior pattern
+  silently recovered a potentially-inconsistent map and continued
+  mutating it, corrupting the exponential-backoff schedule for every
+  subsequent error.
+- `src/reconcilers/helpers.rs`: new `validate_cluster_name()` and
+  `validate_kill_if_commands()` with bounds from `src/constants.rs`.
+  Called early in `reconcile_inner` (defence-in-depth against clusters
+  that have not enabled the ValidatingAdmissionPolicy) and re-called in
+  `add_machine_to_cluster` before touching CAPI.
+- `src/constants.rs`: new `MAX_CLUSTER_NAME_LEN = 63` (RFC-1123 DNS
+  label — the effective CAPI cluster-name cap), plus
+  `MAX_KILL_IF_COMMANDS_COUNT = 100` and `MAX_KILL_IF_COMMAND_LEN = 256`.
+- `src/crd.rs`: attach bounded JSON-schema generators
+  (`cluster_name_schema`, `kill_if_commands_schema`) so the generated
+  CRD enforces the same limits at the kube API level.
+- `src/metrics.rs`: replace `unreachable!()` in the metric-registration
+  fallback constructors with `panic!()` carrying
+  `FALLBACK_METRIC_BUG_MSG` — a pointed diagnostic that identifies the
+  offending metric name instead of a generic "entered unreachable code"
+  message.
+- `deploy/admission/validatingadmissionpolicy.yaml`: add CEL expressions
+  1b/1c/1d/1e mirroring the new runtime validators — `clusterName`
+  length/charset, `killIfCommands` count/per-entry-length — so invalid
+  specs are rejected at admission instead of only at reconciliation
+  time.
+- `src/reconcilers/helpers_tests.rs`: 20 new tests covering the grace-
+  period clock-skew fix, `validate_cluster_name` (happy path, empty,
+  over-cap, control chars, non-ASCII), and `validate_kill_if_commands`
+  (None/empty/typical/cap-boundary/over-cap/empty-entry).
+- `deploy/crds/scheduledmachine.yaml`: regenerated via `cargo run --bin
+  crdgen` to pick up the new schema constraints.
+- `docs/src/reference/api.md`: regenerated via `make crddoc`.
+
+### Why
+Findings from the deep security audit of the codebase (see
+conversation transcript 2026-04-24). Three issues were actionable: a
+clock-skew bypass of the graceful-drain window (critical; violates the
+contract of graceful shutdown under SOX §404 / NIST AC-3), an
+unbounded `killIfCommands` that could balloon Prometheus label
+cardinality and pin reclaim-agent CPU (high; DoS vector), and
+silent recovery from a poisoned retry-count mutex (medium; corrupts
+the exponential-backoff schedule). Cluster-name was also unbounded;
+the 63-char cap matches the effective CAPI constraint (cluster names
+flow into DNS labels downstream). Metric `unreachable!()` is a
+cosmetic bug-diagnostic improvement.
+
+### Impact
+- [ ] Breaking change
+- [x] Requires cluster rollout (new CRD schema constraints; existing
+      CRs that violate them will be rejected on next `kubectl apply`)
+- [ ] Config change only
+- [ ] Documentation only
+
+---
+
 ## [2026-04-23 08:00] - Fix vexctl-install Linux path (wrong asset filename)
 
 **Author:** Erick Bourgeois

deploy/admission/validatingadmissionpolicy.yaml

@@ -34,11 +34,58 @@ spec:
 
   validations:
 
-    # ── 1. clusterName ────────────────────────────────────────────────────────
+    # ── 1. clusterName: non-empty ────────────────────────────────────────────
     - expression: "object.spec.clusterName.size() > 0"
       message: "spec.clusterName must not be empty"
       reason: Invalid
 
+    # ── 1b. clusterName: length ≤ MAX_CLUSTER_NAME_LEN (63) ──────────────────
+    # Mirrors MAX_CLUSTER_NAME_LEN in src/constants.rs. 63 is the effective
+    # CAPI cluster-name cap because the value flows downstream into DNS
+    # labels (RFC-1123 limit) and into the
+    # `cluster.x-k8s.io/cluster-name` label value. Rejecting longer values
+    # at admission also bounds Prometheus label cardinality — the metrics
+    # pipeline emits cluster_name as a label on several counters.
+    - expression: "object.spec.clusterName.size() <= 63"
+      message: >-
+        spec.clusterName must be ≤ 63 characters (RFC-1123 DNS label limit;
+        the effective CAPI cluster-name cap)
+      reason: Invalid
+
+    # ── 1c. clusterName: safe character set ──────────────────────────────────
+    # Restricts clusterName to ASCII alphanumerics, '-', '.', '_'. Blocks
+    # log-injection via embedded newlines or NUL bytes and prevents exotic
+    # Unicode from bloating Prometheus label cardinality. Mirrors the
+    # runtime check in validate_cluster_name() (src/reconcilers/helpers.rs).
+    - expression: "object.spec.clusterName.matches('^[A-Za-z0-9][A-Za-z0-9._-]*$')"
+      message: >-
+        spec.clusterName must start with an ASCII alphanumeric and contain
+        only ASCII alphanumerics, '-', '.', or '_'
+      reason: Invalid
+
+    # ── 1d. killIfCommands: count ≤ MAX_KILL_IF_COMMANDS_COUNT (100) ─────────
+    # Mirrors MAX_KILL_IF_COMMANDS_COUNT in src/constants.rs. The per-node
+    # reclaim agent evaluates every pattern against every PID in /proc; an
+    # unbounded list lets a malicious CR pin agent CPU. Trivially true when
+    # killIfCommands is absent (optional field).
+    - expression: "!has(object.spec.killIfCommands) || object.spec.killIfCommands.size() <= 100"
+      message: >-
+        spec.killIfCommands must have at most 100 entries — each entry is
+        evaluated against every /proc/<pid> by the per-node reclaim agent
+      reason: Invalid
+
+    # ── 1e. killIfCommands: per-entry length ≤ MAX_KILL_IF_COMMAND_LEN (256) ─
+    # Mirrors MAX_KILL_IF_COMMAND_LEN in src/constants.rs. Caps both agent
+    # CPU per match attempt and the size of the per-node ConfigMap
+    # projection. Empty strings are also rejected — they would match every
+    # process and are almost certainly a mistake.
+    - expression: |
+        !has(object.spec.killIfCommands) ||
+        object.spec.killIfCommands.all(c, c.size() >= 1 && c.size() <= 256)
+      message: >-
+        spec.killIfCommands entries must be 1..=256 characters each
+      reason: Invalid
+
     # ── 2. gracefulShutdownTimeout duration format ────────────────────────────
     # Must be <positive-integer><unit> where unit ∈ {s, m, h}.
     # Mirrors the parse_duration() check in src/reconcilers/helpers.rs.

deploy/crds/scheduledmachine.yaml

@@ -63,7 +63,19 @@ spec:
                 - spec
                 type: object
               clusterName:
-                description: Name of the CAPI cluster this machine belongs to
+                description: |-
+                  Name of the CAPI cluster this machine belongs to.
+
+                  Bounded to 63 characters — the RFC-1123 DNS label limit and the
+                  effective CAPI cluster-name cap, since the value flows downstream
+                  into the `cluster.x-k8s.io/cluster-name` label and into generated
+                  DNS labels. The schema also restricts the charset to ASCII
+                  alphanumerics, `-`, `.`, and `_` to block log-injection via
+                  embedded control characters and to bound Prometheus label
+                  cardinality.
+                maxLength: 63
+                minLength: 1
+                pattern: ^[A-Za-z0-9][A-Za-z0-9._-]*$
                 type: string
               gracefulShutdownTimeout:
                 default: 5m
@@ -104,9 +116,17 @@ spec:
                   basename) and `/proc/<pid>/cmdline` (substring). See the
                   `5spot-emergency-reclaim-by-process-match.md` roadmap for full
                   semantics.
+
+                  Bounded to 100 entries × 256 characters each. The caps guard the
+                  per-node agent's CPU (every pattern is evaluated against every
+                  `/proc/<pid>`) and cap the size of the per-node `ConfigMap`
+                  projection — an unbounded list is both an operator foot-gun and a
+                  denial-of-service vector when driven from a malicious CR.
                 items:
+                  maxLength: 256
+                  minLength: 1
                   type: string
-                nullable: true
+                maxItems: 100
                 type: array
               killSwitch:
                 default: false

src/constants.rs

@@ -486,6 +486,34 @@ pub const MAX_DURATION_SECS: u64 = 86_400;
 /// Maximum allowed timezone string length
 pub const MAX_TIMEZONE_LEN: usize = 64;
 
+/// Maximum allowed length of `spec.clusterName`.
+///
+/// CAPI's `Cluster.metadata.name` inherits the Kubernetes DNS-1123 subdomain
+/// cap (253 chars), but cluster names are used downstream as DNS labels and
+/// as label values (`cluster.x-k8s.io/cluster-name`), both of which are
+/// bounded by RFC-1123's 63-character DNS label limit. 63 is therefore the
+/// effective CAPI constraint. Rejecting longer values at the CR boundary
+/// also bounds Prometheus label cardinality (metrics emit the cluster name
+/// via `CAPI_CLUSTER_NAME_LABEL`) and caps log-line width, closing a
+/// cheap log-injection / cardinality-DoS vector.
+pub const MAX_CLUSTER_NAME_LEN: usize = 63;
+
+/// Maximum number of `spec.killIfCommands` patterns accepted on a single
+/// `ScheduledMachine`. The reclaim agent evaluates every pattern against
+/// every PID in `/proc`; an unbounded list lets a malicious (or
+/// misconfigured) CR pin agent CPU. 100 is well above any realistic
+/// workload — a single node rarely runs more than a dozen distinct
+/// kill-switchable processes — and keeps the worst-case match cost bounded.
+pub const MAX_KILL_IF_COMMANDS_COUNT: usize = 100;
+
+/// Maximum length of a single `spec.killIfCommands` entry. Patterns are
+/// matched against `/proc/<pid>/comm` (15 chars max per the kernel) and
+/// `/proc/<pid>/cmdline` (longer, but arguments beyond 256 bytes are a red
+/// flag for the intended use case of process-basename matching). This bound
+/// also caps Prometheus label widths if a future metric tags reclaim
+/// outcomes by matched pattern.
+pub const MAX_KILL_IF_COMMAND_LEN: usize = 256;
+
 /// Timeout for finalizer cleanup operations (10 minutes in seconds)
 pub const FINALIZER_CLEANUP_TIMEOUT_SECS: u64 = 600;
 

src/crd.rs

@@ -45,7 +45,16 @@ pub struct ScheduledMachineSpec {
     /// Machine scheduling configuration
     pub schedule: ScheduleSpec,
 
-    /// Name of the CAPI cluster this machine belongs to
+    /// Name of the CAPI cluster this machine belongs to.
+    ///
+    /// Bounded to 63 characters — the RFC-1123 DNS label limit and the
+    /// effective CAPI cluster-name cap, since the value flows downstream
+    /// into the `cluster.x-k8s.io/cluster-name` label and into generated
+    /// DNS labels. The schema also restricts the charset to ASCII
+    /// alphanumerics, `-`, `.`, and `_` to block log-injection via
+    /// embedded control characters and to bound Prometheus label
+    /// cardinality.
+    #[schemars(schema_with = "cluster_name_schema")]
     pub cluster_name: String,
 
     /// Inline bootstrap configuration spec (e.g., `K0sWorkerConfig`)
@@ -101,7 +110,14 @@ pub struct ScheduledMachineSpec {
     /// basename) and `/proc/<pid>/cmdline` (substring). See the
     /// `5spot-emergency-reclaim-by-process-match.md` roadmap for full
     /// semantics.
+    ///
+    /// Bounded to 100 entries × 256 characters each. The caps guard the
+    /// per-node agent's CPU (every pattern is evaluated against every
+    /// `/proc/<pid>`) and cap the size of the per-node `ConfigMap`
+    /// projection — an unbounded list is both an operator foot-gun and a
+    /// denial-of-service vector when driven from a malicious CR.
     #[serde(skip_serializing_if = "Option::is_none", default)]
+    #[schemars(schema_with = "kill_if_commands_schema")]
     pub kill_if_commands: Option<Vec<String>>,
 }
 
@@ -222,6 +238,35 @@ fn timezone_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema {
     })
 }
 
+/// Schema for `spec.clusterName` — bounded to the effective CAPI cluster-name
+/// cap (RFC-1123 DNS label, 63 chars) with an ASCII-safe charset. Mirrors the
+/// runtime check in `validate_cluster_name()` (src/reconcilers/helpers.rs).
+fn cluster_name_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema {
+    schemars::json_schema!({
+        "type": "string",
+        "minLength": 1,
+        "maxLength": 63,
+        "pattern": "^[A-Za-z0-9][A-Za-z0-9._-]*$"
+    })
+}
+
+/// Schema for `spec.killIfCommands` — bounded list of bounded strings.
+/// Mirrors the runtime check in `validate_kill_if_commands()`
+/// (src/reconcilers/helpers.rs). 100 patterns × 256 chars is well above any
+/// realistic workload and caps both reclaim-agent CPU cost and the per-node
+/// `ConfigMap` projection size.
+fn kill_if_commands_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema {
+    schemars::json_schema!({
+        "type": "array",
+        "maxItems": 100,
+        "items": {
+            "type": "string",
+            "minLength": 1,
+            "maxLength": 256
+        }
+    })
+}
+
 /// Schema for `EmbeddedResource` — requires apiVersion, kind, and spec fields.
 /// The `spec` field uses `x-kubernetes-preserve-unknown-fields` to allow any
 /// provider-specific fields (`K0sWorkerConfig`, `RemoteMachine`, `AWSMachine`, etc.).

src/metrics.rs

@@ -39,28 +39,41 @@ use prometheus::{
 //
 // If metric registration fails (e.g., duplicate name in tests), we log a
 // warning and fall back to an *unregistered* metric so the process continues.
-// The fallback constructors use hardcoded valid names and cannot panic.
+// The fallback constructors use hardcoded metric names that the Prometheus
+// crate must accept (ASCII alphanumerics + underscores, non-empty, not
+// starting with a digit). They have never failed in practice — but the
+// contract is enforced by `prometheus`, not us, so we guard with `expect()`
+// carrying a pointed diagnostic rather than `unreachable!()` which compiles
+// to a panic with a misleading message. Either way a failure here is a
+// programming bug (likely a rename that introduced an invalid character),
+// not a runtime configuration issue.
 // ============================================================================
 
+/// Error message used by every fallback constructor — identifies the failing
+/// metric so a crash log points straight at the offending hardcoded name.
+const FALLBACK_METRIC_BUG_MSG: &str = "BUG: hardcoded metric name failed Prometheus validation; \
+     this is a programming error, not a runtime issue — \
+     see src/metrics.rs for the offending static";
+
 /// Create an *unregistered* `CounterVec` used as a no-op fallback when
 /// `register_counter_vec!` fails (e.g. duplicate name in test processes).
 fn fallback_counter_vec(name: &str, help: &str, labels: &[&str]) -> CounterVec {
     CounterVec::new(Opts::new(name, help), labels)
-        .unwrap_or_else(|_| unreachable!("hardcoded metric name '{name}' is always valid"))
+        .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}"))
 }
 
 /// Create an *unregistered* `Gauge` used as a no-op fallback when
 /// `register_gauge!` fails.
 fn fallback_gauge(name: &str, help: &str) -> Gauge {
     Gauge::new(name, help)
-        .unwrap_or_else(|_| unreachable!("hardcoded metric name '{name}' is always valid"))
+        .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}"))
 }
 
 /// Create an *unregistered* `GaugeVec` used as a no-op fallback when
 /// `register_gauge_vec!` fails.
 fn fallback_gauge_vec(name: &str, help: &str, labels: &[&str]) -> GaugeVec {
     GaugeVec::new(Opts::new(name, help), labels)
-        .unwrap_or_else(|_| unreachable!("hardcoded metric name '{name}' is always valid"))
+        .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}"))
 }
 
 /// Create an *unregistered* `HistogramVec` used as a no-op fallback when
@@ -75,7 +88,7 @@ fn fallback_histogram_vec(
         prometheus::HistogramOpts::new(name, help).buckets(buckets),
         labels,
     )
-    .unwrap_or_else(|_| unreachable!("hardcoded metric name '{name}' is always valid"))
+    .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}"))
 }
 
 // ============================================================================