fix(storage): enforce upstream host equality to harden against SSRF#227
Merged
Conversation
CodeQL go/request-forgery flags httpClient.Do(req) in sendToPrometheus() because the taint tracker cannot prove that mapURL() rewrites Host/Scheme to the configured upstream before the request leaves the process. The prior isValidURL() only checked scheme + non-empty host. Replace it with validateUpstreamURL(), a method on prometheusStorageClient that additionally rejects any host other than promCli.url.Host or, when distinct, promCli.federateURL.Host. The mapURL() rewrite still does the real work; this validator turns the safety property into something both a static analyzer and future maintainers can see explicitly. Add table-driven tests covering: trusted upstream, trusted federate, foreign host, localhost, empty host, unknown scheme, ftp scheme, malformed URL — plus an integration test confirming sendToPrometheus short-circuits before any HTTP call when given an untrusted host. No behavior change for legitimate traffic. mapURL() is unmodified.
The Makefile (generated by go-makefile-maker) writes build/coverprofile.out on every `make check` run, but the file was both tracked in git and missing from .gitignore. Every test run polluted the diff with thousands of lines of regenerated coverage data, and security-review hooks flagged it as noise. Add the path to .gitignore and remove the tracked copy from the index. The working copy on disk is preserved and will be regenerated by future test runs.
notque
requested review from
Kuckkuck,
LeoZhangZXZ,
ashifnihalb,
ibakshay,
joluc,
notandy,
olandr,
richardtief,
timojohlo,
trouaux and
viennaa
as code owners
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
3 tasks
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
2 participants
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.
Summary
go/request-forgeryand to future maintainers.build/coverprofile.outand add it to.gitignore— the Makefile rewrites it on everymake check, polluting diffs and security-review hook output.Why
CodeQL flagged
httpClient.Do(req)inpkg/storage/prometheus.goas a request-forgery sink. The code was already structurally safe —mapURL()rewritesScheme/Host/Userto the trusted upstream before any request goes out — butisValidURL()only checked scheme + non-empty host, so the safety property was implicit. A future change tomapURL()could regress that invariant silently.This PR replaces
isValidURLwith a method on*prometheusStorageClientthat validates the post-rewrite URL host againstpromCli.url.HostandpromCli.federateURL.Host. Anything else is rejected before the HTTP call. Defense-in-depth against future regressions, and the property is now provable to a static analyzer.Changes
pkg/storage/prometheus.goisValidURLwith(*prometheusStorageClient).validateUpstreamURL. Useslices.Containsfor the allow-list check (golangci-lint v2modernizerequirement).pkg/storage/prometheus_test.goTestValidateUpstreamURL(8 table cases) +TestSendToPrometheusRejectsUntrustedHostintegration test confirming the validator short-circuits before any HTTP call.CHANGELOG.md### Securityentry under## [Unreleased]..gitignorebuild/coverprofile.out.build/coverprofile.outgit rm --cached). Working copy preserved; futuremake checkruns regenerate it.Test plan
make generate && make checkpasses clean (golangci-lint: 0 issues, all 5 packages green, build succeeds)TestValidateUpstreamURLcovers: trusted upstream, trusted federate, foreign host rejected, localhost rejected, empty host rejected, unknown scheme rejected, ftp scheme rejected, malformed URL rejectedTestSendToPrometheusRejectsUntrustedHostconfirms validation short-circuits before HTTP dispatchgo/request-forgeryfinding cleared