Subnetting doesn't appear to work for groups

troykelly · February 27, 2020, 10:41am

Problem with Beta 5.0:
Defining groups with subnets doesn't appear to work

What happens
A group defined as a subnet - eg 2404:79c0:1002:303::/64
A domain is enabled for the group analytics . google . com
Requesting that domain still generates a 0.0.0.0 response

What is expected
Although the domain is in a block file - it should be allowed for requests within that domain.

Debug Token:
https://tricorder.pi-hole.net/v0cwplvo81

DL6ER · February 29, 2020, 9:43am

Where did you try to set up this subnet? Please note that subnets are not available at the moment anywhere in FTL, each client has to be specified exactly.

troykelly · February 29, 2020, 11:07am

Defined in admin/groups-clients.php

There's no value (for us) in identifying hosts uniquely - perhaps the interface should reject subnets if they aren't supported?

DL6ER · February 29, 2020, 11:15am

Yes, adding validation for the IPs is WIP. Week ETA: next week.
You can submit a feature request for subnets and maybe we can work this out for some future version of Pi-hole. I don't think this could still go in v5.0.

troykelly · February 29, 2020, 11:21am

I can add the feature request - or perhaps a PR for subnet support?
I'm not sure why it would be so difficult to support subnets over single IP addresses - I would have thought it would have been exactly as easy as comparing an IP address (assuming an IP library is being used?)

DL6ER · February 29, 2020, 11:28am

No, we're not using any auxiliary libaries to keep dependencies low. We do batteries included. Currently, the IP addresses are compared like strings to simplify the interaction with the database. If we'd add support for subnets, some things would complicate such as we'd need to detect which entry is the most relevant (assume there are /32, /64, /8, etc.) subnets for the same prefix, we'd need to iterate over them all and chose the most suitable one. Many things to consider and SQL as database language is not made for this, so we'd have to implement more things ourselves.

A code contribution that adds this would obviously the best way of getting this added!

troykelly · February 29, 2020, 11:37am

It might not be worth the PR if there is a concern about iterative testing of IP's?

DL6ER · February 29, 2020, 11:38am

I don't think there should really be a concern here. We detect only once for each client which groups to apply (when we first see it). Even if this is a bit computationally expensive, it is not a task that is running often, so it should not be an issue.

troykelly · February 29, 2020, 11:41am

If you can point me where I need to look so I don't have to ingest everything - I've only ever used the platform - never contributed.
I'm happy to have a look and see.

DL6ER · February 29, 2020, 1:49pm

Thank you very much. There will be nothing to investigate on the web or core code, only FTL's internals would have to be modified for this.

Currently, the client IP checking is done in this routine:

github.com

pi-hole/FTL/blob/71e849816b126f14300cde39d6ce4b9b40e651e1/src/database/gravity-db.c#L107-L227


      
          static bool get_client_groupids(const clientsData* client, char **groups)
          {
          	// Get associated groups for this client (if defined)
          	char *querystr = NULL;
          	const char *ip = getstr(client->ippos);
          	*groups = NULL;
          
          	// Do not proceed when database is not available
          	if(!gravity_database_avail && !gravityDB_open())
          	{
          		logg("get_client_groupids(): Gravity database not available");
          		return false;
          	}
          
          	if(config.debug & DEBUG_DATABASE)
          		logg("Querying gravity database for client %s", ip);
          
          	// Check if client is configured through the client table
          	if(asprintf(&querystr, "SELECT COUNT(*) FROM client WHERE ip = \'%s\';", ip) < 1)
          	{

This file has been truncated. show original

Where we first check if a given client is configured and, if so, we get the groups. This is the routine that would need to get changed to work with subnets as the list of group IDs is everything we need to extract from the database.

troykelly · March 1, 2020, 1:47am

Given SQLite doesn't support cidr notation in SQL searches - my thinking is that if a subnet is entered - an integer representation for start and end of range should be stored.
That way the query can just be if (integer representation of) ip between the two numbers.
Would that align with your thinking?

DanSchaper · March 1, 2020, 2:22am

It should just be a string, how are you finding that sqlite3 can't handle a 12.34.56.78/99 string?

troykelly · March 1, 2020, 2:41am

To keep things simply - I'm proposing that the IP addresses should be stored as a number.
That allows subnetting to work - a subnet is just a range - for example the subnet 10.0.0.0/24 is 0x0a000000 to 0x0a0000ff - so given an IP address of 0x0a000022 I can just simply check with a select query that the IP is between the two start and end points of the subnet.

If IP addresses are stored as strings - you can't do anything with them apart from string comparison - which has us ending up at my initial post highlighting a lack of subnet support.

DanSchaper · March 1, 2020, 2:42am

Would storing the number as the ip in hex/bin with the mask already applied work? Store the network value and not the hosts?

troykelly · March 1, 2020, 3:17am

Because SQLite is missing some of the IP address features that other DB engines have (not a bad thing - it is "lite" after all) - there are a few options for how to handle the storage

Option 1 - Store only what is provided

User adds an IP address
IP address and subnet mask is converted to hex and stored in two fields

This option will require iterative retrieval and testing of all addresses because we can't use BETWEEN SQL matching - we have to calculate the start and end IP each time and test in code.

User supplies 10.0.0.1

API converts a single IP address to IP and mask notation eg 10.0.0.1/32
Database stores:

IP Address	Mask
0x0a000001	32

User supplies 10.0.0.0/24

No conversion needed - IP and mask supplied
Database stores:

IP Address	Mask
0x0a000000	24

User supplies 10.0.0.128/27

No conversion needed - IP and mask supplied
Database stores:

IP Address	Mask
0x0a000080	27

User supplies 10.0.0.55/24

No conversion needed - IP and mask supplied
An IP address that is not the start of a range has been supplied - how should that be handled?
maybe Ignore the technically invalid input and calculate the proper starting IP of the range
Database stores:

IP Address	Mask
0x0a000000	24

User supplies 2404:79c0:1002:1404::/64

No conversion needed - IP and mask supplied
Database stores:

IP Address	Mask
0x240479c0100214040000000000000000	64

User supplies 2404:79c0:1002:1404::

No mask has been supplied - assuming a single IP so append /128
Database stores:

IP Address	Mask
0x240479c0100214040000000000000000	128

Option 2 - Store some derived information to speed up searches

User adds an IP address
If missing - the subnet mask of a single IP is generated and attached to the input
The IP and subnet is used to find the starting and ending IP of the range
All the data is stored

This allows us to use BETWEEN in SQL statements - given the IP address - find all the results that are between the start and end.

Using the same examples as above we would store

IP Address	Mask	Start	End
0x0a000001	32	0x0a000001	0x0a000001
0x0a000000	24	0x0a000000	0x0a0000ff
0x0a000080	27	0x0a000080	0x0a00009f
0x0a000000	24	0x0a000000	0x0a0000ff
0x240479c0100214040000000000000000	64	0x240479c0100214040000000000000000	0x240479c010021404ffffffffffffffff
0x240479c0100214040000000000000000	128	0x240479c0100214040000000000000000	0x240479c0100214040000000000000000

DanSchaper · March 1, 2020, 4:01am

Thanks! This is great information and will really help with trying to find a solution.

troykelly · March 1, 2020, 6:07am

My strength is definitely not writing code .. especially not C#.
But this seems more like a DB opportunity than something that needs to be shipped in the core application anyway.
The application will need to convert IP's to numbers, but that's really the extent of it.
I can look at a PR - but it sounds like it would be a fundamental change to how IP's are stored - which is something I wouldn't want to mess with.
It does seem like a good time to get it done now - before releasing a string comparison IP into "production" while this is still in beta.

DL6ER · March 2, 2020, 5:57pm

It's pure C not pasture fence derivative.

Yes. The benefit of doing it as we do it now is simple interaction with the database. I can run a query like

SELECT * FROM client;

and will immediately obtain human-readable output. With the IP stored in some hex/binary form, things are much less simple and human-friendly. And probably less easy to debug as well.

I do not want to give up this possibility and will rather consider writing a native SQLite plugin for this job, but this will take some time.

troykelly · March 2, 2020, 8:57pm

I get it - that's the benefit of the SQL engine having support for IP addresses, which we don't have here.
I know it's a departure from database normalization - but perhaps store the "human-readable" IP as well?
I will try and work out how I can contribute - but apart from advice - I don't think I can help with code here.

troykelly · March 2, 2020, 9:33pm

Perhaps something like this may help - I will explore it more - I'm not sure what your position on external libs is though.

sqlite-inetfuncs

This is a plugin for SQLite which provides some functions for working with IP addresses. The plugin takes the form of a .dll or .so file depending on your OS.