feat(relay): report client app, platform, and version for live connections - #4543
Open
atishpatel wants to merge 2 commits into
Open
feat(relay): report client app, platform, and version for live connections#4543atishpatel wants to merge 2 commits into
atishpatel wants to merge 2 commits into
Conversation
…tions
The relay could not tell which application, platform, or version its
connections came from. `buzz_ws_connections_active` carries no labels and
`buzz_ws_connections_total` carries only `community`, so questions like
"how many macOS desktop users are on 0.4?" or "is anyone still on the old
CLI?" were unanswerable, and no client sent identifying handshake
metadata for the relay to read.
Rust clients now announce themselves with an advisory RFC 8941
`Buzz-Client` header, and the relay turns it into labeled metrics:
Buzz-Client: v=1, app=buzz-desktop, platform=macos, app-version="0.5.2"
buzz_client_connections_total{app,platform,app_version}
buzz_client_connections_active{app,platform,app_version}
buzz_client_header_parse_failures_total{reason}
Senders are wired for desktop, buzz-ws-client (CLI and test client), and
the buzz-acp harness. The vocabularies and serializer live in buzz-core
so all of them share one definition.
The live gauge is deliberately a new series rather than labels on
`buzz_ws_connections_active`: that gauge is the HPA scaling input
(`deploy/charts/buzz/values.yaml` `websocketMetricName`, an unlabeled
`AverageValue` target), and labeling it would shard the series the
autoscaler reads. `deploy/` is untouched and the chart's HPA unit tests
still pass. Retired label sets are dropped by the recorder's existing
gauge idle timeout.
Complements #2596, which adds the mobile sender and its own relay-side
parser. This change is based on main rather than on that branch, and the
parser ignores unknown dictionary keys, so it already accepts mobile's
richer header (`app-build`, `os-version`, `os-api`) unchanged. Widening
the allowlists to desktop/CLI/ACP and macOS/Windows/Linux, adding the
live gauge, and bucketing unidentified connections as `unknown` so the
totals reconcile are the gaps this closes.
Trust and cardinality: the header arrives before NIP-42 AUTH and is
forgeable, so it is advisory only and never touches authentication,
authorization, tenant selection, or admission. `app` and `platform`
resolve to `&'static str` from closed allowlists and `app_version` is
narrowed to bounded MAJOR.MINOR, so a forged header cannot introduce an
unbounded label value. Clients send the header only over TLS or to
loopback, so it is not exposed to on-path observers on cleartext
connections.
Signed-off-by: npub1x3mmseqygyar04742djuepgk0t2d2t4chzm9sl0hl4vlc2m9whvqza9e5y <3477b86404413a37d7d55365cc85167ad4d52eb8b8b6587df7fd59fc2b6575d8@buzz.block.builderlab.xyz>
Co-authored-by: Atish Patel <atish@squareup.com>
Signed-off-by: Atish Patel <atish@squareup.com>
…e counter Review of #4543 found two problems with how the new client identity reached Prometheus. Both are fixed here, and the parser and sender lost the hand-rolled code the review flagged as removable. Only a real release version is reported. Every sender passed `env!("CARGO_PKG_VERSION")`, but only the desktop app has an independently bumped version: `buzz-ws-client` and `buzz-acp` use `version.workspace = true`, and the workspace version has never been bumped, because `RELEASING.md` "Version Sources" gives release authority only to the desktop manifests and `crates/buzz-relay/Cargo.toml`. So CLI and harness connections would have reported a fixed version forever and a dashboard would have read that as "nobody ever upgrades". `app_version` is now `Option<&str>`; those clients pass `None`, the `app-version` member is omitted from the header, and the relay labels the version `unknown`, which is accurate. A version that is *present* but unusable is still a parse failure, so a genuinely broken version stays visible. A wrong version is worse than no version. Only a gauge is emitted. `buzz_client_connections_total` is removed. The header arrives before NIP-42 AUTH and is forgeable, and the recorder's `idle_timeout` is configured for `MetricKindMask::GAUGE` only, so counter series would have been retained for the process lifetime while gauge series self-clean. Verified with a throwaway probe: 10,000 forged headers produced 10,000 series; after the idle timeout the counter kept all 10,000 and the gauge went to 0. The label alphabet was never the real bound — the metric kind is. Connection rate is left to the existing `buzz_ws_connections_total`, which is not attacker-labeled. Also, per review: - `may_identify_to` takes a parsed `url::Url` and matches on scheme plus `url::Host`, replacing hand-rolled scheme splitting, userinfo stripping, and IPv6 bracket handling. Both callers already parsed the URL, so they now parse once. `url` was already a dependency. - The three duplicated label emitters collapse into one `active_gauge`. - `app_version_detail`, `MAX_LOGGED_VALUE_LEN`, and `truncate_for_log` are gone; the connection log uses the label-safe version. - `MAX_HEADER_LEN` and `MAX_APP_VERSION_LEN` are single constants exported from `buzz-core`, replacing the relay's divergent 512/32. A test proves the longest header the builder can emit still parses, so the two halves cannot drift into counting real clients as failures. `deploy/` is still a zero diff and the HPA series is still unlabeled. Signed-off-by: npub1x3mmseqygyar04742djuepgk0t2d2t4chzm9sl0hl4vlc2m9whvqza9e5y <3477b86404413a37d7d55365cc85167ad4d52eb8b8b6587df7fd59fc2b6575d8@buzz.block.builderlab.xyz> Co-authored-by: Atish Patel <atish@squareup.com> Signed-off-by: Atish Patel <atish@squareup.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The relay can't tell which app, platform, or version its connections come from.
buzz_ws_connections_activehas no labels andbuzz_ws_connections_totalhas onlycommunity, so "how many macOS desktop users are on 0.4?" or "is anyone still on an old CLI?" are unanswerable today — and no client sends identifying handshake metadata for the relay to read.Rust clients now announce themselves with an advisory RFC 8941
Buzz-Clientheader, and the relay turns it into labeled metrics:Senders are wired for desktop, buzz-ws-client (CLI + test client), and the buzz-acp harness. Vocabularies and the serializer live in
buzz-coreso every client shares one definition.Only a version the client can actually bump is reported
app_versionisOption<&str>, and only the desktop app passes a value.Per
RELEASING.md("Version Sources"), release authority sits with the desktop manifests andcrates/buzz-relay/Cargo.toml.buzz-ws-clientandbuzz-acpuseversion.workspace = true, and the workspace version has never been bumped — soenv!("CARGO_PKG_VERSION")would pin every CLI and harness connection to a fixed number forever, and a dashboard would read that as "nobody ever upgrades." Those clients omit theapp-versionmember and the relay labels the versionunknown, which is accurate. A version that is present but unusable is still a parse failure, so a genuinely broken version stays visible rather than being silently downgraded.Giving the CLI a real version is a separate question this PR doesn't answer: today "what version is the CLI" has no answer in this repo.
Gauge only, no counter
Only a gauge is emitted, deliberately. The header arrives before NIP-42 AUTH and is forgeable, and the recorder's
idle_timeoutis configured forMetricKindMask::GAUGEonly (metrics.rs). Verified with a throwaway probe: 10,000 forged headers produced 10,000 series; after the idle timeout and an upkeep pass, the counter still held all 10,000 while the gauge dropped to 0.So the real bound on a forgeable label set is the metric kind, not the label alphabet. Connection rate is left to the existing
buzz_ws_connections_total, which carries no attacker-controlled label.The HPA decision
I deliberately did not add labels to
buzz_ws_connections_active, which is what I'd originally proposed. That gauge is the HPA scaling input —deploy/charts/buzz/values.yamlwebsocketMetricName, consumed as an unlabeledtype: Pods/AverageValuetarget (5000) and asserted indeploy/charts/buzz/tests/hpa_test.yaml. Labeling it would shard the series the autoscaler reads and risk breaking scaling.Instead,
buzz_client_connections_activeis a parallel gauge with the same lifecycle.deploy/is untouched.Trust
The header is advisory only — it never touches authentication, authorization, tenant selection, or admission. Nothing on this path can cause a connection to be rejected: a missing header is normal and silent, a malformed one is counted and discarded.
appandplatformresolve to&'static strfrom closed allowlists; an unrecognized token yields no label, never a passthrough of attacker bytes.app=buzz-desktop-9e1fis rejected, not counted.app_versionis narrowed toMAJOR.MINORwith each component digits-only and length-bounded, so patch releases don't each create a series.reasonvalues are a fixed set.Unidentified connections land in an explicit
unknownbucket, so "unidentified" is a visible dashboard line rather than a silent gap andsum(buzz_client_connections_active)reconciles withbuzz_ws_connections_active.Clients send the header only over TLS or to loopback, so it isn't exposed to on-path observers on cleartext connections. That gate matches on
url::Url's scheme andurl::Host, so userinfo, IPv6 literals, ports, and percent-encoding are theurlcrate's problem —ws://localhost@evil.example.com/andws://127.0.0.1.example.com/are both refused.MAX_HEADER_LENandMAX_APP_VERSION_LENare single constants exported frombuzz-coreand consumed by the relay, and a test proves the longest header the builder can emit still parses — the two halves can't drift into counting real clients as parse failures.Relationship to #2596
#2596 adds the mobile sender plus its own relay-side parser. This PR is based on
main, not on that branch — #2596 is conflicting and owned by another author, so rebasing it wasn't mine to do. The two are complementary:app-build,os-version,os-api) unchanged — there's a test asserting exactly that.buzz-desktop/buzz-cli/buzz-acpandmacos/windows/linux; a live gauge rather than a connection counter; and the explicitunknownbucket.Whichever lands second will need a small merge in
connection.rs/router.rsand a choice of one parser. Mobile needs no change to be counted by this one.No new dependency
#2596 pulls in
sfvfor RFC 8941 parsing. This PR hand-rolls a strict reader for just the accepted dictionary subset (quote-aware member splitting, rejects control characters and backslash escapes rather than unescaping them). For an unauthenticated pre-auth parser I'd rather have something fully auditable and no new supply-chain surface. Happy to switch tosfvif reviewers prefer.Validation
cargo test -p buzz-core— 250 unit + 4 doctests passed, including both changed doc examplescargo test -p buzz-relay— 859 passed, 37 ignoredcargo test -p buzz-ws-client— 3 passedcargo clippy --workspace --all-targets -- -D warnings— clean; same for the desktop Tauri cratecargo fmt --all --check— cleanhelm unittest deploy/charts/buzz— 45 passed / 9 suites, including the HPA suite;deploy/diff is 0 linesMobileandWebskip, confirming the diff doesn't touch those paths)buzz-corebuilder and the relay parser agree across every app × platform combination, with and without a version, so the cross-crate wire contract can't silently driftOne known local-only failure:
api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echoreturns 504 on my machine. It reproduces deterministically on a completely clean tree with zero local changes (verified by stashing this work and re-running at the parent commit) — a cold-start QUIC handshake timing artifact, not caused by this change. It passes in CI.Not included
Mobile sending (left to #2596) and web (
nostr-client.ts— browsers can't set custom headers onWebSocket, so that path needs a different mechanism such as a subprotocol token). Web therefore lands in theunknownbucket, which is now visible rather than silent. Flutter isn't installed in my environment, so I couldn't have validated mobile Dart tests anyway.