fix(safehttp): clone http.DefaultTransport, sharpen Image.Transport contract
Address review feedback on PR #2044. safehttp.Transport(): * Clone http.DefaultTransport instead of building a bare &http.Transport{} so Proxy, ForceAttemptHTTP2, MaxIdleConns, IdleConnTimeout, TLSHandshakeTimeout and ExpectContinueTimeout are inherited (the bare struct loses them all). Verified by new TestTransport_PreservesDefaultTransportSettings. * TestTransport_AllowsPublic: bound the dial of TEST-NET-3 with a 100ms context so the test does not depend on real-world routing of 203.0.113.0/24, and drop the dead dialer var. proxy/image.go: * Document Image.Transport contract: nil installs safehttp.Transport (SSRF-safe); caller-supplied transport is the caller's responsibility. * Replace the misleading "SSRF mitigated by safehttp.Transport" nolint comments with one that points at the documented contract above.
This commit is contained in:
committed by
Umputun
parent
e98657a88a
commit
3b1d7be6fc
@@ -16,40 +16,44 @@ import (
|
||||
// that resolves to a private/reserved IP, choosing the IP itself for the dial
|
||||
// to defeat DNS rebinding (an attacker cannot have the resolver hand back a
|
||||
// public IP at the check and a private one at the connect).
|
||||
//
|
||||
// The returned transport is a clone of http.DefaultTransport with only
|
||||
// DialContext overridden, preserving Proxy, HTTP/2, idle/keep-alive and
|
||||
// TLS handshake timeouts that bare &http.Transport{} would lose.
|
||||
func Transport() *http.Transport {
|
||||
dialer := &net.Dialer{Timeout: 30 * time.Second}
|
||||
return &http.Transport{
|
||||
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
|
||||
host, port, err := net.SplitHostPort(addr)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("invalid address %s: %w", addr, err)
|
||||
}
|
||||
t := http.DefaultTransport.(*http.Transport).Clone()
|
||||
t.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
|
||||
host, port, err := net.SplitHostPort(addr)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("invalid address %s: %w", addr, err)
|
||||
}
|
||||
|
||||
ips, err := net.DefaultResolver.LookupIPAddr(ctx, host)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("can't resolve host %s: %w", host, err)
|
||||
}
|
||||
if len(ips) == 0 {
|
||||
return nil, fmt.Errorf("no IP addresses resolved for host %s", host)
|
||||
}
|
||||
ips, err := net.DefaultResolver.LookupIPAddr(ctx, host)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("can't resolve host %s: %w", host, err)
|
||||
}
|
||||
if len(ips) == 0 {
|
||||
return nil, fmt.Errorf("no IP addresses resolved for host %s", host)
|
||||
}
|
||||
|
||||
for _, ip := range ips {
|
||||
if IsPrivateIP(ip.IP) {
|
||||
return nil, fmt.Errorf("access to private address is not allowed")
|
||||
}
|
||||
for _, ip := range ips {
|
||||
if IsPrivateIP(ip.IP) {
|
||||
return nil, fmt.Errorf("access to private address is not allowed")
|
||||
}
|
||||
}
|
||||
|
||||
var lastErr error
|
||||
for _, ip := range ips {
|
||||
conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port))
|
||||
if dialErr == nil {
|
||||
return conn, nil
|
||||
}
|
||||
lastErr = dialErr
|
||||
var lastErr error
|
||||
for _, ip := range ips {
|
||||
conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port))
|
||||
if dialErr == nil {
|
||||
return conn, nil
|
||||
}
|
||||
return nil, fmt.Errorf("can't connect to %s: %w", host, lastErr)
|
||||
},
|
||||
lastErr = dialErr
|
||||
}
|
||||
return nil, fmt.Errorf("can't connect to %s: %w", host, lastErr)
|
||||
}
|
||||
return t
|
||||
}
|
||||
|
||||
// privateCIDRs holds pre-parsed private/reserved CIDR blocks.
|
||||
|
||||
@@ -63,16 +63,28 @@ func TestTransport_BlocksPrivate(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestTransport_AllowsPublic(t *testing.T) {
|
||||
dialer := &net.Dialer{Timeout: 2 * time.Second}
|
||||
tr := Transport()
|
||||
// monkey-check the Dialer wiring directly: the transport must reject 127.0.0.1
|
||||
// the policy check must reject the loopback literal
|
||||
_, err := tr.DialContext(context.Background(), "tcp", "127.0.0.1:1")
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "access to private address is not allowed")
|
||||
|
||||
// public IP literal goes through the dial path (will likely error on connect, but NOT on policy)
|
||||
_, err = tr.DialContext(context.Background(), "tcp", "203.0.113.1:1")
|
||||
// public IP literal passes the policy check; bound the dial with a tight context
|
||||
// so the test does not depend on real-world routing of TEST-NET-3 (203.0.113.0/24).
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
|
||||
defer cancel()
|
||||
_, err = tr.DialContext(ctx, "tcp", "203.0.113.1:1")
|
||||
require.Error(t, err)
|
||||
assert.NotContains(t, err.Error(), "access to private address is not allowed")
|
||||
_ = dialer
|
||||
}
|
||||
|
||||
func TestTransport_PreservesDefaultTransportSettings(t *testing.T) {
|
||||
def := http.DefaultTransport.(*http.Transport)
|
||||
tr := Transport()
|
||||
assert.NotNil(t, tr.Proxy, "Proxy must be inherited from http.DefaultTransport")
|
||||
assert.Equal(t, def.ForceAttemptHTTP2, tr.ForceAttemptHTTP2, "ForceAttemptHTTP2")
|
||||
assert.Equal(t, def.MaxIdleConns, tr.MaxIdleConns, "MaxIdleConns")
|
||||
assert.Equal(t, def.IdleConnTimeout, tr.IdleConnTimeout, "IdleConnTimeout")
|
||||
assert.Equal(t, def.TLSHandshakeTimeout, tr.TLSHandshakeTimeout, "TLSHandshakeTimeout")
|
||||
assert.Equal(t, def.ExpectContinueTimeout, tr.ExpectContinueTimeout, "ExpectContinueTimeout")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user