DosHandler

[gregw]

Fix #10478. This is a simple DosHandler that delays and then rejects all requests over a limit.

[gregw]

@sbordet @lorban Can you give this a quick initial review before I commit time to tests, config and doco.

[lorban]

Speaking about the design, I see two problems if you configure a moderately large throughput (say 10K req/s):

• You need to allocate an array with one 64-bit entry per request per second. For 10K req/s that's ~80 KB of memory that's constantly scanned and updated. That alone will totally trash all CPU caches.
• There's a single lock protecting that array. I'm not sure this could even reach 10K req/s.

[gregw]

@lorban how about this version that uses an exponential moving average?

[gregw]

@sbordet Can you take this one on?

[sbordet]

What I'd like:

• Pluggable algorithm for rate exceeded -- this will reduce the number of parameters to the constructor, by separating the parameters for the algorithm from those just related to the DoSHandler like maxTrackers.
• Now each tracker is a CyclicTimeout, but DosHandler should handle all the Trackers via CyclicTimeouts.
• usePort seems weird, as using the ephemeral port from the client seems going against the purpose of the DoS defense: the client will use many ephemeral ports.
• Not sure I understand the current algorithm: if a client sends 11 requests, and the 11th exceeds the rate, it is queued, but I'd say it's simpler to reject it immediately. Right now there is a hard-coded 2 s timeout that when expires rejects the queued request. Rejecting immediately would simplify (no queue, no queue parameters), and I see no point waiting 2 seconds to reject?

I would keep this Handler as simple as possible: if rate is exceeded, reject.

[gregw]

@sbordet

What I'd like:

• Pluggable algorithm for rate exceeded -- this will reduce the number of parameters to the constructor, by separating the parameters for the algorithm from those just related to the DoSHandler like maxTrackers.

OK, but I don't see replacing two algorithm arg (samplePeriodMs and alpha) with one new ExponentialMovingAverage(samplePeriodMs, alpha) as much of a saving in parameters. But I'm OK to have the algorithm pluggable.

But then perhaps we should do the same for the ID extraction, which is currently a protected method and two constructor arguments. I'll have a play and see...

• Now each tracker is a CyclicTimeout, but DosHandler should handle all the Trackers via CyclicTimeouts.

OK.

• usePort seems weird, as using the ephemeral port from the client seems going against the purpose of the DoS defense: the client will use many ephemeral ports.

It if for the use case when the server is behind some intermediary, so the remote IP is the intermediary and not the client. Sometimes you cannot trust the Forwarded-for headers because not all intermediaries are smart enough to police that they are not sent from the client itself. So using the source port on the intermediary is a proxy for identifying the connection and thus the client. This is (was?) commonly used by Cisco smart routers. But if we make the ID algorithm pluggable, then this can be done at no cost.

• Not sure I understand the current algorithm: if a client sends 11 requests, and the 11th exceeds the rate, it is queued, but I'd say it's simpler to reject it immediately. Right now there is a hard-coded 2 s timeout that when expires rejects the queued request. Rejecting immediately would simplify (no queue, no queue parameters), and I see no point waiting 2 seconds to reject?

The idea of delay rather than rejecting is to delay additional requests on the same connection. An attacker can pipeline many HTTP/1 requests in a single TCP frame that is buffered. If you just reject the request, then the next one will be there and you will need to do work to reject that. Closing the connection can avoid that, but then that tells the attacker that they need to open another connection to continue the attack. By delaying, the attacker does not know if they are causing DOS or not, and they have to hold open resources to keep the connection alive.

I would keep this Handler as simple as possible: if rate is exceeded, reject.

Reject is not good enough. We'd have to close the connection to avoid issues of many pipelined h1 requests. But then we don't have that semantic for H2 and H3 connections, i.e. we can send a response to a single request that will close all other streams on the same h2 connection.

Delay is expensive for the server, so perhaps we should come up with a way of closing h2 and h3 connections?

[gregw]

@sbordet I've pushed a change to make the ID a pluggable function rather than fixed. It is OK, but might a bit of effort to make it configurable from a module ini file.

I'll try the same approach for the rate algorithm...

[sbordet]

@gregw are you planning of integrating request handling with connection acceptance? I.e. stop accepting connections from the suspicious IP?

[gregw]

@gregw are you planning of integrating request handling with connection acceptance? I.e. stop accepting connections from the suspicious IP?

I wasn't..... but that could be a good idea. Would need new semantic in our connectors, but we need to add some new semantic any if we are to be able to close a connection for h2/h3.

[gregw]

@sbordet I've made the Rate pluggable now as well. It is all a bit messy and lacks javadoc, but give me some feedback on the direction before I commit any more effort. I might work a little bit tomorrow as well.

[sbordet]

@gregw I think Rate could be renamed to RateControl (which we already have in jetty-http2) or similar, but rather than having a getRate() and having to pass the maxRequestPerSecond as an extra parameter, I'd prefer a RateControl.isExceeded(Request), so all parameters end up in the RateControl.Factory specific implementation.

Also, given that reject+block connections, or wait+reject are valid strategies, then perhaps we need to abstract that too, introducing a RejectHandler that also can be implementation specific.
One of the implementations can be linked to the Connector to block connections (which perhaps requires more changes -- I think we have suspend accepting for all remote clients, but not from specific IPs).

DoSHandler(Handler, PeerMapper, RateControl.Factory, RejectHandler) { ... }

where PeerMapper is your Function<Request, String> to map the remote peer.

[gregw]

@sbordet A wise programmer once said:

I would keep this Handler as simple as possible

By making this handler entirely pluggable, I think perhaps we are adding complexity where none is needed. Writing an entirely new custom handler is not that difficult and is better documented than writing three implementations of unknown interfaces to plug into this Handler.
This pluggability will also make XML configuration really difficult.

I suggest we consider going back to a simple handler, with the algorithms in protected methods where possible, and keep it simple. If anybody wants something different, they can extend or replace.
I'd prefer reject+block as the default algorithm, but that needs wider changes and less simplicity. So I think the delay+reject approach is fine for a simple DosHandler.

[gregw]

@sbordet I've added in pluggable rejection. It is not too ugly. I'll try the xml configuration soon. If you have time for some more feedback/review, it would be appreciated before I commit too much to the XML

[gregw]

@sbordet I've added XmlConfiguration and filled out the DosHandler a bit more. It's a little more complex than I'd like, but it is not too bad.

[gregw]

@sbordet @lachlan-roberts @lorban nudge!!! 3 weeks is too long to wait for feedback!

[sbordet]

I think the implementation can be largely simplified.

onRequest() is able to calculate the nanoTime at which will become idle if there are no more events (now + one/two periods), so CyclicTimeouts can take care of idle trackers.

The state should only need the nanoTime of the start of the period, and now many requests in that period, so just 2 longs that can be guarded by a lock (it's per-client anyway, and it's only contended if the client bombs the server).
Simplifying and using a hardcoded 1 second period should result in much simpler code, but a generic period is as simple.

Let's discuss this in person.

[sbordet]

I would remove these constructors, as they are accepting a parameter that should really be specific to the Tracker implementation only.

[sbordet]

I think onRequest(long) should be changed to onRequest(Request rq), so that implementations can examine the request and possibly allow some unconditionally.

[gregw]

That will make trackers impossible to test, as they will be taking nanotime internally rather than having it passed in from a unit test. Everytime we do something like that, we create flaky tests.

Besides, we are already in onConditionsMet method, so the request has already been filtered and unconditional requests can be setup in the conditional handler. There is no need for a) two mechanisms for the same thing; b) being overly generic based on zero use-cases.

[sbordet]

This line is _lastDrip = now, rather than all the math.

[gregw]

That will make the algorithm numerically insensitive because of rounding errors. If we were 1 nanosecond short of another drip, then setting now will effectively lose that drip and the rate will be slow.

[sbordet]

This is a "max", so I'd prefer _maxBucketSize

[gregw]

That doesn't make sense to me as a buckets size is independent of how full it is. max bucket size implies that we can have lots of buckets of different sizes or that buckets can change their size.

[gregw]

If you want max in the name, then I think we should just drop the "bucket" analogy. Using an analogy is a short hand, as we all know what a bucket is and that it has a fixed size and can be empty, full or somewhere in between. If we start trying to over explain that, then we start doubting if the analogy is meaningful and then we may as well not have it. We should call this _threshold or similar

[gregw]

@lachlan-roberts nudge

away at conference