Page MenuHomeVyOS Platform

Rewrite firewall in new XML/Python style
Closed, ResolvedPublic

Details

Difficulty level
Unknown (require assessment)
Version
-
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Perfectly compatible
Issue type
Internal change (not visible to end users)

Related Objects

StatusSubtypeAssignedTask
Resolvedsarthurdev
ResolvedBUGc-po
ResolvedBUGsarthurdev
ResolvedFEATURE REQUESTsarthurdev
ResolvedFEATURE REQUESTsarthurdev
OpenBUGNone
ResolvedFEATURE REQUESTsarthurdev
Resolved N/ABUGNone
ResolvedBUGzsdc
ResolvedFEATURE REQUESTViacheslav
ResolvedBUGSrividyaA
ResolvedFEATURE REQUESTViacheslav
ResolvedFEATURE REQUESTsarthurdev
WontfixBUGViacheslav
WontfixBUGViacheslav
ResolvedViacheslav
OpenBUGNone
ResolvedFEATURE REQUESTsarthurdev
ConfirmedBUGNone
ResolvedBUGsarthurdev
ResolvedBUGsarthurdev
ResolvedBUGbjw-s
ResolvedFEATURE REQUESTsarthurdev
In progressBUGn.fort

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sarthurdev changed the task status from Open to Needs testing.Jan 12 2022, 5:11 PM
sarthurdev claimed this task.

@sdev: in your original commit for this task, recent rules are somehow semi-discarded (the time/counter condition will not be written out; however, the action will be written out) because of an apparent problem with nftables in this area.

My question here is how to deal with this: would it be possible (and desirable) to flag rules with recent conditions and discard them *entirely* (while issuing a warning) until a fix for this part of the code exists?

Presently, it's not clear to a user that recent won't work and will most likely even break a “typical” setup, for example by blocking all traffic matching the remaining conditions. In my opinion, asking to change all rule sets with recent conditions is impractical for something that ought to work anyway.

Background:

In python/vyos/firewall.py on L160-L186 one finds the following code & comment (abridged):

if 'recent' in rule_conf:
    count = rule_conf['recent']['count']
    time = rule_conf['recent']['time']
    # output.append(f'meter {fw_name}_{rule_id} {{ ip saddr and 255.255.255.255 limit rate over {count}/{time} burst {count} packets }}')
    # Waiting on input from nftables developers due to
    # bug with above line and atomic chain flushing.

# [...]

if 'set' in rule_conf:
    output.append(parse_policy_set(rule_conf['set'], def_suffix))

if 'action' in rule_conf:
    output.append(nft_action(rule_conf['action']))
else:
    output.append('return')

output.append(f'comment "{fw_name}-{rule_id}"')

This currently leads to a situation where a rule set like the following will lead to a complete drop of *any* packet that matches the remaining conditions (e.g. port/state in the example) because the recent counter / meter will be missing in the rule set.

set firewall name OUTSIDE-LOCAL rule 30 action 'drop'
set firewall name OUTSIDE-LOCAL rule 30 destination port '22'
set firewall name OUTSIDE-LOCAL rule 30 protocol 'tcp'
set firewall name OUTSIDE-LOCAL rule 30 recent count '4'
set firewall name OUTSIDE-LOCAL rule 30 recent time '60'
set firewall name OUTSIDE-LOCAL rule 30 state new 'enable'

Obviously, this breaks many existing rule sets with recent on one hand. But on the other, it's not a bug in VyOS (technically speaking) but a limitation of nftables for which you are waiting on input from the nftables project to solve / work-around it.

@johannrichard Hey sorry I didn't see your comment, I suggest we move the discussion to the dedicated task: https://phabricator.vyos.net/T4209

But you're right, we do need to figure this out asap.

sarthurdev moved this task from In Progress to Finished on the VyOS 1.4 Sagitta board.