-
Notifications
You must be signed in to change notification settings - Fork 41
Netty 4.1.128 with support for lenient default line ending #1942
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
base: main
Are you sure you want to change the base?
Netty 4.1.128 with support for lenient default line ending #1942
Conversation
Signed-off-by: Peter Nied <peternied@hotmail.com>
When a recent issue was fixed GHSA-jq43-27x9-3v86 this tighted the strictness of checks during parsing which the test started failing. By switching from LF -> CRLF this fixed the out of compliance configuration. Signed-off-by: Peter Nied <peternied@hotmail.com>
Signed-off-by: Peter Nied <peternied@hotmail.com>
Signed-off-by: Peter Nied <peternied@hotmail.com>
Signed-off-by: Peter Nied <peternied@hotmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1942 +/- ##
============================================
+ Coverage 78.50% 78.52% +0.01%
Complexity 21 21
============================================
Files 599 599
Lines 23592 23599 +7
Branches 1824 1825 +1
============================================
+ Hits 18521 18531 +10
+ Misses 4175 4171 -4
- Partials 896 897 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…g jvm property by default to retain existing behavior of allowing CRLF or LF line endings in proxy and replayer to retain compatibility with older clients who may use LF line endings Signed-off-by: Andre Kurait <akurait@amazon.com>
7d76bbc to
6500891
Compare
… tests in jsonJoltMessageTransformerProvider Signed-off-by: Andre Kurait <akurait@amazon.com>
b685835 to
4176321
Compare
|
@AndreKurait thanks for spending the time to look into this. Is there a way to do this without using jvm parameters? While its nice that we can keep the more lenient default, it creates yet another configuration path for how netty operates that seems previously unused. |
gregschohn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up...
I agree w/ Peter & would add that for the capture proxy, this should be one of the most visible and loudest options to configure. One application of replaying traffic is to confirm that security bugs have been fixed. We wouldn't want to have a source getting attacked, and having the proxy neutralize those so that users never find out if their targets are resilient to the attacks.
I was expecting to see a command line flag for our programs - at least the proxy (I don't think that it will matter for the replayer since the replayer will generally always be contacting newer services - not legacy ones w/ CVEs). That said, with where we're going for workflows, we could also say - here's an options flag for all of our programs and here's how we route it appropriately. We DO set JVM properties from argo workflows today to override log4j properties.
Should we have any way to test both policies here?
| -XX:MaxRAMPercentage=80.0 \ | ||
| -XX:+ExitOnOutOfMemoryError \ | ||
| -XshowSettings:vm \ | ||
| -Dio.netty.handler.codec.http.defaultStrictLineParsing=false \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a user override this setting/flip it to true? They'd have to eschew this file or edit it, right? That's a rough UX.
| imageTag : "latest", | ||
| jvmFlags : [ | ||
| // By default enable transparent proxy with older clients for maximum compatibility | ||
| "-Dio.netty.handler.codec.http.defaultStrictLineParsing=false", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as before - how does jib wire up the java properties - are they env variables? How would a user override this?
Description
Follow-up to Proposed PR #1938, which upgraded Netty from 4.1.124 → 4.1.128 to address GHSA-jq43-27x9-3v86 and GHSA-fghv-69vj-qj49.
The previous bump introduced stricter request-parsing checks in Netty 4.1.125, causing possible failures where clients use
LFinstead ofCRLF. That change aligned behavior with the current HTTP spec but could break compatibility with some older clients.This PR adds configuration support for lenient line-ending parsing by default, disabling strict line checks to preserve backward compatibility with legacy clients. This ensures we can continue handling replayed or proxied traffic that doesn’t fully conform to modern HTTP formatting.
Netty Reviewed CVE: GHSA-fghv-69vj-qj49 (Marked as Low)
Original Netty Issue: netty/netty#15522
Original Netty PR: netty/netty#15611
Key Changes
io.netty.handler.codec.http.defaultStrictLineParsingto disable strict line parsing by default.Note: Includes an update to the JIB building to include the JVM flags we've been applying to our existing docker builds.
Testing
Existing tests broke with the change, adding system property during test runs reverts to existing behavior shown by the tests passing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.