Pull request: 3443 dhcp broadcast

Merge in DNS/adguard-home from 3443-fix-wired-dhcp to master

Updates #3443.

Squashed commit of the following:

commit ec7c3b73e274ba7c05c5856cb2f4ea218ad46870
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Sep 7 18:21:07 2021 +0300

    dhcpd: imp docs & naming

commit d87c646af44d837c81aa032e82aad846bde97bc8
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Sep 7 16:56:36 2021 +0300

    dhcpd: fix build tags & log changes

commit cbd0b3c1aa0efb32aeaa9130111719ab3861e817
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Sep 7 16:26:50 2021 +0300

    dhcpd: revert 3443 & imp broadcasting

CHANGELOG.md

@@ -110,6 +110,7 @@ In this release, the schema version has changed from 10 to 12.
 
 ### Fixed
 
+- DHCP now follows RFCs more closely ([#3443]).
 - Occasional panics when reading old statistics databases ([#3506]).
 - `reload` service action on macOS and FreeBSD ([#3457]).
 - Inaccurate using of service actions in the installation script ([#3450]).
@@ -177,6 +178,7 @@ In this release, the schema version has changed from 10 to 12.
 [#3417]: https://github.com/AdguardTeam/AdGuardHome/issues/3417
 [#3435]: https://github.com/AdguardTeam/AdGuardHome/issues/3435
 [#3437]: https://github.com/AdguardTeam/AdGuardHome/issues/3437
+[#3443]: https://github.com/AdguardTeam/AdGuardHome/issues/3443
 [#3450]: https://github.com/AdguardTeam/AdGuardHome/issues/3450
 [#3457]: https://github.com/AdguardTeam/AdGuardHome/issues/3457
 [#3506]: https://github.com/AdguardTeam/AdGuardHome/issues/3506

internal/dhcpd/broadcast_bsd.go

@@ -0,0 +1,37 @@
+//go:build freebsd || openbsd
+// +build freebsd openbsd
+
+package dhcpd
+
+import (
+	"net"
+
+	"github.com/AdguardTeam/golibs/log"
+	"github.com/insomniacslk/dhcp/dhcpv4"
+)
+
+// broadcast sends resp to the broadcast address specific for network interface.
+func (s *v4Server) broadcast(peer net.Addr, conn net.PacketConn, resp *dhcpv4.DHCPv4) {
+	// peer is expected to be of type *net.UDPConn as the server4.NewServer
+	// initializes it.
+	udpPeer, ok := peer.(*net.UDPAddr)
+	if !ok {
+		log.Error("dhcpv4: peer is of unexpected type %T", peer)
+
+		return
+	}
+
+	// Despite the fact that server4.NewIPv4UDPConn explicitly sets socket
+	// options to allow broadcasting, it also binds the connection to a
+	// specific interface.  On FreeBSD and OpenBSD conn.WriteTo causes
+	// errors while writing to the addresses that belong to another
+	// interface.  So, use the broadcast address specific for the binded
+	// interface.
+	udpPeer.IP = s.conf.broadcastIP
+
+	log.Debug("dhcpv4: sending to %s: %s", peer, resp.Summary())
+
+	if _, err := conn.WriteTo(resp.ToBytes(), peer); err != nil {
+		log.Error("dhcpv4: conn.Write to %s failed: %s", peer, err)
+	}
+}

internal/dhcpd/broadcast_bsd_test.go

@@ -0,0 +1,87 @@
+//go:build freebsd || openbsd
+// +build freebsd openbsd
+
+package dhcpd
+
+import (
+	"bytes"
+	"net"
+	"testing"
+
+	"github.com/AdguardTeam/golibs/netutil"
+	"github.com/insomniacslk/dhcp/dhcpv4"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestV4Server_Send_broadcast(t *testing.T) {
+	b := &bytes.Buffer{}
+	var peer *net.UDPAddr
+
+	conn := &fakePacketConn{
+		writeTo: func(p []byte, addr net.Addr) (n int, err error) {
+			udpPeer, ok := addr.(*net.UDPAddr)
+			require.True(t, ok)
+
+			peer = cloneUDPAddr(udpPeer)
+
+			n, err = b.Write(p)
+			require.NoError(t, err)
+
+			return n, nil
+		},
+	}
+
+	defaultPeer := &net.UDPAddr{
+		IP: net.IP{1, 2, 3, 4},
+		// Use neither client nor server port.
+		Port: 1234,
+	}
+	s := &v4Server{
+		conf: V4ServerConf{
+			broadcastIP: net.IP{1, 2, 3, 255},
+		},
+	}
+
+	testCases := []struct {
+		name string
+		req  *dhcpv4.DHCPv4
+		resp *dhcpv4.DHCPv4
+	}{{
+		name: "nak",
+		req: &dhcpv4.DHCPv4{
+			GatewayIPAddr: netutil.IPv4Zero(),
+		},
+		resp: &dhcpv4.DHCPv4{
+			Options: dhcpv4.OptionsFromList(
+				dhcpv4.OptMessageType(dhcpv4.MessageTypeNak),
+			),
+		},
+	}, {
+		name: "fully_unspecified",
+		req: &dhcpv4.DHCPv4{
+			GatewayIPAddr: netutil.IPv4Zero(),
+			ClientIPAddr:  netutil.IPv4Zero(),
+		},
+		resp: &dhcpv4.DHCPv4{
+			Options: dhcpv4.OptionsFromList(
+				dhcpv4.OptMessageType(dhcpv4.MessageTypeOffer),
+			),
+		},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			s.send(cloneUDPAddr(defaultPeer), conn, tc.req, tc.resp)
+			assert.EqualValues(t, tc.resp.ToBytes(), b.Bytes())
+			assert.Equal(t, &net.UDPAddr{
+				IP:   s.conf.broadcastIP,
+				Port: defaultPeer.Port,
+				Zone: defaultPeer.Zone,
+			}, peer)
+		})
+
+		b.Reset()
+		peer = nil
+	}
+}

internal/dhcpd/broadcast_others.go

@@ -0,0 +1,51 @@
+//go:build aix || darwin || dragonfly || linux || netbsd || solaris
+// +build aix darwin dragonfly linux netbsd solaris
+
+package dhcpd
+
+import (
+	"net"
+
+	"github.com/AdguardTeam/golibs/log"
+	"github.com/insomniacslk/dhcp/dhcpv4"
+)
+
+// broadcast sends resp to the broadcast address specific for network interface.
+func (s *v4Server) broadcast(peer net.Addr, conn net.PacketConn, resp *dhcpv4.DHCPv4) {
+	respData := resp.ToBytes()
+
+	log.Debug("dhcpv4: sending to %s: %s", peer, resp.Summary())
+
+	// This write to 0xffffffff reverts some behavior changes made in
+	// https://github.com/AdguardTeam/AdGuardHome/issues/3289.  The DHCP
+	// server should broadcast the message to 0xffffffff but it's
+	// inconsistent with the actual mental model of DHCP implementation
+	// which requires the network interface selection to bind to.
+	//
+	// See https://github.com/AdguardTeam/AdGuardHome/issues/3480 and
+	// https://github.com/AdguardTeam/AdGuardHome/issues/3366.
+	//
+	// See also https://github.com/AdguardTeam/AdGuardHome/issues/3539.
+	if _, err := conn.WriteTo(respData, peer); err != nil {
+		log.Error("dhcpv4: conn.Write to %s failed: %s", peer, err)
+	}
+
+	// peer is expected to be of type *net.UDPConn as the server4.NewServer
+	// initializes it.
+	udpPeer, ok := peer.(*net.UDPAddr)
+	if !ok {
+		log.Error("dhcpv4: peer is of unexpected type %T", peer)
+
+		return
+	}
+
+	// Broadcast the message one more time using the interface-specific
+	// broadcast address.
+	udpPeer.IP = s.conf.broadcastIP
+
+	log.Debug("dhcpv4: sending to %s: %s", peer, resp.Summary())
+
+	if _, err := conn.WriteTo(respData, peer); err != nil {
+		log.Error("dhcpv4: conn.Write to %s failed: %s", peer, err)
+	}
+}

internal/dhcpd/broadcast_others_test.go

@@ -0,0 +1,96 @@
+//go:build aix || darwin || dragonfly || linux || netbsd || solaris
+// +build aix darwin dragonfly linux netbsd solaris
+
+package dhcpd
+
+import (
+	"bytes"
+	"net"
+	"testing"
+
+	"github.com/AdguardTeam/golibs/netutil"
+	"github.com/insomniacslk/dhcp/dhcpv4"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestV4Server_Send_broadcast(t *testing.T) {
+	b := &bytes.Buffer{}
+	var peers []*net.UDPAddr
+
+	conn := &fakePacketConn{
+		writeTo: func(p []byte, addr net.Addr) (n int, err error) {
+			udpPeer, ok := addr.(*net.UDPAddr)
+			require.True(t, ok)
+
+			peers = append(peers, cloneUDPAddr(udpPeer))
+
+			n, err = b.Write(p)
+			require.NoError(t, err)
+
+			return n, nil
+		},
+	}
+
+	defaultPeer := &net.UDPAddr{
+		IP: net.IP{1, 2, 3, 4},
+		// Use neither client nor server port.
+		Port: 1234,
+	}
+	s := &v4Server{
+		conf: V4ServerConf{
+			broadcastIP: net.IP{1, 2, 3, 255},
+		},
+	}
+
+	testCases := []struct {
+		name string
+		req  *dhcpv4.DHCPv4
+		resp *dhcpv4.DHCPv4
+	}{{
+		name: "nak",
+		req: &dhcpv4.DHCPv4{
+			GatewayIPAddr: netutil.IPv4Zero(),
+		},
+		resp: &dhcpv4.DHCPv4{
+			Options: dhcpv4.OptionsFromList(
+				dhcpv4.OptMessageType(dhcpv4.MessageTypeNak),
+			),
+		},
+	}, {
+		name: "fully_unspecified",
+		req: &dhcpv4.DHCPv4{
+			GatewayIPAddr: netutil.IPv4Zero(),
+			ClientIPAddr:  netutil.IPv4Zero(),
+		},
+		resp: &dhcpv4.DHCPv4{
+			Options: dhcpv4.OptionsFromList(
+				dhcpv4.OptMessageType(dhcpv4.MessageTypeOffer),
+			),
+		},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			s.send(cloneUDPAddr(defaultPeer), conn, tc.req, tc.resp)
+
+			// The same response is written twice.
+			respData := tc.resp.ToBytes()
+			assert.EqualValues(t, append(respData, respData...), b.Bytes())
+
+			require.Len(t, peers, 2)
+
+			assert.Equal(t, &net.UDPAddr{
+				IP:   defaultPeer.IP,
+				Port: defaultPeer.Port,
+			}, peers[0])
+			assert.Equal(t, &net.UDPAddr{
+				IP:   s.conf.broadcastIP,
+				Port: defaultPeer.Port,
+			}, peers[1])
+		})
+
+		b.Reset()
+		peers = nil
+	}
+}

internal/dhcpd/dhcpd_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghtest"
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -136,3 +137,12 @@ func TestNormalizeLeases(t *testing.T) {
 	assert.Equal(t, leases[1].HWAddr, staticLeases[1].HWAddr)
 	assert.Equal(t, leases[2].HWAddr, dynLeases[1].HWAddr)
 }
+
+// cloneUDPAddr returns a deep copy of a.
+func cloneUDPAddr(a *net.UDPAddr) (copy *net.UDPAddr) {
+	return &net.UDPAddr{
+		IP:   netutil.CloneIP(a.IP),
+		Port: a.Port,
+		Zone: a.Zone,
+	}
+}

internal/dhcpd/v4.go

@@ -928,28 +928,50 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4
 		resp.Options.Update(dhcpv4.OptMessageType(dhcpv4.MessageTypeNak))
 	}
 
-	// peer is expected to be of type *net.UDPConn as the server4.NewServer
+	s.send(peer, conn, req, resp)
-	// initializes it.
+}
-	udpPeer, ok := peer.(*net.UDPAddr)
+
-	if !ok {
+// send writes resp for peer to conn considering the req's fields and options
-		log.Error("dhcpv4: peer is of unexpected type %T", peer)
+// according to RFC-2131.
+//
+// See https://datatracker.ietf.org/doc/html/rfc2131#section-4.1.
+func (s *v4Server) send(peer net.Addr, conn net.PacketConn, req, resp *dhcpv4.DHCPv4) {
+	var unicast bool
+	if giaddr, unspec := req.GatewayIPAddr, netutil.IPv4Zero(); !giaddr.Equal(unspec) {
+		// Send any return messages to the server port on the BOOTP
+		// relay agent whose address appears in giaddr.
+		peer = &net.UDPAddr{
+			IP:   giaddr,
+			Port: dhcpv4.ServerPort,
+		}
+		unicast = true
+	} else if mtype := resp.MessageType(); mtype == dhcpv4.MessageTypeNak {
+		// Broadcast any DHCPNAK messages to 0xffffffff.
+	} else if ciaddr := req.ClientIPAddr; !ciaddr.Equal(unspec) {
+		// Unicast DHCPOFFER and DHCPACK messages to the address in
+		// ciaddr.
+		peer = &net.UDPAddr{
+			IP:   ciaddr,
+			Port: dhcpv4.ClientPort,
+		}
+		unicast = true
+	}
+
+	// TODO(e.burkov):  Unicast the message to the actual link-layer address
+	// of the client if broadcast bit is not set.  Perhaps, use custom
+	// connection when creating the server.
+	//
+	// See https://github.com/AdguardTeam/AdGuardHome/issues/3443.
+
+	if !unicast {
+		s.broadcast(peer, conn, resp)
 
 		return
 	}
 
-	// Despite the fact that server4.NewIPv4UDPConn explicitly sets socket
+	log.Debug("dhcpv4: sending to %s: %s", peer, resp.Summary())
-	// options to allow broadcasting, it also binds the connection to a
-	// specific interface.  On FreeBSD conn.WriteTo causes errors while
-	// writing to the addresses that belong to another interface.  So, use
-	// the broadcast address specific for the binded interface in case
-	// server4.Server.Serve sets it to net.IPv4Bcast.
-	if udpPeer.IP.Equal(net.IPv4bcast) {
-		udpPeer.IP = s.conf.broadcastIP
-	}
 
-	log.Debug("dhcpv4: sending: %s", resp.Summary())
+	_, err := conn.WriteTo(resp.ToBytes(), peer)
-
-	_, err = conn.WriteTo(resp.ToBytes(), peer)
 	if err != nil {
 		log.Error("dhcpv4: conn.Write to %s failed: %s", peer, err)
 	}

internal/dhcpd/v4_test.go

@@ -4,9 +4,11 @@
 package dhcpd
 
 import (
+	"bytes"
 	"net"
 	"testing"
 
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/insomniacslk/dhcp/dhcpv4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -365,3 +367,83 @@ func TestNormalizeHostname(t *testing.T) {
 		})
 	}
 }
+
+// fakePacketConn is a mock implementation of net.PacketConn to simplify
+// testing.
+type fakePacketConn struct {
+	// writeTo is used to substitute net.PacketConn's WriteTo method.
+	writeTo func(p []byte, addr net.Addr) (n int, err error)
+	// net.PacketConn is embedded here simply to make *fakePacketConn a
+	// net.PacketConn without actually implementing all methods.
+	net.PacketConn
+}
+
+// WriteTo implements net.PacketConn interface for *fakePacketConn.
+func (fc *fakePacketConn) WriteTo(p []byte, addr net.Addr) (n int, err error) {
+	return fc.writeTo(p, addr)
+}
+
+func TestV4Server_Send_unicast(t *testing.T) {
+	b := &bytes.Buffer{}
+	var peer *net.UDPAddr
+
+	conn := &fakePacketConn{
+		writeTo: func(p []byte, addr net.Addr) (n int, err error) {
+			udpPeer, ok := addr.(*net.UDPAddr)
+			require.True(t, ok)
+
+			peer = cloneUDPAddr(udpPeer)
+
+			n, err = b.Write(p)
+			require.NoError(t, err)
+
+			return n, nil
+		},
+	}
+
+	defaultPeer := &net.UDPAddr{
+		IP: net.IP{1, 2, 3, 4},
+		// Use neither client nor server port.
+		Port: 1234,
+	}
+	defaultResp := &dhcpv4.DHCPv4{
+		OpCode: dhcpv4.OpcodeBootReply,
+	}
+	s := &v4Server{}
+
+	testCases := []struct {
+		name     string
+		req      *dhcpv4.DHCPv4
+		wantPeer net.Addr
+	}{{
+		name: "relay_agent",
+		req: &dhcpv4.DHCPv4{
+			GatewayIPAddr: defaultPeer.IP,
+		},
+		wantPeer: &net.UDPAddr{
+			IP:   defaultPeer.IP,
+			Port: dhcpv4.ServerPort,
+		},
+	}, {
+		name: "known_client",
+		req: &dhcpv4.DHCPv4{
+			GatewayIPAddr: netutil.IPv4Zero(),
+			ClientIPAddr:  net.IP{2, 3, 4, 5},
+		},
+		wantPeer: &net.UDPAddr{
+			IP:   net.IP{2, 3, 4, 5},
+			Port: dhcpv4.ClientPort,
+		},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			s.send(defaultPeer, conn, tc.req, defaultResp)
+			assert.EqualValues(t, defaultResp.ToBytes(), b.Bytes())
+			assert.Equal(t, tc.wantPeer, peer)
+		})
+
+		b.Reset()
+		peer = nil
+	}
+}