Bug 10629 - DHCP server issues duplicate addresses when overlapping fixed lease defined.
Summary: DHCP server issues duplicate addresses when overlapping fixed lease defined.
Status: ON_QA
Alias: None
Product: IPFire
Classification: Unclassified
Component: --- (show other bugs)
Version: 2
Hardware: all All
: - Unknown - Major Usability
Assignee: Adolf Belka
QA Contact:
Depends on:
Reported: 2014-09-23 03:52 UTC by Tom Rymes
Modified: 2023-03-22 08:54 UTC (History)
4 users (show)

See Also:
bbitsch: needinfo+

patch file for evaluation of dhcp.cgi changes (1.63 KB, patch)
2021-01-22 18:50 UTC, Adolf Belka
patch file for evaluation of general-functions.pl changes (972 bytes, patch)
2021-01-22 18:51 UTC, Adolf Belka
patch file for evaluation of en.pl language file changes (899 bytes, patch)
2021-01-22 18:52 UTC, Adolf Belka

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Rymes 2014-09-23 03:52:44 UTC
While keeping the fixed addresses out of the DHCP pool range is ideal, it is not always practicable or convenient. In the current implementation, if you specify a fixed lease that overlaps with the DHCP dynamic address pool, DHCP will issue conflicting addresses under certain circumstances. This is because the range definition is not modified in dhcp.conf to exclude the fixed lease. Here are a few links that discuss this topic:


DHCP Start Address:
DHCP End Address:
Fixed Lease for host foo:
Fixed Least for host bar:

In this scenario, IPFire writes its dhcp.conf (in part) like this:

subnet netmask #BLUE
	option subnet-mask;
	option domain-name "domain.dom";
	option routers;
	option domain-name-servers;
	default-lease-time 86400;
	max-lease-time 172800;

host fix0 # foo
	hardware ethernet aa:bb:cc:dd:ee:ff;

host fix1 # bar
	hardware ethernet ff:ee:dd:cc:bb:aa;

Instead, it should be writing it like this:

subnet netmask #BLUE
	option subnet-mask;


By modifying the range entries to exclude the fixed leases, we can eliminate the possibility that the server will provide the fixed address to a different host.
Comment 1 Bernhard Bitsch 2014-09-23 15:32:55 UTC
I'm working a solution according to my remark in https://bugzilla.ipfire.org/show_bug.cgi?id=10598
Comment 2 Tom Rymes 2014-09-23 16:47:10 UTC
As I have posted on the forum, I would like to chime in here and express my opinion that modifying the WUI to prevent users from defining fixed addresses that overlap the dynamic pool is NOT the proper way to deal with this problem. 

I understand that it is preferable to keep the fixed and dynamic spaces separate (this is how I set up my networks. However, sometimes that is not always easily achieved, and there are any number of situations where choosing not to do that might make sense. For that reason, among others, I very strongly feel that using multiple range statements is a far better solution.
Comment 3 Bernhard Bitsch 2014-09-23 17:55:41 UTC
Because of the IP allocation process in DHCP server it is necessary, that the two sets are disjunct. Therefore in first place we need a check in the WUI to assure this condition.
The discussion about the contents of these two sets is another topic.

The check function is ready now. I'm working on the incorporation into the DHCP page at the moment.
Comment 4 Peter Müller 2018-02-06 20:59:35 UTC
This was in the wrong category. Besides: Is it still valid?
Comment 5 Bernhard Bitsch 2018-02-06 23:00:14 UTC
Don't know.
Didn't work on this since. I also got no more informations.

There two questions remaining:
- Is the violation of the assumption checked?
- Is it worth the effort to construct the range parts suggested in the desciption or is it possible to avoid that sectioning?
Comment 6 Tom Rymes 2018-02-06 23:34:04 UTC
This is still valid. As I see it, there are three options:

1.) Rewrite the dhcp.conf file to break the range up into multiple segments, excluding the fixed leases that overlap.
2.) Prohibit the creation of fixed leases that overlap with the dynamic range.
3.) Leave it as-is and tell users that they should have known better.

#1 is the most complicated, and #2 is easier, but will need to take into account when someone migrates a dynamic lease to DHCP and re-assign that device a new address somehow. Presumably other open source implementations have already dealt with this problem, and perhaps we can implement their solution?

The problem with #3 is that Windows' DHCP server doesn't do this, so many are not expecting this to be a problem. Secondly, the WUI, when you click the "Add" button, will create an overlapping lease by default, as it simply re-uses the existing IP Address, so the WUI is doing it wrong, too.
Comment 7 Bernhard Bitsch 2019-04-17 23:35:09 UTC
Without reading the cited discussions once more ( some time gone since 2014 ;) ),
I think we should do it as follows:
- sets of dynamic IPs and static IPs MUST be disjunct
- condition SHOULD be checked in the WUI
- adding a dynamic lease MAC to the fixed leases set SHOULD request an editing of the definition ( behaviour similiar to 'add new' operation in current Ipfire version ( 130 ) )

During the remainder of the lease time of the dynamic there is double definition, with warnings in the logs. 
We should investigate to delete this unwanted ( and somehow 'illegal' ) dynamic lease definition.
Comment 8 Tom Rymes 2020-11-17 13:15:01 UTC
One more thing to consider is that adding a fixed lease from an existing dynamic lease does not force the user to choose an address outside of the dynamic pool. I see two possible solutions:

1.) Always force segregation of dynamic and fixed leases.
2.) Write the file properly to exclude all of the fixed leases from the pool, even if the numbers overlap.

I think #2 would be easiest. It still allows any user to implement #1 if they really want to, but will require less work to check everywhere to ensure that no entries overlap. 

My $0.02, after inflation.
Comment 9 Adolf Belka 2020-12-30 12:08:08 UTC
I am willing to pick up this bug and have a go at it if I am not treading on any toes by doing that.
Comment 10 Bernhard Bitsch 2020-12-30 12:22:05 UTC
No problem from my side.
I started some work, but didn't really produce a solution.
The dhcp.cgi file is bit messy, thus I think it is necessary to clean up the code.
But looking at the problem, which is solved, that was too much work in generating separate patches for clean up/cosmetics.
With a bit admin discipline, there is no problem. ;)

You can contact me for information on my former steps or for testing your changes.

- Bernhard
Comment 11 Tom Rymes 2020-12-30 15:45:36 UTC
Adolf: I think you will find nothing but thanks for giving it a go.
Comment 12 Adolf Belka 2021-01-19 14:13:31 UTC
I have read through the links and comments in this bug and carried out some searching on the topic.

Splitting the dynamic pool is usually carried out to have a separate contiguous range of IP's for different groups of clients with different options defined.
So for instance one pool could be for clients on a subnet that are given leases with a long lease time and the other pool gives leases in the same subnet but with shorter lease times.

What is being described here is splitting the range up into multiple pools because an existing IP is being moved from dynamic to fixed, not to structurally have different pools for clients groups with different settings.
The WUI would need to be modified to show these multiple ranges otherwise you would end up with multiple ranges in the conf file but not visible in the wui. In fact you would need to keep the ranges separated for all IP's listed in the fixed table, even if they are not enabled.

The other option is to check if an IP is being specified for a fixed address that is from the range and prevent its use with an error message. This then forces the selection of an IP from outside the defined range.
This is how pfSense and OPNsense do it. They prevent the selection of an IP from the dynamic range. I think it makes sense for us to follow the same approach.

Having the ability to have an additional dynamic pool where different dhcp-options can be set could be an extension for the future. pfSense has this Additional Pools option.

Keeping a dynamic pool with one set of dhcp-options contiguous is also talked about in the isc form as "best practice" rather than splitting the pool up.

So based on what I have read about my proposal is to keep a defined contiguous dynamic range intact, check for fixed IP's being specified from this dynamic range and giving an error. For the situation of moving a client from a dynamic address to a fixed address then this would require the IP address to be modified to an IP outside from the dynamic range.

The only downside of this option is that the client will maintain its previous IP until the client network is restarted or the 50% leasetime recorded in the client is reached. This will apply in all situations where an IP address is changed for a client in the server. If moving to the new IP is important then the network admin needs to ensure that the client network is restarted.
Comment 13 Adolf Belka 2021-01-22 18:49:59 UTC
I have made some changes to dhcp.cgi, created a sub-routine in general-functions.pl to detect if an ip is in an ip-range and added an error message into en.pl.

I have tested the changes and they gave me an error message if the Fixed IP Address selected was in the dynamic range. I have tried this with fixed addresses in both Green and Blue and also for the Next Server IP Address.
Also tested for adding an existing dynamic lease to the fixed lease list.

I would appreciate it if someone could test this and confirm that it works as expected or highlight issues that I have missed.

The patches are based on the next git repository.
Comment 14 Adolf Belka 2021-01-22 18:50:59 UTC
Created attachment 851 [details]
patch file for evaluation of dhcp.cgi changes
Comment 15 Adolf Belka 2021-01-22 18:51:29 UTC
Created attachment 852 [details]
patch file for evaluation of general-functions.pl changes
Comment 16 Adolf Belka 2021-01-22 18:52:15 UTC
Created attachment 853 [details]
patch file for evaluation of en.pl language file changes
Comment 17 Adolf Belka 2021-02-13 09:30:29 UTC
No feedback so I will create a patch for the change and send it to the development list.
Comment 18 Adolf Belka 2021-02-14 12:36:18 UTC
I was thinking about the patch for this bug while I was walking to the supermarket today and I suddenly realised that I need to do some more work on it.

The patch works to prevent any fixed ip being specified that overlaps with the dynamic range.

However if the dynamic range is modified to overlap already existing fixed ip's there is nothing to stop that. Just confirmed that.
So need to extend the patch to also check any change to the dynamic range does not overlap the existing fixed ip's (enabled or not).
Comment 19 Adolf Belka 2021-02-17 14:01:56 UTC
Modified patch created and tested and works both for creating fixed lease IP's in the existing dynamic range and for modifying the dynamic range to overlap with existing fixed lease definitions enabled or disabled.

Patch submitted to development list for review and additional testing.

Comment 20 Peter Müller 2021-05-14 17:03:46 UTC
Since the patch proposed by Adolf did not made it into the ipfire-2.x repository (yet), I am resetting this bug back to ASSIGNED.
Comment 21 Adolf Belka 2023-02-14 09:13:59 UTC
Created an updated patch set. The first patch updates the bgcolor entries to css background-color approach as used by @Leo in the Zone Configuration cgi page.

bgcolor was deprecated in HTML4.01 and is no longer used in HTML5

The orange colouring for cells with IP's that are outside of the IPFire green and blue subnets is not showing up on my browsers.

The conversion to using css for bgcolor works for IUP's outside of the subnets.

The second patch then uses the new css approach to highlight in red any IP's in the Fixed Leases table that overlap with the dynamic assigned IP range. The IP's are still usable but being highlighted in red makes it clear that it is an incorrect assignment.


Comment 24 Adolf Belka 2023-03-22 08:54:30 UTC
An update done for the bgcolor fix had an error in it. The css statements were moved to header.pl as they will be used in many of the cgi files so it made sense to move to thye central header.pl file.

However forgot to add the variable definition in header.pl to reference colors.txt which has all the color= statements such as for the shading differentiation for each row in a table.

So this bug has the red shading on any fixed leaswe IP's that are in the defined dynamic range and also the orange shading works again but the row shading for the table does not work due to the above error.

Patch submitted to dev mailing list to fix this error.