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
tests and fixes for issue #69 #71
Conversation
Added the new fix. Let me know what you think. |
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.
The code mostly LGTM. Other than two comments I kindly request you tidy up the commit log.
I'd also like to compensate you for this contribution as it seems a serious security issue. Can you please reach out over Twitter (@therealpires) or email pjpires at googlemail?
Thanks a ton!
cc @emersion @mschneider82 @TimWolla @jefferai (I wish I remembered everyone's name/handle). Someone at Snyk is also willing to help getting a CVE for this which will be a first for me. Thanks everybody out there paying attention and willing to help. |
Thanks for the ping! I'm afraid I won't be much of a help here, though. I'm not even a go programmer. My contribution regarding the unique ID TLV was motivated by the fact that I was the person that added this TLV to the PROXYv2 spec (I'm somewhat involved with upstream HAProxy) and I just searched for proxy protocol libraries to make sure they are up to date. |
@pires Thanks for pinging me! This generally looks good, left a couple of comments. |
Any suggestions how I tidy up the commit log? I agree needs to be done, just no idea how to without new pull request. If that's what it takes, more than happy to. |
Use an interactive rebase |
Thanks. Learn something new everyday :) |
476d66a
to
a368adc
Compare
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.
LGTM, thanks a tonne!
@pires can you tag a new release? |
Working on it. Want to get Go 1.16 in and some fixes to tests I spotted while doing so. |
i do a pull req for traefik, where i used this lib to provide proxy protocol support, but traefik still depends on go 1.15, so requiring 1.16 here would break it. I will stick on last commit for now |
There's no such thing as enforcing Go 1.16 requirement. CI compiles and tests for Go 1.14 and Go 1.15, and now want to add 1.16, that's all. |
Ok fine. The traefik maintainers requested a new version tag |
OK the tests fixing will have to wait as I need more time to find a reasonable strategy while also refactoring to reduce lots of duplicated code. |
2As suggested in PR#70, a new pull request which captures just the tests proving issue #69, with some refinements:
The second issue above can be used for DoS by consuming server process sockets.
Will then submit fixes - further testing shows that pull request #70 does not solve the problem properly.
Fixes #69