From 9e6b8e02055188ec39f7916fecb3ef4aac5aa189 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Fri, 29 May 2026 10:49:39 -0700 Subject: [PATCH 1/4] feat(tcp): route the tunnel through an HTTP/SOCKS5 forward proxy pomerium-cli tcp previously dialed the Pomerium edge directly, so it could not run from networks whose only egress is a forward proxy. Honor a forward proxy for the tunnel when configured via HTTP_PROXY/HTTPS_PROXY/NO_PROXY (HTTP CONNECT), ALL_PROXY (SOCKS5), or an explicit --forward-proxy flag; the flag also routes the auth fetch. When no proxy is configured behavior is unchanged, and HTTP/3 is skipped when a proxy is in use. Forward proxying is TCP-only; UDP always dials direct. ENG-4082 --- authclient/authclient.go | 24 +++- authclient/authclient_test.go | 32 +++++ authclient/config.go | 9 ++ cmd/pomerium-cli/tcp.go | 8 +- internal/httputil/fetch.go | 26 +++- internal/httputil/proxy.go | 190 +++++++++++++++++++++++++++ internal/httputil/proxy_test.go | 219 ++++++++++++++++++++++++++++++++ internal/testutil/proxy.go | 195 ++++++++++++++++++++++++++++ tunnel/config.go | 10 ++ tunnel/dial.go | 68 ++++++++++ tunnel/tunnel.go | 3 +- tunnel/tunnel_http1.go | 25 ++-- tunnel/tunnel_http2.go | 11 +- tunnel/tunnel_proxy_test.go | 201 +++++++++++++++++++++++++++++ tunnel/tunnel_tcp.go | 9 ++ tunnel/tunnel_udp_test.go | 66 ++++++++++ 16 files changed, 1068 insertions(+), 28 deletions(-) create mode 100644 internal/httputil/proxy.go create mode 100644 internal/httputil/proxy_test.go create mode 100644 internal/testutil/proxy.go create mode 100644 tunnel/dial.go create mode 100644 tunnel/tunnel_proxy_test.go diff --git a/authclient/authclient.go b/authclient/authclient.go index 010eeaa9..4e09bf38 100644 --- a/authclient/authclient.go +++ b/authclient/authclient.go @@ -42,10 +42,26 @@ func (client *AuthClient) CheckBearerToken(ctx context.Context, serverURL *url.U req.Header.Set("Accept", "application/json") req.Header.Set("Authorization", "Bearer "+bearerToken) - _, err = httputil.Fetch(ctx, client.cfg.tlsConfig, req) + opts, err := client.fetchOpts(serverURL) + if err != nil { + return err + } + _, err = httputil.Fetch(ctx, client.cfg.tlsConfig, req, opts...) return err } +// fetchOpts resolves the forward proxy for serverURL, if any. +func (client *AuthClient) fetchOpts(serverURL *url.URL) ([]httputil.FetchOption, error) { + proxyURL, err := httputil.ResolveProxy(client.cfg.forwardProxy, serverURL) + if err != nil { + return nil, err + } + if proxyURL == nil { + return nil, nil + } + return []httputil.FetchOption{httputil.WithProxyURL(proxyURL)}, nil +} + // GetJWT retrieves a JWT from Pomerium. func (client *AuthClient) GetJWT(ctx context.Context, serverURL *url.URL, onOpenBrowser func(string)) (rawJWT string, err error) { if client.cfg.serviceAccount != "" { @@ -136,7 +152,11 @@ func (client *AuthClient) runOpenBrowser(ctx context.Context, li net.Listener, s return err } - bs, err := httputil.Fetch(ctx, client.cfg.tlsConfig, req) + opts, err := client.fetchOpts(serverURL) + if err != nil { + return err + } + bs, err := httputil.Fetch(ctx, client.cfg.tlsConfig, req, opts...) if err != nil { return err } diff --git a/authclient/authclient_test.go b/authclient/authclient_test.go index ee82d7c5..d02211dc 100644 --- a/authclient/authclient_test.go +++ b/authclient/authclient_test.go @@ -2,8 +2,11 @@ package authclient import ( "context" + "crypto/tls" + "crypto/x509" "net" "net/http" + "net/http/httptest" "net/url" "os" "path/filepath" @@ -13,8 +16,37 @@ import ( "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/pomerium/cli/internal/testutil" ) +// TestCheckBearerTokenViaProxy verifies the auth path routes through a forward +// proxy. Auth uses an HTTPS server so http.Transport issues a CONNECT. +func TestCheckBearerTokenViaProxy(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + pool := x509.NewCertPool() + pool.AddCert(srv.Certificate()) + + proxy := testutil.NewConnectProxy(t) + + ac := New(WithForwardProxy(proxy.Addr)) + ac.cfg.tlsConfig = &tls.Config{RootCAs: pool} + + serverURL, err := url.Parse(srv.URL) + require.NoError(t, err) + + require.NoError(t, ac.CheckBearerToken(ctx, serverURL, "token")) + assert.Positive(t, proxy.Conns(), "auth request should route through the forward proxy") + assert.Equal(t, serverURL.Host, proxy.Target()) +} + func TestAuthClient(t *testing.T) { t.Parallel() diff --git a/authclient/config.go b/authclient/config.go index e349c3af..9180a8bc 100644 --- a/authclient/config.go +++ b/authclient/config.go @@ -11,6 +11,7 @@ type config struct { serviceAccount string serviceAccountFile string tlsConfig *tls.Config + forwardProxy string } func getConfig(options ...Option) *config { @@ -38,6 +39,14 @@ func WithBrowserCommand(browserCommand string) Option { } } +// WithForwardProxy sets an explicit forward proxy override (host:port or URL). +// An empty value falls back to the standard proxy environment variables. +func WithForwardProxy(forwardProxy string) Option { + return func(cfg *config) { + cfg.forwardProxy = forwardProxy + } +} + // WithServiceAccount sets the service account in the config. func WithServiceAccount(serviceAccount string) Option { return func(cfg *config) { diff --git a/cmd/pomerium-cli/tcp.go b/cmd/pomerium-cli/tcp.go index 243f7e82..dd8f7015 100644 --- a/cmd/pomerium-cli/tcp.go +++ b/cmd/pomerium-cli/tcp.go @@ -15,8 +15,9 @@ import ( ) var tcpCmdOptions struct { - listen string - pomeriumURL string + listen string + pomeriumURL string + forwardProxy string } func init() { @@ -28,6 +29,8 @@ func init() { "local address to start a listener on") flags.StringVar(&tcpCmdOptions.pomeriumURL, "pomerium-url", "", "the URL of the pomerium server to connect to") + flags.StringVar(&tcpCmdOptions.forwardProxy, "forward-proxy", "", + "HTTP CONNECT or SOCKS5 forward proxy for the tunnel (host:port or URL); authoritative and ignores NO_PROXY. Without it, HTTP_PROXY/HTTPS_PROXY (honoring NO_PROXY) and ALL_PROXY apply.") rootCmd.AddCommand(tcpCmd) } @@ -65,6 +68,7 @@ var tcpCmd = &cobra.Command{ tunnel.WithServiceAccount(serviceAccountOptions.serviceAccount), tunnel.WithServiceAccountFile(serviceAccountOptions.serviceAccountFile), tunnel.WithTLSConfig(tlsConfig), + tunnel.WithForwardProxy(tcpCmdOptions.forwardProxy), ) if tcpCmdOptions.listen == "-" { diff --git a/internal/httputil/fetch.go b/internal/httputil/fetch.go index 1cadc9e2..adc4bf54 100644 --- a/internal/httputil/fetch.go +++ b/internal/httputil/fetch.go @@ -7,20 +7,44 @@ import ( "fmt" "io" "net/http" + "net/url" "time" ) // ErrUnauthenticated indicates the user needs to authenticate. var ErrUnauthenticated = errors.New("unauthenticated") +type fetchConfig struct { + proxyURL *url.URL +} + +// A FetchOption modifies the behavior of Fetch. +type FetchOption func(*fetchConfig) + +// WithProxyURL routes the request through the given forward proxy. When unset, +// the default transport still honors HTTP_PROXY/HTTPS_PROXY. +func WithProxyURL(u *url.URL) FetchOption { + return func(c *fetchConfig) { + c.proxyURL = u + } +} + // Fetch fetches the http request. -func Fetch(ctx context.Context, tlsConfig *tls.Config, req *http.Request) ([]byte, error) { +func Fetch(ctx context.Context, tlsConfig *tls.Config, req *http.Request, opts ...FetchOption) ([]byte, error) { + var cfg fetchConfig + for _, o := range opts { + o(&cfg) + } + ctx, clearTimeout := context.WithTimeout(ctx, 10*time.Second) defer clearTimeout() req = req.WithContext(ctx) transport := http.DefaultTransport.(*http.Transport).Clone() transport.TLSClientConfig = tlsConfig + if cfg.proxyURL != nil { + transport.Proxy = func(*http.Request) (*url.URL, error) { return cfg.proxyURL, nil } + } hc := &http.Client{ Transport: transport, } diff --git a/internal/httputil/proxy.go b/internal/httputil/proxy.go new file mode 100644 index 00000000..535bbed2 --- /dev/null +++ b/internal/httputil/proxy.go @@ -0,0 +1,190 @@ +package httputil + +import ( + "bufio" + "context" + "crypto/tls" + "encoding/base64" + "fmt" + "net" + "net/http" + "net/url" + "os" + "strings" + "time" + + "golang.org/x/net/http/httpproxy" + "golang.org/x/net/proxy" +) + +// ResolveProxy determines the forward proxy to use to reach edge, if any. +// +// Precedence: an explicit override (authoritative, ignores NO_PROXY), then +// HTTP_PROXY/HTTPS_PROXY honoring NO_PROXY (scheme-aware), then ALL_PROXY as a +// fallback. It returns (nil, nil) when no proxy applies. +func ResolveProxy(override string, edge *url.URL) (*url.URL, error) { + if override = strings.TrimSpace(override); override != "" { + return normalizeForwardProxy(override) + } + + if u, err := httpproxy.FromEnvironment().ProxyFunc()(edge); err != nil { + return nil, err + } else if u != nil { + return u, nil + } + + all := os.Getenv("ALL_PROXY") + if all == "" { + all = os.Getenv("all_proxy") + } + if all == "" { + return nil, nil + } + noProxy := os.Getenv("NO_PROXY") + if noProxy == "" { + noProxy = os.Getenv("no_proxy") + } + return (&httpproxy.Config{HTTPProxy: all, HTTPSProxy: all, NoProxy: noProxy}).ProxyFunc()(edge) +} + +// normalizeForwardProxy validates and normalizes an explicit --forward-proxy +// override: a bare host:port defaults to http, the scheme must be one we can +// dial, and a host is required. Error messages use Redacted so any embedded +// credentials never leak. +func normalizeForwardProxy(override string) (*url.URL, error) { + if !strings.Contains(override, "://") { + override = "http://" + override + } + u, err := url.Parse(override) + if err != nil { + // the raw string may carry credentials, so don't echo it. + return nil, fmt.Errorf("invalid forward proxy: %w", err) + } + switch u.Scheme { + case "http", "https", "socks5", "socks5h": + default: + return nil, fmt.Errorf("unsupported forward proxy scheme %q (want http, https, socks5, or socks5h)", u.Scheme) + } + if u.Hostname() == "" { + return nil, fmt.Errorf("forward proxy %q has no host", u.Redacted()) + } + if u.Path != "" || u.RawQuery != "" || u.Fragment != "" { + return nil, fmt.Errorf("forward proxy %q must not have a path, query, or fragment", u.Redacted()) + } + return u, nil +} + +// proxyDialTimeout bounds the proxy connect and CONNECT exchange when the +// caller's context carries no deadline, so a dead proxy fails fast. +const proxyDialTimeout = 30 * time.Second + +// DialThroughProxy dials target (host:port) through the given forward proxy. +func DialThroughProxy(ctx context.Context, proxyURL *url.URL, target string) (net.Conn, error) { + if _, ok := ctx.Deadline(); !ok { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, proxyDialTimeout) + defer cancel() + } + switch proxyURL.Scheme { + case "socks5", "socks5h": + d, err := proxy.FromURL(proxyURL, proxy.Direct) + if err != nil { + return nil, fmt.Errorf("failed to create socks5 dialer: %w", err) + } + if cd, ok := d.(proxy.ContextDialer); ok { + return cd.DialContext(ctx, "tcp", target) + } + return d.Dial("tcp", target) + case "http", "https", "": + return dialHTTPConnect(ctx, proxyURL, target) + default: + return nil, fmt.Errorf("unsupported proxy scheme: %q", proxyURL.Scheme) + } +} + +func dialHTTPConnect(ctx context.Context, proxyURL *url.URL, target string) (_ net.Conn, retErr error) { + proxyAddr := proxyURL.Host + if proxyURL.Port() == "" { + if proxyURL.Scheme == "https" { + proxyAddr = net.JoinHostPort(proxyURL.Hostname(), "443") + } else { + proxyAddr = net.JoinHostPort(proxyURL.Hostname(), "80") + } + } + + conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", proxyAddr) + if err != nil { + return nil, fmt.Errorf("failed to dial proxy: %w", err) + } + defer func() { + if retErr != nil { + _ = conn.Close() + } + }() + + if deadline, ok := ctx.Deadline(); ok { + if err := conn.SetDeadline(deadline); err != nil { + return nil, err + } + } + + if proxyURL.Scheme == "https" { + tc := tls.Client(conn, &tls.Config{ServerName: proxyURL.Hostname()}) + if err := tc.HandshakeContext(ctx); err != nil { + return nil, fmt.Errorf("forward proxy %s TLS handshake failed (only system-trusted CAs are supported): %w", proxyURL.Redacted(), err) + } + conn = tc + } + + // abort the CONNECT exchange on cancellation; stop() before a successful + // return so the deadline-cleared conn handed to the caller is never closed. + stop := context.AfterFunc(ctx, func() { _ = conn.Close() }) + defer stop() + + req := &http.Request{ + Method: http.MethodConnect, + URL: &url.URL{Opaque: target}, + Host: target, + Header: http.Header{}, + } + if proxyURL.User != nil { + user := proxyURL.User.Username() + pass, _ := proxyURL.User.Password() + auth := base64.StdEncoding.EncodeToString([]byte(user + ":" + pass)) + req.Header.Set("Proxy-Authorization", "Basic "+auth) + } + if err := req.Write(conn); err != nil { + return nil, fmt.Errorf("failed to write CONNECT request: %w", err) + } + + br := bufio.NewReader(conn) + res, err := http.ReadResponse(br, req) + if err != nil { + return nil, fmt.Errorf("failed to read CONNECT response: %w", err) + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return nil, fmt.Errorf("proxy CONNECT failed: %s", res.Status) + } + + if err := conn.SetDeadline(time.Time{}); err != nil { + return nil, err + } + + if br.Buffered() > 0 { + return &bufferedConn{Conn: conn, br: br}, nil + } + return conn, nil +} + +// bufferedConn drains bytes the CONNECT response parser buffered before reading +// from the underlying connection. The bufio.Reader transparently falls through +// to the underlying conn once its buffer is exhausted. +type bufferedConn struct { + net.Conn + br *bufio.Reader +} + +func (c *bufferedConn) Read(p []byte) (int, error) { + return c.br.Read(p) +} diff --git a/internal/httputil/proxy_test.go b/internal/httputil/proxy_test.go new file mode 100644 index 00000000..e018e69a --- /dev/null +++ b/internal/httputil/proxy_test.go @@ -0,0 +1,219 @@ +package httputil + +import ( + "bufio" + "context" + "io" + "net" + "net/url" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/pomerium/cli/internal/testutil" +) + +func TestResolveProxy(t *testing.T) { + httpsEdge := &url.URL{Scheme: "https", Host: "edge.example.com:443"} + httpEdge := &url.URL{Scheme: "http", Host: "edge.example.com:80"} + + for _, tc := range []struct { + name string + override string + env map[string]string + edge *url.URL + want string + wantErr bool + }{ + {name: "override url", override: "http://proxy:8080", edge: httpsEdge, want: "http://proxy:8080"}, + {name: "override bare host:port", override: "proxy:8080", edge: httpsEdge, want: "http://proxy:8080"}, + {name: "override socks5", override: "socks5://proxy:1080", edge: httpsEdge, want: "socks5://proxy:1080"}, + {name: "override socks5h", override: "socks5h://proxy:1080", edge: httpsEdge, want: "socks5h://proxy:1080"}, + {name: "override trims whitespace", override: " http://proxy:8080 ", edge: httpsEdge, want: "http://proxy:8080"}, + {name: "https_proxy", env: map[string]string{"HTTPS_PROXY": "http://hp:3128"}, edge: httpsEdge, want: "http://hp:3128"}, + {name: "http_proxy", env: map[string]string{"HTTP_PROXY": "http://hp:3128"}, edge: httpEdge, want: "http://hp:3128"}, + {name: "no_proxy", env: map[string]string{"HTTPS_PROXY": "http://hp:3128", "NO_PROXY": "edge.example.com"}, edge: httpsEdge, want: ""}, + {name: "all_proxy socks5", env: map[string]string{"ALL_PROXY": "socks5://sp:1080"}, edge: httpsEdge, want: "socks5://sp:1080"}, + {name: "all_proxy honors no_proxy", env: map[string]string{"ALL_PROXY": "socks5://sp:1080", "NO_PROXY": "edge.example.com"}, edge: httpsEdge, want: ""}, + {name: "override beats env", override: "http://override:9", env: map[string]string{"HTTPS_PROXY": "http://hp:3128"}, edge: httpsEdge, want: "http://override:9"}, + {name: "whitespace override is unset", override: " ", env: map[string]string{"HTTPS_PROXY": "http://hp:3128"}, edge: httpsEdge, want: "http://hp:3128"}, + {name: "none", edge: httpsEdge, want: ""}, + {name: "bad scheme", override: "socks4://p:1", edge: httpsEdge, wantErr: true}, + {name: "path rejected", override: "http://p:8080/path", edge: httpsEdge, wantErr: true}, + {name: "query rejected", override: "http://p:8080?q=1", edge: httpsEdge, wantErr: true}, + {name: "no host", override: "http://", edge: httpsEdge, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "no_proxy", "all_proxy"} { + t.Setenv(k, "") + } + for k, v := range tc.env { + t.Setenv(k, v) + } + + got, err := ResolveProxy(tc.override, tc.edge) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + if tc.want == "" { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tc.want, got.String()) + }) + } +} + +func TestResolveProxyRedactsCredentials(t *testing.T) { + _, err := ResolveProxy("http://user:hunter2@proxy:8080/path", &url.URL{Scheme: "https", Host: "edge:443"}) + require.Error(t, err) + assert.NotContains(t, err.Error(), "hunter2") +} + +func TestDialThroughProxyHTTPConnect(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + backend := echoListener(t) + proxy := testutil.NewConnectProxy(t) + + conn, err := DialThroughProxy(ctx, &url.URL{Scheme: "http", Host: proxy.Addr}, backend) + require.NoError(t, err) + defer conn.Close() + + assert.Equal(t, "ping\n", roundTrip(t, conn)) + assert.Equal(t, backend, proxy.Target()) +} + +func TestDialThroughProxySOCKS5(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + backend := echoListener(t) + proxy := testutil.NewSOCKS5Proxy(t) + + conn, err := DialThroughProxy(ctx, &url.URL{Scheme: "socks5", Host: proxy.Addr}, backend) + require.NoError(t, err) + defer conn.Close() + + assert.Equal(t, "ping\n", roundTrip(t, conn)) + assert.Equal(t, backend, proxy.Target()) +} + +func TestDialThroughProxyUnsupportedScheme(t *testing.T) { + _, err := DialThroughProxy(context.Background(), &url.URL{Scheme: "ftp", Host: "p:21"}, "edge:443") + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported proxy scheme") +} + +func TestDialThroughProxyConnectRejected(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + proxyLi, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer proxyLi.Close() + go func() { + conn, err := proxyLi.Accept() + if err != nil { + return + } + defer conn.Close() + br := bufio.NewReader(conn) + for { + h, err := br.ReadString('\n') + if err != nil || h == "\r\n" || h == "\n" { + break + } + } + _, _ = io.WriteString(conn, "HTTP/1.1 403 Forbidden\r\n\r\n") + }() + + _, err = DialThroughProxy(ctx, &url.URL{Scheme: "http", Host: proxyLi.Addr().String()}, "edge.example.com:443") + require.Error(t, err) + assert.Contains(t, err.Error(), "403") +} + +// TestDialThroughProxyHTTPSProxyError verifies that a TLS failure to an https +// proxy reports the system-trust limitation without leaking credentials. The +// listener speaks plaintext, so the handshake fails. +func TestDialThroughProxyHTTPSProxyError(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + li, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer li.Close() + go func() { + if c, err := li.Accept(); err == nil { + _ = c.Close() + } + }() + + _, err = DialThroughProxy(ctx, &url.URL{Scheme: "https", User: url.UserPassword("u", "hunter2"), Host: li.Addr().String()}, "edge:443") + require.Error(t, err) + assert.Contains(t, err.Error(), "system-trusted") + assert.NotContains(t, err.Error(), "hunter2") +} + +// TestDialThroughProxyContextCancel verifies a stalled CONNECT exchange unblocks +// promptly when the context is canceled (via the AfterFunc guard). +func TestDialThroughProxyContextCancel(t *testing.T) { + li, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer li.Close() + go func() { + conn, err := li.Accept() + if err != nil { + return + } + defer conn.Close() + _, _ = io.Copy(io.Discard, conn) // read forever, never reply + }() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { time.Sleep(100 * time.Millisecond); cancel() }() + + done := make(chan error, 1) + go func() { + _, err := DialThroughProxy(ctx, &url.URL{Scheme: "http", Host: li.Addr().String()}, "edge:443") + done <- err + }() + + select { + case err := <-done: + require.Error(t, err) + case <-time.After(5 * time.Second): + t.Fatal("DialThroughProxy did not return after context cancellation") + } +} + +func echoListener(t *testing.T) string { + t.Helper() + li, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = li.Close() }) + go func() { + conn, err := li.Accept() + if err != nil { + return + } + defer conn.Close() + _, _ = io.Copy(conn, conn) + }() + return li.Addr().String() +} + +func roundTrip(t *testing.T, conn net.Conn) string { + t.Helper() + _, err := io.WriteString(conn, "ping\n") + require.NoError(t, err) + got, err := bufio.NewReader(conn).ReadString('\n') + require.NoError(t, err) + return got +} diff --git a/internal/testutil/proxy.go b/internal/testutil/proxy.go new file mode 100644 index 00000000..c5a42548 --- /dev/null +++ b/internal/testutil/proxy.go @@ -0,0 +1,195 @@ +package testutil + +import ( + "bufio" + "encoding/binary" + "io" + "net" + "strconv" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" +) + +// RecordingProxy is a fake forward proxy for tests. It records the connections +// it accepts, the CONNECT/SOCKS5 target it was asked to reach, and any +// Proxy-Authorization header, then splices through to that target. +type RecordingProxy struct { + Addr string + upstream string // when set, splice here instead of the requested target + conns atomic.Int64 + target atomic.Value + auth atomic.Value +} + +// Conns returns how many connections the proxy accepted. +func (p *RecordingProxy) Conns() int64 { return p.conns.Load() } + +// Target returns the last target the proxy was asked to reach. +func (p *RecordingProxy) Target() string { s, _ := p.target.Load().(string); return s } + +// ProxyAuth returns the last Proxy-Authorization header value (CONNECT only). +func (p *RecordingProxy) ProxyAuth() string { s, _ := p.auth.Load().(string); return s } + +func listen(t *testing.T) (net.Listener, *RecordingProxy) { + t.Helper() + li, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = li.Close() }) + return li, &RecordingProxy{Addr: li.Addr().String()} +} + +// NewConnectProxy starts an HTTP CONNECT proxy that splices to the requested +// target. +func NewConnectProxy(t *testing.T) *RecordingProxy { + return newConnectProxy(t, "") +} + +// NewConnectProxyTo starts an HTTP CONNECT proxy that splices every CONNECT to +// upstream, ignoring the requested target (which is still recorded). Use this +// when the advertised edge host must be non-loopback so HTTP(S)_PROXY engages. +func NewConnectProxyTo(t *testing.T, upstream string) *RecordingProxy { + return newConnectProxy(t, upstream) +} + +func newConnectProxy(t *testing.T, upstream string) *RecordingProxy { + li, p := listen(t) + p.upstream = upstream + go func() { + for { + conn, err := li.Accept() + if err != nil { + return + } + go p.handleConnect(conn) + } + }() + return p +} + +func (p *RecordingProxy) handleConnect(conn net.Conn) { + defer conn.Close() + p.conns.Add(1) + + br := bufio.NewReader(conn) + line, err := br.ReadString('\n') + if err != nil { + return + } + if f := strings.Fields(line); len(f) >= 2 { + p.target.Store(f[1]) + } + for { + h, err := br.ReadString('\n') + if err != nil { + return + } + if strings.HasPrefix(strings.ToLower(h), "proxy-authorization:") { + _, val, _ := strings.Cut(h, ":") + p.auth.Store(strings.TrimSpace(val)) + } + if h == "\r\n" || h == "\n" { + break + } + } + _, _ = io.WriteString(conn, "HTTP/1.1 200 Connection established\r\n\r\n") + + dst := p.Target() + if p.upstream != "" { + dst = p.upstream + } + out, err := net.Dial("tcp", dst) + if err != nil { + return + } + defer out.Close() + splice(conn, br, out) +} + +// NewSOCKS5Proxy starts a no-auth SOCKS5 proxy that splices to the requested +// target. +func NewSOCKS5Proxy(t *testing.T) *RecordingProxy { + li, p := listen(t) + go func() { + for { + conn, err := li.Accept() + if err != nil { + return + } + go p.handleSOCKS5(conn) + } + }() + return p +} + +func (p *RecordingProxy) handleSOCKS5(conn net.Conn) { + defer conn.Close() + br := bufio.NewReader(conn) + + // greeting: VER, NMETHODS, METHODS... + greeting := make([]byte, 2) + if _, err := io.ReadFull(br, greeting); err != nil { + return + } + if _, err := io.ReadFull(br, make([]byte, int(greeting[1]))); err != nil { + return + } + if _, err := conn.Write([]byte{0x05, 0x00}); err != nil { // no auth + return + } + + // request: VER CMD RSV ATYP DST.ADDR DST.PORT + req := make([]byte, 4) + if _, err := io.ReadFull(br, req); err != nil { + return + } + var host string + switch req[3] { + case 0x01: // IPv4 + b := make([]byte, 4) + if _, err := io.ReadFull(br, b); err != nil { + return + } + host = net.IP(b).String() + case 0x03: // domain + l := make([]byte, 1) + if _, err := io.ReadFull(br, l); err != nil { + return + } + b := make([]byte, int(l[0])) + if _, err := io.ReadFull(br, b); err != nil { + return + } + host = string(b) + default: + return + } + portBytes := make([]byte, 2) + if _, err := io.ReadFull(br, portBytes); err != nil { + return + } + target := net.JoinHostPort(host, strconv.Itoa(int(binary.BigEndian.Uint16(portBytes)))) + p.conns.Add(1) + p.target.Store(target) + + // reply success with a dummy bound address (0.0.0.0:0). + if _, err := conn.Write([]byte{0x05, 0x00, 0x00, 0x01, 0, 0, 0, 0, 0, 0}); err != nil { + return + } + + out, err := net.Dial("tcp", target) + if err != nil { + return + } + defer out.Close() + splice(conn, br, out) +} + +func splice(client net.Conn, clientReader io.Reader, upstream net.Conn) { + errc := make(chan error, 2) + go func() { _, e := io.Copy(upstream, clientReader); errc <- e }() + go func() { _, e := io.Copy(client, upstream); errc <- e }() + <-errc +} diff --git a/tunnel/config.go b/tunnel/config.go index 8fb67ac9..06eb46dc 100644 --- a/tunnel/config.go +++ b/tunnel/config.go @@ -14,6 +14,7 @@ type config struct { serviceAccountFile string tlsConfig *tls.Config browserConfig string + forwardProxy string } func getConfig(options ...Option) *config { @@ -42,6 +43,15 @@ func WithDestinationHost(dstHost string) Option { } } +// WithForwardProxy returns an option to configure the forward proxy used to +// reach the Pomerium edge (host:port or URL). An empty value falls back to the +// standard proxy environment variables. +func WithForwardProxy(forwardProxy string) Option { + return func(cfg *config) { + cfg.forwardProxy = forwardProxy + } +} + // WithJWTCache returns an option to configure the jwt cache. func WithJWTCache(jwtCache jwt.Cache) Option { return func(cfg *config) { diff --git a/tunnel/dial.go b/tunnel/dial.go new file mode 100644 index 00000000..5172891e --- /dev/null +++ b/tunnel/dial.go @@ -0,0 +1,68 @@ +package tunnel + +import ( + "context" + "crypto/tls" + "net" + "net/url" + + "github.com/pomerium/cli/internal/httputil" +) + +// edgeURL builds the URL of the Pomerium edge for proxy resolution. +func edgeURL(cfg *config) *url.URL { + scheme := "http" + if cfg.tlsConfig != nil { + scheme = "https" + } + return &url.URL{Scheme: scheme, Host: cfg.proxyHost} +} + +// resolveEdgeProxy resolves the forward proxy (if any) for reaching the edge. +// Forward proxying is TCP-only; UDP callers must not use it. +func resolveEdgeProxy(cfg *config) (*url.URL, error) { + return httputil.ResolveProxy(cfg.forwardProxy, edgeURL(cfg)) +} + +// dialEdgeTLS establishes a connection to the Pomerium edge, routing through +// proxyURL when non-nil, and TLS-wraps it when a tls config is configured. +// +// A nil nextProtos leaves the configured ALPN untouched (preserving the +// behavior of the original tls.Dialer for callers that don't pin a protocol). +func dialEdgeTLS(ctx context.Context, cfg *config, proxyURL *url.URL, nextProtos []string) (net.Conn, error) { + var raw net.Conn + var err error + if proxyURL == nil { + raw, err = (&net.Dialer{}).DialContext(ctx, "tcp", cfg.proxyHost) + } else { + raw, err = httputil.DialThroughProxy(ctx, proxyURL, cfg.proxyHost) + } + if err != nil { + return nil, err + } + + if cfg.tlsConfig == nil { + return raw, nil + } + + tlsCfg := cfg.tlsConfig.Clone() + if nextProtos != nil { + tlsCfg.NextProtos = nextProtos + } + // the old tls.Dialer derived ServerName from the dial address, so we must + // set it explicitly here for certificate verification to succeed. + if tlsCfg.ServerName == "" { + host, _, err := net.SplitHostPort(cfg.proxyHost) + if err != nil { + host = cfg.proxyHost + } + tlsCfg.ServerName = host + } + + tc := tls.Client(raw, tlsCfg) + if err := tc.HandshakeContext(ctx); err != nil { + _ = raw.Close() + return nil, err + } + return tc, nil +} diff --git a/tunnel/tunnel.go b/tunnel/tunnel.go index 099f0d15..6d9934ea 100644 --- a/tunnel/tunnel.go +++ b/tunnel/tunnel.go @@ -44,7 +44,8 @@ func New(options ...Option) *Tunnel { authclient.WithBrowserCommand(cfg.browserConfig), authclient.WithServiceAccount(cfg.serviceAccount), authclient.WithServiceAccountFile(cfg.serviceAccountFile), - authclient.WithTLSConfig(cfg.tlsConfig)), + authclient.WithTLSConfig(cfg.tlsConfig), + authclient.WithForwardProxy(cfg.forwardProxy)), } } diff --git a/tunnel/tunnel_http1.go b/tunnel/tunnel_http1.go index cb89ef68..4a98003f 100644 --- a/tunnel/tunnel_http1.go +++ b/tunnel/tunnel_http1.go @@ -3,7 +3,6 @@ package tunnel import ( "bufio" "context" - "crypto/tls" "fmt" "io" "net" @@ -44,16 +43,11 @@ func (t *http1tunneler) TunnelTCP( Header: hdr, }).WithContext(ctx) - var remote net.Conn - var err error - if t.cfg.tlsConfig != nil { - cfg := t.cfg.tlsConfig.Clone() - cfg.NextProtos = []string{"http/1.1"} - - remote, err = (&tls.Dialer{Config: cfg}).DialContext(ctx, "tcp", t.cfg.proxyHost) - } else { - remote, err = (&net.Dialer{}).DialContext(ctx, "tcp", t.cfg.proxyHost) + proxyURL, err := resolveEdgeProxy(t.cfg) + if err != nil { + return fmt.Errorf("http/1: failed to resolve forward proxy: %w", err) } + remote, err := dialEdgeTLS(ctx, t.cfg, proxyURL, []string{"http/1.1"}) if err != nil { return fmt.Errorf("http/1: failed to establish connection to proxy: %w", err) } @@ -114,13 +108,10 @@ func (t *http1tunneler) TunnelUDP( ) error { eventSink.OnConnecting(ctx) - var remote net.Conn - var err error - if t.cfg.tlsConfig != nil { - remote, err = (&tls.Dialer{Config: t.cfg.tlsConfig}).DialContext(ctx, "tcp", t.cfg.proxyHost) - } else { - remote, err = (&net.Dialer{}).DialContext(ctx, "tcp", t.cfg.proxyHost) - } + // UDP never traverses a forward proxy (TCP-only feature), so dial direct + // (nil proxyURL). nil nextProtos preserves the original UDP/MASQUE handshake, + // which did not pin ALPN (only the TCP CONNECT path negotiates http/1.1). + remote, err := dialEdgeTLS(ctx, t.cfg, nil, nil) if err != nil { return fmt.Errorf("http/1: failed to establish connection to proxy: %w", err) } diff --git a/tunnel/tunnel_http2.go b/tunnel/tunnel_http2.go index 986699bb..c11722d3 100644 --- a/tunnel/tunnel_http2.go +++ b/tunnel/tunnel_http2.go @@ -35,10 +35,11 @@ func (t *http2tunneler) TunnelTCP( return fmt.Errorf("%w: http2 requires TLS", errUnsupported) } - cfg := t.cfg.tlsConfig.Clone() - cfg.NextProtos = []string{"h2"} - - raw, err := (&tls.Dialer{Config: cfg}).DialContext(ctx, "tcp", t.cfg.proxyHost) + proxyURL, err := resolveEdgeProxy(t.cfg) + if err != nil { + return fmt.Errorf("http/2: failed to resolve forward proxy: %w", err) + } + raw, err := dialEdgeTLS(ctx, t.cfg, proxyURL, []string{"h2"}) if err != nil { return fmt.Errorf("http/2: failed to establish connection to proxy: %w", err) } @@ -46,7 +47,7 @@ func (t *http2tunneler) TunnelTCP( remote, ok := raw.(*tls.Conn) if !ok { - return fmt.Errorf("http/2: unexpected connection type returned from dial: %T", raw) + return fmt.Errorf("%w: unexpected connection type returned from dial: %T", errUnsupported, raw) } protocol := remote.ConnectionState().NegotiatedProtocol diff --git a/tunnel/tunnel_proxy_test.go b/tunnel/tunnel_proxy_test.go new file mode 100644 index 00000000..36f8f3f1 --- /dev/null +++ b/tunnel/tunnel_proxy_test.go @@ -0,0 +1,201 @@ +package tunnel + +import ( + "bufio" + "bytes" + "context" + "encoding/base64" + "io" + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/pomerium/cli/internal/testutil" +) + +// fakeEdge starts a backend plus a Pomerium-edge server that terminates the +// tunnel CONNECT and splices to the backend. It returns the edge address and a +// channel that receives the first line the backend reads from the tunnel. +func fakeEdge(t *testing.T) (edgeAddr string, got <-chan string) { + t.Helper() + gotc := make(chan string, 1) + + backend, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = backend.Close() }) + go func() { + for { + conn, err := backend.Accept() + if err != nil { + return + } + go func() { + defer conn.Close() + ln, _, _ := bufio.NewReader(conn).ReadLine() + select { + case gotc <- string(ln): + default: + } + }() + } + }() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !assert.Equal(t, "CONNECT", r.Method) { + return + } + w.WriteHeader(200) + in, brw, err := w.(http.Hijacker).Hijack() + if !assert.NoError(t, err) { + return + } + defer in.Close() + out, err := net.Dial("tcp", backend.Addr().String()) + if !assert.NoError(t, err) { + return + } + defer out.Close() + errc := make(chan error, 2) + go func() { _, e := io.Copy(in, out); errc <- e }() + go func() { _, e := io.Copy(out, deBuffer(brw.Reader, in)); errc <- e }() + <-errc + })) + t.Cleanup(srv.Close) + + return srv.Listener.Addr().String(), gotc +} + +func clearProxyEnv(t *testing.T) { + t.Helper() + for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "no_proxy", "all_proxy"} { + t.Setenv(k, "") + } +} + +// runTCPTunnel drives one "HELLO WORLD" line through the tunnel and returns what +// the backend received. +func runTCPTunnel(t *testing.T, got <-chan string, opts ...Option) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + var buf bytes.Buffer + err := New(opts...).Run(ctx, readWriter{strings.NewReader("HELLO WORLD\n"), &buf}, DiscardEvents()) + require.NoError(t, err) + + select { + case line := <-got: + assert.Equal(t, "HELLO WORLD", line) + case <-time.After(5 * time.Second): + t.Fatal("backend never received the tunneled data") + } +} + +func TestTunnelViaForwardProxyFlag(t *testing.T) { + clearProxyEnv(t) + edge, got := fakeEdge(t) + proxy := testutil.NewConnectProxy(t) + + runTCPTunnel(t, got, + WithDestinationHost("example.com:9999"), + WithProxyHost(edge), + WithForwardProxy(proxy.Addr)) + + assert.Equal(t, edge, proxy.Target()) +} + +// Env-var proxies never apply to loopback edges (Go's httpproxy bypasses +// localhost), so these tests advertise a non-loopback edge name and use a proxy +// that splices to the real loopback edge. NO_PROXY and override precedence at +// the resolution layer are covered by TestResolveProxy. +func TestTunnelHonorsEnvProxy(t *testing.T) { + clearProxyEnv(t) + edge, got := fakeEdge(t) + proxy := testutil.NewConnectProxyTo(t, edge) + t.Setenv("HTTP_PROXY", "http://"+proxy.Addr) + + runTCPTunnel(t, got, + WithDestinationHost("example.com:9999"), + WithProxyHost("edge.example.com:443")) + + assert.Equal(t, "edge.example.com:443", proxy.Target(), "tunnel should route through HTTP_PROXY") +} + +func TestTunnelFlagOverridesEnv(t *testing.T) { + clearProxyEnv(t) + edge, got := fakeEdge(t) + envProxy := testutil.NewConnectProxyTo(t, edge) + flagProxy := testutil.NewConnectProxyTo(t, edge) + t.Setenv("HTTP_PROXY", "http://"+envProxy.Addr) + + runTCPTunnel(t, got, + WithDestinationHost("example.com:9999"), + WithProxyHost("edge.example.com:443"), + WithForwardProxy(flagProxy.Addr)) + + assert.Positive(t, flagProxy.Conns(), "explicit flag proxy should be used") + assert.Zero(t, envProxy.Conns(), "env proxy should be ignored when the flag is set") +} + +func TestTunnelViaSOCKS5(t *testing.T) { + clearProxyEnv(t) + edge, got := fakeEdge(t) + proxy := testutil.NewSOCKS5Proxy(t) + + runTCPTunnel(t, got, + WithDestinationHost("example.com:9999"), + WithProxyHost(edge), + WithForwardProxy("socks5://"+proxy.Addr)) + + assert.Equal(t, edge, proxy.Target()) +} + +func TestTunnelUnsupportedProxyScheme(t *testing.T) { + clearProxyEnv(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var buf bytes.Buffer + err := New( + WithDestinationHost("example.com:9999"), + WithProxyHost("edge.example.com:443"), + WithForwardProxy("ftp://proxy:21"), + ).Run(ctx, readWriter{strings.NewReader("HELLO WORLD\n"), &buf}, DiscardEvents()) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported forward proxy scheme") +} + +func TestTunnelProxyCredentialsNotLogged(t *testing.T) { + clearProxyEnv(t) + edge, got := fakeEdge(t) + proxy := testutil.NewConnectProxy(t) + + var logBuf bytes.Buffer + ctx := zerolog.New(&logBuf).WithContext(context.Background()) + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + var buf bytes.Buffer + err := New( + WithDestinationHost("example.com:9999"), + WithProxyHost(edge), + WithForwardProxy("http://user:s3cret@"+proxy.Addr), + ).Run(ctx, readWriter{strings.NewReader("HELLO WORLD\n"), &buf}, DiscardEvents()) + require.NoError(t, err) + + select { + case <-got: + case <-time.After(5 * time.Second): + t.Fatal("backend never received the tunneled data") + } + + assert.Equal(t, "Basic "+base64.StdEncoding.EncodeToString([]byte("user:s3cret")), proxy.ProxyAuth()) + assert.NotContains(t, logBuf.String(), "s3cret", "credentials must never be logged") +} diff --git a/tunnel/tunnel_tcp.go b/tunnel/tunnel_tcp.go index 9aef8c51..bc864997 100644 --- a/tunnel/tunnel_tcp.go +++ b/tunnel/tunnel_tcp.go @@ -34,6 +34,15 @@ func (tun *Tunnel) pickTCPTunneler(ctx context.Context) TCPTunneler { return fallback } + // a forward proxy can't traverse QUIC, so skip the http3 probe entirely. + if proxyURL, err := resolveEdgeProxy(tun.cfg); err != nil { + log.Ctx(ctx).Error().Err(err).Msg("failed to resolve forward proxy, using http1") + return fallback + } else if proxyURL != nil { + log.Ctx(ctx).Info().Msg("forward proxy configured, using http1") + return fallback + } + client := &http.Client{ Transport: httputil.NewLoggingRoundTripper(log.Logger, &http.Transport{ ForceAttemptHTTP2: true, diff --git a/tunnel/tunnel_udp_test.go b/tunnel/tunnel_udp_test.go index 426aec18..f3afe412 100644 --- a/tunnel/tunnel_udp_test.go +++ b/tunnel/tunnel_udp_test.go @@ -97,3 +97,69 @@ func TestUDPSessionManager(t *testing.T) { } assert.NoError(t, err, "tunnel should shutdown cleanly") } + +// TestTunnelUDPIgnoresForwardProxy proves the forward proxy is TCP-only: with +// HTTP_PROXY/HTTPS_PROXY set, a UDP tunnel still dials the edge directly and +// never touches the proxy. +func TestTunnelUDPIgnoresForwardProxy(t *testing.T) { + clearProxyEnv(t) + proxy := testutil.NewConnectProxy(t) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + tunnelPort := testutil.GetPort(t) + localPort := testutil.GetPort(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Transfer-Encoding", "identity") + w.WriteHeader(200) + w.(http.Flusher).Flush() + in, brw, err := w.(http.Hijacker).Hijack() + require.NoError(t, err) + defer in.Close() + payload, err := readUDPCapsuleDatagram(quicvarint.NewReader(in)) + require.NoError(t, err) + require.Equal(t, []byte("\x00SEND HELLO WORLD"), payload) + require.NoError(t, http3.WriteCapsule(quicvarint.NewWriter(brw), 0, []byte("\x00RECV HELLO WORLD"))) + require.NoError(t, brw.Flush()) + <-ctx.Done() + })) + defer srv.Close() + + // Explicit forward proxy (not loopback-bypassed like env vars). TunnelUDP + // must ignore it entirely, so the proxy should never be dialed. + tun := New( + WithDestinationHost("example.com:9999"), + WithProxyHost(srv.Listener.Addr().String()), + WithForwardProxy(proxy.Addr)) + + tunnelAddr, err := net.ResolveUDPAddr("udp", "127.0.0.1:"+tunnelPort) + require.NoError(t, err) + tunnelConn, err := net.ListenUDP("udp", tunnelAddr) + require.NoError(t, err) + + tunErrC := make(chan error, 1) + go func() { tunErrC <- tun.RunUDPSessionManager(ctx, tunnelConn, LogEvents()) }() + + localAddr, err := net.ResolveUDPAddr("udp", "127.0.0.1:"+localPort) + require.NoError(t, err) + conn, err := net.ListenUDP("udp", localAddr) + require.NoError(t, err) + context.AfterFunc(ctx, func() { _ = conn.Close() })() + + _, err = conn.WriteToUDP([]byte("SEND HELLO WORLD"), tunnelAddr) + require.NoError(t, err) + + payload := make([]byte, maxUDPPacketSize) + n, _, err := conn.ReadFromUDP(payload) + require.NoError(t, err) + assert.Equal(t, []byte("RECV HELLO WORLD"), payload[:n]) + + assert.Zero(t, proxy.Conns(), "UDP tunnel must not use a forward proxy") + + cancel() + if err := <-tunErrC; err != nil && !errors.Is(err, context.Canceled) { + t.Errorf("tunnel should shut down cleanly: %v", err) + } +} From 53ae7da26b20d8b30f0fc8c95560161684183cac Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Sat, 30 May 2026 14:02:06 -0700 Subject: [PATCH 2/4] fix(tcp): harden forward-proxy auth trust, validation, and leaks Follow-up hardening for the forward-proxy support added in this PR: - Validate an explicit --forward-proxy at startup, before binding the listener, via httputil.ValidateForwardProxyFlag. It is explicit-only and never consults the proxy environment, so a bad flag fails as a normal CLI error while env-derived proxies keep resolving at request time. - Route the auth-path Fetch through DialThroughProxy instead of transport.Proxy. An https proxy hop is now verified against the system trust store on both the tunnel and auth paths (previously the auth path validated the proxy cert against the edge CA, a trust split). The target's TLS still uses the edge tls config over the CONNECT tunnel. - Return an error from the dead SOCKS5 non-ContextDialer branch instead of silently dropping context cancellation. - Fix a per-connection goroutine leak in the http/1 tunneler: replace the parked ctx.Done waiter with context.AfterFunc + defer stop() in both TunnelTCP and TunnelUDP (UDP previously discarded the stop too). - Delete the dead http2tunneler: it had no Name() method, so it never satisfied TCPTunneler and could never be selected. Tests: explicit-flag validation (asserts env is not consulted), an https-proxy system-trust regression, an https target round-trip through a proxy with a single CONNECT hop, proxy Basic-auth credential lock-in, and a goroutine-leak assertion. make build/test/lint green; go test -race green on internal/httputil, tunnel, and authclient. Assisted-by: Claude (Opus 4.8); reviewed by Go-specialist + independent reviewer agents and the workspace review helper; verified with make build/test/lint and go test -race. ENG-4082 --- cmd/pomerium-cli/tcp.go | 15 +++ internal/httputil/fetch.go | 10 +- internal/httputil/fetch_test.go | 162 ++++++++++++++++++++++++++++++++ internal/httputil/proxy.go | 18 +++- internal/httputil/proxy_test.go | 23 +++++ tunnel/tunnel_http1.go | 11 +-- tunnel/tunnel_http1_test.go | 88 +++++++++++++++++ tunnel/tunnel_http2.go | 107 --------------------- tunnel/tunnel_http2_test.go | 70 -------------- 9 files changed, 316 insertions(+), 188 deletions(-) create mode 100644 internal/httputil/fetch_test.go create mode 100644 tunnel/tunnel_http1_test.go delete mode 100644 tunnel/tunnel_http2.go delete mode 100644 tunnel/tunnel_http2_test.go diff --git a/cmd/pomerium-cli/tcp.go b/cmd/pomerium-cli/tcp.go index dd8f7015..cd23cac9 100644 --- a/cmd/pomerium-cli/tcp.go +++ b/cmd/pomerium-cli/tcp.go @@ -9,8 +9,10 @@ import ( "os/signal" "syscall" + "github.com/rs/zerolog/log" "github.com/spf13/cobra" + "github.com/pomerium/cli/internal/httputil" "github.com/pomerium/cli/tunnel" ) @@ -45,6 +47,19 @@ var tcpCmd = &cobra.Command{ } cacheLastURL(proxyURL.String()) + // Validate an explicit --forward-proxy before binding the listener so a + // bad value fails as a normal CLI error. Environment proxies are + // resolved later, at request time. + fwdProxyURL, err := httputil.ValidateForwardProxyFlag(tcpCmdOptions.forwardProxy) + if err != nil { + return err + } + if fwdProxyURL != nil { + log.Info(). + Str("forward_proxy", fwdProxyURL.Redacted()). + Msg("routing tunnel and auth through forward proxy (--forward-proxy); HTTP/3 disabled") + } + var tlsConfig *tls.Config if proxyURL.Scheme == "https" { tlsConfig, err = getTLSConfig() diff --git a/internal/httputil/fetch.go b/internal/httputil/fetch.go index adc4bf54..225f0ce0 100644 --- a/internal/httputil/fetch.go +++ b/internal/httputil/fetch.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" "time" @@ -43,7 +44,14 @@ func Fetch(ctx context.Context, tlsConfig *tls.Config, req *http.Request, opts . transport := http.DefaultTransport.(*http.Transport).Clone() transport.TLSClientConfig = tlsConfig if cfg.proxyURL != nil { - transport.Proxy = func(*http.Request) (*url.URL, error) { return cfg.proxyURL, nil } + // Dial the proxy ourselves so the proxy hop's TLS is validated against + // system trust (matching the tunnel path), while the target's TLS still + // uses tlsConfig. Setting transport.Proxy would instead validate an https + // proxy against tlsConfig and re-read the proxy environment. + transport.Proxy = nil + transport.DialContext = func(ctx context.Context, _, addr string) (net.Conn, error) { + return DialThroughProxy(ctx, cfg.proxyURL, addr) + } } hc := &http.Client{ Transport: transport, diff --git a/internal/httputil/fetch_test.go b/internal/httputil/fetch_test.go new file mode 100644 index 00000000..39ac94d3 --- /dev/null +++ b/internal/httputil/fetch_test.go @@ -0,0 +1,162 @@ +package httputil + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/base64" + "math/big" + "net" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/pomerium/cli/internal/testutil" +) + +// TestFetchHTTPSTargetThroughProxy proves the auth path reaches an https target +// through a forward proxy with a single CONNECT hop, and that the target's TLS +// is still validated against the edge tlsConfig (over the tunnel) rather than +// skipped. It locks in the "no double-proxy, target TLS preserved" contract of +// the custom DialContext. +func TestFetchHTTPSTargetThroughProxy(t *testing.T) { + target := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("hello through proxy")) + })) + defer target.Close() + + pool := x509.NewCertPool() + pool.AddCert(target.Certificate()) + + proxy := testutil.NewConnectProxy(t) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + req, err := http.NewRequest(http.MethodGet, target.URL, nil) + require.NoError(t, err) + + body, err := Fetch(ctx, &tls.Config{RootCAs: pool}, req, + WithProxyURL(&url.URL{Scheme: "http", Host: proxy.Addr})) + require.NoError(t, err) + assert.Equal(t, "hello through proxy", string(body)) + + // Exactly one CONNECT, addressed to the target host:port (not the proxy). + assert.Equal(t, int64(1), proxy.Conns(), "expected a single CONNECT hop") + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + assert.Equal(t, targetURL.Host, proxy.Target()) +} + +// TestFetchProxyBasicAuth locks in that proxy credentials still reach the proxy +// on the auth path. With transport.Proxy nil, net/http no longer injects +// Proxy-Authorization, so the header must come from dialHTTPConnect using the +// proxy URL's userinfo. +func TestFetchProxyBasicAuth(t *testing.T) { + target := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("ok")) + })) + defer target.Close() + + pool := x509.NewCertPool() + pool.AddCert(target.Certificate()) + + proxy := testutil.NewConnectProxy(t) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + req, err := http.NewRequest(http.MethodGet, target.URL, nil) + require.NoError(t, err) + + _, err = Fetch(ctx, &tls.Config{RootCAs: pool}, req, + WithProxyURL(&url.URL{Scheme: "http", User: url.UserPassword("alice", "s3cret"), Host: proxy.Addr})) + require.NoError(t, err) + + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("alice:s3cret")) + assert.Equal(t, want, proxy.ProxyAuth(), "proxy credentials must reach the proxy via CONNECT") +} + +// TestFetchHTTPSProxyValidatesAgainstSystemTrust guards the auth-path trust fix: +// an https forward proxy's certificate must be validated against system trust, +// not the edge tlsConfig pool. The proxy cert here is trusted by the edge pool +// (which would have satisfied the old transport.Proxy path) but not by system +// trust, so the dial must fail with the system-trust error. +func TestFetchHTTPSProxyValidatesAgainstSystemTrust(t *testing.T) { + cert, leaf := selfSignedCert(t, "127.0.0.1") + proxyAddr := startTLSProxy(t, cert) + + pool := x509.NewCertPool() + pool.AddCert(leaf) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + req, err := http.NewRequest(http.MethodGet, "https://edge.example.com/path", nil) + require.NoError(t, err) + + _, err = Fetch(ctx, &tls.Config{RootCAs: pool}, req, + WithProxyURL(&url.URL{Scheme: "https", Host: proxyAddr})) + require.Error(t, err) + assert.Contains(t, err.Error(), "system-trusted") +} + +func selfSignedCert(t *testing.T, host string) (tls.Certificate, *x509.Certificate) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: host}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + if ip := net.ParseIP(host); ip != nil { + tmpl.IPAddresses = []net.IP{ip} + } else { + tmpl.DNSNames = []string{host} + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key) + require.NoError(t, err) + leaf, err := x509.ParseCertificate(der) + require.NoError(t, err) + return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key, Leaf: leaf}, leaf +} + +// startTLSProxy starts a TLS listener that presents cert and drives the TLS +// handshake on each connection. A client that does not trust cert fails during +// the handshake, before any CONNECT is exchanged. +func startTLSProxy(t *testing.T, cert tls.Certificate) string { + t.Helper() + li, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{Certificates: []tls.Certificate{cert}}) + require.NoError(t, err) + t.Cleanup(func() { _ = li.Close() }) + go func() { + for { + conn, err := li.Accept() + if err != nil { + return + } + go func() { + defer conn.Close() + if tc, ok := conn.(*tls.Conn); ok { + _ = tc.Handshake() + } + }() + } + }() + return li.Addr().String() +} diff --git a/internal/httputil/proxy.go b/internal/httputil/proxy.go index 535bbed2..dcd30a1e 100644 --- a/internal/httputil/proxy.go +++ b/internal/httputil/proxy.go @@ -5,6 +5,7 @@ import ( "context" "crypto/tls" "encoding/base64" + "errors" "fmt" "net" "net/http" @@ -47,6 +48,16 @@ func ResolveProxy(override string, edge *url.URL) (*url.URL, error) { return (&httpproxy.Config{HTTPProxy: all, HTTPSProxy: all, NoProxy: noProxy}).ProxyFunc()(edge) } +// ValidateForwardProxyFlag validates an explicit --forward-proxy value without +// consulting the environment. An empty value is valid and returns (nil, nil); +// the caller then falls back to environment-based resolution at request time. +func ValidateForwardProxyFlag(raw string) (*url.URL, error) { + if raw = strings.TrimSpace(raw); raw == "" { + return nil, nil + } + return normalizeForwardProxy(raw) +} + // normalizeForwardProxy validates and normalizes an explicit --forward-proxy // override: a bare host:port defaults to http, the scheme must be one we can // dial, and a host is required. Error messages use Redacted so any embedded @@ -91,10 +102,11 @@ func DialThroughProxy(ctx context.Context, proxyURL *url.URL, target string) (ne if err != nil { return nil, fmt.Errorf("failed to create socks5 dialer: %w", err) } - if cd, ok := d.(proxy.ContextDialer); ok { - return cd.DialContext(ctx, "tcp", target) + cd, ok := d.(proxy.ContextDialer) + if !ok { + return nil, errors.New("socks5 dialer does not support context cancellation") } - return d.Dial("tcp", target) + return cd.DialContext(ctx, "tcp", target) case "http", "https", "": return dialHTTPConnect(ctx, proxyURL, target) default: diff --git a/internal/httputil/proxy_test.go b/internal/httputil/proxy_test.go index e018e69a..c8627d51 100644 --- a/internal/httputil/proxy_test.go +++ b/internal/httputil/proxy_test.go @@ -75,6 +75,29 @@ func TestResolveProxyRedactsCredentials(t *testing.T) { assert.NotContains(t, err.Error(), "hunter2") } +func TestValidateForwardProxyFlag(t *testing.T) { + // The explicit-flag validator must never consult the environment, otherwise + // env-proxy errors would surface at startup instead of at request time. + t.Setenv("HTTPS_PROXY", "http://env-proxy:3128") + t.Setenv("ALL_PROXY", "socks5://env-proxy:1080") + + u, err := ValidateForwardProxyFlag("") + require.NoError(t, err) + assert.Nil(t, u, "empty flag is valid and must not resolve env proxies") + + u, err = ValidateForwardProxyFlag(" ") + require.NoError(t, err) + assert.Nil(t, u, "whitespace flag is treated as empty") + + u, err = ValidateForwardProxyFlag("proxy:8080") + require.NoError(t, err) + require.NotNil(t, u) + assert.Equal(t, "http://proxy:8080", u.String()) + + _, err = ValidateForwardProxyFlag("ftp://nope:21") + require.Error(t, err) +} + func TestDialThroughProxyHTTPConnect(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() diff --git a/tunnel/tunnel_http1.go b/tunnel/tunnel_http1.go index 4a98003f..7a93b84d 100644 --- a/tunnel/tunnel_http1.go +++ b/tunnel/tunnel_http1.go @@ -52,12 +52,8 @@ func (t *http1tunneler) TunnelTCP( return fmt.Errorf("http/1: failed to establish connection to proxy: %w", err) } defer remote.Close() - if done := ctx.Done(); done != nil { - go func() { - <-done - _ = remote.Close() - }() - } + stop := context.AfterFunc(ctx, func() { _ = remote.Close() }) + defer stop() err = req.Write(remote) if err != nil { @@ -116,7 +112,8 @@ func (t *http1tunneler) TunnelUDP( return fmt.Errorf("http/1: failed to establish connection to proxy: %w", err) } defer remote.Close() - context.AfterFunc(ctx, func() { _ = remote.Close() }) + stop := context.AfterFunc(ctx, func() { _ = remote.Close() }) + defer stop() dstHost, dstPort, err := net.SplitHostPort(t.cfg.dstHost) if err != nil { diff --git a/tunnel/tunnel_http1_test.go b/tunnel/tunnel_http1_test.go new file mode 100644 index 00000000..cc97a390 --- /dev/null +++ b/tunnel/tunnel_http1_test.go @@ -0,0 +1,88 @@ +package tunnel + +import ( + "bufio" + "io" + "net" + "runtime" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHTTP1TunnelTCPNoGoroutineLeak guards the per-connection goroutine leak +// fix: a completed TunnelTCP must not leave a goroutine parked on ctx.Done while +// the parent context stays alive. It is intentionally non-parallel so it runs in +// isolation from the package's t.Parallel tests, keeping the goroutine count +// stable. +func TestHTTP1TunnelTCPNoGoroutineLeak(t *testing.T) { + clearProxyEnv(t) + + edge := startCONNECTEdge(t) + ctx := t.Context() + + run := func() { + tun := &http1tunneler{getConfig( + WithDestinationHost("example.com:9999"), + WithProxyHost(edge), + )} + c1, c2 := net.Pipe() + _ = c1.Close() // local EOF: the tunnel completes immediately + require.NoError(t, tun.TunnelTCP(ctx, DiscardEvents(), c2, "")) + _ = c2.Close() + } + + run() // warm up one-time goroutines before sampling the baseline + base := runtime.NumGoroutine() + + const n = 30 + for range n { + run() + } + + // Transient goroutines (edge handlers, copy loops) exit once each connection + // closes; the buggy ctx.Done waiter never would. Poll until the count settles. + var leaked int + for range 200 { + if leaked = runtime.NumGoroutine() - base; leaked < n/2 { + break + } + time.Sleep(10 * time.Millisecond) + } + assert.Less(t, leaked, n/2, "TunnelTCP leaked goroutines after connections closed") +} + +// startCONNECTEdge accepts CONNECT requests, replies 200, then drains until the +// client closes. It serves the goroutine-leak test as a minimal Pomerium edge. +func startCONNECTEdge(t *testing.T) string { + t.Helper() + li, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = li.Close() }) + go func() { + for { + conn, err := li.Accept() + if err != nil { + return + } + go func() { + defer conn.Close() + br := bufio.NewReader(conn) + for { + line, err := br.ReadString('\n') + if err != nil { + return + } + if line == "\r\n" { + break + } + } + _, _ = io.WriteString(conn, "HTTP/1.1 200 OK\r\n\r\n") + _, _ = io.Copy(io.Discard, conn) + }() + } + }() + return li.Addr().String() +} diff --git a/tunnel/tunnel_http2.go b/tunnel/tunnel_http2.go deleted file mode 100644 index c11722d3..00000000 --- a/tunnel/tunnel_http2.go +++ /dev/null @@ -1,107 +0,0 @@ -package tunnel - -import ( - "context" - "crypto/tls" - "fmt" - "io" - "net/http" - "net/url" - - "github.com/rs/zerolog/log" - "golang.org/x/net/http2" -) - -type http2tunneler struct { - cfg *config -} - -func (t *http2tunneler) TunnelTCP( - ctx context.Context, - eventSink EventSink, - local io.ReadWriter, - rawJWT string, -) error { - ctx = log.Ctx(ctx).With().Str("component", "http2tunneler").Logger().WithContext(ctx) - - eventSink.OnConnecting(ctx) - - hdr := http.Header{} - if rawJWT != "" { - hdr.Set("Authorization", "Pomerium "+rawJWT) - } - - if t.cfg.tlsConfig == nil { - return fmt.Errorf("%w: http2 requires TLS", errUnsupported) - } - - proxyURL, err := resolveEdgeProxy(t.cfg) - if err != nil { - return fmt.Errorf("http/2: failed to resolve forward proxy: %w", err) - } - raw, err := dialEdgeTLS(ctx, t.cfg, proxyURL, []string{"h2"}) - if err != nil { - return fmt.Errorf("http/2: failed to establish connection to proxy: %w", err) - } - defer raw.Close() - - remote, ok := raw.(*tls.Conn) - if !ok { - return fmt.Errorf("%w: unexpected connection type returned from dial: %T", errUnsupported, raw) - } - - protocol := remote.ConnectionState().NegotiatedProtocol - if protocol != "h2" { - return fmt.Errorf("%w: unexpected TLS protocol: %s", errUnsupported, protocol) - } - - cc, err := (&http2.Transport{}).NewClientConn(remote) - if err != nil { - return fmt.Errorf("http/2: failed to establish connection: %w", err) - } - defer cc.Close() - - pr, pw := io.Pipe() - - req := (&http.Request{ - Method: "CONNECT", - URL: &url.URL{Opaque: t.cfg.dstHost}, - Host: t.cfg.dstHost, - Header: hdr, - Body: pr, - ContentLength: -1, - }).WithContext(ctx) - - res, err := cc.RoundTrip(req) - if err != nil { - return fmt.Errorf("http/2: error making connect request: %w", err) - } - defer res.Body.Close() - - err = httpStatusCodeToError(res.StatusCode) - if err != nil { - return err - } - - eventSink.OnConnected(ctx) - - errc := make(chan error, 2) - go func() { - _, err := io.Copy(pw, local) - errc <- err - }() - go func() { - _, err := io.Copy(local, res.Body) - errc <- err - }() - - select { - case err = <-errc: - case <-ctx.Done(): - err = context.Cause(ctx) - } - - eventSink.OnDisconnected(ctx, err) - - return err -} diff --git a/tunnel/tunnel_http2_test.go b/tunnel/tunnel_http2_test.go deleted file mode 100644 index 0840e370..00000000 --- a/tunnel/tunnel_http2_test.go +++ /dev/null @@ -1,70 +0,0 @@ -package tunnel - -import ( - "context" - "crypto/tls" - "io" - "net" - "net/http" - "net/http/httptest" - "testing" - "time" - - "github.com/stretchr/testify/assert" -) - -func TestTCPTunnelViaHTTP2(t *testing.T) { - t.Parallel() - - ctx, clearTimeout := context.WithTimeout(context.Background(), time.Second*10) - defer clearTimeout() - - srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !assert.Equal(t, "CONNECT", r.Method) { - return - } - if !assert.Equal(t, "Pomerium JWT", r.Header.Get("Authorization")) { - return - } - if !assert.Equal(t, "example.com:9999", r.Host) { - return - } - - defer r.Body.Close() - w.WriteHeader(http.StatusOK) - w.(http.Flusher).Flush() - - buf := make([]byte, 4) - _, err := io.ReadFull(r.Body, buf) - assert.NoError(t, err) - assert.Equal(t, []byte{1, 2, 3, 4}, buf) - - _, _ = w.Write([]byte{5, 6, 7, 8}) - })) - srv.EnableHTTP2 = true - srv.StartTLS() - - c1, c2 := net.Pipe() - go func() { - _, _ = c1.Write([]byte{1, 2, 3, 4}) - }() - go func() { - buf := make([]byte, 4) - _, err := io.ReadFull(c1, buf) - assert.NoError(t, err) - assert.Equal(t, []byte{5, 6, 7, 8}, buf) - _ = c1.Close() - }() - - tun := &http2tunneler{ - getConfig( - WithDestinationHost("example.com:9999"), - WithProxyHost(srv.Listener.Addr().String()), - WithTLSConfig(&tls.Config{ - InsecureSkipVerify: true, - }), - ), - } - err := tun.TunnelTCP(ctx, DiscardEvents(), c2, "JWT") - assert.NoError(t, err) -} From a12e354aa099b6050be84fcd843935d494a3c8d3 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Sat, 30 May 2026 16:52:08 -0700 Subject: [PATCH 3/4] fix(tcp): accept a trailing slash in --forward-proxy normalizeForwardProxy rejected any path on an explicit --forward-proxy value, so a harmless trailing slash (http://proxy:3128/) was an error. Treat a lone "/" path as empty before the path/query/fragment check; real paths, queries, and fragments are still rejected. Covers explicit-scheme, socks5, and bare host:port forms. Assisted-by: Claude (Opus 4.8); reviewed by Go-specialist and codex agents; make build/test/lint and go test -race green. ENG-4082 --- internal/httputil/proxy.go | 5 +++++ internal/httputil/proxy_test.go | 3 +++ 2 files changed, 8 insertions(+) diff --git a/internal/httputil/proxy.go b/internal/httputil/proxy.go index dcd30a1e..428355d4 100644 --- a/internal/httputil/proxy.go +++ b/internal/httputil/proxy.go @@ -79,6 +79,11 @@ func normalizeForwardProxy(override string) (*url.URL, error) { if u.Hostname() == "" { return nil, fmt.Errorf("forward proxy %q has no host", u.Redacted()) } + // A lone trailing slash (http://proxy:3128/) is harmless; normalize it away. + // Any other path, query, or fragment is still rejected. + if u.Path == "/" { + u.Path = "" + } if u.Path != "" || u.RawQuery != "" || u.Fragment != "" { return nil, fmt.Errorf("forward proxy %q must not have a path, query, or fragment", u.Redacted()) } diff --git a/internal/httputil/proxy_test.go b/internal/httputil/proxy_test.go index c8627d51..b8588872 100644 --- a/internal/httputil/proxy_test.go +++ b/internal/httputil/proxy_test.go @@ -32,6 +32,9 @@ func TestResolveProxy(t *testing.T) { {name: "override socks5", override: "socks5://proxy:1080", edge: httpsEdge, want: "socks5://proxy:1080"}, {name: "override socks5h", override: "socks5h://proxy:1080", edge: httpsEdge, want: "socks5h://proxy:1080"}, {name: "override trims whitespace", override: " http://proxy:8080 ", edge: httpsEdge, want: "http://proxy:8080"}, + {name: "override trailing slash ok", override: "http://proxy:8080/", edge: httpsEdge, want: "http://proxy:8080"}, + {name: "override socks5 trailing slash ok", override: "socks5://proxy:1080/", edge: httpsEdge, want: "socks5://proxy:1080"}, + {name: "override bare host trailing slash ok", override: "proxy:8080/", edge: httpsEdge, want: "http://proxy:8080"}, {name: "https_proxy", env: map[string]string{"HTTPS_PROXY": "http://hp:3128"}, edge: httpsEdge, want: "http://hp:3128"}, {name: "http_proxy", env: map[string]string{"HTTP_PROXY": "http://hp:3128"}, edge: httpEdge, want: "http://hp:3128"}, {name: "no_proxy", env: map[string]string{"HTTPS_PROXY": "http://hp:3128", "NO_PROXY": "edge.example.com"}, edge: httpsEdge, want: ""}, From ff6a163d7977a28a16dc197eed47a2767a9535c1 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Wed, 10 Jun 2026 15:03:59 -0700 Subject: [PATCH 4/4] fix(tcp): address forward-proxy review findings - never echo credentials in --forward-proxy parse errors: unwrap url.Error and replace url.EscapeError, both of which echo raw input; cover the parse-failure paths in the redaction test - keep plain-http targets on the default transport when the proxy comes from the environment (absolute-form proxying instead of CONNECT to :80, which proxy ACLs commonly deny); an explicit --forward-proxy still applies everywhere; new ProxyFetchOptions helper shared by authclient and the routes portal so one flow takes one path to the server - splice the test CONNECT proxy to its per-connection target instead of the shared last-recorded one - drop the dead nextProtos parameter on dialEdgeTLS (WithTLSConfig already pins ALPN) and the no-op context.AfterFunc(...)() calls in the UDP tests - disambiguate forward-proxy dial errors from the Pomerium edge, shorten the --forward-proxy help text, consolidate the tunnel package's CONNECT edge fakes and proxy-env helpers, trim diff-relative comments and duplicate scheme-validation tests --- authclient/authclient.go | 9 +---- authclient/authclient_test.go | 8 ++--- cmd/pomerium-cli/tcp.go | 2 +- internal/httputil/fetch_test.go | 21 ++++------- internal/httputil/proxy.go | 43 ++++++++++++++++++----- internal/httputil/proxy_test.go | 62 ++++++++++++++++++++++++--------- internal/portal/portal.go | 8 ++++- internal/testutil/proxy.go | 14 ++++++-- tunnel/dial.go | 13 +++---- tunnel/tunnel_http1.go | 8 ++--- tunnel/tunnel_http1_test.go | 56 +++++++---------------------- tunnel/tunnel_proxy_test.go | 23 +++++------- tunnel/tunnel_udp_test.go | 13 +++---- 13 files changed, 144 insertions(+), 136 deletions(-) diff --git a/authclient/authclient.go b/authclient/authclient.go index 4e09bf38..507fa7c1 100644 --- a/authclient/authclient.go +++ b/authclient/authclient.go @@ -52,14 +52,7 @@ func (client *AuthClient) CheckBearerToken(ctx context.Context, serverURL *url.U // fetchOpts resolves the forward proxy for serverURL, if any. func (client *AuthClient) fetchOpts(serverURL *url.URL) ([]httputil.FetchOption, error) { - proxyURL, err := httputil.ResolveProxy(client.cfg.forwardProxy, serverURL) - if err != nil { - return nil, err - } - if proxyURL == nil { - return nil, nil - } - return []httputil.FetchOption{httputil.WithProxyURL(proxyURL)}, nil + return httputil.ProxyFetchOptions(client.cfg.forwardProxy, serverURL) } // GetJWT retrieves a JWT from Pomerium. diff --git a/authclient/authclient_test.go b/authclient/authclient_test.go index d02211dc..d78e7d76 100644 --- a/authclient/authclient_test.go +++ b/authclient/authclient_test.go @@ -20,8 +20,7 @@ import ( "github.com/pomerium/cli/internal/testutil" ) -// TestCheckBearerTokenViaProxy verifies the auth path routes through a forward -// proxy. Auth uses an HTTPS server so http.Transport issues a CONNECT. +// Auth requests route through the forward proxy. func TestCheckBearerTokenViaProxy(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -36,8 +35,9 @@ func TestCheckBearerTokenViaProxy(t *testing.T) { proxy := testutil.NewConnectProxy(t) - ac := New(WithForwardProxy(proxy.Addr)) - ac.cfg.tlsConfig = &tls.Config{RootCAs: pool} + ac := New( + WithForwardProxy(proxy.Addr), + WithTLSConfig(&tls.Config{RootCAs: pool})) serverURL, err := url.Parse(srv.URL) require.NoError(t, err) diff --git a/cmd/pomerium-cli/tcp.go b/cmd/pomerium-cli/tcp.go index cd23cac9..6c6bb1e7 100644 --- a/cmd/pomerium-cli/tcp.go +++ b/cmd/pomerium-cli/tcp.go @@ -32,7 +32,7 @@ func init() { flags.StringVar(&tcpCmdOptions.pomeriumURL, "pomerium-url", "", "the URL of the pomerium server to connect to") flags.StringVar(&tcpCmdOptions.forwardProxy, "forward-proxy", "", - "HTTP CONNECT or SOCKS5 forward proxy for the tunnel (host:port or URL); authoritative and ignores NO_PROXY. Without it, HTTP_PROXY/HTTPS_PROXY (honoring NO_PROXY) and ALL_PROXY apply.") + "forward proxy for the tunnel and auth (host:port or http/https/socks5/socks5h URL); overrides proxy environment variables and ignores NO_PROXY") rootCmd.AddCommand(tcpCmd) } diff --git a/internal/httputil/fetch_test.go b/internal/httputil/fetch_test.go index 39ac94d3..bfc62e93 100644 --- a/internal/httputil/fetch_test.go +++ b/internal/httputil/fetch_test.go @@ -23,11 +23,8 @@ import ( "github.com/pomerium/cli/internal/testutil" ) -// TestFetchHTTPSTargetThroughProxy proves the auth path reaches an https target -// through a forward proxy with a single CONNECT hop, and that the target's TLS -// is still validated against the edge tlsConfig (over the tunnel) rather than -// skipped. It locks in the "no double-proxy, target TLS preserved" contract of -// the custom DialContext. +// One CONNECT hop to the target; the target's TLS is still validated against +// tlsConfig over the tunnel. func TestFetchHTTPSTargetThroughProxy(t *testing.T) { target := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("hello through proxy")) @@ -57,10 +54,8 @@ func TestFetchHTTPSTargetThroughProxy(t *testing.T) { assert.Equal(t, targetURL.Host, proxy.Target()) } -// TestFetchProxyBasicAuth locks in that proxy credentials still reach the proxy -// on the auth path. With transport.Proxy nil, net/http no longer injects -// Proxy-Authorization, so the header must come from dialHTTPConnect using the -// proxy URL's userinfo. +// With transport.Proxy nil, net/http no longer injects Proxy-Authorization, so +// the header must come from dialHTTPConnect using the proxy URL's userinfo. func TestFetchProxyBasicAuth(t *testing.T) { target := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("ok")) @@ -86,11 +81,9 @@ func TestFetchProxyBasicAuth(t *testing.T) { assert.Equal(t, want, proxy.ProxyAuth(), "proxy credentials must reach the proxy via CONNECT") } -// TestFetchHTTPSProxyValidatesAgainstSystemTrust guards the auth-path trust fix: -// an https forward proxy's certificate must be validated against system trust, -// not the edge tlsConfig pool. The proxy cert here is trusted by the edge pool -// (which would have satisfied the old transport.Proxy path) but not by system -// trust, so the dial must fail with the system-trust error. +// An https forward proxy is validated against system trust, not the edge +// tlsConfig pool: the proxy cert here is trusted by the edge pool but not by +// system trust, so the dial must fail. func TestFetchHTTPSProxyValidatesAgainstSystemTrust(t *testing.T) { cert, leaf := selfSignedCert(t, "127.0.0.1") proxyAddr := startTLSProxy(t, cert) diff --git a/internal/httputil/proxy.go b/internal/httputil/proxy.go index 428355d4..accafcdb 100644 --- a/internal/httputil/proxy.go +++ b/internal/httputil/proxy.go @@ -48,6 +48,27 @@ func ResolveProxy(override string, edge *url.URL) (*url.URL, error) { return (&httpproxy.Config{HTTPProxy: all, HTTPSProxy: all, NoProxy: noProxy}).ProxyFunc()(edge) } +// ProxyFetchOptions resolves the forward proxy for target and returns the +// Fetch options that route requests through it. Plain-http targets with an +// environment-resolved proxy return no options: the default transport already +// proxies them in absolute form, which proxy ACLs commonly allow where a +// CONNECT to port 80 is denied. That carve-out keeps the default transport's +// env semantics for http targets (no ALL_PROXY, proxy TLS verified against +// the request's tls config). An explicit override always applies. +func ProxyFetchOptions(override string, target *url.URL) ([]FetchOption, error) { + if strings.TrimSpace(override) == "" && target.Scheme != "https" { + return nil, nil + } + proxyURL, err := ResolveProxy(override, target) + if err != nil { + return nil, err + } + if proxyURL == nil { + return nil, nil + } + return []FetchOption{WithProxyURL(proxyURL)}, nil +} + // ValidateForwardProxyFlag validates an explicit --forward-proxy value without // consulting the environment. An empty value is valid and returns (nil, nil); // the caller then falls back to environment-based resolution at request time. @@ -60,15 +81,22 @@ func ValidateForwardProxyFlag(raw string) (*url.URL, error) { // normalizeForwardProxy validates and normalizes an explicit --forward-proxy // override: a bare host:port defaults to http, the scheme must be one we can -// dial, and a host is required. Error messages use Redacted so any embedded -// credentials never leak. +// dial, and a host is required. func normalizeForwardProxy(override string) (*url.URL, error) { if !strings.Contains(override, "://") { override = "http://" + override } u, err := url.Parse(override) if err != nil { - // the raw string may carry credentials, so don't echo it. + // url.Error echoes the raw URL and url.EscapeError echoes the bytes + // around a bad escape; either may carry credentials, so surface only a + // credential-free cause. + if ue, ok := errors.AsType[*url.Error](err); ok { + err = ue.Err + } + if _, ok := errors.AsType[url.EscapeError](err); ok { + err = errors.New("invalid percent-escape") + } return nil, fmt.Errorf("invalid forward proxy: %w", err) } switch u.Scheme { @@ -80,7 +108,6 @@ func normalizeForwardProxy(override string) (*url.URL, error) { return nil, fmt.Errorf("forward proxy %q has no host", u.Redacted()) } // A lone trailing slash (http://proxy:3128/) is harmless; normalize it away. - // Any other path, query, or fragment is still rejected. if u.Path == "/" { u.Path = "" } @@ -131,7 +158,7 @@ func dialHTTPConnect(ctx context.Context, proxyURL *url.URL, target string) (_ n conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", proxyAddr) if err != nil { - return nil, fmt.Errorf("failed to dial proxy: %w", err) + return nil, fmt.Errorf("failed to dial forward proxy: %w", err) } defer func() { if retErr != nil { @@ -153,8 +180,8 @@ func dialHTTPConnect(ctx context.Context, proxyURL *url.URL, target string) (_ n conn = tc } - // abort the CONNECT exchange on cancellation; stop() before a successful - // return so the deadline-cleared conn handed to the caller is never closed. + // abort the CONNECT exchange on cancellation; stop() detaches the conn from + // ctx before it is handed to the caller. stop := context.AfterFunc(ctx, func() { _ = conn.Close() }) defer stop() @@ -181,7 +208,7 @@ func dialHTTPConnect(ctx context.Context, proxyURL *url.URL, target string) (_ n } defer res.Body.Close() if res.StatusCode != http.StatusOK { - return nil, fmt.Errorf("proxy CONNECT failed: %s", res.Status) + return nil, fmt.Errorf("forward proxy CONNECT failed: %s", res.Status) } if err := conn.SetDeadline(time.Time{}); err != nil { diff --git a/internal/httputil/proxy_test.go b/internal/httputil/proxy_test.go index b8588872..11c9f179 100644 --- a/internal/httputil/proxy_test.go +++ b/internal/httputil/proxy_test.go @@ -49,9 +49,7 @@ func TestResolveProxy(t *testing.T) { {name: "no host", override: "http://", edge: httpsEdge, wantErr: true}, } { t.Run(tc.name, func(t *testing.T) { - for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "no_proxy", "all_proxy"} { - t.Setenv(k, "") - } + testutil.ClearProxyEnv(t) for k, v := range tc.env { t.Setenv(k, v) } @@ -73,9 +71,22 @@ func TestResolveProxy(t *testing.T) { } func TestResolveProxyRedactsCredentials(t *testing.T) { - _, err := ResolveProxy("http://user:hunter2@proxy:8080/path", &url.URL{Scheme: "https", Host: "edge:443"}) + edge := &url.URL{Scheme: "https", Host: "edge:443"} + for _, override := range []string{ + "http://user:hunter2@proxy:8080/path", // rejected after parsing + "http://user:hunter2@proxy:badport", // url.Parse failure + "http://user:hunter2%@proxy:8080", // invalid escape in the password itself + } { + _, err := ResolveProxy(override, edge) + require.Error(t, err) + assert.NotContains(t, err.Error(), "hunter2", "override %q", override) + } + + // url.EscapeError quotes the bytes around a bad escape ("%te" here), which + // would leak a password fragment. + _, err := ResolveProxy("http://user:hun%ter2@proxy:8080", edge) require.Error(t, err) - assert.NotContains(t, err.Error(), "hunter2") + assert.NotContains(t, err.Error(), "%te") } func TestValidateForwardProxyFlag(t *testing.T) { @@ -88,17 +99,40 @@ func TestValidateForwardProxyFlag(t *testing.T) { require.NoError(t, err) assert.Nil(t, u, "empty flag is valid and must not resolve env proxies") - u, err = ValidateForwardProxyFlag(" ") - require.NoError(t, err) - assert.Nil(t, u, "whitespace flag is treated as empty") - u, err = ValidateForwardProxyFlag("proxy:8080") require.NoError(t, err) require.NotNil(t, u) assert.Equal(t, "http://proxy:8080", u.String()) +} - _, err = ValidateForwardProxyFlag("ftp://nope:21") - require.Error(t, err) +// Plain-http targets keep the default transport's absolute-form env proxying; +// an explicit override always routes through the custom dialer. +func TestProxyFetchOptions(t *testing.T) { + testutil.ClearProxyEnv(t) + t.Setenv("HTTP_PROXY", "http://hp:3128") + t.Setenv("HTTPS_PROXY", "http://hp:3128") + + httpEdge := &url.URL{Scheme: "http", Host: "edge.example.com:80"} + httpsEdge := &url.URL{Scheme: "https", Host: "edge.example.com:443"} + + opts, err := ProxyFetchOptions("", httpEdge) + require.NoError(t, err) + assert.Empty(t, opts, "env proxy + plain-http target stays on the default transport") + + // the carve-out must not pre-empt env errors the default transport owns. + t.Setenv("ALL_PROXY", "::bad::") + opts, err = ProxyFetchOptions("", httpEdge) + require.NoError(t, err) + assert.Empty(t, opts) + t.Setenv("ALL_PROXY", "") + + opts, err = ProxyFetchOptions("", httpsEdge) + require.NoError(t, err) + assert.Len(t, opts, 1) + + opts, err = ProxyFetchOptions("op:9", httpEdge) + require.NoError(t, err) + assert.Len(t, opts, 1, "explicit override applies to plain-http targets") } func TestDialThroughProxyHTTPConnect(t *testing.T) { @@ -131,12 +165,6 @@ func TestDialThroughProxySOCKS5(t *testing.T) { assert.Equal(t, backend, proxy.Target()) } -func TestDialThroughProxyUnsupportedScheme(t *testing.T) { - _, err := DialThroughProxy(context.Background(), &url.URL{Scheme: "ftp", Host: "p:21"}, "edge:443") - require.Error(t, err) - assert.Contains(t, err.Error(), "unsupported proxy scheme") -} - func TestDialThroughProxyConnectRejected(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() diff --git a/internal/portal/portal.go b/internal/portal/portal.go index de295881..ff82e3c2 100644 --- a/internal/portal/portal.go +++ b/internal/portal/portal.go @@ -103,7 +103,13 @@ func (p *Portal) listRoutes(ctx context.Context, serverURL *url.URL, rawJWT stri req.Header.Set("Accept", "application/json") req.Header.Set("Authorization", "Bearer Pomerium-"+rawJWT) - bs, err := httputil.Fetch(ctx, p.cfg.tlsConfig, req) + // match the auth client's proxy resolution so the whole portal flow takes + // one path to the server. + opts, err := httputil.ProxyFetchOptions("", serverURL) + if err != nil { + return nil, fmt.Errorf("error resolving forward proxy: %w", err) + } + bs, err := httputil.Fetch(ctx, p.cfg.tlsConfig, req, opts...) if err != nil { return nil, fmt.Errorf("error fetching routes portal: %w", err) } diff --git a/internal/testutil/proxy.go b/internal/testutil/proxy.go index c5a42548..6e333dd5 100644 --- a/internal/testutil/proxy.go +++ b/internal/testutil/proxy.go @@ -69,6 +69,14 @@ func newConnectProxy(t *testing.T, upstream string) *RecordingProxy { return p } +// ClearProxyEnv blanks every proxy environment variable for the test. +func ClearProxyEnv(t *testing.T) { + t.Helper() + for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "no_proxy", "all_proxy"} { + t.Setenv(k, "") + } +} + func (p *RecordingProxy) handleConnect(conn net.Conn) { defer conn.Close() p.conns.Add(1) @@ -78,8 +86,10 @@ func (p *RecordingProxy) handleConnect(conn net.Conn) { if err != nil { return } + var target string if f := strings.Fields(line); len(f) >= 2 { - p.target.Store(f[1]) + target = f[1] + p.target.Store(target) } for { h, err := br.ReadString('\n') @@ -96,7 +106,7 @@ func (p *RecordingProxy) handleConnect(conn net.Conn) { } _, _ = io.WriteString(conn, "HTTP/1.1 200 Connection established\r\n\r\n") - dst := p.Target() + dst := target if p.upstream != "" { dst = p.upstream } diff --git a/tunnel/dial.go b/tunnel/dial.go index 5172891e..1c405211 100644 --- a/tunnel/dial.go +++ b/tunnel/dial.go @@ -26,10 +26,8 @@ func resolveEdgeProxy(cfg *config) (*url.URL, error) { // dialEdgeTLS establishes a connection to the Pomerium edge, routing through // proxyURL when non-nil, and TLS-wraps it when a tls config is configured. -// -// A nil nextProtos leaves the configured ALPN untouched (preserving the -// behavior of the original tls.Dialer for callers that don't pin a protocol). -func dialEdgeTLS(ctx context.Context, cfg *config, proxyURL *url.URL, nextProtos []string) (net.Conn, error) { +// The config's ALPN is already pinned to http/1.1 by WithTLSConfig. +func dialEdgeTLS(ctx context.Context, cfg *config, proxyURL *url.URL) (net.Conn, error) { var raw net.Conn var err error if proxyURL == nil { @@ -46,11 +44,8 @@ func dialEdgeTLS(ctx context.Context, cfg *config, proxyURL *url.URL, nextProtos } tlsCfg := cfg.tlsConfig.Clone() - if nextProtos != nil { - tlsCfg.NextProtos = nextProtos - } - // the old tls.Dialer derived ServerName from the dial address, so we must - // set it explicitly here for certificate verification to succeed. + // tls.Client does not derive ServerName from the dial address; set it or + // certificate verification fails. if tlsCfg.ServerName == "" { host, _, err := net.SplitHostPort(cfg.proxyHost) if err != nil { diff --git a/tunnel/tunnel_http1.go b/tunnel/tunnel_http1.go index 7a93b84d..66928c17 100644 --- a/tunnel/tunnel_http1.go +++ b/tunnel/tunnel_http1.go @@ -47,7 +47,7 @@ func (t *http1tunneler) TunnelTCP( if err != nil { return fmt.Errorf("http/1: failed to resolve forward proxy: %w", err) } - remote, err := dialEdgeTLS(ctx, t.cfg, proxyURL, []string{"http/1.1"}) + remote, err := dialEdgeTLS(ctx, t.cfg, proxyURL) if err != nil { return fmt.Errorf("http/1: failed to establish connection to proxy: %w", err) } @@ -104,10 +104,8 @@ func (t *http1tunneler) TunnelUDP( ) error { eventSink.OnConnecting(ctx) - // UDP never traverses a forward proxy (TCP-only feature), so dial direct - // (nil proxyURL). nil nextProtos preserves the original UDP/MASQUE handshake, - // which did not pin ALPN (only the TCP CONNECT path negotiates http/1.1). - remote, err := dialEdgeTLS(ctx, t.cfg, nil, nil) + // UDP never traverses a forward proxy (TCP-only feature), so dial direct. + remote, err := dialEdgeTLS(ctx, t.cfg, nil) if err != nil { return fmt.Errorf("http/1: failed to establish connection to proxy: %w", err) } diff --git a/tunnel/tunnel_http1_test.go b/tunnel/tunnel_http1_test.go index cc97a390..374b7ad3 100644 --- a/tunnel/tunnel_http1_test.go +++ b/tunnel/tunnel_http1_test.go @@ -1,8 +1,6 @@ package tunnel import ( - "bufio" - "io" "net" "runtime" "testing" @@ -10,17 +8,16 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/pomerium/cli/internal/testutil" ) -// TestHTTP1TunnelTCPNoGoroutineLeak guards the per-connection goroutine leak -// fix: a completed TunnelTCP must not leave a goroutine parked on ctx.Done while -// the parent context stays alive. It is intentionally non-parallel so it runs in -// isolation from the package's t.Parallel tests, keeping the goroutine count -// stable. +// A completed TunnelTCP must not leave a goroutine parked on ctx.Done while the +// parent context stays alive. Non-parallel so the goroutine count stays stable. func TestHTTP1TunnelTCPNoGoroutineLeak(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) - edge := startCONNECTEdge(t) + edge, _ := fakeEdge(t) ctx := t.Context() run := func() { @@ -29,7 +26,11 @@ func TestHTTP1TunnelTCPNoGoroutineLeak(t *testing.T) { WithProxyHost(edge), )} c1, c2 := net.Pipe() - _ = c1.Close() // local EOF: the tunnel completes immediately + // one line then EOF: the backend closes per line, unwinding the chain. + go func() { + _, _ = c1.Write([]byte("x\n")) + _ = c1.Close() + }() require.NoError(t, tun.TunnelTCP(ctx, DiscardEvents(), c2, "")) _ = c2.Close() } @@ -43,7 +44,7 @@ func TestHTTP1TunnelTCPNoGoroutineLeak(t *testing.T) { } // Transient goroutines (edge handlers, copy loops) exit once each connection - // closes; the buggy ctx.Done waiter never would. Poll until the count settles. + // closes; a leaked ctx.Done waiter never would. Poll until the count settles. var leaked int for range 200 { if leaked = runtime.NumGoroutine() - base; leaked < n/2 { @@ -53,36 +54,3 @@ func TestHTTP1TunnelTCPNoGoroutineLeak(t *testing.T) { } assert.Less(t, leaked, n/2, "TunnelTCP leaked goroutines after connections closed") } - -// startCONNECTEdge accepts CONNECT requests, replies 200, then drains until the -// client closes. It serves the goroutine-leak test as a minimal Pomerium edge. -func startCONNECTEdge(t *testing.T) string { - t.Helper() - li, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - t.Cleanup(func() { _ = li.Close() }) - go func() { - for { - conn, err := li.Accept() - if err != nil { - return - } - go func() { - defer conn.Close() - br := bufio.NewReader(conn) - for { - line, err := br.ReadString('\n') - if err != nil { - return - } - if line == "\r\n" { - break - } - } - _, _ = io.WriteString(conn, "HTTP/1.1 200 OK\r\n\r\n") - _, _ = io.Copy(io.Discard, conn) - }() - } - }() - return li.Addr().String() -} diff --git a/tunnel/tunnel_proxy_test.go b/tunnel/tunnel_proxy_test.go index 36f8f3f1..b1789379 100644 --- a/tunnel/tunnel_proxy_test.go +++ b/tunnel/tunnel_proxy_test.go @@ -72,15 +72,8 @@ func fakeEdge(t *testing.T) (edgeAddr string, got <-chan string) { return srv.Listener.Addr().String(), gotc } -func clearProxyEnv(t *testing.T) { - t.Helper() - for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "no_proxy", "all_proxy"} { - t.Setenv(k, "") - } -} - -// runTCPTunnel drives one "HELLO WORLD" line through the tunnel and returns what -// the backend received. +// runTCPTunnel drives one "HELLO WORLD" line through the tunnel and asserts +// the backend received it. func runTCPTunnel(t *testing.T, got <-chan string, opts ...Option) { t.Helper() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -99,7 +92,7 @@ func runTCPTunnel(t *testing.T, got <-chan string, opts ...Option) { } func TestTunnelViaForwardProxyFlag(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) edge, got := fakeEdge(t) proxy := testutil.NewConnectProxy(t) @@ -116,7 +109,7 @@ func TestTunnelViaForwardProxyFlag(t *testing.T) { // that splices to the real loopback edge. NO_PROXY and override precedence at // the resolution layer are covered by TestResolveProxy. func TestTunnelHonorsEnvProxy(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) edge, got := fakeEdge(t) proxy := testutil.NewConnectProxyTo(t, edge) t.Setenv("HTTP_PROXY", "http://"+proxy.Addr) @@ -129,7 +122,7 @@ func TestTunnelHonorsEnvProxy(t *testing.T) { } func TestTunnelFlagOverridesEnv(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) edge, got := fakeEdge(t) envProxy := testutil.NewConnectProxyTo(t, edge) flagProxy := testutil.NewConnectProxyTo(t, edge) @@ -145,7 +138,7 @@ func TestTunnelFlagOverridesEnv(t *testing.T) { } func TestTunnelViaSOCKS5(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) edge, got := fakeEdge(t) proxy := testutil.NewSOCKS5Proxy(t) @@ -158,7 +151,7 @@ func TestTunnelViaSOCKS5(t *testing.T) { } func TestTunnelUnsupportedProxyScheme(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -173,7 +166,7 @@ func TestTunnelUnsupportedProxyScheme(t *testing.T) { } func TestTunnelProxyCredentialsNotLogged(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) edge, got := fakeEdge(t) proxy := testutil.NewConnectProxy(t) diff --git a/tunnel/tunnel_udp_test.go b/tunnel/tunnel_udp_test.go index f3afe412..0308afda 100644 --- a/tunnel/tunnel_udp_test.go +++ b/tunnel/tunnel_udp_test.go @@ -77,7 +77,7 @@ func TestUDPSessionManager(t *testing.T) { conn, err := net.ListenUDP("udp", localAddr) require.NoError(t, err) - context.AfterFunc(ctx, func() { _ = conn.Close() })() + context.AfterFunc(ctx, func() { _ = conn.Close() }) n, err := conn.WriteToUDP([]byte("SEND HELLO WORLD"), tunnelAddr) assert.Equal(t, 16, n) @@ -98,11 +98,9 @@ func TestUDPSessionManager(t *testing.T) { assert.NoError(t, err, "tunnel should shutdown cleanly") } -// TestTunnelUDPIgnoresForwardProxy proves the forward proxy is TCP-only: with -// HTTP_PROXY/HTTPS_PROXY set, a UDP tunnel still dials the edge directly and -// never touches the proxy. +// The forward proxy is TCP-only: a UDP tunnel dials the edge directly. func TestTunnelUDPIgnoresForwardProxy(t *testing.T) { - clearProxyEnv(t) + testutil.ClearProxyEnv(t) proxy := testutil.NewConnectProxy(t) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -127,8 +125,7 @@ func TestTunnelUDPIgnoresForwardProxy(t *testing.T) { })) defer srv.Close() - // Explicit forward proxy (not loopback-bypassed like env vars). TunnelUDP - // must ignore it entirely, so the proxy should never be dialed. + // an explicit flag proxy: env proxies would bypass a loopback edge. tun := New( WithDestinationHost("example.com:9999"), WithProxyHost(srv.Listener.Addr().String()), @@ -146,7 +143,7 @@ func TestTunnelUDPIgnoresForwardProxy(t *testing.T) { require.NoError(t, err) conn, err := net.ListenUDP("udp", localAddr) require.NoError(t, err) - context.AfterFunc(ctx, func() { _ = conn.Close() })() + context.AfterFunc(ctx, func() { _ = conn.Close() }) _, err = conn.WriteToUDP([]byte("SEND HELLO WORLD"), tunnelAddr) require.NoError(t, err)