mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-06 06:51:29 +02:00
c3cf7c0c37
* [client] Clarify that metrics ingest X-Peer-ID is not a credential The ingest endpoint is intentionally unauthenticated: it accepts telemetry from peers of both cloud and self-hosted deployments, and for a self-hosted peer there is no shared trust anchor to authenticate against. The X-Peer-ID header is a correlation tag whose format check exists to bound InfluxDB tag cardinality. Both the function name (validateAuth) and the 401 response implied an authentication control that was never there, which invites the reading that the check can be bypassed. Rename it to validatePeerIDFormat and return 400, matching the other input validation failures in the same handler. Document the intent in the godoc and the infra README. No behavioural change for clients: push.go classifies responses by 2xx range rather than by status code, so 400 and 401 are handled identically. * [client] Reject metrics ingest bodies whose peer_id tag disagrees with the header validateTag checked tag names against the per-measurement allowlist but only bounded the value length, so the peer_id tag was free-form text up to 64 bytes and could differ from the X-Peer-ID header the request was accepted with. Tie the two together: the tag value must equal the header value. Since the header is already checked to be 16 hex characters, this transitively constrains the tag to the same shape. Every client sends the same value in both places (metrics.go feeds agentInfo.peerID to both push.SetPeerID and the body tags), so well-behaved clients are unaffected. The mismatch is rejected rather than silently overwritten: rewriting the value would re-serialize caller-controlled text back into line protocol and would hide misbehaving senders instead of surfacing them. Rejection also matches the other input validation failures in the same handler, which all return 400. This narrows the value space of the peer_id tag but does not by itself bound InfluxDB series cardinality: a sender that puts the same arbitrary 16 hex characters in both the header and the body still passes. Limiting that needs a per-source rate limit in front of the service. * [client] Document what the metrics ingest peer_id check does and does not bound The README described X-Peer-ID as the correlation tag, but grouping is done by the peer_id tag in the submitted line protocol: that is what is forwarded to InfluxDB, while the header only serves as the value each tag is checked against. It also claimed the format check bounds tag cardinality. It bounds the value space of the tag, not the number of distinct series, so state that explicitly and point out that series cardinality has to be limited outside this service. * [client] Set timeouts on the metrics ingest HTTP server The server ran on http.ListenAndServe with no timeouts, silenced with a nolint:gosec for G114. Without ReadHeaderTimeout a client can hold a connection open by sending headers slowly, and without ReadTimeout or IdleTimeout connections accumulate on an endpoint that takes unauthenticated requests. Construct an http.Server with explicit limits instead, which also drops the nolint. Handler stays nil so the existing DefaultServeMux registrations are unaffected. WriteTimeout is deliberately larger than the 10s upstream client timeout: the response is only written after the forward to InfluxDB completes, so a tighter value would cut off the server's own valid response. * Revert "[client] Document what the metrics ingest peer_id check does and does not bound" This reverts commit837a5d8dda. * Revert "[client] Reject metrics ingest bodies whose peer_id tag disagrees with the header" This reverts commit91d4f6128. Tying the body peer_id tag to the X-Peer-ID header assumed the two always agree, but a profile switch breaks that. UpdateAgentInfo swaps agentInfo.peerID and calls push.SetPeerID with the new value while leaving the sample buffer alone, and the peer_id is baked into each buffered line at record time, so samples from the previous profile ship under the new header. The consequences compound: validateLineProtocol rejects the whole batch on the first bad line, so fresh samples are dropped along with the stale ones, and push.go only resets the buffer after a successful push, so the batch is retried and fails again. Metrics from that client stay stuck until the old samples age out of the buffer, up to maxSampleAge (5 days). Deciding whether the previous profile's unsent samples may be discarded, or whether the push has to be partitioned per identity, is a product call, so restore the previous behaviour for now. The header keeps its format check; the body peer_id tag goes back to being bounded only by maxTagValueLength. * [client] Stop claiming the metrics ingest peer ID format check bounds tag cardinality The X-Peer-ID header is never forwarded to InfluxDB; the stored peer_id tag comes from the request body and is constrained only by the tag allowlist and the maximum tag value length. Align the README and the validatePeerIDFormat godoc with the actual behavior after the header/body match check was reverted.