[apiserver] validate request host before Do() to address gosec G704 (#4731)#4771
Open
1fanwang wants to merge 1 commit intoray-project:masterfrom
Open
[apiserver] validate request host before Do() to address gosec G704 (#4731)#47711fanwang wants to merge 1 commit intoray-project:masterfrom
1fanwang wants to merge 1 commit intoray-project:masterfrom
Conversation
…ay-project#4731) ray-project#4703 surfaces a `gosec G704: SSRF via taint analysis` warning on `apiserver/pkg/http/client.go:673` because every per-method URL is built by concatenating `krc.baseURL` with request fields. The current quick fix in ray-project#4703 suppresses the warning with a bare `//nolint:gosec`. This change adds a real defense at the same point: pre-parse `baseURL` once at construction, store the host, and verify `httpRequest.URL.Host` matches it immediately before `httpClient.Do()`. Any stray rewrite of the request URL becomes an explicit error instead of a silent foreign call. Tests cover both the foreign-host and empty-baseURL-host paths. The suppression cannot be fully removed: gosec's taint analysis flags `Do(httpRequest)` purely on the URL provenance (string concat from a struct field) and does not model field-level validation as sanitization. The `//nolint:gosec` is kept on the `Do()` call only, with a comment that points at the host check as the actual SSRF defense - which is a more honest suppression than "URL is constructed from internal config".
4d8b6c5 to
75cbdd2
Compare
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.
Problem
#4703 upgrades golangci-lint to a version where
gosec G704(SSRF via taint analysis) starts firing onapiserver/pkg/http/client.go:673:The URL passed to every method is built by concatenating
krc.baseURLwith request fields, which gosec treats as tainted. #4731 asks for a structural fix rather than a bare//nolint:gosecsuppression.Direction
Add a real defense at the same point - pre-parse
baseURLonce, then verify each request URL host matches it immediately beforehttpClient.Do(). Any stray rewrite ofhttpRequest.URL(the actual SSRF concern) becomes an explicit error instead of a silent foreign call.I attempted to fully eliminate the suppression but gosec G704's taint analysis flags
Do(httpRequest)purely on the URL's provenance (string concat from a struct field) and does not model the field-level host check as a sanitization. So a//nolint:gosecon theDo()line is still required; the change is to make that suppression honest by pointing at concrete validation rather than just claiming "internal config".Happy to take a different shape if there's a refactor pattern (e.g., constructing requests from a
*url.URLderived purely viaJoinPath, or routing through a wrapper that gosec's taint can resolve) the maintainers prefer.Evidence
Reproducer (against current
master, since #4703 isn't merged yet but the new linter shows the same flag):go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest cd kuberay golangci-lint run --no-config -E gosec --timeout 5m ./apiserver/pkg/http/...Before this change, output includes:
After this change, with the project's actual lint config:
Tested with
golangci-lint v2.11.4 / go1.26.2on macOS arm64.Implementation
apiserver/pkg/http/client.go:baseURLHostfield, populated byurl.Parseonce inNewKuberayAPIServerClient.executeRequestnow refuses to dispatch whenhttpRequest.URLis nil, whenbaseURLHostis empty (malformed input), or whenhttpRequest.URL.Hostdoes not equalbaseURLHost. The check sits immediately beforehttpClient.Do().//nolint:goseconDo()now references the validation above.apiserver/pkg/http/client_test.go:executeRequesttests now construct the client with"http://mock"so the host check passes (request URL ishttp://mock/test).TestExecuteRequestRejectsForeignHost- configuredhttp://configured-host:8888, request tohttp://attacker.example/evil, expects validation error andtransport.callCount == 0.TestExecuteRequestRejectsEmptyBaseURLHost- covers the malformed-baseURL path.TestUnmarshal*tests still pass"baseurl"because they mockExecuteHttpRequestand don't reach the validation.Interaction with #4703
If #4731 lands first, the
//nolint:gosec // URL is constructed from internal config, not user inputline that #4703 adds in the same spot can be dropped (or replaced by a rebase that picks up the version in this PR). If #4703 lands first, this PR will need a small rebase to remove that suppression and replace it with the version here. Either order works - just flagging so reviewers know.Verification
go test ./apiserver/pkg/http/...- all existing tests + 2 new tests passgolangci-lint run --timeout 5m ./apiserver/pkg/http/...(project config) - 0 issuesgolangci-lint run --no-config -E gosec --timeout 5m ./apiserver/pkg/http/...- G704 gonego build ./apiserver/...- clean