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

Consul Connect SDK HTTPClient #4466

Closed
nicholasjackson opened this issue Jul 30, 2018 · 6 comments
Closed

Consul Connect SDK HTTPClient #4466

nicholasjackson opened this issue Jul 30, 2018 · 6 comments
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies type/bug Feature does not function as expected

Comments

@nicholasjackson
Copy link
Contributor

When attempting to make a connection to an upstream, the provided HTTPClient attempts to connect using HTTP2 when the downstream is only HTTP/1.1 with no TLS connection.

I believe this is because the HTTP2 is negotiated in the handshake phase of the connection, the proxy is responding that it has ALPN capabilities and therefore the client will use this when attempting to send a request upstream.

However the problem is that upstream service in this instance is not using HTTP2 and is only HTTP/1.1 with no TLS, the protocol has been negotiated with the proxy not the upstream. When a request is sent to the upstream it is sent as a HTTP2 request the upstream server will reject this as it is not supported.

Reproduction Steps

Steps to reproduce this issue, eg:

  1. Run upstream http service with managed proxy
  2. Use Connect SDK to obtain HTTPClient as defined in the docs
	r.service, err = connect.NewService("connect-router", r.consulClient)
	if err != nil {
		return err
	}
	defer r.service.Close()

	// Get an HTTP client
	r.httpClient = r.service.HTTPClient()
  1. Attempt to make a request to the upstream using the HTTPClient

Since Go does not support HTTP2 without TLS and it is not possible to use HTTPS upstream from the SDK as there is no way to manually configure self signed TLS roots. A quick fix would be to disable HTTP2 support in the SDK.

A long term fix would be to correctly report ALPN status based on the upstreams ability to speak HTTP2 or to have this as a manual configuration option when configuring a proxy. In either case configuration would need to be enabled for TLS roots in the HTTP client or allow insecure based on the assumption that the connection between the proxy and service over localhost is OK.

Operating system and Environment details

Tested on Darwin however behavior covers all environments

Log Fragments

Below is the output from ngrep using the command sudo ngrep -W byline -d all port 8087 or 8443

The proxy is running on port 8443 and the upstream service 8087

T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #9
............18C.vzB_i.~.../0+,̨̩...R3t............
.
.................................................h2.http/1.1....
#
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #10
............18C.vzB_i.~.../0+,̨̩...R3t............
.
.................................................h2.http/1.1....
###########
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #21
....:...6..Jgn2.Ub!.pH..h...\..+..............h2.............0...0..;......F
..*.H...0.1.0...U....Consul CA 70...180730155402Z..180802155402Z0.1.0...U....http-echo0Y0...*.H....*.H....B..i.f.^
.q......A.Kx.y~=T|...[...ξ...x0..t0...U.........0...U.%..0...+.........+.......0...U......0.0h..U...a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450j..U.#.c0a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450_..U...X0V.Tspiffe://2396e39c-2f35-2c5b-540a-8085f424d671.consul/ns/default/dc/dc1/svc/http-echo0
..*.H....I.0F.!.>\..$.te..z^ .$t:!.!...|.f.m.|ݠ...]..gq....t...p... j.! '.*pY].u&*WK..uWCr4...H0F.!.ί`cT.7.+.T.o8xC.=S.!.⩢xs.2./[email protected] CA 7.........
#
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #22
....:...6..Jgn2.Ub!.pH..h...\..+..............h2.............0...0..;......F
..*.H...0.1.0...U....Consul CA 70...180730155402Z..180802155402Z0.1.0...U....http-echo0Y0...*.H....*.H....B..i.f.^
.q......A.Kx.y~=T|...[...ξ...x0..t0...U.........0...U.%..0...+.........+.......0...U......0.0h..U...a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450j..U.#.c0a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450_..U...X0V.Tspiffe://2396e39c-2f35-2c5b-540a-8085f424d671.consul/ns/default/dc/dc1/svc/http-echo0
..*.H....I.0F.!.>\..$.te..z^ .$t:!.!...|.f.m.|ݠ...]..gq....t...p... j.! '.*pY].u&*WK..uWCr4...H0F.!.ί`cT.7.+.T.o8xC.=S.!.⩢xs.2./[email protected] CA 7.........
###
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #25
...........0...0..E......F
..*.H...0.1.0...U....Consul CA 70...180730155442Z..180802155442Z0.1.0...U....connect-router0Y0...*.H....*.H....B..EWb.;:<x.#M.T..г۱!S?)'....).Bh.W.AeR{.4ӱ...}0..y0...U.........0...U.%..0...+.........+.......0...U......0.0h..U...a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450j..U.#.c0a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450d..U...]0[.Yspiffe://2396e39c-2f35-2c5b-540a-8085f424d671.consul/ns/default/dc/dc1/svc/connect-router0
..*.H....H.0E. cs.>kw..Tn.b"..-.!.پ..Q.9...kk'i>.2pr'....%...! T>c..%0...w).*Z.6+s.....O...K...G0E. N...
".;t=K...m,p_.!.4....j.p.Y.4.;L?./............(........XQᢥݼc.&....;...u.*.R
#
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #26
...........0...0..E......F
..*.H...0.1.0...U....Consul CA 70...180730155442Z..180802155442Z0.1.0...U....connect-router0Y0...*.H....*.H....B..EWb.;:<x.#M.T..г۱!S?)'....).Bh.W.AeR{.4ӱ...}0..y0...U.........0...U.%..0...+.........+.......0...U......0.0h..U...a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450j..U.#.c0a._86:0f:9d:a2:2a:2a:b3:60:7b:0a:64:1d:f7:c5:97:c3:f4:b6:72:28:17:31:ad:d8:5a:82:d0:b8:d8:b5:d0:450d..U...]0[.Yspiffe://2396e39c-2f35-2c5b-540a-8085f424d671.consul/ns/default/dc/dc1/svc/connect-router0
..*.H....H.0E. cs.>kw..Tn.b"..-.!.پ..Q.9...kk'i>.2pr'....%...! T>c..%0...w).*Z.6+s.....O...K...G0E. N...
".;t=K...m,p_.!.4....j.p.Y.4.;L?./............(........XQᢥݼc.&....;...u.*.R
###
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #29
..........(........LUS&..)*..T5...X
#
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #30
..........(........LUS&..)*..T5...X
###
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #33
....X........k..:.sڡm+.ӥ0...s.l..b.4N=..}J.S...+.y{h-k..M*`..
                                                             #
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #34
....X........k..:.sڡm+.ӥ0...s.l..b.4N=..}J.S...+.y{h-k..M*`..
                                                             ###
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #37
....d...........,. yҧ[email protected].\2..9..O~.;.Op.q.7..A._n.0.%..
                                                        #
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #38
....d...........,. yҧ[email protected].\2..9..O~.;.Op.q.7..A._n.0.%..
                                                        ###
T 127.0.0.1:51990 -> 127.0.0.1:8087 [AP] #41
PRI * HTTP/2.0.
.
SM.
.
..................@................@...
#
T 127.0.0.1:51990 -> 127.0.0.1:8087 [AP] #42
PRI * HTTP/2.0.
.
SM.
.
..................@................@...
#
T 127.0.0.1:51990 -> 127.0.0.1:8087 [AP] #43
..C......A..)b.s.H....@.򴧳-)[::1]:51987z.%PëS.*/*P..٫
##
T 127.0.0.1:51990 -> 127.0.0.1:8087 [AP] #45
..C......A..)b.s.H....@.򴧳-)[::1]:51987z.%PëS.*/*P..٫
####
T 127.0.0.1:8087 -> 127.0.0.1:51990 [AP] #49
HTTP/1.1 400 Bad Request.
Connection: close.
Date: Mon, 30 Jul 2018 18:40:39 GMT.
Content-Length: 0.
Content-Type: text/plain; charset=utf-8.
.

#
T 127.0.0.1:8087 -> 127.0.0.1:51990 [AP] #50
HTTP/1.1 400 Bad Request.
Connection: close.
Date: Mon, 30 Jul 2018 18:40:39 GMT.
Content-Length: 0.
Content-Type: text/plain; charset=utf-8.
.

#######
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #57
..............b.زԸD
.cjoS^..W_...-z".1.ĪdJ...)..(.PGr8,.!2Y!e...Ax.<x;IWǽ.=..6...iW145..pA/...~..
#
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #58
..............b.زԸD
.cjoS^..W_...-z".1.ĪdJ...)..(.PGr8,.!2Y!e...Ax.<x;IWǽ.=..6...iW145..pA/...~..
#
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #59
..............pި..`ME.л
##
T 192.168.192.131:8443 -> 192.168.192.131:51989 [AP] #61
..............pި..`ME.л
############
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #73
.................7r.1PZV.
#
T 192.168.192.131:51989 -> 192.168.192.131:8443 [AP] #74
.................7r.1PZV.
@banks
Copy link
Member

banks commented Jul 31, 2018

The root cause here is that our built-in proxy is advertising h2 support in it's TLSConfig when in fact the proxy itself doesn't support HTTP2 at all.

To be clear it's a Layer 4 proxy so it makes no sense to advertise support for a L7 protocol, not a limitation of the proxy implementation. If the services either end of two proxies use http2 then our proxy should remain oblivious and just forward the packets over it's own TLS - this is what happens right now if you use http2 client and server which are both talking to a Connect proxy it works fine. TLS-in-TLS is perhaps not ideal but it's workable and not something we can solve when only proxying layer 4.

The actual bug here is that when the client end is a native integration, and the server end is our built-in proxy, then the following happens:

  • http.Client gets it's client cert from our SDK
  • it Dials the remote service via our custom Dialer
  • The proxy responds with a TLS config that includes NextProtos: ["h2"]
  • So http.Client starts an HTTP2 connection - sending binary h2 protocol over the TLS connection to the proxy
  • The proxy is only acting as an L4 proxy and terminates the TLS, forwarding the raw h2 binary over TCP to the destination service
  • Destination service, even if it does support http2, probably doesn't support h2c (http2 without TLS) and even if it does probably wasn't configured that way as it was unexpected and so it tries to interpret the http2 binary as HTTP/1.1 and fails.

The real fix here is that our proxy should not advertise h2 next proto. The reason it does is that it just uses our SDK which we explicitly built to support h2 servers. We need to make it configurable for server that it doesn't support that and then configure our proxy not to add "h2".

Once this is done the above example would look like:

  • http.Client gets it's client cert from our SDK
  • it Dials the remote service via our custom Dialer
  • proxy responds with a handshake but no h2 support
  • http.Client sends HTTP/1.1
  • proxy terminates TLS and forwards it
  • destination service handles HTTP/1.1 just fine.

This gets us back to a non-broken state, but it does mean that different combinations of proxy/native client and server will have varying http2 support.

Later we can consider adding support for http2 proxying end to end by having the proxy establish connections to the service first and discover whether it supports http2. The downside is that for now the go http.Client only supports http2 over TLS which means that to actually work this would need the app to be configured for TLS separately from connect already. At that point it's likely better to just natively integrate.

We could also consider making it possible to re-enable the current behaviour in the proxy config such that, if you know your application will support h2c, the proxy can still terminate the TLS and pass raw http2 on to the server. This is the case for gRPC services by default (when not provided explicit TLS creds) for example.

As an aside, an issue I opened years ago to add h2c support to net/http was merged last week so it's possible that a near-future go release will have first class support for h2c servers and clients which might change the options here a little.

tl;dr

The right solution for now is to make h2 nextProto support optional in the SDK and then make the built-in proxy not advertise it.

@pearkes pearkes added type/bug Feature does not function as expected theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies labels Aug 1, 2018
@hanshasselberg
Copy link
Member

This is no longer an issue because the builtin proxy is no longer part of consul.

@hanshasselberg
Copy link
Member

Well, the managed proxies were removed, but not the builtin proxy. Sorry!

@apollo13
Copy link
Contributor

Any chance that this could get fixed? I am running into this currently. Envoy handles it properly but for testing the simpler builtin proxy is often enough.

@apollo13
Copy link
Contributor

@banks

The right solution for now is to make h2 nextProto support optional in the SDK and then make the built-in proxy not advertise it.

If you could tell me how you'd want the interface to look like, I can add that I guess. Add enableH2Support to all functions as needed? Or should it be a list of ALPN next protos?

@banks
Copy link
Member

banks commented Mar 23, 2021

@apollo13 honestly given that we don't recommend using the built in proxy any more I'm not sure if we'll get to prioritizing this super soon.

The ideal fix I think would be something like:

  1. NewServiceWithConfig (in connect/service.go) which takes a config struct (this doesn't exist yet but could be defined) - that could accept the logger dependency, client dependency etc. and so be called by the other NewService* functions.
  2. Add a field like ServerNextProtos which defaults to ["h2"] but can be configured. Doc comment could link to this issue to help users decide if their case should support h2 or not. (essentially: probably yes unless you are building an L4 proxy)
  3. Set that to empty on the built in proxy in connect/proxy/proxy.go Serve method.

If that's something you'd be willing to contribute that would be welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants