if_ovpn: support floating clients
ClosedPublic
Actions

Authored by kp on Wed, Jul 23, 8:31 AM.

Details

Reviewers

Group Reviewers

network
pfsense

Commits

rGfc387ed68f3c: if_ovpn: support floating clients
rG9c52600a5a15: if_ovpn: support floating clients

Summary

If a client changes its IP address notify userspace of this.

The UDP filtering function supplies the remote IP address, so we check if the
address changed there. If so, we tag the packet with the new address. Once the
packet is decrypted (and as part of that, has had its signature checked) we
can commit to the address change. Take the write lock and notify userspace of
the change.

MFC after: 3 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65665
Build 62548: arc lint + arc unit

Event Timeline

kp created this revision.Wed, Jul 23, 8:31 AM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptWed, Jul 23, 8:31 AM

kp requested review of this revision.Wed, Jul 23, 8:31 AM

kp added a child revision: D51469: if_ovpn tests: basic float test case.

Harbormaster completed remote builds in B65624: Diff 158929.Wed, Jul 23, 8:31 AM

markj added a subscriber: markj.Wed, Jul 23, 3:04 PM

markj added inline comments.

sys/net/if_ovpn.c
1490	This is missing error checking.
1706	If ovpn_notify_float() fails due to a memory allocation error or because the ring is full, we'll have updated the kernel's state and thus won't generate a notification until the address changes again. Presumably we should avoid updating `peer->remote` unless the notification was successfully sent?

ae added inline comments.Wed, Jul 23, 5:54 PM

sys/net/if_ovpn.c
323	I'm not sure how acceptable this is in your case, but for link-local addresses IPv6 it is also needed to check zone id.

ae added inline comments.Wed, Jul 23, 6:02 PM

sys/net/if_ovpn.c
323	Also it would be good to know, what type of link-local address is expected here. E.g. link-local address with embedded zone id or address with zone id initialized in sockaddr_in6->sin6_scope_id.

This verion improves error handling.

I'll take a closer look at what the udp input code does for link-local addresses
tomorrow.

Harbormaster completed remote builds in B65638: Diff 158958.Wed, Jul 23, 7:34 PM

kp added inline comments.Thu, Jul 24, 7:37 AM

sys/net/if_ovpn.c
323	We're getting the zone id in sockaddr_in6->sin6_scope_id. We get called as a udp tunnel function by udp6_append(). It gets a sockaddr_in6 from udp6_input(), which sets that up with init_sin6(), and that calls sa6_recoverscope(). So there's no embedded scope in the address, but the sin6_scope_id is populated. Looking at the rest of the kernel-side code, I think we currently don't work with link-local addresses at all, because we only copy the sin6_addr in ovpn_encap(), and don't deal with scopes as we'd need for link-local handling. I can look at adding this support, but I'd like to keep that outside of this patch. I think we don't need much more than passing the scope to/from the kernel in the configuration path, and adding a scope insertion in ovpn_encap(). And a test case, obviously.

markj added inline comments.Thu, Jul 24, 2:26 PM

sys/net/if_ovpn.c
331	This line is unreachable.
455	Why not do `nvlist_move_nvlist(parent, name, nvl)`?
1502	This path needs to free `nvl` as well.
2479

Review remarks

Harbormaster completed remote builds in B65665: Diff 159017.Thu, Jul 24, 5:14 PM

markj accepted this revision as: markj.Fri, Jul 25, 1:21 PM

markj added inline comments.

sys/net/if_ovpn.c
1499	Here you have a notification structure, `n`, which you could recycle instead of allocating a new one. That doesn't help if the ring buffer is full, but at least it closes one error path.

This revision is now accepted and ready to land.Fri, Jul 25, 1:21 PM

kp added inline comments.Fri, Jul 25, 1:53 PM

sys/net/if_ovpn.c
1499	Yeah, I considered that, but figured it wasn't worth having yet another piece of slightly different code to insert something into the ring buffer. It'd eliminate the possibility of failing to allocate memory, but it wouldn't actually guarantee the insertion succeeds, so we're still left having to handle errors either way.