Message ID | 20161220212829.19947-5-joe@ovn.org |
---|---|
State | Changes Requested |
Headers | show |
2016-12-20 13:28 GMT-08:00 Joe Stringer <joe@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@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 21 December 2016 at 17:06, Daniele Di Proietto <diproiettod@ovn.org> wrote: > 2016-12-20 13:28 GMT-08:00 Joe Stringer <joe@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@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@vmware.com> Thanks for the feedback, I agree. The first four patches had minimal feedback so I applied it and pushed them to master. I'll send a v2 for this patch to address your feedback. >> 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. If it's defined there, do we need it in the sparse/windows headers? >> 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? I agree this makes more sense and would resolve the atlocal update issue. >> 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. Yes, that's the right way to do this.
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. + - 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 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 \ 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 #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 # 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] 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
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@ovn.org> --- 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(-)