-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Experimental DosHandler #12068
base: jetty-12.1.x
Are you sure you want to change the base?
Experimental DosHandler #12068
Conversation
Speaking about the design, I see two problems if you configure a moderately large throughput (say 10K req/s):
|
@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
Outdated
Show resolved
Hide resolved
@sbordet Can you take this one on? |
What I'd like:
I would keep this Handler as simple as possible: if rate is exceeded, reject. |
OK, but I don't see replacing two algorithm arg ( 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...
OK.
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
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.
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? |
@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... |
@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. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@sbordet A wise programmer once said:
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. 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. |
@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 |
@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. |
@sbordet @lachlan-roberts @lorban nudge!!! 3 weeks is too long to wait for feedback! |
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
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
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
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
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DosHandlerTest.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DosHandlerTest.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
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java
Outdated
Show resolved
Hide resolved
@sbordet nudge... no shove.... no POKE!!!! |
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
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DosHandler.java
Outdated
Show resolved
Hide resolved
protected boolean onConditionsMet(Request request, Response response, Callback callback) throws Exception | ||
{ | ||
// Reject if we have too many Trackers | ||
if (_maxTrackers > 0 && _trackers.size() >= _maxTrackers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think _maxTrackers > 0
is always true
because it is defaulted in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
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
/** | ||
* A Handler to reject DoS requests after first delaying them. | ||
*/ | ||
public static class DelayedRejectHandler extends Handler.Abstract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DosHandlerTest.java
Outdated
Show resolved
Hide resolved
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd swap these 2 lines, so it looks more Java like as in DoSHandler.ID_FROM_REMOTE_ADDRESS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that order does not validate again the DTD
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename these properties as jetty.dos.rateControlFactory.expMovingAvg.samplePeriodMs
etc. so they are scoped to the RateControlFactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for alpha
, but sample periods are likely to be common to all trackers and the maxRequestsPerSecond
is definitely common to any tracker
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename these properties as jetty.dos.rejectHandler.delayed.delayMs
etc. so they are scoped to the RejectHandler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for delayMs, but the reject status will be common to all/most reject handlers
protected boolean onConditionsMet(Request request, Response response, Callback callback) throws Exception | ||
{ | ||
// Reject if we have too many Trackers | ||
if (_maxTrackers > 0 && _trackers.size() >= _maxTrackers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/ | ||
class Tracker implements CyclicTimeouts.Expirable | ||
{ | ||
private final AutoLock _lock = new AutoLock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strict need for a lock -- it penalizes normal requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see below
CyclicTimeouts<Tracker> cyclicTimeouts = _cyclicTimeouts; | ||
if (cyclicTimeouts != null) | ||
{ | ||
// schedule a check to remove this tracker if idle | ||
_expireAt = now + _idleCheck.toNanos(); | ||
cyclicTimeouts.schedule(this); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. This class is already Expirable
, so it just needs to update the _expireAt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ummm no! You still need to actually schedule it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just schedule it at creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to expire the timeout, we want to set it again into the future
That's exactly what CyclicTimeouts does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw see #12308 (comment).
public boolean isRateExceededBySample(long now) | ||
{ | ||
// Count the request | ||
_sampleCount++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use atomics and remove the need for a lock in Tracker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@Override | ||
protected boolean onExpired(Tracker tracker) | ||
{ | ||
return tracker.isIdle(NanoTime.now()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* @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}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this javadoc be moved to the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class javadoc does say pretty much this?
/** | ||
* A Handler to reject DoS requests after first delaying them. | ||
*/ | ||
public static class DelayedRejectHandler extends Handler.Abstract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Count me in. |
@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? |
Fix #10478. This is a simple DosHandler that delays and then rejects all requests over a limit.