firewall: T7781: Firewall chains are created when unneeded

[l0crian1]

Change summary

This fixes incorrect behavior where the input/output/forward/prerouting/etc... chains were created regardless if a user configured them or not. The checks to prevent this were already present in the nftables.j2 Jinja2 template, but it was being skipped due to the <defaultValue> field for default-action making the checks always pass.

This behavior would cause filtering to occur on traffic when there was no intention to filter, effectively slowing down processing.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Code style update (formatting, renaming)
• Refactoring (no functional changes)
• Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
• Other (please describe):

Related Task(s)

https://vyos.dev/T7781

Related PR(s)

How to test / Smoketest result

Configure a single chain:

vyos@vyos# set firewall ipv4 output filter rule 1 action accept 
vyos@vyos# commit

Check the chains that are created:

Before fix:

vyos@vyos# sudo nft list table vyos_filter
table ip vyos_filter {
        chain VYOS_FORWARD_filter {
                type filter hook forward priority filter; policy accept;
                counter packets 0 bytes 0 accept comment "FWD-filter default-action accept"
        }

        chain VYOS_INPUT_filter {
                type filter hook input priority filter; policy accept;
                counter packets 0 bytes 0 accept comment "INP-filter default-action accept"
        }

        chain VYOS_OUTPUT_filter {
                type filter hook output priority filter; policy accept;
                counter packets 0 bytes 0 accept comment "ipv4-OUT-filter-1"
                counter packets 0 bytes 0 accept comment "OUT-filter default-action accept"
        }

        chain VYOS_OUTPUT_raw {
                type filter hook output priority raw; policy accept;
                counter packets 0 bytes 0 accept comment "OUT-raw default-action accept"
        }

        chain VYOS_PREROUTING_raw {
                type filter hook prerouting priority raw; policy accept;
                counter packets 0 bytes 0 accept comment "PRE-raw default-action accept"
        }

        chain VYOS_FRAG_MARK {
                type filter hook prerouting priority -450; policy accept;
                ip frag-off & 0x3fff != 0x0 meta mark set 0x000ffff1 return
        }
}

After fix:

vyos@vyos# sudo nft list table vyos_filter
table ip vyos_filter {
        chain VYOS_OUTPUT_filter {
                type filter hook output priority filter; policy accept;
                counter packets 370 bytes 38689 accept comment "ipv4-OUT-filter-1"
                counter packets 0 bytes 0 accept comment "OUT-filter default-action accept"
        }

        chain VYOS_FRAG_MARK {
                type filter hook prerouting priority -450; policy accept;
                ip frag-off & 0x3fff != 0x0 meta mark set 0x000ffff1 return
        }
}

Smoketest results:

test_bridge_firewall (__main__.TestFirewall.test_bridge_firewall) ... ok
test_cyclic_jump_validation (__main__.TestFirewall.test_cyclic_jump_validation) ... ok
test_disable_conntrack_per_chain (__main__.TestFirewall.test_disable_conntrack_per_chain) ... ok
test_flow_offload (__main__.TestFirewall.test_flow_offload) ... ok
test_geoip (__main__.TestFirewall.test_geoip) ... ok
test_gre_match (__main__.TestFirewall.test_gre_match) ... ok
test_groups (__main__.TestFirewall.test_groups) ... ok
test_ipsec_metadata_match (__main__.TestFirewall.test_ipsec_metadata_match) ... ok
test_ipv4_advanced (__main__.TestFirewall.test_ipv4_advanced) ... ok
test_ipv4_basic_rules (__main__.TestFirewall.test_ipv4_basic_rules) ... ok
test_ipv4_dynamic_groups (__main__.TestFirewall.test_ipv4_dynamic_groups) ... ok
test_ipv4_global_state (__main__.TestFirewall.test_ipv4_global_state) ... ok
test_ipv4_mask (__main__.TestFirewall.test_ipv4_mask) ... ok
test_ipv4_remote_group (__main__.TestFirewall.test_ipv4_remote_group) ... ok
test_ipv4_state_and_status_rules (__main__.TestFirewall.test_ipv4_state_and_status_rules) ... ok
test_ipv4_synproxy (__main__.TestFirewall.test_ipv4_synproxy) ... ok
test_ipv6_advanced (__main__.TestFirewall.test_ipv6_advanced) ... ok
test_ipv6_basic_rules (__main__.TestFirewall.test_ipv6_basic_rules) ... ok
test_ipv6_dynamic_groups (__main__.TestFirewall.test_ipv6_dynamic_groups) ... ok
test_ipv6_mask (__main__.TestFirewall.test_ipv6_mask) ... ok
test_ipv6_remote_group (__main__.TestFirewall.test_ipv6_remote_group) ... ok
test_nested_groups (__main__.TestFirewall.test_nested_groups) ... ok
test_remote_group (__main__.TestFirewall.test_remote_group) ... ok
test_source_validation (__main__.TestFirewall.test_source_validation) ... ok
test_sysfs (__main__.TestFirewall.test_sysfs) ... ok
test_timeout_sysctl (__main__.TestFirewall.test_timeout_sysctl) ... ok
test_zone_basic (__main__.TestFirewall.test_zone_basic) ... ok
test_zone_flow_offload (__main__.TestFirewall.test_zone_flow_offload) ... ok
test_zone_with_vrf (__main__.TestFirewall.test_zone_with_vrf) ... ok

----------------------------------------------------------------------
Ran 29 tests in 562.998s

OK

Checklist:

• I have read the CONTRIBUTING document
• I have linked this PR to one or more Phabricator Task(s)
• I have run the components SMOKETESTS if applicable
• My commit headlines contain a valid Task id
• My change requires a change to the documentation
• I have updated the documentation accordingly

[github-actions]

❌
PR title does not match the required format

[github-actions]

CI integration ❌ failed!

Details

CI logs

• CLI Smoketests (no interfaces) ❌ failed
• CLI Smoketests VPP 👍 passed
• CLI Smoketests (interfaces only) 👍 passed
• Config tests 👍 passed
• Config tests VPP 👍 passed
• RAID1 tests 👍 passed
• TPM tests 👍 passed

[c-po]

The defaultValue not only serves internally as the default for the code, but also for our documentation. defaultValues will be automatically attached to the <help> message during CLI completion.

The is only one "better/ugly/you name it" solution to the problem you identified:

Mangling the dict and removing defaults if required.

vyos-1x/python/vyos/frrender.py

Lines 78 to 83 in 5845c4b

	def dict_helper_ospf_defaults(ospf, path):
	# We have gathered the dict representation of the CLI, but there are default
	# options which we need to update into the dictionary retrived.
	default_values = conf.get_config_defaults(path, key_mangling=('-', '_'),
	get_first_key=True, recursive=True)

[l0crian1]

Generating unnecessary config, just to turn around and delete it so it doesn't cause unintended behavior seems a little backwards to me. It's treating a symptom rather than solving the problem.

Is the only additional concern the help string? I can just manually update it to:

Default-action for rule-set (default: accept)

The defaultValue has a lot of value within the code, (like for the firewall global settings), but to enable a default value of accept in python, all that is required was this change (along with deleting the defaultValue):

    default_action = fw_conf.get('default_action', 'accept')

All of the other changes (smoketests, verify(), Jinja2, etc...) would be required regardless of the solution. Does keeping the default value in the XML really buy anything at that point?

[c-po]

Is the only additional concern the help string? I can just manually update it to:

Currently this is true - but it will (unfortunately until we find a generic solution to this issue) decouple the CLI help string from the internal implementation. I am a fan of a generic solution but yet have no proper idea how to handle this in the firewall context. Thus I tend to agree we go with your implementation.

Please add a <!-- XML COMMENT --> to the XML help string indicating that the default you define here in the help string is implemented in a different file, so we have a future reference by comment in case anyone starts wondering.