dhcpd: revert 3443 & imp broadcasting

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,95 @@
+//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 copyUDPAddr(a *net.UDPAddr) (copy *net.UDPAddr) {
+	return &net.UDPAddr{
+		IP:   netutil.CloneIP(a.IP),
+		Port: a.Port,
+		Zone: a.Zone,
+	}
+}
+
+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 = copyUDPAddr(udpPeer)
+
+			n, err = b.Write(p)
+			require.NoError(t, err)
+
+			return n, nil
+		},
+	}
+
+	defaultPeer := &net.UDPAddr{
+		IP: net.IP{1, 2, 3, 4},
+		// Neither 'Client Port' 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(copyUDPAddr(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 !(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) {
+	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,104 @@
+//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 copyUDPAddr(a *net.UDPAddr) (copy *net.UDPAddr) {
+	return &net.UDPAddr{
+		IP:   netutil.CloneIP(a.IP),
+		Port: a.Port,
+		Zone: a.Zone,
+	}
+}
+
+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, copyUDPAddr(udpPeer))
+
+			n, err = b.Write(p)
+			require.NoError(t, err)
+
+			return n, nil
+		},
+	}
+
+	defaultPeer := &net.UDPAddr{
+		IP: net.IP{1, 2, 3, 4},
+		// Neither 'Client Port' 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(copyUDPAddr(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/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
-	// initializes it.
-	udpPeer, ok := peer.(*net.UDPAddr)
-	if !ok {
-		log.Error("dhcpv4: peer is of unexpected type %T", peer)
+	s.send(peer, conn, req, resp)
+}
 
-		return
+// send writes resp for peer to conn considering the req's fields and options
+// 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 'DHCP 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
 	}
 
-	// Despite the fact that server4.NewIPv4UDPConn explicitly sets socket
-	// 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
+	// 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
 	}
 
-	log.Debug("dhcpv4: sending: %s", resp.Summary())
+	log.Debug("dhcpv4: sending to %s: %s", peer, 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)
 	}