Pull request: dhcpd: fix interface ipv6 check

Merge in DNS/adguard-home from 2355-dhcpcheck-ipv6 to master

Updates #2335.

Squashed commit of the following:

commit 5ce1cc7bc244ba5dd4a065d47dec8884fa3d45e7
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 14:03:24 2020 +0300

    dhcpd: fix loop exit condition

commit 32b4b946bfa30159326dc295fa1a2607b78172af
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 13:26:50 2020 +0300

    dhcpd: fix interface ipv6 check

CHANGELOG.md

@@ -30,6 +30,13 @@ and this project adheres to
 [#2297]: https://github.com/AdguardTeam/AdGuardHome/issues/2297
 [#2306]: https://github.com/AdguardTeam/AdGuardHome/issues/2306
 
+### Fixed
+
+- Incorrect detection of the IPv6 address of an interface as well as another
+  infinite loop in the `/dhcp/find_active_dhcp` HTTP API [#2355].
+
+[#2355]: https://github.com/AdguardTeam/AdGuardHome/issues/2355
+
 
 
 ## [v0.104.3] - 2020-11-19

internal/dhcpd/check_other_dhcp.go

@@ -94,12 +94,11 @@ func CheckIfOtherDHCPServersPresentV4(ifaceName string) (bool, error) {
 
 			continue
 		}
-		if ok {
-			return true, nil
-		}
 		if err != nil {
 			return false, err
 		}
+
+		return ok, nil
 	}
 }
 
@@ -216,12 +215,11 @@ func CheckIfOtherDHCPServersPresentV6(ifaceName string) (bool, error) {
 
 			continue
 		}
-		if ok {
-			return true, nil
-		}
 		if err != nil {
 			return false, err
 		}
+
+		return ok, nil
 	}
 }
 

internal/dhcpd/db.go

@@ -74,7 +74,6 @@ func (s *Server) dbLoad() {
 			} else {
 				v6DynLeases = append(v6DynLeases, &lease)
 			}
-
 		} else {
 			if obj[i].Expiry == leaseExpireStatic {
 				staticLeases = append(staticLeases, &lease)

internal/dhcpd/nclient4/client_test.go

@@ -79,7 +79,7 @@ func serveAndClient(ctx context.Context, responses [][]*dhcpv4.DHCPv4, opts ...C
 	return mc, serverConn
 }
 
-func ComparePacket(got *dhcpv4.DHCPv4, want *dhcpv4.DHCPv4) error {
+func ComparePacket(got, want *dhcpv4.DHCPv4) error {
 	if got == nil && got == want {
 		return nil
 	}
@@ -92,7 +92,7 @@ func ComparePacket(got *dhcpv4.DHCPv4, want *dhcpv4.DHCPv4) error {
 	return nil
 }
 
-func pktsExpected(got []*dhcpv4.DHCPv4, want []*dhcpv4.DHCPv4) error {
+func pktsExpected(got, want []*dhcpv4.DHCPv4) error {
 	if len(got) != len(want) {
 		return fmt.Errorf("got %d packets, want %d packets", len(got), len(want))
 	}
@@ -309,10 +309,10 @@ func TestMultipleSendAndRead(t *testing.T) {
 				newPacket(dhcpv4.OpcodeBootRequest, [4]byte{0x44, 0x44, 0x44, 0x44}),
 			},
 			server: [][]*dhcpv4.DHCPv4{
-				[]*dhcpv4.DHCPv4{ // Response for first packet.
+				{ // Response for first packet.
 					newPacket(dhcpv4.OpcodeBootReply, [4]byte{0x33, 0x33, 0x33, 0x33}),
 				},
-				[]*dhcpv4.DHCPv4{ // Response for second packet.
+				{ // Response for second packet.
 					newPacket(dhcpv4.OpcodeBootReply, [4]byte{0x44, 0x44, 0x44, 0x44}),
 				},
 			},

internal/dhcpd/nclient4/conn_unix.go

@@ -17,17 +17,13 @@ import (
 	"github.com/u-root/u-root/pkg/uio"
 )
 
-var (
+// BroadcastMac is the broadcast MAC address.
-	// BroadcastMac is the broadcast MAC address.
+//
-	//
+// Any UDP packet sent to this address is broadcast on the subnet.
-	// Any UDP packet sent to this address is broadcast on the subnet.
+var BroadcastMac = net.HardwareAddr([]byte{255, 255, 255, 255, 255, 255})
-	BroadcastMac = net.HardwareAddr([]byte{255, 255, 255, 255, 255, 255})
-)
 
-var (
+// ErrUDPAddrIsRequired is an error used when a passed argument is not of type "*net.UDPAddr".
-	// ErrUDPAddrIsRequired is an error used when a passed argument is not of type "*net.UDPAddr".
+var ErrUDPAddrIsRequired = errors.New("must supply UDPAddr")
-	ErrUDPAddrIsRequired = errors.New("must supply UDPAddr")
-)
 
 // NewRawUDPConn returns a UDP connection bound to the interface and port
 // given based on a raw packet socket. All packets are broadcasted.
@@ -68,7 +64,7 @@ func NewBroadcastUDPConn(rawPacketConn net.PacketConn, boundAddr *net.UDPAddr) n
 	}
 }
 
-func udpMatch(addr *net.UDPAddr, bound *net.UDPAddr) bool {
+func udpMatch(addr, bound *net.UDPAddr) bool {
 	if bound == nil {
 		return true
 	}

internal/dhcpd/nclient4/ipv4.go

@@ -281,7 +281,7 @@ func (b UDP) Checksum() uint16 {
 // CalculateChecksum calculates the checksum of the udp packet, given the total
 // length of the packet and the checksum of the network-layer pseudo-header
 // (excluding the total length) and the checksum of the payload.
-func (b UDP) CalculateChecksum(partialChecksum uint16, totalLen uint16) uint16 {
+func (b UDP) CalculateChecksum(partialChecksum, totalLen uint16) uint16 {
 	// Add the length portion of the checksum to the pseudo-checksum.
 	tmp := make([]byte, 2)
 	binary.BigEndian.PutUint16(tmp, totalLen)
@@ -336,13 +336,13 @@ func ChecksumCombine(a, b uint16) uint16 {
 // given destination protocol and network address, ignoring the length
 // field. Pseudo-headers are needed by transport layers when calculating
 // their own checksum.
-func PseudoHeaderChecksum(protocol TransportProtocolNumber, srcAddr net.IP, dstAddr net.IP) uint16 {
+func PseudoHeaderChecksum(protocol TransportProtocolNumber, srcAddr, dstAddr net.IP) uint16 {
 	xsum := Checksum([]byte(srcAddr), 0)
 	xsum = Checksum([]byte(dstAddr), xsum)
 	return Checksum([]byte{0, uint8(protocol)}, xsum)
 }
 
-func udp4pkt(packet []byte, dest *net.UDPAddr, src *net.UDPAddr) []byte {
+func udp4pkt(packet []byte, dest, src *net.UDPAddr) []byte {
 	ipLen := IPv4MinimumSize
 	udpLen := UDPMinimumSize
 

internal/dhcpd/v46_windows.go

@@ -7,41 +7,16 @@ import "net"
 type winServer struct {
 }
 
-func (s *winServer) ResetLeases(leases []*Lease) {
+func (s *winServer) ResetLeases(leases []*Lease)            {}
-}
+func (s *winServer) GetLeases(flags int) []Lease            { return nil }
-func (s *winServer) GetLeases(flags int) []Lease {
+func (s *winServer) GetLeasesRef() []*Lease                 { return nil }
-	return nil
+func (s *winServer) AddStaticLease(lease Lease) error       { return nil }
-}
+func (s *winServer) RemoveStaticLease(l Lease) error        { return nil }
-func (s *winServer) GetLeasesRef() []*Lease {
+func (s *winServer) FindMACbyIP(ip net.IP) net.HardwareAddr { return nil }
-	return nil
+func (s *winServer) WriteDiskConfig4(c *V4ServerConf)       {}
-}
+func (s *winServer) WriteDiskConfig6(c *V6ServerConf)       {}
-func (s *winServer) AddStaticLease(lease Lease) error {
+func (s *winServer) Start() error                           { return nil }
-	return nil
+func (s *winServer) Stop()                                  {}
-}
+func (s *winServer) Reset()                                 {}
-func (s *winServer) RemoveStaticLease(l Lease) error {
+func v4Create(conf V4ServerConf) (DHCPServer, error)        { return &winServer{}, nil }
-	return nil
+func v6Create(conf V6ServerConf) (DHCPServer, error)        { return &winServer{}, nil }
-}
-func (s *winServer) FindMACbyIP(ip net.IP) net.HardwareAddr {
-	return nil
-}
-
-func (s *winServer) WriteDiskConfig4(c *V4ServerConf) {
-}
-func (s *winServer) WriteDiskConfig6(c *V6ServerConf) {
-}
-
-func (s *winServer) Start() error {
-	return nil
-}
-func (s *winServer) Stop() {
-}
-func (s *winServer) Reset() {
-}
-
-func v4Create(conf V4ServerConf) (DHCPServer, error) {
-	return &winServer{}, nil
-}
-
-func v6Create(conf V6ServerConf) (DHCPServer, error) {
-	return &winServer{}, nil
-}

internal/dhcpd/v6.go

@@ -537,8 +537,12 @@ func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6.
 	}
 }
 
+type netIface interface {
+	Addrs() ([]net.Addr, error)
+}
+
 // ifaceIPv6Addrs returns the interface's IPv6 addresses.
-func ifaceIPv6Addrs(iface *net.Interface) (ips []net.IP, err error) {
+func ifaceIPv6Addrs(iface netIface) (ips []net.IP, err error) {
 	addrs, err := iface.Addrs()
 	if err != nil {
 		return nil, err
@@ -550,8 +554,11 @@ func ifaceIPv6Addrs(iface *net.Interface) (ips []net.IP, err error) {
 			continue
 		}
 
-		if ip := ipnet.IP.To16(); ip != nil {
+		if ip := ipnet.IP.To4(); ip == nil {
-			ips = append(ips, ip)
+			// Assume that net.(*Interface).Addrs can only return
+			// valid IPv4 and IPv6 addresses.  Since this isn't an
+			// IPv4 address, it must be an IPv6 one.
+			ips = append(ips, ipnet.IP)
 		}
 	}
 

internal/dhcpd/v6_test.go

@@ -6,6 +6,7 @@ import (
 	"net"
 	"testing"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/agherr"
 	"github.com/insomniacslk/dhcp/dhcpv6"
 	"github.com/insomniacslk/dhcp/iana"
 	"github.com/stretchr/testify/assert"
@@ -223,3 +224,57 @@ func TestV6GetDynamicLease(t *testing.T) {
 	assert.True(t, ip6InRange(net.ParseIP("2001::2"), net.ParseIP("2001::2")))
 	assert.True(t, ip6InRange(net.ParseIP("2001::2"), net.ParseIP("2001::3")))
 }
+
+type fakeIface struct {
+	addrs []net.Addr
+	err   error
+}
+
+// Addrs implements the netIface interface for *fakeIface.
+func (iface *fakeIface) Addrs() (addrs []net.Addr, err error) {
+	if iface.err != nil {
+		return nil, iface.err
+	}
+
+	return iface.addrs, nil
+}
+
+func TestIfaceIPv6Addrs(t *testing.T) {
+	ip := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6}
+	ip4 := net.IP{1, 2, 3, 4}
+	addr := &net.IPNet{IP: ip}
+	errTest := agherr.Error("test error")
+
+	testCases := []struct {
+		name    string
+		iface   netIface
+		want    []net.IP
+		wantErr error
+	}{{
+		name:    "success",
+		iface:   &fakeIface{addrs: []net.Addr{addr}, err: nil},
+		want:    []net.IP{ip},
+		wantErr: nil,
+	}, {
+		name: "success_with_ipv4",
+		iface: &fakeIface{
+			addrs: []net.Addr{addr, &net.IPNet{IP: ip4}},
+			err:   nil,
+		},
+		want:    []net.IP{ip},
+		wantErr: nil,
+	}, {
+		name:    "error",
+		iface:   &fakeIface{addrs: []net.Addr{addr}, err: errTest},
+		want:    nil,
+		wantErr: errTest,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, gotErr := ifaceIPv6Addrs(tc.iface)
+			assert.Equal(t, tc.want, got)
+			assert.Equal(t, tc.wantErr, gotErr)
+		})
+	}
+}