Check for unreasonable data in binary propagation #529
Conversation
Codecov Report
@@ Coverage Diff @@
## master #529 +/- ##
==========================================
- Coverage 88.57% 88.53% -0.05%
==========================================
Files 61 61
Lines 3318 3322 +4
==========================================
+ Hits 2939 2941 +2
- Misses 252 253 +1
- Partials 127 128 +1
Continue to review full report at Codecov.
|
propagation.go
Outdated
| // the data is corrupted, to protect against running out of memory | ||
| const ( | ||
| maxBinaryBaggage = 1024 * 1024 | ||
| maxBinaryValueLen = 100 * 1024 * 1024 |
There was a problem hiding this comment.
I don't think these huge values make sense. W3C baggage is limited to 8k (https://github.com/w3c/baggage/blob/master/baggage/HTTP_HEADER_FORMAT.md#limits).
There was a problem hiding this comment.
Yeah fair enough. I looked in OpenTracing and didn’t see any recommended sizes or limits.
Any thoughts on key/value limits?
There was a problem hiding this comment.
Let's use w3c baggage limits
- Maximum number of name-value pairs: 180.
- Maximum number of bytes per a single name-value pair: 4096.
- Maximum total length of all name-value pairs: 8192.
|
@bboreham are you still interested in this change? |
|
Fairly interested; it cost me a lot of time to figure out the problem when I tripped over it, so just trying to save the next person. I have updated it to use W3C limits as you suggested, though now I look at it again I think they should be applied on the Inject side, so if you have gone over those limits you get the error in a more useful place. |
|
Please rebase on master, it should fix the checks failures. |
Assume the data is corrupt and return an error instead of attempting to allocate hundreds of megabytes. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham
force-pushed
the
protect-binary-overload
branch
from
ad05894
to
0e95726
Compare
|
not sure why CI is not running |
Which problem is this PR solving?
Protect against data corruption between binary Inject and Extract.
E.g. somebody passed in base64 data without remembering to decode it...
Short description of the changes
Assume the data is corrupt and return an error instead of attempting to allocate hundreds of megabytes.
The test I have added still fails without the new checks, but takes about 6 seconds longer because
make(map[string]string, 1094795585)is quite slow and requires 5GB of RAM.