netdev - Re: [RFC net-next 07/13] selftests: ovpn: check asymmetric peer-id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c21b6b5ef18f63e331564edf8ee7832438e163ee.camel@mandelbit.com>
Date: Tue, 02 Dec 2025 17:11:42 +0100
From: Ralf Lici <ralf@...delbit.com>
To: Sabrina Dubroca <sd@...asysnail.net>, Antonio Quartulli
	 <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>, 
	linux-kselftest@...r.kernel.org, Shuah Khan <shuah@...nel.org>
Subject: Re: [RFC net-next 07/13] selftests: ovpn: check asymmetric peer-id

On Thu, 2025-11-27 at 01:13 +0100, Sabrina Dubroca wrote:
> 2025-11-21, 01:20:38 +0100, Antonio Quartulli wrote:
> > diff --git a/tools/testing/selftests/net/ovpn/common.sh
> > b/tools/testing/selftests/net/ovpn/common.sh
> > index b91cf17ab01f..d926413c9f16 100644
> > --- a/tools/testing/selftests/net/ovpn/common.sh
> > +++ b/tools/testing/selftests/net/ovpn/common.sh
> > @@ -75,13 +75,14 @@ add_peer() {
> >  					data64.key
> >  			done
> >  		else
> > -			RADDR=$(awk "NR == ${1} {print \$2}"
> > ${UDP_PEERS_FILE})
> > -			RPORT=$(awk "NR == ${1} {print \$3}"
> > ${UDP_PEERS_FILE})
> > -			LPORT=$(awk "NR == ${1} {print \$5}"
> > ${UDP_PEERS_FILE})
> > -			ip netns exec peer${1} ${OVPN_CLI} new_peer
> > tun${1} ${1} ${LPORT} \
> > -				${RADDR} ${RPORT}
> > -			ip netns exec peer${1} ${OVPN_CLI} new_key
> > tun${1} ${1} 1 0 ${ALG} 1 \
> > -				data64.key
> > +			TX_ID=$(awk "NR == ${1} {print \$2}"
> > ${UDP_PEERS_FILE})
> > +			RADDR=$(awk "NR == ${1} {print \$3}"
> > ${UDP_PEERS_FILE})
> > +			RPORT=$(awk "NR == ${1} {print \$4}"
> > ${UDP_PEERS_FILE})
> > +			LPORT=$(awk "NR == ${1} {print \$6}"
> > ${UDP_PEERS_FILE})
> > +			ip netns exec peer${1} ${OVPN_CLI} new_peer
> > tun${1} ${TX_ID} ${1} \
> > +				${LPORT} ${RADDR} ${RPORT}
> > +			ip netns exec peer${1} ${OVPN_CLI} new_key
> > tun${1} ${TX_ID} 1 0 \
> > +				${ALG} 1 data64.key
> 
> IIUC, we're creating a "client" with peer_id=$TX_ID and tx_id=$ID so
> that they're flipped from what we installed on the multi-peer side?

Exactly. I can see that this part is not very clear so I'll properly
define PEER_ID and TX_ID for the clients in the next version of the
patch.

> > diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c
> > b/tools/testing/selftests/net/ovpn/ovpn-cli.c
> > index 064453d16fdd..baabb4c9120e 100644
> > --- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
> > +++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
> > @@ -103,7 +103,7 @@ struct ovpn_ctx {
> >  
> >  	sa_family_t sa_family;
> >  
> > -	unsigned long peer_id;
> > +	unsigned long peer_id, tx_id;
> >  	unsigned long lport;
> >  
> >  	union {
> > @@ -649,6 +649,7 @@ static int ovpn_new_peer(struct ovpn_ctx *ovpn,
> > bool is_tcp)
> >  
> >  	attr = nla_nest_start(ctx->nl_msg, OVPN_A_PEER);
> >  	NLA_PUT_U32(ctx->nl_msg, OVPN_A_PEER_ID, ovpn->peer_id);
> > +	NLA_PUT_U32(ctx->nl_msg, OVPN_A_PEER_TX_ID, ovpn->tx_id);
> 
> So, with these changes, it's no longer possible to test a userspace
> not passing the new OVPN_A_PEER_TX_ID attribute? But I guess we could
> simulate that behavior by passing TX_ID==ID (is there still a test
> doing that?).
> 
> Do we need to preserve some testing of the case where userspace is not
> passing the new attribute?
> 

That's a valid concern as we're expected to still correctly handle
userspace programs that don't pass the attribute. I'll add a boolean
flag similar to FLOAT in order to extend the tests to cover this case. 

> 
> >  	NLA_PUT_U32(ctx->nl_msg, OVPN_A_PEER_SOCKET, ovpn->socket);
> >  
> >  	if (!is_tcp) {
> > @@ -767,6 +768,10 @@ static int ovpn_handle_peer(struct nl_msg *msg,
> > void (*arg)__always_unused)
> >  		fprintf(stderr, "* Peer %u\n",
> >  			nla_get_u32(pattrs[OVPN_A_PEER_ID]));
> >  
> > +	if (pattrs[OVPN_A_PEER_TX_ID])
> > +		fprintf(stderr, "\tTX peer ID %u\n",
> > +			nla_get_u32(pattrs[OVPN_A_PEER_TX_ID]));
> > +
> >  	if (pattrs[OVPN_A_PEER_SOCKET_NETNSID])
> >  		fprintf(stderr, "\tsocket NetNS ID: %d\n",
> >  			nla_get_s32(pattrs[OVPN_A_PEER_SOCKET_NETNS
> > ID]));
> > @@ -1676,11 +1681,13 @@ static void usage(const char *cmd)
> >  		"\tkey_file: file containing the symmetric key for
> > encryption\n");
> >  
> >  	fprintf(stderr,
> > -		"* new_peer <iface> <peer_id> <lport> <raddr>
> > <rport> [vpnaddr]: add new peer\n");
> > +		"* new_peer <iface> <peer_id> <tx_id> <lport>
> > <raddr> <rport> [vpnaddr]: add new peer\n");
> >  	fprintf(stderr, "\tiface: ovpn interface name\n");
> >  	fprintf(stderr, "\tlport: local UDP port to bind to\n");
> >  	fprintf(stderr,
> > -		"\tpeer_id: peer ID to be used in data packets
> > to/from this peer\n");
> > +		"\tpeer_id: peer ID found in data packets received
> > from this peer\n");
> > +	fprintf(stderr,
> > +		"\ttx_id: peer ID to be used when sending to this
> > peer\n");
> >  	fprintf(stderr, "\traddr: peer IP address\n");
> >  	fprintf(stderr, "\trport: peer UDP port\n");
> >  	fprintf(stderr, "\tvpnaddr: peer VPN IP\n");
> > @@ -1691,7 +1698,7 @@ static void usage(const char *cmd)
> >  	fprintf(stderr, "\tlport: local UDP port to bind to\n");
> >  	fprintf(stderr,
> >  		"\tpeers_file: text file containing one peer per
> > line. Line format:\n");
> > -	fprintf(stderr, "\t\t<peer_id> <raddr> <rport>
> > <vpnaddr>\n");
> > +	fprintf(stderr, "\t\t<peer_id> <tx_id> <raddr> <rport>
> > <laddr> <lport> <vpnaddr>\n");
> 
> Looks like this is missing similar updates for connect and listen,
> based on changes to ovpn_run_cmd and ovpn_parse_cmd_args?

Good catch, thanks.

-- 
Ralf Lici
Mandelbit Srl