Page MenuHomeVyOS Platform

Empty firewall group (address, network & port) generates invalid nftables config, commit fails
Closed, ResolvedPublicBUG

Description

Empty firewall groups fail with the new nftables code, with even the simplest example:

set firewall group address-group VYOS_NFT_TEST description "Test to show empty address-group behaviour"
commit

will fail:

[ firewall ]
Failed to apply firewall

[[firewall]] failed
Commit failed
[edit]

A look at /run/nftables.conf reveals the following code:

#!/usr/sbin/nft -f


define A_VYOS_NFT_TEST = {
    
}

table ip filter {
}

table ip6 filter {
}

If I load this manually with nft -f /run/nftables.conf, I get the following:

/run/nftables.conf:5:5-5: Error: syntax error, unexpected newline
    
    ^
/run/nftables.conf:6:1-1: Error: syntax error, unexpected '}'
}

Manually changing /run/nftables.conf to the following makes it work:

#!/usr/sbin/nft -f


define A_VYOS_NFT_TEST = { }

table ip filter {
}

table ip6 filter {
}

Details

Difficulty level
Unknown (require assessment)
Version
1.4-rolling-202201090317
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

To my understanding, the template data/templates/firewall/nftables.tmpl is probably the culprit, as it doesn't check whether group_conf.address (and similarly the others) has any elements at all and introduces the offending white-space:

{% if group is defined %}
{%   if group.address_group is defined %}
{%     for group_name, group_conf in group.address_group.items() %}
define A_{{ group_name }} = {
    {{ group_conf.address | join(",") }}
}
{%     endfor %}

I'm not a templating master, but I would assume it should be possible to filter out zero-length arrays with something like this:

*Edit*: obviously, it was too late for me to be able to count correctly. It should be {% if group_conf.address|length > 0 %}

{% if group is defined %}
{%   if group.address_group is defined %}
{%     for group_name, group_conf in group.address_group.items() %}
define A_{{ group_name }} = { {% if group_conf.address|length > 0 %}
    {{ group_conf.address | join(",") }}
{%       endif %} }
{%     endfor %}
johannrichard renamed this task from Rewrite firewall in new XML/Python style: Empty firewall group (address, network & port) generate invalid nftables config, commit fails to Empty firewall group (address, network & port) generates invalid nftables config, commit fails.Jan 10 2022, 2:25 AM

I just realize it's getting more complicated as python/vyos/firewall.py will later write out the rules for these empty groups and when reading-them in, nftables will complain (again) when trying to resolve them, e.g.

define A_PRINT_SERVERS = {  }
...
table ip filter {
    chain GUEST-LAN {
        ct state {established,related} counter return comment "GUEST-LAN-1"
        ct state {invalid} log counter drop comment "GUEST-LAN-2"
        meta l4proto tcp ip daddr $A_PRINT_SERVERS tcp dport $P_PRINTER_PORTS_ALLOWED counter return comment "GUEST-LAN-1010" 
        counter  drop comment "GUEST-LAN default-action drop"
    }
...
}

will result in

/run/nftables.conf:14:26-29: Error: Set is empty
define A_PRINT_SERVERS = {  }
                         ^^^^

The CLI doesn't catch that (and didn't so in the past either: presently, it's possible to configure such rules without any difficulties when running VyOS under earlier releases with the old iptables code).

Arguably, the original issue is a pure “syntax” problem (i.e., the generated file is not valid at all when the format of the empty set is not correct) which can and probably should be fixed?

sarthurdev changed the task status from Open to Needs testing.Jan 11 2022, 2:48 PM
sarthurdev claimed this task.

PR: https://github.com/vyos/vyos-1x/pull/1158

PR removes the empty line when there are no group members, also adds a warning message when empty groups are used in rules.

See comment in T4164: my config runs through easily now.