-
Notifications
You must be signed in to change notification settings - Fork 776
server: harden metric query proxy #11077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 5 commits
2be1539
6fd6fe6
bed1b2f
86627cf
375fb95
37df948
194a00e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,42 +15,310 @@ | |
| 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 | ||
| 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()) | ||
| 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 { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/**' || trueRepository: tikv/pd Length of output: 26387 🌐 Web query:
💡 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:
💡 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.
🤖 Prompt for AI Agents |
||
|
|
||
| // RoundTrip does not follow redirects. The target is resolved and validated before dialing. | ||
| response, err := transport.RoundTrip(request) //nolint:gosec | ||
| if err != nil { | ||
| 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") | ||
| } | ||
|
|
||
| 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) | ||
| 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() || | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 resolver := staticMetricResolver{addresses: []netip.Addr{
netip.MustParseAddr("127.0.0.1"),
}}
_, err := resolveMetricStorageTarget(context.Background(),
"http://metrics.example", resolver)
require.Error(t, err)However, 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 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") | ||
| } | ||
| var lastErr error | ||
| for _, address := range target.addresses { | ||
| connection, err := dialContext(ctx, network, net.JoinHostPort(address.String(), target.port)) | ||
| if err == nil { | ||
| return connection, nil | ||
| } | ||
| lastErr = err | ||
| if ctx.Err() != nil { | ||
| break | ||
| } | ||
| } | ||
| return nil, errors.Annotate(lastErr, "failed to dial metric-storage target") | ||
| } | ||
|
|
||
| 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)) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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.NewHandleris invoked bycombineBuilderServerHTTPServiceduringserver.CreateServer(server/server.go:303), whiles.httpClientis assigned only later bystartClient(server/server.go:446) afterRunstarts. Consequently,s.GetHTTPClient()is nil here during normal startup andnewMetricQueryTransportpermanently falls back tohttp.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 throughserver.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-startClienttransport.