Minor fixes to address some vulnerabilities I found

.claude/CHANGELOG.md

@@ -9,6 +9,117 @@ The format is based on the regulated environment requirements:
 
 ---
 
+## [2026-04-25 06:30] - `gitleaks-install` now wires the local pre-commit hook
+
+**Author:** Erick Bourgeois
+
+### Changed
+- `Makefile` (`gitleaks-install`): after installing the binary, invokes
+  `install-git-hooks` so a single `make gitleaks-install` leaves the
+  developer with both the tool AND the local secret-scanning hook in
+  place. Previously the two targets were independent and the hook had
+  to be set up via a second command.
+- `Makefile` (`install-git-hooks`): no longer depends on
+  `gitleaks-install` (would create a circular dependency now that
+  `gitleaks-install` calls it). The hook only invokes gitleaks at
+  *commit* time, so install-time independence is correct.
+- `Makefile` (`install-git-hooks`): now idempotent and non-destructive.
+  A `5spot-managed-gitleaks-hook` sentinel embedded in the hook lets
+  re-runs detect "already installed" and leave the file alone. If a
+  custom pre-commit hook is found (no sentinel), it is preserved at
+  `.git/hooks/pre-commit.bak` rather than overwritten silently.
+- `Makefile` (hook content): the generated hook now checks for
+  gitleaks on PATH and emits a clear "run make gitleaks-install"
+  message if it is missing, instead of failing with a cryptic
+  "command not found".
+
+### Why
+Onboarding friction: developers ran `make gitleaks-install` expecting
+the secret scan to fire on commit, then committed secrets because the
+hook had never been wired. Banking-environment requirement (no
+secrets in commits) was therefore enforced only by CI — too late.
+Wiring hook setup into the install target makes the local guard the
+default for every dev, while idempotency + backup keeps re-runs safe.
+
+### Impact
+- [ ] Breaking change
+- [ ] Requires cluster rollout
+- [x] Config change only (developer tooling)
+- [ ] Documentation only
+
+---
+
+## [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

Makefile

@@ -374,7 +374,11 @@ undeploy: ## Remove operator from cluster
 # Security Scanning
 # ============================================================
 
-gitleaks-install: ## Install gitleaks from GitHub with checksum verification
+# Sentinel string written into .git/hooks/pre-commit so we can recognise our
+# own hook on re-runs and avoid clobbering a developer's custom pre-commit.
+GITLEAKS_HOOK_SENTINEL := 5spot-managed-gitleaks-hook
+
+gitleaks-install: ## Install gitleaks (with checksum verification) AND wire the local pre-commit hook
 	@if ! command -v gitleaks >/dev/null 2>&1; then \
 		echo "Installing gitleaks v$(GITLEAKS_VERSION)..."; \
 		OS=$$(uname -s | tr '[:upper:]' '[:lower:]'); \
@@ -407,30 +411,56 @@ gitleaks-install: ## Install gitleaks from GitHub with checksum verification
 	else \
 		echo "✓ gitleaks already installed: $$(gitleaks version)"; \
 	fi
+	@$(MAKE) --no-print-directory install-git-hooks
 
 gitleaks: gitleaks-install ## Scan for hardcoded secrets and credentials
 	@echo "Scanning for secrets with gitleaks..."
 	@gitleaks detect --source . --verbose --redact
 
-install-git-hooks: gitleaks-install ## Install git hooks for pre-commit secret scanning
-	@echo "Installing git hooks..."
+# install-git-hooks is intentionally decoupled from gitleaks-install — the hook
+# only invokes gitleaks at *commit* time, not at install time, so we avoid a
+# circular dependency (gitleaks-install → install-git-hooks → gitleaks-install).
+# The hook is idempotent: if a custom pre-commit already exists without our
+# sentinel we back it up to pre-commit.bak rather than overwriting silently.
+install-git-hooks: ## Install git pre-commit hook for secret scanning (idempotent; preserves custom hooks)
+	@if [ ! -d .git ]; then \
+		echo "✗ Not inside a git repository (.git missing) — skipping hook install"; \
+		exit 0; \
+	fi
 	@mkdir -p .git/hooks
-	@echo '#!/bin/sh' > .git/hooks/pre-commit
-	@echo '# Pre-commit hook to scan for secrets' >> .git/hooks/pre-commit
-	@echo '' >> .git/hooks/pre-commit
-	@echo 'echo "Running gitleaks pre-commit scan..."' >> .git/hooks/pre-commit
-	@echo 'gitleaks protect --staged --verbose --redact' >> .git/hooks/pre-commit
-	@echo 'if [ $$? -ne 0 ]; then' >> .git/hooks/pre-commit
-	@echo '    echo ""' >> .git/hooks/pre-commit
-	@echo '    echo "ERROR: Secrets detected in staged changes!"' >> .git/hooks/pre-commit
-	@echo '    echo "Please remove secrets before committing."' >> .git/hooks/pre-commit
-	@echo '    echo "If this is a false positive, add to .gitleaks.toml allowlist."' >> .git/hooks/pre-commit
-	@echo '    exit 1' >> .git/hooks/pre-commit
-	@echo 'fi' >> .git/hooks/pre-commit
-	@chmod +x .git/hooks/pre-commit
-	@echo "✓ Pre-commit hook installed"
-	@echo "  Hook location: .git/hooks/pre-commit"
-	@echo "  Gitleaks will scan staged changes before each commit"
+	@if [ -f .git/hooks/pre-commit ] && grep -q "$(GITLEAKS_HOOK_SENTINEL)" .git/hooks/pre-commit 2>/dev/null; then \
+		echo "✓ Pre-commit hook already managed by 5spot — leaving in place"; \
+	else \
+		if [ -f .git/hooks/pre-commit ]; then \
+			echo "⚠ Existing pre-commit hook detected — backing up to .git/hooks/pre-commit.bak"; \
+			mv .git/hooks/pre-commit .git/hooks/pre-commit.bak; \
+		fi; \
+		echo "Installing git pre-commit hook..."; \
+		printf '%s\n' \
+			'#!/bin/sh' \
+			'# $(GITLEAKS_HOOK_SENTINEL)' \
+			'# Pre-commit hook to scan staged changes for secrets via gitleaks.' \
+			'# Reinstall with: make install-git-hooks   (idempotent)' \
+			'' \
+			'if ! command -v gitleaks >/dev/null 2>&1; then' \
+			'    echo "ERROR: gitleaks not found on PATH — run \"make gitleaks-install\" first." >&2' \
+			'    exit 1' \
+			'fi' \
+			'' \
+			'echo "Running gitleaks pre-commit scan..."' \
+			'gitleaks protect --staged --verbose --redact' \
+			'rc=$$?' \
+			'if [ $$rc -ne 0 ]; then' \
+			'    echo "" >&2' \
+			'    echo "ERROR: Secrets detected in staged changes!" >&2' \
+			'    echo "Please remove secrets before committing." >&2' \
+			'    echo "If this is a false positive, add to .gitleaks.toml allowlist." >&2' \
+			'    exit 1' \
+			'fi' \
+			> .git/hooks/pre-commit; \
+		chmod +x .git/hooks/pre-commit; \
+		echo "✓ Pre-commit hook installed at .git/hooks/pre-commit"; \
+	fi
 
 security-scan-local: gitleaks ## Run local security scans (gitleaks)
 	@echo "Running local security scans..."

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;