[ovs-dev] [PATCH 5/5] lib: Add support for tftp ct helper.

Daniele Di Proietto diproiettod at ovn.org
Thu Dec 22 01:06:34 UTC 2016


2016-12-20 13:28 GMT-08:00 Joe Stringer <joe at ovn.org>:
> The kernel datapath provides support for TFTP helpers, so add support
> for this ALG to the commandline and OpenFlow encoding/decoding.
>
> Signed-off-by: Joe Stringer <joe at ovn.org>

Do you want to add a test to tests/ofp-actions.at to parse the new alg?

Do you want to add a test to tests/odp.at to parse the new alg? Maybe
it's not important because for odp flows it's just a string.

I think we should add a check in ofpact_check__() to enforce that
alg=tftp is only used on udp flows, like commit
c9f7de8c2b3f("ofp-actions: Check that 'alg=ftp' matches on TCP.")

I have a few more comments inline, otherwise:

Acked-by: Daniele Di Proietto <diproiettod at vmware.com>

> ---
>  Documentation/intro/install/general.rst |  2 +
>  NEWS                                    |  1 +
>  Vagrantfile                             |  4 +-
>  include/sparse/netinet/in.h             |  1 +
>  include/windows/netinet/in.h            |  1 +
>  lib/ofp-actions.c                       | 13 ++++-
>  lib/ofp-parse.c                         |  4 ++
>  ofproto/ofproto-dpif-xlate.c            | 10 +++-
>  tests/atlocal.in                        | 26 +++++++---
>  tests/system-traffic.at                 | 84 +++++++++++++++++++++++++++++++--
>  tests/test-l7.py                        | 28 +++++++++--
>  utilities/ovs-ofctl.8.in                | 15 ++++--
>  12 files changed, 163 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/intro/install/general.rst b/Documentation/intro/install/general.rst
> index 28c458fc8e43..c44a339aab9a 100644
> --- a/Documentation/intro/install/general.rst
> +++ b/Documentation/intro/install/general.rst
> @@ -120,6 +120,8 @@ The datapath tests for userspace and Linux datapaths also rely upon:
>  - pyftpdlib. Version 1.2.0 is known to work. Earlier versions should
>    also work.
>
> +- tftpy. Version 0.6.2 is known to work. Earlier versions should also work.
> +

Should we add curl here?

>  - GNU wget. Version 1.16 is known to work. Earlier versions should also
>    work.
>
> diff --git a/NEWS b/NEWS
> index 3a08dbc70db6..b58eaf46c293 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -22,6 +22,7 @@ Post-v2.6.0
>         "selection_method" and related options in ovs-ofctl(8) for
>         details.
>       * The "sample" action now supports "ingress" and "egress" options.
> +     * The "ct" action now supports the TFTP ALG.
>     - ovs-ofctl:
>       * 'bundle' command now supports packet-out messages.
>       * New syntax for 'ovs-ofctl packet-out' command, which uses the
> diff --git a/Vagrantfile b/Vagrantfile
> index 1fe60ecf4791..f596322ca4aa 100644
> --- a/Vagrantfile
> +++ b/Vagrantfile
> @@ -11,7 +11,7 @@ dnf -y install autoconf automake openssl-devel libtool \
>                 python-twisted-core python-zope-interface \
>                 desktop-file-utils groff graphviz rpmdevtools nc \
>                 wget python-six pyftpdlib checkpolicy selinux-policy-devel \
> -               libcap-ng-devel kernel-devel-`uname -r` ethtool
> +               libcap-ng-devel kernel-devel-`uname -r` ethtool python-tftpy

and here

>  echo "search extra update built-in" >/etc/depmod.d/search_path.conf
>  SCRIPT
>
> @@ -26,7 +26,7 @@ aptitude -y install -R \
>                 xdg-utils groff graphviz netcat \
>                 wget python-six ethtool \
>                 libcap-ng-dev libssl-dev python-dev openssl \
> -               python-pyftpdlib python-flake8 \
> +               python-pyftpdlib python-flake8 python-tftpy \

and here

>                 linux-headers-`uname -r`
>  SCRIPT
>
> diff --git a/include/sparse/netinet/in.h b/include/sparse/netinet/in.h
> index 8a5b887bd51d..6dba45876e16 100644
> --- a/include/sparse/netinet/in.h
> +++ b/include/sparse/netinet/in.h
> @@ -75,6 +75,7 @@ struct sockaddr_in6 {
>  #define IPPROTO_SCTP 132
>
>  #define IPPORT_FTP 21
> +#define IPPORT_TFTP 69
>
>  /* All the IP options documented in Linux ip(7). */
>  #define IP_ADD_MEMBERSHIP 35
> diff --git a/include/windows/netinet/in.h b/include/windows/netinet/in.h
> index e4169994b14f..bae9f8ceecc5 100644
> --- a/include/windows/netinet/in.h
> +++ b/include/windows/netinet/in.h
> @@ -19,5 +19,6 @@
>
>  #define IPPROTO_GRE 47
>  #define IPPORT_FTP 21
> +#define IPPORT_TFTP 69

This works for sparse and for windows but fails for FreeBSD.

IPPORT_FTP is also defined in include/openvswitch/ofp-actions.h.  Can
we define IPPORT_TFTP there as well?  I'm not sure if we want to leave
it also in windows and sparse headers.

>
>  #endif /* netinet/in.h */
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 41d06fa319b7..391089d5f4dd 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -5454,10 +5454,19 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
>  static void
>  format_alg(int port, struct ds *s)
>  {
> -    if (port == IPPORT_FTP) {
> +    switch(port) {
> +    case IPPORT_FTP:
>          ds_put_format(s, "%salg=%sftp,", colors.param, colors.end);
> -    } else if (port) {
> +        break;
> +    case IPPORT_TFTP:
> +        ds_put_format(s, "%salg=%stftp,", colors.param, colors.end);
> +        break;
> +    case 0:
> +        /* Don't print. */
> +        break;
> +    default:
>          ds_put_format(s, "%salg=%s%d,", colors.param, colors.end, port);
> +        break;
>      }
>  }
>
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 553991c77d81..a26e8ff8cd9f 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -181,6 +181,10 @@ str_to_connhelper(const char *str, uint16_t *alg)
>          *alg = IPPORT_FTP;
>          return NULL;
>      }
> +    if (!strcmp(str, "tftp")) {
> +        *alg = IPPORT_TFTP;
> +        return NULL;
> +    }
>      return xasprintf("invalid conntrack helper \"%s\"", str);
>  }
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index eec1dae7aede..d73ddbccb790 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4512,10 +4512,16 @@ static void
>  put_ct_helper(struct ofpbuf *odp_actions, struct ofpact_conntrack *ofc)
>  {
>      if (ofc->alg) {
> -        if (ofc->alg == IPPORT_FTP) {
> +        switch(ofc->alg) {
> +        case IPPORT_FTP:
>              nl_msg_put_string(odp_actions, OVS_CT_ATTR_HELPER, "ftp");
> -        } else {
> +            break;
> +        case IPPORT_TFTP:
> +            nl_msg_put_string(odp_actions, OVS_CT_ATTR_HELPER, "tftp");
> +            break;
> +        default:
>              VLOG_WARN("Cannot serialize ct_helper %d\n", ofc->alg);
> +            break;
>          }
>      }
>  }
> diff --git a/tests/atlocal.in b/tests/atlocal.in
> index 1353b46fd1ef..d03d5f3767b7 100644
> --- a/tests/atlocal.in
> +++ b/tests/atlocal.in
> @@ -117,12 +117,24 @@ if test "$IS_WIN32" = "yes"; then
>      HAVE_PYTHON3="no"
>  fi
>
> -if test "$HAVE_PYTHON" = "yes" \
> -   && test "x`$PYTHON $abs_top_srcdir/tests/test-l7.py --help | grep 'ftp'`" != x; then
> -    HAVE_PYFTPDLIB="yes"
> -else
> -    HAVE_PYFTPDLIB="no"
> -fi
> +FindL7Lib()
> +{
> +    set +x
> +    var=HAVE_`echo "$1" | tr '[a-z]' '[A-Z]'`
> +    if test "$HAVE_PYTHON" = "yes"; then
> +        result=$($PYTHON $abs_top_srcdir/tests/test-l7.py --help | grep "$1")
> +        if test "x${result}" != x; then
> +            eval ${var}="yes"
> +        else
> +            eval ${var}="no"
> +        fi
> +    else
> +        eval ${var}="no"
> +    fi
> +}
> +
> +FindL7Lib ftp
> +FindL7Lib tftp

I think I'd prefer find_l7_lib over camel case.

Maybe this could be addressed in a separate patch, but I don't see the
need to do these checks at build time.  Can we do them at runtime?

The two comments above also apply to FindCommand.

>
>  # Look for a commnand in the system. If it is found, defines
>  # HAVE_COMMAND="yes", otherwise HAVE_COMMAND="no".
> @@ -148,6 +160,8 @@ else
>      NC_EOF_OPT="-q 1"
>  fi
>
> +CURL_OPT="-g -v --max-time 1 --retry 2 --retry-delay 1 --connect-timeout 1"
> +
>  # Turn off proxies.
>  unset http_proxy
>  unset https_proxy
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 8e424c56031c..272cc168cb35 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1979,7 +1979,7 @@ OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
>  AT_SETUP([conntrack - FTP])
> -AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
> +AT_SKIP_IF([test $HAVE_FTP = no])
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_ALG()
>  OVS_TRAFFIC_VSWITCHD_START()
> @@ -2064,7 +2064,7 @@ OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
>  AT_SETUP([conntrack - FTP over IPv6])
> -AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
> +AT_SKIP_IF([test $HAVE_FTP = no])
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_ALG()
>  OVS_TRAFFIC_VSWITCHD_START()
> @@ -2119,7 +2119,7 @@ OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
>  AT_SETUP([conntrack - FTP with multiple expectations])
> -AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
> +AT_SKIP_IF([test $HAVE_FTP = no])
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_ALG()
>  OVS_TRAFFIC_VSWITCHD_START()
> @@ -2184,6 +2184,80 @@ tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([conntrack - TFTP])
> +AT_SKIP_IF([test $HAVE_TFTP = no])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_ALG()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
> +AT_DATA([flows1.txt], [dnl
> +table=0,priority=1,action=drop
> +table=0,priority=10,arp,action=normal
> +table=0,priority=10,icmp,action=normal
> +table=0,priority=100,in_port=1,udp,action=ct(alg=tftp,commit),2
> +table=0,priority=100,in_port=2,udp,action=ct(table=1)
> +table=1,in_port=2,udp,ct_state=+trk+est,action=1
> +table=1,in_port=2,udp,ct_state=+trk+rel,action=1
> +])
> +
> +dnl Similar policy but without allowing all traffic from ns0->ns1.
> +AT_DATA([flows2.txt], [dnl
> +table=0,priority=1,action=drop
> +table=0,priority=10,arp,action=normal
> +table=0,priority=10,icmp,action=normal
> +
> +dnl Allow outgoing UDP connections, and treat them as TFTP
> +table=0,priority=100,in_port=1,udp,action=ct(table=1)
> +table=1,in_port=1,udp,ct_state=+trk+new-rel,action=ct(commit,alg=tftp),2
> +table=1,in_port=1,udp,ct_state=+trk+new+rel,action=ct(commit),2
> +table=1,in_port=1,udp,ct_state=+trk+est,action=2
> +
> +dnl Allow incoming TFTP data connections and responses to existing connections
> +table=0,priority=100,in_port=2,udp,action=ct(table=1)
> +table=1,in_port=2,udp,ct_state=+trk+est,action=1
> +table=1,in_port=2,udp,ct_state=+trk+new+rel,action=1
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows1.txt])
> +
> +OVS_START_L7([at_ns0], [tftp])
> +OVS_START_L7([at_ns1], [tftp])
> +
> +dnl TFTP requests from p1->p0 should fail due to network failure.
> +NS_CHECK_EXEC([at_ns1], [[curl $CURL_OPT tftp://10.1.1.1/flows1.txt -o foo 2>curl0.log]], [28])
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.1)], [0], [dnl
> +])
> +
> +dnl TFTP requests from p0->p1 should work fine.
> +NS_CHECK_EXEC([at_ns0], [[curl $CURL_OPT tftp://10.1.1.2/flows1.txt -o foo 2>curl1.log]])
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),helper=tftp
> +])
> +
> +dnl Try the second set of flows.
> +AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows2.txt])
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
> +dnl TFTP requests from p1->p0 should fail due to network failure.
> +NS_CHECK_EXEC([at_ns1], [[curl $CURL_OPT tftp://10.1.1.1/flows1.txt -o foo 2>curl2.log]], [28])
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.1)], [0], [dnl
> +])
> +
> +dnl TFTP requests from p0->p1 should work fine.
> +NS_CHECK_EXEC([at_ns0], [[curl $CURL_OPT tftp://10.1.1.2/flows1.txt -o foo 2>curl3.log]])
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),helper=tftp
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_BANNER([conntrack - NAT])
>
>  AT_SETUP([conntrack - simple SNAT])
> @@ -2516,7 +2590,7 @@ dnl Checks the implementation of conntrack with FTP ALGs in combination with
>  dnl NAT, using the provided flow table.
>  m4_define([CHECK_FTP_NAT],
>     [AT_SETUP([conntrack - FTP NAT $1])
> -    AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
> +    AT_SKIP_IF([test $HAVE_FTP = no])
>      CHECK_CONNTRACK()
>      CHECK_CONNTRACK_NAT()
>
> @@ -2728,7 +2802,7 @@ AT_CLEANUP
>
>
>  AT_SETUP([conntrack - IPv6 FTP with NAT])
> -AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
> +AT_SKIP_IF([test $HAVE_FTP = no])
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_NAT()
>  OVS_TRAFFIC_VSWITCHD_START()
> diff --git a/tests/test-l7.py b/tests/test-l7.py
> index aed34f4114d0..24255a2efdcb 100755
> --- a/tests/test-l7.py
> +++ b/tests/test-l7.py
> @@ -48,17 +48,35 @@ def get_ftpd():
>      return server
>
>
> +def get_tftpd():
> +    try:
> +        from tftpy import TftpServer, TftpShared
> +
> +        class OVSTFTPServer(TftpServer):
> +            def __init__(self, listen, handler=None):
> +                (ip, port) = listen
> +                self.ip = ip
> +                self.port = port
> +                TftpServer.__init__(self, tftproot='./')
> +
> +            def serve_forever(self):
> +                self.listen(self.ip, self.port)
> +        server = [OVSTFTPServer, None, TftpShared.DEF_TFTP_PORT]
> +    except ImportError:
> +        server = None
> +        pass
> +    return server
> +
> +
>  def main():
>      SERVERS = {
>          'http': [TCPServer, SimpleHTTPRequestHandler, 80],
>          'http6': [TCPServerV6, SimpleHTTPRequestHandler, 80],
> +        'ftp': get_ftpd(),
> +        'tftp': get_tftpd(),
>      }
>
> -    ftpd = get_ftpd()
> -    if ftpd is not None:
> -        SERVERS['ftp'] = ftpd
> -
> -    protocols = [srv for srv in SERVERS]
> +    protocols = [srv for srv in SERVERS if SERVERS[srv] is not None]

Minor: there's a check below that checks if args.proto is not in
SERVERS and fails.  Should we check now if args.proto is not in
protocols?  Otherwise, when tftpy is not installed we will fail with
an exception instead of exiting.

>      parser = argparse.ArgumentParser(
>              description='Run basic application servers.')
>      parser.add_argument('proto', default='http', nargs='?',
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index af1eb2b7baf2..08fa8a43ed64 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -1840,12 +1840,19 @@ The \fBcommit\fR parameter must be specified to use \fBexec(...)\fR.
>  .
>  .IP \fBalg=\fIalg\fR
>  Specify application layer gateway \fIalg\fR to track specific connection
> -types. Supported types include:
> +types. If subsequent related connections are sent through the \fBct\fR
> +action, then the \fBrel\fR flag in the \fBct_state\fR field will be set.
> +Supported types include:
>  .RS
>  .IP \fBftp\fR
> -Look for negotiation of FTP data connections. If a subsequent FTP data
> -connection arrives which is related, the \fBct\fR action will set the
> -\fBrel\fR flag in the \fBct_state\fR field for packets sent through \fBct\fR.
> +Look for negotiation of FTP data connections. Specify this option for FTP
> +control connections to detect related data connections and populate the
> +\fBrel\fR flag for the data connections.
> +.
> +.IP \fBtftp\fR
> +Look for negotiation of TFTP data connections. Specify this option for FTP
> +control connections to detect related data connections and populate the
> +\fBrel\fR flag for the data connections.
>  .RE
>  .
>  .IP
> --
> 2.10.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list