diff --git a/README.md b/README.md index 15f7b7d..2c523b6 100644 --- a/README.md +++ b/README.md @@ -172,13 +172,13 @@ cluster: > **Apply-time safety gates.** `talm apply` and `talm upgrade` run additional gates around each operation: > -> 1. **Declared-resource existence** (`--skip-resource-validation` opt-out, default on). Before sending the config to the node, the gate walks the rendered MachineConfig, extracts every reference to a host-side resource (network links from v1.12 multi-doc — `LinkConfig.name`, `BondConfig.links[]`, `VLANConfig.parent`, `BridgeConfig.links[]`, `Layer2VIPConfig.link`, `HCloudVIPConfig.link`, `DHCPv4Config.name` / `DHCPv6Config.name` / `EthernetConfig.name`; v1.11 legacy `machine.network.interfaces[].interface`; install disk via `machine.install.disk` literal or `machine.install.diskSelector`; `UserVolumeConfig.provisioning.diskSelector`), and verifies each against the node's COSI `LinkStatus`/`Disk` snapshots. A reference that doesn't resolve fails the apply with a `[blocker]` line listing the available names so the typo or migration miss is fixable from the values without re-running discovery. Disk selectors must match at least one (non-readonly, non-CDROM, non-virtual) disk — zero matches block, multiple matches warn (install picks the first). Virtual-link-creator documents (`BondConfig.name`, `VLANConfig.name`, `BridgeConfig.name`, `WireguardConfig.name`, `DummyLinkConfig.name`, `LinkAliasConfig.name`) are intentionally NOT validated against existing links — those `.name` fields describe new virtual links the apply is creating, not references to pre-existing host resources. Out of scope today: `machine.disks[].device` (extra-disk partitioning); track in a follow-up if you need it. Pass `--skip-resource-validation` for recovery into a maintenance image with mismatched hardware or pre-staging values for hardware that isn't installed yet. +> 1. **Declared-resource existence** (`--skip-resource-validation` opt-out, default on). Before sending the config to the node, the gate walks the rendered MachineConfig, extracts every reference to a host-side resource (network links from v1.12 multi-doc — `LinkConfig.name`, `BondConfig.links[]`, `VLANConfig.parent`, `BridgeConfig.links[]`, `Layer2VIPConfig.link`, `HCloudVIPConfig.link`, `DHCPv4Config.name` / `DHCPv6Config.name` / `EthernetConfig.name`; v1.11 legacy `machine.network.interfaces[].interface`; install disk via `machine.install.disk` literal or `machine.install.diskSelector`; `UserVolumeConfig.provisioning.diskSelector`), and verifies each against the node's COSI `LinkStatus`/`Disk` snapshots. A reference that doesn't resolve fails the apply with a `[blocker]` line listing the available names so the typo or migration miss is fixable from the values without re-running discovery. Disk selectors must match at least one (non-readonly, non-CDROM, non-virtual) disk — zero matches block, multiple matches warn (install picks the first). Virtual-link-creator documents (`BondConfig.name`, `VLANConfig.name`, `BridgeConfig.name`, `WireguardConfig.name`, `DummyLinkConfig.name`, `LinkAliasConfig.name`) are intentionally NOT validated against existing links — those `.name` fields describe new virtual links the apply is creating, not references to pre-existing host resources. The gate also runs a syntactic net-addr walker against `StaticHostConfig.name` (must parse as an IP literal — the `name` field on this kind doubles as the IP the hostnames map to), `NetworkRuleConfig.ingress[].subnet` and `.except` (per-entry CIDR), and `WireguardConfig.peers[].endpoint` (host:port; empty / absent endpoint is a listener-only peer, NOT a finding). Out of scope today: `machine.disks[].device` (extra-disk partitioning); track in a follow-up if you need it. Pass `--skip-resource-validation` for recovery into a maintenance image with mismatched hardware or pre-staging values for hardware that isn't installed yet. > -> 2. **Pre-apply drift preview** (`--skip-drift-preview` opt-out, default on). Reads the node's current MachineConfig via COSI and prints a `+`/`-`/`~`/`=` diff of what's about to change, keyed by `(kind, name)`. Informational only — never blocks. The `-` lines are the most useful: they surface stale documents from a previous apply that the new render no longer emits (e.g. an `eth1` LinkConfig lingering after a migration to `eth0`). Reading the current config requires the auth path — `MachineConfig` is a Sensitive COSI resource and is unreachable on the `--insecure` maintenance connection; the gate prints `drift verification unavailable on maintenance connection` and proceeds in that case. **`--dry-run` runs this gate** — the diff is read-only and "show me what would change" is exactly the dry-run contract. +> 2. **Pre-apply drift preview** (`--skip-drift-preview` opt-out, default on). Reads the node's current MachineConfig via COSI and prints a `+`/`-`/`~`/`=` diff of what's about to change, keyed by `(kind, name)`. Informational only — never blocks. The `-` lines are the most useful: they surface stale documents from a previous apply that the new render no longer emits (e.g. an `eth1` LinkConfig lingering after a migration to `eth0`). Reading the current config requires the auth path — `MachineConfig` is a Sensitive COSI resource and is unreachable on the `--insecure` maintenance connection; the gate prints `drift verification unavailable on maintenance connection` (per-node-prefixed on multi-node insecure apply) and proceeds in that case. Secret-bearing field values (`cluster.token`, `cluster.{ca,aggregatorCA,serviceAccount,etcd.ca}.key`, `machine.token` / `machine.ca.key`, the `cluster.acceptedCAs` / `machine.acceptedCAs` slices, `WireguardConfig.privateKey`, the `peers` slice carrying `presharedKey`s) are redacted by default — both sides render as `***redacted (len=N)***` so a rotation surfaces as different-length sentinels without leaking the value. Pass `--show-secrets-in-drift` to see the raw values verbatim (debugging only — disables the redaction for the run). **`--dry-run` runs this gate** — the diff is read-only and "show me what would change" is exactly the dry-run contract. > > 3. **Post-apply state verification** (`--skip-post-apply-verify` opt-out, **default off** until the Talos-mutated-field allowlist lands — see [#172](https://github.com/cozystack/talm/issues/172)). After `ApplyConfiguration` returns success, re-reads the on-node MachineConfig and structurally compares it against the bytes that were sent. Divergence blocks the apply chain with a per-document diff, primarily catching silent doc drops (Talos parser ignored an unknown field) and controller reverts. Disabled by default because Talos mutates a handful of leaf fields post-apply (cert hashes, timestamps) that would surface as false-positive divergence without an allowlist. The verify runs only on `--mode=no-reboot`. `--mode=staged`, `--mode=try`, `--mode=reboot`, and `--mode=auto` all skip the gate — each for a documented reason: staged stores rather than activates; try auto-rolls back; reboot kills the COSI connection mid-verify; auto is promoted by Talos to REBOOT internally when the change requires it, so the verify would race the reboot. `--dry-run` skips it too. > -> 4. **Post-upgrade version verify** (`--skip-post-upgrade-verify` opt-out, default on — the gate runs). After `talm upgrade` reports success, waits 90s for the node to finish booting then reads `runtime.Version` COSI and compares the running version's `(Major, Minor)` contract against the contract parsed from the target image tag. Point releases share a minor contract; cross-minor mismatch surfaces as a hint-bearing blocker. Catches the silent A/B rollback case where the upgrade RPC acks success but Talos rolled back to the previous partition (cross-vendor image, missing extensions, failed boot readiness check, slow boot exceeding the reconcile window). Best-effort surrender on digest-pinned images and unparseable tags. See [#175](https://github.com/cozystack/talm/issues/175) for the reproduction. +> 4. **Post-upgrade version verify** (`--skip-post-upgrade-verify` opt-out, default on — the gate runs). After `talm upgrade` reports success, waits the configured reconcile window (default 90s; tune via `--post-upgrade-reconcile-window` for slow hardware / large image pulls) for the node to finish booting, then reads `runtime.Version` COSI and compares the running version's `(Major, Minor)` contract against the contract parsed from the target image tag. Point releases share a minor contract; cross-minor mismatch surfaces as a hint-bearing blocker. Catches the silent A/B rollback case where the upgrade RPC acks success but Talos rolled back to the previous partition (cross-vendor image, missing extensions, failed boot readiness check, slow boot exceeding the configured window). Best-effort surrender on digest-pinned images and unparseable tags. See [#175](https://github.com/cozystack/talm/issues/175) for the reproduction. > > The skip flags don't suppress each other — pass them independently. On the `--insecure` (maintenance) path the gates are functionally unreachable for charts that drive discovery via `lookup` — those COSI lookups require an authenticated connection and the render itself errors before any gate runs. Charts that render fully offline (no `lookup` calls) reach the gates on `--insecure` as well, with the Phase 2 hooks degrading gracefully because the `MachineConfig` resource is Sensitive. diff --git a/docs/apply-safety-gates-test-plan.md b/docs/apply-safety-gates-test-plan.md index 72d894c..0e18ab4 100644 --- a/docs/apply-safety-gates-test-plan.md +++ b/docs/apply-safety-gates-test-plan.md @@ -47,6 +47,29 @@ Run all matrix cells against the binary at `/tmp/talm-safety`. Use a 3-node Talo | Boundary case (exactly 11) | 11 links on the host, bad ref | First 10 inline + `... and 1 more` (the suffix fires at >10, not at >=10) | | Empty candidate set | Selector matches zero, no real candidates either (mock) | Hint says `` rather than empty trailing space | +### Net-addr field references + +The Phase 1 walker validates the syntactic shape of net-addr fields in three v1alpha1 multidoc kinds. Pure syntactic — no host snapshot — runs alongside the Ref-based walker via `multidocNetAddrHandlers`. Field names match the actual Talos `network` schema (see `siderolabs/talos/pkg/machinery/config/types/network/`). + +| Case | How to trigger | Expected | +| --- | --- | --- | +| Bad `StaticHostConfig.name` | `StaticHostConfig{name: 999.999.0.1, hostnames: [foo]}` — the `name` field carries the IP literal in this kind | Blocker "StaticHostConfig.name is not a valid IP literal" with hint listing IPv4/IPv6 examples | +| Valid IPv4 / IPv6 | `name: 192.0.2.10` / `name: 2001:db8::1` | No finding | +| Missing `name` | Omit the field | No finding (Talos rejects at RPC with a clearer required-field message) | +| Hostname-shaped name | `name: example.invalid` | Blocker — `name` is required to be an IP literal, not a DNS name | +| Bad `NetworkRuleConfig.ingress[i].subnet` | `ingress: [{subnet: notacidr}]` | Per-entry blocker citing `ingress[i].subnet` | +| Bad `NetworkRuleConfig.ingress[i].except` | `ingress: [{subnet: 192.0.2.0/24, except: notacidr}]` | Blocker on `except` even when `subnet` is valid | +| Bare IP without /N | `ingress: [{subnet: 192.0.2.10}]` | Blocker — schema is CIDR-shaped, not IP-shaped | +| Valid CIDR mix | IPv4 + IPv6 CIDRs across `ingress[].subnet` | No findings | +| Bad `WireguardConfig.peers[].endpoint` | One peer `endpoint: notavalid:endpoint` | Per-peer blocker citing `peers[i].endpoint` | +| Valid IPv4:port | `endpoint: 192.0.2.10:51820` | No finding | +| Valid bracketed IPv6:port | `endpoint: "[2001:db8::1]:51820"` | No finding | +| Empty `endpoint` | `endpoint: ""` | No finding (peer is listener-only — this side does not initiate) | +| Missing `endpoint` field | Omit the field | No finding | +| Unknown multidoc kind | A new kind not in the dispatch map | No finding (Talos extensions / future kinds do not break the gate) | +| Real-schema pin | `TestWalkNetAddrFindings_RealSchema_StaticHostConfig` / `..._NetworkRuleConfig` feed the actual schema shape (`name` carrying the IP, `ingress[].subnet/except` for CIDRs) — the walker fires on what Talos emits, not on a hand-crafted YAML the schema doesn't produce | +| No-overlap pin | Adding a kind to both `multidocHandlers` AND `multidocNetAddrHandlers` | `TestMultidocNetAddrHandlers_NoOverlapWithRefHandlers` fails — double-walking would produce duplicate findings | + ### Opt-out | Case | Trigger | Expected | @@ -78,8 +101,28 @@ Run all matrix cells against the binary at `/tmp/talm-safety`. Use a 3-node Talo | `--mode=staged` | `talm apply --mode=staged -f node.yaml` | Phase 2A runs (operator still wants to see what got staged) | | `--mode=try` | `talm apply --mode=try -f node.yaml` | Phase 2A runs (mirrors --mode=auto from the preview's perspective) | | Insecure path | `talm apply -i -f node.yaml` (where chart can render offline) | `talm: drift verification unavailable on maintenance connection`; no block | +| Insecure path, multi-node | `talm apply -i --nodes a,b -f node.yaml` (each iteration through `openClientPerNodeMaintenance`) | Per-node-prefixed line `node a: talm: drift verification unavailable …` and `node b: talm: …` — disambiguation cohort over the maintenance-warning emission | +| Insecure path, single node | `talm apply -i -f node.yaml` with single `--nodes` | Still gets `node X: talm: …` prefix because `cosiPreflightContext` falls back to `GlobalArgs.Nodes[0]` when there is no outgoing-context metadata | +| Insecure path, empty nodeID | Unusual call shape with `GlobalArgs.Nodes` somehow empty | Bare `talm: drift verification unavailable …` line — never `node : …` garbage prefix | | `--skip-drift-preview` | Pass with any change | Preview suppressed entirely | +### Secret-bearing field redaction + +The drift preview redacts allowlisted paths by default. The opt-out is operator-explicit: `--show-secrets-in-drift`. Allowlist lives in `secretFieldPaths` (`pkg/commands/preflight_apply_safety_redact.go`). + +| Case | Trigger | Expected | +| --- | --- | --- | +| Cluster secret rotation | Change `cluster.token` via `secrets.yaml` rotation | `cluster.token: ***redacted (len=N)*** -> ***redacted (len=M)***` — value never appears in stderr | +| Machine token rotation | Change `machine.token` | Same shape; `machine.token: ***redacted (len=N)*** -> …` | +| Array-indexed secret | Change `cluster.acceptedCAs[2].key` | Bracket-normalised match against `cluster.acceptedCAs[].key`; redacted | +| Wireguard private key | Rotate `WireguardConfig.privateKey` | `privateKey: ***redacted (len=N)*** -> …` — bare path because the differ's flatten step does not prefix multidoc fields with the doc kind | +| Wireguard pre-shared key | Rotate `peers[2].presharedKey` | Bracket-normalised; redacted | +| Non-secret path | Change `machine.network.hostname` | Verbatim — operator-visible information is not redacted | +| False-prefix guard | A non-secret path sharing a prefix (`cluster.tokenExtras`) | Verbatim — `isSecretPath` is path-segment exact, not substring prefix | +| `--show-secrets-in-drift` | Pass with any secret rotation | Verbatim both sides on the secret line; sentinel never appears | +| Non-string secret value | Hypothetical schema drift puts an int on a secret-bearing path | `***redacted (len=N)***` where N is the `%v` length; rotation signal survives non-string types (caveat: maps render with non-deterministic key order — disclaimed in the godoc) | +| Slice-shaped secret path | Hypothetical future allowlist entry naming an array | Redacted via the secret check that runs BEFORE `bothSlices` — elements never leak through `formatSliceSetDiff` | + ### Output pretty-print | Case | Trigger | Expected | @@ -126,6 +169,9 @@ On by default for `talm upgrade`. The gate fires after talosctl upgrade returns | By-design unreachable | Reader returns `("", false, nil)` (cosiVersionReader does not produce this; reserved for future custom readers that need to surrender silently) | Soft warning line `post-upgrade verification skipped, could not read running version from the node`, no block. Distinguishable from the real-read-failure case via the err — three-valued contract makes the contract explicit | | Zero target nodes | `--nodes` empty and talosconfig context has no nodes either | Explanatory "skipped, no target nodes resolved" line (no silent no-op) | | Reconcile wait line | Any non-skipped run | "post-upgrade verify: waiting 1m30s for the node to finish booting..." printed up front so the operator's terminal isn't a mystery hang | +| Configurable reconcile window | `talm upgrade --post-upgrade-reconcile-window=180s …` | "post-upgrade verify: waiting 3m0s for the node to finish booting..." — Go's `time.Duration.String()` renders 180s deterministically as `3m0s`. Hint copy references "the configured reconcile window (`--post-upgrade-reconcile-window`)" instead of the hardcoded "90s reconcile window" wording | +| Window default | `talm upgrade --help` | Flag listed with `default 1m30s`; the const `defaultPostUpgradeReconcileWindow` preserves the previous hardcoded 90s for byte-identical back-compat | +| Window non-positive | `talm upgrade --post-upgrade-reconcile-window=0s …` | Fail-fast error with hint mentioning "positive duration" — validation runs at the TOP of `wrapUpgradeCommand` RunE so the talosctl upgrade RPC never fires. Same shape for `-30s` (negative). Pinned by `TestWrapUpgradeCommand_BadReconcileWindow_FailsFastBeforeOriginalRunE` which asserts the sentinel `originalRunE` stays uninvoked | ## Real-Talos validation diff --git a/docs/manual-test-plan.md b/docs/manual-test-plan.md index c29af61..7908aef 100644 --- a/docs/manual-test-plan.md +++ b/docs/manual-test-plan.md @@ -184,6 +184,39 @@ Expected: each node renders / diffs independently; per-node gate output sections Expected: Phase 2B auto-skipped (staged config doesn't change ActiveID); output ends with `Staged configuration to be applied after the next reboot`. +### C5. Drift preview redacts secret-bearing fields by default + +```bash +# Rotate machine.token by editing secrets.yaml (or any allowlisted path) then: +/tmp/talm-safety apply --dry-run -f nodes/node0.yaml +``` + +Expected: the drift preview line for `machine.token` reads `machine.token: ***redacted (len=N)*** -> ***redacted (len=M)***`. The literal `old-token-value` / `new-token-value` strings MUST NOT appear in stderr. Non-secret paths (e.g. `machine.network.hostname` if it changed) render verbatim. + +Regression anchor: rotating any field in the allowlist (`cluster.{secret,token,aescbcEncryptionSecret,secretboxEncryptionSecret}`, `cluster.{ca,aggregatorCA,serviceAccount,etcd.ca}.key`, `cluster.acceptedCAs[].key`, `machine.{token,ca.key}`, `machine.acceptedCAs[].key`) MUST redact. A regression that silently leaks a secret value into stderr is a security-class bug — verify the substring with `grep -F` against the captured output. + +### C6. Drift preview shows secrets with explicit opt-in + +```bash +/tmp/talm-safety apply --dry-run --show-secrets-in-drift -f nodes/node0.yaml +``` + +Expected: same drift preview as C5, but the secret paths render verbatim — no `***redacted***` sentinel. Operator-explicit bypass for debugging. + +Regression anchor: `--show-secrets-in-drift` is operator opt-in, never default. Verify by running `talm apply --help` and confirming the flag default is `false`. + +### C7. Phase 1 walker rejects malformed net-addr fields before the RPC + +When a rendered MachineConfig carries a malformed value in any of the new walker-covered fields, Phase 1 must block before the apply RPC fires: + +- `StaticHostConfig.name` not a parseable IP literal (the `name` field on this kind is the IP the hostnames map to — Talos's schema does not have a separate `address` field). +- `NetworkRuleConfig.ingress[i].subnet` or `.except` not a parseable CIDR. +- `WireguardConfig.peers[i].endpoint` not a parseable host:port. + +Hand-craft a chart that emits a bad value (e.g. `name: 999.999.0.1` on a `StaticHostConfig`, or `ingress: [{subnet: notacidr}]` on a `NetworkRuleConfig`, or `endpoint: notavalid:endpoint` on a Wireguard peer) and run `apply --dry-run`. Expected: Phase 1 emits a blocker citing the offending field path (`doc[N].name`, `doc[N].ingress[i].subnet` or `.except`, `doc[N].peers[i].endpoint`); exit non-zero before any RPC. Valid values (IPv4, IPv6, IPv6:port via `[host]:port`) pass through. + +Regression anchor: empty / omitted endpoint on a Wireguard peer is NOT a finding — peers without endpoints are listener-only remote peers. Verify a chart with `endpoint: ""` passes Phase 1. + ## D. Apply (insecure / maintenance path) ### D1. Apply with chart that uses discovery @@ -200,6 +233,21 @@ When a chart renders fully offline (no `lookup`), `talm apply -i` runs through t **Regression anchor**: D2's offline-renderable behaviour is also covered by unit-level mocking — see `pkg/commands/preflight_apply_safety_test.go` for the in-process equivalent. Surface that file's tests in the manual suite when D2 is impractical to exercise live. +### D3. Per-node prefix on the maintenance-connection warning + +On a multi-node insecure apply where every node hits the `ok=false` (maintenance) path, each per-node emission of the warning must carry the node identifier prefix so the operator can correlate which line came from which node: + +```bash +/tmp/talm-safety apply -i \ + --nodes 192.0.2.10,192.0.2.11,192.0.2.12 \ + --endpoints 192.0.2.10,192.0.2.11,192.0.2.12 \ + -f nodes/node0.yaml +``` + +Expected (per node): `node 192.0.2.10: talm: drift verification unavailable on maintenance connection`. The single-node case (empty `nodeID`, the implicit path) MUST still emit the bare `talm: drift verification unavailable on maintenance connection` line — no `node : ` garbage prefix. + +Regression anchor: a refactor that always-prefixes (`node : talm: ...` on single-node) is a UX regression. The `nodePrefix("")` helper must collapse to empty for the bare-line single-node case. + ## E. Upgrade ### E1. Stage an upgrade to the same image @@ -221,6 +269,26 @@ Expected: events stream from BOOTING through `post check passed`. Node returns t Expected: `error validating installer image ... not found`. Talos itself catches this; talm passes through the error. +### E3. Configurable post-upgrade reconcile window + +```bash +# Help-text surface — confirms the flag is registered with the 90s default. +/tmp/talm-safety upgrade --help | grep -A1 post-upgrade-reconcile-window + +# Custom widened window (slow hardware / large image pulls). +/tmp/talm-safety upgrade --post-upgrade-reconcile-window=180s \ + --image ghcr.io/siderolabs/installer:v1.13.0 \ + --stage -f nodes/node0.yaml + +# Rejection of non-positive values. +/tmp/talm-safety upgrade --post-upgrade-reconcile-window=0s \ + -f nodes/node0.yaml +``` + +Expected for the help line: flag listed with `default 1m30s`. Expected for the 180s run: stderr emits `post-upgrade verify: waiting 3m0s for the node to finish booting...` — Go's `time.Duration.String()` renders `180 * time.Second` as `3m0s` deterministically, not `180s`. Expected for the `0s` rejection: error with a hint mentioning "positive duration". + +Regression anchor: the version-mismatch hint emitted on a Phase 2C blocker MUST NOT contain the literal string `90s` — operators passing a custom window would see contradictory advice. The hint should reference "the configured reconcile window (`--post-upgrade-reconcile-window`)" instead. + ## F. CA rotation ### F1. Rotate CA dry-run @@ -766,6 +834,32 @@ TALOSCONFIG=$PWD/talosconfig /tmp/talm-safety apply --dry-run \ Expected: same as native `--talosconfig $PWD/talosconfig`. Phase 2A drift preview runs normally. +### M6. Secret redaction false-positive guard (intentional rotation) + +When an operator deliberately rotates a secret (e.g. `cluster.token` via `talm init --update`), the drift preview must render both sides as `***redacted (len=N)***` — same shape as C5. The control case lives here: confirm a "rotation" of a non-secret-shaped path adjacent to the allowlist (`cluster.tokenExtras`, `cluster.acceptedCAsExtras`, or a synthetic test path like `machine.network.hostname`) renders verbatim. + +Expected: paths matching `cluster.token` → redacted; paths matching `cluster.tokenExtras` → verbatim. The path-segment-aware matcher must not false-positive on string-prefix overlap. + +Regression anchor: a future regression to substring matching (`strings.HasPrefix(path, "cluster.token")`) would silently redact `cluster.tokenExtras` and other operator-visible fields that share a prefix. Verify by inspecting a chart with both shapes side-by-side. + +### M7. Net-addr walker boundary cases + +Walk the net-addr walker (C7) on the full boundary set: + +- `StaticHostConfig.name: 2001:db8::1` — valid IPv6, passes. +- `StaticHostConfig.name: 192.0.2.999` — IPv4 with octet >255, blocks. +- `StaticHostConfig` with no `name` field — passes (Talos rejects at RPC with a clearer message about required fields). +- `NetworkRuleConfig.ingress: [{subnet: 192.0.2.0/24}, {subnet: 2001:db8::/32}]` — mixed IPv4 + IPv6 CIDRs, both pass. +- `NetworkRuleConfig.ingress: [{subnet: 192.0.2.0/24}, {subnet: notacidr}]` — one blocker per malformed entry; the count must equal exactly one. +- `NetworkRuleConfig.ingress: [{subnet: 192.0.2.0/24, except: notacidr}]` — `except` validated alongside `subnet`; malformed `except` blocks even when `subnet` is valid. +- `WireguardConfig.peers[].endpoint: "[2001:db8::1]:51820"` — bracketed IPv6:port, passes. +- `WireguardConfig.peers[].endpoint: ""` — listener-only peer, passes. +- `WireguardConfig.peers[].endpoint: example.invalid:51820` — hostname:port, blocks (hostnames must already be resolved in the rendered config). + +Expected: per-entry findings count exactly, valid forms produce zero findings, and the per-finding `Reason` cites the field path with the bracket-normalised index (`peers[1].endpoint`, not `peers[].endpoint`). + +Regression anchor: the no-overlap unit test `TestMultidocNetAddrHandlers_NoOverlapWithRefHandlers` pins the dispatch-map disjointness contract. A future entry that lands in BOTH `multidocHandlers` and `multidocNetAddrHandlers` produces double findings — verify via the unit suite before manual smokes. + ## Sanity-check block Run after every destructive section (E, F, H, and anything that touches `--mode=reboot` / `--mode=staged` / `apply -I`): diff --git a/pkg/applycheck/refs_netaddr.go b/pkg/applycheck/refs_netaddr.go new file mode 100644 index 0000000..03db0ba --- /dev/null +++ b/pkg/applycheck/refs_netaddr.go @@ -0,0 +1,265 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package applycheck + +import ( + "bytes" + "fmt" + "io" + "net/netip" + "strconv" + + "github.com/cockroachdb/errors" + yaml "gopkg.in/yaml.v3" +) + +// netAddrHandler emits syntactic net-addr findings for one v1alpha1 +// multidoc kind. Handlers are registered in multidocNetAddrHandlers +// and dispatched by WalkNetAddrFindings. +type netAddrHandler func(doc map[string]any, basePath string) []Finding + +//nolint:gochecknoglobals // dispatch table for syntactic net-addr handlers; static after init. +var multidocNetAddrHandlers = map[string]netAddrHandler{ + "StaticHostConfig": handleStaticHostConfigName, + "NetworkRuleConfig": handleNetworkRuleConfigIngress, + "WireguardConfig": handleWireguardEndpoints, +} + +// WalkNetAddrFindings parses the rendered MachineConfig bytes and +// returns a Finding for every malformed net-addr field in the three +// kinds registered above. Pure syntactic — no host snapshot +// required — so the walker runs in Phase 1 alongside (not inside) +// the Ref-based walker. +// +// Empty input and YAML decode of a nil document are no-ops; an +// actual YAML parse error is wrapped and returned. Unknown kinds +// are ignored so future Talos kinds and vendor extensions do not +// trip the gate. +func WalkNetAddrFindings(rendered []byte) ([]Finding, error) { + if len(bytes.TrimSpace(rendered)) == 0 { + return nil, nil + } + + dec := yaml.NewDecoder(bytes.NewReader(rendered)) + + var findings []Finding + + for docIndex := 0; ; docIndex++ { + var doc map[string]any + + err := dec.Decode(&doc) + if errors.Is(err, io.EOF) { + break + } + + if err != nil { + return nil, errors.Wrapf(err, "applycheck: decoding YAML document %d for net-addr walk", docIndex) + } + + if doc == nil { + continue + } + + kind, ok := doc["kind"].(string) + if !ok { + continue + } + + handler, ok := multidocNetAddrHandlers[kind] + if !ok { + continue + } + + findings = append(findings, handler(doc, fmt.Sprintf("doc[%d]", docIndex))...) + } + + return findings, nil +} + +// handleStaticHostConfigName validates StaticHostConfig.name as a +// parseable IP literal (IPv4 or IPv6). In Talos's v1alpha1 schema +// `StaticHostConfigV1Alpha1`, the IP literal lives in the `name` +// field (the document's meta-name doubling as the IP to which +// the host entries point) — there is NO separate `address` field, +// despite what one might assume from the docname. +// +// Missing or non-string names are silently skipped — Talos rejects +// those at the RPC layer with a clearer kind-specific message; the +// walker only catches present-but-malformed values. +func handleStaticHostConfigName(doc map[string]any, basePath string) []Finding { + nameAny, ok := doc["name"] + if !ok { + return nil + } + + nameStr, ok := nameAny.(string) + if !ok { + return nil + } + + if nameStr == "" { + return nil + } + + if _, err := netip.ParseAddr(nameStr); err == nil { + return nil + } + + path := basePath + ".name" + + return []Finding{{ + Ref: Ref{ + Name: nameStr, + Source: path, + }, + Severity: SeverityBlocker, + Reason: "StaticHostConfig.name is not a valid IP literal: " + quote(nameStr), + Hint: "expected IPv4 (e.g. 192.0.2.10) or IPv6 (e.g. 2001:db8::1); the `name` field on StaticHostConfig is the IP literal the hostnames map to", + }} +} + +// handleNetworkRuleConfigIngress validates the CIDR-shaped fields +// inside NetworkRuleConfig.ingress[]. In Talos's v1alpha1 schema +// `RuleConfigV1Alpha1`, each entry of `ingress` is an `IngressRule` +// with `subnet` (required CIDR) and `except` (optional CIDR). There +// is NO top-level `matchSourceAddress[]` field, despite operator +// intuition. +// +// A bare IP without /N is NOT accepted — Talos's contract is +// CIDR-shaped at this position. Missing ingress / empty list / +// missing subnet on a rule are no-ops at this layer (Talos +// validates structural requireds at the RPC). +func handleNetworkRuleConfigIngress(doc map[string]any, basePath string) []Finding { + listAny, ok := doc["ingress"] + if !ok { + return nil + } + + list, ok := listAny.([]any) + if !ok { + return nil + } + + var findings []Finding + + for i, entry := range list { + ruleMap, ok := entry.(map[string]any) + if !ok { + continue + } + + findings = appendCIDRFindingIfPresent(findings, ruleMap, "subnet", basePath, i) + findings = appendCIDRFindingIfPresent(findings, ruleMap, "except", basePath, i) + } + + return findings +} + +// appendCIDRFindingIfPresent reads ingress[i]. from the rule +// map and appends a finding when the value is a present-but- +// malformed string. Missing or empty values are no-ops (Talos +// requires `subnet`; absence triggers a clearer RPC error). Used +// for both `subnet` and `except`. +func appendCIDRFindingIfPresent(findings []Finding, rule map[string]any, field, basePath string, i int) []Finding { + valAny, ok := rule[field] + if !ok { + return findings + } + + cidrStr, ok := valAny.(string) + if !ok { + return findings + } + + if cidrStr == "" { + return findings + } + + if _, err := netip.ParsePrefix(cidrStr); err == nil { + return findings + } + + path := fmt.Sprintf("%s.ingress[%d].%s", basePath, i, field) + + return append(findings, Finding{ + Ref: Ref{ + Name: cidrStr, + Source: path, + }, + Severity: SeverityBlocker, + Reason: "NetworkRuleConfig.ingress[" + strconv.Itoa(i) + "]." + field + " is not a valid CIDR: " + quote(cidrStr), + Hint: "expected CIDR like 192.0.2.0/24 or 2001:db8::/32; a bare IP without /N is not accepted by Talos's NetworkRuleConfig schema", + }) +} + +// handleWireguardEndpoints validates each peer's endpoint as a +// parseable host:port literal. Empty or missing endpoint values +// describe a listener-only remote peer (this side accepts but does +// not initiate) and are intentionally NOT findings. +// +// netip.ParseAddrPort accepts both IPv4 host:port and bracketed +// IPv6 [host]:port — the canonical Wireguard endpoint shapes. +func handleWireguardEndpoints(doc map[string]any, basePath string) []Finding { + peersAny, ok := doc["peers"] + if !ok { + return nil + } + + peers, ok := peersAny.([]any) + if !ok { + return nil + } + + var findings []Finding + + for i, peer := range peers { + peerMap, ok := peer.(map[string]any) + if !ok { + continue + } + + endpointAny, ok := peerMap["endpoint"] + if !ok { + continue + } + + endpointStr, ok := endpointAny.(string) + if !ok { + continue + } + + if endpointStr == "" { + continue + } + + if _, err := netip.ParseAddrPort(endpointStr); err == nil { + continue + } + + path := fmt.Sprintf("%s.peers[%d].endpoint", basePath, i) + + findings = append(findings, Finding{ + Ref: Ref{ + Name: endpointStr, + Source: path, + }, + Severity: SeverityBlocker, + Reason: "WireguardConfig.peers[" + strconv.Itoa(i) + "].endpoint is not a valid host:port: " + quote(endpointStr), + Hint: "expected IPv4:port (e.g. 192.0.2.10:51820) or [IPv6]:port (e.g. [2001:db8::1]:51820); hostnames must be resolved to a literal IP in the rendered config", + }) + } + + return findings +} diff --git a/pkg/applycheck/refs_netaddr_test.go b/pkg/applycheck/refs_netaddr_test.go new file mode 100644 index 0000000..ee16055 --- /dev/null +++ b/pkg/applycheck/refs_netaddr_test.go @@ -0,0 +1,380 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package applycheck + +import ( + "strings" + "testing" +) + +// TestWalkNetAddrFindings_StaticHostConfig pins the contract for +// StaticHostConfig.name validation. In Talos's v1alpha1 schema +// (StaticHostConfigV1Alpha1) the IP literal lives in the `name` +// field — the document's meta-name doubling as the IP the +// hostnames map to. There is NO separate `address` field. Tests +// feed the schema's actual shape (the `name:` line carries the IP) +// so the walker fires on what Talos really emits, not on a +// hand-crafted shape that would never appear on the wire. +func TestWalkNetAddrFindings_StaticHostConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + yaml string + wantFinding bool + }{ + { + name: "valid IPv4 in name", + yaml: "apiVersion: v1alpha1\nkind: StaticHostConfig\nname: 192.0.2.10\nhostnames: [foo.example]\n", + wantFinding: false, + }, + { + name: "valid IPv6 in name", + yaml: "apiVersion: v1alpha1\nkind: StaticHostConfig\nname: 2001:db8::1\nhostnames: [foo.example]\n", + wantFinding: false, + }, + { + name: "malformed IPv4 in name (octet >255)", + yaml: "apiVersion: v1alpha1\nkind: StaticHostConfig\nname: 999.999.0.1\nhostnames: [foo.example]\n", + wantFinding: true, + }, + { + name: "hostname-shaped name (not an IP)", + yaml: "apiVersion: v1alpha1\nkind: StaticHostConfig\nname: example.invalid\nhostnames: [foo.example]\n", + wantFinding: true, + }, + { + name: "missing name field — no finding", + yaml: "apiVersion: v1alpha1\nkind: StaticHostConfig\nhostnames: [foo.example]\n", + wantFinding: false, + }, + { + name: "non-string name (int) — no finding", + yaml: "apiVersion: v1alpha1\nkind: StaticHostConfig\nname: 42\nhostnames: [foo.example]\n", + wantFinding: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + findings, err := WalkNetAddrFindings([]byte(tc.yaml)) + if err != nil { + t.Fatalf("WalkNetAddrFindings: %v", err) + } + + gotFinding := len(findings) > 0 + if gotFinding != tc.wantFinding { + t.Errorf("findings=%v, want finding=%v; got: %+v", gotFinding, tc.wantFinding, findings) + } + + if tc.wantFinding && len(findings) > 0 { + f := findings[0] + if f.Severity != SeverityBlocker { + t.Errorf("StaticHostConfig.name malformed should be a blocker, got %v", f.Severity) + } + + if !strings.Contains(f.Reason, "StaticHostConfig.name") { + t.Errorf("Reason should name the field; got %q", f.Reason) + } + } + }) + } +} + +// TestWalkNetAddrFindings_NetworkRuleConfig pins the CIDR validation +// for ingress[].subnet and ingress[].except. In Talos's v1alpha1 +// schema (RuleConfigV1Alpha1) the CIDR-shaped fields live inside +// each `ingress` entry as `subnet` (required) and `except` +// (optional) — NOT at a top-level `matchSourceAddress[]`. Tests +// feed the real shape. +func TestWalkNetAddrFindings_NetworkRuleConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + yaml string + wantFindingCount int + }{ + { + name: "all valid subnets", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r1\n" + + "ingress:\n - subnet: 192.0.2.0/24\n - subnet: 2001:db8::/32\n - subnet: 10.0.0.0/8\n", + wantFindingCount: 0, + }, + { + name: "subnet with except (both valid)", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r2\n" + + "ingress:\n - subnet: 192.0.2.0/24\n except: 192.0.2.128/25\n", + wantFindingCount: 0, + }, + { + name: "one malformed subnet in a list of three", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r3\n" + + "ingress:\n - subnet: 192.0.2.0/24\n - subnet: notacidr\n - subnet: 10.0.0.0/8\n", + wantFindingCount: 1, + }, + { + name: "two malformed subnets", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r4\n" + + "ingress:\n - subnet: 192.0.2.999/24\n - subnet: notacidr\n", + wantFindingCount: 2, + }, + { + name: "bare IP without /N in subnet", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r5\n" + + "ingress:\n - subnet: 192.0.2.10\n", + wantFindingCount: 1, + }, + { + name: "malformed except next to valid subnet", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r6\n" + + "ingress:\n - subnet: 192.0.2.0/24\n except: notacidr\n", + wantFindingCount: 1, + }, + { + name: "empty ingress list — no finding", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r7\n" + + "ingress: []\n", + wantFindingCount: 0, + }, + { + name: "missing ingress field — no finding", + yaml: "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: r8\n", + wantFindingCount: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + findings, err := WalkNetAddrFindings([]byte(tc.yaml)) + if err != nil { + t.Fatalf("WalkNetAddrFindings: %v", err) + } + + if len(findings) != tc.wantFindingCount { + t.Errorf("got %d findings, want %d; findings: %+v", len(findings), tc.wantFindingCount, findings) + } + + for i := range findings { + f := &findings[i] + if !strings.Contains(f.Reason, "ingress") { + t.Errorf("Reason should name the ingress path; got %q", f.Reason) + } + } + }) + } +} + +// TestWalkNetAddrFindings_WireguardConfig pins peers[].endpoint +// validation. Empty endpoint is NOT a finding — peers without an +// endpoint are listener-only remote peers (this side won't initiate; +// it will accept connections from the peer). Malformed endpoints +// (missing port, bad IP, plain hostname) are blockers. +func TestWalkNetAddrFindings_WireguardConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + yaml string + wantFindingCount int + }{ + { + name: "valid IPv4 host:port", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg0\n" + + "peers:\n - publicKey: AAA\n endpoint: 192.0.2.10:51820\n", + wantFindingCount: 0, + }, + { + name: "valid IPv6 [host]:port", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg1\n" + + "peers:\n - publicKey: BBB\n endpoint: \"[2001:db8::1]:51820\"\n", + wantFindingCount: 0, + }, + { + name: "missing port", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg2\n" + + "peers:\n - publicKey: CCC\n endpoint: 192.0.2.10\n", + wantFindingCount: 1, + }, + { + name: "hostname:port (not IP) — flagged", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg3\n" + + "peers:\n - publicKey: DDD\n endpoint: example.invalid:51820\n", + wantFindingCount: 1, + }, + { + name: "empty endpoint — listener-only peer, no finding", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg4\n" + + "peers:\n - publicKey: EEE\n endpoint: \"\"\n", + wantFindingCount: 0, + }, + { + name: "missing endpoint field — listener-only peer, no finding", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg5\n" + + "peers:\n - publicKey: FFF\n", + wantFindingCount: 0, + }, + { + name: "two peers, one malformed", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg6\n" + + "peers:\n - publicKey: GGG\n endpoint: 192.0.2.10:51820\n" + + " - publicKey: HHH\n endpoint: bad:notanumber\n", + wantFindingCount: 1, + }, + { + name: "no peers list — no finding", + yaml: "apiVersion: v1alpha1\nkind: WireguardConfig\nname: wg7\n", + wantFindingCount: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + findings, err := WalkNetAddrFindings([]byte(tc.yaml)) + if err != nil { + t.Fatalf("WalkNetAddrFindings: %v", err) + } + + if len(findings) != tc.wantFindingCount { + t.Errorf("got %d findings, want %d; findings: %+v", len(findings), tc.wantFindingCount, findings) + } + + for i := range findings { + f := &findings[i] + if !strings.Contains(f.Reason, "endpoint") { + t.Errorf("Reason should name 'endpoint' for peers[].endpoint findings; got %q", f.Reason) + } + } + }) + } +} + +// TestWalkNetAddrFindings_UnknownKind_NoFinding pins the no-op +// behaviour for kinds outside the dispatch map. The net-addr walker +// must never error on an unknown kind — Talos extensions and future +// kinds should not break the gate. +func TestWalkNetAddrFindings_UnknownKind_NoFinding(t *testing.T) { + t.Parallel() + + yaml := "apiVersion: v1alpha1\nkind: SomeFutureKind\nname: x\naddress: bogus\n" + + findings, err := WalkNetAddrFindings([]byte(yaml)) + if err != nil { + t.Fatalf("WalkNetAddrFindings: %v", err) + } + + if len(findings) != 0 { + t.Errorf("unknown kind must produce no findings; got %+v", findings) + } +} + +// TestWalkNetAddrFindings_EmptyInput_NoError pins the empty-bytes path: +// zero-length input is not a YAML decode error; it's the trivial +// "nothing to walk" case. +func TestWalkNetAddrFindings_EmptyInput_NoError(t *testing.T) { + t.Parallel() + + findings, err := WalkNetAddrFindings(nil) + if err != nil { + t.Errorf("empty input should not error; got %v", err) + } + + if len(findings) != 0 { + t.Errorf("empty input should produce no findings; got %+v", findings) + } +} + +// TestWalkNetAddrFindings_RealSchema_StaticHostConfig pins the +// walker against the actual v1alpha1 schema shape that the Talos +// machinery package emits. The original walker iteration assumed +// fields that don't exist in StaticHostConfigV1Alpha1 (e.g. +// `address`) and tests with hand-crafted YAML passed because they +// were self-consistent. A YAML body that matches the real schema +// (`apiVersion`, `kind`, `name` carrying the IP) must trigger the +// walker on a malformed IP literal — without this pin, the walker +// could silently revert to validating non-existent fields again. +func TestWalkNetAddrFindings_RealSchema_StaticHostConfig(t *testing.T) { + t.Parallel() + + // Schema-shape body: name field carries the IP literal, hostnames + // list is unrelated. + yamlBody := "apiVersion: v1alpha1\nkind: StaticHostConfig\nname: 999.999.0.1\nhostnames:\n - foo.example\n - bar.example\n" + + findings, err := WalkNetAddrFindings([]byte(yamlBody)) + if err != nil { + t.Fatalf("WalkNetAddrFindings on real-schema body: %v", err) + } + + if len(findings) != 1 { + t.Fatalf("real-schema body with malformed name must produce exactly one finding; got %d: %+v", len(findings), findings) + } + + if !strings.Contains(findings[0].Ref.Source, "name") || strings.Contains(findings[0].Ref.Source, "address") { + t.Errorf("finding source path must reference `name`, not `address`; got %q", findings[0].Ref.Source) + } +} + +// TestWalkNetAddrFindings_RealSchema_NetworkRuleConfig pins the +// walker against the actual ingress[].subnet shape. Original +// walker iteration validated a top-level `matchSourceAddress[]` +// that doesn't exist in RuleConfigV1Alpha1; this pin keeps the +// walker aligned with Talos's real type. +func TestWalkNetAddrFindings_RealSchema_NetworkRuleConfig(t *testing.T) { + t.Parallel() + + yamlBody := "apiVersion: v1alpha1\nkind: NetworkRuleConfig\nname: rule1\n" + + "portSelector:\n ports: [22]\n protocol: tcp\n" + + "ingress:\n - subnet: notacidr\n - subnet: 192.0.2.0/24\n except: 192.0.2.999/30\n" + + findings, err := WalkNetAddrFindings([]byte(yamlBody)) + if err != nil { + t.Fatalf("WalkNetAddrFindings on real-schema body: %v", err) + } + + // Two malformed: ingress[0].subnet and ingress[1].except. + if len(findings) != 2 { + t.Fatalf("expected exactly 2 findings (ingress[0].subnet + ingress[1].except); got %d: %+v", len(findings), findings) + } + + for i := range findings { + src := findings[i].Ref.Source + if !strings.Contains(src, "ingress") || strings.Contains(src, "matchSourceAddress") { + t.Errorf("finding source path must reference `ingress`, not `matchSourceAddress`; got %q", src) + } + } +} + +// TestMultidocNetAddrHandlers_NoOverlapWithRefHandlers pins the +// dispatch-map disjointness contract: net-addr handlers run in a +// parallel walker, so a kind that appears in BOTH maps would get +// double-walked (one finding from each pipeline) — silent +// duplication. None of the three net-addr kinds (StaticHostConfig, +// NetworkRuleConfig, WireguardConfig) are in multidocHandlers today; +// pin that contract so a future entry doesn't create overlap. +func TestMultidocNetAddrHandlers_NoOverlapWithRefHandlers(t *testing.T) { + t.Parallel() + + for kind := range multidocNetAddrHandlers { + if _, exists := multidocHandlers[kind]; exists { + t.Errorf("kind %q registered in BOTH multidocHandlers (ref-based) and multidocNetAddrHandlers (syntactic) — duplicate findings; pick one pipeline", kind) + } + } +} diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index ceb465c..8030bb7 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -67,6 +67,7 @@ var applyCmdFlags struct { skipResourceValidation bool skipDriftPreview bool skipPostApplyVerify bool + showSecretsInDrift bool } //nolint:gochecknoglobals // cobra command, idiomatic for cobra-based CLIs @@ -404,7 +405,7 @@ func runPreApplyGates(ctx context.Context, c *client.Client, rendered []byte, no return nil } - return previewDrift(ctx, cosiMachineConfigReader(c, applyCmdFlags.insecure), rendered, nodeID, w) + return previewDrift(ctx, cosiMachineConfigReader(c, applyCmdFlags.insecure), rendered, nodeID, w, applyCmdFlags.showSecretsInDrift) } // shouldRunDriftPreview is the testable predicate for Phase 2A @@ -441,7 +442,7 @@ func runPostApplyGate(ctx context.Context, c *client.Client, sent []byte, nodeID return nil } - return verifyAppliedState(ctx, cosiMachineConfigReader(c, applyCmdFlags.insecure), sent, nodeID, w) + return verifyAppliedState(ctx, cosiMachineConfigReader(c, applyCmdFlags.insecure), sent, nodeID, w, applyCmdFlags.showSecretsInDrift) } // shouldRunPostApplyVerify is the testable predicate for runPostApplyGate. @@ -670,8 +671,20 @@ func openClientPerNodeAuth(parentCtx context.Context, c *client.Client) openClie // ctx through to a COSI call that apid will reject — the latter is // the exact silent no-op this helper exists to prevent. func cosiPreflightContext(ctx context.Context) (context.Context, string, error) { + //nolint:varnamelen // 'md' is the canonical short name for grpc metadata.MD across the codebase. md, ok := metadata.FromOutgoingContext(ctx) if !ok { + // Maintenance / insecure path: openClientPerNodeMaintenance + // pins the current node in GlobalArgs.Nodes but does not + // attach outgoing-context metadata (the maintenance client + // reads node directly from GlobalArgs). Fall back to the + // single-element GlobalArgs.Nodes so per-node-prefixed + // stderr lines (drift / divergence / maintenance warning) + // still disambiguate. + if len(GlobalArgs.Nodes) == 1 { + return ctx, GlobalArgs.Nodes[0], nil + } + return ctx, "", nil } @@ -881,6 +894,7 @@ func init() { applyCmd.Flags().BoolVar(&applyCmdFlags.skipResourceValidation, "skip-resource-validation", false, "skip the pre-apply check that declared host resources (links, disks) exist on the target node") applyCmd.Flags().BoolVar(&applyCmdFlags.skipDriftPreview, "skip-drift-preview", false, "skip the pre-apply diff of on-node vs rendered MachineConfig") applyCmd.Flags().BoolVar(&applyCmdFlags.skipPostApplyVerify, "skip-post-apply-verify", true, "skip the post-apply structural verification of on-node vs sent MachineConfig (default skip until the Talos-mutated field allowlist lands; see #172)") + applyCmd.Flags().BoolVar(&applyCmdFlags.showSecretsInDrift, "show-secrets-in-drift", false, "show secret-bearing field values verbatim in drift preview / post-apply verify output (default: redacted; cluster.token, cluster.ca.key, machine.token, Wireguard private keys, etc.)") helpers.AddModeFlags(&applyCmdFlags.Mode, applyCmd) addCommand(applyCmd) diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index f953b5b..74ef1db 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -772,6 +772,68 @@ func TestCosiPreflightContext_LeavesNoMetadataAlone(t *testing.T) { } } +// TestCosiPreflightContext_NoMetadata_FallsBackToGlobalArgsNodes pins +// the per-node prefix on the maintenance / insecure path. Maintenance +// flow goes through openClientPerNodeMaintenance which mutates +// GlobalArgs.Nodes to the singular target node but does NOT attach +// outgoing-context metadata — apid's maintenance client reads node +// from GlobalArgs directly. Without this fallback, cosiPreflightContext +// would return nodeID="" on the insecure path even with --nodes set +// explicitly, and the per-node prefix on the drift-preview / +// maintenance-warning lines silently disappears. +// +// Surfaced by real-env testing: `talm apply -i --skip-resource- +// validation` against a healthy node produced the bare line `talm: +// drift verification unavailable on maintenance connection` instead +// of the node-prefixed form. The unit suite passed cleanly with +// that regression in place because the synthetic tests for +// previewDrift call the function with an explicit nodeID arg — +// they don't exercise the cosiPreflightContext → previewDrift +// wiring. +func TestCosiPreflightContext_NoMetadata_FallsBackToGlobalArgsNodes(t *testing.T) { + saved := append([]string(nil), GlobalArgs.Nodes...) + + t.Cleanup(func() { GlobalArgs.Nodes = saved }) + + GlobalArgs.Nodes = []string{"192.0.2.10"} + + in := context.Background() + + _, nodeID, err := cosiPreflightContext(in) + if err != nil { + t.Fatalf("cosiPreflightContext: %v", err) + } + + if nodeID != "192.0.2.10" { + t.Errorf("maintenance ctx with no outgoing metadata should fall back to GlobalArgs.Nodes[0]; got nodeID=%q, want %q", nodeID, "192.0.2.10") + } +} + +// TestCosiPreflightContext_NoMetadata_MultipleGlobalArgsNodes_NoFallback +// pins that the fallback is single-node-only. Multi-node maintenance +// apply hits openClientPerNodeMaintenance once per node — by the time +// cosiPreflightContext sees the call, GlobalArgs.Nodes is already +// scoped to one element. If somehow a multi-element slice leaks +// through here, falling back to GlobalArgs.Nodes[0] would silently +// pick the first and hide the bug. Stay empty in that case so the +// per-node prefix collapses to bare-line rather than wrong-line. +func TestCosiPreflightContext_NoMetadata_MultipleGlobalArgsNodes_NoFallback(t *testing.T) { + saved := append([]string(nil), GlobalArgs.Nodes...) + + t.Cleanup(func() { GlobalArgs.Nodes = saved }) + + GlobalArgs.Nodes = []string{"192.0.2.10", "192.0.2.11"} + + _, nodeID, err := cosiPreflightContext(context.Background()) + if err != nil { + t.Fatalf("cosiPreflightContext: %v", err) + } + + if nodeID != "" { + t.Errorf("multi-element GlobalArgs.Nodes must NOT use fallback (per-node loop should have scoped to one already); got nodeID=%q, want empty", nodeID) + } +} + // TestCosiPreflightContext_RejectsMultiNodeCtx pins that a multi- // element plural slice surfaces as an explicit error rather than a // silent passthrough. applyTemplatesPerNode iterates one node at a diff --git a/pkg/commands/preflight_apply_safety.go b/pkg/commands/preflight_apply_safety.go index 8bf2102..419fa28 100644 --- a/pkg/commands/preflight_apply_safety.go +++ b/pkg/commands/preflight_apply_safety.go @@ -113,6 +113,14 @@ func preflightValidateResources( } findings := applycheck.ValidateRefs(refs, snapshot) + + netAddrFindings, err := applycheck.WalkNetAddrFindings(rendered) + if err != nil { + return errors.Wrap(err, "pre-flight: walking rendered MachineConfig for net-addr fields") + } + + findings = append(findings, netAddrFindings...) + if len(findings) == 0 { return nil } @@ -173,6 +181,7 @@ func previewDrift( rendered []byte, nodeID string, w io.Writer, + showSecrets bool, ) error { current, ok, err := read(ctx) if err != nil { @@ -182,7 +191,7 @@ func previewDrift( } if !ok { - _, _ = fmt.Fprintln(w, "talm:", maintenanceConnectionMessage) + _, _ = fmt.Fprintf(w, "%stalm: %s\n", nodePrefix(nodeID), maintenanceConnectionMessage) return nil } @@ -194,7 +203,7 @@ func previewDrift( return nil } - printDriftPreview(w, headerWithNode("talm: drift preview", nodeID), changes) + printDriftPreview(w, headerWithNode("talm: drift preview", nodeID), changes, showSecrets) return nil } @@ -238,6 +247,7 @@ func verifyAppliedState( sent []byte, nodeID string, w io.Writer, + showSecrets bool, ) error { onNode, ok, err := read(ctx) if err != nil { @@ -249,7 +259,7 @@ func verifyAppliedState( } if !ok { - _, _ = fmt.Fprintln(w, "talm:", maintenanceConnectionMessage) + _, _ = fmt.Fprintf(w, "%stalm: %s\n", nodePrefix(nodeID), maintenanceConnectionMessage) return nil } @@ -264,7 +274,7 @@ func verifyAppliedState( return nil } - printDriftPreview(w, headerWithNode("talm: post-apply divergence", nodeID), changes) + printDriftPreview(w, headerWithNode("talm: post-apply divergence", nodeID), changes, showSecrets) //nolint:wrapcheck // cockroachdb/errors.WithHint at boundary. return errors.WithHint( @@ -277,7 +287,7 @@ func verifyAppliedState( // entries are dropped from the per-line listing but counted in the // trailing summary so the reader can confirm the diff against expected // scope. -func printDriftPreview(w io.Writer, header string, changes []applycheck.Change) { +func printDriftPreview(w io.Writer, header string, changes []applycheck.Change, showSecrets bool) { _, _ = fmt.Fprintln(w, header) var adds, removes, updates, equals int @@ -301,13 +311,20 @@ func printDriftPreview(w io.Writer, header string, changes []applycheck.Change) for j := range change.Fields { f := &change.Fields[j] - _, _ = fmt.Fprintf(w, " %s\n", formatFieldChangeLine(f)) + _, _ = fmt.Fprintf(w, " %s\n", formatFieldChangeLine(f, showSecrets)) } } _, _ = fmt.Fprintf(w, "talm: %d addition, %d removal, %d update, %d unchanged.\n", adds, removes, updates, equals) } +// absentFieldValue is the rendering for a leaf field that does not +// exist on one side of a FieldChange (HasOld=false or HasNew=false). +// Hoisted so formatFieldValue and formatSecretFieldValue stay +// byte-identical on the absent path — a future drift in either would +// obscure add/remove vs rotate semantics in the drift preview. +const absentFieldValue = "(absent)" + // formatFieldChangeLine renders one FieldChange entry for the drift // preview. The default form is "path: old -> new"; the slice-vs-slice // case takes a set-diff fast path so a 50-element certSANs update @@ -316,12 +333,53 @@ func printDriftPreview(w io.Writer, header string, changes []applycheck.Change) // surfaces correctly) and handles the equal-multiset reorder case // with an explicit "(reordered, N element(s))" line so the operator // isn't left wondering why an OpUpdate fired with no apparent change. -func formatFieldChangeLine(f *applycheck.FieldChange) string { - if oldSlice, newSlice, ok := bothSlices(f); ok { - return formatSliceSetDiff(f.Path, oldSlice, newSlice) +func formatFieldChangeLine(change *applycheck.FieldChange, showSecrets bool) string { + // Secret check runs BEFORE bothSlices so a secret-bearing path + // that happens to render as a slice (e.g. a future allowlist + // entry naming the array itself rather than a leaf element) + // still gets redacted instead of leaking the full element + // values through formatSliceSetDiff. + if !showSecrets && isSecretPath(change.Path) { + return fmt.Sprintf("%s: %s -> %s", change.Path, formatSecretFieldValue(change.HasOld, change.Old), formatSecretFieldValue(change.HasNew, change.New)) + } + + if oldSlice, newSlice, ok := bothSlices(change); ok { + return formatSliceSetDiff(change.Path, oldSlice, newSlice) + } + + return fmt.Sprintf("%s: %s -> %s", change.Path, formatFieldValue(change.HasOld, change.Old), formatFieldValue(change.HasNew, change.New)) +} + +// formatSecretFieldValue is the redaction-aware counterpart of +// formatFieldValue. Absent (HasX=false) reads as `(absent)` +// unchanged so the operator can still distinguish add/remove from +// rotation. A present non-string value renders via fmt.Sprintf with +// the length tell so a number/bool rotation still surfaces as +// "different" — preserving the rotation-detection promise that +// motivated redactValue carrying len=N. +// +// Caveat: Go's fmt.Sprintf("%v", m) on a map[string]any iterates +// keys in randomised order (deliberately, since Go 1.0). For +// map-shaped secret values, two semantically-equal maps may render +// as different-length strings (false positive: looks like a +// rotation when nothing changed), and two unequal maps may +// coincidentally collide on length (false negative: rotation +// missed). The allowlist today contains no map-shaped entries; if +// a future addition does, either canonicalise the map (sorted keys +// before %v) or disclaim the rotation signal for that entry. +func formatSecretFieldValue(has bool, value any) string { + if !has { + return absentFieldValue + } + + if s, ok := value.(string); ok { + return redactValue(s) } - return fmt.Sprintf("%s: %s -> %s", f.Path, formatFieldValue(f.HasOld, f.Old), formatFieldValue(f.HasNew, f.New)) + // Non-string values: render the %v form's length so the + // operator still sees a rotation signal. Same shape as + // redactValue but without committing to "this was a string". + return redactValue(fmt.Sprintf("%v", value)) } // bothSlices returns the two sides as []any when the FieldChange @@ -449,7 +507,7 @@ func mustRenderFlow(items []any) string { // lists or nodeLabel maps. func formatFieldValue(has bool, value any) string { if !has { - return "(absent)" + return absentFieldValue } switch value.(type) { diff --git a/pkg/commands/preflight_apply_safety_redact.go b/pkg/commands/preflight_apply_safety_redact.go new file mode 100644 index 0000000..2fa410f --- /dev/null +++ b/pkg/commands/preflight_apply_safety_redact.go @@ -0,0 +1,121 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "fmt" + "regexp" + "slices" +) + +// secretFieldPaths is the operator-visible allowlist of paths the +// drift preview redacts by default. Inclusion criteria: +// +// 1. Cluster-private bootstrap material — CA keys, encryption +// secrets, bootstrap tokens — whose disclosure to a CI log or +// screen-share is an incident. +// 2. Operator-managed credential material — Wireguard private / +// pre-shared keys. +// 3. The path has a stable form in v1alpha1. +// +// Out of scope (matches the issue author's #189 scope): exhaustive +// sweep over every Sensitive-marked field in the Talos v1alpha1 +// schema. The list grows issue-by-issue when an operator reports a +// new leak; each addition should cite the symptom. +// +// Bracket-normalisation lets array-indexed paths +// (cluster.acceptedCAs[2].key) match the wildcard form +// (cluster.acceptedCAs[].key) so an operator-visible diff with +// concrete indices is redacted. +// +//nolint:gochecknoglobals // static allowlist of secret field paths. +var secretFieldPaths = []string{ + // v1alpha1 MachineConfig (cluster.* / machine.*) bootstrap material. + // Scalar leaves: differ emits these paths directly. + "cluster.secret", + "cluster.token", + "cluster.aescbcEncryptionSecret", + "cluster.secretboxEncryptionSecret", + "cluster.ca.key", + "cluster.aggregatorCA.key", + "cluster.serviceAccount.key", + "cluster.etcd.ca.key", + "machine.token", + "machine.ca.key", + + // Slice-of-maps fields that carry secrets nested under each + // element. The differ's flatten step (pkg/applycheck/diff.go) + // treats slices as atomic leaves, so an `acceptedCAs[2].key` + // rotation surfaces at the formatter as a FieldChange whose + // Path is the parent slice (`cluster.acceptedCAs`), value is + // the whole `[]any` of maps. The whole slice is redacted — + // element-level granularity is sacrificed for correctness: the + // formatter cannot today render `{crt: visible, key: redacted}` + // per element because the secret check fires above bothSlices, + // not inside the renderer. If/when the differ recurses into + // slice elements with stable identity (e.g. + // `cluster.acceptedCAs[crt=foo].key`), the bracket forms can + // be added alongside these parent entries. + "cluster.acceptedCAs", + "machine.acceptedCAs", + + // v1alpha1 multidoc kinds. Paths are bare (no doc-kind prefix) + // because the differ does not prepend the doc kind to inner + // paths. `privateKey` matches WireguardConfig.privateKey + // directly (scalar leaf). `peers` matches WireguardConfig.peers + // as a parent slice (same shape as cluster.acceptedCAs — the + // whole peers slice is redacted because the differ won't + // descend into element fields to find the presharedKey leaf). + "privateKey", + "peers", +} + +// arrayIndexPattern matches `[N]` segments (one or more digits) so +// isSecretPath can normalise paths like `cluster.acceptedCAs[2].key` +// down to `cluster.acceptedCAs[].key` before comparing against the +// allowlist. +var arrayIndexPattern = regexp.MustCompile(`\[\d+\]`) + +// isSecretPath reports whether the leaf-field path falls inside the +// drift-preview redaction allowlist. The matcher is exact-equality +// after bracket normalisation, NOT a prefix match: `cluster.token` +// matches only `cluster.token`, not `cluster.tokenExtras` or +// `cluster.token.subkey`. +// +// Numeric array indices are normalised to `[]` before comparison +// so an operator-visible diff with concrete indices +// (`cluster.acceptedCAs[2].key`) matches the allowlist entry +// (`cluster.acceptedCAs[].key`). The normalisation is the only +// transformation; nested-field paths under a secret entry are not +// auto-included — an allowlist entry must name the leaf exactly. +func isSecretPath(path string) bool { + normalised := arrayIndexPattern.ReplaceAllString(path, "[]") + + return slices.Contains(secretFieldPaths, normalised) +} + +// redactValue renders the redaction sentinel for a secret-bearing +// value. Length disclosure is intentional: operators rotating a +// secret want a signal that the rotation actually happened on the +// node (different lengths = the value changed); without the length +// disclosure two `***redacted***` sides look identical regardless +// of whether the value rotated. +// +// Empty / absent values stay distinct: an empty-string secret +// reads as `***redacted (len=0)***`, which is still distinguishable +// from `(absent)` rendered by formatFieldValue. +func redactValue(s string) string { + return fmt.Sprintf("***redacted (len=%d)***", len(s)) +} diff --git a/pkg/commands/preflight_apply_safety_redact_test.go b/pkg/commands/preflight_apply_safety_redact_test.go new file mode 100644 index 0000000..dd5d12d --- /dev/null +++ b/pkg/commands/preflight_apply_safety_redact_test.go @@ -0,0 +1,479 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "strings" + "testing" + + "github.com/cozystack/talm/pkg/applycheck" +) + +// Hoisted secret-path literals avoid goconst on the test slice +// while keeping the per-case strings legible. These mirror entries +// in secretFieldPaths; the production list is the source of truth. +const ( + pathClusterSecret = "cluster.secret" + pathClusterToken = "cluster.token" + pathClusterAescbcEncryption = "cluster.aescbcEncryptionSecret" + pathMachineToken = "machine.token" + pathWireguardPrivateKey = "privateKey" + pathClusterAcceptedCAs = "cluster.acceptedCAs" + pathWireguardPeers = "peers" +) + +// TestIsSecretPath_ExactMatch pins the simplest case: an exact +// path match against a top-level secret returns true. Without this +// case the implementation could regress to bracket-only matching +// and fail on the most common shape (cluster.token / cluster.secret +// are not array elements). +func TestIsSecretPath_ExactMatch(t *testing.T) { + t.Parallel() + + for _, path := range []string{ + pathClusterSecret, + pathClusterToken, + pathClusterAescbcEncryption, + pathMachineToken, + } { + t.Run(path, func(t *testing.T) { + t.Parallel() + + if !isSecretPath(path) { + t.Errorf("isSecretPath(%q) = false, want true", path) + } + }) + } +} + +// TestIsSecretPath_WireguardSecretPaths pins the bare-path entries +// that match Wireguard multidoc secret-bearing fields. The +// --show-secrets-in-drift flag help text advertises that Wireguard +// private and pre-shared keys are redacted by default; without +// these allowlist entries the help text would lie and rotating +// either field would leak the base64 key value to stderr. +// +// The differ's flatten step does not prefix multidoc paths with +// the doc kind (pkg/applycheck/diff.go) and treats slices as +// atomic leaves, so the emitted paths are `privateKey` (scalar +// leaf) and `peers` (whole slice). The presharedKey lives inside +// peer elements; the whole peers slice is redacted because the +// formatter does not descend into element fields. +func TestIsSecretPath_WireguardSecretPaths(t *testing.T) { + t.Parallel() + + for _, path := range []string{ + pathWireguardPrivateKey, + pathWireguardPeers, + } { + t.Run(path, func(t *testing.T) { + t.Parallel() + + if !isSecretPath(path) { + t.Errorf("isSecretPath(%q) = false; Wireguard private/preshared keys are advertised as redacted by --show-secrets-in-drift", path) + } + }) + } +} + +// TestFormatFieldChangeLine_AcceptedCAsSliceRotation_NoLeak is the +// real-differ-shape regression pin against the security-class bug +// the original allowlist had: bracket-form entries +// (`cluster.acceptedCAs[].key`) never matched the differ's actual +// output (`cluster.acceptedCAs`, slice-atomic). A rotation of the +// CA list leaked the new and old `key` bytes through +// formatSliceSetDiff. This test exercises the exact shape the +// differ emits and asserts the key bytes never appear in the +// rendered line. +func TestFormatFieldChangeLine_AcceptedCAsSliceRotation_NoLeak(t *testing.T) { + t.Parallel() + + change := &applycheck.FieldChange{ + Path: pathClusterAcceptedCAs, + Old: []any{ + map[string]any{"crt": "AAA", "key": "SECRET_CA_KEY_AAA"}, + }, + New: []any{ + map[string]any{"crt": "BBB", "key": "SECRET_CA_KEY_BBB"}, + }, + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(change, false) + for _, leaked := range []string{"SECRET_CA_KEY_AAA", "SECRET_CA_KEY_BBB"} { + if strings.Contains(got, leaked) { + t.Errorf("cluster.acceptedCAs slice rotation must not leak key bytes; found %q in %q", leaked, got) + } + } + + if !strings.Contains(got, "redacted") { + t.Errorf("cluster.acceptedCAs slice rotation must render the redaction sentinel; got %q", got) + } +} + +// TestFormatFieldChangeLine_WireguardPeersRotation_NoLeak is the +// counterpart for the multidoc Wireguard kind. The differ emits +// `peers` (slice-atomic) on a peer-list rotation; the entries +// carry presharedKey leaves nested under each element. The whole +// peers slice must redact; presharedKey bytes must never leak. +func TestFormatFieldChangeLine_WireguardPeersRotation_NoLeak(t *testing.T) { + t.Parallel() + + change := &applycheck.FieldChange{ + Path: pathWireguardPeers, + Old: []any{ + map[string]any{"publicKey": "PUB1", "presharedKey": "SECRET_PSK_AAA"}, + }, + New: []any{ + map[string]any{"publicKey": "PUB1", "presharedKey": "SECRET_PSK_BBB"}, + }, + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(change, false) + for _, leaked := range []string{"SECRET_PSK_AAA", "SECRET_PSK_BBB"} { + if strings.Contains(got, leaked) { + t.Errorf("peers slice rotation must not leak presharedKey bytes; found %q in %q", leaked, got) + } + } + + if !strings.Contains(got, "redacted") { + t.Errorf("peers slice rotation must render the redaction sentinel; got %q", got) + } +} + +// TestIsSecretPath_BracketNormalisationStillNormalises pins the +// arrayIndexPattern regex contract independent of the current +// allowlist content. If a future allowlist entry uses the +// bracket-wildcard form (e.g. when the differ learns to descend +// into slice elements with stable identity), normalisation must +// still convert numeric `[N]` to `[]` so `foo[2].bar` matches +// `foo[].bar`. Test against a synthetic entry rather than the +// real allowlist to decouple the regex contract from allowlist +// drift. +func TestIsSecretPath_BracketNormalisationStillNormalises(t *testing.T) { + t.Parallel() + + got := arrayIndexPattern.ReplaceAllString("cluster.acceptedCAs[42].key", "[]") + if got != "cluster.acceptedCAs[].key" { + t.Errorf("arrayIndexPattern must normalise [42] -> []; got %q", got) + } +} + +// TestIsSecretPath_NoFalseMatchOnPrefix pins that a non-secret path +// sharing a string prefix with a secret entry does NOT match. The +// matcher is path-segment-aware (split on dots), not raw substring; +// cluster.tokenExtras must not match cluster.token. Without this +// pin a substring-based implementation would silently redact +// operator-visible fields that happen to share a prefix. +func TestIsSecretPath_NoFalseMatchOnPrefix(t *testing.T) { + t.Parallel() + + for _, path := range []string{ + "cluster.tokenExtras", + "cluster.secretsManager", + "machine.tokenSomething", + "cluster.acceptedCAsExtras", + } { + t.Run(path, func(t *testing.T) { + t.Parallel() + + if isSecretPath(path) { + t.Errorf("isSecretPath(%q) = true, want false (false-prefix match)", path) + } + }) + } +} + +// TestIsSecretPath_NonSecretPath_NoMatch pins that ordinary +// operator-visible paths are not redacted. A regression here would +// hide useful information from the operator (network changes, +// install disk changes, etc.). +func TestIsSecretPath_NonSecretPath_NoMatch(t *testing.T) { + t.Parallel() + + for _, path := range []string{ + "cluster.network.podSubnets", + "cluster.network.serviceSubnets", + "cluster.apiServer.certSANs", + "machine.install.disk", + "machine.network.hostname", + } { + t.Run(path, func(t *testing.T) { + t.Parallel() + + if isSecretPath(path) { + t.Errorf("isSecretPath(%q) = true, want false (non-secret operator-visible path)", path) + } + }) + } +} + +// TestRedactValue_PreservesLength pins the length-disclosure +// contract. Operators rotating a secret want a signal that a +// rotation happened — but not the value. Length carries the +// "something changed" bit without exposing the secret. +func TestRedactValue_PreservesLength(t *testing.T) { + t.Parallel() + + got := redactValue("abcdefg") + if !strings.Contains(got, "len=7") { + t.Errorf("redactValue should disclose length to signal rotation; got %q", got) + } + + if strings.Contains(got, "abcdefg") { + t.Errorf("redactValue must NOT leak the input; got %q", got) + } +} + +// TestFormatFieldChangeLine_RedactsSecretByDefault pins the default +// redaction at the formatter level: a FieldChange whose Path is a +// known secret renders both sides as the redaction sentinel when +// showSecrets is false (the default). +func TestFormatFieldChangeLine_RedactsSecretByDefault(t *testing.T) { + t.Parallel() + + f := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: "old-secret-aaaa", + New: "new-secret-bbbbbbbb", + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(f, false) + if strings.Contains(got, "old-secret-aaaa") || strings.Contains(got, "new-secret-bbbbbbbb") { + t.Errorf("default-redacted formatter must NOT leak secret values; got %q", got) + } + + if !strings.Contains(got, "redacted") { + t.Errorf("default-redacted formatter must render '***redacted...' sentinel; got %q", got) + } +} + +// TestFormatFieldChangeLine_ShowsSecretsWhenFlagSet pins the +// opt-out: passing showSecrets=true bypasses redaction so debugging +// workflows can inspect the actual values. This is operator-explicit +// (via --show-secrets-in-drift) so the leak is intentional. +func TestFormatFieldChangeLine_ShowsSecretsWhenFlagSet(t *testing.T) { + t.Parallel() + + f := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: "old-secret-aaaa", + New: "new-secret-bbbbbbbb", + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(f, true) + if !strings.Contains(got, "old-secret-aaaa") { + t.Errorf("showSecrets=true must render raw old value; got %q", got) + } + + if !strings.Contains(got, "new-secret-bbbbbbbb") { + t.Errorf("showSecrets=true must render raw new value; got %q", got) + } + + if strings.Contains(got, "redacted") { + t.Errorf("showSecrets=true must NOT apply the redaction sentinel; got %q", got) + } +} + +// TestFormatFieldChangeLine_NonSecretPathsUnchanged pins the control: +// non-secret paths render verbatim regardless of the showSecrets +// flag. A regression here would silently redact operator-visible +// information. +func TestFormatFieldChangeLine_NonSecretPathsUnchanged(t *testing.T) { + t.Parallel() + + f := &applycheck.FieldChange{ + Path: "machine.install.disk", + Old: "/dev/sda", + New: "/dev/sdb", + HasOld: true, + HasNew: true, + } + + for _, showSecrets := range []bool{false, true} { + got := formatFieldChangeLine(f, showSecrets) + if !strings.Contains(got, "/dev/sda") || !strings.Contains(got, "/dev/sdb") { + t.Errorf("non-secret path must render verbatim regardless of showSecrets=%v; got %q", showSecrets, got) + } + + if strings.Contains(got, "redacted") { + t.Errorf("non-secret path must NOT trigger redaction; got %q", got) + } + } +} + +// TestFormatFieldChangeLine_SecretWithNonStringValue_StillRedacted +// pins the non-string branch of formatSecretFieldValue. If a future +// schema drift puts a number/bool on a secret-bearing path, the +// redactor must still emit the length tell so a rotation surfaces +// as "different sentinel" instead of two identical ***redacted*** +// strings that hide the change. +func TestFormatFieldChangeLine_SecretWithNonStringValue_StillRedacted(t *testing.T) { + t.Parallel() + + change := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: 42, + New: 1234, + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(change, false) + if !strings.Contains(got, "redacted") { + t.Errorf("non-string secret-path value must still render the redaction sentinel; got %q", got) + } + + if strings.Contains(got, " 42 ") || strings.Contains(got, " 1234 ") { + t.Errorf("non-string secret values must NOT appear verbatim in the rendered line; got %q", got) + } + + // Different inputs must produce different sentinels (rotation + // signal). 42 and 1234 render as "42" (len 2) and "1234" (len 4) + // via fmt.Sprintf("%v", ...), so the redacted sides must + // disclose different lengths. + if !strings.Contains(got, "len=2") || !strings.Contains(got, "len=4") { + t.Errorf("non-string secret values of different lengths must produce different len=N sentinels; got %q", got) + } +} + +// TestFormatFieldChangeLine_SecretNilValue_StillRedacts pins the +// corner case where HasOld=true / HasNew=true but the value is +// literally nil (e.g. a YAML `field: null` round-trip rather than +// an absent field). formatSecretFieldValue's non-string branch +// renders via redactValue(fmt.Sprintf("%v", value)) which produces +// "***redacted (len=5)***" for nil (since %v of nil is "", +// 5 chars). The differ does not produce this shape today, but +// the corner is reachable if a future YAML decode round-trip +// emits null-valued secret fields — pin that the value never +// leaks even on this path. +func TestFormatFieldChangeLine_SecretNilValue_StillRedacts(t *testing.T) { + t.Parallel() + + change := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: nil, + New: "new-secret-value", + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(change, false) + if !strings.Contains(got, "redacted") { + t.Errorf("HasOld=true with nil value must still render the redaction sentinel; got %q", got) + } + + if strings.Contains(got, "new-secret-value") { + t.Errorf("HasNew=true with secret string must redact, not leak; got %q", got) + } +} + +// TestFormatFieldChangeLine_SecretAbsentSide_DistinguishesAddFromRotate +// pins the absent-side branch of formatSecretFieldValue. A CA-list +// addition (HasOld=false, HasNew=true) must render as +// `(absent) -> ***redacted (len=N)***` so the operator can still +// distinguish "this secret was just added" from "this secret was +// rotated". The branch is the only thing keeping add-vs-rotate +// signal alive on a secret path; without this pin a future +// "tighten redaction" refactor could regress to +// `***redacted (len=0)*** -> ***redacted (len=N)***` and silently +// kill the distinction. +func TestFormatFieldChangeLine_SecretAbsentSide_DistinguishesAddFromRotate(t *testing.T) { + t.Parallel() + + addition := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: nil, + New: "new-token-value", + HasOld: false, + HasNew: true, + } + + got := formatFieldChangeLine(addition, false) + if !strings.Contains(got, "(absent)") { + t.Errorf("addition (HasOld=false) must render LEFT side as `(absent)` so add-vs-rotate stays distinguishable; got %q", got) + } + + if strings.Contains(got, "redacted (len=0)") { + t.Errorf("addition LEFT side must NOT collapse to `***redacted (len=0)***` (operator can't tell add from rotate-to-empty); got %q", got) + } + + if !strings.Contains(got, "redacted") { + t.Errorf("addition RIGHT side must still redact the new value; got %q", got) + } + + removal := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: "old-token-value", + New: nil, + HasOld: true, + HasNew: false, + } + + got = formatFieldChangeLine(removal, false) + if !strings.Contains(got, "(absent)") { + t.Errorf("removal (HasNew=false) must render RIGHT side as `(absent)`; got %q", got) + } + + if strings.Contains(got, "old-token-value") { + t.Errorf("removal LEFT side must redact, not leak the old value; got %q", got) + } +} + +// TestFormatFieldChangeLine_SliceSecretPath_NoLeak pins the +// secret-check-before-bothSlices ordering at the formatter. The +// allowlist names parent slice paths (cluster.acceptedCAs, +// machine.acceptedCAs, peers) because the differ flattens slices +// atomically — those entries are NOT speculative. This test uses +// a scalar allowlist entry (machine.token) wrapped in a +// FieldChange whose Old/New are slices to prove the ordering +// works in the abstract: even if a hypothetical future scenario +// puts a scalar-allowlisted path on slice-valued Old/New, the +// secret check still fires before bothSlices and the contents +// never leak through formatSliceSetDiff. The real-shape +// CA / Wireguard slice-rotation regression pins are in +// TestFormatFieldChangeLine_AcceptedCAsSliceRotation_NoLeak and +// TestFormatFieldChangeLine_WireguardPeersRotation_NoLeak. +func TestFormatFieldChangeLine_SliceSecretPath_NoLeak(t *testing.T) { + t.Parallel() + + // Reuse a real allowlist entry; the test pins behaviour at + // the formatter, not the allowlist content. + change := &applycheck.FieldChange{ + Path: pathMachineToken, + Old: []any{"secret-aaa", "secret-bbb"}, + New: []any{"secret-ccc"}, + HasOld: true, + HasNew: true, + } + + got := formatFieldChangeLine(change, false) + for _, leaked := range []string{"secret-aaa", "secret-bbb", "secret-ccc"} { + if strings.Contains(got, leaked) { + t.Errorf("slice-shaped secret path must not leak element %q; got %q", leaked, got) + } + } + + if !strings.Contains(got, "redacted") { + t.Errorf("slice-shaped secret path must render the redaction sentinel; got %q", got) + } +} diff --git a/pkg/commands/preflight_apply_safety_test.go b/pkg/commands/preflight_apply_safety_test.go index 9db6b5f..2356099 100644 --- a/pkg/commands/preflight_apply_safety_test.go +++ b/pkg/commands/preflight_apply_safety_test.go @@ -192,6 +192,7 @@ up: true desired, "", buf, + false, ) if err != nil { t.Fatalf("previewDrift error: %v", err) @@ -217,6 +218,7 @@ func TestPreviewDrift_InsecurePath_DegradesGracefully(t *testing.T) { []byte(renderedV1_12Multidoc), "", buf, + false, ) if err != nil { t.Errorf("previewDrift on insecure path should not block, got err=%v", err) @@ -237,6 +239,7 @@ func TestVerifyAppliedState_Match_NoError(t *testing.T) { sent, "", &bytes.Buffer{}, + false, ) if err != nil { t.Errorf("verifyAppliedState should accept matching configs, got err=%v", err) @@ -272,6 +275,7 @@ up: false sent, "", buf, + false, ) if err == nil { t.Fatal("verifyAppliedState should block on divergence, got nil error") @@ -297,6 +301,7 @@ func TestVerifyAppliedState_ReaderError_Blocks(t *testing.T) { []byte(renderedV1_12Multidoc), "", &bytes.Buffer{}, + false, ) if err == nil { t.Fatal("expected error on reader failure, got nil") @@ -317,6 +322,7 @@ func TestVerifyAppliedState_InsecurePath_NoBlock(t *testing.T) { []byte(renderedV1_12Multidoc), "", buf, + false, ) if err != nil { t.Errorf("verifyAppliedState on insecure path should not block, got err=%v", err) @@ -477,7 +483,7 @@ func TestPrintDriftPreview_SliceSetDiff_RemovesDuplicate(t *testing.T) { }} buf := &bytes.Buffer{} - printDriftPreview(buf, "drift:", changes) + printDriftPreview(buf, "drift:", changes, false) out := buf.String() if !strings.Contains(out, "removed [127.0.0.1]") { @@ -513,7 +519,7 @@ func TestPrintDriftPreview_SliceSetDiff_AddOnly(t *testing.T) { }} buf := &bytes.Buffer{} - printDriftPreview(buf, "drift:", changes) + printDriftPreview(buf, "drift:", changes, false) out := buf.String() if !strings.Contains(out, "added [192.0.2.5]") { @@ -547,7 +553,7 @@ func TestPrintDriftPreview_SliceSetDiff_ReorderOnly(t *testing.T) { }} buf := &bytes.Buffer{} - printDriftPreview(buf, "drift:", changes) + printDriftPreview(buf, "drift:", changes, false) out := buf.String() if !strings.Contains(out, "reordered") { @@ -585,6 +591,7 @@ machine: desired, "192.0.2.10", buf, + false, ) if err != nil { t.Fatalf("previewDrift error: %v", err) @@ -629,6 +636,7 @@ up: false sent, "192.0.2.11", buf, + false, ) if err == nil { t.Fatal("expected divergence to surface as an error") @@ -665,6 +673,7 @@ machine: desired, "", buf, + false, ) if err != nil { t.Fatalf("previewDrift error: %v", err) @@ -698,7 +707,7 @@ func TestPrintDriftPreview_SliceFlowStyle_AbsentOnOneSide(t *testing.T) { }} buf := &bytes.Buffer{} - printDriftPreview(buf, "drift:", changes) + printDriftPreview(buf, "drift:", changes, false) out := buf.String() if strings.Contains(out, "[127.0.0.1 192.0.2.5]") { @@ -735,7 +744,7 @@ func TestPrintDriftPreview_MapFieldChange_RendersFlowStyle(t *testing.T) { }} buf := &bytes.Buffer{} - printDriftPreview(buf, "drift:", changes) + printDriftPreview(buf, "drift:", changes, false) out := buf.String() if strings.Contains(out, "map[role:control-plane]") { @@ -768,7 +777,7 @@ func TestPrintDriftPreview_ScalarFieldChange_StaysInline(t *testing.T) { }} buf := &bytes.Buffer{} - printDriftPreview(buf, "drift:", changes) + printDriftPreview(buf, "drift:", changes, false) out := buf.String() if !strings.Contains(out, "cozy.local -> cozy.example") { @@ -869,3 +878,296 @@ func TestShouldRunPostApplyVerify_RespectsModeAndDryRun(t *testing.T) { }) } } + +// TestPreviewDrift_MaintenanceMessage_CarriesNodePrefix pins the +// per-node prefix on the maintenance-connection warning emitted by +// previewDrift when the reader returns ok=false. The drift / divergence +// headers already disambiguate per node via headerWithNode; the +// maintenance line lagged behind, producing identical bare warnings on +// every node in a multi-node insecure apply with no way for the +// operator to tell which node each line came from. Mirrors +// TestPreviewDrift_MultiNode_HeaderCarriesNodeID — same expectation +// shape, different emission site. +func TestPreviewDrift_MaintenanceMessage_CarriesNodePrefix(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + err := previewDrift( + context.Background(), + stubMachineConfigReader(nil, false, nil), + []byte(renderedV1_12Multidoc), + "192.0.2.10", + buf, + false, + ) + if err != nil { + t.Fatalf("previewDrift on insecure path should not block, got err=%v", err) + } + + want := "node 192.0.2.10: talm: " + maintenanceConnectionMessage + if !strings.Contains(buf.String(), want) { + t.Errorf("non-empty nodeID must prefix the maintenance-connection line; want substring %q, got:\n%s", want, buf.String()) + } +} + +// TestVerifyAppliedState_MaintenanceMessage_CarriesNodePrefix is the +// Phase 2B counterpart of TestPreviewDrift_MaintenanceMessage_CarriesNodePrefix. +// Multi-node apply hits both previewDrift (Phase 2A) and +// verifyAppliedState (Phase 2B) in the same loop, so both maintenance +// emissions need the same per-node disambiguation. +func TestVerifyAppliedState_MaintenanceMessage_CarriesNodePrefix(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + err := verifyAppliedState( + context.Background(), + stubMachineConfigReader(nil, false, nil), + []byte(renderedV1_12Multidoc), + "192.0.2.11", + buf, + false, + ) + if err != nil { + t.Errorf("verifyAppliedState on insecure path should not block, got err=%v", err) + } + + want := "node 192.0.2.11: talm: " + maintenanceConnectionMessage + if !strings.Contains(buf.String(), want) { + t.Errorf("non-empty nodeID must prefix the maintenance-connection line; want substring %q, got:\n%s", want, buf.String()) + } +} + +// TestPreflightValidateResources_NetAddrFinding_Blocks pins the +// Phase 1 integration of WalkNetAddrFindings: a rendered config with +// a malformed WireguardConfig peer endpoint must block before the +// apply RPC. Without this pin, the walker could regress to "called +// but findings discarded" while the unit tests in pkg/applycheck/ +// keep passing. +func TestPreflightValidateResources_NetAddrFinding_Blocks(t *testing.T) { + t.Parallel() + + snapshot := applycheck.HostSnapshot{ + Links: []string{"eth0", "eth1"}, + Disks: []applycheck.DiskInfo{{DevPath: "/dev/sda"}}, + } + + rendered := []byte(`version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda +--- +apiVersion: v1alpha1 +kind: WireguardConfig +name: wg-broken +peers: + - publicKey: ZZZ + endpoint: notavalid:endpoint +`) + + buf := &bytes.Buffer{} + err := preflightValidateResources( + context.Background(), + stubLinksDisksReader(snapshot, true), + rendered, + buf, + ) + if err == nil { + t.Fatal("expected malformed Wireguard peer endpoint to block Phase 1, got nil") + } + + out := buf.String() + if !strings.Contains(out, "WireguardConfig.peers[0].endpoint") { + t.Errorf("preflight output should cite the offending field path, got %q", out) + } +} + +// TestPreflightValidateResources_NetAddrFinding_StaticHostConfig_Blocks +// pins the Phase 1 integration of WalkNetAddrFindings for the +// StaticHostConfig kind. Walker-level unit tests cover the handler +// in isolation; this test exercises the full pipeline +// preflightValidateResources -> applycheck.WalkNetAddrFindings -> +// finding -> printFinding output -> Phase 1 blocker error. Without +// this pin, a walker integration regression for StaticHostConfig +// could pass walker unit tests while production silently no-ops. +func TestPreflightValidateResources_NetAddrFinding_StaticHostConfig_Blocks(t *testing.T) { + t.Parallel() + + snapshot := applycheck.HostSnapshot{ + Links: []string{"eth0"}, + Disks: []applycheck.DiskInfo{{DevPath: "/dev/sda"}}, + } + + rendered := []byte(`version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda +--- +apiVersion: v1alpha1 +kind: StaticHostConfig +name: 999.999.0.1 +hostnames: + - foo.example +`) + + buf := &bytes.Buffer{} + err := preflightValidateResources( + context.Background(), + stubLinksDisksReader(snapshot, true), + rendered, + buf, + ) + if err == nil { + t.Fatal("expected malformed StaticHostConfig.name to block Phase 1, got nil") + } + + out := buf.String() + if !strings.Contains(out, "StaticHostConfig.name") { + t.Errorf("preflight output should cite the offending field path; got %q", out) + } + + if !strings.Contains(out, "999.999.0.1") { + t.Errorf("preflight output should cite the offending value; got %q", out) + } +} + +// TestPreflightValidateResources_NetAddrFinding_NetworkRuleConfig_Blocks +// pins the Phase 1 integration for the NetworkRuleConfig kind. +// Exercises both subnet and except validation paths through the +// full pipeline. Two malformed entries (one bad subnet + one bad +// except next to a valid subnet) must produce TWO blocker findings +// with distinct path indices, so an operator with multiple typos +// sees all of them in one Phase 1 pass. +func TestPreflightValidateResources_NetAddrFinding_NetworkRuleConfig_Blocks(t *testing.T) { + t.Parallel() + + snapshot := applycheck.HostSnapshot{ + Links: []string{"eth0"}, + Disks: []applycheck.DiskInfo{{DevPath: "/dev/sda"}}, + } + + rendered := []byte(`version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda +--- +apiVersion: v1alpha1 +kind: NetworkRuleConfig +name: rule-broken +portSelector: + ports: [22] + protocol: tcp +ingress: + - subnet: 192.0.2.0/24 + - subnet: notacidr + - subnet: 10.0.0.0/24 + except: 999.999.0.1/30 +`) + + buf := &bytes.Buffer{} + err := preflightValidateResources( + context.Background(), + stubLinksDisksReader(snapshot, true), + rendered, + buf, + ) + if err == nil { + t.Fatal("expected malformed NetworkRuleConfig ingress fields to block Phase 1, got nil") + } + + out := buf.String() + if !strings.Contains(out, "ingress[1].subnet") { + t.Errorf("preflight output should cite ingress[1].subnet (the malformed subnet); got %q", out) + } + + if !strings.Contains(out, "ingress[2].except") { + t.Errorf("preflight output should cite ingress[2].except (the malformed except); got %q", out) + } + + if strings.Contains(out, "ingress[0]") { + t.Errorf("ingress[0].subnet is valid (192.0.2.0/24) and must NOT be cited; got %q", out) + } +} + +// TestPreflightValidateResources_NetAddrFinding_ValidPasses pins the +// happy path of the new walker integration: a rendered config with +// valid host:port endpoints (IPv4 and IPv6) must NOT block. Catches +// the symmetric regression where the walker flags valid input as +// malformed. +func TestPreflightValidateResources_NetAddrFinding_ValidPasses(t *testing.T) { + t.Parallel() + + snapshot := applycheck.HostSnapshot{ + Links: []string{"eth0", "eth1"}, + Disks: []applycheck.DiskInfo{{DevPath: "/dev/sda"}}, + } + + rendered := []byte(`version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda +--- +apiVersion: v1alpha1 +kind: WireguardConfig +name: wg-ok +peers: + - publicKey: AAA + endpoint: 192.0.2.10:51820 + - publicKey: BBB + endpoint: "[2001:db8::1]:51820" +--- +apiVersion: v1alpha1 +kind: StaticHostConfig +name: 192.0.2.20 +hostnames: + - host1.example +`) + + buf := &bytes.Buffer{} + err := preflightValidateResources( + context.Background(), + stubLinksDisksReader(snapshot, true), + rendered, + buf, + ) + if err != nil { + t.Errorf("valid net-addr fields should pass Phase 1, got err=%v, output=%q", err, buf.String()) + } +} + +// TestPreviewDrift_MaintenanceMessage_EmptyNodeIDPreservesBareLine +// pins the single-node UX regression guard: when nodeID is empty (the +// implicit-single-node path), the maintenance line must stay bare — +// no leading "node : " prefix. Without this pin, a prefix-always-on +// implementation would produce "node : talm: ..." which is uglier +// than today's bare output for the common single-node case. +func TestPreviewDrift_MaintenanceMessage_EmptyNodeIDPreservesBareLine(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + err := previewDrift( + context.Background(), + stubMachineConfigReader(nil, false, nil), + []byte(renderedV1_12Multidoc), + "", + buf, + false, + ) + if err != nil { + t.Fatalf("previewDrift on insecure path should not block, got err=%v", err) + } + + out := buf.String() + + want := "talm: " + maintenanceConnectionMessage + if !strings.Contains(out, want) { + t.Errorf("empty nodeID: maintenance-connection line must remain present; want substring %q, got:\n%s", want, out) + } + + if strings.Contains(out, "node : ") { + t.Errorf("empty nodeID: must NOT produce 'node : ' prefix; got:\n%s", out) + } +} diff --git a/pkg/commands/preflight_upgrade_verify.go b/pkg/commands/preflight_upgrade_verify.go index f647a20..9a3f33a 100644 --- a/pkg/commands/preflight_upgrade_verify.go +++ b/pkg/commands/preflight_upgrade_verify.go @@ -19,6 +19,7 @@ import ( "fmt" "io" "strings" + "time" "github.com/cockroachdb/errors" machineryconfig "github.com/siderolabs/talos/pkg/machinery/config" @@ -28,10 +29,11 @@ const postUpgradeVersionMismatchHint = "two hypotheses produce this symptom: " + "(1) Talos auto-rolled back after the new partition failed its boot readiness check — " + "cross-vendor upgrades (e.g. cozystack-bundled image -> vanilla siderolabs installer) " + "drop bundled extensions and trigger this. " + - "(2) The node is slower than the 90s reconcile window — large image pulls or cold " + - "hardware can exceed it. Re-run `talm get version` after a minute to distinguish: if " + - "the version updated, the node was just slow; if it's still the old version, the " + - "rollback case is real. Pass --skip-post-upgrade-verify to bypass." + "(2) The node is slower than the configured reconcile window — large image pulls or cold " + + "hardware can exceed it. Widen via --post-upgrade-reconcile-window or re-run " + + "`talm get version` after a minute to distinguish: if the version updated, the node " + + "was just slow; if it's still the old version, the rollback case is real. " + + "Pass --skip-post-upgrade-verify to bypass." // verifyPostUpgradeVersion is the Phase 2C gate: after talosctl upgrade // returns, re-read the node's runtime.Version COSI resource and compare @@ -49,6 +51,7 @@ func verifyPostUpgradeVersion( ctx context.Context, read versionReader, targetImage string, + reconcileWindow time.Duration, w io.Writer, ) error { target := parseTargetVersion(targetImage) @@ -108,7 +111,7 @@ func verifyPostUpgradeVersion( //nolint:wrapcheck // cockroachdb/errors.WithHint at boundary. return errors.WithHint( - errors.Newf("post-upgrade: requested upgrade to %s but running version is %s — either Talos auto-rolled back, or the node is still booting beyond the 90s window", target, running), + errors.Newf("post-upgrade: requested upgrade to %s but running version is %s — either Talos auto-rolled back, or the node is still booting beyond the configured reconcile window (%s)", target, running, reconcileWindow), postUpgradeVersionMismatchHint, ) } diff --git a/pkg/commands/preflight_upgrade_verify_test.go b/pkg/commands/preflight_upgrade_verify_test.go index 5733bfd..a8cb135 100644 --- a/pkg/commands/preflight_upgrade_verify_test.go +++ b/pkg/commands/preflight_upgrade_verify_test.go @@ -19,6 +19,7 @@ import ( "context" "strings" "testing" + "time" "github.com/cockroachdb/errors" ) @@ -109,6 +110,7 @@ func TestVerifyPostUpgradeVersion_Match_NoError(t *testing.T) { context.Background(), stubReader("v1.13.0", true), "ghcr.io/siderolabs/installer:v1.13.0", + time.Millisecond, buf, ) if err != nil { @@ -132,6 +134,7 @@ func TestVerifyPostUpgradeVersion_MinorMismatch_Blocks(t *testing.T) { context.Background(), stubReader("v1.12.6", true), "ghcr.io/siderolabs/installer:v1.13.0", + time.Millisecond, buf, ) if err == nil { @@ -159,6 +162,7 @@ func TestVerifyPostUpgradeVersion_PatchVersion_Match(t *testing.T) { context.Background(), stubReader("v1.12.6", true), "ghcr.io/cozystack/cozystack/talos:v1.12.7", + time.Millisecond, &bytes.Buffer{}, ) if err != nil { @@ -189,6 +193,7 @@ func TestVerifyPostUpgradeVersion_UnparseableTag_Skip(t *testing.T) { context.Background(), stubReader("v1.12.6", true), image, + time.Millisecond, &bytes.Buffer{}, ) if err != nil { @@ -219,6 +224,7 @@ func TestVerifyPostUpgradeVersion_ReaderConnectionRefused_NotSilent(t *testing.T context.Background(), stubReaderErr(errors.New("connection refused")), "ghcr.io/siderolabs/installer:v1.13.0", + time.Millisecond, buf, ) if err == nil { @@ -258,6 +264,7 @@ func TestVerifyPostUpgradeVersion_ReaderFails_SoftWarning_NoBlock(t *testing.T) context.Background(), stubReader("", false), "ghcr.io/siderolabs/installer:v1.13.0", + time.Millisecond, buf, ) if err != nil { @@ -292,6 +299,7 @@ func TestVerifyPostUpgradeVersion_ReaderFails_BestEffort(t *testing.T) { context.Background(), stubReader("", false), "ghcr.io/siderolabs/installer:v1.13.0", + time.Millisecond, buf, ) if err != nil { diff --git a/pkg/commands/talosctl_wrapper.go b/pkg/commands/talosctl_wrapper.go index be51d03..f7f23fd 100644 --- a/pkg/commands/talosctl_wrapper.go +++ b/pkg/commands/talosctl_wrapper.go @@ -282,7 +282,7 @@ func wrapTalosCommand(cmd *cobra.Command, cmdName string) *cobra.Command { } // Special handling for upgrade command - if baseCmdName == "upgrade" { + if baseCmdName == upgradeCmdName { wrapUpgradeCommand(wrappedCmd, originalRunE) } diff --git a/pkg/commands/upgrade_handler.go b/pkg/commands/upgrade_handler.go index f79771f..6dfcb84 100644 --- a/pkg/commands/upgrade_handler.go +++ b/pkg/commands/upgrade_handler.go @@ -28,19 +28,48 @@ import ( "github.com/spf13/cobra" ) -// postUpgradeReconcileWindow is how long we wait after talosctl -// upgrade returns before re-reading the running version. Talos -// reboots and reaches "running" stage in well under a minute on -// healthy hardware; auto-rollback adds ~30s on top of that. 90s -// covers both paths with margin. -const postUpgradeReconcileWindow = 90 * time.Second +const ( + // upgradeCmdName is the upstream cobra command name for the + // upgrade subcommand. Used by both the dispatch site and the + // per-command wrapper tests. + upgradeCmdName = "upgrade" + + // defaultPostUpgradeReconcileWindow is how long we wait after + // talosctl upgrade returns before re-reading the running + // version. Talos reboots and reaches "running" stage in well + // under a minute on healthy hardware; auto-rollback adds ~30s + // on top of that. 90s covers both paths with margin. Operators + // with slow hardware widen via --post-upgrade-reconcile-window. + defaultPostUpgradeReconcileWindow = 90 * time.Second +) // upgradeCmdFlags carries the talm-side flags layered on top of the // talosctl-derived upgrade command (set up in wrapUpgradeCommand). // //nolint:gochecknoglobals // command-scoped flag struct, mirrors applyCmdFlags pattern. var upgradeCmdFlags struct { - skipPostUpgradeVerify bool + skipPostUpgradeVerify bool + postUpgradeReconcileWindow time.Duration +} + +// validatePostUpgradeReconcileWindow rejects non-positive durations. +// A zero or negative window would have the version-read loop run +// while the node is still rebooting and surface a false "rollback" +// verdict every time — far worse failure mode than a small range +// check up front. +// +// Hint mentions "positive duration" verbatim so the boundary test +// can pin the contract against future copy drift. +func validatePostUpgradeReconcileWindow(window time.Duration) error { + if window <= 0 { + //nolint:wrapcheck // cockroachdb/errors.WithHint at boundary. + return errors.WithHint( + errors.Newf("--post-upgrade-reconcile-window must be a positive duration; got %s", window), + "pass a positive duration like 90s or 2m — the default is 90s", + ) + } + + return nil } // wrapUpgradeCommand adds special handling for upgrade command: extract image from config and set --image flag @@ -50,7 +79,21 @@ func wrapUpgradeCommand(wrappedCmd *cobra.Command, originalRunE func(*cobra.Comm wrappedCmd.Flags().BoolVar(&upgradeCmdFlags.skipPostUpgradeVerify, "skip-post-upgrade-verify", false, "skip the post-upgrade check that compares running Talos version against the target image's tag (Phase 2C; detects silent A/B rollback per #175)") + wrappedCmd.Flags().DurationVar(&upgradeCmdFlags.postUpgradeReconcileWindow, "post-upgrade-reconcile-window", defaultPostUpgradeReconcileWindow, + "how long to wait after upgrade returns before re-reading the running version; widen for slow hardware / large image pulls") + wrappedCmd.RunE = func(cmd *cobra.Command, args []string) error { + // Fail-fast on a bad --post-upgrade-reconcile-window BEFORE + // any talosctl upgrade RPC fires. A zero / negative value + // reaching the Phase 2C wait would fall through to the + // version-read loop while the node is still rebooting and + // always report 'rollback'. Worse — the upgrade itself has + // already executed by then; the operator's mistake gets + // validated after the partial state change. Validate first. + if err := validatePostUpgradeReconcileWindow(upgradeCmdFlags.postUpgradeReconcileWindow); err != nil { + return err + } + // Get config files from --file flag var filesToProcess []string @@ -249,6 +292,10 @@ func runPostUpgradeVersionVerify(parentCtx context.Context, image string) error parentCtx = context.Background() } + if err := validatePostUpgradeReconcileWindow(upgradeCmdFlags.postUpgradeReconcileWindow); err != nil { + return err + } + return WithClient(func(ctx context.Context, c *client.Client) error { ctxNodes := []string(nil) if cfg := c.GetConfigContext(); cfg != nil { @@ -257,7 +304,7 @@ func runPostUpgradeVersionVerify(parentCtx context.Context, image string) error nodes := resolveUpgradeTargetNodes(GlobalArgs.Nodes, ctxNodes) - return runPostUpgradeVersionVerifyInner(parentCtx, ctx, nodes, image, cosiVersionReader(c), postUpgradeReconcileWindow, os.Stderr) + return runPostUpgradeVersionVerifyInner(parentCtx, ctx, nodes, image, cosiVersionReader(c), upgradeCmdFlags.postUpgradeReconcileWindow, os.Stderr) }) } @@ -317,7 +364,7 @@ func runPostUpgradeVersionVerifyInner( for _, node := range nodes { nodeCtx := client.WithNode(clientCtx, node) - if err := verifyPostUpgradeVersion(nodeCtx, read, image, stderr); err != nil { + if err := verifyPostUpgradeVersion(nodeCtx, read, image, reconcileWindow, stderr); err != nil { perNodeErrs = append(perNodeErrs, errors.Wrapf(err, "node %s", node)) } } diff --git a/pkg/commands/upgrade_handler_test.go b/pkg/commands/upgrade_handler_test.go index 4cd1101..3a68091 100644 --- a/pkg/commands/upgrade_handler_test.go +++ b/pkg/commands/upgrade_handler_test.go @@ -17,10 +17,15 @@ package commands import ( "bytes" "context" + "os" + "path/filepath" "reflect" "strings" "testing" "time" + + "github.com/cockroachdb/errors" + "github.com/spf13/cobra" ) // TestResolveUpgradeTargetNodes_CLINodesWin pins the resolution @@ -260,3 +265,203 @@ func TestRunPostUpgradeVersionVerifyInner_NonEmptyNodes_WaitsAndVerifies(t *test t.Errorf("non-empty path should print the 'waiting' line, got %q", buf.String()) } } + +// TestDefaultPostUpgradeReconcileWindow_Is90s pins back-compat for +// #190: the upgrade flow defaulted to a hard-coded 90s wait before +// the flag was introduced; the new --post-upgrade-reconcile-window +// must register the same value as its default so operators who +// never pass the flag observe byte-identical timing. +func TestDefaultPostUpgradeReconcileWindow_Is90s(t *testing.T) { + t.Parallel() + + if defaultPostUpgradeReconcileWindow != 90*time.Second { + t.Errorf("default reconcile window changed: got %s, want 90s — back-compat regression (#190)", defaultPostUpgradeReconcileWindow) + } +} + +// TestUpgradeFlag_PostUpgradeReconcileWindow_Registered pins the +// flag registration: wrapUpgradeCommand must register a +// --post-upgrade-reconcile-window DurationVar with the 90s default, +// so `talm upgrade --help` discoverably surfaces the tunable. +func TestUpgradeFlag_PostUpgradeReconcileWindow_Registered(t *testing.T) { + t.Parallel() + + cmd := &cobra.Command{Use: upgradeCmdName} + wrapUpgradeCommand(cmd, nil) + + flag := cmd.Flag("post-upgrade-reconcile-window") + if flag == nil { + t.Fatal("--post-upgrade-reconcile-window must be registered by wrapUpgradeCommand") + } + + if flag.DefValue != "1m30s" { + t.Errorf("flag default rendered as %q, want %q (DurationVar formats 90s as 1m30s)", flag.DefValue, "1m30s") + } + + // Cobra auto-appends "(default )" to the rendered Usage + // line for any flag with a non-zero default. If the inline + // Usage string also carries "(default ...)", `--help` renders + // two confusing clauses. Pin that there is exactly one literal + // "(default" substring in the rendered line. + rendered := cmd.UsageString() + + if count := strings.Count(rendered, "(default"); count != 1 { + t.Errorf("--post-upgrade-reconcile-window: rendered Usage has %d '(default' clauses; want exactly 1 (cobra auto-appends, inline Usage must not duplicate)", count) + } +} + +// TestValidatePostUpgradeReconcileWindow_ZeroRejected pins the +// non-positive-rejection contract. Passing 0s would emit +// "waiting 0s for the node to finish booting..." and immediately +// fall through to the version-read loop while Talos is still +// rebooting — the test would always report "rollback" because +// the old version is still running. Reject explicitly with a +// hint pointing at sensible values. +func TestValidatePostUpgradeReconcileWindow_ZeroRejected(t *testing.T) { + t.Parallel() + + err := validatePostUpgradeReconcileWindow(0) + if err == nil { + t.Fatal("zero must be rejected — fall-through to version read while node is rebooting would always report rollback") + } + + hints := errors.GetAllHints(err) + + found := false + + for _, h := range hints { + if strings.Contains(h, "positive duration") { + found = true + + break + } + } + + if !found { + t.Errorf("hint must mention 'positive duration' so operators see a recovery path; got hints: %v", hints) + } +} + +// TestValidatePostUpgradeReconcileWindow_NegativeRejected is the +// boundary partner of the zero case. A negative DurationVar value +// (e.g. --post-upgrade-reconcile-window=-30s) parses fine via +// pflag but is operationally nonsensical; the validation must +// reject it with the same hint shape. +func TestValidatePostUpgradeReconcileWindow_NegativeRejected(t *testing.T) { + t.Parallel() + + err := validatePostUpgradeReconcileWindow(-30 * time.Second) + if err == nil { + t.Fatal("negative duration must be rejected") + } +} + +// TestValidatePostUpgradeReconcileWindow_PositiveAccepted pins the +// happy path: any positive duration (1 ms, 90 s, 10 m) is accepted. +// Without this case the validator could regress to always-reject +// and the failing-zero / failing-negative tests would still pass. +func TestValidatePostUpgradeReconcileWindow_PositiveAccepted(t *testing.T) { + t.Parallel() + + for _, d := range []time.Duration{time.Millisecond, 90 * time.Second, 10 * time.Minute} { + if err := validatePostUpgradeReconcileWindow(d); err != nil { + t.Errorf("positive duration %s must be accepted, got: %v", d, err) + } + } +} + +// TestWrapUpgradeCommand_BadReconcileWindow_FailsFastBeforeOriginalRunE +// pins fail-fast semantics on the reconcile-window flag: a zero or +// negative value must reject BEFORE the talosctl upgrade RPC fires. +// Validating only inside Phase 2C (after the RPC) would mean an +// operator's typo lands a partial upgrade before the validation +// surfaces, then surfaces a 'rollback' hint that's actually 'you +// passed 0s'. +// +// The test installs wrapUpgradeCommand with a sentinel originalRunE +// that flips a boolean if invoked, sets the flag to 0s, runs RunE, +// and asserts (1) error returned with the hint, (2) originalRunE +// was NOT called. +func TestWrapUpgradeCommand_BadReconcileWindow_FailsFastBeforeOriginalRunE(t *testing.T) { + saved := upgradeCmdFlags.postUpgradeReconcileWindow + + t.Cleanup(func() { upgradeCmdFlags.postUpgradeReconcileWindow = saved }) + + originalRunECalled := false + originalRunE := func(_ *cobra.Command, _ []string) error { + originalRunECalled = true + + return nil + } + + cmd := &cobra.Command{Use: upgradeCmdName} + wrapUpgradeCommand(cmd, originalRunE) + + upgradeCmdFlags.postUpgradeReconcileWindow = 0 + + err := cmd.RunE(cmd, nil) + if err == nil { + t.Fatal("expected fail-fast on 0s reconcile window, got nil") + } + + hints := errors.GetAllHints(err) + + found := false + + for _, h := range hints { + if strings.Contains(h, "positive duration") { + found = true + + break + } + } + + if !found { + t.Errorf("error must carry 'positive duration' hint; got hints: %v", hints) + } + + if originalRunECalled { + t.Error("originalRunE (talosctl upgrade RPC) must NOT be invoked when reconcile-window validation rejects the flag — operator's typo would land a partial upgrade") + } +} + +// TestReadmePostUpgradeVerify_NoHardcoded90s mirrors +// TestPostUpgradeVersionMismatchHint_NoHardcoded90s for the +// operator-facing README. The pre-#190 README claimed "waits 90s +// for the node to finish booting"; after #190 the window is +// operator-tunable via --post-upgrade-reconcile-window. The README +// bullet must reference "the configured reconcile window" rather +// than a literal 90s so an operator running with a custom window +// does not read contradictory documentation. Pin the absence of +// the literal so a future README edit re-introducing it fails this +// test. +func TestReadmePostUpgradeVerify_NoHardcoded90s(t *testing.T) { + t.Parallel() + + readmePath := filepath.Join("..", "..", "README.md") + + body, err := os.ReadFile(readmePath) + if err != nil { + t.Skipf("README.md not present at %s (likely a vendored source release without the repo layout): %v", readmePath, err) + } + + if strings.Contains(string(body), "waits 90s") { + t.Errorf("README.md must not hardcode 'waits 90s' — the post-upgrade reconcile window is operator-tunable via --post-upgrade-reconcile-window; replace with 'the configured reconcile window (default 90s, tune via --post-upgrade-reconcile-window)'") + } +} + +// TestPostUpgradeVersionMismatchHint_NoHardcoded90s catches future +// drift in the hint text. The original const baked "90s reconcile +// window" verbatim; with the new flag, operators running +// --post-upgrade-reconcile-window=180s would see "the 90s reconcile +// window" in the version-mismatch hint, which contradicts what +// they typed on the command line. Pin the absence of the literal +// "90s" so a future "helpful clarification" reintroducing it +// fails this test. +func TestPostUpgradeVersionMismatchHint_NoHardcoded90s(t *testing.T) { + t.Parallel() + + if strings.Contains(postUpgradeVersionMismatchHint, "90s") { + t.Errorf("hint must not hardcode 90s — operators passing --post-upgrade-reconcile-window= see misleading copy; got: %q", postUpgradeVersionMismatchHint) + } +}