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.
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.
I already coded a very similar thing before you posted this, however, as an integrated extension to SQLite3 so without the necessity to load an external lib (this is discouraged even by the SQLite3 developers themselves as it can open up security holes quite easily).
Just mentioning here that I already had some progress and, in fact, have an IPv4 version of this already working. The issue I was having yesterday is, however, that SQLIte3 does at maximum support 64 bit integers. While this is obviously sufficient to implement IPv4 subnetting (32 bit), it does result in an issue with IPv6 addresses (128 bit) as we cannot perform standard math such as BETWEEN on this kind of numbers.
I verified that the same issue exists as well when we prepare and store the 128 bit numbers themselves in the database. So what you proposed above is no alternative, either. Now we know this for sure.
This isn't a real show stopper, the comparison for IPv6 just gets a bit trickier. I'm re-implementing my function to do the entire checking internally so it only needs to return a boolean. We'd use my new sqlite3-internal subroutine like:
SELECT COUNT(*) FROM client WHERE check_subnet(ip, '127.0.0.1') = TRUE;
where ip automatically expands to the ip column data (may or may not contain a trailing /24, etc.) and '127.0.0.1' is some textual IP address provided by FTL. I'm coding this all in highly performance C code, so there should be no performance impact.
3 Likes
I pushed what I have so far. I'm running out of time now (already way beyond midnight over here), but you may want to do some initial testing on it. To ease possibly needed debugging, I added some very verbose logging (see /var/log/pihole-FTL.log) in the style of:
[2020-03-03 23:27:18.695 21965] Comparing database 127.0.0.0/8 (extracted CIDR: /8) to 192.168.2.223 - NO MATCH
[2020-03-03 23:27:18.695 21965] Comparing database 127.0.0.102 (extracted CIDR: /32) to 192.168.2.223 - NO MATCH
[2020-03-03 23:27:18.695 21965] Comparing database 127.0.0.103 (extracted CIDR: /32) to 192.168.2.223 - NO MATCH
[2020-03-03 23:27:18.695 21965] Comparing database 192.168.2.100 (extracted CIDR: /32) to 192.168.2.223 - NO MATCH
[2020-03-03 23:27:18.695 21965] Comparing database 192.168.2.2/24 (extracted CIDR: /24) to 192.168.2.223 - !!! MATCH !!!
or from localhost:
[2020-03-03 23:27:18.692 21965] Comparing database 127.0.0.0/8 (extracted CIDR: /8) to 127.0.0.1 - !!! MATCH !!!
[2020-03-03 23:27:18.692 21965] Comparing database 127.0.0.102 (extracted CIDR: /32) to 127.0.0.1 - NO MATCH
[2020-03-03 23:27:18.692 21965] Comparing database 127.0.0.103 (extracted CIDR: /32) to 127.0.0.1 - NO MATCH
[2020-03-03 23:27:18.692 21965] Comparing database 192.168.2.100 (extracted CIDR: /32) to 127.0.0.1 - NO MATCH
[2020-03-03 23:27:18.692 21965] Comparing database 192.168.2.2/24 (extracted CIDR: /24) to 127.0.0.1 - NO MATCH
[2020-03-03 23:27:18.692 21965] Comparing database 192.168.2.210 (extracted CIDR: /32) to 127.0.0.1 - NO MATCH
@troykelly As you can see, it seems to work for the few IPv4 clients I have configured in my network. However, I did not do any real testing. Any comments you may have, especially with IPv6 clients, would be appreciated!
Database client IPs are now allowed to be either in the "usual" format:
192.168.2.123
or in CIDR notation:
192.168.2.0/24
I wrote the code entirely general so every possible mask should be available, not only multiples of eight or something like that. Also, users should not be able to break anything as any invalid input should simply be skipped without consequences.
You can get the modified version of FTL using
pihole checkout ftl new/CIDR_clients
1 Like
Amazing work @DL6ER
[2020-03-04 11:05:08.374 1330] Comparing database 2404:79c0:1002:1103::/64 (extracted CIDR: /64) to 2404:79c0:1002:304:3402:8fd9:a1d7:1c0 - NO MATCH
[2020-03-04 11:05:08.374 1330] Comparing database 2404:79c0:1002:1203::/64 (extracted CIDR: /64) to 2404:79c0:1002:304:3402:8fd9:a1d7:1c0 - NO MATCH
[2020-03-04 11:05:08.374 1330] Comparing database 2404:79c0:1002:304::/64 (extracted CIDR: /64) to 2404:79c0:1002:304:3402:8fd9:a1d7:1c0 - !!! MATCH !!!
I will try and test something in our dual-stack environment - but wanted to get the IPv6 results to you as quick as I could.
1 Like
Okay, since we're not yet sure if everything is indeed working as I intended on the drawing board, I opened a draft PR for now:
Note that, with the most recent version, you'll have to have
DEBUG_DATABASE=true
in your /etc/pihole/pihole-FTL.conf to still see the debugging output.
1 Like
I'm guessing you don't ever sleep.
Thank you for your work!
2 Likes
I often wonder myself. He's an absolute machine of a man.
1 Like
5 Likes
Subnetting support has now been merged into the beta and will be released with Pi-hole v5.0. Meanwhile, we added a client IP validation accepting a possible CIDR.
Please go back on the main beta testing using:
pihole checkout web release/v5.0
pihole checkout ftl release/v5.0
Thanks for using the Pi-hole and helping us make Pi-hole a better software for us all!
With the current release I am unable to add IPv6 addresses. Getting the following error:
Tested 2404:79c0:1002:1100::/64 and 2404:79c0:1002:1100::
Using the release or new/CIDR_clients both generate the error.
[Edit]
Updating even via the dropdown does not work for known addresses...
Neither of those are valid IPv6 addresses.
Hi @DanSchaper - yes they are both valid - one is a subnet, one is the router's IP
It just expands as 2404:79c0:1002:1100:0:0:0:0
Why do you think they are not valid?
Well, let's rephrase this to: "[...] unable to add these IPv6 addresses." 
Not unable in general.
For the validation algorithm I invented was for IPv6 addresses, I forgot about addresses with trailing :: without anything behind them.
We discussed this before in here, but missed this kind of valid address type.
I already modified our validator, would you mind doing some more testing to see if we prevent any further valid addresses?
Apologies @DL6ER - yes - I was being overdramatic. I didn't even think to try an uncompressed address.
Everything below I tested with the regex worked fine.
fe80::a299:9bff:fe18:50d1
2001:db8:1111:a:b0::200
0000:0000:0000:0000:0000:0000:0000:0001
ff02:0000:0000:0000:0000:0000:0000:0001
ff02::1
fe80::a299:9bff:fe18:50d1