Yi Chen <yiche@xxxxxxxxxx> wrote:
> This test verifies functionality of the FTP helper,
> for both passive, active FTP modes,
> and the functionality of the nf_nat_ftp module.
Thanks for this test case.
Some minor comments below.
> diff --git a/tests/shell/features/tcpdump.sh b/tests/shell/features/tcpdump.sh
> new file mode 100755
> index 00000000..70df9f68
> --- /dev/null
> +++ b/tests/shell/features/tcpdump.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +# check whether tcpdump is installed
> +tcpdump -h >/dev/null 2>&1
Is tcpdump a requirement? AFAICS the dumps are only used
as a debug aid when something goes wrong?
> +INFILE=$(mktemp -p /var/ftp/pub/)
This directory might not be writeable.
Can you use a /tmp/ directory?
I suggest to do:
WORKDIR=$(mktemp -d)
mkdir "$WORKDIR/pub"
... and then place all files there.
> +dd if=/dev/urandom of="$INFILE" bs=4096 count=1 2>/dev/null
> +chmod 755 $INFILE
> +assert_pass "Prepare the file for FTP transmission"
Including this one
... and this config:
> +cat > ./vsftpd.conf <<-EOF
> +anonymous_enable=YES
> +local_enable=YES
> +connect_from_port_20=YES
> +listen=NO
> +listen_ipv6=YES
> +pam_service_name=vsftpd
> +background=YES
> +EOF
> +ip netns exec $S vsftpd ./vsftpd.conf
> +sleep 1
> +ip netns exec $S ss -6ltnp | grep -q '*:21'
> +assert_pass "start vsftpd server"
So no files are created outside of /tmp.
> +# test passive mode
> +reload_ruleset
> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${0##*/}.pcap 2> /dev/null & sleep 2
> +ip netns exec $C curl -s --connect-timeout 5 ftp://[${ip_rc}]:2121${INFILE#/var/ftp} -o $OUTFILE
> +assert_pass "curl ftp passive mode "
> +
> +pkill tcpdump
Can you do this instead?:
> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${0##*/}.pcap 2> /dev/null &
tcpdump_pid=$!
sleep 2
...
kill "$tcpdump_pid"
?
pkill will zap all tcpdump instances.
Since tests are executed in parallel, it might zap other tcpdump
instances as well and not just the one spawned by this script.
> +tcpdump -nnr ${0##*/}.pcap src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP
Not sure why the above is needed. Isn't the 'cmp' enough to see
if the ftp xfer worked?
> +assert_pass "assert FTP traffic NATed"
> +
> +cmp "$INFILE" "$OUTFILE"
... because if there is a problem with the helper, then the cmp
ought to fail?
