Skip to content

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

Merged
snowp merged 6 commits intoenvoyproxy:mainfrom
trail-of-forks:uhv-h1-permissive-parsing-take2
Apr 29, 2022
Merged

uhv: add http-parser permissive parsing for h1 codec#20872
snowp merged 6 commits intoenvoyproxy:mainfrom
trail-of-forks:uhv-h1-permissive-parsing-take2

Conversation

@ameily
Copy link
Copy Markdown
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 added 3 commits April 18, 2022 11:03
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
@ameily ameily requested a review from lizan as a code owner April 18, 2022 16:16
ameily added 3 commits April 18, 2022 12:20
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
@ameily
Copy link
Copy Markdown
Contributor Author

ameily commented Apr 18, 2022

@yanavlasov This should be good for review.

@yanavlasov
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
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