diff --git a/.claude/CHANGELOG.md b/.claude/CHANGELOG.md index 30c525c..4a2aeab 100644 --- a/.claude/CHANGELOG.md +++ b/.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 diff --git a/Makefile b/Makefile index 8359a49..e749ccc 100644 --- a/Makefile +++ b/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..." diff --git a/deploy/admission/validatingadmissionpolicy.yaml b/deploy/admission/validatingadmissionpolicy.yaml index 90f10d1..b0298db 100644 --- a/deploy/admission/validatingadmissionpolicy.yaml +++ b/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/ 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 where unit ∈ {s, m, h}. # Mirrors the parse_duration() check in src/reconcilers/helpers.rs. diff --git a/deploy/crds/scheduledmachine.yaml b/deploy/crds/scheduledmachine.yaml index d500d65..aac4462 100644 --- a/deploy/crds/scheduledmachine.yaml +++ b/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//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/`) 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 diff --git a/src/constants.rs b/src/constants.rs index 6d4b866..41ace26 100644 --- a/src/constants.rs +++ b/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//comm` (15 chars max per the kernel) and +/// `/proc//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; diff --git a/src/crd.rs b/src/crd.rs index 3ae15b2..7689819 100644 --- a/src/crd.rs +++ b/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//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/`) 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>, } @@ -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.). diff --git a/src/metrics.rs b/src/metrics.rs index 2126fc2..16c437e 100644 --- a/src/metrics.rs +++ b/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}")) } // ============================================================================ diff --git a/src/reconcilers/helpers.rs b/src/reconcilers/helpers.rs index 1db9b51..f872471 100644 --- a/src/reconcilers/helpers.rs +++ b/src/reconcilers/helpers.rs @@ -44,8 +44,9 @@ use crate::constants::{ CAPI_CLUSTER_NAME_LABEL, CAPI_GROUP, CAPI_MACHINE_API_VERSION, CAPI_MACHINE_API_VERSION_FULL, CAPI_RESOURCE_MACHINES, CONDITION_STATUS_TRUE, CONDITION_TYPE_READY, DEFAULT_INSTANCE_ID, ENV_OPERATOR_INSTANCE_ID, ERROR_REQUEUE_SECS, FINALIZER_CLEANUP_TIMEOUT_SECS, - FINALIZER_SCHEDULED_MACHINE, MAX_BACKOFF_SECS, MAX_DURATION_SECS, MAX_RECONCILE_RETRIES, - PHASE_ACTIVE, PHASE_ERROR, PHASE_INACTIVE, PHASE_SHUTTING_DOWN, PHASE_TERMINATED, + FINALIZER_SCHEDULED_MACHINE, MAX_BACKOFF_SECS, MAX_CLUSTER_NAME_LEN, MAX_DURATION_SECS, + MAX_KILL_IF_COMMANDS_COUNT, MAX_KILL_IF_COMMAND_LEN, MAX_RECONCILE_RETRIES, PHASE_ACTIVE, + PHASE_ERROR, PHASE_INACTIVE, PHASE_SHUTTING_DOWN, PHASE_TERMINATED, POD_EVICTION_GRACE_PERIOD_SECS, REASON_GRACE_PERIOD, REASON_KILL_SWITCH, REASON_RECONCILE_SUCCEEDED, RESERVED_LABEL_PREFIXES, TIMER_REQUEUE_SECS, }; @@ -405,16 +406,38 @@ pub fn check_grace_period_elapsed(resource: &ScheduledMachine) -> Result= timeout.as_secs() as i64` expression + // silently returned false in that case and could bypass the + // graceful-drain window entirely. Treat any negative elapsed as + // "timeout reached" so the reconciler forces progress rather than + // stalling on a misbehaving clock. + if elapsed_secs < 0 { + warn!( + grace_start = %start_time, + now_utc = %now, + elapsed_secs, + "Negative elapsed time detected (clock adjustment?); treating grace period as elapsed" + ); + return Ok(true); + } + + // Safe cast: u64 timeout converted to i64 cannot wrap because + // MAX_DURATION_SECS (24h) fits in i64, and elapsed_secs is now + // known non-negative. #[allow(clippy::cast_possible_wrap)] - Ok(elapsed.num_seconds() >= timeout.as_secs() as i64) + Ok(elapsed_secs >= timeout.as_secs() as i64) } else { // No grace period started yet Ok(true) @@ -754,6 +777,89 @@ pub fn validate_labels( Ok(()) } +/// Reject `spec.clusterName` values that exceed the effective CAPI cap or +/// contain characters unsafe for log / label / metric emission. +/// +/// CAPI itself inherits the Kubernetes 253-char DNS-1123 subdomain cap on +/// `Cluster.metadata.name`, but the name flows downstream into DNS labels +/// (63 chars) and the `cluster.x-k8s.io/cluster-name` label value. 63 is +/// therefore the *effective* upper bound — see [`MAX_CLUSTER_NAME_LEN`]. +/// +/// The character check rejects control chars, whitespace, and non-ASCII so +/// a malicious CR cannot forge log records via embedded newlines or bloat +/// Prometheus label cardinality with exotic Unicode. +/// +/// # Errors +/// [`ReconcilerError::ValidationError`] if the name is empty, too long, or +/// contains a disallowed character. +pub fn validate_cluster_name(cluster_name: &str) -> Result<(), ReconcilerError> { + if cluster_name.is_empty() { + return Err(ReconcilerError::ValidationError( + "spec.clusterName must not be empty".to_string(), + )); + } + if cluster_name.len() > MAX_CLUSTER_NAME_LEN { + return Err(ReconcilerError::ValidationError(format!( + "spec.clusterName length {} exceeds maximum of {MAX_CLUSTER_NAME_LEN} characters", + cluster_name.len() + ))); + } + // Allow ASCII letters, digits, '-', '.', '_' — the intersection of + // DNS-1123 subdomain and Kubernetes label-value charsets. Anything + // outside that (including embedded NUL, newline, or multi-byte UTF-8) + // is rejected with a single uniform error. + if !cluster_name + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'.' || b == b'_') + { + return Err(ReconcilerError::ValidationError(format!( + "spec.clusterName contains invalid character(s); allowed: ASCII alphanumerics, '-', '.', '_' (got: {cluster_name:?})" + ))); + } + Ok(()) +} + +/// Reject `spec.killIfCommands` lists that exceed the count cap or contain +/// an over-length / empty pattern. +/// +/// The reclaim agent evaluates each pattern against every PID in `/proc`; +/// without a cap, a single CR can pin an entire node's agent CPU or balloon +/// the per-node `ConfigMap` projection. Empty patterns would match every +/// process and are almost certainly a mistake rather than an intentional +/// kill-all rule — reject them loudly. +/// +/// `None` and `Some(&[])` are both accepted: the controller treats them +/// identically ("no reclaim agent on this node"). +/// +/// # Errors +/// [`ReconcilerError::ValidationError`] on any violation, naming the +/// offending index so operators can fix the CR quickly. +pub fn validate_kill_if_commands(commands: Option<&[String]>) -> Result<(), ReconcilerError> { + let Some(cmds) = commands else { + return Ok(()); + }; + if cmds.len() > MAX_KILL_IF_COMMANDS_COUNT { + return Err(ReconcilerError::ValidationError(format!( + "spec.killIfCommands has {} entries, exceeds maximum of {MAX_KILL_IF_COMMANDS_COUNT}", + cmds.len() + ))); + } + for (idx, cmd) in cmds.iter().enumerate() { + if cmd.is_empty() { + return Err(ReconcilerError::ValidationError(format!( + "spec.killIfCommands[{idx}] must not be empty" + ))); + } + if cmd.len() > MAX_KILL_IF_COMMAND_LEN { + return Err(ReconcilerError::ValidationError(format!( + "spec.killIfCommands[{idx}] length {} exceeds maximum of {MAX_KILL_IF_COMMAND_LEN} characters", + cmd.len() + ))); + } + } + Ok(()) +} + /// Validate that an apiVersion string belongs to an allowed API group. /// /// Core Kubernetes API versions (no `/`) are always rejected for CAPI resources. @@ -839,6 +945,16 @@ pub async fn add_machine_to_cluster( .cloned() .unwrap_or_default(); + // Validate cluster name and killIfCommands bounds before touching CAPI. + // `cluster_name` flows into every derived resource's label set and into + // log/metric emission; `killIfCommands` into the per-node reclaim-agent + // `ConfigMap`. Rejecting oversize input here keeps both failure modes + // out of the hot path. Admission policy does the same check at + // CREATE/UPDATE — this runtime guard is defence-in-depth for clusters + // that have not enabled ValidatingAdmissionPolicy. + validate_cluster_name(cluster_name)?; + validate_kill_if_commands(resource.spec.kill_if_commands.as_deref())?; + // Validate API groups before creating any resources validate_api_group( bootstrap_api_version, @@ -1591,14 +1707,27 @@ pub fn error_policy( resource.namespace().unwrap_or_default(), resource.name_any() ); - let retry_count = { - let mut counts = ctx - .retry_counts - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let count = counts.entry(key).or_insert(0); - *count = count.saturating_add(1); - *count + // Explicit poison handling: if another reconciler thread panicked while + // holding `retry_counts`, the map may be in an inconsistent state. Rather + // than silently recovering the inner map and continuing with partially- + // modified retry counters (which would corrupt the exponential-backoff + // schedule), log the incident and requeue after the standard error + // interval — a subsequent reconcile will proceed from a fresh lock. + let retry_count = match ctx.retry_counts.lock() { + Ok(mut counts) => { + let count = counts.entry(key).or_insert(0); + *count = count.saturating_add(1); + *count + } + Err(poisoned) => { + error!( + resource_key = %key, + error = %poisoned, + original_error = %err, + "retry_counts mutex poisoned; aborting this error-policy pass and requeuing after ERROR_REQUEUE_SECS" + ); + return Action::requeue(Duration::from_secs(ERROR_REQUEUE_SECS)); + } }; let backoff = compute_backoff_secs(retry_count); error!( diff --git a/src/reconcilers/helpers_tests.rs b/src/reconcilers/helpers_tests.rs index f37efd3..1f65faf 100644 --- a/src/reconcilers/helpers_tests.rs +++ b/src/reconcilers/helpers_tests.rs @@ -5,7 +5,8 @@ mod tests { use super::super::*; use crate::constants::{ - ALLOWED_BOOTSTRAP_API_GROUPS, ALLOWED_INFRASTRUCTURE_API_GROUPS, MAX_DURATION_SECS, + ALLOWED_BOOTSTRAP_API_GROUPS, ALLOWED_INFRASTRUCTURE_API_GROUPS, MAX_CLUSTER_NAME_LEN, + MAX_DURATION_SECS, MAX_KILL_IF_COMMANDS_COUNT, MAX_KILL_IF_COMMAND_LEN, }; use std::collections::BTreeMap; @@ -3310,4 +3311,271 @@ mod tests { } srv.await.unwrap(); } + + // ======================================================================== + // check_grace_period_elapsed — clock-skew safety + // + // Regression: when the grace-period start timestamp is *ahead* of the + // current wall clock (system clock adjusted backward, NTP step, VM + // freeze-thaw, etc.), `elapsed.num_seconds()` is negative. The prior + // implementation cast the negative i64 to unsigned via + // `as i64 >= timeout.as_secs() as i64`, silently returning false and + // potentially bypassing the graceful-drain window altogether. The fix + // treats any negative elapsed duration as "timeout reached" so we never + // stall a drain on a misbehaving clock. + // ======================================================================== + + use crate::constants::REASON_GRACE_PERIOD; + + fn sm_with_grace_period_start( + start_ts_rfc3339: &str, + graceful_shutdown_timeout: &str, + ) -> crate::crd::ScheduledMachine { + use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; + crate::crd::ScheduledMachine { + metadata: ObjectMeta { + name: Some("sm-grace".to_string()), + namespace: Some("default".to_string()), + ..Default::default() + }, + spec: crate::crd::ScheduledMachineSpec { + cluster_name: "test-cluster".to_string(), + bootstrap_spec: crate::crd::EmbeddedResource(serde_json::json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "spec": {} + })), + infrastructure_spec: crate::crd::EmbeddedResource(serde_json::json!({ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "RemoteMachine", + "spec": {} + })), + machine_template: None, + schedule: crate::crd::ScheduleSpec { + days_of_week: vec!["mon-fri".to_string()], + hours_of_day: vec!["9-17".to_string()], + timezone: "UTC".to_string(), + enabled: true, + }, + priority: 50, + graceful_shutdown_timeout: graceful_shutdown_timeout.to_string(), + node_drain_timeout: "5m".to_string(), + kill_switch: false, + node_taints: vec![], + kill_if_commands: None, + }, + status: Some(crate::crd::ScheduledMachineStatus { + phase: Some("ShuttingDown".to_string()), + conditions: vec![crate::crd::Condition { + r#type: "Scheduled".to_string(), + status: "False".to_string(), + last_transition_time: start_ts_rfc3339.to_string(), + reason: REASON_GRACE_PERIOD.to_string(), + message: "Grace period started".to_string(), + }], + ..Default::default() + }), + } + } + + #[test] + fn test_check_grace_period_elapsed_future_start_returns_true() { + // Clock skew: start time is 1 hour in the future. Prior code returned + // false (bypassing the drain check); fix must return true so the + // reconciler forces progress rather than hanging. + let future = (Utc::now() + chrono::Duration::hours(1)).to_rfc3339(); + let sm = sm_with_grace_period_start(&future, "5m"); + let elapsed = check_grace_period_elapsed(&sm).expect("must not error"); + assert!( + elapsed, + "negative elapsed (future start time) must be treated as timeout reached" + ); + } + + #[test] + fn test_check_grace_period_elapsed_recent_start_returns_false() { + // 10 seconds ago, timeout 5m — grace period still active. + let recent = (Utc::now() - chrono::Duration::seconds(10)).to_rfc3339(); + let sm = sm_with_grace_period_start(&recent, "5m"); + let elapsed = check_grace_period_elapsed(&sm).expect("must not error"); + assert!(!elapsed, "recent start within timeout must return false"); + } + + #[test] + fn test_check_grace_period_elapsed_exceeded_start_returns_true() { + // 10 minutes ago, timeout 5m — grace period exceeded. + let old = (Utc::now() - chrono::Duration::minutes(10)).to_rfc3339(); + let sm = sm_with_grace_period_start(&old, "5m"); + let elapsed = check_grace_period_elapsed(&sm).expect("must not error"); + assert!(elapsed, "start older than timeout must return true"); + } + + #[test] + fn test_check_grace_period_elapsed_invalid_timestamp_errors() { + let sm = sm_with_grace_period_start("not-a-timestamp", "5m"); + let err = check_grace_period_elapsed(&sm).unwrap_err(); + assert!(err.to_string().contains("Invalid timestamp")); + } + + #[test] + fn test_check_grace_period_elapsed_no_status_errors() { + use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; + let sm = crate::crd::ScheduledMachine { + metadata: ObjectMeta { + name: Some("sm".to_string()), + namespace: Some("default".to_string()), + ..Default::default() + }, + spec: sm_with_grace_period_start(&Utc::now().to_rfc3339(), "5m") + .spec + .clone(), + status: None, + }; + let err = check_grace_period_elapsed(&sm).unwrap_err(); + assert!(err.to_string().contains("no status")); + } + + // ======================================================================== + // validate_cluster_name — CAPI cluster-name bounds + // + // `spec.clusterName` is stamped onto every derived resource as a label + // value (`cluster.x-k8s.io/cluster-name`) and appears in log lines and + // metric labels. Bounding it here stops both Prometheus label-cardinality + // DoS and log-line / label-value overflow. + // ======================================================================== + + #[test] + fn test_validate_cluster_name_accepts_short_dns_label() { + assert!(validate_cluster_name("prod-east-1").is_ok()); + } + + #[test] + fn test_validate_cluster_name_accepts_max_length() { + let name = "a".repeat(MAX_CLUSTER_NAME_LEN); + assert!( + validate_cluster_name(&name).is_ok(), + "exactly MAX_CLUSTER_NAME_LEN must be accepted" + ); + } + + #[test] + fn test_validate_cluster_name_rejects_empty() { + let err = validate_cluster_name("").unwrap_err(); + assert!(err.to_string().contains("must not be empty")); + } + + #[test] + fn test_validate_cluster_name_rejects_over_max() { + let name = "a".repeat(MAX_CLUSTER_NAME_LEN + 1); + let err = validate_cluster_name(&name).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains(&MAX_CLUSTER_NAME_LEN.to_string()), + "error should mention the cap, got: {msg}" + ); + } + + #[test] + fn test_validate_cluster_name_rejects_control_chars() { + // Log-injection vector: newline in cluster name could forge JSON log + // records in a downstream SIEM. + let err = validate_cluster_name("prod\neast").unwrap_err(); + assert!(err.to_string().contains("invalid character")); + } + + #[test] + fn test_validate_cluster_name_rejects_non_ascii() { + let err = validate_cluster_name("prod-é").unwrap_err(); + assert!(err.to_string().contains("invalid character")); + } + + // ======================================================================== + // validate_kill_if_commands — bounded list, bounded entries + // + // `spec.killIfCommands` is evaluated by the per-node reclaim agent + // against every `/proc//{comm,cmdline}`; an unbounded count would + // pin agent CPU, and an unbounded per-entry length could balloon the + // per-node ConfigMap projection. + // ======================================================================== + + #[test] + fn test_validate_kill_if_commands_none_accepted() { + assert!(validate_kill_if_commands(None).is_ok()); + } + + #[test] + fn test_validate_kill_if_commands_empty_accepted() { + assert!(validate_kill_if_commands(Some(&[])).is_ok()); + } + + #[test] + fn test_validate_kill_if_commands_typical_list_accepted() { + let cmds = vec!["java".to_string(), "python".to_string(), "node".to_string()]; + assert!(validate_kill_if_commands(Some(&cmds)).is_ok()); + } + + #[test] + fn test_validate_kill_if_commands_at_count_cap_accepted() { + let cmds: Vec = (0..MAX_KILL_IF_COMMANDS_COUNT) + .map(|i| format!("cmd-{i}")) + .collect(); + assert!(validate_kill_if_commands(Some(&cmds)).is_ok()); + } + + #[test] + fn test_validate_kill_if_commands_over_count_cap_rejected() { + let cmds: Vec = (0..=MAX_KILL_IF_COMMANDS_COUNT) + .map(|i| format!("cmd-{i}")) + .collect(); + let err = validate_kill_if_commands(Some(&cmds)).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains(&MAX_KILL_IF_COMMANDS_COUNT.to_string()), + "error should mention the count cap, got: {msg}" + ); + } + + #[test] + fn test_validate_kill_if_commands_at_len_cap_accepted() { + let cmd = "a".repeat(MAX_KILL_IF_COMMAND_LEN); + assert!(validate_kill_if_commands(Some(&[cmd])).is_ok()); + } + + #[test] + fn test_validate_kill_if_commands_over_len_cap_rejected() { + let cmd = "a".repeat(MAX_KILL_IF_COMMAND_LEN + 1); + let err = validate_kill_if_commands(Some(&[cmd])).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains(&MAX_KILL_IF_COMMAND_LEN.to_string()), + "error should mention the per-entry cap, got: {msg}" + ); + } + + #[test] + fn test_validate_kill_if_commands_empty_entry_rejected() { + let cmds = vec!["java".to_string(), String::new()]; + let err = validate_kill_if_commands(Some(&cmds)).unwrap_err(); + assert!(err.to_string().contains("must not be empty")); + } + + #[test] + fn test_check_grace_period_elapsed_no_grace_condition_returns_true() { + // No condition with reason=GracePeriodActive → conservatively true so + // drain proceeds rather than hanging. + use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; + let sm = crate::crd::ScheduledMachine { + metadata: ObjectMeta { + name: Some("sm".to_string()), + namespace: Some("default".to_string()), + ..Default::default() + }, + spec: sm_with_grace_period_start(&Utc::now().to_rfc3339(), "5m") + .spec + .clone(), + status: Some(crate::crd::ScheduledMachineStatus::default()), + }; + let elapsed = check_grace_period_elapsed(&sm).expect("must not error"); + assert!(elapsed, "missing grace condition must return true"); + } } diff --git a/src/reconcilers/mod.rs b/src/reconcilers/mod.rs index 1e4a812..2080a90 100644 --- a/src/reconcilers/mod.rs +++ b/src/reconcilers/mod.rs @@ -20,7 +20,7 @@ pub mod scheduled_machine; // Re-export main types and functions pub use helpers::{ error_policy, evaluate_schedule, machine_to_scheduled_machine, node_to_scheduled_machines, - parse_duration, reconcile_node_taints, should_process_resource, NodeTaintReconcileOutcome, - ReconcileNodeTaintsInput, + parse_duration, reconcile_node_taints, should_process_resource, validate_cluster_name, + validate_kill_if_commands, NodeTaintReconcileOutcome, ReconcileNodeTaintsInput, }; pub use scheduled_machine::{reconcile_scheduled_machine, Context, ReconcilerError}; diff --git a/src/reconcilers/scheduled_machine.rs b/src/reconcilers/scheduled_machine.rs index b71b224..b0e362c 100644 --- a/src/reconcilers/scheduled_machine.rs +++ b/src/reconcilers/scheduled_machine.rs @@ -377,6 +377,15 @@ async fn reconcile_inner( })?; let name = resource.name_any(); + // Defence-in-depth input validation. ValidatingAdmissionPolicy is the + // first line of defence (rejected at CREATE/UPDATE), but clusters that + // have not enabled VAP still need these bounds enforced. Runs early so + // every downstream phase handler sees a sanitised spec — notably + // cluster_name before any log/metric emission, killIfCommands before + // the reclaim-agent projection. + super::helpers::validate_cluster_name(&resource.spec.cluster_name)?; + super::helpers::validate_kill_if_commands(resource.spec.kill_if_commands.as_deref())?; + // Get current status let current_phase = resource .status