tcp: md5: Fix overlap between vrf and non-vrf keys
Subsystems (4)
v2 cover tcp: md5: Fix overlap between vrf and non-vrf keys
With net.ipv4.tcp_l3mdev_accept=1 it is possible for a listen socket to accept connection from the same client address in different VRFs. It is also possible to set different MD5 keys for these clients which differ only in the tcpm_l3index field.
This appears to work when distinguishing between different VRFs but not between non-VRF and VRF connections. In particular:
* tcp_md5_do_lookup_exact will match a non-vrf key against a vrf key.
This means that adding a key with l3index != 0 after a key with l3index == 0 will cause the earlier key to be deleted. Both keys can be present if the non-vrf key is added later.
* _tcp_md5_do_lookup can match a non-vrf key before a vrf key. This
casues failures if the passwords differ.
This can be fixed by making tcp_md5_do_lookup_exact perform an actual exact comparison on l3index and by making __tcp_md5_do_lookup perfer vrf-bound keys above other considerations like prefixlen.
The fact that keys with l3index==0 affect VRF connections is usually not desirable, VRFs are meant to be completely independent. This behavior needs to preserved for backwards compatiblity. Also, applications can just bind listen sockets to VRF and never specify TCP_MD5SIG_IFINDEX at all.
So far the combination of TCP_MD5SIG_IFINDEX with tcpm_ifindex == 0 was an error, accept this to mean "key only applies to default VRF". This is what applications using VRFs for traffic separation want.
This also contains tests for the second part. It does not contain tests for overlapping keys, that would require more changes in nettest to add multiple keys. These scenarios are also covered by my tests for TCP-AO, especially around this area:
https://github.com/cdleonard/tcp-authopt-test/blob/main/tcp_authopt_test/test_vrf_bind.py
Changes since V1:
* Accept (TCP_MD5SIG_IFINDEX with tcpm_ifindex == 0)
* Add flags for explicitly including or excluding TCP_MD5SIG_IFINDEX
to nettest
* Add few more tests in fcnal-test.sh.
Link to v1: https://lore.kernel.org/netdev/3d8387d499f053dba5cd9184c0f7b8445c4470c6.1633542093.git.cdleonard@gmail.com/
Leonard Crestez (4):
tcp: md5: Fix overlap between vrf and non-vrf keys
tcp: md5: Allow MD5SIG_FLAG_IFINDEX with ifindex=0
selftests: nettest: Add --{do,no}-bind-key-ifindex
selftests: net/fcnal: Test --{do,no}-bind-key-ifindex
include/net/tcp.h | 5 +- net/ipv4/tcp_ipv4.c | 45 ++++++++++++----- net/ipv6/tcp_ipv6.c | 15 +++--- tools/testing/selftests/net/fcnal-test.sh | 60 +++++++++++++++++++++++ tools/testing/selftests/net/nettest.c | 28 ++++++++++- 5 files changed, 130 insertions(+), 23 deletions(-)
base-commit: d1f24712a86abd04d82cf4b00fb4ab8ff2d23c8a
Signature · 1 line
2.25.1
PATCH v2 1/4 tcp: md5: Fix overlap between vrf and non-vrf keys
With net.ipv4.tcp_l3mdev_accept=1 it is possible for a listen socket to accept connection from the same client address in different VRFs. It is also possible to set different MD5 keys for these clients which differ only in the tcpm_l3index field.
This appears to work when distinguishing between different VRFs but not between non-VRF and VRF connections. In particular:
* tcp_md5_do_lookup_exact will match a non-vrf key against a vrf key.
This means that adding a key with l3index != 0 after a key with l3index == 0 will cause the earlier key to be deleted. Both keys can be present if the non-vrf key is added later.
* _tcp_md5_do_lookup can match a non-vrf key before a vrf key. This
casues failures if the passwords differ.
Fix this by making tcp_md5_do_lookup_exact perform an actual exact comparison on l3index and by making __tcp_md5_do_lookup perfer vrf-bound keys above other considerations like prefixlen.
--- net/ipv4/tcp_ipv4.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
net/ipv4/tcp_ipv4.c +16 -3
@@ -1035,10 +1035,24 @@ static void tcp_v4_reqsk_destructor(struct request_sock *req)*/ DEFINE_STATIC_KEY_FALSE (tcp_md5_needed);EXPORT_SYMBOL (tcp_md5_needed); +static bool better_md5_match (struct tcp_md5sig_key *old,struct tcp_md5sig_key *new) +{ +if (!old) +return true; + +/* l3index always overrides non-l3index */ +if (old->l3index && new->l3index ==0 ) +return false; +if (old->l3index ==0 && new->l3index) +return true; + +return old->prefixlen < new->prefixlen; +} +/* Find the Key structure for an address. */ struct tcp_md5sig_key *__tcp_md5_do_lookup (const struct sock *sk,int l3index,const union tcp_md5_addr *addr,int family) {
@@ -1072,12 +1086,11 @@ struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,#endif }else { match = false; } -if (match && (!best_match || - key->prefixlen > best_match->prefixlen)) +if (match &&better_md5_match (best_match, key)) best_match = key; }return best_match; }EXPORT_SYMBOL (__tcp_md5_do_lookup);
@@ -1103,11 +1116,11 @@ static struct tcp_md5sig_key *tcp_md5_do_lookup_exact(const struct sock *sk,#endif hlist_for_each_entry_rcu (key, &md5sig->head, node,lockdep_sock_is_held (sk)) {if (key->family != family)continue ; -if (key->l3index && key->l3index != l3index) +if (key->l3index != l3index)continue ;if (!memcmp (&key->addr, addr, size) && key->prefixlen == prefixlen)return key; }
Signature · 1 line
2.25.1
Quoted 19 lines · Leonard Crestez
With net.ipv4.tcp_l3mdev_accept=1 it is possible for a listen socket to accept connection from the same client address in different VRFs. It is also possible to set different MD5 keys for these clients which differ only in the
tcpm_l3indexfield.This appears to work when distinguishing between different VRFs but not between non-VRF and VRF connections. In particular:
*
tcp_md5_do_lookup_exactwill match a non-vrf key against a vrf key.
This means that adding a key with l3index != 0 after a key with l3index == 0 will cause the earlier key to be deleted. Both keys can be present if the non-vrf key is added later.
*_tcp_md5_do_lookupcan match a non-vrf key before a vrf key. This
casues failures if the passwords differ.Fix this by making
tcp_md5_do_lookup_exactperform an actual exact comparison on l3index and by making__tcp_md5_do_lookupperfer vrf-bound keys above other considerations like prefixlen.
Quoted 6 lines
Fixes dea53bb80e07 ("tcp: Add l3index to tcp_md5sig_key and md5 functions")Signed-off-by Leonard Crestez <[email protected]>--- net/ipv4/tcp_ipv4.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
PATCH v2 2/4 tcp: md5: Allow MD5SIG_FLAG_IFINDEX with ifindex=0
In reply to Leonard CrestezMultiple VRFs are generally meant to be "separate" but right now md5 keys for the default VRF also affect connections inside VRFs if the IP addresses happen to overlap.
So far the combination of TCP_MD5SIG_IFINDEX with tcpm_ifindex == 0 was an error, accept this to mean "key only applies to default VRF". This is what applications using VRFs for traffic separation want.
--- include/net/tcp.h | 5 +++-- net/ipv4/tcp_ipv4.c | 26 ++++++++++++++++---------- net/ipv6/tcp_ipv6.c | 15 +++++++++------ 3 files changed, 28 insertions(+), 18 deletions(-)
include/net/tcp.h +3 -2
@@ -1588,10 +1588,11 @@ union tcp_md5_addr {struct tcp_md5sig_key {struct hlist_node node;u8 keylen;u8 family;/* AF_INET or AF_INET6 */ u8 prefixlen; +u8 flags;union tcp_md5_addr addr;int l3index;/* set if key added with L3 scope */ u8 key[TCP_MD5SIG_MAXKEYLEN ];struct rcu_head rcu; };
@@ -1633,14 +1634,14 @@ struct tcp_md5sig_pool {/* - functions */ int tcp_v4_md5_hash_skb (char *md5_hash,const struct tcp_md5sig_key *key,const struct sock *sk,const struct sk_buff *skb);int tcp_md5_do_add (struct sock *sk,const union tcp_md5_addr *addr, -int family,u8 prefixlen,int l3index, +int family,u8 prefixlen,int l3index,u8 flags,const u8 *newkey,u8 newkeylen,gfp_t gfp);int tcp_md5_do_del (struct sock *sk,const union tcp_md5_addr *addr, -int family,u8 prefixlen,int l3index); +int family,u8 prefixlen,int l3index,u8 flags);struct tcp_md5sig_key *tcp_v4_md5_lookup (const struct sock *sk,const struct sock *addr_sk);#ifdef CONFIG_TCP_MD5SIG #include <linux/jump_label.h>
net/ipv4/tcp_ipv4.c +16 -10
@@ -1071,11 +1071,11 @@ struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,hlist_for_each_entry_rcu (key, &md5sig->head, node,lockdep_sock_is_held (sk)) {if (key->family != family)continue ; -if (key->l3index && key->l3index != l3index) +if (key->flags &TCP_MD5SIG_FLAG_IFINDEX && key->l3index != l3index)continue ;if (family ==AF_INET ) { mask =inet_make_mask (key->prefixlen); match = (key->addr.a4.s_addr & mask) == (addr->a4.s_addr & mask);
@@ -1096,11 +1096,11 @@ struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,EXPORT_SYMBOL (__tcp_md5_do_lookup);static struct tcp_md5sig_key *tcp_md5_do_lookup_exact (const struct sock *sk,const union tcp_md5_addr *addr,int family,u8 prefixlen, -int l3index) +int l3index,u8 flags) {const struct tcp_sock *tp =tcp_sk (sk);struct tcp_md5sig_key *key;unsigned int size =sizeof (struct in_addr );const struct tcp_md5sig_info *md5sig;
@@ -1116,10 +1116,12 @@ static struct tcp_md5sig_key *tcp_md5_do_lookup_exact(const struct sock *sk,#endif hlist_for_each_entry_rcu (key, &md5sig->head, node,lockdep_sock_is_held (sk)) {if (key->family != family)continue ; +if ((key->flags &TCP_MD5SIG_FLAG_IFINDEX ) != (flags &TCP_MD5SIG_FLAG_IFINDEX )) +continue ;if (key->l3index != l3index)continue ;if (!memcmp (&key->addr, addr, size) && key->prefixlen == prefixlen)return key;
@@ -1140,19 +1142,19 @@ struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk, }EXPORT_SYMBOL (tcp_v4_md5_lookup);/* This can be called on a newly created socket, from other files */ int tcp_md5_do_add (struct sock *sk,const union tcp_md5_addr *addr, -int family,u8 prefixlen,int l3index, +int family,u8 prefixlen,int l3index,u8 flags,const u8 *newkey,u8 newkeylen,gfp_t gfp) {/* Add Key to the list */ struct tcp_md5sig_key *key;struct tcp_sock *tp =tcp_sk (sk);struct tcp_md5sig_info *md5sig; - key =tcp_md5_do_lookup_exact (sk, addr, family, prefixlen, l3index); + key =tcp_md5_do_lookup_exact (sk, addr, family, prefixlen, l3index, flags);if (key) {/* Pre-existing entry - just update that one. * Note that the key might be used concurrently. * data_race() is telling kcsan that we do not care of * key mismatches, since changing MD5 key on live flows
@@ -1193,24 +1195,25 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,memcpy (key->key, newkey, newkeylen); key->keylen = newkeylen; key->family = family; key->prefixlen = prefixlen; key->l3index = l3index; + key->flags = flags;memcpy (&key->addr, addr, (family ==AF_INET6 ) ?sizeof (struct in6_addr ) :sizeof (struct in_addr ));hlist_add_head_rcu (&key->node, &md5sig->head);return 0 ; }EXPORT_SYMBOL (tcp_md5_do_add);int tcp_md5_do_del (struct sock *sk,const union tcp_md5_addr *addr,int family, -u8 prefixlen,int l3index) +u8 prefixlen,int l3index,u8 flags) {struct tcp_md5sig_key *key; - key =tcp_md5_do_lookup_exact (sk, addr, family, prefixlen, l3index); + key =tcp_md5_do_lookup_exact (sk, addr, family, prefixlen, l3index, flags);if (!key)return -ENOENT ;hlist_del_rcu (&key->node);atomic_sub (sizeof (*key), &sk->sk_omem_alloc);kfree_rcu (key, rcu);
@@ -1240,28 +1243,31 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname,struct tcp_md5sig cmd;struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.tcpm_addr;const union tcp_md5_addr *addr;u8 prefixlen =32 ;int l3index =0 ; +u8 flags;if (optlen <sizeof (cmd))return -EINVAL ;if (copy_from_sockptr (&cmd, optval,sizeof (cmd)))return -EFAULT ;if (sin->sin_family !=AF_INET )return -EINVAL ; + flags = cmd.tcpm_flags &TCP_MD5SIG_FLAG_IFINDEX ; +if (optname ==TCP_MD5SIG_EXT && cmd.tcpm_flags &TCP_MD5SIG_FLAG_PREFIX ) { prefixlen = cmd.tcpm_prefixlen;if (prefixlen >32 )return -EINVAL ; } -if (optname ==TCP_MD5SIG_EXT && +if (optname ==TCP_MD5SIG_EXT && cmd.tcpm_ifindex && cmd.tcpm_flags &TCP_MD5SIG_FLAG_IFINDEX ) {struct net_device *dev;rcu_read_lock (); dev =dev_get_by_index_rcu (sock_net (sk), cmd.tcpm_ifindex);
@@ -1278,16 +1284,16 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname, } addr = (union tcp_md5_addr *)&sin->sin_addr.s_addr;if (!cmd.tcpm_keylen) -return tcp_md5_do_del (sk, addr,AF_INET , prefixlen, l3index); +return tcp_md5_do_del (sk, addr,AF_INET , prefixlen, l3index, flags);if (cmd.tcpm_keylen >TCP_MD5SIG_MAXKEYLEN )return -EINVAL ; -return tcp_md5_do_add (sk, addr,AF_INET , prefixlen, l3index, +return tcp_md5_do_add (sk, addr,AF_INET , prefixlen, l3index, flags, cmd.tcpm_key, cmd.tcpm_keylen,GFP_KERNEL ); } static int tcp_v4_md5_hash_headers(struct tcp_md5sig_pool *hp, __be32 daddr, __be32 saddr,
@@ -1607,11 +1613,11 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,* We're using one, so create a matching key * on the newsk structure. If we fail to get * memory, then we end up not copying the key * across. Shucks. */ -tcp_md5_do_add (newsk, addr,AF_INET ,32 , l3index, +tcp_md5_do_add (newsk, addr,AF_INET ,32 , l3index, key->flags, key->key, key->keylen,GFP_ATOMIC ); sk_nocaps_add(newsk, NETIF_F_GSO_MASK); }#endif
net/ipv6/tcp_ipv6.c +9 -6
@@ -597,31 +597,34 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname, {struct tcp_md5sig cmd;struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&cmd.tcpm_addr;int l3index =0 ;u8 prefixlen; +u8 flags;if (optlen <sizeof (cmd))return -EINVAL ;if (copy_from_sockptr (&cmd, optval,sizeof (cmd)))return -EFAULT ;if (sin6->sin6_family !=AF_INET6 )return -EINVAL ; + flags = cmd.tcpm_flags &TCP_MD5SIG_FLAG_IFINDEX ; +if (optname ==TCP_MD5SIG_EXT && cmd.tcpm_flags &TCP_MD5SIG_FLAG_PREFIX ) { prefixlen = cmd.tcpm_prefixlen;if (prefixlen >128 || (ipv6_addr_v4mapped (&sin6->sin6_addr) && prefixlen >32 ))return -EINVAL ; }else { prefixlen =ipv6_addr_v4mapped (&sin6->sin6_addr) ?32 :128 ; } -if (optname ==TCP_MD5SIG_EXT && +if (optname ==TCP_MD5SIG_EXT && cmd.tcpm_ifindex && cmd.tcpm_flags &TCP_MD5SIG_FLAG_IFINDEX ) {struct net_device *dev;rcu_read_lock (); dev =dev_get_by_index_rcu (sock_net (sk), cmd.tcpm_ifindex);
@@ -638,26 +641,26 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname,if (!cmd.tcpm_keylen) {if (ipv6_addr_v4mapped (&sin6->sin6_addr))return tcp_md5_do_del (sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3 ],AF_INET , prefixlen, - l3index); + l3index, flags);return tcp_md5_do_del (sk, (union tcp_md5_addr *)&sin6->sin6_addr, -AF_INET6 , prefixlen, l3index); +AF_INET6 , prefixlen, l3index, flags); }if (cmd.tcpm_keylen >TCP_MD5SIG_MAXKEYLEN )return -EINVAL ;if (ipv6_addr_v4mapped (&sin6->sin6_addr))return tcp_md5_do_add (sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3 ], -AF_INET , prefixlen, l3index, +AF_INET , prefixlen, l3index, flags, cmd.tcpm_key, cmd.tcpm_keylen,GFP_KERNEL );return tcp_md5_do_add (sk, (union tcp_md5_addr *)&sin6->sin6_addr, -AF_INET6 , prefixlen, l3index, +AF_INET6 , prefixlen, l3index, flags, cmd.tcpm_key, cmd.tcpm_keylen,GFP_KERNEL ); } static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp, const struct in6_addr *daddr,
@@ -1402,11 +1405,11 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff ** on the newsk structure. If we fail to get * memory, then we end up not copying the key * across. Shucks. */ tcp_md5_do_add (newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr, -AF_INET6 ,128 , l3index, key->key, key->keylen, +AF_INET6 ,128 , l3index, key->flags, key->key, key->keylen, sk_gfp_mask(sk, GFP_ATOMIC)); }#endif if (__inet_inherit_port (sk, newsk) <0 ) {
Signature · 1 line
2.25.1
Re: [PATCH v2 2/4] tcp: md5: Allow MD5SIG_FLAG_IFINDEX with ifindex=0
Quoted 5 lines · Leonard Crestez
Multiple VRFs are generally meant to be "separate" but right now md5 keys for the default VRF also affect connections inside VRFs if the IP addresses happen to overlap.
So far the combination of
TCP_MD5SIG_IFINDEXwithtcpm_ifindex== 0
TCP_MD5SIG_IFINDEX does not exist in net-next and it was not added by patch 1 or this patch.
Re: [PATCH v2 2/4] tcp: md5: Allow MD5SIG_FLAG_IFINDEX with ifindex=0
Quoted 9 lines · David Ahern
Quoted 5 lines · Leonard Crestez
Multiple VRFs are generally meant to be "separate" but right now md5 keys for the default VRF also affect connections inside VRFs if the IP addresses happen to overlap.
So far the combination of
TCP_MD5SIG_IFINDEXwithtcpm_ifindex== 0
TCP_MD5SIG_IFINDEXdoes not exist in net-next and it was not added by patch 1 or this patch.
Commit message is wrong, it should refer to TCP_MD5SIG_FLAG_IFINDEX.
--
Regards,
Leonard
Re: [PATCH v2 2/4] tcp: md5: Allow MD5SIG_FLAG_IFINDEX with ifindex=0
In reply to Leonard CrestezQuoted 8 lines · Leonard Crestez
Multiple VRFs are generally meant to be "separate" but right now md5 keys for the default VRF also affect connections inside VRFs if the IP addresses happen to overlap.
So far the combination of
TCP_MD5SIG_IFINDEXwithtcpm_ifindex== 0 was an error, accept this to mean "key only applies to default VRF". This is what applications using VRFs for traffic separation want.
Quoted 7 lines
Signed-off-by Leonard Crestez <[email protected]>--- include/net/tcp.h | 5 +++-- net/ipv4/tcp_ipv4.c | 26 ++++++++++++++++---------- net/ipv6/tcp_ipv6.c | 15 +++++++++------ 3 files changed, 28 insertions(+), 18 deletions(-)
PATCH v2 3/4 selftests: nettest: Add --{do,no}-bind-key-ifindex
In reply to Leonard CrestezThese options allow explicit control over the TCP_MD5SIG_FLAG_IFINDEX flag instead of always setting it based on binding to an interface.
Do this by converting to getopt_long because nettest has too many single-character flags already and getopt_long is widely used in selftests.
--- tools/testing/selftests/net/nettest.c | 28 +++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
tools/testing/selftests/net/nettest.c +26 -2
@@ -26,10 +26,11 @@#include <stdlib.h> #include <string.h> #include <unistd.h> #include <time.h> #include <errno.h> +#include <getopt.h> #include <linux/xfrm.h> #include <linux/ipsec.h> #include <linux/pfkeyv2.h>
@@ -99,10 +100,12 @@ struct sock_args {union {struct sockaddr_in v4;struct sockaddr_in6 v6; } md5_prefix;unsigned int prefix_len; +/* 0: default, -1: force off, +1: force on */ +int bind_key_ifindex;/* expected addresses and device index for connection */ const char *expected_dev;const char *expected_server_dev;int expected_ifindex;
@@ -269,15 +272,18 @@ static int tcp_md5sig(int sd, void *addr, socklen_t alen, struct sock_args *args md5sig.tcpm_prefixlen = args->prefix_len; addr = &args->md5_prefix; }memcpy (&md5sig.tcpm_addr, addr, alen); -if (args->ifindex) { +if ((args->ifindex && args->bind_key_ifindex >=0 ) || args->bind_key_ifindex >=1 ) { opt =TCP_MD5SIG_EXT ; md5sig.tcpm_flags |=TCP_MD5SIG_FLAG_IFINDEX ; md5sig.tcpm_ifindex = args->ifindex; +log_msg ("TCP_MD5SIG_FLAG_IFINDEX set tcpm_ifindex=%d\n" , md5sig.tcpm_ifindex); + }else { +log_msg ("TCP_MD5SIG_FLAG_IFINDEX off\n" , md5sig.tcpm_ifindex); } rc =setsockopt (sd,IPPROTO_TCP , opt, &md5sig,sizeof (md5sig));if (rc <0 ) {/* ENOENT is harmless. Returned when a password is cleared */
@@ -1820,10 +1826,18 @@ static int ipc_parent(int cpid, int fd, struct sock_args *args)wait (&status);return client_status; }#define GETOPT_STR "sr:l:c:p:t:g:P:DRn:M:X:m:d:I:BN:O:SCi6xL:0:1:2:3:Fbq" +#define OPT_DO_BIND_KEY_IFINDEX 1001 +#define OPT_NO_BIND_KEY_IFINDEX 1002 + +static struct option long_opts[] = { + {"do-bind-key-ifindex" ,0 ,0 ,OPT_DO_BIND_KEY_IFINDEX }, + {"no-bind-key-ifindex" ,0 ,0 ,OPT_NO_BIND_KEY_IFINDEX }, + {0 ,0 ,0 ,0 } +};static void print_usage (char *prog) {printf ( "usage: %s OPTS\n"
@@ -1856,10 +1870,14 @@ static void print_usage(char *prog)" -n num number of times to send message\n" "\n" " -M password use MD5 sum protection\n" " -X password MD5 password for client mode\n" " -m prefix/len prefix and length to use for MD5 key\n" +" --no-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX off\n" +" --do-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX on\n" +" (default: only if -I is passed)\n" +"\n" " -g grp multicast group (e.g., 239.1.1.1)\n" " -i interactive mode (default is echo and terminate)\n" "\n" " -0 addr Expected local address\n" " -1 addr Expected remote address\n"
@@ -1891,11 +1909,11 @@ int main(int argc, char *argv[])/* * process input args */ -while ((rc =getopt (argc, argv,GETOPT_STR )) !=-1 ) { +while ((rc =getopt_long (argc, argv,GETOPT_STR , long_opts,NULL )) !=-1 ) {switch (rc) {case 'B' : both_mode =1 ;break ;case 's' :
@@ -1964,10 +1982,16 @@ int main(int argc, char *argv[]) msg =random_msg (atoi (optarg));break ;case 'M' : args.password = optarg;break ; +case OPT_DO_BIND_KEY_IFINDEX : + args.bind_key_ifindex =1 ; +break ; +case OPT_NO_BIND_KEY_IFINDEX : + args.bind_key_ifindex =-1 ; +break ;case 'X' : args.client_pw = optarg;break ;case 'm' : args.md5_prefix_str = optarg;
Signature · 1 line
2.25.1
Re: [PATCH v2 3/4] selftests: nettest: Add --{do,no}-bind-key-ifindex
tools/testing/selftests/net/nettest.c static void print_usage(char *prog) · line 1877
Quoted 9 lines · Leonard Crestez
tools/testing/selftests/net/nettest.c +3
-0@@ -1856,10 +1870,14 @@ static void print_usage(char *prog)" -n num number of times to send message\n" "\n" " -M password use MD5 sum protection\n" " -X password MD5 password for client mode\n" " -m prefix/len prefix and length to use for MD5 key\n" +" --no-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX off\n" +" --do-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX on\n" +" (default: only if -I is passed)\n"
a nit:
just --bind-key-ifindex and --no-bind-key-ifindex
Re: [PATCH v2 3/4] selftests: nettest: Add --{do,no}-bind-key-ifindex
Quoted 14 lines · David Ahern
tools/testing/selftests/net/nettest.c static void print_usage(char *prog) · line 1877
Quoted 9 lines · Leonard Crestez
tools/testing/selftests/net/nettest.c +3
-0@@ -1856,10 +1870,14 @@ static void print_usage(char *prog)" -n num number of times to send message\n" "\n" " -M password use MD5 sum protection\n" " -X password MD5 password for client mode\n" " -m prefix/len prefix and length to use for MD5 key\n" +" --no-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX off\n" +" --do-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX on\n" +" (default: only if -I is passed)\n" a nit:
just --bind-key-ifindex and --no-bind-key-ifindex
Quoted 1 line
Reviewed-by David Ahern <[email protected]>
That would be slightly confusing because it would be easy to assume that the default behavior is "--bind-key-ifindex".
Instead of using "--do-" as an intensifier I can use "--force-".
--
Regards,
Leonard
PATCH v2 4/4 selftests: net/fcnal: Test --{do,no}-bind-key-ifindex
In reply to Leonard CrestezTest that applications binding listening sockets to VRFs without specifying TCP_MD5SIG_FLAG_IFINDEX will work as expected. This would be broken if __tcp_md5_do_lookup always made a strict comparison on l3index. See this email:
https://lore.kernel.org/netdev/[email protected]/
Applications using tcp_l3mdev_accept=1 and a single global socket (not bound to any interface) also should have a way to specify keys that are only for the default VRF, this is done by --do-bind-key-ifindex without otherwise binding to a device.
--- tools/testing/selftests/net/fcnal-test.sh | 60 +++++++++++++++++++++++ 1 file changed, 60 insertions(+)
tools/testing/selftests/net/fcnal-test.sh +60 -0
@@ -287,10 +287,16 @@ set_sysctl() echo "SYSCTL: $*" echo run_cmd sysctl -q -w $* } +# get sysctl values in NS-A +get_sysctl() +{ + ${NSA_CMD} sysctl -n $* +} + ################################################################################ # Setup for tests addr2str() {
@@ -1001,10 +1007,64 @@ ipv4_tcp_md5() log_start run_cmd nettest -s -I ${NSA_DEV} -M ${MD5_PW} -m ${NS_NET} log_test $? 1 "MD5: VRF: Device must be a VRF - prefix" + test_ipv4_md5_vrf__vrf_server__no_bind_ifindex + test_ipv4_md5_vrf__global_server__bind_ifindex0 +} + +test_ipv4_md5_vrf__vrf_server__no_bind_ifindex() +{ + log_start + show_hint "Simulates applications using VRF without TCP_MD5SIG_FLAG_IFINDEX" + run_cmd nettest -s -I ${VRF} -M ${MD5_PW} -m ${NS_NET} --no-bind-key-ifindex & + sleep 1 + run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW} + log_test $? 0 "MD5: VRF: VRF-bound server, unbound key accepts connection" + + log_start + show_hint "Binding both the socket and the key is not required but it works" + run_cmd nettest -s -I ${VRF} -M ${MD5_PW} -m ${NS_NET} --do-bind-key-ifindex & + sleep 1 + run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW} + log_test $? 0 "MD5: VRF: VRF-bound server, bound key accepts connection" +} + +test_ipv4_md5_vrf__global_server__bind_ifindex0() +{ + # This particular test needs tcp_l3mdev_accept=1 for Global server to accept VRF connections + local old_tcp_l3mdev_accept + old_tcp_l3mdev_accept=$(get_sysctl net.ipv4.tcp_l3mdev_accept) + set_sysctl net.ipv4.tcp_l3mdev_accept=1 + + log_start + run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --do-bind-key-ifindex & + sleep 1 + run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW} + log_test $? 2 "MD5: VRF: Global server, Key bound to ifindex=0 rejects VRF connection" + + log_start + run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --do-bind-key-ifindex & + sleep 1 + run_cmd_nsc nettest -r ${NSA_IP} -X ${MD5_PW} + log_test $? 0 "MD5: VRF: Global server, key bound to ifindex=0 accepts non-VRF connection" + log_start + + run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --no-bind-key-ifindex & + sleep 1 + run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW} + log_test $? 0 "MD5: VRF: Global server, key not bound to ifindex accepts VRF connection" + + log_start + run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --no-bind-key-ifindex & + sleep 1 + run_cmd_nsc nettest -r ${NSA_IP} -X ${MD5_PW} + log_test $? 0 "MD5: VRF: Global server, key not bound to ifindex accepts non-VRF connection" + + # restore value + set_sysctl net.ipv4.tcp_l3mdev_accept="$old_tcp_l3mdev_accept" } ipv4_tcp_novrf() { local a
Signature · 1 line
2.25.1
Re: [PATCH v2 4/4] selftests: net/fcnal: Test --{do,no}-bind-key-ifindex
Quoted 12 lines · Leonard Crestez
Test that applications binding listening sockets to VRFs without specifying
TCP_MD5SIG_FLAG_IFINDEXwill work as expected. This would be broken if__tcp_md5_do_lookupalways made a strict comparison on l3index. See this email:https://lore.kernel.org/netdev/[email protected]/
Applications using tcp_l3mdev_accept=1 and a single global socket (not bound to any interface) also should have a way to specify keys that are only for the default VRF, this is done by --do-bind-key-ifindex without otherwise binding to a device.
Quoted 5 lines
Signed-off-by Leonard Crestez <[email protected]>--- tools/testing/selftests/net/fcnal-test.sh | 60 +++++++++++++++++++++++ 1 file changed, 60 insertions(+)