Conversation
…ped IPv6 CGNAT evasion
Contributor
Reviewer's GuideThis PR hardens internal network protection for HTTP tools by making validateToolEndpoint return a pinned resolved IP, updating HTTP client creation to use that pinned address and enforce redirect-time revalidation, and extending internal address detection to cover IPv4-mapped IPv6 (including CGNAT), along with focused tests for these behaviors. Sequence diagram for HTTP tool execution with pinned DNS and redirect validationsequenceDiagram
actor User
participant GinContext as GinContext
participant Server as Server
participant Session as SessionConnection
participant Validator as validateToolEndpoint
participant HTTPClientFactory as createHTTPClient
participant HTTPClient as http_Client
participant Resolver as net_DefaultResolver
participant Endpoint as ToolEndpoint
User->>GinContext: HTTP request triggers tool
GinContext->>Server: executeHTTPTool(ctx, conn, tool, args)
Server->>Validator: validateToolEndpoint(ctx, req.URL)
alt internalNetEnabled is false
Validator-->>Server: resolvedIP "", nil
else internalNetEnabled is true
Validator->>Validator: parse host
alt host is IP literal
Validator-->>Server: resolvedIP "", nil
else host is hostname
Validator->>Resolver: LookupIPAddr(ctx, host)
Resolver-->>Validator: []IPAddr
Validator->>Validator: Unmap IPs, track firstAddr
alt internal IP found and allowed
Validator-->>Server: resolvedIP firstAddr, nil
else internal IP found but not allowed
Validator-->>Server: "", error
else only external IPs
Validator-->>Server: resolvedIP firstAddr, nil
end
end
end
alt validation error
Server-->>GinContext: log and return error
GinContext-->>User: HTTP error response
else validation ok
Server->>HTTPClientFactory: createHTTPClient(tool, pinnedAddr)
HTTPClientFactory->>HTTPClientFactory: configure baseTransport (proxy/default)
alt pinnedAddr != "" and DialContext is nil
HTTPClientFactory->>HTTPClientFactory: set DialContext to always use pinnedAddr
end
HTTPClientFactory-->>Server: http.Client with CheckRedirect
Server->>HTTPClient: Do(req)
HTTPClient->>Endpoint: initial HTTP request
loop redirects (max 10)
Endpoint-->>HTTPClient: 3xx redirect response
HTTPClient->>Validator: CheckRedirect hook calls validateToolEndpoint(ctx, redirectURL)
alt redirect target blocked
Validator-->>HTTPClient: error
HTTPClient-->>Server: error
Server-->>GinContext: log and return error
GinContext-->>User: HTTP error response
else redirect allowed
Validator-->>HTTPClient: nil
HTTPClient->>Endpoint: follow redirect
end
end
Endpoint-->>HTTPClient: final response
HTTPClient-->>Server: http.Response
Server-->>GinContext: write tool response
GinContext-->>User: HTTP success response
end
Updated class diagram for internal network validation and HTTP client creationclassDiagram
class Server {
bool internalNetEnabled
InternalNetACL internalNetACL
executeHTTPTool(c GinContext, conn SessionConnection, tool *ToolConfig, args map[string]any) error
createHTTPClient(tool *ToolConfig, pinnedAddr string) (*http.Client, error)
validateToolEndpoint(ctx context.Context, endpoint *url.URL) (resolvedIP string, err error)
}
class InternalNetACL {
allowsHost(host string) bool
allowsAddr(addr netip.Addr) bool
}
class ToolConfig {
string Name
ProxyConfig *ProxyConfig
}
class ProxyConfig {
string Type
string Host
int Port
}
class http_Client {
http.RoundTripper Transport
func(req *http.Request, via []*http.Request) error CheckRedirect
}
class http_Transport {
func(*http.Request) (*url.URL, error) Proxy
func(ctx context.Context, network string, addr string) (net.Conn, error) DialContext
}
class net_Resolver {
LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, error)
}
class netip_Addr {
bool IsPrivate()
bool IsLoopback()
bool IsLinkLocalUnicast()
bool IsLinkLocalMulticast()
bool IsMulticast()
bool IsUnspecified()
bool Is4()
bool IsGlobalUnicast()
netip.Addr Unmap()
}
class Functions {
isInternalAddr(addr netip.Addr) bool
}
class otelhttp_Transport {
NewTransport(base http.RoundTripper) http.RoundTripper
}
Server --> InternalNetACL : uses internalNetACL
Server --> http_Client : creates
http_Client --> http_Transport : uses as Transport
http_Transport <|-- otelhttp_Transport : wrapped by
Server --> net_Resolver : uses for DNS
Functions --> netip_Addr : checks internal IPs
Server --> ToolConfig : uses tool
ToolConfig --> ProxyConfig : has Proxy
Functions : isInternalAddr(addr netip.Addr) bool
netip_Addr : Unmap()
Server : validateToolEndpoint(...) returns resolvedIP for pinning
Server : createHTTPClient(...) installs CheckRedirect and pins DNS
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- When a SOCKS5 proxy is configured,
baseTransport.DialContextis already set and thepinnedAddrlogic is skipped, so DNS rebinding protection won’t apply in that mode; consider either explicitly documenting this limitation or layering the pinning on top of the SOCKS5 dialer where feasible. - In
CheckRedirect, you callvalidateToolEndpointbut ignore the returned resolved IP, so redirects are not connection-pinned the way the initial request is; if you intend to protect against DNS rebinding on redirect targets as well, consider updating the pinning strategy to account for the latest validated address.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When a SOCKS5 proxy is configured, `baseTransport.DialContext` is already set and the `pinnedAddr` logic is skipped, so DNS rebinding protection won’t apply in that mode; consider either explicitly documenting this limitation or layering the pinning on top of the SOCKS5 dialer where feasible.
- In `CheckRedirect`, you call `validateToolEndpoint` but ignore the returned resolved IP, so redirects are not connection-pinned the way the initial request is; if you intend to protect against DNS rebinding on redirect targets as well, consider updating the pinning strategy to account for the latest validated address.
## Individual Comments
### Comment 1
<location path="internal/core/tool.go" line_range="311-313" />
<code_context>
+ // Pin DNS: when validateToolEndpoint resolved a hostname to an IP, we
+ // force the transport to connect to that IP instead of re-resolving.
+ // This closes the DNS-rebinding TOCTOU window.
+ if pinnedAddr != "" && baseTransport.DialContext == nil {
+ stdDialer := &net.Dialer{}
+ baseTransport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+ _, port, err := net.SplitHostPort(addr)
+ if err != nil {
</code_context>
<issue_to_address>
**🚨 issue (security):** DNS pinning is skipped for the default transport because DialContext is already non-nil there.
Because `http.DefaultTransport.(*http.Transport).Clone()` already sets `DialContext`, the pinning logic will not apply in the typical non-proxy case, so the default path does not actually get the DNS-rebinding protection described. Only the custom `&http.Transport{}` path does. To make pinning effective and consistent, wrap whatever `DialContext` is present (capture the original and delegate to it with `addr` rewritten to use `pinnedAddr`) instead of only setting `DialContext` when it is nil. This preserves existing dial behavior (timeouts, keep-alive, etc.) while still closing the TOCTOU window.
</issue_to_address>
### Comment 2
<location path="internal/core/internal_network_test.go" line_range="149-122" />
<code_context>
+ assert.Contains(t, err.Error(), "blocked")
+}
+
+func TestCreateHTTPClient_PinnedAddr(t *testing.T) {
+ // Verify that when a pinnedAddr is provided, the client connects to it
+ target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ fmt.Fprint(w, "pinned")
+ }))
+ defer target.Close()
+
+ // Extract host:port from the test server to use as pinnedAddr
+ u, _ := url.Parse(target.URL)
+
+ s := &Server{}
+ cli, err := s.createHTTPClient(nil, u.Hostname())
+ assert.NoError(t, err)
+
+ // Request a URL with the same port but "127.0.0.1" host;
+ // the pinned addr should override the hostname resolution.
+ req, _ := http.NewRequest("GET", target.URL, nil)
+ resp, err := cli.Do(req)
+ assert.NoError(t, err)
+ assert.Equal(t, http.StatusOK, resp.StatusCode)
+ resp.Body.Close()
}
</code_context>
<issue_to_address>
**issue (testing):** Strengthen `TestCreateHTTPClient_PinnedAddr` to actually verify that the pinned address is used
The test comment promises to verify that the client actually connects to `pinnedAddr`, but the current assertions only check for a 200 response. Because `httptest.NewServer` already binds to `127.0.0.1`, the test would still pass even if the pinned address were ignored.
To better match the stated intent, you could either:
- Inject a custom `net.Dialer` (or wrapper around `DialContext`) that records the dialed `addr` and assert the host matches `pinnedAddr`, or
- Update the test comment to say it only checks that a client created with `pinnedAddr` works, and add a more targeted unit test around the dial function to validate the pinning behavior.
That way, the test either truly enforces DNS pinning or accurately documents its limited scope.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+311
to
+313
| if pinnedAddr != "" && baseTransport.DialContext == nil { | ||
| stdDialer := &net.Dialer{} | ||
| baseTransport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { |
Contributor
There was a problem hiding this comment.
🚨 issue (security): DNS pinning is skipped for the default transport because DialContext is already non-nil there.
Because http.DefaultTransport.(*http.Transport).Clone() already sets DialContext, the pinning logic will not apply in the typical non-proxy case, so the default path does not actually get the DNS-rebinding protection described. Only the custom &http.Transport{} path does. To make pinning effective and consistent, wrap whatever DialContext is present (capture the original and delegate to it with addr rewritten to use pinnedAddr) instead of only setting DialContext when it is nil. This preserves existing dial behavior (timeouts, keep-alive, etc.) while still closing the TOCTOU window.
| resp, err := cli.Do(req) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| resp.Body.Close() |
Contributor
There was a problem hiding this comment.
issue (testing): Strengthen TestCreateHTTPClient_PinnedAddr to actually verify that the pinned address is used
The test comment promises to verify that the client actually connects to pinnedAddr, but the current assertions only check for a 200 response. Because httptest.NewServer already binds to 127.0.0.1, the test would still pass even if the pinned address were ignored.
To better match the stated intent, you could either:
- Inject a custom
net.Dialer(or wrapper aroundDialContext) that records the dialedaddrand assert the host matchespinnedAddr, or - Update the test comment to say it only checks that a client created with
pinnedAddrworks, and add a more targeted unit test around the dial function to validate the pinning behavior.
That way, the test either truly enforces DNS pinning or accurately documents its limited scope.
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
1 participant
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 by Sourcery
Harden internal network protections for HTTP tools by validating DNS-resolved addresses, pinning connections to the validated IP, and enforcing redirect checks to prevent SSRF and DNS rebinding bypasses.
Bug Fixes:
Enhancements:
Tests: