Pull request: 4745 Fix DHCP hostnames

Merge in DNS/adguard-home from 4745-fix-dhcp-hostnames to master

Closes #4745.

Squashed commit of the following:

commit fe03c8eda6c8ee35a10eb5f5a8e4d4d0c7373246
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 18:16:16 2022 +0300

    dhcpd: imp code, naming

commit 7a7129268917d99ba16781b7f2e9bfb7ae84ff3e
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 18:10:12 2022 +0300

    dhcpd: add tests

commit bb14a4a62df1eed6492d30f622c3e22da9a6f4be
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 14:58:29 2022 +0300

    dhcpd: imp code, docs

commit 2ada39f994cb9dbb2208d47a588eb72056bb5306
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 14:44:35 2022 +0300

    all: log changes

commit cbd3ed254865921be09376097dac9f5926b2349a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 14:40:54 2022 +0300

    dhcpd: imp option 81

commit 64dabb52560f5edc08f17aadaa43172a5d74463d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 14:10:15 2022 +0300

    dhcpd: fix empty hostname in static lease

commit 0df5d10d0d94863b9bbab28129bcc3436fb71222
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 23 13:34:31 2022 +0300

    dhcpd: report dupl hostnames of static lease

CHANGELOG.md

@@ -30,9 +30,11 @@ and this project adheres to
 
 ### Fixed
 
+- Dynamic leases created with empty hostnames ([#4745]).
 - Unnecessary logging of non-critical statistics errors ([#4850]).
 
 [#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993
+[#4745]: https://github.com/AdguardTeam/AdGuardHome/issues/4745
 [#4850]: https://github.com/AdguardTeam/AdGuardHome/issues/4850
 
 

internal/dhcpd/v4.go

@@ -20,6 +20,7 @@ import (
 	"github.com/go-ping/ping"
 	"github.com/insomniacslk/dhcp/dhcpv4"
 	"github.com/insomniacslk/dhcp/dhcpv4/server4"
+	"golang.org/x/exp/slices"
 
 	//lint:ignore SA1019 See the TODO in go.mod.
 	"github.com/mdlayher/raw"
@@ -93,6 +94,9 @@ func (s *v4Server) validHostnameForClient(cliHostname string, ip net.IP) (hostna
 
 	if hostname == "" {
 		hostname = aghnet.GenerateHostname(ip)
+	} else if s.leaseHosts.Has(hostname) {
+		log.Info("dhcpv4: hostname %q already exists", hostname)
+		hostname = aghnet.GenerateHostname(ip)
 	}
 
 	err = netutil.ValidateDomainName(hostname)
@@ -252,11 +256,11 @@ func (s *v4Server) rmLeaseByIndex(i int) {
 // Remove a dynamic lease with the same properties
 // Return error if a static lease is found
 func (s *v4Server) rmDynamicLease(lease *Lease) (err error) {
-	for i := 0; i < len(s.leases); i++ {
+	for i, l := range s.leases {
-		l := s.leases[i]
+		isStatic := l.IsStatic()
 
-		if bytes.Equal(l.HWAddr, lease.HWAddr) {
+		if bytes.Equal(l.HWAddr, lease.HWAddr) || l.IP.Equal(lease.IP) {
-			if l.IsStatic() {
+			if isStatic {
 				return errors.Error("static lease already exists")
 			}
 
@@ -268,20 +272,7 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) {
 			l = s.leases[i]
 		}
 
-		if l.IP.Equal(lease.IP) {
+		if !isStatic && l.Hostname == lease.Hostname {
-			if l.IsStatic() {
-				return errors.Error("static lease already exists")
-			}
-
-			s.rmLeaseByIndex(i)
-			if i == len(s.leases) {
-				break
-			}
-
-			l = s.leases[i]
-		}
-
-		if l.Hostname == lease.Hostname {
 			l.Hostname = ""
 		}
 	}
@@ -289,6 +280,10 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) {
 	return nil
 }
 
+// ErrDupHostname is returned by addLease when the added lease has a not empty
+// non-unique hostname.
+const ErrDupHostname = errors.Error("hostname is not unique")
+
 // addLease adds a dynamic or static lease.
 func (s *v4Server) addLease(l *Lease) (err error) {
 	r := s.conf.ipRange
@@ -304,13 +299,17 @@ func (s *v4Server) addLease(l *Lease) (err error) {
 		return fmt.Errorf("lease %s (%s) out of range, not adding", l.IP, l.HWAddr)
 	}
 
-	s.leases = append(s.leases, l)
-	s.leasedOffsets.set(offset, true)
-
 	if l.Hostname != "" {
+		if s.leaseHosts.Has(l.Hostname) {
+			return ErrDupHostname
+		}
+
 		s.leaseHosts.Add(l.Hostname)
 	}
 
+	s.leases = append(s.leases, l)
+	s.leasedOffsets.set(offset, true)
+
 	return nil
 }
 
@@ -365,10 +364,11 @@ func (s *v4Server) AddStaticLease(l *Lease) (err error) {
 			return fmt.Errorf("validating hostname: %w", err)
 		}
 
-		// Don't check for hostname uniqueness, since we try to emulate
+		// Don't check for hostname uniqueness, since we try to emulate dnsmasq
-		// dnsmasq here, which means that rmDynamicLease below will
+		// here, which means that rmDynamicLease below will simply empty the
-		// simply empty the hostname of the dynamic lease if there even
+		// hostname of the dynamic lease if there even is one.  In case a static
-		// is one.
+		// lease with the same name already exists, addLease will return an
+		// error and the lease won't be added.
 
 		l.Hostname = hostname
 	}
@@ -523,11 +523,7 @@ func (s *v4Server) findExpiredLease() int {
 // reserveLease reserves a lease for a client by its MAC-address.  It returns
 // nil if it couldn't allocate a new lease.
 func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease, err error) {
-	l = &Lease{
+	l = &Lease{HWAddr: slices.Clone(mac)}
-		HWAddr: make([]byte, len(mac)),
-	}
-
-	copy(l.HWAddr, mac)
 
 	l.IP = s.nextIP()
 	if l.IP == nil {
@@ -620,33 +616,25 @@ func (s *v4Server) processDiscover(req, resp *dhcpv4.DHCPv4) (l *Lease, err erro
 	return l, nil
 }
 
-type optFQDN struct {
+// OptionFQDN returns a DHCPv4 option for sending the FQDN to the client
-	name string
+// requested another hostname.
-}
+//
+// See https://datatracker.ietf.org/doc/html/rfc4702.
+func OptionFQDN(fqdn string) (opt dhcpv4.Option) {
+	optData := []byte{
+		// Set only S and O DHCP client FQDN option flags.
+		//
+		// See https://datatracker.ietf.org/doc/html/rfc4702#section-2.1.
+		1<<0 | 1<<1,
+		// The RCODE fields should be set to 0xFF in the server responses.
+		//
+		// See https://datatracker.ietf.org/doc/html/rfc4702#section-2.2.
+		0xFF,
+		0xFF,
+	}
+	optData = append(optData, fqdn...)
 
-func (o *optFQDN) String() string {
+	return dhcpv4.OptGeneric(dhcpv4.OptionFQDN, optData)
-	return "optFQDN"
-}
-
-// flags[1]
-// A-RR[1]
-// PTR-RR[1]
-// name[]
-func (o *optFQDN) ToBytes() []byte {
-	b := make([]byte, 3+len(o.name))
-	i := 0
-
-	b[i] = 0x03 // f_server_overrides | f_server
-	i++
-
-	b[i] = 255 // A-RR
-	i++
-
-	b[i] = 255 // PTR-RR
-	i++
-
-	copy(b[i:], []byte(o.name))
-	return b
 }
 
 // checkLease checks if the pair of mac and ip is already leased.  The mismatch
@@ -679,6 +667,8 @@ func (s *v4Server) checkLease(mac net.HardwareAddr, ip net.IP) (lease *Lease, mi
 // processRequest is the handler for the DHCP Request request.
 func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) {
 	mac := req.ClientHWAddr
+	// TODO(e.burkov):  The IP address can only be requested in DHCPDISCOVER
+	// message.
 	reqIP := req.RequestedIPAddress()
 	if reqIP == nil {
 		reqIP = req.ClientIPAddr
@@ -711,24 +701,17 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needs
 	if !lease.IsStatic() {
 		cliHostname := req.HostName()
 		hostname := s.validHostnameForClient(cliHostname, reqIP)
-		if hostname != lease.Hostname && s.leaseHosts.Has(hostname) {
+		if lease.Hostname != hostname {
-			log.Info("dhcpv4: hostname %q already exists", hostname)
-			lease.Hostname = ""
-		} else {
 			lease.Hostname = hostname
+			resp.UpdateOption(dhcpv4.OptHostName(hostname))
 		}
 
 		s.commitLease(lease)
 	} else if lease.Hostname != "" {
-		o := &optFQDN{
+		// TODO(e.burkov):  This option is used to update the server's DNS
-			name: lease.Hostname,
+		// mapping.  The option should only be answered when it has been
-		}
+		// requested.
-		fqdn := dhcpv4.Option{
+		resp.UpdateOption(OptionFQDN(lease.Hostname))
-			Code:  dhcpv4.OptionFQDN,
-			Value: o,
-		}
-
-		resp.UpdateOption(fqdn)
 	}
 
 	resp.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeAck))
@@ -851,7 +834,7 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int {
 
 	// TODO(a.garipov): Refactor this into handlers.
 	var l *Lease
-	switch req.MessageType() {
+	switch mt := req.MessageType(); mt {
 	case dhcpv4.MessageTypeDiscover:
 		l, err = s.processDiscover(req, resp)
 		if err != nil {

internal/dhcpd/v4_test.go

@@ -8,7 +8,9 @@ import (
 	"net"
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/golibs/stringutil"
 	"github.com/AdguardTeam/golibs/testutil"
 	"github.com/insomniacslk/dhcp/dhcpv4"
@@ -23,6 +25,7 @@ var (
 	DefaultRangeStart = net.IP{192, 168, 10, 100}
 	DefaultRangeEnd   = net.IP{192, 168, 10, 200}
 	DefaultGatewayIP  = net.IP{192, 168, 10, 1}
+	DefaultSelfIP     = net.IP{192, 168, 10, 2}
 	DefaultSubnetMask = net.IP{255, 255, 255, 0}
 )
 
@@ -39,6 +42,7 @@ func defaultV4ServerConf() (conf V4ServerConf) {
 		GatewayIP:  DefaultGatewayIP,
 		SubnetMask: DefaultSubnetMask,
 		notify:     notify4,
+		dnsIPAddrs: []net.IP{DefaultSelfIP},
 	}
 }
 
@@ -54,6 +58,148 @@ func defaultSrv(t *testing.T) (s DHCPServer) {
 	return s
 }
 
+func TestV4Server_leasing(t *testing.T) {
+	const (
+		staticName  = "static-client"
+		anotherName = "another-client"
+	)
+
+	staticIP := net.IP{192, 168, 10, 10}
+	anotherIP := DefaultRangeStart
+	staticMAC := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}
+	anotherMAC := net.HardwareAddr{0xBB, 0xBB, 0xBB, 0xBB, 0xBB, 0xBB}
+
+	s := defaultSrv(t)
+
+	t.Run("add_static", func(t *testing.T) {
+		err := s.AddStaticLease(&Lease{
+			Expiry:   time.Unix(leaseExpireStatic, 0),
+			Hostname: staticName,
+			HWAddr:   staticMAC,
+			IP:       staticIP,
+		})
+		require.NoError(t, err)
+
+		t.Run("same_name", func(t *testing.T) {
+			err = s.AddStaticLease(&Lease{
+				Expiry:   time.Unix(leaseExpireStatic, 0),
+				Hostname: staticName,
+				HWAddr:   anotherMAC,
+				IP:       anotherIP,
+			})
+			assert.ErrorIs(t, err, ErrDupHostname)
+		})
+
+		t.Run("same_mac", func(t *testing.T) {
+			wantErrMsg := "dhcpv4: adding static lease: removing " +
+				"dynamic leases for " + anotherIP.String() +
+				" (" + staticMAC.String() + "): static lease already exists"
+
+			err = s.AddStaticLease(&Lease{
+				Expiry:   time.Unix(leaseExpireStatic, 0),
+				Hostname: anotherName,
+				HWAddr:   staticMAC,
+				IP:       anotherIP,
+			})
+			testutil.AssertErrorMsg(t, wantErrMsg, err)
+		})
+
+		t.Run("same_ip", func(t *testing.T) {
+			wantErrMsg := "dhcpv4: adding static lease: removing " +
+				"dynamic leases for " + staticIP.String() +
+				" (" + anotherMAC.String() + "): static lease already exists"
+
+			err = s.AddStaticLease(&Lease{
+				Expiry:   time.Unix(leaseExpireStatic, 0),
+				Hostname: anotherName,
+				HWAddr:   anotherMAC,
+				IP:       staticIP,
+			})
+			testutil.AssertErrorMsg(t, wantErrMsg, err)
+		})
+	})
+
+	t.Run("add_dynamic", func(t *testing.T) {
+		s4, ok := s.(*v4Server)
+		require.True(t, ok)
+
+		discoverAnOffer := func(
+			t *testing.T,
+			name string,
+			ip net.IP,
+			mac net.HardwareAddr,
+		) (resp *dhcpv4.DHCPv4) {
+			testutil.CleanupAndRequireSuccess(t, func() (err error) {
+				return s.ResetLeases(s.GetLeases(LeasesStatic))
+			})
+
+			req, err := dhcpv4.NewDiscovery(
+				mac,
+				dhcpv4.WithOption(dhcpv4.OptHostName(name)),
+				dhcpv4.WithOption(dhcpv4.OptRequestedIPAddress(ip)),
+				dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte{1, 2, 3, 4, 5, 6, 8})),
+				dhcpv4.WithGatewayIP(DefaultGatewayIP),
+			)
+			require.NoError(t, err)
+
+			resp = &dhcpv4.DHCPv4{}
+			res := s4.process(req, resp)
+			require.Positive(t, res)
+			require.Equal(t, dhcpv4.MessageTypeOffer, resp.MessageType())
+
+			resp.ClientHWAddr = mac
+
+			return resp
+		}
+
+		t.Run("same_name", func(t *testing.T) {
+			resp := discoverAnOffer(t, staticName, anotherIP, anotherMAC)
+
+			req, err := dhcpv4.NewRequestFromOffer(resp, dhcpv4.WithOption(
+				dhcpv4.OptHostName(staticName),
+			))
+			require.NoError(t, err)
+
+			res := s4.process(req, resp)
+			require.Positive(t, res)
+
+			assert.Equal(t, aghnet.GenerateHostname(resp.YourIPAddr), resp.HostName())
+		})
+
+		t.Run("same_mac", func(t *testing.T) {
+			resp := discoverAnOffer(t, anotherName, anotherIP, staticMAC)
+
+			req, err := dhcpv4.NewRequestFromOffer(resp, dhcpv4.WithOption(
+				dhcpv4.OptHostName(anotherName),
+			))
+			require.NoError(t, err)
+
+			res := s4.process(req, resp)
+			require.Positive(t, res)
+
+			fqdnOptData := resp.Options.Get(dhcpv4.OptionFQDN)
+			require.Len(t, fqdnOptData, 3+len(staticName))
+			assert.Equal(t, []uint8(staticName), fqdnOptData[3:])
+
+			assert.Equal(t, staticIP, resp.YourIPAddr)
+		})
+
+		t.Run("same_ip", func(t *testing.T) {
+			resp := discoverAnOffer(t, anotherName, staticIP, anotherMAC)
+
+			req, err := dhcpv4.NewRequestFromOffer(resp, dhcpv4.WithOption(
+				dhcpv4.OptHostName(anotherName),
+			))
+			require.NoError(t, err)
+
+			res := s4.process(req, resp)
+			require.Positive(t, res)
+
+			assert.NotEqual(t, staticIP, resp.YourIPAddr)
+		})
+	})
+}
+
 func TestV4Server_AddRemove_static(t *testing.T) {
 	s := defaultSrv(t)