From de7f33579148e4c7cce9257f63d69d0d2662d651 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Fri, 7 Aug 2026 16:37:03 +0200 Subject: [PATCH] fix(outbound): keep dial pinning for hops the environment proxy declines --- pkg/gotenberg/outbound.go | 112 +++++++++++++++-- pkg/gotenberg/outbound_envproxy_test.go | 155 ++++++++++++++++++++++++ pkg/modules/api/api.go | 7 ++ pkg/modules/chromium/chromium.go | 7 ++ pkg/modules/libreoffice/api/api.go | 7 ++ pkg/modules/webhook/webhook.go | 16 +++ 6 files changed, 295 insertions(+), 9 deletions(-) create mode 100644 pkg/gotenberg/outbound_envproxy_test.go diff --git a/pkg/gotenberg/outbound.go b/pkg/gotenberg/outbound.go index 874f315f..0f1912cb 100644 --- a/pkg/gotenberg/outbound.go +++ b/pkg/gotenberg/outbound.go @@ -11,6 +11,7 @@ import ( "net/http" "net/netip" "net/url" + "os" "strings" "time" @@ -187,6 +188,12 @@ type OutboundDecision struct { // is stored. type outboundDecisionKey struct{} +// outboundProxiedKey is the context key under which [outboundRoundTripper] +// records that the environment proxy will carry this request, so that the +// dialer knows the address it receives is the proxy's rather than the +// destination's. +type outboundProxiedKey struct{} + // decideConfig carries optional settings for [DecideOutbound] and // [FilterOutboundURL]. See [DecideOption] for how callers configure it. type decideConfig struct { @@ -364,6 +371,10 @@ type outboundRoundTripper struct { allowList []*regexp2.Regexp denyList []*regexp2.Regexp opts []DecideOption + + // proxyFunc mirrors the transport's own proxy resolution. It is nil unless + // the environment proxy is enabled. + proxyFunc func(*url.URL) (*url.URL, error) } // RoundTrip validates req.URL and delegates to the base transport. @@ -379,6 +390,18 @@ func (rt *outboundRoundTripper) RoundTrip(req *http.Request) (*http.Response, er } ctx := context.WithValue(req.Context(), outboundDecisionKey{}, decision) + + // A request the proxy will not carry is dialed directly, so it still gets + // pinned. Without this, enabling the environment proxy would silently drop + // DNS-rebinding protection for every NO_PROXY host, and for all traffic + // when no proxy variable is set at all. + if rt.proxyFunc != nil { + proxyURL, proxyErr := rt.proxyFunc(req.URL) + if proxyErr == nil && proxyURL != nil { + ctx = context.WithValue(ctx, outboundProxiedKey{}, true) + } + } + return rt.base.RoundTrip(req.WithContext(ctx)) } @@ -396,24 +419,34 @@ func (rt *outboundRoundTripper) RoundTrip(req *http.Request) (*http.Response, er // // When enableEnvironmentProxy is true, the client routes through the proxy // defined by the standard HTTP_PROXY, HTTPS_PROXY, and NO_PROXY variables, -// including any credentials embedded in those URLs. In that mode the proxy -// owns DNS and egress, so destination dial pinning does not apply; the URL -// allow/deny and IP-class validation still runs. Callers gate this behind -// their module's opt-in flag. See +// including any credentials embedded in those URLs. Dial pinning does not apply +// to a hop the proxy carries, since the proxy owns DNS and egress there; a hop +// the proxy declines, such as a NO_PROXY host, is dialed directly and stays +// pinned. The URL allow/deny and IP-class validation runs either way. Callers +// gate this behind their module's opt-in flag. See // https://github.com/gotenberg/gotenberg/issues/1592. func NewOutboundHttpClient(timeout time.Duration, allowList, denyList []*regexp2.Regexp, enableEnvironmentProxy bool, opts ...DecideOption) *http.Client { base := http.DefaultTransport.(*http.Transport).Clone() + var proxyFunc func(*url.URL) (*url.URL, error) + if enableEnvironmentProxy { // Route through the operator's proxy (standard env vars, credentials - // included). NO_PROXY hosts get a direct, unpinned dial. - // httpproxy.FromEnvironment reads the environment now rather than - // caching it process-wide like http.ProxyFromEnvironment. - proxyFunc := httpproxy.FromEnvironment().ProxyFunc() + // included). httpproxy.FromEnvironment reads the environment now rather + // than caching it process-wide like http.ProxyFromEnvironment. + proxyFunc = httpproxy.FromEnvironment().ProxyFunc() base.Proxy = func(req *http.Request) (*url.URL, error) { return proxyFunc(req.URL) } - base.DialContext = outboundDialer.DialContext + // Only a hop the proxy actually carries skips pinning: there the dial + // targets the proxy, not the destination, and the proxy owns DNS. A hop + // the proxy declines is dialed directly and stays pinned. + base.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + if proxied, _ := ctx.Value(outboundProxiedKey{}).(bool); proxied { + return outboundDialer.DialContext(ctx, network, addr) + } + return secureDialContext(ctx, network, addr) + } } else { // Default: ignore any proxy environment variables and pin the dial to // the IPs resolved during validation, closing the DNS-rebinding @@ -431,10 +464,60 @@ func NewOutboundHttpClient(timeout time.Duration, allowList, denyList []*regexp2 allowList: allowList, denyList: denyList, opts: opts, + proxyFunc: proxyFunc, }, } } +// environmentProxyVariables are the variables golang.org/x/net/http/httpproxy +// reads, in the casing precedence it applies. +var environmentProxyVariables = []string{ + "HTTP_PROXY", "http_proxy", + "HTTPS_PROXY", "https_proxy", + "ALL_PROXY", "all_proxy", +} + +// ValidateEnvironmentProxyVariables checks that every proxy variable currently +// set can be parsed as a proxy URL. +// +// httpproxy discards a parse error and falls back to a direct connection, so an +// operator who mistypes a proxy URL would silently lose the egress path they +// meant to enforce. Modules exposing an environment proxy flag call this from +// their Validate so that startup fails loudly instead. +// +// Values are never included in the error: a proxy URL may carry credentials. +func ValidateEnvironmentProxyVariables() error { + var err error + + for _, name := range environmentProxyVariables { + if os.Getenv(name) == "" { + continue + } + + if !isUsableProxyURL(os.Getenv(name)) { + err = errors.Join(err, fmt.Errorf("environment variable %s is not a usable proxy URL; unset it, or set it to a value like 'http://user:password@host:3128'", name)) + } + } + + return err +} + +// isUsableProxyURL mirrors httpproxy's own parsing: a URL with a proxy scheme, +// or anything that becomes one once a scheme is prefixed. +func isUsableProxyURL(value string) bool { + proxyURL, err := url.Parse(value) + if err == nil { + switch proxyURL.Scheme { + case "http", "https", "socks5", "socks5h": + return true + } + } + + // httpproxy retries bare values such as "host:3128" with a scheme. + _, err = url.Parse("http://" + value) + return err == nil +} + // secureDialContext consumes the [OutboundDecision] stashed in ctx by // [outboundRoundTripper]. When the decision is to bypass (allow-list // match), it dials directly. When the decision contains pinned IPs, it @@ -580,3 +663,14 @@ type bufferedConn struct { func (c *bufferedConn) Read(b []byte) (int, error) { return c.r.Read(b) } + +// CloseWrite half-closes the underlying connection. Embedding [net.Conn] hides +// the method, so a CONNECT splice over this connection could never signal EOF +// to the upstream and both sides waited for the other until a timeout. +func (c *bufferedConn) CloseWrite() error { + cw, ok := c.Conn.(interface{ CloseWrite() error }) + if !ok { + return fmt.Errorf("underlying %T does not support half-close", c.Conn) + } + return cw.CloseWrite() +} diff --git a/pkg/gotenberg/outbound_envproxy_test.go b/pkg/gotenberg/outbound_envproxy_test.go new file mode 100644 index 00000000..b77d448b --- /dev/null +++ b/pkg/gotenberg/outbound_envproxy_test.go @@ -0,0 +1,155 @@ +package gotenberg + +import ( + "net" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + "testing" +) + +func TestValidateEnvironmentProxyVariables(t *testing.T) { + for _, tc := range []struct { + name string + env map[string]string + wantErr bool + // wantIn is a substring the error must name, so that an operator can + // find the offending variable. + wantIn string + }{ + { + name: "nothing set", + env: map[string]string{}, + }, + { + name: "well formed URL", + env: map[string]string{"HTTP_PROXY": "http://proxy.example.com:3128"}, + }, + { + name: "credentials are accepted", + env: map[string]string{"HTTPS_PROXY": "http://user:password@proxy.example.com:3128"}, + }, + { + name: "bare host and port is accepted, as httpproxy prefixes a scheme", + env: map[string]string{"HTTP_PROXY": "proxy.example.com:3128"}, + }, + { + name: "socks5 is accepted", + env: map[string]string{"ALL_PROXY": "socks5://proxy.example.com:1080"}, + }, + { + name: "lowercase variables are checked too", + env: map[string]string{"http_proxy": "http://proxy.example.com:3128"}, + }, + { + name: "unparseable URL", + env: map[string]string{"HTTP_PROXY": "http://proxy.example.com:3128/%zz"}, + wantErr: true, + wantIn: "HTTP_PROXY", + }, + { + name: "the failing variable is named", + env: map[string]string{"HTTPS_PROXY": "://%zz"}, + wantErr: true, + wantIn: "HTTPS_PROXY", + }, + } { + t.Run(tc.name, func(t *testing.T) { + for _, name := range environmentProxyVariables { + t.Setenv(name, "") + } + for name, value := range tc.env { + t.Setenv(name, value) + } + + err := ValidateEnvironmentProxyVariables() + if tc.wantErr && err == nil { + t.Fatal("expected an error, got none") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if tc.wantIn != "" && !strings.Contains(err.Error(), tc.wantIn) { + t.Errorf("error %q does not name %q", err, tc.wantIn) + } + }) + } +} + +// TestValidateEnvironmentProxyVariables_DoesNotLeakCredentials pins that a +// proxy URL, which may embed a password, never reaches the error text. +func TestValidateEnvironmentProxyVariables_DoesNotLeakCredentials(t *testing.T) { + for _, name := range environmentProxyVariables { + t.Setenv(name, "") + } + t.Setenv("HTTP_PROXY", "http://admin:hunter2@proxy.example.com:3128/%zz") + + err := ValidateEnvironmentProxyVariables() + if err == nil { + t.Fatal("expected an error, got none") + } + if strings.Contains(err.Error(), "hunter2") { + t.Errorf("error leaks the proxy password: %q", err) + } + if strings.Contains(err.Error(), "admin") { + t.Errorf("error leaks the proxy username: %q", err) + } +} + +// TestNewOutboundHttpClient_EnvironmentProxyPinsDirectHops is the regression +// test for the dial-pinning gap: with the environment proxy enabled but no +// proxy applicable to the request, the dial must still go through the pinning +// dialer rather than a plain one. +// +// The request targets a hostname that only the stub resolver knows, so a plain +// dial would hand that unresolvable name to the OS and fail. Only a pinned dial, +// which substitutes the address resolved during validation, can connect. +// See https://github.com/gotenberg/gotenberg/issues/1592. +func TestNewOutboundHttpClient_EnvironmentProxyPinsDirectHops(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + + _, port, err := net.SplitHostPort(strings.TrimPrefix(srv.URL, "http://")) + if err != nil { + t.Fatalf("split server address: %v", err) + } + + const host = "pinned-only.invalid" + + // NO_PROXY covers the destination, so httpproxy declines it and the + // transport dials directly. That direct dial is the hop that used to lose + // pinning. + for _, name := range environmentProxyVariables { + t.Setenv(name, "") + } + t.Setenv("HTTP_PROXY", "http://proxy.invalid:3128") + t.Setenv("NO_PROXY", host) + + withStubResolver(t, func(string) ([]netip.Addr, error) { + return []netip.Addr{netip.MustParseAddr("127.0.0.1")}, nil + }) + + client := NewOutboundHttpClient(0, nil, nil, true) + rt, ok := client.Transport.(*outboundRoundTripper) + if !ok { + t.Fatalf("transport is %T, want *outboundRoundTripper", client.Transport) + } + if rt.proxyFunc == nil { + t.Fatal("proxyFunc is nil, want the environment proxy to be resolved per request") + } + + resp, err := client.Get("http://" + net.JoinHostPort(host, port)) + if err != nil { + t.Fatalf("GET failed, so the direct hop was not pinned: %v", err) + } + defer func() { + _ = resp.Body.Close() + }() + + if resp.StatusCode != http.StatusNoContent { + t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusNoContent) + } +} diff --git a/pkg/modules/api/api.go b/pkg/modules/api/api.go index 54423207..0cd1e904 100644 --- a/pkg/modules/api/api.go +++ b/pkg/modules/api/api.go @@ -380,6 +380,13 @@ func (a *Api) Validate() error { err = errors.Join(err, errors.New("IP must be a valid IP address")) } + if a.downloadFromCfg.enableEnvironmentProxy { + proxyErr := gotenberg.ValidateEnvironmentProxyVariables() + if proxyErr != nil { + err = errors.Join(err, fmt.Errorf("--api-download-from-enable-environment-proxy is set: %w", proxyErr)) + } + } + if (a.tlsCertFile != "" && a.tlsKeyFile == "") || (a.tlsCertFile == "" && a.tlsKeyFile != "") { err = errors.Join(err, errors.New("both TLS certificate and key files must be set"), diff --git a/pkg/modules/chromium/chromium.go b/pkg/modules/chromium/chromium.go index 5438b08d..967e7170 100644 --- a/pkg/modules/chromium/chromium.go +++ b/pkg/modules/chromium/chromium.go @@ -669,6 +669,13 @@ func (mod *Chromium) Validate() error { return fmt.Errorf("chromium-max-concurrency must be between 1 and 6, got %d", mod.maxConcurrency) } + if mod.args.enableEnvironmentProxy { + proxyErr := gotenberg.ValidateEnvironmentProxyVariables() + if proxyErr != nil { + return fmt.Errorf("--chromium-enable-environment-proxy is set: %w", proxyErr) + } + } + _, err := os.Stat(mod.args.binPath) if os.IsNotExist(err) { return fmt.Errorf("Chromium binary does not exist at %q; check the CHROMIUM_BIN_PATH environment variable: %w", mod.args.binPath, err) diff --git a/pkg/modules/libreoffice/api/api.go b/pkg/modules/libreoffice/api/api.go index ca6c9785..7bf282ea 100644 --- a/pkg/modules/libreoffice/api/api.go +++ b/pkg/modules/libreoffice/api/api.go @@ -522,6 +522,13 @@ func (a *Api) Validate() error { err = errors.Join(err, fmt.Errorf("unoconverter binary does not exist at %q; check the UNOCONVERTER_BIN_PATH environment variable: %w", a.args.unoBinPath, statErr)) } + if a.args.proxyOptions.enableEnvironmentProxy { + proxyErr := gotenberg.ValidateEnvironmentProxyVariables() + if proxyErr != nil { + err = errors.Join(err, fmt.Errorf("--libreoffice-enable-environment-proxy is set: %w", proxyErr)) + } + } + return err } diff --git a/pkg/modules/webhook/webhook.go b/pkg/modules/webhook/webhook.go index 85fdae81..ff036a40 100644 --- a/pkg/modules/webhook/webhook.go +++ b/pkg/modules/webhook/webhook.go @@ -1,6 +1,7 @@ package webhook import ( + "fmt" "sync/atomic" "time" @@ -107,10 +108,25 @@ func (w *Webhook) AsyncCount() int64 { return w.asyncCount.Load() } +// Validate checks the module's configuration. +func (w *Webhook) Validate() error { + if !w.enableEnvironmentProxy { + return nil + } + + err := gotenberg.ValidateEnvironmentProxyVariables() + if err != nil { + return fmt.Errorf("--webhook-enable-environment-proxy is set: %w", err) + } + + return nil +} + // Interface guards. var ( _ gotenberg.Module = (*Webhook)(nil) _ gotenberg.Provisioner = (*Webhook)(nil) + _ gotenberg.Validator = (*Webhook)(nil) _ api.MiddlewareProvider = (*Webhook)(nil) _ api.AsynchronousCounter = (*Webhook)(nil) )