pf: Aliases exclusions (supress)

[kulikov-a]

Hi!
pf supports exclusions in tables (eg { 192.168.2.0/24, !192.168.2.5 })
(for now AliasContentField.php and alias.py do not allow to add addresses with exclamation mark)
the idea is to create an Alias (Network(s) type) with subnets to exclude from other Aliases (blacklists) and create a merged table (Network group type alias) consisting of blacklist URLTable(IPs) and Exclude alias (eg use FireHOL level1 list "cutting" some private and "bulletproof " subnets from it).
I tried (add "!" sign processing in address validation in php and py files) and it works.
of course, there are some more subtleties: the order of records in the final table (exclusion shoud go first imo) , references check stops working in the GUI (pfctl -test works just fine)
but imho it would be great to add this feature to Aliases.
Thanks!!

[kulikov-a]

just checked. yes, if exclusions is on botton of merged table they must be subsets of networks in blacklist or they will be ignored when loading in pf.
if exclusions is on top of merged table file they completly overcome networks in blacklist.
so to be shure that exclusions completly suppress networks from main table we need to sort merged table file ascending before load in pf.
(just two more strings in update_tables.py)

[kulikov-a]

upd2: find_table_references.py find references of ip in tables by reading all tables and testing each line "if ip in IPNetwork"
why not to use perfect pfctl command "test"? and its exclusions aware

[AdSchellevis]

because it wouldn't perform due to context switches between your script and the pfctl command. (by the way our feature request template is https://github.com/opnsense/core/issues/new?assignees=&labels=&template=feature_request.md , which we really do prefer)

[kulikov-a]

thanks again for quick responses!
oops. sorry, I believed that configd allows to do this..
about request template - sorry. im new at github. i used this template, read carefully all text and just delete it.
should I remake my request and not delete anyting from template? just paste text in relevant part?

[AdSchellevis]

we'll let it slip this time, just make sure you use the template next time.

It's easier to read if details are more structured (going from "what you want to achieve" to the proposed solution), some times there are other options as well which is easier to validate when the functional question is clear.

[kulikov-a]

got it. thank you!

[kulikov-a]

checked: pfctl -Ttest is working in find_table_references.py
just need to read stderr insted of stdout:

for table in tables:
                sp = subprocess.run(['/sbin/pfctl', '-t', table, '-Ttest', sys.argv[1]], capture_output=True, text=True)
                line = sp.stderr.strip()
                if line.find("1/1") == 0:
                   result['matches'].append(table)
            print(ujson.dumps(result))

works for me

[AdSchellevis]

I misread your previous comment, my mistake, the function to check if a (one) address is matched can indeed be refactored like this, using test in the update tables likely will lead to performance issues.

Just open a PR and we'll work on it from there, looks like a good addition in my opinion.

[kulikov-a]

Great!)
Sorry, what shoud i put in PR? just find_table_references.py with -Ttest or or all my experiments for aliases exclusions?

[AdSchellevis]

just start with a PR for find_table_references, since it's a single function call without a relation to other components. If you would like to work on the other piece as well, better do that in a separate PR.

[kulikov-a]

#4320
I don't think I should PR for the rest of the files. These are just test changes that works for me.
But I'm sure you will do it faster and more cleanly. If you suddenly decide to look at my changes, I can always show them here for a quick review.

[AdSchellevis]

@kulikov-a no worries, thanks for the PR

[AdSchellevis]

@kulikov-a fe25f69 should do the trick, maybe you want to improve our docs on the subject?

https://docs.opnsense.org/manual/aliases.html https://github.com/opnsense/docs/blob/6fa0e8da483f2c1e12067999f4cb1db1d718451c/source/manual/aliases.rst

[kulikov-a]

cool! for fe25f69
in php: i also changed validateHost function on my tests. isn't it necessary?
in py: i also allow "!" in address ranges. for now it will throw exception i think?
I got the idea. I'll try to add it to the doc)
How to do this right? PR? or post it here for quick review and then to PR?

[AdSchellevis]

well, I'm not sure if it adds anything, maybe it's easier to explain that this option is only available for networks (using either a netmask or a single host). just post a PR there, thanks in advance!

[kulikov-a]

well, Im already using host exclusion for my remote branch offices public IPs (for bulletproofing). I suppose that might be useful.
Yes, host may be added in networks type Alias. I just guess that since Hosts and Networks Alias types allready there, it will be more intuitive.
im not using address ranges, but I also can imagine a situation when it might be useful (eg exclude remote branch public ip subnet but not isp routers or something like that).
in .php i just cutting "!" in the beginning of loop in two functions. in .py im cutting "!" in the beginning of parsing and check "isexclaim" variable whein yielding (and paste it back if its true)
gone to see into rst-format ..
thanks!

[AdSchellevis]

The use of the feature is easier to explain from the context of a network entry (all of 192.168.0.0/24, without 192.168.0.1 for example). From a host perspective itself it doesn't really make sense, in which case you're forced to explain the relation with alias nesting.

[kulikov-a]

Yeah) But Idea is indead in using nesting: {big blocklist (eg firehol_level1 which includes bogons), excluded_networks (to exclude some bogons), excluded hosts (to protect some important hosts against accidental blacklisting)} so we can put blocking rule with this nested alias on top.
i was going to add text to Alias types descitpion, add some additional info with link on pf man and add example to Nesting paragraph

[kulikov-a]

Hm. just realized our misunderstanding. yes, idea IS in nesting. not only in ability tо exclude subnet in one same table.
that's why I talked about hosts and ranges with "!"

[kulikov-a]

my PR for docs (for the case with nesting, hosts and subnets Aliases support): opnsense/docs#283

[AdSchellevis]

@kulikov-a I'll add the validation to the hosts as well and close this issue, thanks for working on the docs!

[kulikov-a]

great!
sorry. one more thing. as i mentioned in second comment, when pfctl load table (pfctl -Treplace) order of exclusions is vital (if exclusion goes after included matching subent, eg {192.168.0.0/24, !192.168.0.0/24}) pfctl will drop exclusion record (in this case exclusion subnet must be a subset of included network eg {192.168.0.0/24, !192.168.0.0/25} will work). so merged (nesting) table shoud be ordered asc and exclusion records will be at the top of file. if i understand ir right update_table.py does not guarantee this for now (am I right?). so for my test i just call
aliasfile = ('/var/db/aliastables/%s.txt' % alias_name) sp = subprocess.run(['sort', aliasfile, '-o', aliasfile])
before calling 'pfctl -Treplace'.
but im not shure that this is right method to sort table file before loading.

[AdSchellevis]

@kulikov-a are you sure the order is relevant? It didn't look like it when testing on my end (didn't see it in the docs either), if it does matter, we probably need to change the sorting to descending here (or use a lambda to push all ! to bottom):

core/src/opnsense/scripts/filter/update_tables.py

Line 141 in abcb237

alias_content_txt = '\n'.join(sorted(alias_content))

(alias_content contains all entries for the alias including the ones resolved via nesting, so there's no real difference between a nested alias or a single one from pf's perspective)

Can you check if the current state really doesn't work on your end?

[kulikov-a]

yes, based on my test. i manualy placed "!" strings on top or on bottom of table-file and load tables with pfctl -Treplace via shell. pfctl shows how many strings imported and total strings count. also in both variants after table loading i tested IPs from excluded networks via pfctl -Ttest. Results was different (fully matched strings ignored regardless of "!". pfctl load first and ignores second). This and https://www.freebsd.org/cgi/man.cgi?pfctl (search for "duplicated " word on page) makes me think that pfctl -Treplace reads table-file sting by string an do some inside magic (check conflict, check duplicates) and in this case order is very relevant.
I tested original update_tables.py and "!" is always on top. thats what we need but i could not understand why it happends..
I thought that the "alias_content_txt = '\n'.join(sorted(alias_content))" sort only current table in loop and join it to the existing content (previosly added related aliases). so result will depend on aliases order in ("for alias in aliases"). and i just add final file sorting.
now you point me, i look at file again, see "for related_alias_name in aliases.get_alias_deps(alias_name)" loop and understand my mistake.
Sorry!!

[AdSchellevis]

@kulikov-a no problem, better to check again than to find out our new feature is only partially functional. Thanks again for your help.