# 0041. Four fixes from the phase 3-6 security review

Status: Accepted · Date: 2026-10-26 · Design section: Configuration reconcile, Trust, Bootstrap, GitOps and Argo CD

Decided: redact bundle material from dry-run warnings unconditionally rather than relying on Talos v1.14.3's current (verified) behavior of never echoing one; renamed a registry-auth leak test to match what it actually exercises and documented its two genuinely unreachable paths; validate `ichor init --cluster` as a Kubernetes object name before any file write; added CEL format checks to `TalosClusterRegistry.host`/`.endpoints` since machinery validates neither.

## Context

A read-only security review of everything added since an earlier phase-1-2 review (`internal/controller`, `internal/uiapi`, `internal/cli`, `pkg/`, `api/v1alpha1`, RBAC manifests) turned up four findings worth fixing. This ADR covers the four; a fifth (a missing `nodes` RBAC grant on the UI backend) went to a different agent owning `internal/uiapi`.

## Decision: dry-run warnings are redacted unconditionally, not because Talos echoes a value today

`TalosMachineReconciler.setWarnings` (`internal/controller/talosmachine_controller.go`) joins Talos's own dry-run warning strings (`machined`'s `ApplyConfiguration` RPC, `DryRun: true`) into a `TalosMachine.status` Condition -- a field readable far more broadly than Secret RBAC allows. The review flagged this as untested against real bundle material.

**What was actually read, not assumed** (Talos v1.14.3, both modules in `$(go env GOMODCACHE)/github.com/siderolabs/talos@v1.14.3` and `.../talos/pkg/machinery@v1.14.3`):

- `internal/app/machined/internal/server/v1alpha1/v1alpha1_server.go`'s `ApplyConfiguration`: for `in.DryRun == true`, the response is built with only `Mode`/`ModeDetails` (a `configdiff.DiffConfigs` summary) -- `Messages[i].Warnings` is never set on that branch at all. It *is* set, from `ValidateAtRuntime`'s own return value, on the non-dry-run branch (`Warnings: warnings`, line 316 in that file). Ichor's own `DryRunApply` (`internal/controller/talosclient_machinery.go`) always sends `DryRun: true`, so `msg.GetWarnings()` is, for this exact Talos version, always empty -- the warnings feature this code reads from has never had anything to read, through the real RPC, in production.
- `pkg/machinery/config/container/validate.go`'s `validate`/`runtimeValidate`/`validateContainer`, and every warning-string call site in `pkg/machinery/config/types/v1alpha1/v1alpha1_validation.go` (grepped for every `warnings = append` in that file, read each call site's surrounding code): every warning is either a static deprecation notice, a field *name*, or a non-secret value (machine type, network interface name, CNI name). None echoes a submitted secret-shaped value. No validator exists at all for `RegistryMirrorConfig`/`RegistryConfig`/`RegistryAuthConfig` (grepped, zero matches) -- relevant to the fourth decision below too.

So: today, for Talos v1.14.3, this is not a live leak, through either of the two facts above (the RPC drops warnings for dry-run; and even if it didn't, nothing generates a value-echoing one). Fixed anyway, because both facts are about Talos's current version, not about this code, and `setWarnings` itself had zero redaction logic -- a future Talos release changing either fact would reopen the gap with no corresponding change needed on Ichor's side to trigger it.

**Fix**: `redactBundleMaterial` (`internal/controller/bundle.go`) replaces any of the bundle's own secret-shaped scalars found verbatim in the joined warning text with `"[redacted]"`, called unconditionally from `setWarnings` regardless of whether today's warnings could ever actually contain one. `bundleSecretMaterial` (the field list) moved from `leak_test.go` into production code in the same file, so the list the test checks against and the list the redaction uses are the same list, not two that could drift. `TestTalosMachineBundleLeak` (`leak_test.go`) gained an eighth route, `"dry run warnings echo bundle material"`, feeding `fakeTalosClient.warnings` the bundle's own secret material directly -- standing in for whatever a future Talos version might start echoing, rather than waiting for one to actually ship before this is tested at all.

## Decision: name the registry-auth leak test for what it tests, document what it can't

`pkg/render/registry_test.go`'s `TestRegistryAuthLeak` had a subtest named "render fails downstream after resolving a real credential." It forces failure via `VersionContract = "not-a-version"`, which fails at `coreconfig.ParseContractFromVersion` (`render.go`, in `buildConfig`, before `injectRegistries` is ever called) -- not downstream of credential resolution at all. The credential in scope never gets near a failing code path.

Checked whether `injectRegistries`'s own two error returns (`registry.go`: `container.New(doc)`, `configpatcher.Apply`) could be forced instead, reading both functions: `container.New` is called with exactly one already-typed, in-process document, so its only failure modes (duplicate/conflicting document) cannot trigger against a single document; `configpatcher.Apply` merges that document onto a `cfg` `baseConfig` just generated fresh via machinery's own generator, so there is no reachable way through this package's public API to make either call fail. Both are exactly as unreachable, and for the same shape of reason, as the two paths `TestRenderBundleLeak` already documents it could not force.

**Fix**: renamed the subtest to `"render fails before registries are ever touched (invalid versionContract)"`, and added a comment naming both unreachable paths and why, in the same style `TestRenderBundleLeak`'s own doc comment already uses for its two.

## Decision: validate `--cluster` before any file write, not after the API server would anyway

`ichor init`'s Step 1 (`internal/cli/init_run.go`) writes the secrets bundle to `~/.ichor/clusters/<cluster>/secrets.yaml` (`internal/cli/bundlefile.go`'s `filepath.Join(home, ".ichor", "clusters", cluster)`) before the `TalosCluster` object -- whose `metadata.name` is the same `--cluster` value -- ever reaches the API server, which is the only thing that would otherwise reject a malformed value. `--cluster` becomes that object name regardless, so checking the same constraint earlier is not a new restriction, only an earlier application of one that already existed.

**Fix**: `buildInitPlan` (`internal/cli/init_plan.go`) now rejects `--cluster` with `k8s.io/apimachinery/pkg/util/validation.IsDNS1123Subdomain` before any other check, including the ones already there -- `buildInitPlan` runs entirely before Step 1's file write, so this is enough to close the gap without touching `init_run.go`'s own ordering. Covered by a new case in `TestBuildInitPlan_Errors`'s existing table (`init_plan_test.go`).

## Decision: `TalosClusterRegistry.host`/`.endpoints` get the same CEL rigor `spec.discovery` already has, confirmed necessary first

Checked first, as the review asked, whether machinery's own `ValidateAsClient`/`ValidateAtRuntime` catches a malformed `host` or `endpoints` value: grepped `pkg/machinery/config/types/v1alpha1/v1alpha1_validation.go` for `RegistryMirror`/`RegistriesConfig`/`MachineRegistries` -- zero matches. Machinery has no validator at all for this part of the config; a malformed value reaches a node's machine config completely unexamined.

**Fix**: added CEL to both fields -- `host` must be `"*"` or match `^[a-zA-Z0-9.-]+(:[0-9]{1,5})?$`; every `endpoints` entry must satisfy the CEL extension library's `isURL()`. The first attempt (a stricter, RFC-shaped hostname regex, and `isURL` with no length bound) was rejected by the API server's CEL cost estimator (`estimated rule cost exceeds budget by factor of 25.2x` for the regex, `16.8x` for the unbounded `isURL` loop) -- fixed by adding `MaxLength=255` on `host` and `MaxItems=8`/`items:MaxLength=2048` on `endpoints` (the estimator's own suggested fix) and simplifying the regex to drop nested optional groups, the same style `spec.discovery.addresses`/`.cidrs` already uses for the same reason. Verified against envtest (not just `go build`, which cannot catch a CEL compile-time rejection): `make manifests generate` followed by `go test ./api/v1alpha1/...` with `KUBEBUILDER_ASSETS` set, which installs the real CRD into a real (test) API server and would fail exactly the way the first attempt did if the budget were still exceeded. Three new specs in `validation_test.go` cover accepting `"*"` and a `host:port`, and rejecting a non-matching host and a non-URL endpoint.

## Consequences

- `internal/controller/bundle.go` gained `bundleSecretMaterial` and `redactBundleMaterial`; `setWarnings` takes `*secrets.Bundle` as a new parameter.
- `pkg/render/registry_test.go`: one subtest renamed, no behavior change.
- `internal/cli/init_plan.go`: one new, early validation; no change to any already-valid `--cluster` value's behavior.
- `api/v1alpha1/taloscluster_types.go`: two new CEL rules, both additive (narrower schema, not wider) -- any already-valid `TalosClusterRegistry` continues to validate, confirmed by the existing fixtures in `pkg/render` (which never go through API-server admission, so are unaffected either way) and `api/v1alpha1/validation_test.go` (which does, and still passes).
- The RBAC finding from the same review (`internal/uiapi`'s `GetNode` missing a `nodes` grant) is out of scope here -- handled by the agent owning `internal/uiapi`.
- Follow-up: `redactBundleMaterial` was removed; `setWarnings` and the etcd-backup controller (ADR 0047) now share one `scrubber` in `internal/controller/bundle.go`, which replaces longer values first so a secret that contains another is not left half-visible.