Remove semantic re-validation of values received from trusted peers
A sweep of the codebase (done during the review work for #4264) found a recurring pattern: code that semantically re-validates values received from a trusted peer, or re-checks invariants that the producer of the value already guarantees. We don't want to check trusted values. Structural handling stays (a parse that must happen to use a value can fail, and that error propagates), but range checks, plausibility checks, "sanity checks", and accusatory error branches on data from a trusted source are noise: they add dead branches, imply a threat model we don't defend against anywhere else, and mislead readers about which edges are actually untrusted.
The trusted edges are: client consuming traffic-manager and traffic-agent responses (authenticated connections), CLI consuming daemon responses, user daemon and root daemon consuming each other (versions are locked in step and guarded), manager consuming Kubernetes API server objects (already validated by the API server), agent consuming manager-generated config, and any component consuming values it produced itself. Validation of client-sent requests on the manager side, authentication and authorization checks, and handling of genuinely possible Kubernetes states (headless services, informer races, deleted objects) are all correct and are not part of this issue.
Line numbers are as of current release/v2; findings are anchored to function names since lines drift.
Client side
pkg/client/k8s/x509_token.go — three checks on the manager's x509 auth response
The cache-expiry clamp, whose own comment states the distrust:
x509TokenSourceMaxTTLexists "so a misbehaving manager cannot mint a token that is cached far into the future". The token is only ever presented back to the same manager; trust the expiry the manager supplies (minus the existing safety margin) and drop the clamp.An explicit size-limit rejection of the response body: the
io.LimitReaderalone is a structural read bound, but the code reads limit+1 bytes specifically so it can reject with "manager x509 auth response exceeds N bytes". Read with a plain bound and let the JSON parse fail structurally if anything is off.An accusatory empty-token check ("manager x509 auth response carried no token") after the protocol's own error channel (
resp.Error) was already consulted. An empty token would simply produce a normalUnauthenticatedon the next RPC.
pkg/client/userd/trafficmgr/ingest.go — ingest() checks that the manager "honored" node-agent mode
After a successful EnsureAgent, the client verifies ai.NodeAgent matches what was requested and fails with "traffic-manager did not honor node-agent mode". The comment even hypothesizes that the version gate a few lines earlier "was bypassed". The same response is trusted enough to index as.Agents[0] unguarded. Remove the check; the version gate is the mechanism, and the manager's answer is authoritative.
pkg/client/rootd/intercept_shortcuts.go — root daemon second-guesses the user daemon
validAddrPort adds a semantic !ap.IsValid() on top of the structural UnmarshalBinary, newShortcutTable re-checks sc.ContainerPort > 0, and invalid entries are skipped with a warning. The stated rationale is "so that a newer user daemon doesn't lose the feature entirely when this daemon doesn't understand one of its entries" — a scenario the lock-step daemon version guard makes impossible. Reduce to plain unmarshal-and-use; propagate a structural unmarshal error instead of the skip-with-warning machinery.
pkg/client/portforward/podaddr.go — parseAddr runs uuid.Parse on a self-generated UID
The ~<uid> suffix of a k8spf dial address is written by the client itself, from pod UIDs supplied by the manager or the API server. The grammar only ever carries the no-lookup marker or a real UID, so the uuid.Parse filter (which silently drops a non-UUID and degrades the address to a GetPod lookup) can go: anything after the separator that isn't the marker is the UID.
Cluster side
cmd/traffic/cmd/manager/data.go — validateAgent/validateMechanisms re-validate the manager's own output
Called from ArriveAsAgent. Name and Namespace come from the agent config the manager itself generated, Product is a literal, Version is the build stamp, and Mechanisms is a hardcoded two-element slice in the agent binary. Nothing checked here can be wrong unless the manager generated a broken config.
cmd/traffic/cmd/manager/managerutil/data.go — "sanity check" comparisons that are true by construction
The golden-vs-agent name comparison is unconditionally true at its only call site (state.go filters the list by name and namespace before calling), and the mechanism-name uniqueness check runs over that compile-time-constant two-element list.
cmd/traffic/cmd/manager/state/nodeagent.go — the same invariants checked repeatedly
buildNodeAgentJobre-checks preconditions (nodeName,containerIDs,podIP) that its only caller and the producingnodeAgentTargetsalready established — the scheduler setsSpec.NodeNamefor any Running pod, and the pod IP was already checked upstream.- The pod-admissibility loop re-checks
pod.Status.PodIP == ""on a Running-and-Ready pod, with a comment conceding it is "an invariant violation, not a normal admissibility failure". - The staleness classifier returns
nodeAgentStaleFatal, "desired job has no container"for the Job thatbuildNodeAgentJobjust built, which always contains exactly one container.
cmd/traffic/cmd/agent/nodeagent_linux.go — node agent re-checks the manager's container-ID map
The container-IDs env and the agent config are both written onto the Job by the manager, which refuses to build the Job unless every configured container has an ID. The "no container ID for container %q" branch re-derives that guarantee.
cmd/traffic/cmd/manager/state/workload_exposure.go — nil/empty checks on values the neighboring producer filtered
appendMatch guards service == nil || service.GetName() == "", but ingressBackendMatch never returns a nil service with ok true, and every service came out of serviceIndex, which applied the identical filter. The port != nil checks in ingressBackendMatch run over a slice the same file built without nils.
pkg/agentconfig/intercepttarget.go — panicking assertion over manager-generated grouping
NewInterceptTarget panics if the []*Intercept group is empty or mixes ports/protocols — an invariant the manager's generator establishes. It escalates distrust of the manager to a panic in the agent. (The constructor currently has no non-test caller at all, so this may reduce to deleting dead code.)
cmd/traffic/cmd/agent/quic.go — manager-written port env treated as possibly malformed
An unset AGENT_QUIC_PORT is a genuine state (older or QUIC-less manager) and is handled; a set-but-unparseable value can only come from a manager that wrote garbage, and is currently silently discarded. Low severity, listed for completeness.
Suggested approach
These group naturally into a few small PRs: the x509 trio, the node-agent cluster (manager state + agent side), the arrival-validation pair, and the remaining one-liners. Each removal should keep structural error propagation intact and only delete the semantic second-guessing.
Source: telepresenceio/telepresence