Added deny list for checking external user submitted urls#39947
Added deny list for checking external user submitted urls#39947
Conversation
| } | ||
|
|
||
| // CheckURLForSSRF validates rawURL against SSRF attack vectors using a static blocklist. | ||
| func CheckURLForSSRF(ctx context.Context, rawURL string, resolver func(ctx context.Context, host string) ([]string, error)) error { |
There was a problem hiding this comment.
Rails does this differently. It resolves DNS once, validates the IP, then connects directly to that resolved IP rather than the hostname. Our current implementation does not do this. CheckURLForSSRF resolves and validates, but then GetSmallstepSCEPChallenge calls http.NewRequest(http.MethodPost, ca.ChallengeURL, ...) with the original hostname. Thus requiring the SSRFDialContext. See the PR description for a more detailed explanation.
| func CheckURLForSSRF(ctx context.Context, rawURL string, resolver func(ctx context.Context, host string) ([]string, error)) error { | ||
| if dev_mode.IsEnabled { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Notice this is disabled in dev mode (pass --dev) flag.
| return func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| if dev_mode.IsEnabled { | ||
| return dial(ctx, network, addr) | ||
| } |
There was a problem hiding this comment.
Notice this is disabled in dev mode (pass --dev) flag.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| cli.CheckRedirect = noFollowRedirect | ||
| } | ||
| if co.tlsConf != nil { | ||
| switch { |
There was a problem hiding this comment.
[WARNING]: NewClient() without explicit transport has no SSRF protection
I'd like to be your neighbor and point something out! When NewClient is called without WithTransport() or WithTLSClientConfig(), no custom transport is set, so the client falls through to Go's http.DefaultTransport — which has no SSRF dial-time protection. This means any existing or future NewClient() call that omits WithTransport(NewTransport()) silently lacks SSRF protection.
Consider adding a default case to always use NewTransport():
switch {
case co.transport != nil:
cli.Transport = co.transport
case co.tlsConf != nil:
cli.Transport = NewTransport(WithTLSConfig(co.tlsConf))
default:
cli.Transport = NewTransport()
}There was a problem hiding this comment.
No. This is intentional.
There was a problem hiding this comment.
For example, we would need to change ALL of these https://github.com/fleetdm/fleet/actions/runs/22078563488?pr=39947
There was a problem hiding this comment.
Also adding SSRF protection to all urls is semantically incorrect in my opinion. The intent is that not all clients need SSRF protection because not all client accept user-supplied URLs.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Well, I've taken a careful look through this PR, and I have to say — it's really thoughtful work. The SSRF protection is implemented in a way that I think shows real care for the safety of the system and the people who use it. Here are some things I especially appreciated:
The existing review comments about TOCTOU/DNS rebinding and the Files Reviewed (8 files)
|
| client := fleethttp.NewClient( | ||
| fleethttp.WithTimeout(30*time.Second), | ||
| fleethttp.WithTransport(fleethttp.NewTransport()), | ||
| ) |
There was a problem hiding this comment.
I opted not to change NewClient internals to always use NewSSRFProtectedTransport(). Doing this would likely break all integration and unit tests that dial localhost, about 100+ places. Since Localhost (127.0.0.0/8) is in the blocklist.
There was a problem hiding this comment.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds SSRF protection for outbound requests: new utilities in 🚥 Pre-merge checks | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/certificate_authorities.go (1)
406-428:⚠️ Potential issue | 🟠 MajorMissing
CheckURLForSSRFforChallengeURLin the create path.
smallstepSCEP.URLis validated at line 410, butsmallstepSCEP.ChallengeURLis not checked viaCheckURLForSSRFbefore being used. The update path (validateSmallstepSCEPProxyUpdate, line 1503) does perform this check, creating an inconsistency.While
GetSmallstepSCEPChallengeuses a transport withSSRFDialContext, the early explicit check provides a clearer error and avoids unnecessary work.🛡️ Proposed fix
func (svc *Service) validateSmallstepSCEPProxy(ctx context.Context, smallstepSCEP *fleet.SmallstepSCEPProxyCA, errPrefix string) error { if err := validateCAName(smallstepSCEP.Name, errPrefix); err != nil { return err } if err := fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.URL, nil); err != nil { return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sSmallstep SCEP URL is invalid: %v", errPrefix, err)) } + if err := fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.ChallengeURL, nil); err != nil { + return fleet.NewInvalidArgumentError("challenge_url", fmt.Sprintf("%sChallenge URL is invalid: %v", errPrefix, err)) + } if smallstepSCEP.Username == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/certificate_authorities.go` around lines 406 - 428, In validateSmallstepSCEPProxy add the same SSRF protection used for smallstepSCEP.URL to also validate smallstepSCEP.ChallengeURL: call fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.ChallengeURL, nil) and return a fleet.NewInvalidArgumentError("challenge_url", ...) on error (consistent with the update path in validateSmallstepSCEPProxyUpdate); keep the existing logging/BadRequest behavior for later ValidateSmallstepChallengeURL/ValidateSCEPURL calls.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@changes/14284-external-deny-list`:
- Line 1: Fix the typo "Certifcate" → "Certificate" by updating all occurrences
(PR description and code/doc comments) to read "Certificate authorities";
specifically search and replace in the commit/description text that mentions the
deny list and in pkg/fleethttp/ssrf.go (and any related docs or flags text
referencing certificate authorities) so the wording is correct everywhere.
In `@ee/server/service/certificate_authorities.go`:
- Around line 406-428: In validateSmallstepSCEPProxy add the same SSRF
protection used for smallstepSCEP.URL to also validate
smallstepSCEP.ChallengeURL: call fleethttp.CheckURLForSSRF(ctx,
smallstepSCEP.ChallengeURL, nil) and return a
fleet.NewInvalidArgumentError("challenge_url", ...) on error (consistent with
the update path in validateSmallstepSCEPProxyUpdate); keep the existing
logging/BadRequest behavior for later
ValidateSmallstepChallengeURL/ValidateSCEPURL calls.
In `@pkg/fleethttp/fleethttp.go`:
- Around line 125-129: The SSRFDialContext implementation performs hostname
resolution for validation but then calls the underlying dialer with the original
hostname, allowing a TOCTOU DNS-rebinding attack; update SSRFDialContext (and
the helper checkResolvedAddrs) to perform DNS resolution once, validate the
resolved IPs, select a safe resolved IP, and call the underlying dialer with
net.JoinHostPort(safeIP, port) so the connect uses the validated IP rather than
re-resolving; ensure the port extraction from addr and any preserved network
parameter (e.g., "tcp") are handled when invoking dial(ctx, network,
resolvedAddr).
In `@pkg/fleethttp/ssrf.go`:
- Around line 83-85: The SSRFError.Error() message should include the substring
"IP" so tests expecting "blocked IP address" pass; update the Error() method on
type SSRFError (function SSRFError.Error) to return a string that contains
"blocked IP address" (e.g., change "resolves to a blocked address" to "resolves
to a blocked IP address") so TestCheckURLForSSRFSSRFErrorMessage's assertion
matches.
- Around line 13-50: The constants IPV4_BLACKLIST and IPV6_BLACKLIST use
SCREAMING_SNAKE_CASE and are exported mutable slices—rename them and/or change
their API to follow Go conventions: either make them unexported (e.g.,
ipv4Blocklist, ipv6Blocklist) since only init() populates blockedCIDRs, or
expose read-only accessors (e.g., func IPv4Blocklist() []string and func
IPv6Blocklist() []string) that return copies (use append([]string(nil), ...)).
Update references in init() and any other uses (blockedCIDRs, init) to the new
names so callers cannot mutate the internal slices.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fleethttp/ssrf.go`: - Around line 13-50: The constants IPV4_BLACKLIST and IPV6_BLACKLIST use SCREAMING_SNAKE_CASE and are exported mutable slices—rename them and/or change their API to follow Go conventions: either make them unexported (e.g., ipv4Blocklist, ipv6Blocklist) since only init() populates blockedCIDRs, or expose read-only accessors (e.g., func IPv4Blocklist() []string and func IPv6Blocklist() []string) that return copies (use append([]string(nil), ...)). Update references in init() and any other uses (blockedCIDRs, init) to the new names so callers cannot mutate the internal slices.pkg/fleethttp/ssrf.go (1)
13-50: Go naming:IPV4_BLACKLIST/IPV6_BLACKLISTdon't follow Go conventions.Go uses MixedCaps for exported identifiers (e.g.,
IPv4Blocklist,IPv6Blocklist). SCREAMING_SNAKE_CASE is unconventional. Additionally, these are exported mutable slices, but sinceblockedCIDRsis populated ininit(), mutations after initialization have no effect — which could be confusing for consumers.Consider either making them unexported (since only
init()uses them) or converting to functions that return copies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fleethttp/ssrf.go` around lines 13 - 50, The constants IPV4_BLACKLIST and IPV6_BLACKLIST use SCREAMING_SNAKE_CASE and are exported mutable slices—rename them and/or change their API to follow Go conventions: either make them unexported (e.g., ipv4Blocklist, ipv6Blocklist) since only init() populates blockedCIDRs, or expose read-only accessors (e.g., func IPv4Blocklist() []string and func IPv6Blocklist() []string) that return copies (use append([]string(nil), ...)). Update references in init() and any other uses (blockedCIDRs, init) to the new names so callers cannot mutate the internal slices.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/certificate_authorities.go (1)
406-428:⚠️ Potential issue | 🟠 MajorMissing SSRF check on
ChallengeURLin the create path.
validateSmallstepSCEPProxyvalidatessmallstepSCEP.URLfor SSRF (line 410) but does not validatesmallstepSCEP.ChallengeURL. The update path (validateSmallstepSCEPProxyUpdate, line 1503) does checkChallengeURL. This inconsistency means a user could submit a private-IP ChallengeURL during CA creation and only hit the dial-time SSRF transport block rather than getting a clear validation error.Proposed fix
func (svc *Service) validateSmallstepSCEPProxy(ctx context.Context, smallstepSCEP *fleet.SmallstepSCEPProxyCA, errPrefix string) error { if err := validateCAName(smallstepSCEP.Name, errPrefix); err != nil { return err } if err := fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.URL, nil); err != nil { return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sSmallstep SCEP URL is invalid: %v", errPrefix, err)) } + if err := fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.ChallengeURL, nil); err != nil { + return fleet.NewInvalidArgumentError("challenge_url", fmt.Sprintf("%sChallenge URL is invalid: %v", errPrefix, err)) + } if smallstepSCEP.Username == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/certificate_authorities.go` around lines 406 - 428, The create-path validator validateSmallstepSCEPProxy is missing an SSRF check for smallstepSCEP.ChallengeURL; add a call to fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.ChallengeURL, nil) (similar to the existing check for smallstepSCEP.URL) before invoking svc.scepConfigService.ValidateSmallstepChallengeURL, and return a fleet.NewInvalidArgumentError("challenge_url", ...) with a clear message on failure so creation matches the update-path behavior in validateSmallstepSCEPProxyUpdate.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@ee/server/service/certificate_authorities.go`:
- Around line 406-428: The create-path validator validateSmallstepSCEPProxy is
missing an SSRF check for smallstepSCEP.ChallengeURL; add a call to
fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.ChallengeURL, nil) (similar to the
existing check for smallstepSCEP.URL) before invoking
svc.scepConfigService.ValidateSmallstepChallengeURL, and return a
fleet.NewInvalidArgumentError("challenge_url", ...) with a clear message on
failure so creation matches the update-path behavior in
validateSmallstepSCEPProxyUpdate.
In `@ee/server/service/scep_proxy.go`:
- Around line 514-517: The code creates a client with fleethttp.NewClient then
directly assigns client.Transport to an ntlmssp.Negotiator wrapping
fleethttp.NewSSRFProtectedTransport(); instead, construct the negotiator
(ntlmssp.Negotiator{RoundTripper: fleethttp.NewSSRFProtectedTransport()}) and
pass it into fleethttp.NewClient via fleethttp.WithTransport so the client is
configured consistently (use fleethttp.WithTimeout and fleethttp.WithTransport
together) rather than overwriting client.Transport after creation.
In `@pkg/fleethttp/ssrf_test.go`:
- Around line 106-112: The test TestCheckURLForSSRFBadScheme currently uses
assert.NotErrorIs with a typed-nil pointer which doesn't correctly check for an
SSRFError; update the assertion to use errors.As to verify the returned error is
not an SSRFError. In TestCheckURLForSSRFBadScheme, declare a var ssrfErr
*SSRFError and replace the NotErrorIs assertion with an assertion that
errors.As(err, &ssrfErr) is false (e.g., assert.False) so the test correctly
validates that CheckURLForSSRF did not return an SSRFError for the bad-scheme
case.
In `@pkg/fleethttp/ssrf.go`:
- Around line 87-111: In checkResolvedAddrs, add a brief comment above the
net.SplitHostPort call explaining that resolver implementations like
net.DefaultResolver.LookupHost return plain IP strings without ports so
SplitHostPort will normally fail and we fallback to using addr; this documents
the defensive parsing (handling potential resolver implementations that may
include ports) and prevents future confusion around the SplitHostPort + fallback
logic.
- Around line 15-49: The exported variables IPV4_BLACKLIST and IPV6_BLACKLIST
use SCREAMING_SNAKE_CASE and "blacklist" which violates Go naming conventions;
rename them to mixed caps and use the modern term (e.g., IPv4Blocklist and
IPv6Blocklist), update all references across the package to the new names
(including any tests or callers), and keep the slice contents unchanged so
behavior remains identical.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/scep_proxy.go`: - Around line 514-517: The code creates a client with fleethttp.NewClient then directly assigns client.Transport to an ntlmssp.Negotiator wrapping fleethttp.NewSSRFProtectedTransport(); instead, construct the negotiator (ntlmssp.Negotiator{RoundTripper: fleethttp.NewSSRFProtectedTransport()}) and pass it into fleethttp.NewClient via fleethttp.WithTransport so the client is configured consistently (use fleethttp.WithTimeout and fleethttp.WithTransport together) rather than overwriting client.Transport after creation. In `@pkg/fleethttp/ssrf.go`: - Around line 87-111: In checkResolvedAddrs, add a brief comment above the net.SplitHostPort call explaining that resolver implementations like net.DefaultResolver.LookupHost return plain IP strings without ports so SplitHostPort will normally fail and we fallback to using addr; this documents the defensive parsing (handling potential resolver implementations that may include ports) and prevents future confusion around the SplitHostPort + fallback logic. - Around line 15-49: The exported variables IPV4_BLACKLIST and IPV6_BLACKLIST use SCREAMING_SNAKE_CASE and "blacklist" which violates Go naming conventions; rename them to mixed caps and use the modern term (e.g., IPv4Blocklist and IPv6Blocklist), update all references across the package to the new names (including any tests or callers), and keep the slice contents unchanged so behavior remains identical.ee/server/service/scep_proxy.go (1)
514-517: Consider usingWithTransportinstead of overwritingclient.Transportdirectly.The NDES path creates a client via
NewClientand then overwritesclient.Transport. SinceWithTransportnow exists, you could wire it more cleanly. However, thentlmssp.Negotiatorwraps the transport, so you'd still need to construct the negotiator first and pass it in. Current approach works fine — just a minor consistency note versus the Smallstep path below.Optional: use WithTransport for consistency
- client := fleethttp.NewClient(fleethttp.WithTimeout(*s.Timeout)) - client.Transport = ntlmssp.Negotiator{ - RoundTripper: fleethttp.NewSSRFProtectedTransport(), - } + client := fleethttp.NewClient( + fleethttp.WithTimeout(*s.Timeout), + fleethttp.WithTransport(ntlmssp.Negotiator{ + RoundTripper: fleethttp.NewSSRFProtectedTransport(), + }), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/scep_proxy.go` around lines 514 - 517, The code creates a client with fleethttp.NewClient then directly assigns client.Transport to an ntlmssp.Negotiator wrapping fleethttp.NewSSRFProtectedTransport(); instead, construct the negotiator (ntlmssp.Negotiator{RoundTripper: fleethttp.NewSSRFProtectedTransport()}) and pass it into fleethttp.NewClient via fleethttp.WithTransport so the client is configured consistently (use fleethttp.WithTimeout and fleethttp.WithTransport together) rather than overwriting client.Transport after creation.pkg/fleethttp/ssrf.go (2)
87-111:SplitHostPorton resolver results is misleading but harmless.
net.DefaultResolver.LookupHostreturns plain IP strings (no port), soSplitHostPortalways fails and falls through toh = addr. The defensive code isn't wrong, but a brief comment explaining why it's there would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fleethttp/ssrf.go` around lines 87 - 111, In checkResolvedAddrs, add a brief comment above the net.SplitHostPort call explaining that resolver implementations like net.DefaultResolver.LookupHost return plain IP strings without ports so SplitHostPort will normally fail and we fallback to using addr; this documents the defensive parsing (handling potential resolver implementations that may include ports) and prevents future confusion around the SplitHostPort + fallback logic.
15-49: Exported variable naming:IPV4_BLACKLIST/IPV6_BLACKLISTdeviates from Go convention.Go convention for exported variables is
IPv4Blocklist/IPv6Blocklist(MixedCaps). SCREAMING_SNAKE_CASE is a C/Python convention. Also, "blocklist" is the modern preferred term over "blacklist."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fleethttp/ssrf.go` around lines 15 - 49, The exported variables IPV4_BLACKLIST and IPV6_BLACKLIST use SCREAMING_SNAKE_CASE and "blacklist" which violates Go naming conventions; rename them to mixed caps and use the modern term (e.g., IPv4Blocklist and IPv6Blocklist), update all references across the package to the new names (including any tests or callers), and keep the slice contents unchanged so behavior remains identical.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39947 +/- ##
==========================================
+ Coverage 66.27% 66.29% +0.01%
==========================================
Files 2439 2440 +1
Lines 195464 195503 +39
Branches 8551 8551
==========================================
+ Hits 129538 129601 +63
+ Misses 54195 54162 -33
- Partials 11731 11740 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| // net.Dialer has no API to accept a pre-resolved IP list | ||
| // This is similar to what go does with dialSerial | ||
| // /usr/local/go/src/net/dial.go#dialSerial |
There was a problem hiding this comment.
I do not like the code below. However we either do
dial(ctx, network, net.JoinHostPort(safeIPs[0].String(), port)) and potentially ignore multiple IP addresses from hostnames or do
dial(ctx, network, net.JoinHostPort(host, port)) and risk a DNS rebinding attack.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good and follows industry best practices. Thank you for this fix.
I left a couple non-blocking comments.
| if err := validateURL(digicertCA.URL, "DigiCert", errPrefix); err != nil { | ||
| return err | ||
| if err := fleethttp.CheckURLForSSRF(ctx, digicertCA.URL, nil); err != nil { | ||
| return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sDigiCert URL is invalid: %v", errPrefix, err)) |
There was a problem hiding this comment.
Just curious, are we getting a clear message in the frontend for this?
| dial = base.DialContext | ||
| } | ||
| return func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| if dev_mode.IsEnabled { |
There was a problem hiding this comment.
Does this mean that any tests that touch this function cannot use t.Parallel() because another test may be changing this value?
There was a problem hiding this comment.
yes. I had to remove a t.Parallel() in ee/server/service/certificate_authorities_test.go#TestUpdatingCertificateAuthorities
- t.Parallel()
+ // Enable dev mode so CheckURLForSSRF skips the private-IP blocklist for the duration of this test.
+ dev_mode.IsEnabled = true
+ t.Cleanup(func() { dev_mode.IsEnabled = false })
Certainly annoying. But I am not quite sure how we can get around this without setting --dev on for every test, or having tests not point to localhost
2 participants
This PR changes 3 things.
admin_url+ all URLs for HTTPS/non-privateDialContexthook in fleethttp.NewClient(), this is needed for DNS-rebinding protection at connection timeIMPORTANT
There are two validations occurring.
CheckURLForSSRFSSRFDialContextWhy?
CheckURLForSSRFchecks the hostname. It resolves DNS, validates the ip, and then returns an error to the user. It protects certificate authority create/update API endpoints. But thenGetSmallstepSCEPChallengecallshttp.NewRequest(http.MethodPost, ca.ChallengeURL, ...)with the original hostnameThis is where
SSRFDialContextcomes into play. It fires when an actual HTTP request is attempted. Meaning Fleet would first build the request, encode the body, set up TLS, etc., before being blocked at the dial.CheckURLForSSRFstops the operation before any of that work happens.SSRFDialContextprotects the actual challenge fetch that happens later at enrollment time. They're not always called together. The dial-time check is the only thing protecting the enrollment request and DNS rebinding.Should we remove
CheckURLForSSRFThis is debatable and I don't have a strong opinion. Removing
CheckURLForSSRFwould still provide the same protection. However, it would return a generic connection error from the HTTP client which would make it slightly hard to diagnose why it is broken.What's next
I implemented this for certificate authorities. I am sure there are other places in the code base that take user submitted urls and could also use this check. That is outside the scope of this particular PR. But worthy to investigate in the near future.
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit