Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
360 changes: 344 additions & 16 deletions server/api/metric.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,42 +15,370 @@
package api

import (
"fmt"
"context"
"encoding/json"
"io"
"net"
"net/http"
"net/netip"
"net/url"
"strconv"
"strings"
"time"

"go.uber.org/zap"

"github.com/pingcap/errors"
"github.com/pingcap/log"

"github.com/tikv/pd/pkg/utils/apiutil"
"github.com/tikv/pd/server"
)

const (
metricQueryTimeout = 30 * time.Second
metricQueryDialFallbackDelay = 300 * time.Millisecond
maxMetricQueryResponseBodySize = int64(32 << 20)
metricQueryErrorBody = `{"status":"error","errorType":"proxy","error":"metric query failed"}`
)

type metricIPResolver interface {
LookupNetIP(context.Context, string, string) ([]netip.Addr, error)
}

type queryMetric struct {
s *server.Server
s *server.Server
transport *http.Transport
transportErr error
resolver metricIPResolver
}

type resolvedMetricTarget struct {
url *url.URL
port string
addresses []netip.Addr
}

type metricTargetContextKey struct{}

func newqueryMetric(s *server.Server) *queryMetric {
return &queryMetric{s: s}
transport, err := newMetricQueryTransport(s.GetHTTPClient())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

return &queryMetric{
s: s,
transport: transport,
transportErr: err,
resolver: net.DefaultResolver,
}
}

func (h *queryMetric) queryMetric(w http.ResponseWriter, r *http.Request) {
metricAddr := h.s.GetConfig().PDServerCfg.MetricStorage
if metricAddr == "" {
http.Error(w, "metric storage doesn't set", http.StatusInternalServerError)
if h.transportErr != nil {
writeMetricQueryError(w, h.transportErr)
return
}
u, err := url.Parse(metricAddr)
proxyMetricQuery(
w,
r,
h.s.GetConfig().PDServerCfg.MetricStorage,
h.transport,
h.resolver,
)
}

func proxyMetricQuery(
w http.ResponseWriter,
r *http.Request,
metricStorage string,
transport http.RoundTripper,
resolver metricIPResolver,
) {
ctx, cancel := context.WithTimeout(r.Context(), metricQueryTimeout)
defer cancel()

target, err := resolveMetricStorageTarget(ctx, metricStorage, resolver)
if err != nil {
writeMetricQueryError(w, err)
return
}
ctx = context.WithValue(ctx, metricTargetContextKey{}, target)

request := r.Clone(ctx)
request.RequestURI = ""
request.URL.Scheme = target.url.Scheme
request.URL.Host = target.url.Host

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

request.URL.Path = strings.Replace(r.URL.Path, "/pd/api/v1/metric", "/api/v1", 1)
// Preserve end-to-end headers for authenticated metric storage, matching the
// historical proxy contract, but never forward connection-specific headers.
request.Header = r.Header.Clone()
removeHopByHopHeaders(request.Header)
request.Header.Del("Accept-Encoding")
request.Header.Set("Accept", "application/json")
Comment on lines +111 to +114

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 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/**' || true

Repository: 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:


🌐 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:


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.


// RoundTrip does not follow redirects. The target is resolved and validated before dialing.
response, err := transport.RoundTrip(request) //nolint:gosec
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
writeMetricQueryError(w, errors.Annotate(err, "metric storage request failed"))
return
}
defer response.Body.Close()
if response.StatusCode < http.StatusOK || response.StatusCode >= http.StatusMultipleChoices {
writeMetricQueryError(w, errors.Errorf("metric storage returned status %d", response.StatusCode))
return
}

body, err := readMetricQueryResponse(response.Body, maxMetricQueryResponseBodySize)
if err != nil {
writeMetricQueryError(w, err)
return
}
if err := validatePrometheusQueryResponse(body); err != nil {
writeMetricQueryError(w, err)
return
}

w.Header().Set("Content-Type", "application/json; charset=utf-8")
w.Header().Set("Cache-Control", "no-store")
w.WriteHeader(http.StatusOK)
if _, err := w.Write(body); err != nil {
log.Warn("failed to write metric query response", zap.Error(err))
}
}

func resolveMetricStorageTarget(
ctx context.Context,
rawURL string,
resolver metricIPResolver,
) (*resolvedMetricTarget, error) {
targetURL, err := url.Parse(rawURL)
if err != nil || targetURL.Opaque != "" || targetURL.Hostname() == "" || targetURL.User != nil || targetURL.Fragment != "" {
return nil, errors.New("invalid metric-storage URL")
}
targetURL.Scheme = strings.ToLower(targetURL.Scheme)
if targetURL.Scheme != "http" && targetURL.Scheme != "https" {
return nil, errors.New("metric-storage must use HTTP or HTTPS")
}
hostname := targetURL.Hostname()
port := targetURL.Port()
if port == "" {
if strings.HasSuffix(targetURL.Host, ":") {
return nil, errors.New("metric-storage URL has an empty port")
}
if targetURL.Scheme == "http" {
port = "80"
} else {
port = "443"
}
} else if portNumber, err := strconv.Atoi(port); err != nil || portNumber < 1 || portNumber > 65535 {
return nil, errors.New("metric-storage URL has an invalid port")
}

var addresses []netip.Addr
if address, err := netip.ParseAddr(hostname); err == nil {
addresses = []netip.Addr{address}
} else {
addresses, err = resolver.LookupNetIP(ctx, "ip", hostname)
if err != nil {
return nil, errors.Annotate(err, "failed to resolve metric-storage hostname")
}
}
if len(addresses) == 0 {
return nil, errors.New("metric-storage hostname resolved to no addresses")
}

for i, address := range addresses {
addresses[i] = address.Unmap()
if !isSafeMetricTargetIP(addresses[i]) {
return nil, errors.New("metric-storage resolved to an unsafe address")
}
}

return &resolvedMetricTarget{
url: targetURL,
port: port,
addresses: addresses,
}, nil
}

func isSafeMetricTargetIP(address netip.Addr) bool {
address = address.Unmap()
unsafe := !address.IsValid() || address.Zone() != "" || address.IsLoopback() ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

address.IsLinkLocalUnicast() || address.IsLinkLocalMulticast() ||
address.IsUnspecified() || address.IsMulticast() || !address.IsGlobalUnicast()
if address.Is4() {
octets := address.As4()
unsafe = unsafe || octets[0] == 0 || octets[0] >= 240
}
switch address.String() {
case "169.254.169.254", "100.100.100.200", "fd00:ec2::254":
unsafe = true
}
return !unsafe
}

func metricQueryDialDeadline(now, deadline time.Time, addressesRemaining int) time.Time {
timeRemaining := deadline.Sub(now)
timeout := timeRemaining / time.Duration(addressesRemaining)
if timeout < 2*time.Second {
timeout = min(timeRemaining, 2*time.Second)
}
return now.Add(timeout)
}

func newMetricQueryTransport(baseClient *http.Client) (*http.Transport, error) {
baseTransport := http.DefaultTransport
if baseClient != nil && baseClient.Transport != nil {
baseTransport = baseClient.Transport
}
transport, ok := baseTransport.(*http.Transport)
if !ok {
return nil, errors.New("metric query requires an HTTP transport")
}
transport = transport.Clone()
dialContext := transport.DialContext
if dialContext == nil {
dialContext = (&net.Dialer{}).DialContext
}
// A proxy would resolve and dial the target outside this transport, bypassing
// the destination validation below.
transport.Proxy = nil
transport.DialTLS = nil //nolint:staticcheck // Clear the deprecated hook as well to prevent a validation bypass.
transport.DialTLSContext = nil
transport.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) {
target, ok := ctx.Value(metricTargetContextKey{}).(*resolvedMetricTarget)
if !ok {
return nil, errors.New("metric query target is not validated")
}
type dialResult struct {
connection net.Conn
err error
}
dialCtx, cancel := context.WithCancel(ctx)
defer cancel()
dialDeadline, ok := dialCtx.Deadline()
if !ok {
dialDeadline = time.Now().Add(metricQueryTimeout)
}
// Keep fast fallback bounded while preserving the standard Dialer's multi-address behavior.
results := make(chan dialResult, 2)
next, inFlight := 0, 0
launch := func() {
address := target.addresses[next]
addressesRemaining := len(target.addresses) - next
next++
inFlight++
attemptCtx, attemptCancel := context.WithDeadline(
dialCtx,
metricQueryDialDeadline(time.Now(), dialDeadline, addressesRemaining),
)
go func() {
defer attemptCancel()
connection, err := dialContext(attemptCtx, network, net.JoinHostPort(address.String(), target.port))
results <- dialResult{connection: connection, err: err}
}()
}
launch()

var lastErr error
for inFlight > 0 {
var fallback <-chan time.Time
if next < len(target.addresses) && inFlight < cap(results) {
fallback = time.After(metricQueryDialFallbackDelay)
}
select {
case result := <-results:
inFlight--
if result.err == nil && result.connection != nil {
cancel()
for inFlight > 0 {
result := <-results
inFlight--
if result.connection != nil {
_ = result.connection.Close()
}
}
return result.connection, nil
}
lastErr = result.err
if lastErr == nil {
lastErr = errors.New("metric-storage dial returned an empty connection")
}
if next < len(target.addresses) {
launch()
}
case <-fallback:
launch()
}
}
return nil, errors.Annotate(lastErr, "failed to dial metric-storage target")
}

switch u.Scheme {
case "http", "https":
// Replace the pd path with the prometheus http API path.
r.URL.Path = strings.Replace(r.URL.Path, "pd/api/v1/metric", "api/v1", 1)
apiutil.NewCustomReverseProxies(h.s.GetHTTPClient(), []url.URL{*u}).ServeHTTP(w, r)
return transport, nil
}

func removeHopByHopHeaders(header http.Header) {
for _, connectionHeader := range header.Values("Connection") {
for _, name := range strings.Split(connectionHeader, ",") {
header.Del(strings.TrimSpace(name))
}
}
for _, name := range []string{
"Connection",
"Proxy-Connection",
"Keep-Alive",
"Proxy-Authenticate",
"Proxy-Authorization",
"Te",
"Trailer",
"Transfer-Encoding",
"Upgrade",
} {
header.Del(name)
}
}

func readMetricQueryResponse(body io.Reader, limit int64) ([]byte, error) {
limitedBody := io.LimitReader(body, limit+1)
data, err := io.ReadAll(limitedBody)
if err != nil {
return nil, errors.Annotate(err, "failed to read metric query response")
}
if int64(len(data)) > limit {
return nil, errors.New("metric query response exceeds the size limit")
}
return data, nil
}

func validatePrometheusQueryResponse(body []byte) error {
response := struct {
Status string `json:"status"`
Data *struct {
ResultType string `json:"resultType"`
Result json.RawMessage `json:"result"`
} `json:"data"`
}{}
if err := json.Unmarshal(body, &response); err != nil {
return errors.Annotate(err, "metric storage returned invalid JSON")
}
if response.Status != "success" || response.Data == nil {
return errors.New("metric storage returned a non-success Prometheus response")
}
result := strings.TrimSpace(string(response.Data.Result))
if len(result) < 2 || result[0] != '[' || result[len(result)-1] != ']' {
return errors.New("metric storage returned an invalid Prometheus result")
}
switch response.Data.ResultType {
case "matrix", "vector", "scalar", "string":
return nil
default:
// TODO: Support read data by self after support store metric data in PD/TiKV.
http.Error(w, fmt.Sprintf("schema of metric storage address is no supported, address: %v", metricAddr), http.StatusInternalServerError)
return errors.New("metric storage returned an invalid Prometheus result type")
}
}

func writeMetricQueryError(w http.ResponseWriter, err error) {
log.Warn("failed to query metric storage", zap.Error(err))
w.Header().Set("Content-Type", "application/json; charset=utf-8")
w.Header().Set("Cache-Control", "no-store")
w.WriteHeader(http.StatusBadGateway)
if _, writeErr := io.WriteString(w, metricQueryErrorBody); writeErr != nil {
log.Warn("failed to write metric query error response", zap.Error(writeErr))
}
}
Loading
Loading