Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add antigravity (subscribed allowlists with wildcard support) #1579

Merged
merged 11 commits into from Sep 29, 2023

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jul 6, 2023

What does this implement/fix?

Add support for new antigravity table added in pi-hole/pi-hole#5330

Subscribed allowlists support the exact same syntax as in gravity:

  • exact domains, and
  • @@||...^ ABP-style wildcards

Subscribed allowlists can only permit domains from subscribed blocklists. They cannot overwrite manually blocked domains, e.g., by regex or local blocklist domains.

The existing logic is extended such that gravity is only checked when there was no antigravity match (performance optimization). ABP-style domains are only checked when Pi-hole knows there are any in the lists (performance optimization).

Pick your subscribed allowlists wisely, they can easily undo all the adlist-related blocking on your Pi-hole, e.g., when containing something like @@||com^, no *.com domain will be blocked due to gravity any longer.

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): pi-hole/docs@3e3d72c


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/use-to-whitelist-in-abp-style-adlists/63603/63

@yubiuser
Copy link
Member

yubiuser commented Jul 6, 2023

The existing logic is extended such that antigravity is only checked when there was a gravity match. ABP-style domains are only checked when Pi-hole knows there are any in the lists (performance optimization).

Would it make sense (speed?) to check antigravity before gravity? If it's in the former we don't need to check the latter.


Pull request in docs with documentation (if applicable): N/A

I think this needs some words added to the documentation. Probably a small list of the order things are checked by FTL and what takes precedence over other things.

@DL6ER
Copy link
Member Author

DL6ER commented Jul 8, 2023

You are right about a possible speed benefit, thanks for the suggestion. I already pushed an inversion of antigravity with gravity. I also agree about the documentation improvement and will add this as well. edit done, see PR message above

@DL6ER
Copy link
Member Author

DL6ER commented Jul 28, 2023

Anything missing on this one? Just in case it got forgotten (asking for a friend). If this needs more review time that's fine, too.

@yubiuser
Copy link
Member

Needs more time.

@DL6ER
Copy link
Member Author

DL6ER commented Jul 30, 2023

Rebased on latest development-v6

@DL6ER DL6ER force-pushed the new/antigravity branch 2 times, most recently from ba0824c to 8db2156 Compare July 30, 2023 21:38
@DL6ER
Copy link
Member Author

DL6ER commented Sep 8, 2023

Kindly trying to bump this up in the review todo lists before it gets considered completely abandonned. I still think this is an important (even of not essential) feature for Pi-hole to go forward.

@PromoFaux
Copy link
Member

PromoFaux commented Sep 11, 2023

Subscribed allowlists support the exact same syntax as in gravity:
exact domains, and
|>|...^ ABP-style wildcards

Are we sure this is the case? It looks like the syntax begins @@ (see https://adblockplus.org/filter-cheatsheet#:~:text=or%20its%20subdomains.-,Exception%20rules,-Exception%20rules%20are)

Tried this branch with @hagezi's whitelist (here), which contains entries that start @@| noting here that there is only one pipe... docs state there should be two, however.

Edit: Note to self, read the linked Discourse thread above

https://discourse.pi-hole.net/t/use-to-whitelist-in-abp-style-adlists/63603/60

@hagezi
Copy link

hagezi commented Sep 11, 2023

Tried this branch with @hagezi's whitelist (here), which contains entries that start @@| noting here that there is only one pipe... docs state there should be two, however.

@@|example.com^ allows example.com but not a.example.com
@@||example.com^ allows *.example.com

@hagezi
Copy link

hagezi commented Sep 11, 2023

Allow AdBlock rule Alternative rule
example.com @@|example.com^ @@example.com
example.* @@|example.*^ @@|example.
example-*.* @@|example-*.*^ @@|example-*.
*.example.com @@||example.com^ @@|*.example.com^
*.example.* @@||example.*^ @@|*.example. / @@||example.
*.example-*.com @@||example-*.com^ @@|*.example-*.com^

…y deleting related domains in antigravity

Signed-off-by: DL6ER <dl6er@dl6er.de>
… match was found

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@yubiuser
Copy link
Member

Rebased on latest development-v6

@PromoFaux
Copy link
Member

So that I'm super crystal clear on this... Are we saying with this PR that we will accept subscribed allowlists only if they are in the same format as the actual blocklists, and not with the "correct" @@||domain^ syntax?

I don't think I've seen any example allowlists that use the ||domain^ format in the wild

Signed-off-by: DL6ER <dl6er@dl6er.de>
…ion in parse-list

Signed-off-by: DL6ER <dl6er@dl6er.de>
@PromoFaux
Copy link
Member

Functionally this appears to work as expected.

I added the following as a blocklist:
https://gist.githubusercontent.com/PromoFaux/578cdaa8ce8da7036a265765eed09501/raw/b4fc7edc9cf5c90679d298e55d0608306180e117/gistfile1.txt

nslookup a.189.cn
Server:  pi.hole
Address:  192.168.1.253

Name:    a.189.cn
Addresses:  ::
          0.0.0.0

And then added the following an allowlist (which contains the single domain I added in the above gist)
https://raw.githubusercontent.com/hagezi/dns-blocklists/main/adblock/whitelist-urlshortener.txt

nslookup a.189.cn
Server:  pi.hole
Address:  192.168.1.253

Non-authoritative answer:
Name:    bj58cloud.v.lxcvc.com
Addresses:  240e:940:e009:182::1:1d
          240e:f7:8e00:405::6:105
          240e:940:e009:182::1:1a
          240e:f7:8e00:405::6:106
          240e:940:e009:182::1:1b
          240e:f7:8e00:405::6:109
          183.131.179.70
          183.131.179.71
          183.131.179.74
          183.131.179.81
          183.131.179.78
          183.131.179.75
          183.131.179.72
          183.131.179.79
          183.131.179.76
          183.131.179.73
Aliases:  a.189.cn
          a.189.mixcdn.58cloud.com
          a.189.cn.lxcvc.com

I suppose all that remains is to tweak the functionality of the query list endpoint (and the webpage wording) to also check allowlists

image

Signed-off-by: DL6ER <dl6er@dl6er.de>
@PromoFaux
Copy link
Member

After some more testing, I'm happy for this to land - but I will restate that the pihole -q functionality should probably take these lists into account, too

Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to work as intended

@PromoFaux PromoFaux merged commit ff8f708 into development-v6 Sep 29, 2023
18 checks passed
@PromoFaux PromoFaux deleted the new/antigravity branch September 29, 2023 18:09
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-v6-beta-testing/65413/1

@olljanat
Copy link

Pick your subscribed allowlists wisely, they can easily undo all the adlist-related blocking on your Pi-hole, e.g., when containing something like @@||com^, no *.com domain will be blocked due to gravity any longer.

Is that correct?

For me it looks that TLD domains are not supported on v6, neither on allow or blocklists.
image

v5 added support for those in allowlist with this pi-hole/pi-hole#5240

@yubiuser
Copy link
Member

This is a regression of https://github.com/pi-hole/FTL/pull/1667/files

// Domain must be valid
return valid_domain(line+2, len-3);

in conjunction with

// There must be at least two labels (i.e. one dot)
// e.g., "example.com" but not "localhost"
if(last_dot == -1)
return false;

@DL6ER
Copy link
Member Author

DL6ER commented Nov 14, 2023

Indeed. I wasn't aware of this when doing #1667

edit Should be fixed by #1756

@olljanat could you please test your list with

pihole checkout ftl fix/abp_tld

(binaries should become available in about half an hour from now, see here)

@olljanat
Copy link

olljanat commented Nov 14, 2023

edit Should be fixed by #1756

Yes it fixes LTD issue like commented in there.

Another note however, is there way to ignore the fact that DNS returned CNAME, not A record?
I mean example with @@||msftconnecttest.com^ there is log events like these and allowing all those CNAMEs would mean a lot of work:

image
EDIT: Found it, disabling dns.CNAMEdeepInspect did the trick.

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/subscribe-to-user-maintained-whitelists-and-whitelist-from-a-file/3327/58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants