Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DHCPv6 Server multicast handling improvements #316

Merged
merged 3 commits into from
Sep 23, 2019

Conversation

Natolumin
Copy link
Collaborator

Summary

  • Patch 1 makes listening on an explicit multicast address work, and removes the useless joining on multicast groups when listening on an unicast address. It adds the second RFC-defined multicast group to the default groups joined when listening on the wildcard address
  • Patch 2 moves the definitions for dhcpv4.BindToInterface to the interfaces package, which seems more appropriate as they are not ipv4 specific.
    From the code comments in bsd/netinet/ip_output.c#L2715 IP_BOUND_IF for darwin doesn't do anything on the receive path, only send.IP_RECVIF however is also supported on darwin (I assume inherited from freebsd) and should do the same thing
    Note: I do not have access to either darwin or *bsd machines to test this change, but at least it does build. Any help / pointers for testing on those (esp. darwin, which I can't easily start in a cloud somewhere) would be appreciated
  • Patch 3 makes an equivalent of NewIPv4UDPConn for ipv6, using bindtodevice and v6only, to give greater control over listen parameters, and allow binding to a specific multicast address instead of wildcard

Testing

Since we can't really use default ports or require privileges in unittests, I tested the additions manually for the handling of bindtodevice.

This is a build of coredhcp with these patches, listening on wildcard addresses:

$ sudo ip netns exec dhcp_servers ss -lnpue
State                           Recv-Q                          Send-Q                                                    Local Address:Port                                                     Peer Address:Port
UNCONN                          0                               0                                                         0.0.0.0%srv_u:67                                                            0.0.0.0:*                              users:(("coredhcp",pid=126583,fd=6)) ino:837636 sk:3 <->
UNCONN                          0                               0                                                            [::]%srv_u:547                                                              [::]:*                              users:(("coredhcp",pid=126583,fd=5)) ino:837635 sk:4 v6only:1 <->

An strace log shows it joined both multicast groups (and I verified a relayed packet to ff05::1:3 was handled):

setsockopt(5, SOL_IPV6, MCAST_JOIN_GROUP, {gr_interface=if_nametoindex("srv_u"), gr_group={sa_family=AF_INET6, sin6_port=htons(0), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "ff02::1:2", &sin6_addr), sin6_scope_id=0}}, 136) = 0
setsockopt(5, SOL_IPV6, MCAST_JOIN_GROUP, {gr_interface=if_nametoindex("srv_u"), gr_group={sa_family=AF_INET6, sin6_port=htons(0), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "ff05::1:3", &sin6_addr), sin6_scope_id=0}}, 136) = 0

Similarly, I verified that asking explicitly to bind on ff05::1:3, interface srv_u, results in a socket listening on [ff05::1:3]%srv_u:547, and the mcast group is joined (but not the other one):

132015 setsockopt(5, SOL_IPV6, MCAST_JOIN_GROUP, {gr_interface=if_nametoindex("srv_u"), gr_group={sa_family=AF_INET6, sin6_port=htons(0), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "ff05::1:3", &sin6_addr), sin6_scope_id=0}}, 136) = 0

Joining a multicast group with an address that can't be received on a
socket is ineffective, at least on linux.
This updates the logic of NewServer in a mostly backwards-compatible
way, to enable listening on arbitrary multicast addresses:

 * Unicast addresses see no user-visible change, but don't join a
   multicast group for which they don't receive traffic anyway
 * Multicast addresses start actually receiving traffic for the group
   they represent, and don't join the default group. **this is a
   behaviour change**: previously they would receive traffic for the
   default group if it was on the same port and **not** for the group
   they represent. I consider that previous behaviour a bug
 * Wildcard addresses, if on the proper port, will join both
   AllDHCPRelayAgentsAndServers and AllDHCPServers **this is a behaviour
   change**: previously only AllDHCPRelayAgentsAndServers was joined
 * Wildcard addresses on another port: no visible change, same as
   unicast case

Signed-off-by: Anatole Denis <[email protected]>
This moves the implementations of the BindToInterface to the interfaces/
package, since they aren't ipv4-specific.
The BindToInterface function remains in dhcpv4 (simply wraps the one in
interfaces) to keep backwards-compatibility

Additionally, fold bindtodevice_darwin into bindtodevice_bsd: darwin is
mostly a BSD, and happens to support IP_RECVIF, so use that instead of
IP_BOUND_IF, which only affects sends, not receives according to the
code comments in bsd/netinet/ip_output.c as well as being v4-only

Signed-off-by: Anatole Denis <[email protected]>
@codecov
Copy link

codecov bot commented Sep 17, 2019

Codecov Report

Merging #316 into master will decrease coverage by 0.42%.
The diff coverage is 30.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
- Coverage    73.2%   72.78%   -0.43%     
==========================================
  Files          76       78       +2     
  Lines        3728     3762      +34     
==========================================
+ Hits         2729     2738       +9     
- Misses        860      880      +20     
- Partials      139      144       +5
Impacted Files Coverage Δ
interfaces/bindtodevice_linux.go 0% <ø> (ø)
dhcpv4/bindtointerface.go 0% <0%> (ø)
dhcpv6/server6/conn.go 29.16% <29.16%> (ø)
dhcpv6/server6/server.go 50.94% <34.78%> (-11.28%) ⬇️
dhcpv4/nclient4/client.go 63.09% <0%> (+1.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 834a63b...a8311df. Read the comment docs.

@pmazzini
Copy link
Collaborator

I don't think darwin is currenly working: #229 and #230.

pmazzini
pmazzini previously approved these changes Sep 17, 2019
Copy link
Collaborator

@pmazzini pmazzini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Natolumin
Copy link
Collaborator Author

OK, I won't worry too much about darwin then.
Let's not merge this yet though, I want to finish the coredhcp parts that go with it first in case it reveals API issues with the updated behaviour

Similar to server4 where the UDP connection is manually created using
the socket interfaces, this creates a connection with adequate options:

 * SO_BINDTODEVICE or equivalent if an interface is requested
 * V6ONLY when supported by the operating system
 * Allows binding to a multicast address specifically instead of
   falling back to wildcard

Signed-off-by: Anatole Denis <[email protected]>
@Natolumin
Copy link
Collaborator Author

v2 changed the last commit, correcting the documentation and handling the case addr == nil correctly

@pmazzini
Copy link
Collaborator

Let's not merge this yet though, I want to finish the coredhcp parts that go with it first in case it reveals API issues with the updated behaviour

Are you still testing it?

@Natolumin
Copy link
Collaborator Author

I'm pretty sure it's still broken for darwin, but no worse than it was before, so I think it's good to go in the current state

@pmazzini pmazzini merged commit 3b29cbd into insomniacslk:master Sep 23, 2019
@Natolumin Natolumin deleted the v6_multicast_fix branch October 1, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants