saucelabs / forwarder

Forwarder is a production-ready, fast MITM proxy with PAC support. It's suitable for debugging, intercepting and manipulating HTTP traffic. It's used as a core component of Sauce Labs Sauce Connect Proxy.
https://forwarder-proxy.io
Mozilla Public License 2.0
202 stars 13 forks source link

http_proxy: remove ambiguity in connect headers configuration #787

Closed Choraden closed 2 months ago

Choraden commented 2 months ago

This patch:

Fixes #782

mmatczuk commented 2 months ago

I think that we lost the ability to set the connect headers with this patch. The only place where d.ProxyConnectHeader is set is in.

    if id := req.Header.Get(p.RequestIDHeader); id != "" {
        h := make(http.Header)
        h.Set(p.RequestIDHeader, id)
        d.ProxyConnectHeader = h
    }

Please add an e2e test for this flag.

Choraden commented 2 months ago

I think that we lost the ability to set the connect headers with this patch.

Yes, we did and I did it on purpose. It turns out we can't reuse regular RequestHeaders as they contain basic auth and some other modifiers that should not apply to dialvia's CONNECT request. In order to do that we would need to add some extra logic, which would definitely increase complexity. The primary reason to modify proxy connect headers was to add request-id which now we do. At this point I'm wondering if trading code simplicity and readability for having this feature is worth it.

WDYT? @mmatczuk

mmatczuk commented 2 months ago

AFAIMC the only mussing bit is to copy the processed headers if any to dialvia in martian.

Choraden commented 2 months ago
  1. ProxyConnectHeader in dialvia is now a full clone of headers from received CONNECT request
  2. set http.Transport.GetProxyConnectHeader to specified --connect-header flag
  3. added e2e tests

~I'm not sure, if we really need to support case nr 2.~ As you can see in tests it really takes effort to achieve it. One needs to send http request with X-Forwarded-Proto header explicitly set to https.

EDIT: MITM is a valid use case. After the tls is terminated, forwarder receives https requests that issue CONNECTs via http.Transport.

mmatczuk commented 2 months ago

I agree that --connect-headers are better name can we have --proxy-headers as deprecated alias for backwards compatibility.

mmatczuk commented 2 months ago

LGTM