Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Adolf Belka (ipfire-dev)" adolf.belka@ipfire.org Cc: development@lists.ipfire.org Betreff: Re: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping
Hi,
On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) adolf.belka@ipfire.org wrote:
Hi Michael,
On 18/02/2021 14:06, Michael Tremer wrote:
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 still 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 should be able to figure out how to do that. I will need to look at how to do Warnings. I have seen them in the code but not looked closely at them yet. This is my opportunity to do so.
Maybe instead of showing this after hitting Save, it is better to show it next to the IP address field it it overlaps. You wouldn’t need to care whether the entry existed before or not.
I think that would help us to inform users about potential problems (which most of them wouldn’t have experienced up to this point in time) and new setup will be correct.
The only thing that would be more difficult is if the user expands an existing dynamic range that already overlaps with two fixed leases so that it now overlaps with four fixed leases. It will be difficult to determine which were the original existing and which are the new overlapped leases without having a parameter stored in a file that counts which leases were overlapping when the Core Update upgrade is carried out. That could be done but I think the simplest will be that any overlap due to a change in the dynamic range should just get a warning and not an error. Does that sound okay.
Oh yeah, difficult question.
I did not assume that this was very dynamic before.
Potentially it is a good solution to simply split the pool in the backend and not tell the user. So in the configuration instead of writing 192.168.0.100-192.168.0.200, you would add a break for every static lease:
- 192.168.0.100 - 192.168.0.105
- 192.168.0.107 - 192.168.0.112
- 192.168.0.114 - 192.168.0.200
In this example, 192.168.0.106 and 192.168.0.113 would be static leases.
That would make the solution transparent for the user, but a pain for the developer.
I do not think, that pools are right tool for the problem. Address pools are used with a slight different semantic in dhcpd. This difference may increase in future. Not mentioning the effort to split the set of possible IP addresses and to verify this process.
Bernhard
But I digress…
I would say the warning is a simple solution to this problem, too.
Is that an acceptable compromise?
Yes, I can live with that.
I will also deal with the other feedback given about the ne in place of the !eq and also the location of the subroutine ip_address_in_ip_range
Just call it “ip_address_in_range”. It's shorter :)
-Michael
Regards,
Adolf.
-Michael
On 18 Feb 2021, at 12:17, Adolf Belka (ipfire-dev) adolf.belka@ipfire.org wrote:
Hi Michael,
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 make 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?
If the static lease is edited to have an IP address in the dynamic range then it will not be possible any more. There will be an error message saying that you have selected an IP from the dynamic range. It will not be entered into the dhcp.conf file.
If we don't implement this fix then anyone who defines a fixed IP address from the dynamic range is doing something that ISC DHCP say should not be done.
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 to a different computer.
pfSense and OPNsense have this restriction implemented in their WUI.
Regards,
Adolf.
On 17 Feb 2021, at 13:58, Adolf Belka adolf.belka@ipfire.org wrote:
- 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 dynamic
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
Signed-off-by: Adolf Belka adolf.belka@ipfire.org
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(+)
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{'RED_NETMASK'}", $network)){ $errormessage=$Lang::tr{'ccd err red'};return $errormessage;} }
+sub ip_address_in_ip_range($$) { +# Returns True if $ipaddress is within $ipstart and $ipend range.
- my $ipaddress = shift;
- my $ipstart = shift;
- my $ipend = shift;
- my $ipaddress_bin = &Network::ip2bin($ipaddress);
- return undef unless (defined $ipaddress_bin);
- my $ipstart_bin = &Network::ip2bin($ipstart);
- return undef unless (defined $ipstart_bin);
- my $ipend_bin = &Network::ip2bin($ipend);
- return undef unless (defined $ipend_bin);
- return (($ipaddress_bin >= $ipstart_bin) && ($ipaddress_bin <= $ipend_bin));
+}
This function should live in network-functions.pl since it clearly is a network function :) Ideally a test could be added for it at the end of it.
sub validport { $_ = $_[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 = Desired +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: disable = Disable WARNING: untranslated string: enable = Enable WARNING: untranslated string: error the to date has to be later than the from date = 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 = Key Name WARNING: untranslated string: dhcp dns update = DNS Update WARNING: untranslated string: dhcp dns update algo = Algorithm WARNING: untranslated string: dhcp dns update secret = Secret +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp server = DHCP Server WARNING: untranslated string: dhcp server disabled = DHCP server disabled. Stopped. WARNING: untranslated string: dhcp server enabled = 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 = Key Name WARNING: untranslated string: dhcp dns update = DNS Update WARNING: untranslated string: dhcp dns update algo = Algorithm WARNING: untranslated string: dhcp dns update secret = Secret +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp valid range required when deny known clients checked = Valid range required when "Deny known clients:" is checked WARNING: untranslated string: disable = Disable WARNING: untranslated string: disconnected = 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: = Deny known clients: +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp valid range required when deny known clients checked = Valid range required when "Deny known clients:" is checked WARNING: untranslated string: fwhost cust locationgrp = unknown string WARNING: untranslated string: fwhost err hostip = 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 = Key Name WARNING: untranslated string: dhcp dns update = DNS Update WARNING: untranslated string: dhcp dns update algo = Algorithm WARNING: untranslated string: dhcp dns update secret = Secret +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp valid range required when deny known clients checked = Valid range required when "Deny known clients:" is checked WARNING: untranslated string: disable = Disable WARNING: untranslated string: disconnected = 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 = Key Name WARNING: untranslated string: dhcp dns update = DNS Update WARNING: untranslated string: dhcp dns update algo = Algorithm WARNING: untranslated string: dhcp dns update secret = Secret +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: disable = Disable WARNING: untranslated string: disconnected = Disconnected WARNING: untranslated string: dl client arch insecure = Download insecure 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 = Key Name WARNING: untranslated string: dhcp dns update = DNS Update WARNING: untranslated string: dhcp dns update algo = Algorithm WARNING: untranslated string: dhcp dns update secret = Secret +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp valid range required when deny known clients checked = Valid range required when "Deny known clients:" is checked WARNING: untranslated string: disable = Disable WARNING: untranslated string: disconnected = 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 = Key Name WARNING: untranslated string: dhcp dns update = DNS Update WARNING: untranslated string: dhcp dns update algo = Algorithm WARNING: untranslated string: dhcp dns update secret = Secret +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp valid range required when deny known clients checked = Valid range required when "Deny known clients:" is checked WARNING: untranslated string: disable = Disable WARNING: untranslated string: disconnected = 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 = Dangerous WARNING: untranslated string: default IP address = Default IP Address WARNING: untranslated string: desired = Desired WARNING: untranslated string: dhcp deny known clients: = Deny known clients: +WARNING: untranslated string: dhcp dynamic range overlap = Dynamic range overlapped with +WARNING: untranslated string: dhcp fixed ip address = Fixed IP Address(es) WARNING: untranslated string: dhcp valid range required when deny known clients checked = Valid range required when "Deny known clients:" is checked WARNING: untranslated string: disable = Disable WARNING: untranslated string: disconnected = 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 = <FILE>; close(FILE);
# 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'}) { } }
# Check if dynamic range and Fixed IP Addresses overlapif ((!$dhcpsettings{"START_ADDR_${itf}"}) eq '' && (!$dhcpsettings{"END_ADDR_${itf}"}) eq '') {For better readability, writing “ne” instead of !eq might be a good idea.
my $count=0;foreach my $line (@current2) {chomp($line);my @temp = split(/\,/,$line);if (&General::ip_address_in_ip_range($temp[1],$dhcpsettings{"START_ADDR_${itf}"},$dhcpsettings{"END_ADDR_${itf}"})) {$count++;}}if ($count > 0) {$errormessage = "DHCP on ${itf}: " . $Lang::tr{'dhcp dynamic range overlap'} . $count . $Lang::tr{'dhcp fixed ip address'};goto ERROR;}}if (!($dhcpsettings{"DEFAULT_LEASE_TIME_${itf}"} =~ /^\d+$/)) {@@ -415,10 +434,34 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'toggle enable disable'}.'2') { if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { $dhcpsettings{'FIX_MAC'} =~ tr/-/:/; unless(&General::validip($dhcpsettings{'FIX_ADDR'})) { $errormessage = $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 = $Lang::tr{"dhcp fixed ip address in dynamic range"};}}- }
- } unless(&General::validmac($dhcpsettings{'FIX_MAC'})) { $errormessage = $Lang::tr{'invalid fixed mac address'}; } if ($dhcpsettings{'FIX_NEXTADDR'}) { unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $errormessage = $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 = $Lang::tr{"dhcp fixed ip address in dynamic range"};}}}
}
my $key = 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' => 'DNS Update', 'dhcp dns update algo' => 'Algorithm', 'dhcp dns update secret' => 'Secret', +'dhcp dynamic range overlap' => 'Dynamic range overlapped with ', +'dhcp fixed ip address' => ' Fixed IP Address(es)', +'dhcp fixed ip address in dynamic range' => 'Fixed IP Address in dynamic range is not allowed', 'dhcp fixed lease err1' => 'For a fix lease you have to enter the MAC address or the hostname, or you enter both.', 'dhcp fixed lease help1' => 'IP Addresses might be entered as FQDN', 'dhcp mode' => 'DHCP', -- 2.30.1