fuzz: don't panic when the proxy closes the conn #986

hawkw · 2021-04-21T21:00:41Z

Currently, the inbound HTTP fuzz tests are failing. This is because they
reuse the test-support code for making an HTTP request to a proxy stack
and running the futures necessary to drive that request in the
background. This code currently unwraps both the JoinHandles of the
spawned tasks (which would be Err if the task panicked) and the
returned Result from those JoinHandles (which is an Err if the
Service::call future returned an error, or if the client returned an
error). If the future completes with an error, then the proxy simply
tears down the connection.

In the integration tests, we currently expect both of these
Results --- since the inputs are valid, we want to assert the proxy
doesn't return an error incorrectly. However, the fuzz tests can and
will generate malformed HTTP requests, and in this case, the proxy will
reject those requests by returning an error and closing the connection.
This is not incorrect behavior. Instead, we want to ensure that the
proxy doesn't panic in the face of potentially malformed requests.

This branch changes the test-support HTTP code to return the Result of
serving a request, and unwrap it in the integration tests rather than in
the support code. The fuzz logic is updated to simply log errors
returned here, since returning an error is expected behavior when we
receive invalid inputs.

If we wanted to be really fancy, a potential follow-up would be to
extend the fuzz logic to determine whether or not a fuzz spec should
result in an error, and assert that errors are only returned for invalid
requests...but, doing this without using any of the code that's being
exercised in the fuzz test (e.g. all of hyper's request parsing etc)
would be a lot of work...

Depends on #985

In order to make debugging fuzz test failures easier, it's useful to be able to get logs from the proxy and libraries during a fuzz run. However, because running the fuzz targets with a fuzzer will generate a massive number of imputs, we probably don't want verbose logs by default --- this would produce a huge amount of mostly useless data. This branch adds a simple tracing setup to the fuzz targets. When running without the `RUST_LOG` environment variable set, each run won't log anything. This means the current behavior on cluster-fuzz is unchanged and we won't output giant amounts of logs. However, when running a minimized reproducer locally for debugging, we can enable any amount of logging by setting `RUST_LOG`, which should make it easier to diagnose fuzz failures.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

linkerd/app/test/src/http_util.rs

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)

this is a follow-up commit related to 24dc5d8 (#3445). see <linkerd/linkerd2#8733> for more information on upgrading to hyper 1.0. --- this addresses hyper deprecations in the http/1 tests for the inbound proxy. prior, we made use of `tower::ServiceExt::oneshot`, which consumes a service and drops it after sending a request and polling the response future to completion. <https://docs.rs/tower/0.5.2/src/tower/util/oneshot.rs.html#96-100> tower is not a 1.0 library yet, so `SendRequest` does not provide an implementation of `tower::Service` in hyper's 1.0 interface: - <https://docs.rs/hyper/0.14.31/hyper/client/conn/struct.SendRequest.html#impl-Service%3CRequest%3CB%3E%3E-for-SendRequest%3CB%3E> - <https://docs.rs/hyper/1.5.1/hyper/client/conn/http1/struct.SendRequest.html#trait-implementations> consequentially, we must drop the sender ourselves after receiving a response now. --- this commit *also* addresses hyper deprecations in the http/1 downgrade tests for the inbound proxy. because these tests involve a http/2 client and an http/1 server, we take the choice of inlining the body of `http_util::connect_and_accept()` rather than introducing a new, third `http_util::connect_and_accept_http_downgrade()` function. we will refactor these helper functions in follow-on commits. NB: because `ContextError` is internal to the `linkerd-app-test` crate, we do not wrap the errors. these are allegedly used by the fuzzing tests (_see f.ex #986 and #989_), but for our purposes with respect to the inbound proxy we can elide them rather than making `ctx()` a public method. --- Signed-off-by: katelyn martin <kate@buoyant.io>

* chore(app/inbound): address hyper deprecations in http/1 tests this is a follow-up commit related to 24dc5d8 (#3445). see <linkerd/linkerd2#8733> for more information on upgrading to hyper 1.0. --- this addresses hyper deprecations in the http/1 tests for the inbound proxy. prior, we made use of `tower::ServiceExt::oneshot`, which consumes a service and drops it after sending a request and polling the response future to completion. <https://docs.rs/tower/0.5.2/src/tower/util/oneshot.rs.html#96-100> tower is not a 1.0 library yet, so `SendRequest` does not provide an implementation of `tower::Service` in hyper's 1.0 interface: - <https://docs.rs/hyper/0.14.31/hyper/client/conn/struct.SendRequest.html#impl-Service%3CRequest%3CB%3E%3E-for-SendRequest%3CB%3E> - <https://docs.rs/hyper/1.5.1/hyper/client/conn/http1/struct.SendRequest.html#trait-implementations> consequentially, we must drop the sender ourselves after receiving a response now. --- this commit *also* addresses hyper deprecations in the http/1 downgrade tests for the inbound proxy. because these tests involve a http/2 client and an http/1 server, we take the choice of inlining the body of `http_util::connect_and_accept()` rather than introducing a new, third `http_util::connect_and_accept_http_downgrade()` function. we will refactor these helper functions in follow-on commits. NB: because `ContextError` is internal to the `linkerd-app-test` crate, we do not wrap the errors. these are allegedly used by the fuzzing tests (_see f.ex #986 and #989_), but for our purposes with respect to the inbound proxy we can elide them rather than making `ctx()` a public method. --- Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/test): remove unused `http_util::connect_and_accept(..)` this removes `connect_and_accept(..)`. this will break fuzzing builds, but it is not used elsewhere. Signed-off-by: katelyn martin <kate@buoyant.io> * chore(fuzz): address hyper deprecation in inbound fuzz tests Signed-off-by: katelyn martin <kate@buoyant.io> * chore(fuzz): address preëxisting fuzz breakage this commit addresses other breakage found in the fuzz tests, tied to other previous work. after these changes, one can observe that the fuzz tests build and run once more by running the following: ```sh cargo +nightly fuzz run --fuzz-dir=linkerd/app/inbound/fuzz/ fuzz_target_1 ``` Signed-off-by: katelyn martin <kate@buoyant.io> * nit(fuzz): remove stray newline from manifest Signed-off-by: katelyn martin <kate@buoyant.io> --------- Signed-off-by: katelyn martin <kate@buoyant.io>

hawkw added 3 commits April 21, 2021 12:09

nicer debug impl for fuzz specs

41e6083

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

fuzz: don't panic when the proxy closes the conn

8720bc7

hawkw requested review from olix0r and a team April 21, 2021 21:00

Base automatically changed from eliza/fuzz-logs to main April 21, 2021 21:48

olix0r approved these changes Apr 21, 2021

View reviewed changes

linkerd/app/test/src/http_util.rs Outdated Show resolved Hide resolved

review feedback

1a8a6f9

hawkw merged commit 5dec0de into main Apr 21, 2021

hawkw deleted the eliza/fix-fuzz-panic branch April 21, 2021 22:42

olix0r mentioned this pull request May 12, 2021

proxy: v2.143.0 linkerd/linkerd2#6107

Merged

cratelyn mentioned this pull request Dec 13, 2024

chore(app/inbound): address hyper deprecations in http/1 tests #3454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuzz: don't panic when the proxy closes the conn #986

fuzz: don't panic when the proxy closes the conn #986

hawkw commented Apr 21, 2021

fuzz: don't panic when the proxy closes the conn #986

fuzz: don't panic when the proxy closes the conn #986

Conversation

hawkw commented Apr 21, 2021