Subnetting is not working with most relevant entry

mgg · May 3, 2020, 6:28am

Problem with Beta 5.0:
I have added a subnet entry for a /24 network and assigned it to group id 1.
Also I have added a native Client IP-Address with /32 and assigned it to group id 0 and group id 2.

The client will get the group id 1.

The same behavior when adding the client with the dropdown menu.

As I saw, in the selection from the database the first matching record is taken.
See line 278-279 in get_client_groupids (gravitiy-db.c). The Select is limited to 1.
Maybe there must be evaluated if there are more than 1 matching entry. If there is only one everything works great.

What is expected
In my opinion the most relevant entry should be taken.
So the client will be assigned to group id 0 and 2.

Im placing a log snippet where the client-init is shown.
Log Snippet

[2020-05-03 07:56:32.880 29303] Initializing gravity statements for 10.4.0.189
[2020-05-03 07:56:32.880 29303] Querying gravity database for client 10.4.0.189 (counting)
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 10.4.0.0/24 (database) - !! MATCH !!
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 10.4.0.189/32 (database) - !! MATCH !!
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 10.8.1.0/24 (database) - NO MATCH
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 192.168.168.111 (database) - NO MATCH
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 192.168.168.139 (database) - NO MATCH
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 192.168.168.141 (database) - NO MATCH
[2020-05-03 07:56:32.881 29303] SQL: Comparing 10.4.0.189 vs. 192.168.168.210 (database) - NO MATCH
[2020-05-03 07:56:32.881 29303] Querying gravity database for client 10.4.0.189 (getting groups)
[2020-05-03 07:56:32.882 29303] SQL: Comparing 10.4.0.189 vs. 10.4.0.0/24 (database) - !! MATCH !!
[2020-05-03 07:56:32.882 29303] gravityDB_open(): Preparing vw_whitelist statement for client 10.4.0.189
[2020-05-03 07:56:32.882 29303] get_client_querystr: SELECT EXISTS(SELECT domain from vw_whitelist WHERE domain = ? AND group_id IN (1));

DL6ER · May 3, 2020, 9:25am

Yes. SQLite is unable to do subnetting at all so we had to teach it how this works ourselves. The current implementation can only decide if two IP addresses are the same under a given bitmask due to certain subnet. Our self-engineered SQL subroutine currently only returns 1 (= true) if the addresses match (under the given bitmask) and 0 if they don't match.

We could change this to return the number of bits that a given bitmask has set and then chose the maximum match (in case there is any). This would implement the desired behavior. Pi-hole v5.0 already entered feature-freeze mode and I wouldn't like to postpone the nearing release any further, but implementing this for v5.1 is surely doable. It will be a certain performance hit, however, it shouldn't be a hefty one.

mgg · May 3, 2020, 12:20pm

Thank‘s for the reply.
I fully agree to ship 5.0 with all the great features as soon as possible.

So i‘m looking forward to 5.1

yubiuser · May 4, 2020, 7:54am

Thanks @mgg for pointing out this behavior.
I'm with you that pihole should select group ids based on the smallest IP range for a given client (meaning it excludes the client from group assignments of wider IP ranges)

But I can think of another possible way how users might interpret /24 and /32 configurations - inclusive. "All clients in /24 should fall in group X and client /32 should additionally fall in group Y".

Therefore I suggest to add a small note to the Group management/client page about this behavior.
"Note: If a client is part of more than one IP range only the most exact group assignment is applicable."

mgg · May 4, 2020, 8:10am

Hi @yubiuser,
yes I think that should be noted in the manual.

At the moment (5.0) a client must only match one IP Range to get the expected group assignment.

DL6ER · May 4, 2020, 9:45am

Even wen we add this, you can still add multiple CIDR masks which are unique but still the same. For instance,

127.0.0.1/24
127.0.0.123/24

are actually the same thing. Due to algorithm optimization reasons, this will still lead to unpredictable behavior as in you cannot forecast which of the two most exact matches will be the chosen one. Most often, it will be the first added one (as it comes first in the index table), however, it may also be the second one in case you had a lot of groups and clients at some point, deleted them and added new ones. Your database will still be fast, however, it may not be in sequential order any longer.

yubiuser · May 4, 2020, 9:50am

Mhh.. I think this might be a potential source of user's misconfiguration. Can you add a check if an identical subnet is already defined? (Like it is for entries in general by SQL's unique constrain)

DL6ER · May 4, 2020, 11:15am

No, for one because SQL had no idea what IP addresses are and how they could be unique under certain conditions and also because users could still add such entries through the CLI/direct database interactions.

I have something more in the pipeline to address such issues...

yubiuser · May 4, 2020, 11:17am

DL6ER · May 8, 2020, 7:19pm

6 posts were split to a new topic: Client subnet mask incorrect

DL6ER · May 11, 2020, 6:36pm

This will ensure we will use the most recent + make the choice deterministic even in case of duplicates

github.com/pi-hole/FTL

Always chose best (= maximum match) subnet for clients

pi-hole:development ← pi-hole:tweak/best_subnet

opened 06:32PM - 11 May 20 UTC

DL6ER

+70 -34

**By submitting this pull request, I confirm the following:** - [X] I have re…ad and understood the [contributors guide](https://github.com/pi-hole/pi-hole/blob/master/CONTRIBUTING.md). - [X] I have checked that [another pull request](https://github.com/pi-hole/FTL/pulls) for this purpose does not exist. - [X] I have considered, and confirmed that this submission will be valuable to others. - [X] I accept that this submission may not be used, and the pull request closed at the will of the maintainer. - [X] I give this submission freely, and claim no ownership to its content. **How familiar are you with the codebase?:** ## 10 --- This allows to configure specific settings for a whole range of devices but still exclude others. Complain softly (no error) if multiple configured subnets match with the same number of relevant bits. We log in case there are multiple groups defined matching equally which one FTL chose. It will deterministic chose the most matching *last added one*. ![Screenshot at 2020-05-11 20-25-22](https://user-images.githubusercontent.com/16748619/81597148-8e180280-93c5-11ea-9a23-8d834d38fd4d.png) resulting in the following log line (only the last line is logged in non-DEBUG mode, I kept all for illustrative proposes here): ```plain [2020-05-11 20:26:55.425 18858] SQL: Comparing 127.0.0.1 vs. 127.0.0.0/8 (subnet 255.0.0.0) - !! MATCH !! [2020-05-11 20:26:55.425 18858] SQL: Comparing 127.0.0.1 vs. 127.0.0.0/8 (subnet 255.0.0.0) - !! MATCH !! [2020-05-11 20:26:55.425 18858] SQL: Comparing 127.0.0.1 vs. 127.0.0.1/8 (subnet 255.0.0.0) - !! MATCH !! [2020-05-11 20:26:55.425 18858] SQL: Comparing 127.0.0.1 vs. 127.0.0.1/8 (subnet 255.0.0.0) - !! MATCH !! [2020-05-11 20:26:55.425 18858] SQL: Comparing 127.0.0.1 vs. 127.0.0.1/8 (subnet 255.0.0.0) - !! MATCH !! [2020-05-11 20:26:55.425 18858] SUBNET WARNING: Client 127.0.0.1 is managed by 2 groups (IDs 1,37), all describing /8 subnets. FTL chose the most recent entry 127.0.0.1/8 (ID 37) for this client. ``` This will also show up on the dashboard when the new Pi-hole diagnostics system has been merged: ![Screenshot_2020-05-11 Pi-hole - ubuntu-server(1)](https://user-images.githubusercontent.com/16748619/81597737-78570d00-93c6-11ea-9072-d5a772ec0509.png)

@yubiuser Check out the web interface screeshot in this PR to see what I meant with "something more in the pipeline".

mgg · May 12, 2020, 5:36am

This is awesome !
If you want some help on testing just give me a ping.

Regards Markus

yubiuser · May 12, 2020, 7:49am

That's awesome!

One small hint: I've tried this in conjunction with new/diagnostics and it is already great that the icon changes when new messages arrive. But the triangle icon is always present - subconsciously I always think something might be wrong even when it is not. Maybe you could change the default symbol to something else not triggering a brain warning.

DL6ER · May 12, 2020, 6:58pm

Something seems to be broken for you. I don't see the triangle when there are no messages available. Note that there should also be a small orange label with the number of available warnings.

Please ensure you're on the most recent version.

yubiuser · May 12, 2020, 7:34pm

Switched back from tweak/debug_token and the triangle is gone.

The only thing that remains brocken is this HEAD>>>> MASTER seen on API/Web_Interface.

Great feature!

PromoFaux · May 12, 2020, 7:41pm

I usually only reserve this for desperate times, but sometimes it's easier just to nuke things:

sudo rm -rf /var/www/html/admin
sudo git clone https://github.com/pi-hole/adminLTE /var/www/html/admin

yubiuser · May 12, 2020, 7:48pm

Works until I switch to pihole checkout web new/diagnostics

from sudo cat /var/log/lighttpd/error.log

2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Notice:  Undefined index: PORTFILE in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Warning:  file_get_contents(): Filename cannot be empty in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Notice:  Undefined index: PORTFILE in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Warning:  file_get_contents(): Filename cannot be empty in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Notice:  Undefined index: PORTFILE in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Warning:  file_get_contents(): Filename cannot be empty in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Notice:  Undefined index: PORTFILE in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Warning:  file_get_contents(): Filename cannot be empty in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39
2020-05-12 09:47:31: (mod_fastcgi.c.421) FastCGI-stderr: PHP Notice:  Undefined index: PORTFILE in /var/www/html/admin/scripts/pi-hole/php/FTL.php on line 39

github.com

pi-hole/AdminLTE/blob/2edaaab7358a7848cd96b80780285abbad924612/scripts/pi-hole/php/FTL.php#L39


	return $piholeFTLConfig;
}
function connectFTL($address, $port=4711)
{
	if($address == "127.0.0.1")
	{
		$config = piholeFTLConfig();
		// Read port
		$portfile = file_get_contents($config["PORTFILE"]);
		if(is_numeric($portfile))
			$port = intval($portfile);
	}
	// Open Internet socket connection
	$socket = @fsockopen($address, $port, $errno, $errstr, 1.0);
	return $socket;
}
function sendRequestFTL($requestin)

-rw-rw-r-- 1 pihole root         172 Mai 12 20:02 pihole-FTL.conf

DL6ER · May 12, 2020, 7:54pm

This is not an issue as such, only a warning.

yubiuser · May 12, 2020, 7:56pm

Ok, but still strange that I see this