[PATCH 1/1] tests: shell: Updated nat_ftp tests

[PATCH 1/1] tests: shell: Updated nat_ftp tests

[Andrii Melnychenko]

Added DNAT and SNAT only tests.
There was an issue with DNAT that was not covered by tests.
DNAT misses setting up the `seqadj`, which leads to FTP failures.
The fix for DNAT has already been proposed to the kernel.

Acked-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Andrii Melnychenko <a.melnychenko@vyos.io>
---
 tests/shell/testcases/packetpath/nat_ftp | 127 +++++++++++++++++++++--
 1 file changed, 120 insertions(+), 7 deletions(-)

diff --git a/tests/shell/testcases/packetpath/nat_ftp b/tests/shell/testcases/packetpath/nat_ftp
index d0faf2ef..8d9e5d45 100755
--- a/tests/shell/testcases/packetpath/nat_ftp
+++ b/tests/shell/testcases/packetpath/nat_ftp
@@ -77,31 +77,45 @@ ip -net $S route add ${ip_rc}/64 via ${ip_rs} dev s_r
 ip netns exec $C ping -q -6 ${ip_sr} -c1 > /dev/null
 assert_pass "topo initialization"
 
-reload_ruleset()
+reload_ruleset_base()
 {
-	ip netns exec $R conntrack -F 2> /dev/null
-	ip netns exec $R $NFT -f - <<-EOF
+	[[ $# -eq 2 && ( $1 -ne 0 || $2 -ne 0 ) ]]
+	assert_pass "reload ruleset options"
+
+	add_dnat=$1
+	add_snat=$2
+	ruleset=""
+
+	# flush and add FTP helper
+	read -r -d '' str <<-EOF
 	flush ruleset
 	table ip6 ftp_helper_nat_test {
 		ct helper ftp-standard {
 			type "ftp" protocol tcp;
 		}
+	EOF
+	ruleset+=$str$'\n'
 
+	# add DNAT
+	if [[ $add_dnat -ne 0 ]]; then
+		read -r -d '' str <<-EOF
 		chain PRE-dnat {
 			type nat hook prerouting priority dstnat; policy accept;
 			# Dnat the control connection, data connection will be automaticly NATed.
 			ip6 daddr ${ip_rc} counter ip6 nexthdr tcp tcp dport 2121 counter dnat ip6 to [${ip_sr}]:21
 		}
+		EOF
+		ruleset+=$str$'\n'
+	fi
 
+	# add FORWARD
+	read -r -d '' str <<-EOF
 		chain PRE-aftnat {
 			type filter hook prerouting priority 350; policy drop;
 			iifname r_c tcp dport 21 ct state new ct helper set "ftp-standard" counter accept
-
 			ip6 nexthdr tcp ct state related counter accept
 			ip6 nexthdr tcp ct state established counter accept
-
 			ip6 nexthdr icmpv6 counter accept
-
 			counter log
 		}
 
@@ -111,18 +125,51 @@ reload_ruleset()
 			ip6 nexthdr tcp ct state established counter accept
 			ip6 nexthdr tcp ct state related     counter log accept
 		}
+	EOF
+	ruleset+=$str$'\n'
 
+	# add SNAT
+	if [[ $add_snat -ne 0 ]]; then
+		read -r -d '' str <<-EOF
 		chain POST-srcnat {
 			type nat hook postrouting priority srcnat; policy accept;
 			ip6 daddr ${ip_sr} ip6 nexthdr tcp tcp dport 21 counter snat ip6 to [${ip_rs}]:16500
 		}
-	}
+		EOF
+	ruleset+=$str$'\n'
+	fi
+
+	ruleset+=$'}\n'
+	
+	ip netns exec $R conntrack -F 2> /dev/null
+	ip netns exec $R $NFT -f - <<-EOF
+		${ruleset}
 	EOF
+
 	assert_pass "apply ftp helper ruleset"
 }
 
+reload_ruleset()
+{
+	reload_ruleset_base 1 1
+}
+
+reload_ruleset_dnat_only()
+{
+	reload_ruleset_base 1 0
+}
+
+reload_ruleset_snat_only()
+{
+	reload_ruleset_base 0 1
+}
+
 dd if=/dev/urandom of="$INFILE" bs=4096 count=1 2>/dev/null
 chmod 755 $INFILE
+
+mkdir -p /var/run/vsftpd/empty/
+cp $INFILE /var/run/vsftpd/empty/
+
 assert_pass "Prepare the file for FTP transmission"
 
 cat > ${FTPCONF} <<-EOF
@@ -158,6 +205,38 @@ tcpdump -nnr ${PCAP} src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP
 assert_pass "assert FTP traffic NATed"
 
 
+# test passive mode DNAT only
+reload_ruleset_dnat_only
+ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
+pid=$!
+sleep 0.5
+ip netns exec $C curl --no-progress-meter --connect-timeout 5 ftp://[${ip_rc}]:2121/$(basename $INFILE) -o $OUTFILE
+assert_pass "curl ftp passive mode DNAT only"
+
+cmp "$INFILE" "$OUTFILE"
+assert_pass "FTP Passive mode DNAT only: The input and output files remain the same when traffic passes through NAT."
+
+kill $pid; sync
+tcpdump -nnr ${PCAP} src ${ip_cr} and dst ${ip_sr} 2>&1 |grep -q FTP
+assert_pass "assert FTP traffic DNATed"
+
+
+# test passive mode SNAT only
+reload_ruleset_snat_only
+ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
+pid=$!
+sleep 0.5
+ip netns exec $C curl --no-progress-meter --connect-timeout 5 ftp://[${ip_sr}]:21/$(basename $INFILE) -o $OUTFILE
+assert_pass "curl ftp passive mode SNAT only"
+
+cmp "$INFILE" "$OUTFILE"
+assert_pass "FTP Passive mode SNAT only: The input and output files remain the same when traffic passes through NAT."
+
+kill $pid; sync
+tcpdump -nnr ${PCAP} src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP
+assert_pass "assert FTP traffic SNATed"
+
+
 # test active mode
 reload_ruleset
 
@@ -174,5 +253,39 @@ kill $pid; sync
 tcpdump -nnr ${PCAP} src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP
 assert_pass "assert FTP traffic NATed"
 
+
+# test active mode DNAT only
+reload_ruleset_dnat_only
+
+ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
+pid=$!
+sleep 0.5
+ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_rc}]:2121/$(basename $INFILE) -o $OUTFILE
+assert_pass "curl ftp active mode "
+
+cmp "$INFILE" "$OUTFILE"
+assert_pass "FTP Active mode: in and output files remain the same when FTP traffic passes through NAT."
+
+kill $pid; sync
+tcpdump -nnr ${PCAP} src ${ip_cr} and dst ${ip_sr} 2>&1 |grep -q FTP
+assert_pass "assert FTP traffic DNATed"
+
+
+# test active mode SNAT only
+reload_ruleset_snat_only
+
+ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
+pid=$!
+sleep 0.5
+ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_sr}]:21/$(basename $INFILE) -o $OUTFILE
+assert_pass "curl ftp active mode "
+
+cmp "$INFILE" "$OUTFILE"
+assert_pass "FTP Active mode: in and output files remain the same when FTP traffic passes through NAT."
+
+kill $pid; sync
+tcpdump -nnr ${PCAP} src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP
+assert_pass "assert FTP traffic SNATed"
+
 # trap calls cleanup
 exit 0
-- 
2.43.0

Re: [PATCH 1/1] tests: shell: Updated nat_ftp tests

[Florian Westphal]

Andrii Melnychenko <a.melnychenko@vyos.io> wrote:
> Added DNAT and SNAT only tests.
> There was an issue with DNAT that was not covered by tests.
> DNAT misses setting up the `seqadj`, which leads to FTP failures.
> The fix for DNAT has already been proposed to the kernel.

Thanks, could you please send a v2 that splits the refactoring
from the new test case (i.e. two changes)?

> Acked-by: Florian Westphal <fw@strlen.de>

This should not be here unless you would be re-sending a v2
of a patch that is almost 100% the same as the one I acked before.

And I don't recall acking this change, so please don't add this
yourself.

> +	# flush and add FTP helper
> +	read -r -d '' str <<-EOF
>  	flush ruleset
>  	table ip6 ftp_helper_nat_test {
>  		ct helper ftp-standard {
>  			type "ftp" protocol tcp;
>  		}
> +	EOF
> +	ruleset+=$str$'\n'

I'd suggest to just use multiple nft -f invocations
instead of this.

> +	# add DNAT
> +	if [[ $add_dnat -ne 0 ]]; then
> +		read -r -d '' str <<-EOF
>  		chain PRE-dnat {
>  			type nat hook prerouting priority dstnat; policy accept;
>  			# Dnat the control connection, data connection will be automaticly NATed.
>  			ip6 daddr ${ip_rc} counter ip6 nexthdr tcp tcp dport 2121 counter dnat ip6 to [${ip_sr}]:21
>  		}
> +		EOF
> +		ruleset+=$str$'\n'
> +	fi

Just move this from reload_ruleset to a helper function.

> @@ -111,18 +125,51 @@ reload_ruleset()
>  			ip6 nexthdr tcp ct state established counter accept
>  			ip6 nexthdr tcp ct state related     counter log accept
>  		}
> +	EOF
> +	ruleset+=$str$'\n'
>  
> +	# add SNAT
> +	if [[ $add_snat -ne 0 ]]; then
> +		read -r -d '' str <<-EOF

Same here, just omit this.

> +reload_ruleset()
> +{
> +	reload_ruleset_base 1 1
> +}

Then this would be something like:

reload_ruleset_dnat()
{
	reload_ruleset
	load_dnat
}

reload_ruleset_snat()
{
	reload_ruleset
	load_snat
}

reload_ruleset_allnat
{
	reload_ruleset
	load_snat
	load_dnat
}

(or similar naming).  I find that easier to follow, esp. because this
allows a refactor patch before adding the _snat/_dnat tests.

The reload_ruleset -> reload_ruleset_base rename is also ok if you
prefer that.

> +
>  dd if=/dev/urandom of="$INFILE" bs=4096 count=1 2>/dev/null
>  chmod 755 $INFILE
> +
> +mkdir -p /var/run/vsftpd/empty/
> +cp $INFILE /var/run/vsftpd/empty/

I don't understand this change, how is that related?

> +reload_ruleset_dnat_only
> +
> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
> +pid=$!
> +sleep 0.5
> +ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_rc}]:2121/$(basename $INFILE) -o $OUTFILE
> +assert_pass "curl ftp active mode "

Not a requirement, but there is a lot of repitition
of this sequence now, albeit with small changes.

Perhaps this should be in a reuseable function first
before adding the new test cases to this script.

> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
> +pid=$!
> +sleep 0.5
> +ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_sr}]:21/$(basename $INFILE) -o $OUTFILE
> +assert_pass "curl ftp active mode "
> +
> +cmp "$INFILE" "$OUTFILE"
> +assert_pass "FTP Active mode: in and output files remain the same when FTP traffic passes through NAT."
> +
> +kill $pid; sync

I don't understand this 'sync' (I know its already there is source).
It seems its not needed?

Re: [PATCH 1/1] tests: shell: Updated nat_ftp tests

[Andrii Melnychenko]

Hi everyone,


On Mon, Oct 27, 2025 at 8:02 PM Florian Westphal <fw@strlen.de> wrote:
>
> Andrii Melnychenko <a.melnychenko@vyos.io> wrote:
> > Added DNAT and SNAT only tests.
> > There was an issue with DNAT that was not covered by tests.
> > DNAT misses setting up the `seqadj`, which leads to FTP failures.
> > The fix for DNAT has already been proposed to the kernel.
>
> Thanks, could you please send a v2 that splits the refactoring
> from the new test case (i.e. two changes)?

Ok, I can change it to a series of patches:
 * first one that would present a function to set up a ruleset with
DNAT/SNAT variations
 * second - with new test cases.

>
> > Acked-by: Florian Westphal <fw@strlen.de>
>
> This should not be here unless you would be re-sending a v2
> of a patch that is almost 100% the same as the one I acked before.
>
> And I don't recall acking this change, so please don't add this
> yourself.

Well, we have a discussion during the patches for the kernel, so I've
decided to mention you here.
https://lkml.org/lkml/2025/10/24/924
Hesitated between "Asked-by" and "Suggested-by", but I'll remove it in v2.

>
> > +     # flush and add FTP helper
> > +     read -r -d '' str <<-EOF
> >       flush ruleset
> >       table ip6 ftp_helper_nat_test {
> >               ct helper ftp-standard {
> >                       type "ftp" protocol tcp;
> >               }
> > +     EOF
> > +     ruleset+=$str$'\n'
>
> I'd suggest to just use multiple nft -f invocations
> instead of this.
>
> > +     # add DNAT
> > +     if [[ $add_dnat -ne 0 ]]; then
> > +             read -r -d '' str <<-EOF
> >               chain PRE-dnat {
> >                       type nat hook prerouting priority dstnat; policy accept;
> >                       # Dnat the control connection, data connection will be automaticly NATed.
> >                       ip6 daddr ${ip_rc} counter ip6 nexthdr tcp tcp dport 2121 counter dnat ip6 to [${ip_sr}]:21
> >               }
> > +             EOF
> > +             ruleset+=$str$'\n'
> > +     fi
>
> Just move this from reload_ruleset to a helper function.
>
> > @@ -111,18 +125,51 @@ reload_ruleset()
> >                       ip6 nexthdr tcp ct state established counter accept
> >                       ip6 nexthdr tcp ct state related     counter log accept
> >               }
> > +     EOF
> > +     ruleset+=$str$'\n'
> >
> > +     # add SNAT
> > +     if [[ $add_snat -ne 0 ]]; then
> > +             read -r -d '' str <<-EOF
>
> Same here, just omit this.
>
> > +reload_ruleset()
> > +{
> > +     reload_ruleset_base 1 1
> > +}
>
> Then this would be something like:
>
> reload_ruleset_dnat()
> {
>         reload_ruleset
>         load_dnat
> }
>
> reload_ruleset_snat()
> {
>         reload_ruleset
>         load_snat
> }
>
> reload_ruleset_allnat
> {
>         reload_ruleset
>         load_snat
>         load_dnat
> }
>
> (or similar naming).  I find that easier to follow, esp. because this
> allows a refactor patch before adding the _snat/_dnat tests.

Oh yeah, it's a good idea.

>
> The reload_ruleset -> reload_ruleset_base rename is also ok if you
> prefer that.
>
> > +
> >  dd if=/dev/urandom of="$INFILE" bs=4096 count=1 2>/dev/null
> >  chmod 755 $INFILE
> > +
> > +mkdir -p /var/run/vsftpd/empty/
> > +cp $INFILE /var/run/vsftpd/empty/
>
> I don't understand this change, how is that related?
>

Not really related, I'll remove it.

> > +reload_ruleset_dnat_only
> > +
> > +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
> > +pid=$!
> > +sleep 0.5
> > +ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_rc}]:2121/$(basename $INFILE) -o $OUTFILE
> > +assert_pass "curl ftp active mode "
>
> Not a requirement, but there is a lot of repitition
> of this sequence now, albeit with small changes.
>
> Perhaps this should be in a reuseable function first
> before adding the new test cases to this script.
>

It could be a tricky one - for example, an SNAT-only case requires
connecting directly to the FTP server.
A DNAT-only case would require checking the client's IP in the PCAP file.
So the "test function" may look like this:
```
test_case( ip_and_port, client_ip_to_check)
{
...
ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5
ftp://${ip_and_port}/$(basename $INFILE) -o $OUTFILE
...
tcpdump -nnr ${PCAP} src ${client_ip_to_check} and dst ${ip_sr} 2>&1
|grep -q FTP
}
```
If something like this ok, then I can do it:

> > +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
> > +pid=$!
> > +sleep 0.5
> > +ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_sr}]:21/$(basename $INFILE) -o $OUTFILE
> > +assert_pass "curl ftp active mode "
> > +
> > +cmp "$INFILE" "$OUTFILE"
> > +assert_pass "FTP Active mode: in and output files remain the same when FTP traffic passes through NAT."
> > +
> > +kill $pid; sync
>
> I don't understand this 'sync' (I know its already there is source).
> It seems its not needed?

I'm not sure, maybe the idea is to make sure that the data is
"flushed" to the PCAP file?
I can remove it.

-- 

Andrii Melnychenko

Phone +1 844 980 2188

Email a.melnychenko@vyos.io

Website vyos.io

linkedin.com/company/vyos

vyosofficial

x.com/vyos_dev

reddit.com/r/vyos/

youtube.com/@VyOSPlatform

Subscribe to Our Blog Keep up with VyOS

Re: [PATCH 1/1] tests: shell: Updated nat_ftp tests

[Florian Westphal]

Andrii Melnychenko <a.melnychenko@vyos.io> wrote:
> It could be a tricky one - for example, an SNAT-only case requires
> connecting directly to the FTP server.
> A DNAT-only case would require checking the client's IP in the PCAP file.
> So the "test function" may look like this:
> ```
> test_case( ip_and_port, client_ip_to_check)
> {
> ...
> ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5
> ftp://${ip_and_port}/$(basename $INFILE) -o $OUTFILE
> ...
> tcpdump -nnr ${PCAP} src ${client_ip_to_check} and dst ${ip_sr} 2>&1
> |grep -q FTP
> }
> ```
> If something like this ok, then I can do it:

Above looks good to me.

> 
> I'm not sure, maybe the idea is to make sure that the data is
> "flushed" to the PCAP file?
> I can remove it.

Thanks!