From b77ceaaf550573da557809b5a76c832ed0f922c9 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Tue, 7 Sep 2021 18:33:23 +0300 Subject: [PATCH] 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 Date: Tue Sep 7 18:21:07 2021 +0300 dhcpd: imp docs & naming commit d87c646af44d837c81aa032e82aad846bde97bc8 Author: Eugene Burkov Date: Tue Sep 7 16:56:36 2021 +0300 dhcpd: fix build tags & log changes commit cbd0b3c1aa0efb32aeaa9130111719ab3861e817 Author: Eugene Burkov Date: Tue Sep 7 16:26:50 2021 +0300 dhcpd: revert 3443 & imp broadcasting --- CHANGELOG.md | 2 + internal/dhcpd/broadcast_bsd.go | 37 ++++++++++ internal/dhcpd/broadcast_bsd_test.go | 87 ++++++++++++++++++++++ internal/dhcpd/broadcast_others.go | 51 +++++++++++++ internal/dhcpd/broadcast_others_test.go | 96 +++++++++++++++++++++++++ internal/dhcpd/dhcpd_test.go | 10 +++ internal/dhcpd/v4.go | 56 ++++++++++----- internal/dhcpd/v4_test.go | 82 +++++++++++++++++++++ 8 files changed, 404 insertions(+), 17 deletions(-) create mode 100644 internal/dhcpd/broadcast_bsd.go create mode 100644 internal/dhcpd/broadcast_bsd_test.go create mode 100644 internal/dhcpd/broadcast_others.go create mode 100644 internal/dhcpd/broadcast_others_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e274ab58..4ecdbdd5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dhcpd/broadcast_bsd.go b/internal/dhcpd/broadcast_bsd.go new file mode 100644 index 00000000..b2d41f75 --- /dev/null +++ b/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) + } +} diff --git a/internal/dhcpd/broadcast_bsd_test.go b/internal/dhcpd/broadcast_bsd_test.go new file mode 100644 index 00000000..49221b04 --- /dev/null +++ b/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 + } +} diff --git a/internal/dhcpd/broadcast_others.go b/internal/dhcpd/broadcast_others.go new file mode 100644 index 00000000..f801497e --- /dev/null +++ b/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) + } +} diff --git a/internal/dhcpd/broadcast_others_test.go b/internal/dhcpd/broadcast_others_test.go new file mode 100644 index 00000000..246be382 --- /dev/null +++ b/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 + } +} diff --git a/internal/dhcpd/dhcpd_test.go b/internal/dhcpd/dhcpd_test.go index faae9e8d..71878369 100644 --- a/internal/dhcpd/dhcpd_test.go +++ b/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, + } +} diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index 8d2832a1..3e9a954c 100644 --- a/internal/dhcpd/v4.go +++ b/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) +} + +// 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 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 - // 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 to %s: %s", peer, resp.Summary()) - 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) } diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index 9630d8cc..bbc1dcea 100644 --- a/internal/dhcpd/v4_test.go +++ b/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 + } +}