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

fuzz: allow client requests to fail #989

Merged
merged 3 commits into from
Apr 26, 2021
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Apr 26, 2021

Currently, the inbound HTTP fuzz tests are failing when the fuzzer
generates invalid HTTP headers and the proxy rejects the request by
closing the connection:

Failing fuzz test logs:
Apr 26 10:06:12.697  INFO fuzz_target_1: running with input requests=[HttpRequestSpec { http_method: false, uri: "ookm$o", header_name: "o\"mooookm$o", header_value: " " }, HttpRequestSpec { http_method: false, uri: "o", header_name: "o", header_value: "" }]
Apr 26 10:06:12.701 DEBUG linkerd_proxy_http::server: Creating HTTP service version=Http1
Apr 26 10:06:12.701  INFO linkerd_app_test::http_util: connecting client with settings=Builder { exec: Exec, h09_responses: false, h1_title_case_headers: false, h1_read_buf_exact_size: None, h1_max_buf_size: None, h2_builder: Config { adaptive_window: false, initial_conn_window_size: 5242880, initial_stream_window_size: 2097152, max_frame_size: 16384, keep_alive_interval: None, keep_alive_timeout: 20s, keep_alive_while_idle: false }, version: Http1 }
Apr 26 10:06:12.701 DEBUG linkerd_proxy_http::server: Handling as HTTP version=Http1
Apr 26 10:06:12.702 DEBUG proxy: linkerd_proxy_http::server: The client is shutting down the connection res=Err(hyper::Error(Parse(Header)))
Apr 26 10:06:12.702 DEBUG proxy: linkerd_app_test::http_util: dropped server
Apr 26 10:06:12.702  INFO proxy: linkerd_app_test::http_util: proxy serve task complete res=Err(hyper::Error(Parse(Header)))
Apr 26 10:06:12.703  INFO client_bg: linkerd_app_test::http_util: Client background complete res=Ok(())
Apr 26 10:06:12.703  INFO http_request{request=Request { method: POST, uri: ookm$o, version: HTTP/1.1, headers: {"o\"mooookm$o": " "}, body: Body(Empty) }}: linkerd_app_test::http_util: rsp=Response { status: 400, version: HTTP/1.1, headers: {"content-length": "0", "date": "Mon, 26 Apr 2021 17:06:12 GMT"}, body: Body(Empty) }
Apr 26 10:06:12.703  INFO linkerd_app_inbound::http::fuzz_logic: rsp=Response { status: 400, version: HTTP/1.1, headers: {"content-length": "0", "date": "Mon, 26 Apr 2021 17:06:12 GMT"}, body: Body(Empty) }
thread '<unnamed>' panicked at 'Client must not fail: hyper::Error(ChannelClosed)', /home/eliza/Code/linkerd2-proxy/linkerd/app/test/src/http_util.rs:111:10
stack backtrace:
    ...

This is because the test-support HTTP request helper expects the client
request to never fail --- valid behavior for the tests where this is
used, but not for the fuzz logic. This branch changes these functions to
return Results so that the test that uses them can choose whether or
not to panic.

The test helper code currently uses expect messages to provide context
for where a particular error occurred (e.g. were we driving the client
to readiness, or actually making a request?). Changing the test helpers
to return Results would remove some of this context. Therefore, I
added a quick error wrapper type to wrap an error with a message,
allowing us to indicate where an error occurred while still returning a
Result that the test can choose to unwrap.

@hawkw hawkw requested a review from a team April 26, 2021 18:06
@hawkw hawkw merged commit 5e717fb into main Apr 26, 2021
@hawkw hawkw deleted the eliza/client-may-actually-fail branch April 26, 2021 21:44
olix0r added a commit to linkerd/linkerd2 that referenced this pull request May 12, 2021
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 added a commit to linkerd/linkerd2 that referenced this pull request May 18, 2021
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants