Skip to content

Commit f4f1d3c

Browse files
authored
Minor fixes to address some vulnerabilities I found (#46)
Signed-off-by: Erick Bourgeois <erick@jeb.ca>
1 parent 33cfc8e commit f4f1d3c

11 files changed

Lines changed: 743 additions & 43 deletions

File tree

.claude/CHANGELOG.md

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

1010
---
1111

12+
## [2026-04-25 06:30] - `gitleaks-install` now wires the local pre-commit hook
13+
14+
**Author:** Erick Bourgeois
15+
16+
### Changed
17+
- `Makefile` (`gitleaks-install`): after installing the binary, invokes
18+
`install-git-hooks` so a single `make gitleaks-install` leaves the
19+
developer with both the tool AND the local secret-scanning hook in
20+
place. Previously the two targets were independent and the hook had
21+
to be set up via a second command.
22+
- `Makefile` (`install-git-hooks`): no longer depends on
23+
`gitleaks-install` (would create a circular dependency now that
24+
`gitleaks-install` calls it). The hook only invokes gitleaks at
25+
*commit* time, so install-time independence is correct.
26+
- `Makefile` (`install-git-hooks`): now idempotent and non-destructive.
27+
A `5spot-managed-gitleaks-hook` sentinel embedded in the hook lets
28+
re-runs detect "already installed" and leave the file alone. If a
29+
custom pre-commit hook is found (no sentinel), it is preserved at
30+
`.git/hooks/pre-commit.bak` rather than overwritten silently.
31+
- `Makefile` (hook content): the generated hook now checks for
32+
gitleaks on PATH and emits a clear "run make gitleaks-install"
33+
message if it is missing, instead of failing with a cryptic
34+
"command not found".
35+
36+
### Why
37+
Onboarding friction: developers ran `make gitleaks-install` expecting
38+
the secret scan to fire on commit, then committed secrets because the
39+
hook had never been wired. Banking-environment requirement (no
40+
secrets in commits) was therefore enforced only by CI — too late.
41+
Wiring hook setup into the install target makes the local guard the
42+
default for every dev, while idempotency + backup keeps re-runs safe.
43+
44+
### Impact
45+
- [ ] Breaking change
46+
- [ ] Requires cluster rollout
47+
- [x] Config change only (developer tooling)
48+
- [ ] Documentation only
49+
50+
---
51+
52+
## [2026-04-24 15:00] - Security audit remediation: grace period, retry-count poisoning, input bounds
53+
54+
**Author:** Erick Bourgeois
55+
56+
### Changed
57+
- `src/reconcilers/helpers.rs` (`check_grace_period_elapsed`): guard
58+
against negative `elapsed` durations produced by clock-skew (NTP step,
59+
VM freeze-thaw, backward wall-clock adjustment). The prior comparison
60+
`elapsed.num_seconds() >= timeout.as_secs() as i64` silently returned
61+
`false` when `elapsed` was negative, potentially bypassing the
62+
graceful-drain window entirely. Negative elapsed is now treated as
63+
"timeout reached" so the reconciler forces progress rather than
64+
stalling on a misbehaving clock.
65+
- `src/reconcilers/helpers.rs` (`error_policy`): replace
66+
`ctx.retry_counts.lock().unwrap_or_else(PoisonError::into_inner)`
67+
with an explicit `match` that aborts the error-policy pass and
68+
requeues after `ERROR_REQUEUE_SECS` on poison. The prior pattern
69+
silently recovered a potentially-inconsistent map and continued
70+
mutating it, corrupting the exponential-backoff schedule for every
71+
subsequent error.
72+
- `src/reconcilers/helpers.rs`: new `validate_cluster_name()` and
73+
`validate_kill_if_commands()` with bounds from `src/constants.rs`.
74+
Called early in `reconcile_inner` (defence-in-depth against clusters
75+
that have not enabled the ValidatingAdmissionPolicy) and re-called in
76+
`add_machine_to_cluster` before touching CAPI.
77+
- `src/constants.rs`: new `MAX_CLUSTER_NAME_LEN = 63` (RFC-1123 DNS
78+
label — the effective CAPI cluster-name cap), plus
79+
`MAX_KILL_IF_COMMANDS_COUNT = 100` and `MAX_KILL_IF_COMMAND_LEN = 256`.
80+
- `src/crd.rs`: attach bounded JSON-schema generators
81+
(`cluster_name_schema`, `kill_if_commands_schema`) so the generated
82+
CRD enforces the same limits at the kube API level.
83+
- `src/metrics.rs`: replace `unreachable!()` in the metric-registration
84+
fallback constructors with `panic!()` carrying
85+
`FALLBACK_METRIC_BUG_MSG` — a pointed diagnostic that identifies the
86+
offending metric name instead of a generic "entered unreachable code"
87+
message.
88+
- `deploy/admission/validatingadmissionpolicy.yaml`: add CEL expressions
89+
1b/1c/1d/1e mirroring the new runtime validators — `clusterName`
90+
length/charset, `killIfCommands` count/per-entry-length — so invalid
91+
specs are rejected at admission instead of only at reconciliation
92+
time.
93+
- `src/reconcilers/helpers_tests.rs`: 20 new tests covering the grace-
94+
period clock-skew fix, `validate_cluster_name` (happy path, empty,
95+
over-cap, control chars, non-ASCII), and `validate_kill_if_commands`
96+
(None/empty/typical/cap-boundary/over-cap/empty-entry).
97+
- `deploy/crds/scheduledmachine.yaml`: regenerated via `cargo run --bin
98+
crdgen` to pick up the new schema constraints.
99+
- `docs/src/reference/api.md`: regenerated via `make crddoc`.
100+
101+
### Why
102+
Findings from the deep security audit of the codebase (see
103+
conversation transcript 2026-04-24). Three issues were actionable: a
104+
clock-skew bypass of the graceful-drain window (critical; violates the
105+
contract of graceful shutdown under SOX §404 / NIST AC-3), an
106+
unbounded `killIfCommands` that could balloon Prometheus label
107+
cardinality and pin reclaim-agent CPU (high; DoS vector), and
108+
silent recovery from a poisoned retry-count mutex (medium; corrupts
109+
the exponential-backoff schedule). Cluster-name was also unbounded;
110+
the 63-char cap matches the effective CAPI constraint (cluster names
111+
flow into DNS labels downstream). Metric `unreachable!()` is a
112+
cosmetic bug-diagnostic improvement.
113+
114+
### Impact
115+
- [ ] Breaking change
116+
- [x] Requires cluster rollout (new CRD schema constraints; existing
117+
CRs that violate them will be rejected on next `kubectl apply`)
118+
- [ ] Config change only
119+
- [ ] Documentation only
120+
121+
---
122+
12123
## [2026-04-23 08:00] - Fix vexctl-install Linux path (wrong asset filename)
13124

14125
**Author:** Erick Bourgeois

Makefile

Lines changed: 49 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,11 @@ undeploy: ## Remove operator from cluster
374374
# Security Scanning
375375
# ============================================================
376376

377-
gitleaks-install: ## Install gitleaks from GitHub with checksum verification
377+
# Sentinel string written into .git/hooks/pre-commit so we can recognise our
378+
# own hook on re-runs and avoid clobbering a developer's custom pre-commit.
379+
GITLEAKS_HOOK_SENTINEL := 5spot-managed-gitleaks-hook
380+
381+
gitleaks-install: ## Install gitleaks (with checksum verification) AND wire the local pre-commit hook
378382
@if ! command -v gitleaks >/dev/null 2>&1; then \
379383
echo "Installing gitleaks v$(GITLEAKS_VERSION)..."; \
380384
OS=$$(uname -s | tr '[:upper:]' '[:lower:]'); \
@@ -407,30 +411,56 @@ gitleaks-install: ## Install gitleaks from GitHub with checksum verification
407411
else \
408412
echo "✓ gitleaks already installed: $$(gitleaks version)"; \
409413
fi
414+
@$(MAKE) --no-print-directory install-git-hooks
410415

411416
gitleaks: gitleaks-install ## Scan for hardcoded secrets and credentials
412417
@echo "Scanning for secrets with gitleaks..."
413418
@gitleaks detect --source . --verbose --redact
414419

415-
install-git-hooks: gitleaks-install ## Install git hooks for pre-commit secret scanning
416-
@echo "Installing git hooks..."
420+
# install-git-hooks is intentionally decoupled from gitleaks-install — the hook
421+
# only invokes gitleaks at *commit* time, not at install time, so we avoid a
422+
# circular dependency (gitleaks-install → install-git-hooks → gitleaks-install).
423+
# The hook is idempotent: if a custom pre-commit already exists without our
424+
# sentinel we back it up to pre-commit.bak rather than overwriting silently.
425+
install-git-hooks: ## Install git pre-commit hook for secret scanning (idempotent; preserves custom hooks)
426+
@if [ ! -d .git ]; then \
427+
echo "✗ Not inside a git repository (.git missing) — skipping hook install"; \
428+
exit 0; \
429+
fi
417430
@mkdir -p .git/hooks
418-
@echo '#!/bin/sh' > .git/hooks/pre-commit
419-
@echo '# Pre-commit hook to scan for secrets' >> .git/hooks/pre-commit
420-
@echo '' >> .git/hooks/pre-commit
421-
@echo 'echo "Running gitleaks pre-commit scan..."' >> .git/hooks/pre-commit
422-
@echo 'gitleaks protect --staged --verbose --redact' >> .git/hooks/pre-commit
423-
@echo 'if [ $$? -ne 0 ]; then' >> .git/hooks/pre-commit
424-
@echo ' echo ""' >> .git/hooks/pre-commit
425-
@echo ' echo "ERROR: Secrets detected in staged changes!"' >> .git/hooks/pre-commit
426-
@echo ' echo "Please remove secrets before committing."' >> .git/hooks/pre-commit
427-
@echo ' echo "If this is a false positive, add to .gitleaks.toml allowlist."' >> .git/hooks/pre-commit
428-
@echo ' exit 1' >> .git/hooks/pre-commit
429-
@echo 'fi' >> .git/hooks/pre-commit
430-
@chmod +x .git/hooks/pre-commit
431-
@echo "✓ Pre-commit hook installed"
432-
@echo " Hook location: .git/hooks/pre-commit"
433-
@echo " Gitleaks will scan staged changes before each commit"
431+
@if [ -f .git/hooks/pre-commit ] && grep -q "$(GITLEAKS_HOOK_SENTINEL)" .git/hooks/pre-commit 2>/dev/null; then \
432+
echo "✓ Pre-commit hook already managed by 5spot — leaving in place"; \
433+
else \
434+
if [ -f .git/hooks/pre-commit ]; then \
435+
echo "⚠ Existing pre-commit hook detected — backing up to .git/hooks/pre-commit.bak"; \
436+
mv .git/hooks/pre-commit .git/hooks/pre-commit.bak; \
437+
fi; \
438+
echo "Installing git pre-commit hook..."; \
439+
printf '%s\n' \
440+
'#!/bin/sh' \
441+
'# $(GITLEAKS_HOOK_SENTINEL)' \
442+
'# Pre-commit hook to scan staged changes for secrets via gitleaks.' \
443+
'# Reinstall with: make install-git-hooks (idempotent)' \
444+
'' \
445+
'if ! command -v gitleaks >/dev/null 2>&1; then' \
446+
' echo "ERROR: gitleaks not found on PATH — run \"make gitleaks-install\" first." >&2' \
447+
' exit 1' \
448+
'fi' \
449+
'' \
450+
'echo "Running gitleaks pre-commit scan..."' \
451+
'gitleaks protect --staged --verbose --redact' \
452+
'rc=$$?' \
453+
'if [ $$rc -ne 0 ]; then' \
454+
' echo "" >&2' \
455+
' echo "ERROR: Secrets detected in staged changes!" >&2' \
456+
' echo "Please remove secrets before committing." >&2' \
457+
' echo "If this is a false positive, add to .gitleaks.toml allowlist." >&2' \
458+
' exit 1' \
459+
'fi' \
460+
> .git/hooks/pre-commit; \
461+
chmod +x .git/hooks/pre-commit; \
462+
echo "✓ Pre-commit hook installed at .git/hooks/pre-commit"; \
463+
fi
434464

435465
security-scan-local: gitleaks ## Run local security scans (gitleaks)
436466
@echo "Running local security scans..."

deploy/admission/validatingadmissionpolicy.yaml

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,58 @@ spec:
3434

3535
validations:
3636

37-
# ── 1. clusterName ────────────────────────────────────────────────────────
37+
# ── 1. clusterName: non-empty ────────────────────────────────────────────
3838
- expression: "object.spec.clusterName.size() > 0"
3939
message: "spec.clusterName must not be empty"
4040
reason: Invalid
4141

42+
# ── 1b. clusterName: length ≤ MAX_CLUSTER_NAME_LEN (63) ──────────────────
43+
# Mirrors MAX_CLUSTER_NAME_LEN in src/constants.rs. 63 is the effective
44+
# CAPI cluster-name cap because the value flows downstream into DNS
45+
# labels (RFC-1123 limit) and into the
46+
# `cluster.x-k8s.io/cluster-name` label value. Rejecting longer values
47+
# at admission also bounds Prometheus label cardinality — the metrics
48+
# pipeline emits cluster_name as a label on several counters.
49+
- expression: "object.spec.clusterName.size() <= 63"
50+
message: >-
51+
spec.clusterName must be ≤ 63 characters (RFC-1123 DNS label limit;
52+
the effective CAPI cluster-name cap)
53+
reason: Invalid
54+
55+
# ── 1c. clusterName: safe character set ──────────────────────────────────
56+
# Restricts clusterName to ASCII alphanumerics, '-', '.', '_'. Blocks
57+
# log-injection via embedded newlines or NUL bytes and prevents exotic
58+
# Unicode from bloating Prometheus label cardinality. Mirrors the
59+
# runtime check in validate_cluster_name() (src/reconcilers/helpers.rs).
60+
- expression: "object.spec.clusterName.matches('^[A-Za-z0-9][A-Za-z0-9._-]*$')"
61+
message: >-
62+
spec.clusterName must start with an ASCII alphanumeric and contain
63+
only ASCII alphanumerics, '-', '.', or '_'
64+
reason: Invalid
65+
66+
# ── 1d. killIfCommands: count ≤ MAX_KILL_IF_COMMANDS_COUNT (100) ─────────
67+
# Mirrors MAX_KILL_IF_COMMANDS_COUNT in src/constants.rs. The per-node
68+
# reclaim agent evaluates every pattern against every PID in /proc; an
69+
# unbounded list lets a malicious CR pin agent CPU. Trivially true when
70+
# killIfCommands is absent (optional field).
71+
- expression: "!has(object.spec.killIfCommands) || object.spec.killIfCommands.size() <= 100"
72+
message: >-
73+
spec.killIfCommands must have at most 100 entries — each entry is
74+
evaluated against every /proc/<pid> by the per-node reclaim agent
75+
reason: Invalid
76+
77+
# ── 1e. killIfCommands: per-entry length ≤ MAX_KILL_IF_COMMAND_LEN (256) ─
78+
# Mirrors MAX_KILL_IF_COMMAND_LEN in src/constants.rs. Caps both agent
79+
# CPU per match attempt and the size of the per-node ConfigMap
80+
# projection. Empty strings are also rejected — they would match every
81+
# process and are almost certainly a mistake.
82+
- expression: |
83+
!has(object.spec.killIfCommands) ||
84+
object.spec.killIfCommands.all(c, c.size() >= 1 && c.size() <= 256)
85+
message: >-
86+
spec.killIfCommands entries must be 1..=256 characters each
87+
reason: Invalid
88+
4289
# ── 2. gracefulShutdownTimeout duration format ────────────────────────────
4390
# Must be <positive-integer><unit> where unit ∈ {s, m, h}.
4491
# Mirrors the parse_duration() check in src/reconcilers/helpers.rs.

deploy/crds/scheduledmachine.yaml

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,19 @@ spec:
6363
- spec
6464
type: object
6565
clusterName:
66-
description: Name of the CAPI cluster this machine belongs to
66+
description: |-
67+
Name of the CAPI cluster this machine belongs to.
68+
69+
Bounded to 63 characters — the RFC-1123 DNS label limit and the
70+
effective CAPI cluster-name cap, since the value flows downstream
71+
into the `cluster.x-k8s.io/cluster-name` label and into generated
72+
DNS labels. The schema also restricts the charset to ASCII
73+
alphanumerics, `-`, `.`, and `_` to block log-injection via
74+
embedded control characters and to bound Prometheus label
75+
cardinality.
76+
maxLength: 63
77+
minLength: 1
78+
pattern: ^[A-Za-z0-9][A-Za-z0-9._-]*$
6779
type: string
6880
gracefulShutdownTimeout:
6981
default: 5m
@@ -104,9 +116,17 @@ spec:
104116
basename) and `/proc/<pid>/cmdline` (substring). See the
105117
`5spot-emergency-reclaim-by-process-match.md` roadmap for full
106118
semantics.
119+
120+
Bounded to 100 entries × 256 characters each. The caps guard the
121+
per-node agent's CPU (every pattern is evaluated against every
122+
`/proc/<pid>`) and cap the size of the per-node `ConfigMap`
123+
projection — an unbounded list is both an operator foot-gun and a
124+
denial-of-service vector when driven from a malicious CR.
107125
items:
126+
maxLength: 256
127+
minLength: 1
108128
type: string
109-
nullable: true
129+
maxItems: 100
110130
type: array
111131
killSwitch:
112132
default: false

src/constants.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,34 @@ pub const MAX_DURATION_SECS: u64 = 86_400;
486486
/// Maximum allowed timezone string length
487487
pub const MAX_TIMEZONE_LEN: usize = 64;
488488

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

0 commit comments

Comments
 (0)