-
Notifications
You must be signed in to change notification settings - Fork 388
firewall: T7739: Default ruleset for firewall zones #4672
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
base: current
Are you sure you want to change the base?
Conversation
All contributors have signed the CLA ✍️ ✅ |
👍 |
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements default firewall rulesets for firewall zones, allowing administrators to define fallback rules that apply when specific zone-to-zone rules are not configured. This simplifies management of large networks with many zones by reducing the need to explicitly configure rules between every zone pair.
Key changes:
- Adds
default-firewall
configuration syntax for zones with IPv4 and IPv6 ruleset options - Implements logic to apply default rulesets when explicit zone-to-zone rules are absent
- Updates nftables templates to generate appropriate rules for default firewall behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/conf_mode/firewall.py | Adds configuration parsing and validation for default firewall rulesets |
interface-definitions/firewall.xml.in | Defines XML schema for new default-firewall configuration node |
data/templates/firewall/nftables-zone.j2 | Updates nftables template to generate rules for default firewall behavior |
smoketest/scripts/cli/test_firewall.py | Adds comprehensive test case validating default firewall functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
A few changes needed, also please address the indentation issue raised by copilot.
['jump NAME_smoketest-default'], | ||
['chain VZONE_smoketest-eth1'], | ||
['jump NAME_smoketest-default'], | ||
['chain VZONE_smoketest-eth2'], | ||
['chain VZONE_smoketest-local_IN'], | ||
['iifname "eth0"', 'jump NAME_smoketest'], | ||
['jump NAME_smoketest-default'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These duplicate ['jump NAME_smoketest-default']
searchs are redundant.
If you want to check for these jumps in specific chains you can use verify_nftables_chain
https://github.com/vyos/vyos-1x/blob/current/smoketest/scripts/cli/base_vyostest_shim.py#L181
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've struggled to understand exactly how the smoke tests actually function. I more or less hoped there was some ordering expectation in the matching. I'll investigate verify_nftables_chain as just looking to see that something like the string I want does not actually make sure the feature works as intended.
5a05887
to
51850f5
Compare
In large networks with many zones where simple allow/deny rules are not sufficient, zones become tedious to manage. Many use cases can be simplified by providing an ability to define a default ruleset for traffic from other zones. This change proposes adding the follwing syntax: set firewall zone <name> default_firewall name <name> set firewall zone <name> default_firewall ipv6_name <name> The proposed behavior is the following: local in: The default firewall ruleset for the local zone will be appended after all from configurations. local out: If a non-local zone does not have a from local ruleset but does have a default_firewall ruleset, the default_firewall ruleset will be appended using oifname forward: The default firewall ruleset for the zone will be appended after all from configurations To keep the behavior consistent with from ruleset configurations, a return is appended after the default_firewall ruleset. The proposed behavior differs slightly from the default_policy configuration for the local out chains. The default_policy applied in the out templates comes from the local zone, not the actual outbound zone. The proposed change does not amend this, but does make default_firewall logically consistent with the intent of the out rules.
51850f5
to
112264c
Compare
CI integration ❌ failed! Details
|
In large networks with many zones where simple allow/deny rules are not sufficient, zones become tedious to manage. Many use cases can be simplified by providing an ability to define a default ruleset for traffic from other zones. This change proposes adding the follwing syntax:
set firewall zone default_firewall name
set firewall zone default_firewall ipv6_name
The proposed behavior is the following:
To keep the behavior consistent with from ruleset configurations, a return is appended after the default_firewall ruleset.
The proposed behavior differs slightly from the default_policy configuration for the local out chains. The default_policy applied in the out templates comes from the local zone, not the actual outbound zone. The proposed change does not amend this, but does make default_firewall logically consistent with the intent of the out rules.
Change summary
Types of changes
Related Task(s)
https://vyos.dev/T7739
Related PR(s)
How to test / Smoketest result
Added additional firewall smoketest case to validate. All smoke tests are passing.
Checklist: