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: h1_capture_fuzz with direct response #3787

Merged
merged 14 commits into from Aug 9, 2018

Conversation

Projects
None yet
3 participants
@anirudhmurali
Copy link
Member

anirudhmurali commented Jul 3, 2018

Title: fuzz: h1_capture_fuzz with direct response

Description:

h1_capture_fuzz_test as of now fuzzes both upstream and downstream. With direct response enabled, fuzzing happens only in the downstream. Overrided the initialize() of BaseIntegrationTest and supplied the config to enable direct response.

Risk Level: Low

Testing: Tested unit tests (bazel test //test/integration:h1_capture_direct_response_fuzz_test), built and ran fuzzers with oss-fuzz.

Currently using the same corpus as h1_capture_fuzz's, let me know if it's fine or modification to the corpus, like removing the upstream events, is needed.

Signed-off-by: Anirudh M m.anirudh18@gmail.com

initialize();
fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http"));
for (int i = 0; i < input.events().size(); ++i) {

This comment has been minimized.

@htuch

htuch Jul 3, 2018

Member

Can you refactor this with `h1_capture_fuzz_test to minimize code overlap? Ideally we have some configurable input event processing loop here, and can share corpus.

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Jul 3, 2018

@mattklein123 for shepherding.

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jul 5, 2018

@anirudhmurali please ping me when this is ready for another pass.

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 9, 2018

Minimised code overlap, the failing test is because of format check, running format fix moves h1_fuzz.h above all headers, which apparently causes bazel test fail, something because of the sequence in which importing of headers happen.
But when it's imported last, the bazel test passes, and format fails. Tried using include guards as well. Is it not because of header conflict?

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 9, 2018

It was not possible to share the same corpus in this type of file specification, however if I move the DEFINE_PROTO_FUZZER to h1_fuzz.cc, I could use one corpus, and the replay function would be shared, but if I override initialize(), the config applies on the whole corpus, that way, only direct response mode would be implemented. How do I go about sharing the corpus with one file then?

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jul 9, 2018

@anirudhmurali what compile error to do you get when the formatter moves the header file?

@htuch htuch self-assigned this Jul 9, 2018

srcs = ["h1_capture_fuzz_test.cc"],
srcs = [
"h1_capture_fuzz_test.cc",
"h1_fuzz.cc",

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

Rather than directly depending on these source files in multiple tests, you should have an envoy_cc_test_library that captures the h1_fuzz.{cc,h} build target.

void H1FuzzIntegrationTest::initialize() {
const std::string body = "Response body";
const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", body);
static const std::string domain("direct.example.com");

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

Why are these static?

static const std::string prefix("/");
static const Http::Code status(Http::Code::OK);
config_helper_.addConfigModifier(
[&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm)

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

You should explicitly enumerate the capture variables here, as per the Google C++ style guide.

void H1FuzzIntegrationTest::initialize() {
const std::string body = "Response body";
const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", body);
static const std::string domain("direct.example.com");

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

Is the test actually exercising the routes for direct.example.com? Or will it be sending the corpus data to the default route (which is not direct response)?

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Jul 9, 2018

@anirudhmurali as discussed on Slack, we probably don't want to move the DEFINE_PROTO_FUZZER into the common library code. Instead, you should modify the corpus = ... line in the envoy_cc_fuzz_target definitions to point to a common corpus directory.

@anirudhmurali anirudhmurali force-pushed the anirudhmurali:h1_capture_direct_response branch from 17b3727 to e94c74d Jul 10, 2018

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 10, 2018

Earlier tests were failing because of incorrect header import sequence, fixed it. BUILD is better now, yet to find a way to share the same corpus for both fuzz tests. Shall work on it.

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Jul 11, 2018

@anirudhmurali the structure looks better, but yeah, let's make sure we're sharing the corpus. Also, I'm still suspicious we're not exercising the direct response rule here, and using the default route instead. Can you provide evidence in the `-l trace`` that this is happening? If not, can you fix the test to do this? Thanks.

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 11, 2018

Update: Since sharing of corpus is not possible in current build.sh. PR has been sent to modify that: google/oss-fuzz#1607
Once that gets merged, shall send a commit here.

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 17, 2018

Two commits added, the first one asserts the response ensuring the file is being served by direct response, I believe the tests should break if the file isn't served, right?
Second commit, made both the fuzz tests to share the same corpus. Can be merged after the oss-fuzz PR is merged.

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Jul 17, 2018

@anirudhmurali in 09eecc2, you send a single request via the direct response path. This does not mean that the corpus is actually going through that route. There are two routes configured; the default one and the direct response route you add. You need to probably switch default over to direct to ensure rather than adding a new route, since the corpus will not have the required host headers to access the direct virtual host you added.

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 24, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Jul 24, 2018

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 31, 2018

Picking it up.

@stale stale bot removed the stale label Jul 31, 2018

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Aug 6, 2018

The trace is present at: https://pastebin.com/raw/hYCyPUEw
The configuration seems to have direct response set now, and the correct route is exercised now I believe, can you confirm it?

@mattklein123 mattklein123 removed their assignment Aug 6, 2018

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Aug 7, 2018

@anirudhmurali as mentioned on Slack, you are seeing direct response headers because you added them to the default route, and traffic is taking the default route, rather than the new virtual host you added with the direct response configuration. You should add the custom response headers to the new virtual host and observe them in the logs to confirm that the correct route is exercised.

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Aug 7, 2018

Have modified the default routes to have direct response enabled, the headers remain in the default route itself.
Trace of the same: https://pastebin.com/raw/wFHFrwf8

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Aug 7, 2018

You can see in the trace the following line:

[2018-08-06 20:31:08.914][32429][debug][router] source/common/router/router.cc:237] [C1][S2864627540746053274] cluster 'cluster_0' match for URL '/test/long/url'

This indicates its taking the default route, which is routing to a backend cluster_0, not the direct response route.

What you should probably do is replace auto* additional_route = hcm.mutable_route_config()->mutable_virtual_hosts(0)->add_routes(); with something like auto* default_route = hcm.mutable_route_config()->mutable_virtual_hosts(0)->mutable_routes(0); and override the default route rather than adding a new one.

@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Aug 8, 2018

Updated accordingly, the trace doesn't seem to take the cluster_0 backend now.
Trace: https://pastebin.com/raw/F1GH02XR

@htuch
Copy link
Member

htuch left a comment

Yep, this looks right now, awesome.

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Aug 8, 2018

@anirudhmurali looks like there is a merge conflict you need to fix.

anirudhmurali added some commits Jul 3, 2018

fuzz: h1_capture_fuzz with direct response
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
fuzz: minimized h1_fuzz code overlap
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>

anirudhmurali added some commits Jul 9, 2018

addressed failing tests
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
fixed broken tests, modified BUILD
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
added direct response evidence
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
h1 shares the same corpus
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
format fix
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
updated config initialization
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
format fix
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
addressed comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
fixed failing build & addressed comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
fixed format
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>

@anirudhmurali anirudhmurali requested review from lizan and zuercher as code owners Aug 8, 2018

@anirudhmurali anirudhmurali force-pushed the anirudhmurali:h1_capture_direct_response branch from 5dff4bf to 17155fa Aug 8, 2018

anirudhmurali added some commits Aug 8, 2018

fixed build
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
fixed incorrect comment
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch

htuch approved these changes Aug 9, 2018

@htuch htuch merged commit 14baa40 into envoyproxy:master Aug 9, 2018

12 checks passed

DCO All commits have a DCO sign-off from the author
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: filter_example_mirror Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details

vvinod1310 added a commit to vvinod1310/envoy that referenced this pull request Aug 12, 2018

fuzz: h1_capture_fuzz with direct response (envoyproxy#3787)
h1_capture_fuzz_test as of now fuzzes both upstream and downstream. With direct response enabled, fuzzing happens only in the downstream. Overrided the initialize() of BaseIntegrationTest and supplied the config to enable direct response.

Risk Level: Low

Testing: Tested unit tests (bazel test //test/integration:h1_capture_direct_response_fuzz_test), built and ran fuzzers with oss-fuzz.

Currently using the same corpus as h1_capture_fuzz's, let me know if it's fine or modification to the corpus, like removing the upstream events, is needed.

Signed-off-by: Anirudh M <m.anirudh18@gmail.com>

nickrmc83 added a commit to thales-e-security/envoy that referenced this pull request Aug 23, 2018

fuzz: h1_capture_fuzz with direct response (envoyproxy#3787)
h1_capture_fuzz_test as of now fuzzes both upstream and downstream. With direct response enabled, fuzzing happens only in the downstream. Overrided the initialize() of BaseIntegrationTest and supplied the config to enable direct response.

Risk Level: Low

Testing: Tested unit tests (bazel test //test/integration:h1_capture_direct_response_fuzz_test), built and ran fuzzers with oss-fuzz.

Currently using the same corpus as h1_capture_fuzz's, let me know if it's fine or modification to the corpus, like removing the upstream events, is needed.

Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment