[apiserver] validate request host before Do() to address gosec G704 (#4731)

apiserver/pkg/http/client.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 
 	rpcStatus "google.golang.org/genproto/googleapis/rpc/status"
@@ -27,7 +28,10 @@ type KuberayAPIServerClient struct {
 	// Store http request handling function for unit test purpose.
 	ExecuteHttpRequest func(httpRequest *http.Request, URL string) ([]byte, *rpcStatus.Status, error)
 	baseURL            string
-	retryCfg           apiserversdkutil.RetryConfig
+	// baseURLHost is the parsed Host (host[:port]) of baseURL, used by executeRequest
+	// to verify each outbound request points at the configured server before issuing it.
+	baseURLHost string
+	retryCfg    apiserversdkutil.RetryConfig
 }
 
 type KuberayAPIServerClientError struct {
@@ -49,9 +53,17 @@ func IsNotFoundError(err error) bool {
 }
 
 func NewKuberayAPIServerClient(baseURL string, httpClient *http.Client, retryCfg apiserversdkutil.RetryConfig) *KuberayAPIServerClient {
+	// Pre-parse baseURL so executeRequest can verify each outbound request targets the
+	// configured host. A malformed baseURL leaves baseURLHost empty; executeRequest will
+	// then refuse to dispatch with a descriptive error.
+	var baseURLHost string
+	if parsed, err := url.Parse(baseURL); err == nil {
+		baseURLHost = parsed.Host
+	}
 	client := &KuberayAPIServerClient{
-		httpClient: httpClient,
-		baseURL:    baseURL,
+		httpClient:  httpClient,
+		baseURL:     baseURL,
+		baseURLHost: baseURLHost,
 		marshaler: &protojson.MarshalOptions{
 			Multiline:       true,
 			Indent:          "    ",
@@ -664,13 +676,29 @@ func (krc *KuberayAPIServerClient) executeRequest(httpRequest *http.Request, URL
 		}
 	}
 
+	// Pin every outbound request to the configured baseURL host. This both adds an explicit
+	// SSRF defense (a stray rewrite of httpRequest.URL becomes observable instead of silent)
+	// and gives gosec G704 a sanitization point so the linter no longer treats httpClient.Do
+	// as a tainted call.
+	if httpRequest.URL == nil {
+		return nil, nil, fmt.Errorf("failed to execute http request for url '%s': request URL is nil", URL)
+	}
+	if krc.baseURLHost == "" || httpRequest.URL.Host != krc.baseURLHost {
+		return nil, nil, fmt.Errorf(
+			"failed to execute http request for url '%s': request host %q does not match configured baseURL host %q",
+			URL, httpRequest.URL.Host, krc.baseURLHost)
+	}
+
 	doReq := func() ([]byte, int, error) {
 		// new ReadCloser for httpRequest body
 		if requestBodyBytes != nil {
 			httpRequest.Body = io.NopCloser(bytes.NewBuffer(requestBodyBytes))
 		}
 
-		response, err := krc.httpClient.Do(httpRequest)
+		// httpRequest.URL.Host has been verified above to equal the baseURL host configured
+		// at NewKuberayAPIServerClient time, so this is not a tainted external destination.
+		// gosec G704's taint analysis cannot model that check, hence the suppression.
+		response, err := krc.httpClient.Do(httpRequest) //nolint:gosec // URL host is validated against configured baseURL above
 		if err != nil {
 			return nil, 0, fmt.Errorf("failed to execute http request for url '%s': %w", URL, err)
 		}

apiserver/pkg/http/client_test.go

@@ -214,7 +214,7 @@ func TestAPIServerClientRetry(t *testing.T) {
 				MaxBackoff:     util.HTTPClientDefaultMaxBackoff,
 				OverallTimeout: util.HTTPClientDefaultOverallTimeout,
 			}
-			client := NewKuberayAPIServerClient("baseurl", mockClient, retryCfg)
+			client := NewKuberayAPIServerClient("http://mock", mockClient, retryCfg)
 
 			body, status, err := client.executeRequest(req, "http://mock/test")
 
@@ -270,7 +270,7 @@ func TestAPIServerClientBackoff(t *testing.T) {
 		OverallTimeout: util.HTTPClientDefaultOverallTimeout,
 	}
 
-	client := NewKuberayAPIServerClient("baseurl", mockClient, retryCfg)
+	client := NewKuberayAPIServerClient("http://mock", mockClient, retryCfg)
 
 	ctx := context.Background()
 	req, err := http.NewRequestWithContext(ctx, "GET", "http://mock/test", nil)
@@ -312,7 +312,7 @@ func TestAPIServerClientOverallTimeout(t *testing.T) {
 		OverallTimeout: 1 * time.Millisecond,
 	}
 
-	client := NewKuberayAPIServerClient("baseurl", mockClient, retryCfg)
+	client := NewKuberayAPIServerClient("http://mock", mockClient, retryCfg)
 
 	ctx := context.Background()
 	req, err := http.NewRequestWithContext(ctx, "GET", "http://mock/test", nil)
@@ -324,3 +324,60 @@ func TestAPIServerClientOverallTimeout(t *testing.T) {
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "retry timeout exceeded context deadline")
 }
+
+// Verify executeRequest refuses to dispatch when the request URL host does not match the
+// configured baseURL host, defending against accidental SSRF (and giving gosec G704 a
+// sanitization point).
+func TestExecuteRequestRejectsForeignHost(t *testing.T) {
+	retryCfg := util.RetryConfig{
+		MaxRetry:       util.HTTPClientDefaultMaxRetry,
+		BackoffFactor:  util.HTTPClientDefaultBackoffFactor,
+		InitBackoff:    util.HTTPClientDefaultInitBackoff,
+		MaxBackoff:     util.HTTPClientDefaultMaxBackoff,
+		OverallTimeout: util.HTTPClientDefaultOverallTimeout,
+	}
+
+	transport := &mockTransport{statusSequence: []int{http.StatusOK}}
+	mockClient := &http.Client{Transport: transport}
+	client := NewKuberayAPIServerClient("http://configured-host:8888", mockClient, retryCfg)
+
+	ctx := context.Background()
+	req, err := http.NewRequestWithContext(ctx, "GET", "http://attacker.example/evil", nil)
+	require.NoError(t, err)
+
+	body, status, err := client.executeRequest(req, "http://attacker.example/evil")
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "does not match configured baseURL host")
+	require.Nil(t, status)
+	require.Nil(t, body)
+	// The request must never reach the transport.
+	require.Equal(t, 0, transport.callCount)
+}
+
+// Verify executeRequest refuses to dispatch when the client was constructed with a
+// malformed baseURL that yields no host. Without this guard, gosec G704 has no
+// sanitization point and every Do() call would be flagged.
+func TestExecuteRequestRejectsEmptyBaseURLHost(t *testing.T) {
+	retryCfg := util.RetryConfig{
+		MaxRetry:       util.HTTPClientDefaultMaxRetry,
+		BackoffFactor:  util.HTTPClientDefaultBackoffFactor,
+		InitBackoff:    util.HTTPClientDefaultInitBackoff,
+		MaxBackoff:     util.HTTPClientDefaultMaxBackoff,
+		OverallTimeout: util.HTTPClientDefaultOverallTimeout,
+	}
+
+	transport := &mockTransport{statusSequence: []int{http.StatusOK}}
+	mockClient := &http.Client{Transport: transport}
+	// "baseurl" parses without error but yields an empty Host.
+	client := NewKuberayAPIServerClient("baseurl", mockClient, retryCfg)
+
+	ctx := context.Background()
+	req, err := http.NewRequestWithContext(ctx, "GET", "http://mock/test", nil)
+	require.NoError(t, err)
+
+	_, _, err = client.executeRequest(req, "http://mock/test")
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "does not match configured baseURL host")
+	require.Equal(t, 0, transport.callCount)
+}