admin: Assume meshed connections are HTTP/2 #982

olix0r · 2021-04-20T19:45:14Z

We've recently updated the admin server to assume that protocol
detection timeouts indicate that the client was just HTTP/1; but this
will almost never be true for meshed connections, which are transported
over HTTP/2.

This change updates the fallback logic to account for this. Detection
timeouts on meshed connections are now assumed to be HTTP/2. When we get
TLS connections that we can't terminate, we now return an error message
that includes the SNI value to more clearly indicate that the client
expected an alternate server identity.

We've recently updated the admin server to assume that protocol detection timeouts indicate that the client was just HTTP/1; but this will almost never be true for meshed connections, which are transported over HTTP/2. This change updates the fallback logic to account for this. Detection timeouts on meshed connections are now assumed to be HTTP/2. When we get TLS connections that we can't terminate, we now return an error message that includes the SNI value to more clearly indicate that the client expected an alternate server identity.

kleimkuhler

One nit but otherwise looks good 👍

kleimkuhler · 2021-04-20T19:49:45Z

linkerd/app/src/admin.rs

+                                    return Err(Error::from(UnexpectedSni(sni, tcp.client_addr)));
+                                }
+                            };
+                            debug!(%version, "HTTP detection timed out; assuming HTTP");


Maybe be a little more specific?

Suggested change

debug!(%version, "HTTP detection timed out; assuming HTTP");

debug!(%version, "HTTP detection timed out; assuming HTTP/2");

not necessarily true, which is why we log the version field -- it will Http1 if no identity was present

tls::ConditionalServerTls::None(_) => http::Version::Http1,

This release primarily improves protocol detection error messages in the admin server so that logs clearly indicate when the client expected a different TLS server identity than that of the running proxy. A number of internal improvements have been made, especially eliminating some potential runtime panics detected by oss-fuzz. It is not expected that these panics could be triggered in typical cluster configurations. --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983)

We've recently updated the admin server to assume that protocol detection timeouts indicate that the client was just HTTP/1; but this will almost never be true for meshed connections, which are transported over HTTP/2. This change updates the fallback logic to account for this. Detection timeouts on meshed connections are now assumed to be HTTP/2. When we get TLS connections that we can't terminate, we now return an error message that includes the SNI value to more clearly indicate that the client expected an alternate server identity.

This release simplifies internals so that endpoint-forwarding logic is completely distinct from handling of load balanced services. The ingress-mode outbound proxy has been simplified to *require* the `l5d-dst-override` header and to fail non-HTTP communication. This ensures that the ingress-mode proxy does not unexpectedly revert to insecure communication. Finally, a regression was recently introduced that caused all proxy logs to be output with ANSI control characters. Logs are now output in plaintext by default --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983) * fuzz: Add logging to fuzz targets (linkerd/linkerd2-proxy#985) * fuzz: don't panic when the proxy closes the conn (linkerd/linkerd2-proxy#986) * Commit lock files for fuzzers (linkerd/linkerd2-proxy#984) * fuzz: allow client requests to fail (linkerd/linkerd2-proxy#989) * tower: update dependency to 0.4.7 (linkerd/linkerd2-proxy#990) * outbound: Make the Endpoint::logical_addr field optional (linkerd/linkerd2-proxy#991) * trace: explicitly disable ANSI terminal colors (linkerd/linkerd2-proxy#994) * ingress: Require the l5d-dst-override header (linkerd/linkerd2-proxy#992) * outbound: Do not support TCP-forwarding in ingress-mode (linkerd/linkerd2-proxy#995) * Decouple tcp forward stack from Endpoint target (linkerd/linkerd2-proxy#996) * Pickup linkerd-await wrapper in docker build (linkerd/linkerd2-proxy#999) * docs: Add fuzzing report (linkerd/linkerd2-proxy#1000) * tests: Use io::Error in mocked connector (linkerd/linkerd2-proxy#1001) * outbound: Decouple endpoint & logical stacks (linkerd/linkerd2-proxy#1002)

olix0r requested a review from a team April 20, 2021 19:45

kleimkuhler approved these changes Apr 20, 2021

View reviewed changes

olix0r merged commit 832bb85 into main Apr 20, 2021

olix0r deleted the ver/admin-h2-meshed branch April 20, 2021 22:47

olix0r mentioned this pull request Apr 21, 2021

proxy: v2.142.0 linkerd/linkerd2#6047

Closed

olix0r mentioned this pull request May 12, 2021

proxy: v2.143.0 linkerd/linkerd2#6107

Merged

cpretzer mentioned this pull request May 20, 2021

Release notes for edge-21.5.2 linkerd/linkerd2#6145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin: Assume meshed connections are HTTP/2 #982

admin: Assume meshed connections are HTTP/2 #982

olix0r commented Apr 20, 2021

kleimkuhler left a comment

kleimkuhler Apr 20, 2021

olix0r Apr 20, 2021

	debug!(%version, "HTTP detection timed out; assuming HTTP");
	debug!(%version, "HTTP detection timed out; assuming HTTP/2");

admin: Assume meshed connections are HTTP/2 #982

admin: Assume meshed connections are HTTP/2 #982

Conversation

olix0r commented Apr 20, 2021

kleimkuhler left a comment

Choose a reason for hiding this comment

kleimkuhler Apr 20, 2021

Choose a reason for hiding this comment

olix0r Apr 20, 2021

Choose a reason for hiding this comment