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

[Antonio Quartulli]

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.