trident-acl-agent: verify Node systemUUID matches local hardware

[bfjelds (bfjelds)]

Fixes trident-acl-agent so a stale/recycled Node name can't be mistaken for this machine's own Node object. Opt-in, disabled by default.

Change

• NodeClient::get_node and NodeClient::watch_node can verify the fetched Node's status.nodeInfo.systemUUID against this host's own /sys/class/dmi/id/product_uuid (the same source kubelet uses to populate systemUUID). On mismatch, logs a warning with both UUIDs and returns K8sClientError::NodeGone, so every caller - connection check, startup recovery, reconcile loop, and the long-lived watch stream - treats a misidentified Node exactly like a missing one.
• Verification is skipped (logged at warn), not treated as a mismatch, when the Node's systemUUID is empty or the local product_uuid can't be read - either case only means the UUID is undetermined, not that the Node describes a different machine.
• Gated behind TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID, a real boolean config field (loaded via envy, like every other trident-acl-agent setting) that must be set to the literal string true to enable verification. Disabled (false) by default: the assumption that kubelet's systemUUID always traces back to this machine's product_uuid hasn't been confirmed across every environment this agent runs in, so it's opt-in per-deployment rather than unconditional.
• Extracted the DMI product_uuid path/read into a shared osutils::dmi helper, now reused by trident-acl-agent and trident's tracestream metadata instead of each defining its own copy of the path and read-with-fallback logic.
• Documented the config field and behavior (including watch-stream coverage) in docs/Explanation/Trident-ACL-Agent.md.

Testing

• https://dev.azure.com/mariner-org/ECF/_build/results?buildId=1220485&view=results
• Re-validated end-to-end against a live VM after the env-var-to-envy-config migration: rebuilt the trident-acl RPM and VM image, ran the full storm-trident run aclagent suite - 7/7 PASS, including run-node-resilience's systemUUID mismatch phases.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

Watched Node objects bypass identity verification, allowing reconciliation against recycled Nodes.

2 open findings

• Watch events bypass Node identity verification · New
• Move product UUID probing into osutils · New

What changed in this PR

Adds hardware UUID validation to prevent the ACL agent from accepting a stale Kubernetes Node object.

Changes:

• Compares Node and local DMI system UUIDs.
• Treats mismatches as NodeGone.
• Adds focused identity-verification tests.

File	Description
crates/​trident-acl-agent/​src/​annotations/​k8s.rs	Adds Node identity verification and tests.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Watch events bypass identity verification and can still trigger servicing for a same-name replacement Node.

2 open findings

• Watch events bypass Node identity verification
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Added run-node-resilience phase 3 (storm E2E) + fast Go unit tests covering the systemUUID-mismatch case end-to-end: sets a mismatched systemUUID on the fake Node, restarts trident-acl-agent, and confirms it (a) logs the mismatch distinctly from a plain 404 and (b) treats it the same as NodeGone — no crash, same PID, resumes once the systemUUID matches again. See tools/storm/aclagent/tests/node_resilience.go and proxies/apiserver_test.go.

[Copilot]

🔵 Needs a closer look

Long-lived watch events bypass identity verification and can still trigger operations for mismatched hardware.

2 open findings

• Watch events bypass Node identity verification
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Pushed a follow-up fix after investigating whether AKS nodes guarantee status.nodeInfo.systemUUID matches /sys/class/dmi/id/product_uuid:

Confirmed via cadvisor source that kubelet populates systemUUID by reading the exact same file we check against. However, if cadvisor fails to read that file on the kubelet side, it logs an error but still lets node registration proceed with an empty systemUUID rather than failing. An empty value only means kubelet could not determine the UUID - it is not evidence the Node describes a different machine.

Updated verify_node_identity to skip verification (warn + Ok) when the Node reports an empty systemUUID, same as when our local file is unreadable, instead of treating it as a mismatch. Without this, a rare kubelet-side DMI read failure could have falsely locked us out of an otherwise-healthy node forever.

Updated/added unit tests accordingly (missing_node_system_info_skips_verification, empty_node_system_uuid_skips_verification).

[Copilot]

🟡 Changes recommended

Watch-delivered Nodes still bypass identity verification and can trigger reconciliation for mismatched hardware.

3 open findings

• Watch events bypass Node identity verification
• Update empty systemUUID verification comments · New
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Fixed the storm run-node-resilience failure from build 1220454: readVmProductUUID ran a plain cat /sys/class/dmi/id/product_uuid over SSH as the non-root test user, which hit Permission denied (file is 0400 root-only) and aborted the whole aclagent scenario before any assertions ran. Every other privileged command in this harness already uses sudo; added it here too.

[Copilot]

🔵 Needs a closer look

Watch-delivered Nodes remain unverified and can still trigger servicing against recycled hardware.

3 open findings

• Watch events bypass Node identity verification
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Watch-delivered Nodes still bypass identity validation and can trigger reconciliation for recycled hardware.

3 open findings

• Watch events bypass Node identity verification
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[Copilot]

🟢 Approval recommended

The opt-in validation is consistently applied, documented, and covered by unit and VM-level tests.

1 open finding

• Test watch-stream validation without restarting

5 resolved since last review

• Watch events bypass Node identity verification
• Enable Node identity verification by default
• Use the shared DMI product UUID reader
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Both presence-only environment checks incorrectly disable validation when the variable contains non-UTF-8 bytes.

0 open findings

1 resolved since last review

• Test watch-stream validation without restarting

Previously missed (1)

In code that hasn't changed since last review

Use Unicode-safe environment presence check in get_node

crates/​trident-acl-agent/​src/​annotations/​k8s.rs:97

Issue: This presence-only gate still depends on the value being valid Unicode. Evidence: std::env::var returns Err(VarError::NotUnicode) for a present non-UTF-8 value, so get_node silently disables validation despite the documented “value is ignored” contract. Suggestion: use var_os(...).is_some() to test presence without decoding the value.

This issue also appears on line 170 of the same file.

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Addressed in 7dc0304 — and more thoroughly than just the var_os swap originally suggested. Migrated TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID onto this crate's established envy-based config system: renamed to TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID, added a real validate_node_uuid: bool field to KubernetesConfig, loaded through envy::prefixed(...) like every other Kubernetes setting. NodeClient now stores this as a plain bool at construction instead of re-reading the environment on every get_node/watch_node call, which also fixes the Unicode-safety gap for free (envy's std::env::vars()-based collection skips non-UTF8 pairs rather than erroring, consistent with "malformed/absent" handling everywhere else in this config system).

Validated end-to-end against a live VM: rebuilt the trident-acl RPM and trident-vm-acl-agent-testimage.qcow2 with this change, and re-ran the full storm-trident run aclagent suite - all 7 test cases pass, including run-node-resilience's systemUUID mismatch phases (3 and 4), now driven by the new config field.

[Copilot]

🟡 Changes recommended

Kubernetes writes remain vulnerable to a Node-recycling race despite the new read validation.

1 open finding

• Correct deployment variable name and boolean configuration contract · New

Previously missed (1)

In code that hasn't changed since last review

Validate node identity before writes and enforce atomic preconditions

crates/​trident-acl-agent/​src/​annotations/​k8s.rs:92

Issue: The identity check protects GET/watch inputs but not writes, so a Node name recycled during an in-flight operation can still be mutated by this host. Evidence: servicing pauses watch consumption while reconcile_node runs, and run_with_status_heartbeat calls publish_status repeatedly; patch_node_metadata patches by name without verifying identity or using a precondition. A replacement can therefore receive this machine's status annotations before its queued watch event is checked. Suggestion: when validation is enabled, base each PATCH on a verified read and add an atomic server-side precondition (for example, the verified object's resourceVersion), mapping mismatch/conflict to NodeGone.

🧠 Review effort: Balanced

[Copilot]

🟢 Approval recommended

The implementation and coverage are coherent; only a non-blocking inaccurate test comment remains.

1 open finding

• Correct recovery description to reflect GET-based node recreation detection · New

1 resolved since last review

• Correct deployment variable name and boolean configuration contract

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

An empty readable local UUID incorrectly causes an indefinite NodeGone recovery loop.

2 open findings

• Treat empty local product UUID as undetermined, not a mismatch · New
• Correct recovery description to reflect GET-based node recreation detection

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Pending-commit startup recovery can still perform side effects and publish to a mismatched Node before UUID validation occurs.

1 open finding

• Pending-commit recovery bypasses UUID validation before Node mutations · New

2 resolved since last review

• Treat empty local product UUID as undetermined, not a mismatch
• Correct recovery description to reflect GET-based node recreation detection

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

The shared reader changes the established telemetry asset ID representation for valid uppercase UUIDs.

1 open finding

• Pending-commit recovery bypasses UUID validation before Node mutations

Previously missed (1)

In code that hasn't changed since last review

UUID normalization changes telemetry asset_id representation

crates/​trident/​src/​logging/​tracestream.rs:355

Issue: This changes the existing telemetry asset_id representation instead of only sharing its reader. Evidence: dmi::read_product_uuid parses the raw text into OsUuid, whose Display canonicalizes valid UUIDs to lowercase (crates/sysdefs/src/osuuid.rs:36-40,57-62); the removed reader returned the trimmed sysfs text verbatim. An uppercase product UUID therefore acquires a different asset ID after upgrade, and PLATFORM_INFO is reused in telemetry and diagnostics. Suggestion: preserve the trimmed raw value for the telemetry path and parse/normalize only for the Kubernetes identity comparison, or expose separate raw and typed DMI readers.

🧠 Review effort: Balanced