diff mbox series

bug#10629: Prevent dynamic and fixed leases overlapping

Message ID	20210217135826.3705690-1-adolf.belka@ipfire.org
State	Changes Requested
Headers	From: Adolf Belka <adolf.belka@ipfire.org> To: development@lists.ipfire.org Subject: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping Date: Wed, 17 Feb 2021 14:58:26 +0100 Message-Id: <20210217135826.3705690-1-adolf.belka@ipfire.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: development-bounces@lists.ipfire.org Sender: "Development" <development-bounces@lists.ipfire.org>
Series	bug#10629: Prevent dynamic and fixed leases overlapping \| bug#10629: Prevent dynamic and fixed leases overlapping

Commit Message

Adolf Belka Feb. 17, 2021, 1:58 p.m. UTC

  - 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(+)

Comments

Bernhard Bitsch Feb. 17, 2021, 7:38 p.m. UTC | #1

Hi,
just an annotation, see below

> Gesendet: Mittwoch, 17. Februar 2021 um 14:58 Uhr
> Von: "Adolf Belka" <adolf.belka@ipfire.org>
> An: development@lists.ipfire.org
> Betreff: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping
>
> - 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));
> +}
> +
>  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 overlap
> +	    if ((!$dhcpsettings{"START_ADDR_${itf}"}) eq '' && (!$dhcpsettings{"END_ADDR_${itf}"}) eq '') {

Shouldn't this read
            if (($dhcpsettings{"START_ADDR_${itf}"}) ne '' && ($dhcpsettings{"END_ADDR_${itf}"}) ne '') {

- Bernhard
> +		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+$/)) {
>  		$errormessage = "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 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
>
>

Adolf Belka Feb. 17, 2021, 9:21 p.m. UTC | #2

Hi Bernhard,

Yes that does look better. I will wait a few days for any other feedback and then create a v2 patch.
Thanks very much.

Adolf.

On 17/02/2021 20:38, Bernhard Bitsch wrote:
> Hi,
> just an annotation, see below
> 
>> Gesendet: Mittwoch, 17. Februar 2021 um 14:58 Uhr
>> Von: "Adolf Belka" <adolf.belka@ipfire.org>
>> An: development@lists.ipfire.org
>> Betreff: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping
>>
>> - 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));
>> +}
>> +
>>   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 overlap
>> +	    if ((!$dhcpsettings{"START_ADDR_${itf}"}) eq '' && (!$dhcpsettings{"END_ADDR_${itf}"}) eq '') {
> 
> Shouldn't this read
>              if (($dhcpsettings{"START_ADDR_${itf}"}) ne '' && ($dhcpsettings{"END_ADDR_${itf}"}) ne '') {
> 
> - Bernhard
>> +		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+$/)) {
>>   		$errormessage = "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 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
>>
>>

Michael Tremer Feb. 18, 2021, 11:37 a.m. UTC | #3

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?

> 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 overlap
> +	    if ((!$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+$/)) {
> 		$errormessage = "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 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
>

Adolf Belka Feb. 18, 2021, 12:17 p.m. UTC | #4

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 overlap
>> +	    if ((!$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+$/)) {
>> 		$errormessage = "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 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
>>
>

Michael Tremer Feb. 18, 2021, 1:06 p.m. UTC | #5

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 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.

Is that an acceptable compromise?

-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 overlap
>>> +	    if ((!$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+$/)) {
>>> 		$errormessage = "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 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
>>>

Adolf Belka Feb. 18, 2021, 2:01 p.m. UTC | #6

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.

> 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.
> 
> 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

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 overlap
>>>> +	    if ((!$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+$/)) {
>>>> 		$errormessage = "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 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
>>>>
>

Michael Tremer Feb. 18, 2021, 3:18 p.m. UTC | #7

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.

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 overlap
>>>>> +	    if ((!$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+$/)) {
>>>>> 		$errormessage = "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 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
>>>>>

Bernhard Bitsch Feb. 18, 2021, 3:29 p.m. UTC | #8

> 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 overlap
> >>>>> +	    if ((!$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+$/)) {
> >>>>> 		$errormessage = "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 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
> >>>>> 
> 
>

Tom Rymes Feb. 18, 2021, 4:05 p.m. UTC | #9

> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> 
>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>> 
>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>> 
>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>> 

[snip]

>>> 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.

[snip]

This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.

Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.

Tom

Bernhard Bitsch Feb. 18, 2021, 4:23 p.m. UTC | #10

Hi,


> Gesendet: Donnerstag, 18. Februar 2021 um 17:05 Uhr
> Von: "Tom Rymes" <tom@rymes.net>
> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
> Cc: development@lists.ipfire.org
> Betreff: Re: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping
>
> 
> 
> > On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> > 
> >> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
> >> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> >> 
> >>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
> >>> 
> >>> On 18/02/2021 14:06, Michael Tremer wrote:
> >>> 
> 
> [snip]
> 
> >>> 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.
> 
> [snip]
> 
> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
> 
> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
> 
> Tom

In instances where it is a big effort to change a host's IP, it is also the (same?) effort to set this IP. Therefore it is possible to define this IP as a fixed lease for this (known!) device.

Bernhard

Adolf Belka Feb. 18, 2021, 5:08 p.m. UTC | #11

Hi All,

On 18/02/2021 17:05, Tom Rymes wrote:
>
>> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>
>>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>
>>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>>>
> [snip]
>
>>>> 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.
> [snip]
>
> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
>
> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
>
> Tom

I investigated this when I picked up this bug and I came to the same conclusion as Bernhard.

dhcpd has separate address pools so that users can have a range of dynamic IP's that have tailored options for specific clients.

You can then have a specific address pool for a group of clients that for instance have a short default lease time and specific ntp and dns server addresses etc. Another address pool can have a longer default lease time and different ntp and dns servers etc

pfSense has implemented Additional Pools into its WUI and that could be something that IPFire looks at later. If we have created already split pools with no specified options then we have to decide how to deal with these when we do move to additional pools. We are likely to run into the same problem as we have now of being limited in what we do because the code has already been modified in a different direction that could create problems for those existing users, so we end up deciding that Additional Pools is too difficult to do because it could disrupt existing users. They might not even know they have split pools. Then we would have to figure out how to deal with split pools that don't want to be seen as Additional Pools and with those that do.

If we stay as we are with a single pool then moving to Additional Pools at some future time is not obstructed. With the current bug fix we will have warnings for existing users that what they are doing has problems but it won't block them from using their existing fixed leases that overlap with the dynamic range. If they cannot change they at least know that they are doing something wrong and could have problems. We don't create an obstruction that make future improvements difficult or impossible to implement.

Regards,

Adolf.

Tom Rymes Feb. 18, 2021, 10:38 p.m. UTC | #12

> On Feb 18, 2021, at 11:23 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> 
> Hi,
> 
> 
>> Gesendet: Donnerstag, 18. Februar 2021 um 17:05 Uhr
>> Von: "Tom Rymes" <tom@rymes.net>
>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>> Cc: development@lists.ipfire.org
>> Betreff: Re: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping
>> 
>> 
>> 
>>>> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>> 
>>>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>> 
>>>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>>> 
>>>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>>>> 
>> 
>> [snip]
>> 
>>>>> 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.
>> 
>> [snip]
>> 
>> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
>> 
>> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
>> 
>> Tom
> 
> In instances where it is a big effort to change a host's IP, it is also the (same?) effort to set this IP. Therefore it is possible to define this IP as a fixed lease for this (known!) device.
> 
> Bernhard

Bernhard: not really. The way I see it is that someone sets up a server, then assigns it a reserved/fixed address that overlaps with the dynamic range. Years pass. I inherit this network and I have to move the IP out of the dynamic range, but I’m going to have to maniacally change various clients, etc.

It’s not the end of the world, but the currently proposed strategy requires me to move that IP, whereas the option of multiple range definitions per subnet ( excluding the reserved addresses) allows me to keep it for now and move it when. I  able to properly plan to do so.

Tom

Tom Rymes Feb. 18, 2021, 10:40 p.m. UTC | #13

> On Feb 18, 2021, at 12:08 PM, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
> 
> Hi All,
> 
>> On 18/02/2021 17:05, Tom Rymes wrote:
>> 
>>>> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>> 
>>>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>> 
>>>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>>>> 
>> [snip]
>> 
>>>>> 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.
>> [snip]
>> 
>> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
>> 
>> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
>> 
>> Tom
> 
> I investigated this when I picked up this bug and I came to the same conclusion as Bernhard.
> 
> dhcpd has separate address pools so that users can have a range of dynamic IP's that have tailored options for specific clients.
> 
> You can then have a specific address pool for a group of clients that for instance have a short default lease time and specific ntp and dns server addresses etc. Another address pool can have a longer default lease time and different ntp and dns servers etc
> 
> pfSense has implemented Additional Pools into its WUI and that could be something that IPFire looks at later. If we have created already split pools with no specified options then we have to decide how to deal with these when we do move to additional pools. We are likely to run into the same problem as we have now of being limited in what we do because the code has already been modified in a different direction that could create problems for those existing users, so we end up deciding that Additional Pools is too difficult to do because it could disrupt existing users. They might not even know they have split pools. Then we would have to figure out how to deal with split pools that don't want to be seen as Additional Pools and with those that do.
> 
> If we stay as we are with a single pool then moving to Additional Pools at some future time is not obstructed. With the current bug fix we will have warnings for existing users that what they are doing has problems but it won't block them from using their existing fixed leases that overlap with the dynamic range. If they cannot change they at least know that they are doing something wrong and could have problems. We don't create an obstruction that make future improvements difficult or impossible to implement.
> 
> 
> Regards,
> 
> Adolf.
> 

Adolf,

Isn’t the only way to make these multiple pools work to manually specify which MAC addresses qualify for each pool? Doesn’t that make them fixed leases?

I think I’m missing a piece of this puzzle.

Tom

Adolf Belka Feb. 19, 2021, 11:37 a.m. UTC | #14

Hi Tom,

On 18/02/2021 23:40, Tom Rymes wrote:
> 
> 
>> On Feb 18, 2021, at 12:08 PM, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>
>> Hi All,
>>
>>> On 18/02/2021 17:05, Tom Rymes wrote:
>>>
>>>>> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>
>>>>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>
>>>>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>>>>>
>>> [snip]
>>>
>>>>>> 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.
>>> [snip]
>>>
>>> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
>>>
>>> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
>>>
>>> Tom
>>
>> I investigated this when I picked up this bug and I came to the same conclusion as Bernhard.
>>
>> dhcpd has separate address pools so that users can have a range of dynamic IP's that have tailored options for specific clients.
>>
>> You can then have a specific address pool for a group of clients that for instance have a short default lease time and specific ntp and dns server addresses etc. Another address pool can have a longer default lease time and different ntp and dns servers etc
>>
>> pfSense has implemented Additional Pools into its WUI and that could be something that IPFire looks at later. If we have created already split pools with no specified options then we have to decide how to deal with these when we do move to additional pools. We are likely to run into the same problem as we have now of being limited in what we do because the code has already been modified in a different direction that could create problems for those existing users, so we end up deciding that Additional Pools is too difficult to do because it could disrupt existing users. They might not even know they have split pools. Then we would have to figure out how to deal with split pools that don't want to be seen as Additional Pools and with those that do.
>>
>> If we stay as we are with a single pool then moving to Additional Pools at some future time is not obstructed. With the current bug fix we will have warnings for existing users that what they . That pool then covers are doing has problems but it won't block them from using their existing fixed leases that overlap with the dynamic range. If they cannot change they at least know that they are doing something wrong and could have problems. We don't create an obstruction that make future improvements difficult or impossible to implement.
>>
>>
>> Regards,
>>
>> Adolf.
>>
> 
> Adolf,
> 
> Isn’t the only way to make these multiple pools work to manually specify which MAC addresses qualify for each pool? Doesn’t that make them fixed leases?

For one pool you have allow unknown-clients. That pool then acts like the normal dynamic allocation of IPs.
The second pool you have deny unknown-clients. This then allows clients with host declarations that don't have fixed IP addresses defined to get dynamic IPs from a different range.
Then you still have the host declarations where you define the IP address which are then like the traditional IPFire Fixed Leases.

The second pool does require mac addresses to be defined but not IP addresses. Currently IPFire only has host declarations with both mac address and IP address.

So you then have unknown clients getting one dynamic IP range, known clients that don't have IPs preset getting another dynamic IP range and known clients with defined IPs getting those IPs. You can then have settings like DNS server, nip server, default lease time etc defined differently at a global level and for each address pool.


Another way is to create a class based on the option host-name if the client has been set up to send it. You can select all those clients that have part of their host-name defined in a certain way. i.e. you have clients that are named foobar-xyz then you can create a class for all clients that have a hostname starting with foobar. Then you can create a pool that only applies to members of that class.


None of the above can be done currently with the WUI but would be possible with dhcpd if someone wanted to implement it into the WUI.

Regards,

Adolf.

> 
> I think I’m missing a piece of this puzzle.
> 
> Tom
>

Michael Tremer Feb. 19, 2021, 6:57 p.m. UTC | #15

Hi,

Great discussion everyone.

I guess we have two problems on our hand:

1) How do we do this right?

2) How do we make it compatible with the current UI?

Rewriting the whole UI is a big thing and changing anything *will* break people’s networks. That is something we cannot do.

In IPFire 3, I added the option to add multiple pools and in those add multiple ranges of IP addresses that can be assigned. Maybe those who understood how this is supposed to work can review this and tell me if this was a good idea, or if I got it wrong:

  https://git.ipfire.org/?p=network.git;a=blob;f=src/functions/functions.dhcpd;h=3b1214fb937d0efd8ea8c3a4d4113f13bc3b5d52;hb=HEAD

Maybe we can use this as a study for question 1) because we do not need to care about compatibility.

Secondly, we need to look at IPFire 2 and *if* this can be done right without breaking anything.

-Michael

> On 19 Feb 2021, at 11:37, Adolf Belka (ipfire) <adolf.belka@ipfire.org> wrote:
> 
> Hi Tom,
> 
> On 18/02/2021 23:40, Tom Rymes wrote:
>>> On Feb 18, 2021, at 12:08 PM, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>> 
>>> Hi All,
>>> 
>>>> On 18/02/2021 17:05, Tom Rymes wrote:
>>>> 
>>>>>> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>> 
>>>>>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>> 
>>>>>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>>>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>>>>>> 
>>>> [snip]
>>>> 
>>>>>>> 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.
>>>> [snip]
>>>> 
>>>> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
>>>> 
>>>> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
>>>> 
>>>> Tom
>>> 
>>> I investigated this when I picked up this bug and I came to the same conclusion as Bernhard.
>>> 
>>> dhcpd has separate address pools so that users can have a range of dynamic IP's that have tailored options for specific clients.
>>> 
>>> You can then have a specific address pool for a group of clients that for instance have a short default lease time and specific ntp and dns server addresses etc. Another address pool can have a longer default lease time and different ntp and dns servers etc
>>> 
>>> pfSense has implemented Additional Pools into its WUI and that could be something that IPFire looks at later. If we have created already split pools with no specified options then we have to decide how to deal with these when we do move to additional pools. We are likely to run into the same problem as we have now of being limited in what we do because the code has already been modified in a different direction that could create problems for those existing users, so we end up deciding that Additional Pools is too difficult to do because it could disrupt existing users. They might not even know they have split pools. Then we would have to figure out how to deal with split pools that don't want to be seen as Additional Pools and with those that do.
>>> 
>>> If we stay as we are with a single pool then moving to Additional Pools at some future time is not obstructed. With the current bug fix we will have warnings for existing users that what they . That pool then covers are doing has problems but it won't block them from using their existing fixed leases that overlap with the dynamic range. If they cannot change they at least know that they are doing something wrong and could have problems. We don't create an obstruction that make future improvements difficult or impossible to implement.
>>> 
>>> 
>>> Regards,
>>> 
>>> Adolf.
>>> 
>> Adolf,
>> Isn’t the only way to make these multiple pools work to manually specify which MAC addresses qualify for each pool? Doesn’t that make them fixed leases?
> 
> For one pool you have allow unknown-clients. That pool then acts like the normal dynamic allocation of IPs.
> The second pool you have deny unknown-clients. This then allows clients with host declarations that don't have fixed IP addresses defined to get dynamic IPs from a different range.
> Then you still have the host declarations where you define the IP address which are then like the traditional IPFire Fixed Leases.
> 
> The second pool does require mac addresses to be defined but not IP addresses. Currently IPFire only has host declarations with both mac address and IP address.
> 
> So you then have unknown clients getting one dynamic IP range, known clients that don't have IPs preset getting another dynamic IP range and known clients with defined IPs getting those IPs. You can then have settings like DNS server, nip server, default lease time etc defined differently at a global level and for each address pool.
> 
> 
> Another way is to create a class based on the option host-name if the client has been set up to send it. You can select all those clients that have part of their host-name defined in a certain way. i.e. you have clients that are named foobar-xyz then you can create a class for all clients that have a hostname starting with foobar. Then you can create a pool that only applies to members of that class.
> 
> 
> None of the above can be done currently with the WUI but would be possible with dhcpd if someone wanted to implement it into the WUI.
> 
> Regards,
> 
> Adolf.
> 
>> I think I’m missing a piece of this puzzle.
>> Tom

Adolf Belka Feb. 21, 2021, 2:02 p.m. UTC | #16

Hi Michael & Tom

On 19/02/2021 19:57, Michael Tremer wrote:
> Hi,
> 
> Great discussion everyone.
> 
> I guess we have two problems on our hand:
> 
> 1) How do we do this right?
> 
> 2) How do we make it compatible with the current UI?
> 
> Rewriting the whole UI is a big thing and changing anything *will* break people’s networks. That is something we cannot do.
> 
> In IPFire 3, I added the option to add multiple pools and in those add multiple ranges of IP addresses that can be assigned. Maybe those who understood how this is supposed to work can review this and tell me if this was a good idea, or if I got it wrong:
> 
>    https://git.ipfire.org/?p=network.git;a=blob;f=src/functions/functions.dhcpd;h=3b1214fb937d0efd8ea8c3a4d4113f13bc3b5d52;hb=HEAD
> 
> Maybe we can use this as a study for question 1) because we do not need to care about compatibility.
> 
> Secondly, we need to look at IPFire 2 and *if* this can be done right without breaking anything.

I think the suggestion to only have a warning for existing overlapped 
leases and an error for creating new ones is a good solution. It allows 
existing systems with overlapped IP's to continue running but makes the 
issue visible. The error message to prevent creating new overlapped IP's 
makes sure that the problem is not extended further.

Regards,

Adolf.

> 
> -Michael
> 
>> On 19 Feb 2021, at 11:37, Adolf Belka (ipfire) <adolf.belka@ipfire.org> wrote:
>>
>> Hi Tom,
>>
>> On 18/02/2021 23:40, Tom Rymes wrote:
>>>> On Feb 18, 2021, at 12:08 PM, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>>
>>>> Hi All,
>>>>
>>>>> On 18/02/2021 17:05, Tom Rymes wrote:
>>>>>
>>>>>>> On Feb 18, 2021, at 10:29 AM, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>>>
>>>>>>> Gesendet: Donnerstag, 18. Februar 2021 um 16:18 Uhr
>>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>>>
>>>>>>>>> On 18 Feb 2021, at 14:01, Adolf Belka (ipfire-dev) <adolf.belka@ipfire.org> wrote:
>>>>>>>> On 18/02/2021 14:06, Michael Tremer wrote:
>>>>>>>>
>>>>> [snip]
>>>>>
>>>>>>>> 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.
>>>>> [snip]
>>>>>
>>>>> This was my original suggestion for how to handle this, and at the time I opened the big, it was my understanding that this was the proper way to handle this. Others have arrived at a different conclusion, and to me it seems like more of a style thing than a functional thing.
>>>>>
>>>>> Using multiple range definitions to exclude fixed addresses also has the benefit of allowing a user to keep a host’s address the same, which can be very helpful in instances where a lot of work may be required to change a host’s IP.
>>>>>
>>>>> Tom
>>>>
>>>> I investigated this when I picked up this bug and I came to the same conclusion as Bernhard.
>>>>
>>>> dhcpd has separate address pools so that users can have a range of dynamic IP's that have tailored options for specific clients.
>>>>
>>>> You can then have a specific address pool for a group of clients that for instance have a short default lease time and specific ntp and dns server addresses etc. Another address pool can have a longer default lease time and different ntp and dns servers etc
>>>>
>>>> pfSense has implemented Additional Pools into its WUI and that could be something that IPFire looks at later. If we have created already split pools with no specified options then we have to decide how to deal with these when we do move to additional pools. We are likely to run into the same problem as we have now of being limited in what we do because the code has already been modified in a different direction that could create problems for those existing users, so we end up deciding that Additional Pools is too difficult to do because it could disrupt existing users. They might not even know they have split pools. Then we would have to figure out how to deal with split pools that don't want to be seen as Additional Pools and with those that do.
>>>>
>>>> If we stay as we are with a single pool then moving to Additional Pools at some future time is not obstructed. With the current bug fix we will have warnings for existing users that what they . That pool then covers are doing has problems but it won't block them from using their existing fixed leases that overlap with the dynamic range. If they cannot change they at least know that they are doing something wrong and could have problems. We don't create an obstruction that make future improvements difficult or impossible to implement.
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Adolf.
>>>>
>>> Adolf,
>>> Isn’t the only way to make these multiple pools work to manually specify which MAC addresses qualify for each pool? Doesn’t that make them fixed leases?
>>
>> For one pool you have allow unknown-clients. That pool then acts like the normal dynamic allocation of IPs.
>> The second pool you have deny unknown-clients. This then allows clients with host declarations that don't have fixed IP addresses defined to get dynamic IPs from a different range.
>> Then you still have the host declarations where you define the IP address which are then like the traditional IPFire Fixed Leases.
>>
>> The second pool does require mac addresses to be defined but not IP addresses. Currently IPFire only has host declarations with both mac address and IP address.
>>
>> So you then have unknown clients getting one dynamic IP range, known clients that don't have IPs preset getting another dynamic IP range and known clients with defined IPs getting those IPs. You can then have settings like DNS server, nip server, default lease time etc defined differently at a global level and for each address pool.
>>
>>
>> Another way is to create a class based on the option host-name if the client has been set up to send it. You can select all those clients that have part of their host-name defined in a certain way. i.e. you have clients that are named foobar-xyz then you can create a class for all clients that have a hostname starting with foobar. Then you can create a pool that only applies to members of that class.
>>
>>
>> None of the above can be done currently with the WUI but would be possible with dhcpd if someone wanted to implement it into the WUI.
>>
>> Regards,
>>
>> Adolf.
>>
>>> I think I’m missing a piece of this puzzle.
>>> Tom
>

Tom Rymes Feb. 21, 2021, 4:33 p.m. UTC | #17

> On Feb 21, 2021, at 9:02 AM, Adolf Belka (ipfire) <adolf.belka@ipfire.org> wrote:
> 
> Hi Michael & Tom

[snip]

> I think the suggestion to only have a warning for existing overlapped leases and an error for creating new ones is a good solution. It allows existing systems with overlapped IP's to continue running but makes the issue visible. The error message to prevent creating new overlapped IP's makes sure that the problem is not extended further.
> 
> Regards,
> 
> Adolf.

Adolf,

So long as the user isn’t forced to change pre-existing fixed leases before adding new ones, this is fine by me, a good compromise, I suppose. 

Perhaps it would be wise to highlight any rows that do over lap to alert the user that they should endeavor to fix them?

Tom

diff mbox series

Patch

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));
+}
+
 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 overlap
+	    if ((!$dhcpsettings{"START_ADDR_${itf}"}) eq '' && (!$dhcpsettings{"END_ADDR_${itf}"}) eq '') {
+		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+$/)) {
 		$errormessage = "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 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',