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

Clean-up how interface address/network rules are generated. #1419

Merged
merged 2 commits into from
Feb 25, 2017
Merged

Clean-up how interface address/network rules are generated. #1419

merged 2 commits into from
Feb 25, 2017

Conversation

phpb-com
Copy link
Contributor

This change cleans-up the rule generation for interface addresses and networks. It also addresses the issue with network addresses for interface groups (https://forum.opnsense.org/index.php?topic=4556.0). I have done some testing but not anywhere close to cover all of the scenarios. In theory, this should work as expected, and will also work with multiple interface alias IPs that I would like to add in the near future. Please consider this change as WiP and suggest any other areas and things that should be considered. Thanks.

@fichtner
Copy link
Member

I don't want to stop you, simply make you aware of how we are trying to migrate/convert code to:

https://github.com/opnsense/core/blob/master/src/etc/inc/filter.lib.inc
https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/library/OPNsense/Firewall/FilterRule.php
https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/library/OPNsense/Firewall/Plugin.php

In order to make the firewall system more "pluggable" and approachable from an architectural standpoint. "filter.inc" in itself is a very static and hardcoded file which will be removed/replaced eventually.

@phpb-com
Copy link
Contributor Author

@fichtner Thanks, looks very promising. Am I correct in my assumptions that there is still a lot to do before new filter generation library will be released? Do you have any design documents that I could refer to? I would like to contribute where possible but would not know what to start with or how to test it. Also, I noticed that you pre-hardcoded some of the basic/core filters, do you plan to keep it this way or there is a plan to move it out into more generic (xml, csv, etc...) file. Thanks.

As for this change, I have done some more extensive testing, and it does work as expected. Since the new library is already in works, I would like to stop right here and see if this an be included in the future release before the new library goes life. Let me know, I would really like to have this included since it fixes some bugs that stop me from using OPNSense in my setup.

@fichtner
Copy link
Member

fichtner commented Feb 22, 2017

@phpb-com perhaps @AdSchellevis can explain this better, but I'll go ahead and answer as best as I can.

We are in the transition. The code has been tested through most of 17.1's development cycle and was made the default with the release of 17.1. The hardcoded rules you see are already the results of a conversion away from filter.inc in an attempts to ease the hardcoding. In its current form, the rules can be generated from the new code based on shared properties, not having to be injected by static code and pf syntax. That's a good first step.

The transition isn't complete in this regard, more rules need to be extracted from filter.inc and the NAT rules need to be converted as well. The goal here is to provide a "pluggable" framework for, well, plugins to inject firewall rules (either auto-generated or user-defined) for single services like e.g. the redirection rule(s) in the web proxy for transparent mode.

The bigger picture of course is less code, less friction, reduce maintenance and bug hunting efforts and to easily adapt to newer changes in the base OS and/or some fine day migrate or fork away. You know there are other pf.conf variants out there. ;)

To that end, filter.lib.inc is where we put converted code from the PHP backend, but try to keep the rules generation inside the MVC library parts. filter.inc should eventually not consist of any more rules generation code besides "a generate rules" call into the subsystems and e.g. rules for IPsec should move to the IPsec (plugin) code.

Another interleaved goal here is to provide the base for a firewall rules API which many have asked about. It's a lot of work, but it's worth it and we may see the first results in 17.7. :)

Cheers,
Franco

@AdSchellevis
Copy link
Member

@fichtner Thanks for explaining, I'm still trying to complete the rule generation for 17.7.

Next step on my list is adding all types which exists in the current ruleset to our new structure and replace the static filter.inc code for calls to registerFilterRule().
Then I'm planning to give all nat rules a similar treatment.

Another thing on my mind is to look at the aliases, I've already cleaned-up the code, but the end game should probably be a service which detaches the maintenance of it's content with the loading process, there's still quite some weirdness in there, which probably can only be solved by adding a new service to handle both filterdns and large (remote) lists.

I will try to check and inspect this pull asap, @phpb-com thanks for contributing.

@phpb-com
Copy link
Contributor Author

@fichtner and @AdSchellevis Thank you both for the information, it gives more insight into the plans. I will try to look around the new code and see if I can contribute anything useful. First I will need to try and understand it further, since I only glanced at it for now.

In any case, looking forward to your feedback @AdSchellevis , and thanks.

@AdSchellevis
Copy link
Member

@phpb-com This looks good to me, removes more unneeded code. I think we can simplify it even a bit more, but let me look into that. Parts may shift in the future, but if there's less to move into the new framework it will eventually get easier to do so.

@AdSchellevis AdSchellevis merged commit ac28bc5 into opnsense:master Feb 25, 2017
AdSchellevis added a commit that referenced this pull request Feb 25, 2017
@AdSchellevis
Copy link
Member

@phpb-com @fichtner this probably needs some more time testing on devel, but a203c69 seems to be working fine on my end.

@phpb-com phpb-com deleted the pfrules_cleanup branch February 25, 2017 21:22
@phpb-com
Copy link
Contributor Author

Thanks, all works on my end as expected, did tests for different interface types and networks. Your clean-up makes things even simpler, thanks for that.

@Ivan-Smagleson
Copy link

Thanks, this patch fix the pf syntax as need.

but it is still miss configured for me:
as ifconfig ovpns2 show for me
inet 10.50.21.1 --> 10.50.21.2 netmask 0xffffffff
pf apply same address in pf rules, but in GUI settings of interface v5021, I specify IPv4: 10.50.21.1/24 (same settings in openvpn).
How I can deal with filter by network for openvpn? should I avoid using 'network' in openvpn related rules?

@AdSchellevis
Copy link
Member

@Ivan-Smagleson please use your own issue for feedback, this pull is closed already. I will respond there.

AdSchellevis added a commit that referenced this pull request Mar 5, 2017
…heck for existence of the interface. edge-case of #1419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants