feat: add asset service rate limiting, peer authentication and request visibility#643
Open
feat: add asset service rate limiting, peer authentication and request visibility#643
Conversation
…t visibility (#4521) Add three-tier per-IP rate limiting, Ed25519 peer request signing, always-on access logging with Prometheus metrics, and proper real IP extraction for reverse proxy deployments on the asset service HTTP endpoints. Rate limiting tiers: - Mining peers (high reputation + blocks received): fully exempt - Non-mining peers (valid signature, in peer registry): multiplied rate (default 5x) - Unverified clients: base rate (configurable, default 1024 req/s global, 10 req/s heavy) Data-heavy endpoints (subtree_data, blocks, block, subtree, batch txs, legacy block) get a stricter per-route rate limit to protect against UTXO store abuse from on-demand subtreeData creation. Peer authentication: - Outgoing HTTP requests are automatically signed with the node's Ed25519 P2P key - Asset service verifies signatures, derives peer ID, checks cached peer registry - Peer tier cache refreshes every 30s, fails open on P2P service unavailability New settings: - asset_httpRateLimit (default 1024): global per-IP req/s - asset_httpHeavyRateLimit (default 10): per-IP req/s on heavy endpoints - asset_httpPeerRateMultiplier (default 5): rate multiplier for non-mining peers - asset_httpBodyLimit (default 100MB): max request body size - asset_trustedProxyCIDRs: pipe-separated CIDRs for X-Forwarded-For trust - asset_peerMinerReputationThreshold (default 50.0): min reputation for miner tier Additional changes: - Replace debug-only logger with always-on access log middleware - Add Prometheus metrics: request duration, response size, rate limited count, in-flight - Fix all handlers to use c.RealIP() instead of c.Request().RemoteAddr - Configure Echo IPExtractor for correct IP extraction behind reverse proxies
Contributor
|
🤖 Claude Code Review Status: Complete This is a well-implemented security and observability feature for the Asset service. The PR adds three-tier rate limiting, Ed25519 peer authentication, and comprehensive access logging with Prometheus metrics. Summary: No critical issues found. The implementation is solid with good test coverage and proper error handling. Minor observations:
Architecture notes:
The implementation follows project conventions and integrates cleanly with existing code. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-03-31 17:00 UTC |
Contributor
There was a problem hiding this comment.
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.
Show a summary per file
| File | Description |
|---|---|
| util/http_signer.go | Adds Ed25519 request signer + package-level signer hook for outgoing HTTP requests |
| util/http_signer_test.go | Tests for request signing and signer wiring |
| util/http.go | Signs outgoing requests when a signer is configured |
| settings/settings.go | Wires new Asset HTTP security/rate-limit settings into runtime settings |
| settings/asset_settings.go | Documents new Asset HTTP settings (rate limits, body limit, trusted proxies, miner threshold) |
| services/p2p/Server.go | Sets the global HTTP request signer using the node’s P2P Ed25519 key |
| services/asset/httpimpl/peer_auth_middleware.go | Adds middleware to verify signed requests and assign peer_tier using a cached peer registry |
| services/asset/httpimpl/peer_auth_middleware_test.go | Tests peer auth behavior (valid/invalid signatures, expiry, unknown peers) |
| services/asset/httpimpl/rate_limiter.go | Adds tiered per-IP rate limiting middleware with cleanup of stale limiter entries |
| services/asset/httpimpl/rate_limiter_test.go | Tests tiered rate limiting behavior (unverified, peer multiplier, miner exemption, disabled) |
| services/asset/httpimpl/metrics.go | Adds Prometheus metrics for HTTP duration, response size, rate-limited count, and in-flight gauge |
| services/asset/httpimpl/http.go | Wires middleware stack (trusted proxy IP extraction, peer auth, access logs, tiered rate limits, body limit, heavy RL) and applies heavy RL to selected routes |
| services/asset/httpimpl/http_test.go | Updates middleware test to reflect new access log middleware name |
| services/asset/httpimpl/* handlers | Switches tracing/log messages from RemoteAddr to RealIP() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
freemans13
approved these changes
Mar 30, 2026
- Fix goroutine leak: rate limiter cleanup goroutines now accept a context and stop on cancellation, matching the peerTierCache pattern. Created via Start() in the HTTP server's Start method. - Fix double error handling: remove c.Error(err) call in accessLogMiddleware to prevent Echo invoking HTTPErrorHandler twice. - Fix panic on negative settings: clamp defaultRate <= 0 as disabled and peerMultiplier to minimum 1. - Fix misleading metric label: rename "tier" to "scope" on the rate limited counter since values are "global"/"heavy" (limiter scope, not peer tier). - Fix bucket comment: ExponentialBuckets(256, 4, 10) maxes at ~64MB not ~256MB.
Contributor
There was a problem hiding this comment.
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.
Reformat struct field alignment to satisfy golangci-lint's embedded gci formatter. Pre-existing formatting issue surfaced because the file was modified in this branch.
- Fix accessLogMiddleware: call c.Error(err) and return nil so status/size are finalized by the error handler before metrics observe them - Fix stale comment: "counts rate-limited requests by tier" → "by scope" - Fix pre-existing copy-paste log message in GetTxMetaByTxID (was logging "GetUTXOsByTXID") - Add warning when asset_trustedProxyCIDRs is configured but all entries fail to parse
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+236
to
+239
| // Sign the request if a signer is configured (silently skip on error) | ||
| if httpRequestSigner != nil { | ||
| _ = httpRequestSigner.SignRequest(req) | ||
| } |
There was a problem hiding this comment.
executeHTTPRequest now signs all outgoing HTTP requests when a global signer is configured. Since util.DoHTTPRequest/DoHTTPRequestBodyReader are also used for non-peer HTTP calls (e.g., local health checks and test harness endpoints), this will attach X-Peer-* headers (and a signature) to requests that aren’t peer-to-peer asset-service calls, potentially leaking node identity and creating unexpected coupling. Consider making signing opt-in per request (or only for known peer/DataHub targets), or providing a way for callers to explicitly disable signing for non-peer URLs.
Contributor
Author
There was a problem hiding this comment.
The signer is only set when util.SetHTTPRequestSigner() is called during P2P service startup. Test processes and non-peer callers (test_daemon.go, test_containers.go, helper.go) don't start the P2P service, so the signer is nil and no headers are added. The only production callers are blockvalidation, subtreevalidation, and legacy peer_server — all legitimate peer-to-peer paths. Adding per-request opt-in would require changing 13 call sites for no practical benefit.
|
|
||
| tier := cache.GetTier(peerID) | ||
| c.Set("peer_tier", tier) | ||
| logger.Debugf("[PeerAuth] authenticated peer %s as %s", peerID, tier) |
There was a problem hiding this comment.
The format string uses tier (type peerTier) with %s. Since peerTier is not a string type, this will log as %!s(httpimpl.peerTier=...) rather than the intended peer/miner/unverified. Use tier.String() or %v here (and similarly anywhere else peerTier is formatted as %s).
Suggested change
| logger.Debugf("[PeerAuth] authenticated peer %s as %s", peerID, tier) | |
| logger.Debugf("[PeerAuth] authenticated peer %s as %v", peerID, tier) |
Contributor
Author
There was a problem hiding this comment.
peerTier implements fmt.Stringer (line 27 of this file), so %s works correctly — fmt.Sprintf calls String() on any type implementing the Stringer interface. Verified: fmt.Printf("%s", tierMiner) outputs "miner".
| prometheusAssetHTTPResponseSize.WithLabelValues(method, path, statusStr).Observe(float64(size)) | ||
|
|
||
| return err | ||
| logger.Infof("[Asset_http] %s %s client_ip=%s status=%d duration=%v size=%d tier=%s uri=%s", |
There was a problem hiding this comment.
The access log uses tier (type peerTier) with %s, which will produce %!s(httpimpl.peerTier=...) in logs. Use tier.String() or change the format verb to %v so the intended tier name is logged.
Suggested change
| logger.Infof("[Asset_http] %s %s client_ip=%s status=%d duration=%v size=%d tier=%s uri=%s", | |
| logger.Infof("[Asset_http] %s %s client_ip=%s status=%d duration=%v size=%d tier=%v uri=%s", |
Contributor
Author
There was a problem hiding this comment.
Same as above — peerTier has a String() method so %s works correctly via the fmt.Stringer interface. No change needed.
|
|
||
| if !limiter.Allow() { | ||
| prometheusAssetHTTPRateLimited.WithLabelValues(rl.tierLabel).Inc() | ||
| return c.JSON(http.StatusTooManyRequests, map[string]string{"error": "rate limit exceeded"}) |
There was a problem hiding this comment.
Rate-limited responses return JSON { "error": "rate limit exceeded" }, but the service’s customHTTPErrorHandler (and handler docs) use a { "message": ... } schema. For API consistency, consider returning the same top-level field (message) (or routing through echo.HTTPError/customHTTPErrorHandler) so clients don’t need special-case parsing for 429s.
Suggested change
| return c.JSON(http.StatusTooManyRequests, map[string]string{"error": "rate limit exceeded"}) | |
| return c.JSON(http.StatusTooManyRequests, map[string]string{"message": "rate limit exceeded"}) |
Contributor
Author
There was a problem hiding this comment.
Fixed. Changed to {"message": "rate limit exceeded"} to match the customHTTPErrorHandler schema.
The customHTTPErrorHandler returns {"message": ...} for all error responses.
Match that schema for rate limit 429 responses.
|
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
Addresses #4521 — adds visibility and access control for the asset service (DataHubURL) HTTP endpoints.
Rate Limiting Tiers
BlocksReceived > 0ANDReputationScore >= thresholdbase_rate × multiplier(default 5x)Request Signing Protocol
All outgoing peer HTTP requests (block validation, subtree validation, catchup) are automatically signed:
X-Peer-PubKey(hex Ed25519 public key),X-Peer-Timestamp(unix seconds),X-Peer-Signature(hex signature oftimestamp:METHOD:/path)New Settings
asset_httpRateLimitasset_httpHeavyRateLimitasset_httpPeerRateMultiplierasset_httpBodyLimitasset_trustedProxyCIDRsasset_peerMinerReputationThresholdHeavy-Rate-Limited Endpoints
These endpoints get the stricter
asset_httpHeavyRateLimitbecause they serve large payloads or trigger expensive backend operations:GET /subtree_data/:hash— can trigger on-demand UTXO storeBatchDecorate()lookupsPOST /subtree/:hash/txs— batch transaction fetch (32MB buffer, 1024 goroutines)GET /blocks/:hash— up to 1000 blocks per requestGET /block/:hash— full block dataGET /subtree/:hash— large subtree dataGET /block_legacy/:hash,GET /rest/block/:hash.bin— legacy block formatsPeer Tier Cache
The asset service caches peer IDs and their tiers locally (refreshed every 30 seconds via
GetPeerRegistry()gRPC call). This means:Middleware Stack Order
Files Changed
util/http_signer.go,services/asset/httpimpl/peer_auth_middleware.go,services/asset/httpimpl/rate_limiter.go+ testssettings/asset_settings.go,settings/settings.go,services/asset/httpimpl/http.go,services/asset/httpimpl/metrics.go,services/p2p/Server.go,util/http.go, 23 handler files (RemoteAddr→RealIP())Test plan
go build ./...— compiles cleanlymake lint— 0 issuesgo vet— cleanutil,httpimpl,settings,p2p)tier=unverified/peer/minerand real client IP/metricsendpointX-Forwarded-Forextraction with trusted proxy CIDRs