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.

Sign up for GitHub

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

Jump to bottom

Add support for ReadHeaderTimeout #74

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Add support for ReadHeaderTimeout #74

merged 1 commit into from
Apr 22, 2021

Conversation

unmarshal
Copy link
Contributor

Set a read deadline when waiting for the PROXY protocol header.

@coveralls
Copy link

coveralls commented Apr 10, 2021 •
edited

Coverage Status

Coverage increased (+0.9%) to 94.353% when pulling cdc6386 on unmarshal:read_header_timeout into 7f48261 on pires:main.

@unmarshal unmarshal mentioned this pull request Apr 10, 2021
Closed
@unmarshal unmarshal force-pushed the read_header_timeout branch 2 times, most recently from 19cc42d to de9de94 Compare April 12, 2021 16:46
@pires
Copy link
Owner

pires commented Apr 13, 2021

I'm under the impression I'm about to learn something here but how can the coverage increase if no tests were implemented? Are the examples added here considered example tests?

@pires pires self-assigned this Apr 13, 2021
@pires pires added this to the 0.6 milestone Apr 13, 2021
@unmarshal
Copy link
Contributor Author

I was scratching my head about the test coverage too. I ended up just removing the examples and moving the https server to the README. I didn't want to restructure your project.

@pires
Copy link
Owner

pires commented Apr 13, 2021

No, no, no, please, keep the code in the examples folder or let's make sure they are testable examples which is so much better than keeping examples in README.md. I am just curious how the hell goveralls reports a 0.7% increment in code coverage 😂

@unmarshal
Copy link
Contributor Author

Sounds good. Will do.

@unmarshal
Copy link
Contributor Author

code coverage didn't increase because of the examples. guessing it has to do with the new lines added when the deadline is set.

pires
pires requested changes Apr 15, 2021
View reviewed changes
Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

Overall LGTM but please remove the binaries (maybe you added them accidentally) and add tests that show that an error is thrown when a client doesn't write the header after a defined amount of time.

@unmarshal
Copy link
Contributor Author

Whoops. Yes, that was a mistake. I will fix that and also add a test. Cheers.

@unmarshal
Add support for ReadHeaderTimeout
cdc6386 
Set a read deadline when waiting for the PROXY protocol header.
Fix for pires#65
@unmarshal unmarshal requested a review from pires April 16, 2021 03:35
@unmarshal
Copy link
Contributor Author

Sorry about the premature squashing and force pushes. Didn't realize it messed with github's review system.

pires
pires approved these changes Apr 22, 2021
View reviewed changes
Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

This is great, thank you very much!

@pires pires merged commit 3aa7ea9 into pires:main Apr 22, 2021
@unmarshal unmarshal deleted the read_header_timeout branch April 22, 2021 14:40
@vansante vansante mentioned this pull request Jul 13, 2021
Closed
@pires pires mentioned this pull request Jul 13, 2021
Merged
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this pull request Jul 26, 2021
@stanhu
Update go-proxyproto to v0.6.0
ee306c5 
From https://github.com/pires/go-proxyproto/releases:

Prevent potentially malicious client(s) from opening connections and not
send the proxy protocol header, which could lead to DoS as the server
would hold those socket descriptors open indefinitely, eventually
running out of resources. The solution is to set a read deadline when
waiting for the PROXY protocol header:
pires/go-proxyproto#74
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this pull request Jul 26, 2021
@stanhu
Update go-proxyproto to v0.6.0
d71be6b 
From https://github.com/pires/go-proxyproto/releases:

Prevent potentially malicious client(s) from opening connections and not
send the proxy protocol header, which could lead to DoS as the server
would hold those socket descriptors open indefinitely, eventually
running out of resources. The solution is to set a read deadline when
waiting for the PROXY protocol header:
pires/go-proxyproto#74
@jefferai
Copy link

@pires I strongly urge you to pick a suitable default for when no read timeout is set. Otherwise you put the onus on library users to know that they have to do this in order to be safe.

@pires
Copy link
Owner

pires commented Jul 28, 2021

Agreed.

@antoniomika antoniomika mentioned this pull request Aug 5, 2021
Closed
@GoVulnBot GoVulnBot mentioned this pull request Jan 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pires pires pires approved these changes

Assignees

@pires pires

Labels
enhancement
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@unmarshal @coveralls @pires @jefferai