Feature/rate limit dhcp #1845

julsemaan · 2016-11-17T20:46:04Z

Description

Rate limit the pfdhcplistener
Move the decoding logic back in the pfdhcplistener (since it has to take decision on the L4 payload)

Impacts

pfdhcplistener

Issue

fixes #1722

Delete branch after merge

NO (to backport on setup)

NEWS file entries

Enhancements

Rate limited the DHCP listener processes to prevent specific device(s) from performing a denial of service on the DHCP listening processes (pfdhcplistener rate-limiting #1722)

julsemaan · 2016-11-28T15:15:21Z

This is ready for review.

Opinions wanted on whether or not this should be on by default (right now it rate limits the packets for 5 seconds by default)

fdurand · 2016-11-29T13:51:57Z

sbin/pfdhcplistener

@@ -105,6 +107,9 @@ my $pcap;
 my $net_type;
 my $process;

+my $rate_limit_hash = {};
+my $rate_limit_cache = CHI->new( driver => 'Memory', datastore => $rate_limit_hash );


Is there a way to see what is in the cache with pfcmd cache ...

I'll change it to use a pf::CHI namespace

Just looked into it and if we want to have this available through pfcmd then we can't use the memory storage for obvious reasons..

Now the question is whether we prefer using Redis or the memory storage. I put it in memory for faster access and reduced load on Redis so we'll need to discard this advantage if we want access through pfcmd

IMO keep it faster as possible

fdurand · 2016-11-29T13:56:53Z

conf/pf.conf.defaults

+# For example, a DHCPREQUEST for the same MAC/IP will only be processed once in the timeframe configured below.
+# This is independant of the DHCP server/relay handling the packet and is only based on the IP, MAC Address and DHCP type inside the packet.
+# A value of 0 will disable the rate limitation.
+dhcp_rate_limiting=5s


maybe something lower than the maximum we set for dhcp lease time (29s)

The thing is we could miss some renewals and if someone sets the DHCP lease to 15 seconds, it won't adjust this automatically, so for the default value I wanted to keep something that would work for all the cases and wouldn't create a hard to debug issue.

fdurand · 2016-11-29T14:13:14Z

sbin/pfdhcplistener

+                    }
+                }
+                else {
+                    $logger->info("Ignoring packet due to rate-limiting");


$logger->info("[mac:$dhcp_mac] Ignoring packet due to rate-limiting");

This could create a lot of noise in the log files can you change to debug or trace.

fdurand · 2016-12-15T15:16:40Z

sbin/pfdhcplistener

+
+                my $dhcp_mac = $dhcp->{'chaddr'};
+                # If its a DHCPREQUEST, we take the IP address from the options
+                my $dhcp_ip = ( $dhcp->{'options'}{'53'} == 3 ) ? $dhcp->{'options'}{'50'} : $dhcp->{yiaddr};


It seems that sometimes $dhcp_ip is empty, we probably have to add a condition if define($dhcp_ip) ...after in the code

Correct, I will need to find why its empty sometimes though as it shouldn't be

Just a question is the rate limiting key too specific?

Why would it be too specific ?

I was curious why use the ip in calculating the key and not just $op . $options_53 . $mac

Well since the iplog is mac<->ip based if a device is assigned a different address for whatever reasons, then it won't be ignored by rate limiting.

jrouzierinverse

Should move any logic that is used for deciding to send or not send early in the flow to avoid unnecessary work.

jrouzierinverse · 2016-12-15T15:33:56Z

sbin/pfdhcplistener

+                    return;
+                }
+
+                $dhcp->{'chaddr'} = clean_mac( substr( $dhcp->{'chaddr'}, 0, 12 ) );


Create a temporary variable to hold $dhcp->{'chaddr'} to avoid accessing the hash multiple times for the same variable.

jrouzierinverse · 2016-12-15T15:34:54Z

sbin/pfdhcplistener

+                        "invalid CHADDR value ($dhcp->{'chaddr'}) in DHCP packet from $dhcp->{src_mac} ($dhcp->{src_ip})"
+                    });
+                    return;
+                }


Move any logic that would exit early before creating the %args hash and doing the base64 of the packet to avoid extra work.

jrouzierinverse · 2016-12-15T15:35:16Z

sbin/pfdhcplistener

+
+                my $dhcp_mac = $dhcp->{'chaddr'};
+                # If its a DHCPREQUEST, we take the IP address from the options
+                my $dhcp_ip = ( $dhcp->{'options'}{'53'} == 3 ) ? $dhcp->{'options'}{'50'} : $dhcp->{yiaddr};


Just a question is the rate limiting key too specific?

jrouzierinverse · 2016-12-15T15:37:57Z

sbin/pfdhcplistener

+                    }
+                }
+                else {
+                    $logger->info("Ignoring packet due to rate-limiting");


This could create a lot of noise in the log files can you change to debug or trace.

julsemaan · 2016-12-15T16:14:15Z

Adjustments completed for @fdurand and @jrouzierinverse reviews

jrouzierinverse · 2016-12-15T16:22:15Z

sbin/pfdhcplistener

+                $dhcp->{'chaddr'} = $dhcp_mac;
+
+                # grab DHCP information
+                if ( !defined($dhcp_mac) ) {


This check would be better after the clean_mac of $dhcp->{'chaddr'}

jrouzierinverse · 2016-12-15T16:24:44Z

sbin/pfdhcplistener

+                #  - there is no cache entry (packet wasn't seen in the last $rate_limiting seconds)
+                if(!defined($rate_limit_key) || $rate_limiting == 0 || !$rate_limit_cache->get($rate_limit_key)) {
+                    my $apiclient = pf::api::queue->new(queue => 'pfdhcplistener');
+                    $apiclient->notify('process_dhcp', %args, udp_payload_b64 => $udp_payload_b64);


We do not need to send the udp_payload_b64 anymore for dhcpv4

So to avoid additional processing we should only base64 it when sending it process_dhcpv6.

Also remove the decoding of the udp_payload_b64 in pf::api::process_dhcp

julsemaan · 2016-12-15T17:41:07Z

changes pushaide

jrouzierinverse · 2016-12-15T17:44:34Z

sbin/pfdhcplistener

+                }
+
+                # grab DHCP information
+                if ($dhcp_mac) {


if (!defined $dhcp_mac) {

clean_mac returns "0" when it can't clean the mac

Since you made me move the clean_mac at the top, now the defined($clean_mac) doesn't apply anymore which I've fixed at the same time

Then
if (!$dhcp_mac) {

good catch, let me fit it

julsemaan added 2 commits November 17, 2016 15:19

move packet decoding to pfdhcplistener process

10a3672

rate limit the DHCP listener in case of double packets

e59c8c0

julsemaan added Type: Feature / Enhancement Status: For review labels Nov 28, 2016

julsemaan added this to the PacketFence-6.5 milestone Nov 28, 2016

julsemaan added 2 commits November 28, 2016 10:12

make dhcp rate limiting configurable

4775cf9

add i18n for network.dhcp_rate_limiting

a19e697

fdurand reviewed Nov 29, 2016

View reviewed changes

julsemaan mentioned this pull request Dec 15, 2016

Feature/dhcp rate limiting 6.0 #1906

Closed

fdurand reviewed Dec 15, 2016

View reviewed changes

jrouzierinverse requested changes Dec 15, 2016

View reviewed changes

julsemaan added 3 commits December 15, 2016 11:05

handle undefined option 53 in rate-limiting

0d852c2

variable refactoring

82a6aa6

code cleanup

a951837

jrouzierinverse requested changes Dec 15, 2016

View reviewed changes

julsemaan added 2 commits December 15, 2016 12:38

don't send udp payload to DHCPv4 processor

2cdaff2

💅

81234ca

jrouzierinverse reviewed Dec 15, 2016

View reviewed changes

fix up dhcp_mac detection

2c6bd42

jrouzierinverse approved these changes Dec 15, 2016

View reviewed changes

fdurand merged commit 23b79cd into devel Dec 23, 2016

extrafu added the ready / completed label Jan 23, 2017

jrouzierinverse deleted the feature/rate-limit-dhcp branch January 31, 2017 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/rate limit dhcp #1845

Feature/rate limit dhcp #1845

julsemaan commented Nov 17, 2016 •

edited

Loading

julsemaan commented Nov 28, 2016

fdurand Nov 29, 2016

julsemaan Nov 29, 2016

julsemaan Nov 29, 2016

fdurand Nov 29, 2016

fdurand Nov 29, 2016 •

edited

Loading

julsemaan Nov 29, 2016

fdurand Nov 29, 2016

jrouzierinverse Dec 15, 2016

fdurand Dec 15, 2016 •

edited

Loading

julsemaan Dec 15, 2016

jrouzierinverse Dec 15, 2016

julsemaan Dec 15, 2016

jrouzierinverse Dec 15, 2016

julsemaan Dec 15, 2016

jrouzierinverse left a comment

jrouzierinverse Dec 15, 2016

jrouzierinverse Dec 15, 2016

jrouzierinverse Dec 15, 2016

jrouzierinverse Dec 15, 2016

julsemaan commented Dec 15, 2016

jrouzierinverse Dec 15, 2016

jrouzierinverse Dec 15, 2016

julsemaan commented Dec 15, 2016

jrouzierinverse Dec 15, 2016

julsemaan Dec 15, 2016

jrouzierinverse Dec 15, 2016

julsemaan Dec 15, 2016

Feature/rate limit dhcp #1845

Feature/rate limit dhcp #1845

Conversation

julsemaan commented Nov 17, 2016 • edited Loading

Description

Impacts

Issue

Delete branch after merge

NEWS file entries

Enhancements

julsemaan commented Nov 28, 2016

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

fdurand Nov 29, 2016 • 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

fdurand Dec 15, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrouzierinverse left a 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

julsemaan commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julsemaan commented Dec 15, 2016

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

julsemaan commented Nov 17, 2016 •

edited

Loading

fdurand Nov 29, 2016 •

edited

Loading

fdurand Dec 15, 2016 •

edited

Loading