Experimental DosHandler #12068

gregw · 2024-07-22T06:57:05Z

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

gregw · 2024-07-22T06:59:22Z

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

lorban · 2024-07-23T09:03:18Z

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 · 2024-07-24T00:48:31Z

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

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

gregw · 2024-07-26T22:18:57Z

@sbordet Can you take this one on?

sbordet · 2024-07-29T14:44:18Z

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 · 2024-07-31T07:33:28Z

@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 · 2024-07-31T07:51:16Z

@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 · 2024-07-31T07:59:00Z

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

gregw · 2024-07-31T08:29:57Z

@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 · 2024-07-31T08:57:02Z

@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 · 2024-07-31T20:58:21Z

@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 · 2024-08-01T00:28:30Z

@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 · 2024-08-01T07:08:41Z

@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 · 2024-08-23T01:48:30Z

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

Added tests

named arguments

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DosHandlerTest.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

…tty-12.0.x/DosHandler

gregw · 2024-09-16T07:55:57Z

@sbordet nudge... no shove.... no POKE!!!!

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

sbordet · 2024-09-19T17:05:17Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

+    protected boolean onConditionsMet(Request request, Response response, Callback callback) throws Exception
+    {
+        // Reject if we have too many Trackers
+        if (_maxTrackers > 0 && _trackers.size() >= _maxTrackers)


I think _maxTrackers > 0 is always true because it is defaulted in the constructor.

The intention was to have 0 being unlimited... but perhaps Integer.MAX_VALUE is better for that? For now I've changed the constructor to allow a 0 value.

The value 0 can also be used as an "emergency button", if it could be set via JMX, to stop all requests.
In this way, 0 is not special, -1 is default, and large values are large values.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

sbordet · 2024-09-19T17:50:27Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

+    /**
+     * A Handler to reject DoS requests after first delaying them.
+     */
+    public static class DelayedRejectHandler extends Handler.Abstract


Feels like this class could have a CyclicTimeouts<Exchange> field that does all the expiration logic -- it should simplify the implementation by a lot and avoid bugs.

I had a go at doing this, but I don't think it is right or simpler. CyclicTimeouts is optimized for timeouts that are expected to be cancelled. It also does fine grained timeouts that strive to be accurate. For this delayed handler, we just tick with half the delay period with a timeout that we know will expire and then we process the queue. I think it is best this way.

I fail to understand why it is best having to rewrite the loop, checking for expirations, rescheduling, when it would have been enough to implement onExpired() { Response.writeError(); }, leaving all of the above to existing JDK or Jetty utility classes?
If you don't want to use CyclicTimeouts just use a plain Scheduler, but don't rewrite a scheduler.

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DosHandlerTest.java

jetty-core/jetty-server/src/main/config/etc/jetty-dos.xml

sbordet · 2024-09-24T07:36:02Z

jetty-core/jetty-server/src/main/config/etc/jetty-dos.xml

+            <Name><Property name="jetty.dos.id.type" default="ID_FROM_REMOTE_ADDRESS"/></Name>
+            <Class><Property name="jetty.dos.id.class" default="org.eclipse.jetty.server.handler.DoSHandler"/></Class>


I'd swap these 2 lines, so it looks more Java like as in DoSHandler.ID_FROM_REMOTE_ADDRESS.

Unfortunately that order does not validate again the DTD

sbordet · 2024-09-24T07:38:36Z

jetty-core/jetty-server/src/main/config/etc/jetty-dos.xml

+            <Arg name="samplePeriodMs" type="long"><Property name="jetty.dos.samplePeriodMs" default="-1"/></Arg>
+            <Arg name="alpha" type="double"><Property name="jetty.dos.alpha" default="-1.0"/></Arg>
+            <Arg name="maxRequestsPerSecond" type="int"><Property name="jetty.dos.maxRequestsPerSecond" default="100"/></Arg>


I'd rename these properties as jetty.dos.rateControlFactory.expMovingAvg.samplePeriodMs etc. so they are scoped to the RateControlFactory.

OK for alpha, but sample periods are likely to be common to all trackers and the maxRequestsPerSecond is definitely common to any tracker

sbordet · 2024-09-24T07:40:59Z

jetty-core/jetty-server/src/main/config/etc/jetty-dos.xml

+            <Arg name="delayMs" type="long"><Property name="jetty.dos.delayMs" default="1000"/></Arg>
+            <Arg name="maxDelayQueue" type="int"><Property name="jetty.dos.maxDelayQueue" default="1000"/></Arg>
+            <Arg name="reject">
+              <New class="org.eclipse.jetty.server.handler.DoSHandler$StatusRejectHandler">
+                <Arg name="status"><Property name="jetty.dos.rejectStatus" default="429"/></Arg>


I'd rename these properties as jetty.dos.rejectHandler.delayed.delayMs etc. so they are scoped to the RejectHandler.

OK for delayMs, but the reject status will be common to all/most reject handlers

sbordet · 2024-09-24T07:50:20Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

+    protected boolean onConditionsMet(Request request, Response response, Callback callback) throws Exception
+    {
+        // Reject if we have too many Trackers
+        if (_maxTrackers > 0 && _trackers.size() >= _maxTrackers)


The value 0 can also be used as an "emergency button", if it could be set via JMX, to stop all requests.
In this way, 0 is not special, -1 is default, and large values are large values.

sbordet · 2024-09-24T07:59:40Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java

+     */
+    class Tracker implements CyclicTimeouts.Expirable
+    {
+        private final AutoLock _lock = new AutoLock();


No strict need for a lock -- it penalizes normal requests.

sbordet · 2024-09-24T08:00:45Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java

+                CyclicTimeouts<Tracker> cyclicTimeouts = _cyclicTimeouts;
+                if (cyclicTimeouts != null)
+                {
+                    // schedule a check to remove this tracker if idle
+                    _expireAt = now + _idleCheck.toNanos();
+                    cyclicTimeouts.schedule(this);
+                }


I don't understand this. This class is already Expirable, so it just needs to update the _expireAt.

ummm no! You still need to actually schedule it.

You just schedule it at creation.

ummmm no! The whole point of a cyclic timeout is that it is optimised to be cancelled. If we are getting requests, we don't need to expire the timeout, we want to set it again into the future. We only want to expire if we have not received any requests for a period of time.

In fact, I think I'm missing a schedule for when it expires, but is not idle!

we don't need to expire the timeout, we want to set it again into the future

That's exactly what CyclicTimeouts does.

we don't need to expire the timeout, we want to set it again into the future

That's exactly what CyclicTimeouts does.

Then the contract is strange and we have gotten it "wrong" all over the code base:

https://github.com/jetty/jetty.project/blob/jetty-12.0.13/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpConnection.java#L111

https://github.com/jetty/jetty.project/blob/jetty-12.0.13/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpDestination.java#L319

https://github.com/jetty/jetty.project/blob/jetty-12.0.13/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/QoSHandler.java#L352

I can see a few places where schedule is called from just a setIdleTimeout method, so it is relying on the ability to just mutate the return of getExpireNanoTime, but then the schedule method is named incorrectly. It should just be called add. This is also contradictory to the behaviour in CyclicTimeout, where a call to schedule is needed after a timeout has expired.

So I do not think it wrong to call schedule, but it may be unnecessary. The contract/naming of CyclicTimeouts needs to be improved so that it is used consistently.

@gregw see #12308 (comment).

sbordet · 2024-09-24T08:01:33Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java

+            public boolean isRateExceededBySample(long now)
+            {
+                // Count the request
+                _sampleCount++;


I'd use atomics and remove the need for a lock in Tracker.

You cannot use atomicS plural without it being a race. There is too much info to record into a single atomic long. We could use an atomicReference, but then we'd be allocating... maybe that is better than a lock? @lorban???

Let's get the rest of the class agreed first and we can consider a better lock

sbordet · 2024-09-24T08:18:06Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

+            @Override
+            protected boolean onExpired(Tracker tracker)
+            {
+                return tracker.isIdle(NanoTime.now());


I don't understand.

The idleness is either determined by time, and/or by the specific rate control algorithm.

For example, with a moving window, if you are idle for as long as the sample period, you have no more samples, so you are idle.
The implementation can just set the _expireAt are the right time, so there is no need to have a method to ask.

For another algorithm, it would know differently when it is idle, and therefore set _expireAt at the right time.

So perhaps we need to have the Tracker delegate to the RateControl, either always (RateControl extends Expirable) or optionally by having Tracker test RateControl instanceof Expirable (otherwise defaulting).

So maybe we don't need Tracker at all, since it's just a wrapper to a RateControl.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java

sbordet · 2024-09-24T08:23:11Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java

+        }
+
+        /**
+         * @param status The status used to reject a request, or 0 to fail the request or -1 for a default ({@link HttpStatus#TOO_MANY_REQUESTS_429}.


Can this javadoc be moved to the class?

The class javadoc does say pretty much this?

sbordet · 2024-09-24T08:34:14Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java

+    /**
+     * A Handler to reject DoS requests after first delaying them.
+     */
+    public static class DelayedRejectHandler extends Handler.Abstract


I fail to understand why it is best having to rewrite the loop, checking for expirations, rescheduling, when it would have been enough to implement onExpired() { Response.writeError(); }, leaving all of the above to existing JDK or Jetty utility classes?
If you don't want to use CyclicTimeouts just use a plain Scheduler, but don't rewrite a scheduler.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java

sbordet

I still have concerns about using exponential moving average (does not seem the right algorithm) for rate control, and about RateControl.isIdle() which I think unnecessary.
Let's discuss this face to face.

lorban · 2024-09-25T15:28:31Z

Let's discuss this face to face.

Count me in.

gregw · 2024-09-27T06:44:58Z

@sbordet I've looked at having the rate tracker calculate when it will next go idle, but it involves multiple Math.log calculations, so I'm not sure it is worthwhile. I've delegated the default idleCheckPeriod to the rateTrackerFactory class instead. It can set a period based on the alpha, but currently I'm not doing that,, as it will probably need to know the max rate as well.

Why don't you take over this PR?

gregw requested review from sbordet, lorban and lachlan-roberts July 22, 2024 06:58

lorban reviewed Jul 24, 2024

View reviewed changes

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java Outdated Show resolved Hide resolved

gregw marked this pull request as ready for review July 25, 2024 21:34

gregw assigned sbordet Jul 26, 2024

sbordet requested changes Jul 31, 2024

View reviewed changes

gregw requested review from sbordet and lorban August 1, 2024 08:16

gregw added 7 commits August 23, 2024 11:55

Experimental DosHandler

073719a

Converted to exponential moving average.

a50561c

Added tests

Javadoc

a800fff

Experimental DosHandler

28ef152

Experimental DosHandler

14ae55e

Experimental DosHandler

a929386

Experimental DosHandler

73d546c

named arguments

gregw changed the base branch from jetty-12.0.x to jetty-12.1.x August 23, 2024 01:57

lachlan-roberts requested changes Aug 26, 2024

View reviewed changes

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java Outdated Show resolved Hide resolved

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java Outdated Show resolved Hide resolved

gregw added 2 commits August 27, 2024 09:59

Merge branch 'jetty-12.1.x' into experiment/jetty-12.0.x/DosHandler

b904bbf

Updates from review

7363151

gregw requested a review from lachlan-roberts August 27, 2024 00:13

lachlan-roberts requested changes Aug 27, 2024

View reviewed changes

lorban requested changes Aug 28, 2024

View reviewed changes

updates from review

41a6e8e

gregw requested review from lachlan-roberts and lorban August 31, 2024 22:41

gregw added 2 commits September 2, 2024 07:49

Merge branch 'jetty-12.1.x' into experiment/jetty-12.0.x/DosHandler

d769987

Merge remote-tracking branch 'origin/jetty-12.1.x' into experiment/je…

04dbe06

…tty-12.0.x/DosHandler

lorban approved these changes Sep 2, 2024

View reviewed changes

sbordet requested changes Sep 19, 2024

View reviewed changes

WIP updates from review

26ab55f

gregw requested review from sbordet and lorban September 22, 2024 00:02

gregw added 2 commits September 23, 2024 08:50

Merge branch 'jetty-12.1.x' into experiment/jetty-12.0.x/DosHandler

3f5ec3d

WIP updates from review

033f0bb

sbordet reviewed Sep 24, 2024

View reviewed changes

sbordet requested changes Sep 24, 2024

View reviewed changes

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DoSHandler.java Outdated Show resolved Hide resolved

WIP updates from review

65252a3

gregw requested a review from sbordet September 24, 2024 23:22

sbordet requested changes Sep 25, 2024

View reviewed changes

WIP

40a68bf

		<Name><Property name="jetty.dos.id.type" default="ID_FROM_REMOTE_ADDRESS"/></Name>
		<Class><Property name="jetty.dos.id.class" default="org.eclipse.jetty.server.handler.DoSHandler"/></Class>

Experimental DosHandler #12068

Are you sure you want to change the base?

Experimental DosHandler #12068

Conversation

gregw commented Jul 22, 2024 • edited Loading

gregw commented Jul 22, 2024

lorban commented Jul 23, 2024

gregw commented Jul 24, 2024

gregw commented Jul 26, 2024

sbordet commented Jul 29, 2024

gregw commented Jul 31, 2024

gregw commented Jul 31, 2024

sbordet commented Jul 31, 2024

gregw commented Jul 31, 2024

gregw commented Jul 31, 2024

sbordet left a comment

Choose a reason for hiding this comment

gregw commented Jul 31, 2024

gregw commented Aug 1, 2024

gregw commented Aug 1, 2024

gregw commented Aug 23, 2024

gregw commented Sep 16, 2024

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

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

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

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

sbordet Sep 24, 2024 • edited Loading

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

sbordet left a comment

Choose a reason for hiding this comment

lorban commented Sep 25, 2024

gregw commented Sep 27, 2024

gregw commented Jul 22, 2024 •

edited

Loading

sbordet Sep 24, 2024 •

edited

Loading