Page MenuHomeVyOS Platform
Create Task
Maniphest T4178

policy based routing tcp flags issue
Closed, ResolvedPublicBUG
Actions

Description

policy based routing having some issues with current nftables implementation in rolling release.

for example basic tcp-mss policy:

stannert@vyos# show policy route
+route tcpmssclamp {
+    rule 1 {
+        protocol tcp
+        set {
+            tcp-mss 1452
+        }
+        tcp {
+            flags SYN
+        }
+    }
+}

on committing:

stannert@vyos# commit
[ policy route tcpmssclamp ]
Failed to apply policy based routing

[[policy route tcpmssclamp]] failed
Commit failed
[edit]

nftables_policy.conf

stannert@vyos# cat /run/nftables_policy.conf
#!/usr/sbin/nft -f


include "/run/nftables_defines.conf"

table ip mangle {
    chain VYOS_PBR_tcpmssclamp {
        meta l4proto tcp tcp flags & (SYN) == SYN counter tcp option maxseg size set 1452 return comment "tcpmssclamp-1"
        counter return
    }
}

table ip6 mangle {
}[edit]
stannert@vyos# nft -c -f /run/nftables_policy.conf
/run/nftables_policy.conf:8:39-41: Error: Could not parse TCP flag
        meta l4proto tcp tcp flags & (SYN) == SYN counter tcp option maxseg size set 1452 return comment "tcpmssclamp-1"
                                      ^^^
[edit]

if I edit the nftables_policy.conf and write the "SYN" in lowercase, then it's working.
from vyos cli i can set "tcp flags syn" but if I commit with lowercase "syn" i get an commit error aswell:

Traceback (most recent call last):
  File "/usr/libexec/vyos/conf_mode/policy-route.py", line 226, in <module>
    verify(c)
  File "/usr/libexec/vyos/conf_mode/policy-route.py", line 141, in verify
    verify_rule(policy, rule_conf, ipv6)
  File "/usr/libexec/vyos/conf_mode/policy-route.py", line 100, in verify_rule
    raise ConfigError(f'{name} rule {rule_id}: TCP SYN flag must be set to modify TCP-MSS')
NameError: name 'name' is not defined

So I guess nftables expects lowercase tcp flags (syn,fin,ack,rst) instead of uppercase.
But validator wants uppercase, after changing "policy-route.py" in line 99:

Before:
if not tcp_flags or 'SYN' not in tcp_flags.split(","):

After:
if not tcp_flags or 'syn' not in tcp_flags.split(","):

And changing configuration via VyOS CLI to:

stannert@vyos# compare
[edit policy]
+route tcpmssclamp {
+    rule 1 {
+        protocol tcp
+        set {
+            tcp-mss 1452
+        }
+        tcp {
+            flags syn
+        }
+    }
+}
[edit]

the commit is working.
So possible to change the "policy-route.py" file but the auto completion needs to be changed then aswell and I don't know if there are other implementations with uppercase values.

stannert@vyos# set policy route tcpmssclamp rule 1 tcp flags
Possible completions:
   <text>       TCP flags to match


  Allowed values for TCP flags : SYN ACK FIN RST URG PSH ALL
  When specifying more than one flag, flags should be comma-separated.
 For example : value of 'SYN,!ACK,!FIN,!RST' will only match packets with
  the SYN flag set, and the ACK, FIN and RST flags unset
[edit]
stannert@vyos# run show version

Version:          VyOS 1.4-rolling-202201120317
Release train:    sagitta

cheers

Details

Version
VyOS 1.4-rolling-202201120317
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Related Objects

Event Timeline

mTx87 created this task.Jan 13 2022, 11:49 AM
mTx87 added a project: VyOS 1.4 Sagitta.
sarthurdev changed the task status from Open to In progress.Jan 13 2022, 11:55 AM
sarthurdev claimed this task.
sarthurdev subscribed.

Thanks for the report, working on the fix now.

pasik subscribed.Jan 13 2022, 1:58 PM
sarthurdev changed the task status from In progress to Needs testing.Jan 13 2022, 8:29 PM

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

Restricted Repository Identity mentioned this in rVYOSONEX9aa8e51de06b: Merge pull request #1167 from sarthurdev/firewall.Jan 14 2022, 7:31 PM
sarthurdev mentioned this in rVYOSONEXdf5a862beb84: firewall: T4178: Use lowercase for TCP flags and add an validator.Jan 14 2022, 7:31 PM
n.fort subscribed.Jan 17 2022, 11:23 AM

Think 2 flag options should be added.
According to nft wiki these are all the flags that nft could match: tcp flags { fin, syn, rst, psh, ack, urg, ecn, cwr}

Currently, last 2 flags are not available in VyOS.

Wiki reference: https://wiki.nftables.org/wiki-nftables/index.php/Quick_reference-nftables_in_10_minutes#Tcp

sarthurdev added a comment.Jan 17 2022, 11:29 AM

Included those flags in PR: https://github.com/vyos/vyos-1x/pull/1174

Restricted Repository Identity mentioned this in rVYOSONEX9fb2e1432209: Merge pull request #1174 from sarthurdev/firewall.Jan 17 2022, 5:08 PM
sarthurdev mentioned this in rVYOSONEX64668771d5f1: firewall: policy: T4178: Migrate and refactor tcp flags.Jan 17 2022, 5:08 PM
n.fort added a comment.Jan 18 2022, 3:01 PM

TCP Flags seems to be working on firewall filter config.

vyos@vyos:~$ show config comm | grep fire
set firewall name FOO rule 10 action 'accept'
set firewall name FOO rule 10 protocol 'tcp'
set firewall name FOO rule 10 tcp flags ack
set firewall name FOO rule 10 tcp flags not ecn
set firewall name FOO rule 10 tcp flags syn

vyos@vyos:~$ sudo nft list chain ip filter FOO
table ip filter {
	chain FOO {
		tcp flags & (syn | ack | ecn) == syn | ack counter packets 0 bytes 0 return comment "FOO-10"
		return
	}
}

But scenario of this task is not working

vyos@vyos# compare
[edit policy]
+route TEST {
+    rule 1 {
+        protocol tcp
+        set {
+            tcp-mss 1452
+        }
+        tcp {
+            flags {
+                syn
+            }
+        }
+    }
+}
[edit]
vyos@vyos# commit
[ policy route TEST ]
Failed to apply policy based routing

[[policy route TEST]] failed
Commit failed

Content of /run/nftables_policy.conf (omitting ip6 mangle)

vyos@vyos# cat /run/nftables_policy.conf 
#!/usr/sbin/nft -f


include "/run/nftables_defines.conf"

table ip mangle {
    chain VYOS_PBR_PREROUTING {
        type filter hook prerouting priority -150; policy accept;
    }
    chain VYOS_PBR_POSTROUTING {
        type filter hook postrouting priority -150; policy accept;
    }
    chain VYOS_PBR_TEST {
        meta l4proto  tcp tcp flags & () == syn counter tcp option maxseg size set 1452 return comment "TEST-1"
        counter return
    }
}
sarthurdev added a comment.Jan 27 2022, 12:25 PM

Above fixed in PR: https://github.com/vyos/vyos-1x/pull/1193

Restricted Repository Identity mentioned this in rVYOSONEXe2924920618b: Merge pull request #1193 from sarthurdev/T4178.Jan 27 2022, 2:11 PM
sarthurdev mentioned this in rVYOSONEXdcabea5919e2: firewall: T4178: Fix tcp flags output when `not` isn't used.Jan 27 2022, 2:11 PM
Restricted Repository Identity mentioned this in rVYOSONEXd679e9517657: Merge pull request #1197 from sarthurdev/T4178_1.Jan 29 2022, 6:31 PM
sarthurdev mentioned this in rVYOSONEX1c828cc5a1dc: firewall: T4178: Fix dict_keys issue with tcp flags.Jan 29 2022, 6:31 PM
sarthurdev changed the task status from Needs testing to In progress.Feb 2 2022, 11:07 PM

Adding this issue to this task: https://forum.vyos.io/t/firewall-configuration-issue-after-upgrade/8414

sarthurdev changed the task status from In progress to Needs testing.Feb 2 2022, 11:36 PM

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

Restricted Repository Identity mentioned this in rVYOSONEX26774b890443: Merge pull request #1201 from sarthurdev/T4178_2.Feb 3 2022, 7:27 AM
sarthurdev mentioned this in rVYOSONEX9f7f1ebb15a2: firewall: T4178: Fix only inverse matching on tcp flags.Feb 3 2022, 7:27 AM
sarthurdev closed this task as Resolved.Feb 6 2022, 12:47 PM