Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: Add DialTLSContext to http2.Transport #50392

Closed
francislavoie opened this issue Dec 30, 2021 · 2 comments
Closed

x/net/http2: Add DialTLSContext to http2.Transport #50392

francislavoie opened this issue Dec 30, 2021 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@francislavoie
Copy link

francislavoie commented Dec 30, 2021

What version of Go are you using (go version)?

$ go version
go version go1.17.5 windows/amd64

Does this issue reproduce with the latest release?

Yes -- see https://pkg.go.dev/golang.org/x/net@v0.0.0-20211216030914-fe4d6282115f/http2#Transport which is missing a DialTLSContext field

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
Not relevant

What did you do?

Related issue caddyserver/caddy#4498

In Caddy, we're using a decently common pattern found in a few blog posts (for example https://www.mailgun.com/blog/http-2-cleartext-h2c-client-example-go/) to hack http2.Transport with a custom DialTLS function to skip actually doing TLS, so that we can make an H2C request to an upstream. We use this in the reverse_proxy module.

For non-H2C traffic, we use DialContext and grab the configured dial info from ctx. We do this because the upstream to dial is chosen dynamically, using load balancing selection policies etc.

We use a special config string <network>/<addr> (for example tcp/127.0.0.1:8080 or unix//path/to/unix.sock) so we can embed the network type in a single string.

A user reported that they want to use H2C with a unix socket. The issue is that the network type isn't retained, because the network/address is parsed on each request, and that would require the context so that we can grab a ctx.Value() with the current request's dial info... so it defaults to tcp instead.

We could hack in a thing to make sure the right network is used if the upstream is not dynamic, but that's... hacky and limits usecases.

The better fix would be to actually have access to the context via a new DialTLSContext field on the http2.Transport.

I expect this would have similar semantics to http.Transport's DialContext where if DialContext is non-nil, then it's used, otherwise Dial is used.

At a glance, this looks like it would a trivial change to func (t *Transport) dialTLS(ctx context.Context) right at the top of that function:

func (t *Transport) dialTLS(ctx context.Context) func(string, string, *tls.Config) (net.Conn, error) {
	if t.DialTLSContext != nil {
		return func(network, addr string, cfg *tls.Config) (net.Conn, error) {
			tlsCn, err := t.DialTLSContext(ctx, network, addr, cfg)
			return tlsCn, err
		}
	}
	...

But that might be a naive fix. I'm not comfortable enough with Go internals to attempt a patch.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 7, 2022
@cagedmantis cagedmantis added this to the Backlog milestone Jan 7, 2022
@cagedmantis
Copy link
Contributor

/cc @neild @tombergan

@dmitshur
Copy link
Contributor

Closing as a duplicate of #52114, which has now been resolved.

@dmitshur dmitshur closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2022
@golang golang locked and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants