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.
An alternative would be to select a default (non-zero) port if none is
provided.
> >
> >
> > > +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.
--
Sabrina
