From 36c7735b855f4388d0cf36fb2f26f1979809c217 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Wed, 25 Nov 2020 14:26:26 +0300 Subject: [PATCH] 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 Date: Wed Nov 25 14:03:24 2020 +0300 dhcpd: fix loop exit condition commit 32b4b946bfa30159326dc295fa1a2607b78172af Author: Ainar Garipov Date: Wed Nov 25 13:26:50 2020 +0300 dhcpd: fix interface ipv6 check --- CHANGELOG.md | 7 ++++ internal/dhcpd/check_other_dhcp.go | 10 ++--- internal/dhcpd/db.go | 1 - internal/dhcpd/nclient4/client_test.go | 8 ++-- internal/dhcpd/nclient4/conn_unix.go | 18 ++++----- internal/dhcpd/nclient4/ipv4.go | 6 +-- internal/dhcpd/v46_windows.go | 51 ++++++------------------ internal/dhcpd/v6.go | 13 ++++-- internal/dhcpd/v6_test.go | 55 ++++++++++++++++++++++++++ 9 files changed, 103 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f12d2670..798bf950 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dhcpd/check_other_dhcp.go b/internal/dhcpd/check_other_dhcp.go index aba9e446..e77a7801 100644 --- a/internal/dhcpd/check_other_dhcp.go +++ b/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 } } diff --git a/internal/dhcpd/db.go b/internal/dhcpd/db.go index b01f9635..2618f12e 100644 --- a/internal/dhcpd/db.go +++ b/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) diff --git a/internal/dhcpd/nclient4/client_test.go b/internal/dhcpd/nclient4/client_test.go index 353a9ed7..99f99640 100644 --- a/internal/dhcpd/nclient4/client_test.go +++ b/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}), }, }, diff --git a/internal/dhcpd/nclient4/conn_unix.go b/internal/dhcpd/nclient4/conn_unix.go index 51ec98cb..39009d69 100644 --- a/internal/dhcpd/nclient4/conn_unix.go +++ b/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. - // - // Any UDP packet sent to this address is broadcast on the subnet. - BroadcastMac = net.HardwareAddr([]byte{255, 255, 255, 255, 255, 255}) -) +// BroadcastMac is the broadcast MAC address. +// +// Any UDP packet sent to this address is broadcast on the subnet. +var 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 = errors.New("must supply UDPAddr") -) +// ErrUDPAddrIsRequired is an error used when a passed argument is not of type "*net.UDPAddr". +var 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 } diff --git a/internal/dhcpd/nclient4/ipv4.go b/internal/dhcpd/nclient4/ipv4.go index 5d961852..50a2d684 100644 --- a/internal/dhcpd/nclient4/ipv4.go +++ b/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 diff --git a/internal/dhcpd/v46_windows.go b/internal/dhcpd/v46_windows.go index 152899b9..ebae25af 100644 --- a/internal/dhcpd/v46_windows.go +++ b/internal/dhcpd/v46_windows.go @@ -7,41 +7,16 @@ import "net" type winServer struct { } -func (s *winServer) ResetLeases(leases []*Lease) { -} -func (s *winServer) GetLeases(flags int) []Lease { - return nil -} -func (s *winServer) GetLeasesRef() []*Lease { - return nil -} -func (s *winServer) AddStaticLease(lease Lease) error { - return nil -} -func (s *winServer) RemoveStaticLease(l Lease) error { - return 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 -} +func (s *winServer) ResetLeases(leases []*Lease) {} +func (s *winServer) GetLeases(flags int) []Lease { return nil } +func (s *winServer) GetLeasesRef() []*Lease { return nil } +func (s *winServer) AddStaticLease(lease Lease) error { return nil } +func (s *winServer) RemoveStaticLease(l Lease) error { return 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 } diff --git a/internal/dhcpd/v6.go b/internal/dhcpd/v6.go index b24be499..f1ec57b3 100644 --- a/internal/dhcpd/v6.go +++ b/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 { - ips = append(ips, ip) + if ip := ipnet.IP.To4(); ip == nil { + // 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) } } diff --git a/internal/dhcpd/v6_test.go b/internal/dhcpd/v6_test.go index 7d7dd678..d51c695c 100644 --- a/internal/dhcpd/v6_test.go +++ b/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) + }) + } +}