Skip to content

http: pausing when proxying CONNECT requests for security reasons#10974

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
alyssawilk:pause
Apr 30, 2020
Merged

http: pausing when proxying CONNECT requests for security reasons#10974
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
alyssawilk:pause

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

Risk Level: Low (only affects CONNECT proxying, not yet used)
Testing: new unit tests, modified integration tests
Docs Changes: n/a
Release Notes: n/a
Part of #1630 #1451

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@mattklein123 mattklein123 self-assigned this Apr 28, 2020
@alyssawilk
Copy link
Copy Markdown
Contributor Author

CI failure is integration:quic_http_integration_test
I'll look into deflaking that.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM with small comment.

/wait

Comment on lines +397 to +398
const bool end_stream = !buffered_request_body_ && encode_complete_ && !encode_trailers_;
const bool delay_headers_end_stream = end_stream && !downstream_metadata_map_vector_.empty();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is pretty specific/confusing/copied from above. Small helper that returns a struct/tuple/something with comments that can be shared with above?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the last little cleanup, much easier to understand.

@mattklein123 mattklein123 merged commit 3750bc9 into envoyproxy:master Apr 30, 2020
@alyssawilk alyssawilk deleted the pause branch August 27, 2020 16:33
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