Page MenuHomeVyOS Platform

Rewrite firewall in new XML/Python style
Needs testing, WishlistPublic

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
Needs testingsdev
ResolvedBUGc-po
ResolvedBUGsdev
ResolvedFEATURE REQUESTsdev
OpenFEATURE REQUESTNone
OpenBUGNone
ResolvedFEATURE REQUESTsdev
OpenBUGNone
ResolvedBUGzsdc
ResolvedFEATURE REQUESTViacheslav
ResolvedBUGSrividyaA
OpenFEATURE REQUESTNone
ResolvedFEATURE REQUESTsdev
OpenBUGNone
Needs testingBUGDmitry
ResolvedViacheslav
OpenBUGNone
ResolvedFEATURE REQUESTsdev
ConfirmedBUGNone
ResolvedBUGsdev
ResolvedBUGsdev
ResolvedBUGbjw-s
ResolvedFEATURE REQUESTsdev

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
erkin set Issue type to Internal change (not visible to end users).Aug 30 2021, 7:48 AM
erkin removed a subscriber: Active contributors.

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

Would appreciate any help testing this before marking the PR ready for merge.

sdev changed the task status from Open to Needs testing.Jan 12 2022, 5:11 PM
sdev 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.