Include `rrl`-plugin (Response Rate Limiting) #6904

NicolaiSoeborg · 2024-10-04T20:44:09Z

1. Why is this pull request needed and what does it do?

This is one of the recommendations in the Trail of Bits security audit:

Implement rate-limiting controls in CoreDNS. For instance, consider incorporating the rrl plugin, used for rate limiting, into the main set of CoreDNS plugins to help prevent DoS attacks

2. Which issues (if any) are related?

Running a public coredns server, I see a lot of DNS amplification DDoS attacks.
I think it makes a lot of sense for rrl to be a build-in plugin (allowing for easier usage)

3. Which documentation changes (if any) need to be made?

https://github.com/coredns/coredns.io/blob/master/content/explugins/rrl.md needs to be moved to the "non-ex" plugins folder. Maybe this repos README should include Response Rate Limiting as a feature?

4. Does this introduce a backward incompatible change or deprecation?

Nope

This was one of the recommendations of the Trail of Bits audit: > Implement rate-limiting controls in CoreDNS. For instance, consider incorporating the `rrl` plugin, used for rate limiting, into the main set of CoreDNS plugins to help prevent DoS attacks Signed-off-by: Nicolai Søborg <git@xn--sb-lka.org>

SuperQ · 2024-10-06T19:25:49Z

core/plugin/zplugin.go

@@ -56,4 +56,5 @@ import (
 	_ "github.com/coredns/coredns/plugin/tsig"
 	_ "github.com/coredns/coredns/plugin/view"
 	_ "github.com/coredns/coredns/plugin/whoami"
+	_ "github.com/coredns/rrl/plugins/rrl"


I think we would want to copy the plugin into the repo for "built-in" plugins.

jasonjoo2010 · 2024-10-09T07:00:07Z

It's a good discussion here.
We also use this plugin for workload rebalance internally. But we modified the rrl plugin to make it more like a rate limiter.

For the current implementation,

The query will trigger the rate limit continuously if the QPS is above the configured value. For example, with the rate limit of qps=3, all following queries after the first 3 will be limited.
The balance will only be reset after the configured time window.
(Not a real problem) There's only timeout when the rate limiter is triggered. Sometimes, an active "refused" is also useful.

For 1&2, it may be a small surprise when the user wanted to set a QPS for certain range of clients(or globally) but all requests are blocked once they triggered the rule.

There are not many users using this plugin so far I guess.
As a external plugin, we can modify and build it without issues but we may think about this mechanism carefully if we want to add it as a built-in one.

Shmillerov · 2024-11-07T19:04:25Z

Totally agree with @jasonjoo2010
I made a test of plugin that "send" 100 requests. 50 requests with 20 RPS, then 50 request with 5 RPS. RRL limit 10

func TestDebit_internal(t *testing.T) {
	rrl := defaultRRL()
	rrl.window = 1 * second
	rrl.responsesInterval = second / 10
	rrl.initTable()

	for i := 0; i < 100; i++ {
		b, _, _ := rrl.debit(rrl.responsesInterval, "token")
		if i < 50 {
			time.Sleep(time.Second / 20)
		} else {
			time.Sleep(time.Second / 5)
		}
		fmt.Println("Balance for ", i+1, b)
	}
}

Results

limit 10, window 0:
Balance for 1 0
Balance for 2 850186000
Balance for 3 801241000
Balance for 4 752270000
Balance for 5 702913000
Balance for 6 653479000
Balance for 7 604630000
Balance for 8 555770000
Balance for 9 506949000
Balance for 10 458036000
Balance for 11 409254000
Balance for 12 360391000
Balance for 13 311472000
Balance for 14 262313000
Balance for 15 213425000
Balance for 16 164498000
Balance for 17 115657000
Balance for 18 66931000
Balance for 19 18045000
Balance for 20 0
Balance for 21 0
...
Balance for 51 0
Balance for 52 101140000
Balance for 53 201338000
Balance for 54 302495000
Balance for 55 403647000
Balance for 56 504848000
Balance for 57 605977000
Balance for 58 707113000
Balance for 59 808341000
Balance for 60 909473000
Balance for 61 900000000
Balance for 62 900000000
Balance for 63 900000000
...

limit 10, window 1 sec:
Balance for 1 0
Balance for 2 851043000
Balance for 3 802167000
Balance for 4 753386000
Balance for 5 703844000
Balance for 6 654936000
Balance for 7 606067000
Balance for 8 557138000
Balance for 9 508256000
Balance for 10 459339000
Balance for 11 410402000
Balance for 12 361502000
Balance for 13 312617000
Balance for 14 263741000
Balance for 15 214866000
Balance for 16 165874000
Balance for 17 116989000
Balance for 18 68200000
Balance for 19 19296000
Balance for 20 -29593000
Balance for 21 -78445000
Balance for 22 -127653000
Balance for 23 -176544000
Balance for 24 -226154000
Balance for 25 -275097000
Balance for 26 -323996000
Balance for 27 -372924000
Balance for 28 -421867000
Balance for 29 -471244000
Balance for 30 -520099000
Balance for 31 -569019000
Balance for 32 -617865000
Balance for 33 -667020000
Balance for 34 -715852000
Balance for 35 -764922000
Balance for 36 -814202000
Balance for 37 -863118000
Balance for 38 -912851000
Balance for 39 -962304000
Balance for 40 -1000000000
Balance for 41 -1000000000
Balance for 42 -1000000000
Balance for 43 -1000000000
Balance for 44 -1000000000
Balance for 45 -1000000000
Balance for 46 -1000000000
Balance for 47 -1000000000
Balance for 48 -1000000000
Balance for 49 -1000000000
Balance for 50 -1000000000
Balance for 51 -1000000000
Balance for 52 -898851000
Balance for 53 -798215000
Balance for 54 -697496000
Balance for 55 -596362000
Balance for 56 -495602000
Balance for 57 -394473000
Balance for 58 -293108000
Balance for 59 -192052000
Balance for 60 -90916000
Balance for 61 10535000
Balance for 62 111670000
Balance for 63 212887000
Balance for 64 314059000
Balance for 65 415205000
Balance for 66 516399000
Balance for 67 616933000
Balance for 68 718089000
Balance for 69 819254000
Balance for 70 920390000
Balance for 71 900000000
Balance for 72 900000000
Balance for 73 900000000
...

limit 10, window 15 sec (default):
Balance for 1 0
Balance for 2 851046000
Balance for 3 802083000
Balance for 4 753141000
Balance for 5 704167000
Balance for 6 655197000
Balance for 7 606339000
Balance for 8 557430000
Balance for 9 508628000
Balance for 10 459713000
Balance for 11 410861000
Balance for 12 361971000
Balance for 13 313045000
Balance for 14 264172000
Balance for 15 215199000
Balance for 16 166251000
Balance for 17 117291000
Balance for 18 68409000
Balance for 19 19537000
Balance for 20 -29350000
Balance for 21 -78152000
Balance for 22 -127208000
Balance for 23 -176116000
Balance for 24 -224985000
Balance for 25 -273886000
Balance for 26 -322774000
Balance for 27 -371632000
Balance for 28 -420517000
Balance for 29 -469890000
Balance for 30 -518828000
Balance for 31 -567695000
Balance for 32 -617207000
Balance for 33 -666049000
Balance for 34 -714894000
Balance for 35 -763816000
Balance for 36 -812688000
Balance for 37 -862381000
Balance for 38 -911313000
Balance for 39 -960202000
Balance for 40 -1009410000
Balance for 41 -1058284000
Balance for 42 -1107493000
Balance for 43 -1156381000
Balance for 44 -1205253000
Balance for 45 -1254078000
Balance for 46 -1302957000
Balance for 47 -1351886000
Balance for 48 -1400741000
Balance for 49 -1449610000
Balance for 50 -1498549000
Balance for 51 -1547434000
Balance for 52 -1446280000
Balance for 53 -1345143000
Balance for 54 -1244679000
Balance for 55 -1143537000
Balance for 56 -1042398000
Balance for 57 -941270000
Balance for 58 -840172000
Balance for 59 -739042000
Balance for 60 -637787000
Balance for 61 -536599000
Balance for 62 -436429000
Balance for 63 -335282000
Balance for 64 -234121000
Balance for 65 -132963000
Balance for 66 -31813000
Balance for 67 69322000
Balance for 68 170460000
Balance for 69 271634000
Balance for 70 372773000
Balance for 71 473904000
Balance for 72 575053000
Balance for 73 676082000
Balance for 74 777225000
Balance for 75 878362000
Balance for 76 979545000
Balance for 77 900000000
Balance for 78 900000000
Balance for 79 900000000
Balance for 80 900000000
...

It allows more requests per second than specified limit
But when balance starts to become negative, it limits all requests until the load is reduced

I would also like to see a standard token bucket algorithm so that the rate limiter does not allow more than N requests per second.

jasonjoo2010 · 2024-11-13T03:28:36Z

Btw, I can share the simple internal fix for a literal QPS limiting purpose:

@@ -179,7 +180,10 @@ func (rrl *RRL) debit(allowance int64, t string) (int64, bool, error) {
				// balance can't be more negative than window
				balance = -rrl.window
			}
-			ra.allowTime = now - balance
+			if balance >= 0 {
+				ra.allowTime = now - balance
+			}
			if balance > 0 || ra.slipCountdown == 0 {
				return balances{balance, false}
			}

But as the plugin claims to follow the same behaviour of BIND9, I didn't raise this concern to official repo. The philosophy of BIND9 must be forbidding a client from sending further requests when it violates the rule, I guess.

chrisohaver · 2024-11-15T13:43:04Z

When we move this in tree, I think we should split the functionality of the rrl plugin into two separate plugins: response rate limiting, and request rate limiting.

The rrl plugin was initially intended to be a response rate limiter (not request). Its goal was to prevent DNS reflection attacks specially. The implementation follows BIND's description of its synonymous feature designed for the same purpose. Later, request rate limiting was added to our implementation. In retrospect, request rate limiting should have been a separate plugin. These two functions have different goals, and should ideally track state/rates and limit requests/responses differently.

When incorporating rrl into coredns as an internal plugin, it would be a good opportunity to remove the request rate limiting functionality, and break it out (or re-implement) into a separate plugin that can optimized for that purpose (in terms of performance and expected behavior).

The observations above regarding "literal QPS limiting" vs BIND behavior underscores this: Expectations for request rate limiting are going to be different than response rate limiting geared for reflection attack prevention. Rather than these two functions being at odds in the same plugin, I think they would be better as two separate plugins.

NicolaiSoeborg requested review from miekg, chrisohaver, johnbelamaric, yongtang, stp-ip, bradbeam, dilyevsky, jameshartig, greenpau, Isolus, pmoroney, rajansandeep, SuperQ and Tantalor93 as code owners October 4, 2024 20:44

SuperQ reviewed Oct 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include `rrl`-plugin (Response Rate Limiting) #6904

Include `rrl`-plugin (Response Rate Limiting) #6904

Include rrl-plugin (Response Rate Limiting) #6904

Are you sure you want to change the base?

Include rrl-plugin (Response Rate Limiting) #6904

Conversation

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

Choose a reason for hiding this comment

Include `rrl`-plugin (Response Rate Limiting) #6904

Include `rrl`-plugin (Response Rate Limiting) #6904