From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping Date: Thu, 18 Feb 2021 13:06:35 +0000 Message-ID: <6D50D2AF-E3A4-4E06-AB69-4C124A657440@ipfire.org> In-Reply-To: <4a440c35-f9df-4738-6fc5-eed99b30fab4@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8514806685380969344==" List-Id: --===============8514806685380969344== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, Yes this is an issue of the old implementation indeed. It has not really bothered me because in most cases you are fine. ISC DHCP st= ill behaves predictable, but not in the way that people expect. Could we change this patch to do the following maybe: * If it is an existing lease it can be edited and a warning is being shown * If it is a new lease being created, an error should be shown as you already= implemented. I think that would help us to inform users about potential problems (which mo= st of them wouldn=E2=80=99t have experienced up to this point in time) and ne= w setup will be correct. Is that an acceptable compromise? -Michael > On 18 Feb 2021, at 12:17, Adolf Belka (ipfire-dev) wrote: >=20 > Hi Michael, >=20 > On 18/02/2021 12:37, Michael Tremer wrote: >> Hello, >> This has come up a couple of times before, and I am not sure if we can mak= e this change without breaking any existing setups. >> As I understand it, we do. Editing a static lease and hitting save will no= longer be possible if that IP address is part of the dynamic range. >> Can you confirm that? >=20 > If the static lease is edited to have an IP address in the dynamic range th= en it will not be possible any more. There will be an error message saying th= at you have selected an IP from the dynamic range. It will not be entered int= o the dhcp.conf file. >=20 > If we don't implement this fix then anyone who defines a fixed IP address f= rom the dynamic range is doing something that ISC DHCP say should not be done. >=20 > Most people will probably get away with it most of the time. You could end = up with a fixed lease computer off line for some reason and its lease expires= . That IP Address can then be given to another computer and when the original= client comes back it cannot get the fixed lease. It will probably be offered= a dynamic lease now. Either way the fixed IP address will now be allocated t= o a different computer. >=20 > pfSense and OPNsense have this restriction implemented in their WUI. >=20 > Regards, >=20 > Adolf. >=20 >>> On 17 Feb 2021, at 13:58, Adolf Belka wrote: >>>=20 >>> - This is a fix for bug #10629 >>> - I have tested this out on my vm testbed system. Everything worked fine >>> with this. It would be good to get other test feedback in case I have >>> missed something. >>> - This fix flags up if a fixed lease is created within the existing dynam= ic >>> range >>> - This fix also works if a dynamic lease is converted to a fixed lease. A >>> new IP outside the dynamic range has to be selected. >>> - A check has also been added if the dynamic range is modified to overlap >>> any existing fixed leases. The error message will also inform how many >>> fixed leases are now overlapped by the modified dynamic range. >>> - If an interface is disabled and fixed leases within the dynamic range >>> created or the dynamic range expanded to overlap with existing fixed >>> leases, then when the interface is enabled again the check is carried >>> out and catches these and prevents them being set. >>> - New error messages added to en.pl file >>>=20 >>> Signed-off-by: Adolf Belka >>> --- >>> config/cfgroot/general-functions.pl | 18 ++++++++++++ >>> doc/language_issues.de | 2 ++ >>> doc/language_issues.en | 2 ++ >>> doc/language_issues.es | 2 ++ >>> doc/language_issues.fr | 2 ++ >>> doc/language_issues.it | 2 ++ >>> doc/language_issues.nl | 2 ++ >>> doc/language_issues.pl | 2 ++ >>> doc/language_issues.ru | 2 ++ >>> doc/language_issues.tr | 2 ++ >>> doc/language_missings | 24 ++++++++++++++++ >>> html/cgi-bin/dhcp.cgi | 43 +++++++++++++++++++++++++++++ >>> langs/en/cgi-bin/en.pl | 3 ++ >>> 13 files changed, 106 insertions(+) >>>=20 >>> diff --git a/config/cfgroot/general-functions.pl b/config/cfgroot/general= -functions.pl >>> index a6656ccf5..a8c8d171c 100644 >>> --- a/config/cfgroot/general-functions.pl >>> +++ b/config/cfgroot/general-functions.pl >>> @@ -591,6 +591,24 @@ sub check_net_internal_exact{ >>> if (($ownnet{'RED_NETADDRESS'} ne '' && $ownnet{'RED_NETADDRESS'} ne= '0.0.0.0') && &Network::network_equal("$ownnet{'RED_NETADDRESS'}/$ownnet{'RE= D_NETMASK'}", $network)){ $errormessage=3D$Lang::tr{'ccd err red'};return $er= rormessage;} >>> } >>>=20 >>> +sub ip_address_in_ip_range($$) { >>> +# Returns True if $ipaddress is within $ipstart and $ipend range. >>> + my $ipaddress =3D shift; >>> + my $ipstart =3D shift; >>> + my $ipend =3D shift; >>> + >>> + my $ipaddress_bin =3D &Network::ip2bin($ipaddress); >>> + return undef unless (defined $ipaddress_bin); >>> + >>> + my $ipstart_bin =3D &Network::ip2bin($ipstart); >>> + return undef unless (defined $ipstart_bin); >>> + >>> + my $ipend_bin =3D &Network::ip2bin($ipend); >>> + return undef unless (defined $ipend_bin); >>> + >>> + return (($ipaddress_bin >=3D $ipstart_bin) && ($ipaddress_bin <=3D $ipe= nd_bin)); >>> +} >> This function should live in network-functions.pl since it clearly is a ne= twork function :) >> Ideally a test could be added for it at the end of it. >>> + >>> sub validport >>> { >>> $_ =3D $_[0]; >>> diff --git a/doc/language_issues.de b/doc/language_issues.de >>> index 5d079036a..cb3e89b2e 100644 >>> --- a/doc/language_issues.de >>> +++ b/doc/language_issues.de >>> @@ -840,6 +840,8 @@ WARNING: translation string unused: zoneconf val vlan= amount assignment error >>> WARNING: translation string unused: zoneconf val vlan tag assignment error >>> WARNING: translation string unused: zoneconf val zoneslave amount error >>> WARNING: untranslated string: desired =3D Desired >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: enable =3D Enable >>> WARNING: untranslated string: error the to date has to be later than the = from date =3D The to date has to be later than the from date! >>> diff --git a/doc/language_issues.en b/doc/language_issues.en >>> index 6e30eb995..832ff8d92 100644 >>> --- a/doc/language_issues.en >>> +++ b/doc/language_issues.en >>> @@ -582,6 +582,8 @@ WARNING: untranslated string: dhcp dns key name =3D K= ey Name >>> WARNING: untranslated string: dhcp dns update =3D DNS Update >>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm >>> WARNING: untranslated string: dhcp dns update secret =3D Secret >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp server =3D DHCP Server >>> WARNING: untranslated string: dhcp server disabled =3D DHCP server disabl= ed. Stopped. >>> WARNING: untranslated string: dhcp server enabled =3D DHCP server enabled= . Restarting. >>> diff --git a/doc/language_issues.es b/doc/language_issues.es >>> index 82d65d99c..b65ecd164 100644 >>> --- a/doc/language_issues.es >>> +++ b/doc/language_issues.es >>> @@ -893,6 +893,8 @@ WARNING: untranslated string: dhcp dns key name =3D K= ey Name >>> WARNING: untranslated string: dhcp dns update =3D DNS Update >>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm >>> WARNING: untranslated string: dhcp dns update secret =3D Secret >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp valid range required when deny known c= lients checked =3D Valid range required when "Deny known clients:" is checked >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: disconnected =3D Disconnected >>> diff --git a/doc/language_issues.fr b/doc/language_issues.fr >>> index 942be73ec..71de90bd7 100644 >>> --- a/doc/language_issues.fr >>> +++ b/doc/language_issues.fr >>> @@ -880,6 +880,8 @@ WARNING: translation string unused: zoneconf val vlan= amount assignment error >>> WARNING: translation string unused: zoneconf val vlan tag assignment error >>> WARNING: translation string unused: zoneconf val zoneslave amount error >>> WARNING: untranslated string: dhcp deny known clients: =3D Deny known cli= ents: >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp valid range required when deny known c= lients checked =3D Valid range required when "Deny known clients:" is checked >>> WARNING: untranslated string: fwhost cust locationgrp =3D unknown string >>> WARNING: untranslated string: fwhost err hostip =3D unknown string >>> diff --git a/doc/language_issues.it b/doc/language_issues.it >>> index 98074e59f..a4cd8c5db 100644 >>> --- a/doc/language_issues.it >>> +++ b/doc/language_issues.it >>> @@ -917,6 +917,8 @@ WARNING: untranslated string: dhcp dns key name =3D K= ey Name >>> WARNING: untranslated string: dhcp dns update =3D DNS Update >>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm >>> WARNING: untranslated string: dhcp dns update secret =3D Secret >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp valid range required when deny known c= lients checked =3D Valid range required when "Deny known clients:" is checked >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: disconnected =3D Disconnected >>> diff --git a/doc/language_issues.nl b/doc/language_issues.nl >>> index 8eebbd57f..9cef4790e 100644 >>> --- a/doc/language_issues.nl >>> +++ b/doc/language_issues.nl >>> @@ -918,6 +918,8 @@ WARNING: untranslated string: dhcp dns key name =3D K= ey Name >>> WARNING: untranslated string: dhcp dns update =3D DNS Update >>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm >>> WARNING: untranslated string: dhcp dns update secret =3D Secret >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: disconnected =3D Disconnected >>> WARNING: untranslated string: dl client arch insecure =3D Download insecu= re Client Package (zip) >>> diff --git a/doc/language_issues.pl b/doc/language_issues.pl >>> index 82d65d99c..b65ecd164 100644 >>> --- a/doc/language_issues.pl >>> +++ b/doc/language_issues.pl >>> @@ -893,6 +893,8 @@ WARNING: untranslated string: dhcp dns key name =3D K= ey Name >>> WARNING: untranslated string: dhcp dns update =3D DNS Update >>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm >>> WARNING: untranslated string: dhcp dns update secret =3D Secret >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp valid range required when deny known c= lients checked =3D Valid range required when "Deny known clients:" is checked >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: disconnected =3D Disconnected >>> diff --git a/doc/language_issues.ru b/doc/language_issues.ru >>> index 43c1f8c08..76fd6b350 100644 >>> --- a/doc/language_issues.ru >>> +++ b/doc/language_issues.ru >>> @@ -895,6 +895,8 @@ WARNING: untranslated string: dhcp dns key name =3D K= ey Name >>> WARNING: untranslated string: dhcp dns update =3D DNS Update >>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm >>> WARNING: untranslated string: dhcp dns update secret =3D Secret >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp valid range required when deny known c= lients checked =3D Valid range required when "Deny known clients:" is checked >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: disconnected =3D Disconnected >>> diff --git a/doc/language_issues.tr b/doc/language_issues.tr >>> index 439a58890..bd78a5a4e 100644 >>> --- a/doc/language_issues.tr >>> +++ b/doc/language_issues.tr >>> @@ -896,6 +896,8 @@ WARNING: untranslated string: dangerous =3D Dangerous >>> WARNING: untranslated string: default IP address =3D Default IP Address >>> WARNING: untranslated string: desired =3D Desired >>> WARNING: untranslated string: dhcp deny known clients: =3D Deny known cli= ents: >>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic ran= ge overlapped with >>> +WARNING: untranslated string: dhcp fixed ip address =3D Fixed IP Addres= s(es) >>> WARNING: untranslated string: dhcp valid range required when deny known c= lients checked =3D Valid range required when "Deny known clients:" is checked >>> WARNING: untranslated string: disable =3D Disable >>> WARNING: untranslated string: disconnected =3D Disconnected >>> diff --git a/doc/language_missings b/doc/language_missings >>> index 0d89426ca..3d6c5103d 100644 >>> --- a/doc/language_missings >>> +++ b/doc/language_missings >>> @@ -28,6 +28,9 @@ >>> < could not connect to www ipfire org >>> < cryptographic settings >>> < desired >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp server disabled on blue interface >>> < dhcp server enabled on blue interface >>> < dh name is invalid >>> @@ -230,6 +233,9 @@ >>> < dhcp dns update >>> < dhcp dns update algo >>> < dhcp dns update secret >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp valid range required when deny known clients checked >>> < dh key move failed >>> < dh key warn >>> @@ -969,6 +975,9 @@ >>> < bewan adsl pci st >>> < bewan adsl usb >>> < dhcp deny known clients: >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp valid range required when deny known clients checked >>> < g.dtm >>> < g.lite >>> @@ -1071,6 +1080,9 @@ >>> < dhcp dns update >>> < dhcp dns update algo >>> < dhcp dns update secret >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp valid range required when deny known clients checked >>> < disable >>> < Disabled >>> @@ -1460,6 +1472,9 @@ >>> < dhcp dns update >>> < dhcp dns update algo >>> < dhcp dns update secret >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dh key move failed >>> < dh key warn >>> < dh key warn1 >>> @@ -1965,6 +1980,9 @@ >>> < dhcp dns update >>> < dhcp dns update algo >>> < dhcp dns update secret >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp valid range required when deny known clients checked >>> < dh key move failed >>> < dh key warn >>> @@ -2848,6 +2866,9 @@ >>> < dhcp dns update >>> < dhcp dns update algo >>> < dhcp dns update secret >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp valid range required when deny known clients checked >>> < dh key move failed >>> < dh key warn >>> @@ -3595,6 +3616,9 @@ >>> < default IP address >>> < desired >>> < dhcp deny known clients: >>> +< dhcp dynamic range overlap >>> +< dhcp fixed ip address >>> +< dhcp fixed ip address in dynamic range >>> < dhcp valid range required when deny known clients checked >>> < disable >>> < Disabled >>> diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi >>> index 867614f2a..82ea754c7 100644 >>> --- a/html/cgi-bin/dhcp.cgi >>> +++ b/html/cgi-bin/dhcp.cgi >>> @@ -130,6 +130,7 @@ open(FILE, "$filename2") or die 'Unable to open fixed= leases file.'; >>> our @current2 =3D ; >>> close(FILE); >>>=20 >>> + >>> # Check Settings1 first because they are needed by &buildconf >>> if ($dhcpsettings{'ACTION'} eq $Lang::tr{'save'}) { >>> foreach my $itf (@ITFs) { >>> @@ -183,6 +184,24 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'save'}) { >>> } >>> } >>>=20 >>> + # Check if dynamic range and Fixed IP Addresses overlap >>> + if ((!$dhcpsettings{"START_ADDR_${itf}"}) eq '' && (!$dhcpsettings{= "END_ADDR_${itf}"}) eq '') { >> For better readability, writing =E2=80=9Cne=E2=80=9D instead of !eq might = be a good idea. >>> + my $count=3D0; >>> + foreach my $line (@current2) { >>> + chomp($line); >>> + my @temp =3D split(/\,/,$line); >>> + if (&General::ip_address_in_ip_range($temp[1], >>> + $dhcpsettings{"START_ADDR_${itf}"}, >>> + $dhcpsettings{"END_ADDR_${itf}"})) { >>> + $count++; >>> + } >>> + } >>> + if ($count > 0) { >>> + $errormessage =3D "DHCP on ${itf}: " . $Lang::tr{'dhcp dynamic range = overlap'} . $count . $Lang::tr{'dhcp fixed ip address'}; >>> + goto ERROR; >>> + } >>> + } >>> + >>> if (!($dhcpsettings{"DEFAULT_LEASE_TIME_${itf}"} =3D~ /^\d+$/)) { >>> $errormessage =3D "DHCP on ${itf}: " . $Lang::tr{'invalid default lease= time'} . $dhcpsettings{'DEFAULT_LEASE_TIME_${itf}'}; >>> goto ERROR; >>> @@ -415,10 +434,34 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'toggle en= able disable'}.'2') { >>> if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>> $dhcpsettings{'FIX_MAC'} =3D~ tr/-/:/; >>> unless(&General::validip($dhcpsettings{'FIX_ADDR'})) { $errormessage = =3D $Lang::tr{'invalid fixed ip address'}; } >>> +# Check if fixed address is in the dynamic range, if defined >>> + foreach my $itf (@ITFs) { >>> + if ($dhcpsettings{"ENABLE_${itf}"} eq 'on' ) { >>> + if ($dhcpsettings{"START_ADDR_${itf}"}) { >>> + if (&General::ip_address_in_ip_range($dhcpsettings{'FIX_ADDR'}, >>> + $dhcpsettings{"START_ADDR_${itf}"}, >>> + $dhcpsettings{"END_ADDR_${itf}"})) { >>> + $errormessage =3D $Lang::tr{"dhcp fixed ip address in dynamic range"= }; >>> + } >>> + } >>> + } >>> + } >>> unless(&General::validmac($dhcpsettings{'FIX_MAC'})) { $errormessage = =3D $Lang::tr{'invalid fixed mac address'}; } >>> if ($dhcpsettings{'FIX_NEXTADDR'}) { >>> unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $error= message =3D $Lang::tr{'invalid fixed ip address'}; } >>> } >>> +# Check if fixed next address is in the dynamic range, if defined >>> + foreach my $itf (@ITFs) { >>> + if ($dhcpsettings{"ENABLE_${itf}"} eq 'on' ) { >>> + if ($dhcpsettings{"START_ADDR_${itf}"}) { >>> + if (&General::ip_address_in_ip_range($dhcpsettings{'FIX_NEXTADDR'= }, >>> + $dhcpsettings{"START_ADDR_${itf}"}, >>> + $dhcpsettings{"END_ADDR_${itf}"})) { >>> + $errormessage =3D $Lang::tr{"dhcp fixed ip address in dynamic range"= }; >>> + } >>> + } >>> + } >>> + } >>> =09 >>> my $key =3D 0; >>> CHECK:foreach my $line (@current2) { >>> diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl >>> index 95a1cfda4..0dbdf7bd5 100644 >>> --- a/langs/en/cgi-bin/en.pl >>> +++ b/langs/en/cgi-bin/en.pl >>> @@ -806,6 +806,9 @@ >>> 'dhcp dns update' =3D> 'DNS Update', >>> 'dhcp dns update algo' =3D> 'Algorithm', >>> 'dhcp dns update secret' =3D> 'Secret', >>> +'dhcp dynamic range overlap' =3D> 'Dynamic range overlapped with ', >>> +'dhcp fixed ip address' =3D> ' Fixed IP Address(es)', >>> +'dhcp fixed ip address in dynamic range' =3D> 'Fixed IP Address in dynam= ic range is not allowed', >>> 'dhcp fixed lease err1' =3D> 'For a fix lease you have to enter the MAC a= ddress or the hostname, or you enter both.', >>> 'dhcp fixed lease help1' =3D> 'IP Addresses might be entered as FQDN', >>> 'dhcp mode' =3D> 'DHCP', >>> --=20 >>> 2.30.1 >>>=20 --===============8514806685380969344==--