T6687: add fqdn support to nat rules. #4024

nicolas-fort · 2024-08-30T17:59:54Z

Change Summary

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/T6687

Related PR(s)

Component(s) name

nat

Proposed changes

How to test

vyos@fqdn-nat# run show config comm | grep nat
set nat destination rule 10 destination fqdn 'vyos.dev'
set nat destination rule 10 inbound-interface name 'eth2'
set nat destination rule 10 translation address '198.51.100.55'
set system host-name 'fqdn-nat'
[edit]
vyos@fqdn-nat# sudo nft -s list table ip vyos_nat
table ip vyos_nat {
        set FQDN_nat_destination_10_d {
                type ipv4_addr
                flags interval
                elements = { 104.18.2.133, 104.18.3.133 }
        }

        chain PREROUTING {
                type nat hook prerouting priority dstnat; policy accept;
                counter jump VYOS_PRE_DNAT_HOOK
                iifname "eth2" ip daddr @FQDN_nat_destination_10_d counter dnat to 198.51.100.55 comment "DST-NAT-10"
        }

        chain POSTROUTING {
                type nat hook postrouting priority srcnat; policy accept;
                counter jump VYOS_PRE_SNAT_HOOK
        }

        chain VYOS_PRE_DNAT_HOOK {
                return
        }

        chain VYOS_PRE_SNAT_HOOK {
                return
        }
}
[edit]
vyos@fqdn-nat# sudo ps aux | grep vyos-domain
root        2928  0.6  5.1  64160 51124 ?        Ss   17:56   0:00 /usr/bin/python3 -u /usr/libexec/vyos/vyos-domain-resolver.py
vyos        3134  0.0  0.2   6332  2048 ttyS0    S+   17:57   0:00 grep vyos-domain
[edit]
vyos@fqdn-nat#

Smoketest result

test_firewall.py --> OK
test_nat.py --> 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 · 2024-08-30T18:00:18Z

👍
No issues in PR Title / Commit Title

github-actions · 2024-08-30T18:00:37Z

✅ No issues found in unused-imports check.. Please refer the workflow run

sever-sever · 2024-08-30T18:43:27Z

data/templates/firewall/nftables-nat.j2

@@ -43,4 +56,8 @@ table ip vyos_nat {

 {{ group_tmpl.groups(firewall_group, False, True) }}
 }
+
+{%     if ip_fqdn.items() %}
+# use-domain-resolver=yes


It does nothing
Is it correct?

It is used a flag, in oder to start/stop vyos-domain resolver.

https://github.com/vyos/vyos-1x/pull/4024/files#diff-73b0ee812456b3cb072d98d47135c69929642c2a72b52712fb1c130f14439732R261

https://github.com/vyos/vyos-1x/pull/4024/files#diff-22639aad95616c8d057fd5f8e672c148e7d29e26077361e9175e089971c0f267R579

@nicolas-fort The question is why the line is always generated with a # before it — that means the line is commented out and will not have any effect, unless Viacheslav and I are missing anything about the config file syntax.

Yes, to nftables it does nothing. As you said, it's always generated with #, so it's a comment. But while applying firewall and nat rules, it checks if that comment is present in those files, in order to start/stop vyos-domain-resolver service.

@dmbaturin The line seems to be picked up by a file contents check in the conf script.

Though not sure why the ip_fqdn boolean check is insufficient here @nicolas-fort ?

vyos-domain-resolver now updates domains and/or fqdn entries, if they are configured in firewall and NAT.

While parsing firewall rules, if fqdn or group-domains are used, then this comment is added to /run/nftables.conf as a flag that will be checked by nat scripts (of course, if nat is configured)

While parsing nat rules, if fqdn are used, then this comment is added to /run/nftables_nat.conf as a flag that will be checked by firewall scripts (of course, if firewall is configured).

Since only once service handles this translations, while parsing one part of the config (for example firewall), we need to know if fqdn are configured in the other feature (nat).

I thought that would be better this approach, and check if comments are present in those files in order to decide to stop/start vypos-domain-resolver service, rather than while parsing one feature (for example nat), also grab the full configuration of the other feature (in the example would be firewall), and then decide if the service should be started or stopped.

I have a bad feeling about such action at a distance, to be fair. At the very least it should be documented in the generated file, why a line that has no effect within that file is included in it.

But I think we should look for other solutions before we take this route. In the worst case, can we just enable the resolver daemon by default? It does nothing when there are no domain groups, and doesn't take much memory or CPU when idle, does it?

python/vyos/firewall.py

nicolas-fort · 2024-09-04T13:04:20Z

I'm marking this one as Draft while working on changes.

sever-sever · 2024-09-14T12:21:28Z

Fails:

vyos@r14# compare 
+ nat {
+     source {
+         rule 100 {
+             destination {
+                 fqdn "google.com"
+             }
+             outbound-interface {
+                 name "eth0"
+             }
+             source {
+                 address "10.0.0.0/24"
+             }
+             translation {
+                 address "masquerade"
+             }
+         }
+     }
+ }

[edit]
vyos@r14# commit

Configuration file errors encountered!

[[nat]] failed
Commit failed
[edit]
vyos@r14#

cat /run/nftables_nat.conf

#!/usr/sbin/nft -f


delete table ip vyos_nat
table ip vyos_nat {
    #
    # Destination NAT rules build up here
    #
    chain PREROUTING {
        type nat hook prerouting priority -100; policy accept;
        counter jump VYOS_PRE_DNAT_HOOK
    }
    set FQDN_nat_source_100_d {
        type ipv4_addr
        flags interval
    }

    #
    # Source NAT rules build up here
    #
    chain POSTROUTING {
        type nat hook postrouting priority 100; policy accept;
        counter jump VYOS_PRE_SNAT_HOOK
        oifname  {eth0} ip saddr  10.0.0.0/24  ip daddr  @FQDN_nat_destination_100_d counter masquerade comment "SRC-NAT-100"

    }
    set FQDN_nat_source_100_d {
        type ipv4_addr
        flags interval
    }

    chain VYOS_PRE_DNAT_HOOK {
        return
    }

    chain VYOS_PRE_SNAT_HOOK {
        return
    }




}

nft check:

vyos@r14# sudo nft -c -f /run/nftables_nat.conf 
/run/nftables_nat.conf:24:58-84: Error: No such file or directory; did you mean set ‘FQDN_nat_source_100_d’ in table ip ‘vyos_nat’?
        oifname  {eth0} ip saddr  10.0.0.0/24  ip daddr  @FQDN_nat_destination_100_d counter masquerade comment "SRC-NAT-100"
                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[edit]
vyos@r14#

Version:

vyos@r14# run ping google.com
PING google.com (216.58.208.206) 56(84) bytes of data.
64 bytes from par10s21-in-f206.1e100.net (216.58.208.206): icmp_seq=1 ttl=115 time=24.3 ms
^C
--- google.com ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 24.316/24.316/24.316/0.000 ms
[edit]
vyos@r14# 
[edit]
vyos@r14# run show version 
Version:          VyOS 1.5-rolling-202409130007
Release train:    current
Release flavor:   generic

sever-sever · 2024-09-14T12:33:38Z

interface-definitions/include/nat-rule.xml.i

@@ -18,6 +18,7 @@
        <help>NAT destination parameters</help>
      </properties>
      <children>
+        #include <include/firewall/fqdn.xml.i>


FQDN should be multi, isn't it?

vyos@r14# set nat source rule 100 destination fqdn example.com [edit] vyos@r14# set nat source rule 100 destination fqdn n1.example.com [edit] vyos@r14# set nat source rule 100 destination fqdn n2.example.com [edit] vyos@r14# set nat source rule 100 destination fqdn n3.example.com [edit] vyos@r14# show nat source rule 100 destination +fqdn n3.example.com [edit] vyos@r14#

one FQDN per rule is inconvenient

We should steer users to use a group for multiple per rule.

I agree, this is analogous to the firewall syntax, which supports either a group (including a domain group) or a single FQDN. Domain groups are supported in NAT.

nicolas-fort · 2024-09-16T11:41:32Z

Fails:

vyos@r14# compare 
+ nat {
+     source {
+         rule 100 {
+             destination {
+                 fqdn "google.com"
+             }
+             outbound-interface {
+                 name "eth0"
+             }
+             source {
+                 address "10.0.0.0/24"
+             }
+             translation {
+                 address "masquerade"
+             }
+         }
+     }
+ }

[edit]
vyos@r14# commit

Configuration file errors encountered!

[[nat]] failed
Commit failed
[edit]
vyos@r14#

cat /run/nftables_nat.conf

#!/usr/sbin/nft -f


delete table ip vyos_nat
table ip vyos_nat {
    #
    # Destination NAT rules build up here
    #
    chain PREROUTING {
        type nat hook prerouting priority -100; policy accept;
        counter jump VYOS_PRE_DNAT_HOOK
    }
    set FQDN_nat_source_100_d {
        type ipv4_addr
        flags interval
    }

    #
    # Source NAT rules build up here
    #
    chain POSTROUTING {
        type nat hook postrouting priority 100; policy accept;
        counter jump VYOS_PRE_SNAT_HOOK
        oifname  {eth0} ip saddr  10.0.0.0/24  ip daddr  @FQDN_nat_destination_100_d counter masquerade comment "SRC-NAT-100"

    }
    set FQDN_nat_source_100_d {
        type ipv4_addr
        flags interval
    }

    chain VYOS_PRE_DNAT_HOOK {
        return
    }

    chain VYOS_PRE_SNAT_HOOK {
        return
    }




}

nft check:

vyos@r14# sudo nft -c -f /run/nftables_nat.conf 
/run/nftables_nat.conf:24:58-84: Error: No such file or directory; did you mean set ‘FQDN_nat_source_100_d’ in table ip ‘vyos_nat’?
        oifname  {eth0} ip saddr  10.0.0.0/24  ip daddr  @FQDN_nat_destination_100_d counter masquerade comment "SRC-NAT-100"
                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[edit]
vyos@r14#

Version:

vyos@r14# run ping google.com
PING google.com (216.58.208.206) 56(84) bytes of data.
64 bytes from par10s21-in-f206.1e100.net (216.58.208.206): icmp_seq=1 ttl=115 time=24.3 ms
^C
--- google.com ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 24.316/24.316/24.316/0.000 ms
[edit]
vyos@r14# 
[edit]
vyos@r14# run show version 
Version:          VyOS 1.5-rolling-202409130007
Release train:    current
Release flavor:   generic

Fixed:

vyos@fqdn-nat# compare commands 
set nat source rule 100 destination fqdn 'google.com'
set nat source rule 100 outbound-interface name 'eth0'
set nat source rule 100 source address '10.0.0.0/24'
set nat source rule 100 translation address 'masquerade'

[edit]
vyos@fqdn-nat# commit
[edit]
vyos@fqdn-nat#

And:

vyos@fqdn-nat# sudo nft list table ip vyos_nat
table ip vyos_nat {
        set FQDN_nat_source_100_d {
                type ipv4_addr
                flags interval
                elements = { 142.250.79.142 }
        }

        chain PREROUTING {
                type nat hook prerouting priority dstnat; policy accept;
                counter packets 4 bytes 990 jump VYOS_PRE_DNAT_HOOK
        }

        chain POSTROUTING {
                type nat hook postrouting priority srcnat; policy accept;
                counter packets 5 bytes 337 jump VYOS_PRE_SNAT_HOOK
                oifname "eth0" ip saddr 10.0.0.0/24 ip daddr @FQDN_nat_source_100_d counter packets 0 bytes 0 masquerade comment "SRC-NAT-100"
        }

        chain VYOS_PRE_DNAT_HOOK {
                return
        }

        chain VYOS_PRE_SNAT_HOOK {
                return
        }
}
[edit]
vyos@fqdn-nat#

I've also added an extra entry in smoketests

dmbaturin · 2024-09-17T21:31:11Z

interface-definitions/include/nat-rule.xml.i

@@ -18,6 +18,7 @@
        <help>NAT destination parameters</help>
      </properties>
      <children>
+        #include <include/firewall/fqdn.xml.i>


I agree, this is analogous to the firewall syntax, which supports either a group (including a domain group) or a single FQDN. Domain groups are supported in NAT.

sarthurdev

I'm happy to approve this, sorry it's taken so long.

Only comments are that I'm not a fan of the use of the temporary file in /run for other conf scripts to pick up as a "signal". But agree there isn't really a clean/performant solution for this at the moment (I'm talking to John about how we should best go about it).

Issues already addressed

dmbaturin · 2024-09-30T14:49:33Z

@Mergifyio backport circinus

mergify · 2024-09-30T14:49:39Z

backport circinus

✅ Backports have been created

#4113 T6687: add fqdn support to nat rules. (backport #4024) has been created for branch circinus but encountered conflicts

T6687: add fqdn support to nat rules. (backport #4024)

nicolas-fort requested a review from a team as a code owner August 30, 2024 17:59

nicolas-fort requested review from dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po August 30, 2024 17:59

github-actions bot assigned nicolas-fort Aug 30, 2024

github-actions bot added the current label Aug 30, 2024

sever-sever reviewed Aug 30, 2024

View reviewed changes

c-po previously requested changes Sep 3, 2024

View reviewed changes

python/vyos/firewall.py Outdated Show resolved Hide resolved

nicolas-fort marked this pull request as draft September 4, 2024 13:04

nicolas-fort force-pushed the T6687 branch 3 times, most recently from 5659884 to 20d0593 Compare September 4, 2024 16:21

nicolas-fort force-pushed the T6687 branch from 20d0593 to 457d820 Compare September 12, 2024 18:23

nicolas-fort marked this pull request as ready for review September 12, 2024 18:24

nicolas-fort requested review from c-po and sever-sever September 12, 2024 18:35

sever-sever reviewed Sep 14, 2024

View reviewed changes

T6687: add fqdn support to nat rules.

4c3d037

nicolas-fort force-pushed the T6687 branch from 457d820 to 4c3d037 Compare September 16, 2024 11:36

github-actions bot added the rebase label Sep 16, 2024

nicolas-fort requested a review from sever-sever September 16, 2024 11:41

dmbaturin approved these changes Sep 17, 2024

View reviewed changes

sarthurdev approved these changes Sep 23, 2024

View reviewed changes

dmbaturin merged commit eb55965 into vyos:current Sep 30, 2024
14 of 17 checks passed

mergify bot mentioned this pull request Sep 30, 2024

T6687: add fqdn support to nat rules. (backport #4024) #4113

Merged

12 tasks

github-actions bot added the backport label Sep 30, 2024

c-po added a commit that referenced this pull request Sep 30, 2024

Merge pull request #4113 from vyos/mergify/bp/circinus/pr-4024

e663fc4

T6687: add fqdn support to nat rules. (backport #4024)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T6687: add fqdn support to nat rules. #4024

T6687: add fqdn support to nat rules. #4024

nicolas-fort commented Aug 30, 2024

github-actions bot commented Aug 30, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024 •

edited

Loading

sever-sever Aug 30, 2024

nicolas-fort Aug 30, 2024

dmbaturin Sep 2, 2024

nicolas-fort Sep 2, 2024

sarthurdev Sep 2, 2024

nicolas-fort Sep 2, 2024

dmbaturin Sep 3, 2024

nicolas-fort commented Sep 4, 2024

sever-sever commented Sep 14, 2024

sever-sever Sep 14, 2024

sarthurdev Sep 15, 2024

dmbaturin Sep 17, 2024

nicolas-fort commented Sep 16, 2024

dmbaturin Sep 17, 2024

sarthurdev left a comment

dmbaturin commented Sep 30, 2024

mergify bot commented Sep 30, 2024 •

edited

Loading

T6687: add fqdn support to nat rules. #4024

T6687: add fqdn support to nat rules. #4024

Conversation

nicolas-fort commented Aug 30, 2024

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

github-actions bot commented Aug 30, 2024 • edited Loading

github-actions bot commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-fort commented Sep 4, 2024

sever-sever commented Sep 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-fort commented Sep 16, 2024

Choose a reason for hiding this comment

sarthurdev left a comment

Choose a reason for hiding this comment

dmbaturin commented Sep 30, 2024

mergify bot commented Sep 30, 2024 • edited Loading

✅ Backports have been created

github-actions bot commented Aug 30, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024 •

edited

Loading

mergify bot commented Sep 30, 2024 •

edited

Loading