Skip to content

http_proxy: remove ambiguity in connect headers configuration#787

Merged
Choraden merged 8 commits intomainfrom
hg/connect_req_mod
Apr 25, 2024
Merged

http_proxy: remove ambiguity in connect headers configuration#787
Choraden merged 8 commits intomainfrom
hg/connect_req_mod

Conversation

@Choraden
Copy link
Contributor

@Choraden Choraden commented Apr 23, 2024

This patch:

  • renames --proxy-header to --connect-header as it was confusing
  • merges connect modifiers with the classical ones to streamline its usage in proxy
  • removes ambiguous ConnectRequestModifier from proxy

Fixes #782

@Choraden Choraden force-pushed the hg/connect_req_mod branch from ce03b1c to be99516 Compare April 23, 2024 12:10
@mmatczuk
Copy link
Contributor

mmatczuk commented Apr 24, 2024

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
Copy link
Contributor Author

Choraden commented Apr 24, 2024

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
Copy link
Contributor

mmatczuk commented Apr 24, 2024

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

@Choraden Choraden force-pushed the hg/connect_req_mod branch from 2e584e0 to 9bdfaad Compare April 24, 2024 14:38
@Choraden
Copy link
Contributor Author

Choraden commented Apr 24, 2024 •
edited
Loading

  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
Copy link
Contributor

mmatczuk commented Apr 25, 2024

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

mmatczuk
mmatczuk reviewed Apr 25, 2024
View reviewed changes
internal/martian/proxy_connect.go Outdated
d = dialvia.HTTPProxy(p.DialContext, proxyURL)
}
d.ConnectRequestModifier = p.ConnectRequestModifier
d.ConnectRequestModifier = func(req *http.Request) error {
Copy link
Contributor

@mmatczuk mmatczuk Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we invert it, set only if p.RequestModifier != nil

mmatczuk
mmatczuk reviewed Apr 25, 2024
View reviewed changes
dialvia/http.go Outdated
conn.Close()
return nil, nil, err
}
for k, v := range d.ProxyConnectHeader {
Copy link
Contributor

@mmatczuk mmatczuk Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now you can have maps.Copy from stdlib

@mmatczuk
Copy link
Contributor

mmatczuk commented Apr 25, 2024

LGTM

@mmatczuk mmatczuk self-requested a review April 25, 2024 12:36
Choraden added 8 commits April 25, 2024 15:45
@Choraden
command/run: rename proxy-header to connect-header
f8e43ee 
This patch is introducing a connect-header modifier that will be applied to all connect headers. This will streamline the logic.
Previous flag --proxy-headers didn't have clear use case (connects to upstream proxy).
@Choraden
command/run: merge connect and request headers in proxy config
879400d 
This allows to remove separate ConnectRequestModifier from proxy and always use RequestModifiers.
@Choraden
http_proxy: remove ConnectRequestModifier
673a959 
Connect requests will be modified by the very same RequestModifiers as all other requests.
@Choraden
martian: remove ConnectRequestModifier
cef541f 
Proxy should use RequestModifiers everywhere.
@Choraden
dialvia: replace ConnectRequestModifier with ProxyConnectHeader
e67a39c
@Choraden
martian: rewrite CONNECT headers to dialvia
156ca1d
@Choraden
command/run: set proxy connect header in http.Transport
ff9c6e1 
This allows adding custom headers from --connect-header flag to CONNECT requests in http.Transport.RoundTrip() (HTTP requests via HTTPS proxy).
@Choraden
e2e: add --connect-header flag test
8e62816
@Choraden Choraden force-pushed the hg/connect_req_mod branch from 9bdfaad to 8e62816 Compare April 25, 2024 13:49
@Choraden Choraden merged commit 20818b9 into main Apr 25, 2024
@Choraden Choraden deleted the hg/connect_req_mod branch April 25, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mmatczuk mmatczuk mmatczuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

http_proxy: remove ambiguity in connect headers configuration

2 participants

@Choraden @mmatczuk