Fixed HTTP/2 support for the proxy middleware

[lhecker]

http.Transport instances whose TLSClientConfig, Dial, or DialTLS field is non-nil will be configured without HTTP/2 support by default.

This PR adds the proper calls to http2.ConfigureTransport() everywhere a http.Transport is created and thus fixes HTTP/2 in the proxy middleware whenever insecure_skip_verify or keepalive is provided.

[CLAassistant]

All committers have signed the CLA.

[mholt]

http.Transport instances whose TLSClientConfig, Dial, or DialTLS field is non-nil will be configured without HTTP/2 support by default.

Are you sure? I read:

The http package has transparent support for the HTTP/2 protocol when using HTTPS. Programs that must disable HTTP/2 can do so by setting Transport.TLSNextProto (for clients) or Server.TLSNextProto (for servers) to a non-nil, empty map.
...
// If TLSNextProto is nil, HTTP/2 support is enabled automatically.

(net/http docs)

To me this says that unless we specify TLSNextProto manually to something that doesn't include "h2", http/2 will be enabled automatically.

[lhecker]

Are you sure?
[...]
To me this says that unless we specify TLSNextProto manually to something that doesn't include "h2", http/2 will be enabled automatically.

Yes 100% sure. That statement is not true anymore since Go 1.7 due to golang/go#14275.

I noticed this bug, because I was trying to proxy gRPC calls, which for some reason simply didn't work. At all. I then used GODEBUG=http2debug=1 and realized that no HTTP2 connections where made and thus found this bug.

[lhecker]

@mholt One more thing (partially off topic): Shouldn't we also possibly switch from using (*net.Dialer).Dial to the newer (*net.Dialer).DialContext? The former one is deprecated and doesn't support context.Context, which might get useful sooner or later I imagine…

[mholt]

Oh wow, I hadn't seen that code change. Thanks for finding that. I guess the docs are lagging behind a little bit (Caddy's do too, so ... haha oh well).

One more thing (partially off topic): Shouldn't we also possibly switch from using (*net.Dialer).Dial to the newer (*net.Dialer).DialContext? The former one is deprecated and doesn't support context.Context, which might get useful sooner or later I imagine…

I don't see that Dial() is deprecated, but I do plan on introducing Context to Caddy very soon, like for identifying HTTP requests. I think we could switch to using DialContext any time to just have the context available, I'm okay with that.

[mholt]

Is there an easy way to test HTTP/2 is working? In the proxy tests, we have a "fake" upstream that responds to requests (like this one) -- we could add a test case that checks the protocol on the request to make sure that it's HTTP/2 I think. If that's doable, would you please add that to this PR? Then I'll merge it in.

[lhecker]

wendigo and others added some commits 24 minutes ago

Holy... What the heck is that? I did not fetch upstream commits into my fork and still... There they are. My fork contains 2 new commits from you guys I've never seen before. How did you do that @mholt? Or was that "GitHub's" doing?

---

Anyways: I modified the proxy test accordingly. It took me far too long to figure out why my modifications to that "httptest" package don't work/apply, until I noticed that "httptest" is a Go-package. 😐 I thus added a simple wrapper which overwrites the NextProtos field.

[wendigo]

Sorry, I've updated your branch with upstream changes via Github "Update branch" (out of habit)

[mholt]

Looks great - thank you!! Going out in a release today I hope.

[lhecker]

@mholt I fear I might have broken something inadvertently. Since connections are now made using HTTP/2, WebSockets won't work anymore.

In my case my backend logs:

http2: server: error reading preface from client 127.0.0.1:21052: bogus greeting "GET /..."

I thought I did it correctly, because for WebSockets your code calls newConnHijackerTransport right? I didn't add the call to http2.ConfigureTransport() due to that, but it seems it it still uses HTTP/2.

Or was this broken before too?

Edit: Nope. I broke shit. Confirmed. ✅
How do I fix that now? 😱

[mholt]

@lhecker Strange, the tests are passing... what kind of configuration breaks web sockets?

[lhecker]

@mholt None of your WebSocket proxy tests use a HTTP/2 enabled backend, which is why the tests pass. You can try it yourself by replacing all occurrences of httptest.NewServer with newTLSServer in proxy_test.go.
I'm going to try and find the cause for this now, but since I'm mostly unfamiliar with caddy I'm not sure if I'll be able to solve it by myself.

[mholt]

@lhecker I can perhaps help guide you :) The first step would be to get the tests to fail, as you've explained how to do (add a test case instead of replacing one, though).

I just remembered there's a -http2 command line flag that your code currently does not obey. See this line of code in the HTTP server which disables HTTP/2 (if disabled with the flag) for client connections. You simply need to not configure HTTP2 if that flag is set to false for upstream connections.

However, this'll disable HTTP/2 entirely but users may want HTTP/2 for client connections still. You'll need to find a way to disable HTTP/2 for the ReverseProxy for websocket connections. Maybe the websocket preset needs to set one more (new) configuration parameter. Does that make sense?

[lhecker]

Hm… I'm not entirely sure if I understood that… @mholt

• So you basically want a (e.g.) http2 configuration parameter for proxy, which defaults to the -http2 CLI flag, right?
• I don't think websocket should set the http2 parameter to false. IMO that would just make proxying HTTP/2-only connections (e.g. gRPC) and HTTP/1-only connections (WebSockets) more cumbersome than necessary. I think WebSockets should just continue working as they do now (by checking the HTTP request headers), but transparently create an unpooled HTTP/1.1 connection regardless of the http2 setting.

[mholt]

• The -http2 flag should forcefully disable HTTP/2 entirely, including in upstream connections.
• I agree with you on the second point, as long as it obeys the -http2 flag.

[lhecker]

@mholt Alright I figured out how to modify the fake WS upstream to proxy a self-signed wss:// stream. And it does fail with the above http2 bogus greeting error! I hope I'll be done by tomorrow.

[lhecker]

@mholt I'm stuck again. I added support for switching off HTTP/2 using -http2=false, before fixing the bigger issue here. When testing it I noticed that connections to the wss: backend still fail, even though HTTP/1.1 is now being used! I believe I hit #1292.

The error is:

websocket.Dial wss://127.0.0.1:60244: malformed HTTP response "\x16\x03\x03\x001\x02\x00\x00-\x03\x03>\x91\xbct\xfc\x851\xdcr\x81A\x95\x0f\xd3d]*.\x19,\xee'\x90\xa6O\xda\xf7\xf4\x90\r6S\x00\xc0/\x00\x00\x05\xff\x01\x00\x01\x00\x16\x03\x03\x02!\v\x00\x02\x1d\x00\x02\x1a\x00\x02\x170\x82\x02\x130\x82\x01|\xa0\x03\x02\x01\x02\x02\x100\x83\x02\x84\xc2ƭ\x1f\x90\xbed/\xa7\x00\x14\xeb0\r\x06\t*\x86H\x86\xf7\r\x01\x01\v\x05\x000\x121\x100\x0e\x06\x03U\x04"

(Only the first dozen or so bytes consistently stay the same though.)

To me this looks like a raw TLS 1.2 handshake (which AFAIK starts with \x16\x03\x03) from the wss: backend isn't decrypted, but directly wrapped in a HTTPS response by the proxy and then fed to the wss client. The client decrypts the first layer but then of course still has raw TLS data in its hands. Again this sounds very similar to #1292 for me. I guess this is at fault, because it's not using a tls.Conn?

[mholt]

Ahh, you may be right! Can you investigate that further and see if you can find a simple change to fix it? (And if you come to my neck of the woods sometime, lunch is on me.)

[lhecker]

@mholt Thanks! 🙂

I didn't make any further progress today though… As far as I understand this it's trying to cleverly "replay" the response (headers) from the backend to the client, right? I can't for my life figure out why it doesn't properly handle the TLS data though.

If you have some time left today it'd be nice if you could take a look ¹, since it proved to be a bit more complex for me personally than I thought initially (I'm new to Go! 😭). Otherwise I'll try to further debug it in the coming days, but of course I don't know how much time I can spend on it (due to Christmas).

¹ You can check out the new quick & dirty wss: test case as a PoC in my fork in the proxy folder using go test -run=TestWebSocketReverseProxyFromWSSClient.

[mholt]

@lhecker Sorry, been a bit occupied with the holidays as well. :) Hope you're having a Merry Christmas!

Yes, your understanding of the replay seems right, I think. The replay is there to avoid multiple/duplicate connections to the websocket backend. See #763 and #987.

I'll take a look after the holidays as well, thanks so much for looking at this! I think you're making great progress. Definitely faster than I would have...

[lhecker]

@mholt After I submitted my other comment just now I immediately had a hunch how to solve this one. It's now fixed. lol
This is at fault. If we use the baseTransport.TLSClientConfig instance we forget to explicitly make sure that NextProtos does not contain h2! I'm now simply setting NextProtos to nil, causing Go to not use HTTP/2 for the hijacker. It works! 🙂

A PR is coming soon.