server: harden metric query proxy - #11077
Conversation
Require mutually authenticated TLS for metric-storage origin changes while preserving no-op and same-origin updates. Validate target URLs, prevent redirect following, and strip sensitive forwarding headers from metric proxy requests. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMetric querying now validates and resolves storage targets, blocks unsafe destinations, forwards supported Prometheus requests through controlled transports, bounds and validates responses, and returns sanitized 502 errors for failures. ChangesMetric storage integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MetricAPI
participant TargetResolver
participant HTTPTransport
participant Prometheus
Client->>MetricAPI: Send metric query
MetricAPI->>TargetResolver: Resolve and validate storage target
TargetResolver-->>MetricAPI: Return validated target
MetricAPI->>HTTPTransport: Forward filtered request
HTTPTransport->>Prometheus: Dial validated address and send query
Prometheus-->>HTTPTransport: Return bounded response
HTTPTransport-->>MetricAPI: Return validated response
MetricAPI-->>Client: Return normalized response or sanitized 502
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
server/api/config.go (1)
131-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegenerate the Swagger spec.
This annotation changes the documented API surface. Run
make swagger-specwithSWAGGER=1and commit the result.As per coding guidelines: "Regenerate Swagger spec with
make swagger-spec(SWAGGER=1) when API annotations change".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/config.go` at line 131, Regenerate the Swagger specification after the annotation change in the API documentation near the 403 failure response by running make swagger-spec with SWAGGER=1, then include the generated specification changes in the commit.Source: Coding guidelines
server/api/metric.go (3)
205-228: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrecompute the blocked header set.
The function recomputes
http.CanonicalHeaderKeyfor eight constants on every header of every request. A package-level set removes that work and reads more directly than a single-caseswitch.♻️ Proposed refactor
+var metricProxySensitiveHeaders = map[string]struct{}{ + "Authorization": {}, + "Component": {}, + "Cookie": {}, + "Forwarded": {}, + "Proxy-Authorization": {}, + http.CanonicalHeaderKey(apiutil.PDRedirectorHeader): {}, + http.CanonicalHeaderKey(apiutil.PDAllowFollowerHandleHeader): {}, + http.CanonicalHeaderKey(apiutil.XCallerIDHeader): {}, + http.CanonicalHeaderKey(apiutil.XForbiddenForwardToMicroserviceHeader): {}, + http.CanonicalHeaderKey(apiutil.XForwardedToMicroserviceHeader): {}, + http.CanonicalHeaderKey(apiutil.XPDHandleHeader): {}, + http.CanonicalHeaderKey(apiutil.XRealIPHeader): {}, +} + func removeMetricProxySensitiveHeaders(header http.Header) { for key := range header { canonicalKey := http.CanonicalHeaderKey(key) - switch { - case canonicalKey == "Authorization", - ... - strings.HasPrefix(canonicalKey, "X-Forwarded-"): + _, blocked := metricProxySensitiveHeaders[canonicalKey] + if blocked || strings.HasPrefix(canonicalKey, "X-Forwarded-") { header.Del(key) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric.go` around lines 205 - 228, Update removeMetricProxySensitiveHeaders to use a package-level set of canonical blocked header names, precomputing the constant header keys once instead of recalculating them for every header. Preserve the existing dynamic X-Forwarded- prefix filtering and nil XForwardedForHeader assignment.
167-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe returned URL keeps the un-normalized host.
parseMetricStorageURLnormalizes the hostname into theoriginstring, but it does not updateu.Host.newMetricReverseProxythen dialstarget.Host, which can still contain uppercase letters, a trailing dot, and no explicit port. The validated origin and the dialed host therefore differ.Note that writing
net.JoinHostPort(hostname, port)back intou.Hostalso changes the upstreamHostheader, because the proxy setsreq.Host = origin.Host. An explicit:80or:443can break virtual-host routing on the Prometheus side. If you keep the current behavior, add a short comment that states thatu.Hoststays as configured on purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric.go` around lines 167 - 172, The normalized hostname and dialed target currently diverge in parseMetricStorageURL; either synchronize u.Host with the normalized host while preserving the intended upstream Host-header behavior, or retain the existing configured u.Host deliberately and add a concise comment documenting that choice. Ensure newMetricReverseProxy continues using the validated origin consistently without introducing unwanted explicit :80 or :443 routing changes.
65-71: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider failing fast when the HTTP client is unavailable.
If
GetHTTPClient()returns nil,transportstays nil andhttputil.ReverseProxyuseshttp.DefaultTransport. For an HTTPS metric storage that requires the PD CA or a client certificate, the request then fails at TLS handshake and the caller only sees a generic 502. Returning 500 with a clear message, or logging the fallback, makes the cause visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric.go` around lines 65 - 71, Update the HTTP client handling in the metric reverse-proxy flow around GetHTTPClient so a nil client is handled explicitly instead of silently passing a nil transport to newMetricReverseProxy; fail the request with a clear 500 response (or log the fallback as established by the handler’s conventions) before calling ServeHTTP, while preserving the configured transport path when the client is available.server/api/metric_test.go (2)
122-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNeither test covers clearing a configured
metric-storagevalue. Both clearing cases pass for reasons unrelated to the authorization rule: the unit test still hasrequest.TLSattached, and the integration test starts from an empty value, sovalidateMetricStorageConfigUpdatereturns early onupdated == current. The transition from a non-empty target to an empty target without mutually authenticated TLS is untested.
server/api/metric_test.go#L122-L137: use a second request withoutTLSset, pass a non-emptycurrent, and assert the expected status code for clearing the value.tests/server/config/config_test.go#L178-L182: configure a non-emptymetric-storagevalue first, for example through the server configuration, then assert the result of posting an empty value over the non-TLS client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric_test.go` around lines 122 - 137, Add coverage for clearing a non-empty metric-storage value without mutually authenticated TLS: in server/api/metric_test.go lines 122-137, create a separate request with TLS unset, use a non-empty current value, and assert the expected status; in tests/server/config/config_test.go lines 178-182, configure metric-storage with a non-empty value first, then post an empty value through the non-TLS client and assert the expected result.
62-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the returned target, not only the origin.
The test checks
originand only thattargetis not nil.newMetricReverseProxyusestarget.Scheme,target.Host, and ignorestarget.Path. Add expectedtarget.Hostandtarget.Pathvalues to the table. That assertion documents how a configured base path is handled and pins the behavior discussed in thenewMetricReverseProxycomment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric_test.go` around lines 62 - 74, Extend the metric URL test cases used by parseMetricStorageURL to include expected target.Host and target.Path values, then replace the target non-nil check with assertions against those fields. Preserve the existing origin assertions and ensure the cases cover configured base-path handling consistent with newMetricReverseProxy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/metric.go`:
- Around line 186-197: Update newMetricReverseProxy so its Director preserves
the target URL’s configured base path and query when rewriting requests, rather
than replacing the path with metricPath alone. Keep the existing host assignment
and sensitive-header removal, and ensure metricPath is combined with the target
base path without discarding the original query.
---
Nitpick comments:
In `@server/api/config.go`:
- Line 131: Regenerate the Swagger specification after the annotation change in
the API documentation near the 403 failure response by running make swagger-spec
with SWAGGER=1, then include the generated specification changes in the commit.
In `@server/api/metric_test.go`:
- Around line 122-137: Add coverage for clearing a non-empty metric-storage
value without mutually authenticated TLS: in server/api/metric_test.go lines
122-137, create a separate request with TLS unset, use a non-empty current
value, and assert the expected status; in tests/server/config/config_test.go
lines 178-182, configure metric-storage with a non-empty value first, then post
an empty value through the non-TLS client and assert the expected result.
- Around line 62-74: Extend the metric URL test cases used by
parseMetricStorageURL to include expected target.Host and target.Path values,
then replace the target non-nil check with assertions against those fields.
Preserve the existing origin assertions and ensure the cases cover configured
base-path handling consistent with newMetricReverseProxy.
In `@server/api/metric.go`:
- Around line 205-228: Update removeMetricProxySensitiveHeaders to use a
package-level set of canonical blocked header names, precomputing the constant
header keys once instead of recalculating them for every header. Preserve the
existing dynamic X-Forwarded- prefix filtering and nil XForwardedForHeader
assignment.
- Around line 167-172: The normalized hostname and dialed target currently
diverge in parseMetricStorageURL; either synchronize u.Host with the normalized
host while preserving the intended upstream Host-header behavior, or retain the
existing configured u.Host deliberately and add a concise comment documenting
that choice. Ensure newMetricReverseProxy continues using the validated origin
consistently without introducing unwanted explicit :80 or :443 routing changes.
- Around line 65-71: Update the HTTP client handling in the metric reverse-proxy
flow around GetHTTPClient so a nil client is handled explicitly instead of
silently passing a nil transport to newMetricReverseProxy; fail the request with
a clear 500 response (or log the fallback as established by the handler’s
conventions) before calling ServeHTTP, while preserving the configured transport
path when the client is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac4e2e75-7943-4120-a2cf-9a3147967165
📒 Files selected for processing (4)
server/api/config.goserver/api/metric.goserver/api/metric_test.gotests/server/config/config_test.go
| func newMetricReverseProxy(target *url.URL, metricPath string, transport http.RoundTripper) *httputil.ReverseProxy { | ||
| origin := &url.URL{Scheme: target.Scheme, Host: target.Host} | ||
| proxy := httputil.NewSingleHostReverseProxy(origin) | ||
| proxy.Transport = transport | ||
| director := proxy.Director | ||
| proxy.Director = func(req *http.Request) { | ||
| director(req) | ||
| req.URL.Path = metricPath | ||
| req.URL.RawPath = "" | ||
| req.Host = origin.Host | ||
| removeMetricProxySensitiveHeaders(req.Header) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the previous metric proxy behavior with the new path handling.
git log --oneline -3 -- server/api/metric.go
git show HEAD~1:server/api/metric.go 2>/dev/null | sed -n '1,120p'
# Look for documented metric-storage examples that include a base path.
rg -n 'metric-storage' --type=go --type=toml --type=md -g '!**/*_test.go' | head -40Repository: tikv/pd
Length of output: 2111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server/api/metric.go outline =="
ast-grep outline server/api/metric.go || true
echo "== server/api/metric.go relevant sections =="
wc -l server/api/metric.go
sed -n '150,260p' server/api/metric.go
echo "== parseMetricStorageURL definitions/usages and tests =="
rg -n "parseMetricStorageURL|MetricStorage|metric-storage|Prometheus\\.Example|tenant=1|newMetricReverseProxy|NewReverseProxy" --type=go --type=toml --type=md | head -120
echo "== config/default values around MetricStorage =="
rg -n "MetricStorage" server/config pkg config --type=go --type=toml | head -120Repository: tikv/pd
Length of output: 3943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files containing MetricStorage/validateMetricStorageConfigUpdate/parseMetricStorageURL =="
rg -n "MetricStorage|validateMetricStorageConfigUpdate|parseMetricStorageURL|metric-storage|prometheusMetricPath|newMetricReverseProxy" server pkg --type=go -g '!vendor/**' | head -200
echo "== server/api/metric.go lines 1-110 =="
sed -n '1,110p' server/api/metric.go
echo "== config files mentioning metric-storage or MetricStorage =="
rg -n "metric-storage|MetricStorage" --type=toml . | head -120Repository: tikv/pd
Length of output: 8408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parseMetricStorageURL implementation =="
sed -n '129,174p' server/api/metric.go
echo "== parseMetricStorageURL tests =="
sed -n '32,108p' server/api/metric_test.go
echo "== newMetricReverseProxy tests =="
sed -n '136,259p' server/api/metric_test.go
echo "== Go module version/source availability =="
rg -n "module github.com/tikv/pd|go " go.mod go.mod.sum 2>/dev/null | head -20Repository: tikv/pd
Length of output: 9114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go ReverseProxy XFF source snippets =="
go env GOROOT 2>/dev/null || true
if [ -n "$(go env GOROOT 2>/dev/null)" ]; then
grep -n -A12 -B4 'X-Forwarded-For' "$(go env GOROOT)"/src/net/http/httputil/reverseproxy.go || true
grep -n -A8 -B4 'func NewSingleHostReverseProxy' "$(go env GOROOT)"/src/net/http/httputil/reverseproxy.go || true
else
echo "GOROOT unavailable"
fi
echo "== URL.Path join behavior probe =="
python3 - <<'PY'
from urllib.parse import urljoin
targets = [
("https://prometheus.example:443", "/prometheus", "/api/v1/query_range"),
("https://prometheus.example:443", "/prometheus/", "/api/v1/query_range"),
("http://127.0.0.1:9090", "", "/api/v1/query_range"),
]
for target, path, metricPath in targets:
print({"target": target, "base": path, "metric": metricPath, "result": urljoin(target.rstrip("/") + "/", metricPath)})
PYRepository: tikv/pd
Length of output: 6326
Preserve the configured base path and query in the metric reverse proxy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/api/metric.go` around lines 186 - 197, Update newMetricReverseProxy so
its Director preserves the target URL’s configured base path and query when
rewriting requests, rather than replacing the path with metricPath alone. Keep
the existing host assignment and sensitive-header removal, and ensure metricPath
is combined with the target base path without discarding the original query.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11077 +/- ##
==========================================
+ Coverage 79.23% 79.35% +0.11%
==========================================
Files 540 541 +1
Lines 76010 76701 +691
==========================================
+ Hits 60226 60865 +639
- Misses 11533 11561 +28
- Partials 4251 4275 +24
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api/metric.go (1)
300-346: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider caching the metric query client per target.
The transport is cloned per request and its idle connections are closed when the request ends. Every metric query then performs a new TCP handshake, and for HTTPS a new TLS handshake. Dashboard panels issue these queries repeatedly, so the added latency and CPU cost accumulate.
A cache keyed by scheme, hostname, port, and the validated address set keeps the security properties. The dial guard and the address pinning stay unchanged. Re-resolve on a short TTL so that target changes still take effect.
The security logic itself is correct: the proxy hooks are cleared, the dial address is pinned, and redirects are not followed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric.go` around lines 300 - 346, The metric query client is recreated and its connections closed for every request, causing repeated TCP/TLS handshakes. Add a short-TTL cache around the client-building flow, keyed by scheme, hostname, port, and the validated address set, while preserving the existing DialContext validation, address pinning, proxy clearing, redirect behavior, and re-resolution when the cache expires.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/metric.go`:
- Around line 116-118: Sanitize client headers before the metric proxy request
is built: remove Authorization, Cookie, Proxy-Authorization, and hop-by-hop
headers named by Connection before assigning the cloned headers in the metric
request flow. Update TestMetricQueryReturnsNormalizedPrometheusResponse if it
currently relies on forwarding Authorization, or use metric-storage credentials
from PD configuration instead.
---
Nitpick comments:
In `@server/api/metric.go`:
- Around line 300-346: The metric query client is recreated and its connections
closed for every request, causing repeated TCP/TLS handshakes. Add a short-TTL
cache around the client-building flow, keyed by scheme, hostname, port, and the
validated address set, while preserving the existing DialContext validation,
address pinning, proxy clearing, redirect behavior, and re-resolution when the
cache expires.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80ea9565-2355-4588-8bd9-646d17a3ddbc
📒 Files selected for processing (2)
server/api/metric.goserver/api/metric_test.go
| request.Header = r.Header.Clone() | ||
| request.Header.Del("Accept-Encoding") | ||
| request.Header.Set("Accept", "application/json") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every place that mentions metric storage header handling or config validation.
rg -n --type=go -C4 'validateMetricStorageConfigUpdate|MetricStorage' server pkg -g '!**/vendor/**'
# Check for existing header sanitization helpers in the repo.
rg -n --type=go -C3 'Proxy-Authorization|hop-by-hop|Header\.Del\("Authorization"\)' server pkg -g '!**/vendor/**'Repository: tikv/pd
Length of output: 8030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server/api/metric.go outline =="
ast-grep outline server/api/metric.go || true
echo
echo "== server/api/metric.go relevant lines =="
cat -n server/api/metric.go | sed -n '1,140p'
echo
echo "== server/api/metric_test.go relevant tests =="
cat -n server/api/metric_test.go | sed -n '1,260p'
echo
echo "== search metric proxy terms =="
rg -n --type=go -C3 'proxyMetricQuery|sanitize|sanitizeHeader|AllowedHeaders|Strip|Authorization|Cookie|Proxy-Authorization|TE|Transfer-Encoding|Connection|Upgrade' server/api/metric.go server/api/metric_test.go -g '!**/vendor/**' || trueRepository: tikv/pd
Length of output: 26387
🌐 Web query:
RFC9112 HTTP/1.1 hop-by-hop header fields Connection Keep-Alive Transfer-Encoding TE Trailer Upgrade proxy must not forward
💡 Result:
In the HTTP/1.1 specification (RFC 9112) and the broader HTTP Semantics specification (RFC 9110), hop-by-hop header fields are defined as those that are only intended for the immediate connection between the sender and the recipient [1][2]. These headers must not be forwarded by proxies to the next hop [1][2][3]. The mechanism for managing hop-by-hop headers is primarily governed by the Connection header field [1][2]. Any header field name listed in the Connection header field is considered hop-by-hop and must be removed by the proxy before forwarding the message to the next upstream or downstream recipient [1][2]. In addition to any header explicitly listed in the Connection header, the following standard header fields are defined as hop-by-hop and must not be forwarded by a proxy [1][3]: - Connection [1][2] - Keep-Alive [3] - Proxy-Authenticate [3] - Proxy-Authorization [3] - Proxy-Connection [3] - TE [4][5][3] - Trailer [3] - Transfer-Encoding [4][5][3] - Upgrade [3] Proxies are required to remove these headers because they only apply to the immediate transport connection [1]. For example, the TE header is used to indicate transfer codings for the immediate connection, and if a proxy were to forward it, an intermediary that does not support its semantics might incorrectly interpret or mishandle the request [4][5]. Similarly, the Connection header itself must not be forwarded; instead, a proxy should generate its own Connection header if necessary for the next hop [1][2].
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc9110
- 2: https://www.rfc-editor.org/info/rfc9110/
- 3: dapr/dapr@5924cc9
- 4: https://www.rfc-editor.org/info/rfc9112/
- 5: https://greenbytes.de/tech/specs/rfc9112.html
🌐 Web query:
Go net/http Header Clone copies all headers copy deep copy source Clone
💡 Result:
In Go, the net/http Header.Clone method performs a deep copy of the header map and its associated slice values [1][2]. Key details of its implementation and behavior include: Deep Copy: Because http.Header is defined as map[string][]string, a shallow copy would only copy the map structure, leaving the underlying []string slices pointing to the same backing arrays as the source [2]. The Clone method ensures the integrity of the data by creating a deep copy, meaning changes made to the values in the cloned header do not affect the original header [2]. Efficient Allocation: To optimize performance, the implementation does not allocate a new slice for every single header entry [2][3]. Instead, it calculates the total number of values across all keys, performs a single allocation for a large backing array that can hold all values, and then slices this array to assign the appropriate segments to each header key [1][2]. Nil Handling: - If the receiver (the source header) is nil, Clone returns nil [1][4]. - If a specific header key has a nil value (distinguished from an empty or zero-length slice), Clone preserves this nil value in the new header [1][5]. This method was introduced in Go 1.13 to provide a standard, performant, and safe way to copy headers [6][7].
Citations:
- 1: https://github.com/golang/go/blob/d468ad36/src/net/http/header.go
- 2: https://stackoverflow.com/questions/59807138/why-not-just-assign-value-to-target-in-method-header-clone
- 3: net/http: reduce allocations in (Header).clone golang/go#29915
- 4: https://go.dev/src/net/http/header.go
- 5: golang/go@ed2f33e
- 6: net/http: Header.Clone behavior golang/go#33141
- 7: https://golang.org/pkg/net/http?tab=versions
Sanitize metric proxy headers before cloning.
r.Header.Clone() copies every client header into the forwarded metric storage request. No proxy layer currently strips authorization, cookie, or hop-by-hop headers before line 116, so Authorization, Cookie, Proxy-Authorization, and any hop-by-hop headers listed in Connection may reach Prometheus. Remove the intended forwarding headers before cloning, or take metric-storage credentials from PD config instead. Update TestMetricQueryReturnsNormalizedPrometheusResponse if forwarding Authorization is not the intended contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/api/metric.go` around lines 116 - 118, Sanitize client headers before
the metric proxy request is built: remove Authorization, Cookie,
Proxy-Authorization, and hop-by-hop headers named by Connection before assigning
the cloned headers in the metric request flow. Update
TestMetricQueryReturnsNormalizedPrometheusResponse if it currently relies on
forwarding Authorization, or use metric-storage credentials from PD
configuration instead.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api/metric_test.go (1)
81-102: 🩺 Stability & Availability | 🔵 TrivialVerify the
newMetricQueryTransportsignature and add fallback-across-addresses coverage.
newMetricQueryTransportis called with&http.Client{Transport: baseTransport}at Line 91, and the return value is later used with.DialContextand.CloseIdleConnectionsdirectly (Lines 93, 97). This implies the function accepts an*http.Clientbut returns something shaped like an*http.Transport. Confirm this matches the actual signature inserver/api/metric.go, since accepting a client only to derive a transport is an unusual API shape.Separately, this test only verifies dialing pins to a single validated address. The PR objectives require that hosts with multiple safe addresses support fallback when one address is unavailable. Add a test that resolves multiple safe addresses, fails the first dial, and asserts the proxy retries the next address.
#!/bin/bash # Description: Inspect the newMetricQueryTransport definition and its return type/usage. ast-grep run --pattern 'func newMetricQueryTransport($$$) $$$' --lang go server/api/metric.go rg -n -B2 -A15 'func newMetricQueryTransport' server/api/metric.go🧪 Suggested addition for fallback coverage (illustrative, adapt to actual transport API)
func TestMetricQueryFallsBackAcrossAddresses(t *testing.T) { // resolve a target with two safe addresses, fail dial to the first, // and assert the transport retries the second address. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/metric_test.go` around lines 81 - 102, Confirm the newMetricQueryTransport signature and return type in metric.go match the test’s direct DialContext and CloseIdleConnections usage, adjusting the test or implementation as needed. Add TestMetricQueryFallsBackAcrossAddresses covering a resolved target with two safe addresses, forcing the first dial to fail and asserting the transport retries and connects to the second address.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/metric_test.go`:
- Around line 104-156: Extend TestMetricQueryResponseNormalization by capturing
the outgoing request’s X-Remove header inside the roundTripFunc closure,
alongside authorization and requestPath, then assert it is empty after
proxyMetricQuery runs. Keep the existing setup and assertions unchanged.
---
Nitpick comments:
In `@server/api/metric_test.go`:
- Around line 81-102: Confirm the newMetricQueryTransport signature and return
type in metric.go match the test’s direct DialContext and CloseIdleConnections
usage, adjusting the test or implementation as needed. Add
TestMetricQueryFallsBackAcrossAddresses covering a resolved target with two safe
addresses, forcing the first dial to fail and asserting the transport retries
and connects to the second address.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f276f15-35d0-4679-bcb9-58cecc786760
📒 Files selected for processing (3)
server/api/metric.goserver/api/metric_test.goserver/api/router.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/router.go
- server/api/metric.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
|
|
||
| func newqueryMetric(s *server.Server) *queryMetric { | ||
| return &queryMetric{s: s} | ||
| transport, err := newMetricQueryTransport(s.GetHTTPClient()) |
There was a problem hiding this comment.
This transport is initialized before the server HTTP client exists. api.NewHandler is invoked by combineBuilderServerHTTPService during server.CreateServer (server/server.go:303), while s.httpClient is assigned only later by startClient (server/server.go:446) after Run starts. Consequently, s.GetHTTPClient() is nil here during normal startup and newMetricQueryTransport permanently falls back to http.DefaultTransport; the configured Root CAs and client certificate never reach metric queries. HTTPS or mTLS metric storage will therefore fail, contrary to the TLS compatibility requirement in #11081.
Could we initialize this transport after startClient, or lazily without caching the pre-start nil state? A regression test can build the API through server.CreateServer, start PD with a custom CA/client certificate, and query an HTTPS metric endpoint that requires that client configuration. The request should succeed only when the handler uses the post-startClient transport.
| request := r.Clone(ctx) | ||
| request.RequestURI = "" | ||
| request.URL.Scheme = target.url.Scheme | ||
| request.URL.Host = target.url.Host |
There was a problem hiding this comment.
r.Clone preserves the inbound Request.Host, and changing request.URL.Host does not change that override. Go uses Request.Host as the outbound HTTP/1 Host header and HTTP/2 :authority, so a caller can select a different virtual host behind the validated IP. This means the request is not fully pinned to the configured metric-storage origin.
A minimal regression test fails with the current code:
var gotHost string
transport := roundTripFunc(func(req *http.Request) (*http.Response, error) {
gotHost = req.Host
return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(successMetricResponse)),
}, nil
})
request := httptest.NewRequest(http.MethodGet,
"http://pd.example/pd/api/v1/metric/query", nil)
request.Host = "admin.internal"
recorder := httptest.NewRecorder()
proxyMetricQuery(recorder, request, "http://metrics.example:9090",
transport, safeMetricResolver())
require.Equal(t, "metrics.example:9090", gotHost)The observed value is currently admin.internal. Could we clear request.Host so URL.Host is used, or explicitly set it to target.url.Host?
|
|
||
| func isSafeMetricTargetIP(address netip.Addr) bool { | ||
| address = address.Unmap() | ||
| unsafe := !address.IsValid() || address.Zone() != "" || address.IsLoopback() || |
There was a problem hiding this comment.
Could we reconcile this intentional loopback rejection with the existing supported configuration? The added TestResolveMetricStorageTarget already confirms the behavior:
resolver := staticMetricResolver{addresses: []netip.Addr{
netip.MustParseAddr("127.0.0.1"),
}}
_, err := resolveMetricStorageTarget(context.Background(),
"http://metrics.example", resolver)
require.Error(t, err)However, conf/config.toml:107 recommends http://127.0.0.1:9090, and TiDB Dashboard also publishes that address in its metric-storage setup example. PDServerConfig.Validate still accepts it, so an existing or newly updated configuration succeeds but every metric query becomes a generic 502.
If rejecting loopback is required by the SSRF policy, could we update the example, release note, and preferably reject the configuration update rather than failing later at query time? Otherwise, this breaks the documented local Prometheus deployment.
2 participants
What problem does this PR solve?
Issue Number: Fixes #11081
The metric query API can send requests to a runtime-configurable metric-storage target. Without validating the destination and normalizing the response, it can be abused for SSRF and expose upstream details.
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Release note
Summary by CodeRabbit