Reset read deadline after parsing header #76

pires · 2021-07-13T12:05:24Z

Can you, please, help review @unmarshal, as it is related to your PR #74?

Fixes #75

coveralls · 2021-07-13T12:05:58Z

Coverage increased (+1.2%) to 95.543% when pulling 880d578 on bugfix/reset_conn_read_deadline_after_successful_header into a55009f on main.

vansante · 2021-07-14T11:45:07Z

From my somewhat limited testing it still breaks off the connection with version github.com/pires/go-proxyproto@v0.6.1-0.20210713120647-bab7dd263f97 in my go.mod although I cannot quite explain why 🤷‍♂️ .

pires · 2021-07-15T10:52:58Z

Well, I think the testcase I implemented proves that the code in master fails and later passes with this PR so I'm out of ideas on what the issue may be for you.

unmarshal · 2021-07-15T13:53:33Z

I will be back in town next week. I will take a look first thing. Thanks

On Thu, Jul 15, 2021 at 04:53 Pires ***@***.***> wrote: Well, I think the testcase I implemented prove that the code in master fails and passes with this PR so I'm out of ideas on what the issue may be for you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#76 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAD5RCGK3NCGXUPDN6L2NDTX24ZLANCNFSM5AI6Q4LA> .

-- Sent from Gmail Mobile

antoniomika

I now realize I've actually implemented the fix for this and #74 in a fork I use for one of my projects. I should've opened a PR for that here, sorry about that.

That comparison can be found here. Ultimately, I think we actually want to only timeout when trying to read the proxy proto header and then return the underlying Conn with a zero time deadline and a "no proxy proto" found error. I think setting an explicit flag is important here so we can set a sane default (~200ms from my testing) while still allowing a user to shoot themselves in the foot if they so choose (that can also just be done with a 0 deadline setting, I'm just verbose).

Sorry about just sending this now. I received a dependabot alert a few days ago and upgraded to upstream not realizing this also broke things (i.e. #75)

Happy to chat over email/irc/whatever if there's something else I can help with

protocol.go

antoniomika · 2021-08-04T17:17:03Z

protocol.go

@@ -106,10 +108,12 @@ func NewConn(conn net.Conn, opts ...func(*Conn)) *Conn {
 func (p *Conn) Read(b []byte) (int, error) {
 	p.once.Do(func() {
 		p.readErr = p.readHeader()
+		p.conn.SetReadDeadline(time.Time{})


Move to readHeader

I had tried and am trying again and the new tests introduced in this PR just block.

Can you run your branch on your side with the tests that I introduce here, please?

Actually, in your code you're setting the deadline after the connection has been read already and the p.bufReader is available so that's why it wouldn't work.
My tests are going to block on your changes because there's no read to the underlying connection so no timeout.

I wondered if this ends up somewhat reintroducing the problem that #74 tried to mitigate. At this point, a connection can just sit forever and eat up resources after an initial read. But I think it's fine. At this point the calling client has decided that it is in fact a valid connection (based on the parameters passed in to this lib with acceptance criteria) and so by the time this read is called it's really on the calling client to decide when the connection should be terminated.

That said there is something maybe subtle here: if the client has called SetReadDeadline earlier, then the first call to Read will now overwrite their set deadline.

If you agree this can be an issue, it may be worth using an atomic value that is empty unless SetReadDeadline has been explicitly called, at which point the atomic value is set with the parameter value. When this once function is run, only set the read deadline to infinity if that atomic value is empty (not zero, as they could have passed in zero; or, if you want to avoid pointers/nil interfaces, use an atomic int and in SetReadDeadline, you could record them passing in a zero as a negative value, and check that here). Otherwise set it to the value in the atomic value.

As for a default -- my suggestion would be one or two seconds. For most software in a datacenter or cloud that's an eternity, and if you're using this with e.g. mobile apps you can set it higher to account for disadvantaged networks.

Nice, what do you think of using the proxyproto.ReadTimeout instead of the stdlib Deadlines?

(BTW I'm not gating anything here, mostly just thinking aloud; the final call goes to Pires).

I think personally, I'd prefer using the deadlines in this scenario. Main reason being, I like the idea of this lib (using a listener) acting as a transparent layer which populates the right addresses (ergo it is a drop in replacement) rather than requiring more implementation work. Do you foresee an issue using deadlines if we retain the passed in values?

Do you foresee an issue using deadlines if we retain the passed in values?

Not really, on a quick check #80 does what it says on the tin.

ReadTimeout is already implemented in the lib so I'm thinking it maybe simpler to reuse it inside the readHeader (see my diff above). Anyways, since I'm not using the lib via the listener, but rather the ReadTimeout directly, I think I'm not the target "client" there :)

I don't think there's anything wrong with that. I just figured deadlines would be the preferred way to implement this with minimal goroutines. Ultimately, we're still in line with the synchronous methods recommendation (https://github.com/golang/go/wiki/CodeReviewComments#synchronous-functions) so I think either works!

Thank you all for your invaluable input and help. #80 is highly likely to close this PR and i'll cut a release immediately after it has been merged.

pires · 2021-08-18T12:00:53Z

@unmarshal any chance you'll be able to take a look into this soon?

@antoniomika thanks for the comment. I need to take an hour to think this through and your feedback is definitely appreciated.

unmarshal · 2021-08-20T16:28:02Z

@antoniomika's change looks correct to me.

pires · 2021-08-23T22:16:59Z

@unmarshal are you sure? Please, read this https://github.com/pires/go-proxyproto/pull/76/files#r694345619

As reported in #75 Signed-off-by: Pires <pjpires@gmail.com>

Fixes #75 Signed-off-by: Pires <pjpires@gmail.com>

Signed-off-by: Pires <pjpires@gmail.com>

pires · 2021-09-08T10:09:04Z

Thank you all very much for reporting the issue, the discussion on how to address and the code. I'm happy to have you all around 🙇🏻

pires · 2021-09-08T10:15:38Z

The fix was released as https://github.com/pires/go-proxyproto/releases/tag/v0.6.1

pires added this to the 0.6 milestone Jul 13, 2021

vansante mentioned this pull request Jul 14, 2021

ReadHeaderTimeout is setting a hard timeout on connections regardless of header sending #75

Closed

antoniomika suggested changes Aug 4, 2021

View reviewed changes

antoniomika mentioned this pull request Aug 5, 2021

Connections using proxy protocol on v1.1.6 are broken antoniomika/sish#188

Closed

pires mentioned this pull request Sep 7, 2021

Update the way read header is handled #80

Merged

pires added invalid and removed invalid labels Sep 7, 2021

pires and others added 4 commits September 8, 2021 10:54

protocol: prove erroneous timeout after timeout is set

67d28b3

As reported in #75 Signed-off-by: Pires <pjpires@gmail.com>

protocol: reset timeout after processing header

a4e6227

Fixes #75 Signed-off-by: Pires <pjpires@gmail.com>

ci: remove Go 1.14

094c0b6

Signed-off-by: Pires <pjpires@gmail.com>

protocol: fix header timeout when the user supplied deadline(s)

0c5719a

pires force-pushed the bugfix/reset_conn_read_deadline_after_successful_header branch from f19e669 to 0c5719a Compare September 8, 2021 09:56

protocol: DEFAULT_TIMEOUT is now DefaultReadHeaderTimeout

880d578

Signed-off-by: Pires <pjpires@gmail.com>

pires merged commit 2e44d7a into main Sep 8, 2021

pires deleted the bugfix/reset_conn_read_deadline_after_successful_header branch September 8, 2021 10:09

jjiang-stripe mentioned this pull request Feb 11, 2022

Data race in SetDeadline #86

Closed

tdunlap607 mentioned this pull request Apr 22, 2023

[GHSA-xcf7-q56x-78gh] Connection descriptor exhaustion in proxyproto github/advisory-database#2164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset read deadline after parsing header #76

Reset read deadline after parsing header #76

pires commented Jul 13, 2021

coveralls commented Jul 13, 2021 •

edited

vansante commented Jul 14, 2021

pires commented Jul 15, 2021 •

edited

unmarshal commented Jul 15, 2021 via email

antoniomika left a comment •

edited

antoniomika Aug 4, 2021

pires Aug 23, 2021

pires Aug 23, 2021

pires Aug 23, 2021 •

edited

jefferai Aug 24, 2021 •

edited

igor-kupczynski Aug 31, 2021

antoniomika Aug 31, 2021

igor-kupczynski Aug 31, 2021

antoniomika Sep 1, 2021

pires Sep 7, 2021

pires commented Aug 18, 2021 •

edited

unmarshal commented Aug 20, 2021

pires commented Aug 23, 2021

pires commented Sep 8, 2021

pires commented Sep 8, 2021

Reset read deadline after parsing header #76

Reset read deadline after parsing header #76

Conversation

pires commented Jul 13, 2021

coveralls commented Jul 13, 2021 • edited

vansante commented Jul 14, 2021

pires commented Jul 15, 2021 • edited

unmarshal commented Jul 15, 2021 via email

antoniomika left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pires Aug 23, 2021 • edited

Choose a reason for hiding this comment

jefferai Aug 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pires commented Aug 18, 2021 • edited

unmarshal commented Aug 20, 2021

pires commented Aug 23, 2021

pires commented Sep 8, 2021

pires commented Sep 8, 2021

coveralls commented Jul 13, 2021 •

edited

pires commented Jul 15, 2021 •

edited

antoniomika left a comment •

edited

pires Aug 23, 2021 •

edited

jefferai Aug 24, 2021 •

edited

pires commented Aug 18, 2021 •

edited