feat: HTTP/1.1 CONNECT client #350
Conversation
x/httpproxy/connect_client.go
Outdated
There was a problem hiding this comment.
Could you please move the code to a new package x/httpconnect?
There was a problem hiding this comment.
Sure, done!
x/httpproxy/connect_client.go
Outdated
| return conn, nil | ||
| } | ||
|
|
||
| func (c *connectClient) sendConnectRequest(ctx context.Context, remoteAddr string, conn transport.StreamConn) error { |
There was a problem hiding this comment.
I was hoping to use the http.Client to avoid replicating logic, provide extra functionality, and keep it less tied to HTTP/1.1.
Does https://pkg.go.dev/net/http#Client.Do not work with a CONNECT method?
I think you may need https://pkg.go.dev/net/http#NewResponseController to enable full duplex and hijack the connection.
There was a problem hiding this comment.
A great insight!
http.Client does work with a CONNECT request.
A ResponseController can take over a server-side connection, but not a client-side one. However, I have found a way to achieve this with an http.Client using an io.Pipe.
x/httpproxy/connect_client.go
Outdated
|
|
||
| // connectClient is a [transport.StreamDialer] implementation that sends a CONNECT request | ||
| type connectClient struct { | ||
| endpoint transport.StreamEndpoint |
There was a problem hiding this comment.
You need to know the proxy address to pass to connect.
The connection RemoteAddress has the connected IP, it loses the domain information.
I suggest you pass the address and a dialer instead.
There was a problem hiding this comment.
A good idea, done
x/httpproxy/connect_client.go
Outdated
| endpoint transport.StreamEndpoint | ||
|
|
||
| // proxyAuth is the Proxy-Authorization header value. If empty, the header is not sent | ||
| proxyAuth string |
There was a problem hiding this comment.
Perhaps have a http.Header object here instead that gets merged into the request.
There was a problem hiding this comment.
Right, it is better to merge. Done
x/httpproxy/connect_client_test.go
Outdated
| connClient, err := NewConnectClient(endpoint, WithProxyAuthorization("Basic "+creds)) | ||
| require.NoError(t, err, "NewConnectClient") | ||
|
|
||
| streamConn, err := connClient.DialStream(context.Background(), host) |
There was a problem hiding this comment.
Can you write "Request" to the connection and expect the response to be "Response"? That will exercise the exchange as well, which has been an issue before.
There was a problem hiding this comment.
I am not sure what you are referring to. Could you, please, provide a small example?
If what we read is not a valid response, we should perform an early return with a non-nil error, should we not?
Upd. I have added a testcase for that unhappy path.
nikolaikabanenkov
force-pushed
the
feat-conn-client
branch
from
9869d62 to
2e46ac4
Compare
x/httpconnect/connect_client.go
Outdated
| // WithHeaders appends the given [headers] to the CONNECT request | ||
| func WithHeaders(headers http.Header) ConnectClientOption { | ||
| return func(c *connectClient) { | ||
| c.headers = headers |
There was a problem hiding this comment.
Can you copy the headers into the internal one instead? That is safer and lets the caller reuse the input headers
There was a problem hiding this comment.
A good point, done
x/httpconnect/connect_client.go
Outdated
|
|
||
| pr, pw := io.Pipe() | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodConnect, "http://"+remoteAddr, pr) |
There was a problem hiding this comment.
We may want to specify whether we want to use HTTP or HTTPS. Typically TLS has to be handled by the HTTP Client. But for now I think it's ok.
There was a problem hiding this comment.
Ok. I have added a TODO on that line
x/httpconnect/pipe_conn.go
Outdated
| ) | ||
|
|
||
| // PipeConn is a [transport.StreamConn] that overrides [Read], [Write] (and corresponding [Close]) functions with the given [reader] and [writer] | ||
| type PipeConn struct { |
There was a problem hiding this comment.
Sure, done
x/httpconnect/connect_client_test.go
Outdated
| targetURL, err := url.Parse(targetSrv.URL) | ||
| require.NoError(t, err) | ||
|
|
||
| proxySrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
You could use https://pkg.go.dev/github.com/Jigsaw-Code/outline-sdk/x@v0.0.0-20250110201030-80a3f247a159/httpproxy#NewConnectHandler instead of duplicating the logic. Though we don't have support for authorization there.
There was a problem hiding this comment.
Thanks for the suggestion! I have replaced the duplicating logic with a ConnectHandler
nikolaikabanenkov
force-pushed
the
feat-conn-client
branch
from
8768cb9 to
b9e4f2d
Compare
Hi,
The merge request adds an HTTP/1.1 CONNECT client, as described in #319
As for HTTP/2 support, I'm getting to know the HTTP/2 implementation in Go and will share an implementation later.
Sincerely,
Nikolai