Pull request overview

Adds authentication, observability, and abuse protection to the Asset HTTP service endpoints (DataHubURL), aligning rate limits and logging with peer identity and real client IP handling behind proxies.

Changes:

• Introduces Ed25519 request signing for outgoing peer HTTP calls and verification middleware on the Asset service to classify callers into tiers.
• Adds tier-aware global and heavy-endpoint per-IP rate limiting plus new Prometheus metrics for HTTP request visibility.
• Improves client IP determination by configuring trusted-proxy CIDR handling and updates handlers to use RealIP() for logging/tracing.

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Substantive design weaknesses in the Ed25519 layer plus a docs gap that would mislead operators after merge. The annotation refactor and rate-limit plumbing are sound; the specific issues below need addressing.

Critical (security)

• C1. Signed payload omits query string, body, and host. peer_auth_middleware.go:152 signs only timestamp:METHOD:URL.Path. A captured signature replays across any parameterised endpoint (e.g. ?from=&to= on heavy block routes, body content on POST /subtree/:hash/txs) and across any teranode that knows the pubkey (no host binding). One MITM / log leak / tcpdump inherits the peer's full 120-second window of arbitrary GETs and same-path POSTs fleet-wide. Fix: include RequestURI (path + raw query), Host, and a Digest: SHA-256=... of the body (RFC 9530 style).

• C2. No nonce / no replay cache. peer_auth_middleware.go:131-139 enforces only the ±60s clock-skew bound. A captured signed header set is replayable for the full ~120s span, unbounded count. Fix: bounded LRU of recently-seen (pubkey, signature) pairs with TTL of the freshness window.

• C3. No peer allowlist / pinning / revocation. peerTierCache.refresh (peer_auth_middleware.go:83-102) blanket-classifies every entry from p2pClient.GetPeerRegistry. Any peer that ever connected to libp2p is tierPeer (5× rate). Any peer that has ever forwarded a block becomes tierMiner — full rate-limit exempt. The libp2p registry isn't rate-limited on Put either, so a peer that floods us with valid libp2p handshakes mints itself as authenticated. Fix: add asset_peerAuthAllowlist config; require explicit membership before granting tierPeer / tierMiner.

High (security)

• H1. Authenticated rate-limit buckets keyed by IP (rate_limiter.go:77-95), not by peer-ID. Two peers behind one egress IP share one elevated bucket; auth doesn't elevate the peer, it elevates the IP. Fix: switch to peer-ID keying for authenticated buckets; unverified continues IP-keyed.
• H2. unverifiedLimiters map is unbounded. IPv6 attacker rotates source addresses (/64 has 2^64) → ~1 GB allocation in 5 minutes before cleanup runs. Fix: bound the map (LRU), normalise IPv6 to /64, run cleanup more aggressively above a high-water mark.
• H3. BodyLimit middleware ordered after auth+rate-limit (http.go:181-207). Mitigated today because auth doesn't read the body, but if body verification is added (C1 fix), BodyLimit must move ahead of auth.
• H4. Wall-clock timestamp ±60s. Vulnerable to NTP attacks; cut the window (5-10s typical for well-NTP'd infra) and document the NTP requirement.
• H5. c.Request().RequestURI (path + raw query) logged at INFO (http.go:685). No documented sensitive params today but no redaction either. Fix: log path (route pattern, already computed) and drop uri, or add a query-string redactor.

Medium

• M2 (partial). 030a82b25 added a Warnf for invalid TrustedProxyCIDRs — improvement, but the fail-open downgrade to Echo defaults (loopback + RFC 1918) on all-invalid CIDR config still stands. Fix: if TrustedProxyCIDRs is non-empty but no entries parsed, fail Init() instead of silently downgrading.
• M3. tierMiner is fully exempt from rate limiting (rate_limiter.go:73). Combined with C1-C3 this is the highest-impact bypass. Fix: separate HTTPMinerRateLimit (e.g. defaultRate * 100) instead of hard return next(c).

Go correctness

• util/http_signer.go:19 is a plain package-level var written once on startup and read on every outbound HTTP call. Interface writes are not atomic — type-word and pointer can be observed torn under the race detector. Test suite passes -race only because no test exercises concurrent write+read; real race exists in production. Fix: atomic.Value or mutex.
• rate_limiter.go:83-95 pre-constructs &limiterEntry{rate.NewLimiter(...)} before LoadOrStore for every distinct-IP request, including race-losers. Wasteful under port-scan / catchup bursts. Move the allocation behind a sync.Pool or use a load-then-store pattern.

Docs (merge-blocker)

• docs/topics/services/assetServer.md:837-852 (section 7.2.5.1) explicitly says "Per-IP rate limiting inside the asset service itself is a planned defense-in-depth follow-up but is not currently implemented." This PR implements exactly that — operators reading the docs post-merge will be misinformed. Update the section.
• The 6 new settings (HTTPRateLimit, HTTPHeavyRateLimit, HTTPPeerRateLimit, HTTPRateLimitTTL, EnablePeerAuthentication, PeerAuthFreshnessWindow or whatever the actual names are) need rows in docs/references/settings/services/asset_settings.md using the same table format as propagation_settings.md.

Other

• Stray c.Request().RemoteAddr at services/asset/httpimpl/mining_candidate_legacy_block.go:35 (file added by a separate merged PR, but cheap to fix here for codebase-wide consistency with the conversion sweep this PR is doing).
• peerTier constants use zero-indexed iota. Add a // never renumber after merge comment so a future addition in the middle doesn't shift existing values.
• No metric for "auth attempted and failed". Operators have no visibility into whether legitimate peers are getting bounced (clock drift, key rotation) vs the auth path being unused. Add prometheusAssetHTTPPeerAuthResult{result=ok|expired|bad_sig|unknown_key}.

Recommended fix sequence (each is a focused change): C1 (extend signed payload), C2 (in-memory replay cache), H1 (peer-ID keying for authenticated buckets), H2 (bound + IPv6 normalise + faster cleanup), C3 (allowlist), M3 (cap miner rate), then the docs + Go nits.

freemans13's March 30 approval is at a stale commit — re-approval at HEAD will be needed once these land.

Full per-finding writeups in the team review drafts.

Re-review at 1350186c7. Every Critical/High/Medium from the prior CHANGES_REQUESTED is materially closed in code (v2 signed payload, replay cache, peer-ID keying, IPv6 /64 LRU, ±10s window, atomic.Value signer, fail-closed CIDR, c.RealIP() everywhere). 988 tests pass under -race. Docs match code. Sonar refactor is behaviour-preserving.

Two new P1s found during the re-review.

P1-1 — Settings loader doesn't read asset_httpMinerRateLimit or asset_peerAuthAllowlist

settings/asset_settings.go:32,36 declares the fields with key: struct-tag defaults, but settings/settings.go:198-220 doesn't read either key. The loader is hand-rolled getInt/getString, not reflection over the key: tag — grep -r asset_httpMinerRateLimit --include='*.go' matches only the struct tag and longdesc text. Both fields stay at Go zero value forever.

Effects:

• HTTPMinerRateLimit == 0 → miner tier always fully exempt. M3 belt unfastenable.
• PeerAuthAllowlist == "" → parsePeerAuthAllowlist("") returns empty map → every authenticated peer drops to tierUnverified. C3 mechanism cannot be turned on by any operator.

Defaults happen to match the safe-by-default posture (deny-all elevation, miner-exempt), so this isn't a regression of merge-time security stance. But the configurable mitigations the PR claims to ship don't work — same failure mode as #933. Two-line fix in the loader block:

HTTPMinerRateLimit: getInt("asset_httpMinerRateLimit", 0, alternativeContext...),
PeerAuthAllowlist:  getString("asset_peerAuthAllowlist", "", alternativeContext...),

Plus a regression test that asserts the runtime default via settings.NewSettings() (current tests bypass the broken seam).

P1-2 — customHTTPErrorHandler still logs full RequestURI

H5 fixed accessLogMiddleware at http.go:725 but customHTTPErrorHandler at http.go:672 still logs c.Request().RequestURI. The 97fb42f commit message says "drop RequestURI to keep query-string values out of logs"; error paths are exactly where query-string leakage matters most. One-line fix: c.Path().

Worth fixing in the same PR (not blockers)

• Sonar refactor 1350186c7: claims "no behaviour change" but the tier-miner peer_id-missing fallback rate flipped defaultRate → minerRate at rate_limiter.go. Unreachable in normal flow AND inert with current minerRate==0, but worth amending the commit message or reverting the fallback.
• authBucket fallback at rate_limiter.go:134 calls unverifiedBucket(c.RealIP(), ...) without IPv6 /64 normalisation, inconsistent with the H2 fix on the default tier path at :124. Unreachable today but worth tightening.
• No test for TrustedProxyCIDRs bad-CIDR → New() error path; the fail-loudly intent is only manual-tested.
• No test for StartCleanup / cleanupSyncMap goroutine-leak intent — exactly the kind of thing that gets silently regressed.
• rate_limiter.go:96 has a dead defaultRate <= 0 branch — delete or comment.

Confirmations from the re-review

• C1: signer and verifier build "v2:" + ts + ":" + host + ":" + method + ":" + request_uri + ":" + body_sha256 bit-identically. No canonicalisation on either side (sharper, not looser). Body digest verified against actual buffered body. v1 rejection is structural (verifier only accepts "v2:").
• C2: key sha256(pubkey | sig) — bypass-via-body-variation impossible because body digest is in the signed payload. TTL=15s > freshness=10s (5s clock-skew headroom). Cache Set only after verify succeeds — invalid-sig flood doesn't pollute.
• C3 mechanism (gated by P1-1): empty allowlist truly denies — TestPeerAuthMiddleware_AllowlistEmpty_NoElevation confirms. Hash-based map lookup, no timing leak.
• H4: symmetric math.Abs(now-ts) <= 10. Wall-clock comparison correct (monotonic incomparable to signer ts). NTP requirement documented in code.
• atomic.Value signer: nil-guard on Store, typed coercion on Load, single Load-then-use. TestSetHTTPRequestSigner_ConcurrentRace clean under -race.
• M2 fail-closed: returns ConfigurationError from New() for all-invalid and mixed invalid CIDR config.
• Sonar verifySignedRequest / limiterFor: early-return ordering identical to inlined version, replay-before-sig-verify preserved, no closure captures. Cognitive complexity 40→<15 and 18→<15.

Approve at 1131a3550. All P1+P2 from v3 closed. 600/600 tests -race clean.

P1-1 settings loader (88c07d8fc): settings.go:203, 205 wires getInt("asset_httpMinerRateLimit", ...) + getString("asset_peerAuthAllowlist", ...). New asset_settings_test.go is table-driven, sets via gocore.Config().Set and asserts NewSettings() reads through. C3 mechanism + M3 belt now operator-fastenable.

P1-2 RequestURI leak (622f202c2): http.go:672 switched to c.Path(). Test pings ?q=SECRET_QUERY_STRING&token=DEADBEEF and asserts log contains neither value, no ? at all, and route pattern. Defensive ?-absence check catches partial-strip refactors.

P2 rate-limit fallback (1131a3550) — went further than asked. Rewrote limiterFor to remove authBucket helper; all non-elevated flows (including tierMiner/tierPeer with missing peer_id) now land in unverifiedBucket(unverifiedKey(c.RealIP()), defaultRate) — defaultRate + /64 + LRU all in one path. Two new tests pin both invariants. Also closes the v3 Sonar-refactor drift in code, not just by unreachability.

M2 fail-closed test added: TestNew_TrustedProxyCIDRsFailsClosed covers all-invalid, mixed, all-valid, empty.

Earlier C1/C2/C3/H1-H5/M1/M2/M3 commits rebased onto fresh main — content identical, spot-checked. Nothing new introduced.