coredns / rrl

Response Rate Limiting Plugin for CoreDNS
Apache License 2.0
23 stars 21 forks source link

Configurable response categorization #13

Open rdrozhdzh opened 5 years ago

rdrozhdzh commented 5 years ago

Would it be reasonable to add a config option for response categorization? Like categorize-by rtype qtype qname

The prefix of client IP will always be included in categorization list and won't be configurable.

The reason is that it might have no sense to categorize by query name if coredns is used as forwarder. Attacker can use a very long list of valid domain names and RRL will account all them differently.

chrisohaver commented 5 years ago

I don't think this is necessary at the moment. Categorizing valid names independently is how BIND works, and per my understanding, it makes sense from the perspective of being less disruptive to clients when blocking attacks. In the case of valid names, I don't think it makes a difference if CoreDNS is a forwarder vs an authoritative source (wildcards are an exception, see below). I don't think we should second guess BIND here.

In the case of invalid names, RRL categorizes based on the domain only, which we pull from the neg cache SOA of the response if it exists. This is because it's comparatively trivial to create an indeterminately long list of invalid queries.

Wildcards are a special case for valid queries. If an attacker knows about a wild card entry, they can very easily create an indeterminately long list of valid queries. This form of attack is less trivial than invalid name flooding, because an attacker has to first identify a wildcard entry. CoreDNS has no way of knowing if a valid query is created by a wildcard entry in a zone it's not authoritative for. For zones that CoreDNS is authoritative for, we can know the base domain (see the suggestion here), but for non-authoritative zones (e.g. forward) we don't know the base domain. I raised this concern with @cricketliu, and the decision was to leave it as is for now, and revisit if it proves to be a problem in the field.

chrisohaver commented 5 years ago

That said, if we do find we need make this an option, it should only be to add/remove qname from the index, and only apply to categorization of valid responses (and possibly also nodata responses). Rtype and qtype should not be removable, since they don't pose potential problems.

rdrozhdzh commented 5 years ago

ok, let's keep it as is for now