Thanks for the multiple seq_skew changes David.
Its easy enough to support although I wonder if it is standard and the
added usefulness seems minimal.
comments inline
On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.march...@redhat.com>
wrote:
> The ftp alg deals with packets in two ways for the command connection:
> either they are inspected and can be mangled when nat is enabled
> (CT_FTP_CTL_INTEREST) or they just go through without being modified
> (CT_FTP_CTL_OTHER).
>
> For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq
> number by the connection current offset, then prepare for the next
> packets by setting an accumulated offset in the ct object.
>
> The tests are updated so that ftp+NAT checks send multiple commands in a
> single tcp command connection: wget is not able to do this, so switch to
> lftp.
>
> Signed-off-by: David Marchand <david.march...@redhat.com>
> ---
> Vagrantfile | 9 ++++++---
> Vagrantfile-FreeBSD | 2 +-
> lib/conntrack.c | 52
> ++++++++++++++++++++++++++-----------------------
> tests/system-traffic.at | 11 ++++++++++-
> 4 files changed, 45 insertions(+), 29 deletions(-)
>
> diff --git a/Vagrantfile b/Vagrantfile
> index 0192f66..fbd772a 100644
> --- a/Vagrantfile
> +++ b/Vagrantfile
> @@ -12,7 +12,8 @@ dnf -y install autoconf automake openssl-devel libtool \
> python-twisted python-zope-interface \
> desktop-file-utils groff graphviz rpmdevtools nc curl \
> wget python-six pyftpdlib checkpolicy selinux-policy-devel
> \
> - libcap-ng-devel kernel-devel-`uname -r` ethtool
> python-tftpy
> + libcap-ng-devel kernel-devel-`uname -r` ethtool
> python-tftpy \
> + lftp
> echo "search extra update built-in" >/etc/depmod.d/search_path.conf
> SCRIPT
>
> @@ -28,7 +29,8 @@ aptitude -y install -R \
> wget python-six ethtool \
> libcap-ng-dev libssl-dev python-dev openssl \
> python-pyftpdlib python-flake8 python-tftpy \
> - linux-headers-`uname -r`
> + linux-headers-`uname -r` \
> + lftp
> SCRIPT
>
> $bootstrap_centos = <<SCRIPT
> @@ -37,7 +39,8 @@ yum -y install autoconf automake openssl-devel libtool \
> python-twisted-core python-zope-interface \
> desktop-file-utils groff graphviz rpmdevtools nc curl \
> wget python-six pyftpdlib checkpolicy selinux-policy-devel
> \
> - libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools
> + libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools \
> + lftp
> SCRIPT
>
> $configure_ovs = <<SCRIPT
> diff --git a/Vagrantfile-FreeBSD b/Vagrantfile-FreeBSD
> index 8f00abe..52599ee 100644
> --- a/Vagrantfile-FreeBSD
> +++ b/Vagrantfile-FreeBSD
> @@ -12,7 +12,7 @@ Vagrant.require_version ">=1.7.0"
> $bootstrap_freebsd = <<SCRIPT
> sed -e 's/\#DEFAULT_ALWAYS_YES = false/DEFAULT_ALWAYS_YES = true/g' -e
> 's/\#ASSUME_ALWAYS_YES = false/ASSUME_ALWAYS_YES = true/g'
> /usr/local/etc/pkg.conf > /tmp/pkg.conf
> mv -f /tmp/pkg.conf /usr/local/etc/pkg.conf
> -pkg install automake libtool wget python py27-six gmake
> +pkg install automake libtool wget python py27-six gmake lftp
> SCRIPT
>
> $configure_ovs = <<SCRIPT
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 6f6021a..2e4141a 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -3214,30 +3214,34 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> pkt->md.ct_state |= CS_TRACKED | CS_INVALID;
> return;
> } else if (rc == CT_FTP_CTL_INTEREST) {
> - uint16_t ip_len;
> -
> - if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> - seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
> ftp_data_start,
> -
> addr_offset_from_ftp_data_start,
> - addr_size, mode);
> - if (seq_skew) {
> - ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen);
> - ip_len += seq_skew;
> - nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = htons(ip_len);
> - conn_seq_skew_set(ct, &conn_for_expectation->key, now,
> - seq_skew, ctx->reply);
> - }
> - } else {
> - seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
> ftp_data_start,
> -
> addr_offset_from_ftp_data_start);
> - ip_len = ntohs(l3_hdr->ip_tot_len);
> - if (seq_skew) {
> - ip_len += seq_skew;
> - l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
> - l3_hdr->ip_tot_len,
> htons(ip_len));
> - l3_hdr->ip_tot_len = htons(ip_len);
> - conn_seq_skew_set(ct, &conn_for_expectation->key, now,
> - seq_skew, ctx->reply);
> + seq_skew = conn_for_expectation->seq_skew;
> + if (nat) {
> + uint16_t ip_len;
> + int64_t new_skew;
> +
> + if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
> ftp_data_start,
> +
> addr_offset_from_ftp_data_start,
> + addr_size, mode);
> + if (new_skew) {
> + ip_len =
> ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen);
> + ip_len += new_skew;
> + nh6->ip6_ctlun.ip6_un1.ip6_un1_plen =
> htons(ip_len);
> + conn_seq_skew_set(ct, &conn_for_expectation->key,
> now,
> + new_skew + seq_skew,
> ctx->reply);
> + }
> + } else {
> + new_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
> ftp_data_start,
> +
> addr_offset_from_ftp_data_start);
> + ip_len = ntohs(l3_hdr->ip_tot_len);
> + if (new_skew) {
> + ip_len += new_skew;
> + l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
> + l3_hdr->ip_tot_len,
> htons(ip_len));
> + l3_hdr->ip_tot_len = htons(ip_len);
> + conn_seq_skew_set(ct, &conn_for_expectation->key,
> now,
> + new_skew + seq_skew,
> ctx->reply);
> + }
> }
> }
> } else {
>
1/ I don't think we need another local variable
since conn_for_expectation->seq_skew
keeps the previous seq_skew, so we can just use it.
2/ The "nat" check is semantically correct around the repl_ftp_v4/6_addr()
calls
if (nat) {
seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
ftp_data_start,
addr_offset_from_ftp_data_start);
}
This yields:
@@ -3217,27 +3225,37 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
uint16_t ip_len;
if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
- seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
ftp_data_start,
-
addr_offset_from_ftp_data_start,
- addr_size, mode);
+ if (nat) {
+ seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
+ ftp_data_start,
+ addr_offset_from_ftp_data_start,
+ addr_size, mode);
+ }
+
if (seq_skew) {
ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen);
ip_len += seq_skew;
nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = htons(ip_len);
conn_seq_skew_set(ct, &conn_for_expectation->key, now,
- seq_skew, ctx->reply);
+ seq_skew + conn_for_expectation->seq_skew,
+ ctx->reply);
}
} else {
- seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
ftp_data_start,
-
addr_offset_from_ftp_data_start);
- ip_len = ntohs(l3_hdr->ip_tot_len);
+ if (nat) {
+ seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
+ ftp_data_start,
+ addr_offset_from_ftp_data_start);
+ }
+
if (seq_skew) {
+ ip_len = ntohs(l3_hdr->ip_tot_len);
ip_len += seq_skew;
l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
l3_hdr->ip_tot_len,
htons(ip_len));
l3_hdr->ip_tot_len = htons(ip_len);
conn_seq_skew_set(ct, &conn_for_expectation->key, now,
- seq_skew, ctx->reply);
+ seq_skew + conn_for_expectation->seq_skew,
+ ctx->reply);
}
}
} else {
(END)
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 4c52431..3734497 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -4240,7 +4240,16 @@ m4_define([CHECK_FTP_NAT],
> OVS_START_L7([at_ns1], [ftp])
>
> dnl FTP requests from p0->p1 should work fine.
> - NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -4 --no-passive-ftp -t
> 3 -T 1 --retry-connrefused -v --server-response --no-remove-listing -o
> wget0.log -d])
> + AT_DATA([ftp.cmd], [dnl
> +set net:max-retries 1
> +set net:timeout 1
> +set ftp:passive-mode off
> +cache off
> +connect ftp://anonymous:@10.1.1.2
> +ls
> +ls
> +])
> + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log])
>
A check is needed for running outside of vagrant which is how I ran it:
see atlocal.in and add "find_command lftp"
then check for it in system_traffic.at
>
> dnl Discards CLOSE_WAIT and CLOSING
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)],
> [0], [$4])
> --
> 1.8.3.1
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
