Use go-connlimit to ratelimit with 429 responses #9608
Conversation
This is essentially a port of Consul's similar fix Changes are: go get -u github.com/hashicorp/go-connlimit go mod vendor Use new HTTP429 handler hashicorp/consul@20d1ea7
| Certificates: []tls.Certificate{cert}, | ||
| InsecureSkipVerify: true, // good enough | ||
| }) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
If we don't pass the testing.T from the test case into this function and it's closing over the top-level testing.T, I think we lose the association with the test case and it makes errors harder to track down. Might want to either pass in the test case's testing.T or have the function return the error and do the assertion in the caller.
vercel
bot
temporarily deployed
to
Preview – nomad-storybook-and-ui
There was a problem hiding this comment.
Yikes, great catch in the tests.
I'm still concerned about this approach for 2 reasons:
- Writing a response before reading the request will not work with every HTTP client.
- Allowing an attacker to block new connections from other addresses seems like a DoS. Even if it's only 10ms I don't think there's anything preventing an attacker from filling the kernel's listen backlog with pending connections each of which will cause a 10ms timeout.
I think 1. will make 2 worse. Imagine a Python script which uses a synchronous http library that only reads after successfully writing. Perhaps this script is run as a system job across all nodes to check a bit of Nomad state and react appropriately. This works fine until the user deploys this system in a remote region behind a NAT. The NAT makes every incoming connection from the same IP, so the limit is reached. However the synchronous Python http library hits the 10ms timeout every time because it will never read until its write succeeds (although the write "succeeding" probably depends on its size and various buffers between the systems... so the simple case of a GET may work as expected?).
If there are 500 remote nodes, they could easily DoS the cluster by filling the accept backlog with 500 10ms timeouts. Operators may be unable to use Log Monitoring (either via the UI or API) becaue of the unintnetional DoS. (#7893 would partially solve this.)
There's a lot of socket/TCP subtletly involved here that maybe I'm wrong about? Maybe this is safe? Confirming via testing sounds incredibly difficult considering the wide variety of http clients, requests, socket implementations, and middleboxes involved.
Regardless I think a better solution from a safety and usability standpoint would be to implicitly create soft and hard limits:
The soft limit would read the request and respond with a 429 + normal timeout.
The hard limit would behave as before: immediately reject the connection.
The hard limit could implicitly be set to the soft limit + 1. I can't imagine 1 extra connection over the limit will break anybody's security model.
This would allow us to keep all of the existing hard limit code intact, and add the soft limit as a plain old HTTP handler.
|
That reasoning all makes sense to me; we can't assume clients are well-behaved and non-blocking. I like the idea of keeping the hard limit but adding a soft limit handler. But I might adjust the relationship of the hard limit and soft limit. In the accidental case you described with 500 nodes, only a single one of those in flight requests would get a HTTP429 before the remaining connections get dropped. And if the client is blocking as you've described, no one will see the HTTP429 before we start dropping connections. So maybe we want to give ourselves a little more breathing room than soft limit + 1? (Like 10% or something?) |
|
Ah good point. |
#9608 introduced the use of the built-in HTTP 429 response handler provided by go-connlimit. There is concern though around plausible DOS attacks that need to be addressed, so this PR reverts that functionality. It keeps a fix in the tests around the use of an HTTPS enabled client for when the server is listening on HTTPS. Previously, the tests would fail deterministically with io.EOF because that's how the TLS server terminates invalid connections. Now, the result is much less deterministic. The state of the client connection and the server socket depends on when the connection is closed and how far along the handshake was.
#9608 introduced the use of the built-in HTTP 429 response handler provided by go-connlimit. There is concern though around plausible DOS attacks that need to be addressed, so this PR reverts that functionality. It keeps a fix in the tests around the use of an HTTPS enabled client for when the server is listening on HTTPS. Previously, the tests would fail deterministically with io.EOF because that's how the TLS server terminates invalid connections. Now, the result is much less deterministic. The state of the client connection and the server socket depends on when the connection is closed and how far along the handshake was.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Expands on #9359 from @benbuzbee
In addition this PR fixes the original tests, which produced the correct result for the wrong reason. In TLS mode, the clients were creating HTTP connections which would be shunted by the server in the same way being ratelimited would (i.e.
io.EOF). Instead, use a tlsnet.Connfor tls tests.Also rebased the original commit to remove extraneous file.
Closes #9359