Skip to content

Cargo audit fixes#615

Merged
illuzen merged 7 commits into
mainfrom
chore/cargo-audit-fixes
Jul 3, 2026
Merged

Cargo audit fixes#615
illuzen merged 7 commits into
mainfrom
chore/cargo-audit-fixes

Conversation

@illuzen

@illuzen illuzen commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Fix Security Vulnerabilities: Remove libp2p from sc-telemetry

Summary

This PR addresses security vulnerabilities identified by cargo audit by removing the libp2p dependency from sc-telemetry and bumping quinn to fix a related CVE.

Changes

1. Bump quinn 0.11.9 → 0.11.11

  • Fixes RUSTSEC-2026-0185 (quinn-proto vulnerability)

2. Vendor and rewrite sc-telemetry to remove libp2p dependency

  • Created client/telemetry/ with a complete rewrite using pure tokio + tungstenite
  • Removed all libp2p transitive dependencies from the telemetry subsystem
  • Maintains full API compatibility with upstream sc-telemetry

Security Impact

Fixed in this PR:

  • RUSTSEC-2026-0185: quinn-proto vulnerability

Removed attack surface:

  • All libp2p-related CVEs no longer apply to telemetry (telemetry was the last remaining libp2p user after our litep2p migration)

Remaining Vulnerabilities (out of scope)

The following vulnerabilities remain and will be addressed in a future PR:

Crate CVE Severity Notes
wasmtime 35.0.0 RUSTSEC-2026-0095 Critical Sandbox escape on aarch64 Cranelift
wasmtime 35.0.0 RUSTSEC-2026-0096 Critical Winch backend data leakage
wasmtime 35.0.0 RUSTSEC-2026-0097+ Medium Resource exhaustion, panics
rustls-webpki 0.103.6 Multiple Low-Medium CRL/name constraint issues
tokio 0.2.25 RUSTSEC-2020-0019 Low Data race in oneshot (old transitive dep)

The wasmtime CVEs require a coordinated polkadot-sdk version bump (sc-executor 0.47.0 → 0.49.0) which affects multiple interdependent crates.

Testing

  • cargo check passes
  • cargo audit confirms libp2p CVEs no longer appear
  • Full test suite

Files Changed

  • Cargo.toml - quinn version bump, sc-telemetry patch
  • Cargo.lock - Dependency updates (~800

Note

Medium Risk
Telemetry networking was reimplemented and vendored; behavior should match upstream but WebSocket/TLS edge cases and reconnect semantics warrant validation in integration tests.

Overview
Addresses cargo audit findings by removing libp2p from the telemetry path and patching RUSTSEC-2026-0185 in quinn.

quinn is bumped from 0.11.9 to 0.11.11 in the workspace; Cargo.lock also updates quinn-proto and deduplicates several HTTP/TLS crates.

sc-telemetry is added under client/telemetry/ and patched in [patch.crates-io] so the node uses the in-tree crate instead of crates.io. The implementation keeps the same public API (TelemetryWorker, handles, telemetry! macro) but sends data over WebSockets via tokio-tungstenite instead of libp2p. Endpoints accept ws/wss URLs and still support legacy multiaddr-style strings via conversion in endpoints.rs. Per-server connections auto-reconnect with backoff in node.rs.

Cargo.lock drops the large libp2p subtree (and related networking stacks) that previously came in through upstream telemetry, shrinking the dependency graph for that subsystem.

Reviewed by Cursor Bugbot for commit dedba10. Configure here.

illuzen added 5 commits July 1, 2026 19:56
Vendored from polkadot-sdk for subsequent modification to remove
libp2p dependency. This commit contains the original source to make
changes auditable via git diff.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit dedba10. Configure here.

Comment thread client/telemetry/src/endpoints.rs
@illuzen

illuzen commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

v12 pls audit

@n13

n13 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Review

Verdict: Approve — non-blocking comments below. The security win is real, the API surface is fully preserved, and CI is green across the board.

What was checked

  • Vendoring approach: Vendoring upstream sc-telemetry v30.0.0 unchanged (b88590a) and rewriting in a separate commit (fe3366c) made the rewrite reviewable as a diff against upstream. The Node reconnect state machine, buffer semantics, and 10–20s jittered backoff are line-for-line equivalent to upstream; only the transport layer changed (libp2p → tokio-tungstenite).
  • API compatibility: All consumers in the repo (node/src/service.rs, node/src/chain_spec.rs, client/service, client/cli) use only preserved API (TelemetryWorker::new, handle, new_telemetry, start_telemetry, TelemetryEndpoints::new, telemetry!, SysInfo, Error). The [patch.crates-io] entry correctly redirects all 8 dependent crates in the lock tree to the in-tree crate.
  • Endpoint compatibility: The production chain-spec format /dns/shard-telemetry.quantus.cat/tcp/443/x-parity-wss/%2Fsubmit%2F parses correctly (incl. x-parity-wss and the percent-encoded path fixed in 393f979), and there's a test covering exactly that shape. Round-trip compat holds in both directions: specs serialized by the new code emit wss:// URLs, which older binaries can still parse via their multiaddr::from_url fallback.
  • Lockfile: quinn 0.11.11 / quinn-proto bump lands as described (RUSTSEC-2026-0185), and the entire libp2p subtree is gone from the lock tree.

Main observation (worth a soak test)

The read half of the WebSocket is never polled. Node only uses the Sink half of WsTransport; the Stream impl in transport.rs is dead code. tungstenite only queues Pong replies when a Ping frame is read, so if shard-telemetry uses Ping-based keepalive, the server will drop connections that never Pong, and the node will silently cycle through the 10–20s reconnect loop, losing messages in between. Server Close frames are also unnoticed until a write fails. Upstream had the same node-level structure (soketto behind libp2p also processes control frames on read), so this is likely fine in practice — but a soak test against the live shard-telemetry endpoint confirming a connection stays up for several minutes would settle it. If it turns out to be a problem, draining poll_next inside Node::poll_ready (discarding inbound messages) is a small fix that also answers Pings.

Minor / non-blocking

  • percent_decode in endpoints.rs hand-rolls what the percent-encoding crate (already in the dep tree via url) provides: percent_decode_str(s).decode_utf8_lossy().
  • initialize_transport() is now a no-op returning Ok(()); the "early error detection" call in TelemetryWorker::new and its comment are vestigial and could be dropped (the Error::IoError variant can stay for API compat).
  • In multiaddr_to_url, path segments are only appended when secure || port.is_some(), so a ws multiaddr without an explicit tcp port silently drops its path instead of erroring. No current chain spec hits this, but silent drops are worse than a parse failure.
  • Stream::poll_next returns Poll::Pending with an immediate wake_by_ref() on Ping/Pong frames — a loop would avoid the wasted re-poll. Moot while the stream is unpolled.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This would only impact telemetry right? Networking changes can be a little tricky as we have seen with the DNS problems that were in litep2p (not even litep2p but the hickory package which was updated and ended up worse than the original)

- drain the WebSocket read half in Node::poll_ready so tungstenite
  answers server Pings and server Close frames are detected without
  waiting for a write failure
- loop over Ping/Pong frames in Stream::poll_next instead of
  returning Pending with an immediate wake
- fail multiaddr parsing on unexpected components instead of silently
  dropping path segments
- drop the vestigial initialize_transport()

Co-authored-by: Cursor <cursoragent@cursor.com>
@illuzen illuzen merged commit 0846f21 into main Jul 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants