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

uhv: add http-parser permissive parsing for h1 codec #20872

Merged
merged 6 commits into from
Apr 29, 2022

Conversation

ameily
Copy link
Contributor

@ameily ameily commented Apr 18, 2022

Commit Message: uhv: add http-parser permissive parsing for h1 codec

Additional Description: Honor the new uhv_enabled build flag to disable strict parsing within the H1 parsing library (http-parser). This is a reworking of the previous PR, #20528, but this time using the new compile-time option instead of making it configurable at runtime.

Risk Level: Low

Testing:

Docs Changes:

Release Notes:

Platform Specific Features:

Fixes #19750

@ameily ameily requested a review from lizan as a code owner April 18, 2022 16:16
@ameily
Copy link
Contributor Author

ameily commented Apr 18, 2022

@yanavlasov This should be good for review.

@yanavlasov
Copy link
Contributor

@alyssawilk this is special UHV build for H/1 parser with header validation disabled, using existing #define

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Yeah this has my thumbs up, assuming it fails the compile time build without that early return. Mind taking that define out and checking once?
/wait

@ameily
Copy link
Contributor Author

ameily commented Apr 26, 2022

@alyssawilk I have confirmed that removing the early return in both protocol_integration_test and http_inspector_test results in both failing when UHV is enabled:

[  FAILED  ] Protocols/DownstreamProtocolIntegrationTest.BadRequest/IPv4_HttpDownstream_HttpUpstreamNghttp2NoDeferredProcessing
[  FAILED  ] HttpInspectorTest.MultipleReadsHttp1BadProtocol (156 ms)

Copy link
Contributor

@alyssawilk alyssawilk 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 confirming.
cc @snowp for !Googler pass

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@snowp snowp merged commit 7dc822e into envoyproxy:main Apr 29, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uhv: http-parser: add option to skip header validation
4 participants