Re: [PATCH net-next v24 18/23] ovpn: implement peer add/get/dump/delete via netlink

[Antonio Quartulli <antonio@openvpn.net>]

On 25/03/2025 11:56, Sabrina Dubroca wrote:
> 2025-03-25, 00:15:48 +0100, Antonio Quartulli wrote:
>> On 24/03/2025 11:48, Sabrina Dubroca wrote:
>>> Hello Antonio,
>>>
>>> A few questions wrt the API:
>>>
>>> 2025-03-18, 02:40:53 +0100, Antonio Quartulli wrote:
>>>> +static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,
>>>> +					 struct sockaddr_storage *ss)
>>>> +{
>>>> +	struct sockaddr_in6 *sin6;
>>>> +	struct sockaddr_in *sin;
>>>> +	struct in6_addr *in6;
>>>> +	__be16 port = 0;
>>>> +	__be32 *in;
>>>> +
>>>> +	ss->ss_family = AF_UNSPEC;
>>>> +
>>>> +	if (attrs[OVPN_A_PEER_REMOTE_PORT])
>>>> +		port = nla_get_be16(attrs[OVPN_A_PEER_REMOTE_PORT]);
>>>
>>> What's the expected behavior if REMOTE_PORT isn't provided? We'll send
>>> packets do port 0 (which I'm guessing will get dropped on the other
>>> side) until we get a message from the peer and float sets the correct
>>> port/address?
>>
>> I have never seen a packet going out with port 0 :)
> 
> It will if you hack into ovpn-cli to skip OVPN_A_PEER_REMOTE_PORT.
> I don't know how networks/admins react to such packets.
> 
>> But being dropped is most likely what's going to happen.
>>
>> I'd say this is not something that we expect the user to do:
>> if the remote address if specified, the user should specify a non-zero port
>> too.
>>
>> We could add a check to ensure that a port is always specified if the remote
>> address is there too, just to avoid the user to shoot himself in the foot.
>> But we expect the user to pass an addr:port where the peer is listening to
>> (and that can't be a 0 port).
> 
> If we expect that (even if a well-behaved userspace would never do
> it), I have a preference for enforcing that expectation. Since there's
> already a policy rejecting OVPN_A_PEER_REMOTE_PORT == 0, this would be
> more consistent IMO.

Ok, makes sense.

> 
> An alternative would be to select a default (non-zero) port if none is
> provided.

I prefer to enforce having a port, rather tan going with a default that 
may bite us down the road.

> 
>>>
>>>
>>>> +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
>>>> +			       struct nlattr **attrs)
>>>> +{
>>> [...]
>>>> +	/* when setting the keepalive, both parameters have to be configured */
>>>> +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
>>>> +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
>>>> +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
>>>> +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
>>>> +		ovpn_peer_keepalive_set(peer, interv, timeout);
>>>
>>> Should we interpret OVPN_A_PEER_KEEPALIVE_INTERVAL = 0 &&
>>> OVPN_A_PEER_KEEPALIVE_TIMEOUT == 0 as "disable keepalive/timeout" on
>>> an active peer?  And maybe "one set to 0, the other set to some
>>> non-zero value" as invalid?  Setting either value to 0 doesn't seem
>>> very useful (timeout = 0 will probably kill the peer immediately, and
>>> I suspect interval = 0 would be quite spammy).
>>>
>>
>> Considering "0" as "disable keepalive" is the current intention.
>>
>> In ovpn_peer_keepalive_work_single() you can see that if either one if 0, we
>> just skip the peer:
>>
>> 1217         /* we expect both timers to be configured at the same time,
>> 1218          * therefore bail out if either is not set
>> 1219          */
>> 1220         if (!peer->keepalive_timeout || !peer->keepalive_interval) {
>> 1221                 spin_unlock_bh(&peer->lock);
>> 1222                 return 0;
>> 1223         }
>>
>> does it make sense?
> 
> Ah, true. Sorry, I forgot about that.  So after _NEW/_SET we'll run
> the work once, and that peer will be ignored. And if there's no other
> peer requiring keepalive, next_run will be 0 and we don't
> reschedule. That's good, thanks.
> 

Cool,
Regards,

-- 
Antonio Quartulli
OpenVPN Inc.