Pull request: dhcpd: refactor validations

Updates #3107.
Updates #3127.

Squashed commit of the following:

commit ad64472566ecd1b0864212cd2644a57439aceb85
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu May 13 14:45:21 2021 +0300

    dhcpd: refactor validations

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to
 
 ### Fixed
 
+- DHCP leases validation ([#3107], [#3127]).
 - Local PTR request recursion in Docker containers ([#3064]).
 - Ignoring client-specific filtering settings when filtering is disabled in
   general settings ([#2875]).
@@ -26,7 +27,9 @@ and this project adheres to
 
 [#2875]: https://github.com/AdguardTeam/AdGuardHome/issues/2875
 [#3064]: https://github.com/AdguardTeam/AdGuardHome/issues/3064
+[#3107]: https://github.com/AdguardTeam/AdGuardHome/issues/3107
 [#3115]: https://github.com/AdguardTeam/AdGuardHome/issues/3115
+[#3127]: https://github.com/AdguardTeam/AdGuardHome/issues/3127
 
 
 

internal/dhcpd/v4.go

@@ -73,42 +73,26 @@ func normalizeHostname(hostname string) (norm string, err error) {
 	return norm, nil
 }
 
-// validHostnameForClient accepts the hostname sent by the client and returns
+// validHostnameForClient accepts the hostname sent by the client and its IP and
-// either a normalized version of that hostname or a new hostname generated from
+// returns either a normalized version of that hostname, or a new hostname
-// the client's IP address.  If this new hostname is different from the provided
+// generated from the IP address, or an empty string.
-// previous hostname, additional uniqueness check is performed.
+func (s *v4Server) validHostnameForClient(cliHostname string, ip net.IP) (hostname string) {
-//
+	hostname, err := normalizeHostname(cliHostname)
-// hostname is always a non-empty valid hostname.  If err is not nil, it
+	if err != nil {
-// describes the issues encountered when normalizing cliHostname.
+		log.Info("dhcpv4: %s", err)
-func (s *v4Server) validHostnameForClient(
-	cliHostname string,
-	prevHostname string,
-	ip net.IP,
-) (hostname string, err error) {
-	hostname, err = normalizeHostname(cliHostname)
-	if err == nil && hostname != "" {
-		err = aghnet.ValidateDomainName(hostname)
-		if err != nil {
-			// Go on and assign a hostname made from the IP below,
-			// returning the error that we've got.
-			hostname = ""
-		} else if hostname != prevHostname && s.leaseHosts.Has(hostname) {
-			// Go on and assign a unique hostname made from the IP
-			// below, returning the error about uniqueness.
-			err = agherr.Error("hostname exists")
-			hostname = ""
-		}
 	}
 
 	if hostname == "" {
 		hostname = aghnet.GenerateHostname(ip)
 	}
 
-	if hostname != cliHostname {
+	err = aghnet.ValidateDomainName(hostname)
-		log.Info("dhcpv4: normalized hostname %q into %q", cliHostname, hostname)
+	if err != nil {
+		log.Info("dhcpv4: %s", err)
+		hostname = ""
 	}
 
-	return hostname, err
+	return hostname
 }
 
 // ResetLeases - reset leases
@@ -124,15 +108,7 @@ func (s *v4Server) ResetLeases(leases []*Lease) {
 	s.leases = nil
 
 	for _, l := range leases {
-		l.Hostname, err = s.validHostnameForClient(l.Hostname, l.Hostname, l.IP)
+		l.Hostname = s.validHostnameForClient(l.Hostname, l.IP)
-		if err != nil {
-			log.Info(
-				"dhcpv4: warning: previous hostname %q is invalid: %s",
-				l.Hostname,
-				err,
-			)
-		}
-
 		err = s.addLease(l)
 		if err != nil {
 			// TODO(a.garipov): Wrap and bubble up the error.
@@ -287,6 +263,15 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) {
 			}
 
 			s.rmLeaseByIndex(i)
+			if i == len(s.leases) {
+				break
+			}
+
+			l = s.leases[i]
+		}
+
+		if l.Hostname == lease.Hostname {
+			l.Hostname = ""
 		}
 	}
 
@@ -352,22 +337,25 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) {
 		return err
 	}
 
-	var hostname string
+	if l.Hostname != "" {
-	hostname, err = normalizeHostname(l.Hostname)
+		var hostname string
-	if err != nil {
+		hostname, err = normalizeHostname(l.Hostname)
-		return err
+		if err != nil {
-	}
+			return err
+		}
 
-	err = aghnet.ValidateDomainName(hostname)
+		err = aghnet.ValidateDomainName(hostname)
-	if err != nil {
+		if err != nil {
-		return fmt.Errorf("validating hostname: %w", err)
+			return fmt.Errorf("validating hostname: %w", err)
-	}
+		}
 
-	if s.leaseHosts.Has(hostname) {
+		// Don't check for hostname uniqueness, since we try to emulate
-		return agherr.Error("hostname exists")
+		// dnsmasq here, which means that rmDynamicLease below will
-	}
+		// simply empty the hostname of the dynamic lease if there even
+		// is one.
 
-	l.Hostname = hostname
+		l.Hostname = hostname
+	}
 
 	// Perform the following actions in an anonymous function to make sure
 	// that the lock gets unlocked before the notification step.
@@ -553,7 +541,10 @@ func (s *v4Server) commitLease(l *Lease) {
 		defer s.leasesLock.Unlock()
 
 		s.conf.notify(LeaseChangedDBStore)
-		s.leaseHosts.Add(l.Hostname)
+
+		if l.Hostname != "" {
+			s.leaseHosts.Add(l.Hostname)
+		}
 	}()
 
 	s.conf.notify(LeaseChangedAdded)
@@ -647,7 +638,7 @@ func (o *optFQDN) ToBytes() []byte {
 	return b
 }
 
-// processDiscover is the handler for the DHCP Request request.
+// processRequest is the handler for the DHCP Request request.
 func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) {
 	mac := req.ClientHWAddr
 	err := aghnet.ValidateHardwareAddress(mac)
@@ -662,13 +653,13 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needs
 
 	sid := req.ServerIdentifier()
 	if len(sid) != 0 && !sid.Equal(s.conf.dnsIPAddrs[0]) {
-		log.Debug("dhcpv4: bad OptionServerIdentifier in request message for %s", mac)
+		log.Debug("dhcpv4: bad OptionServerIdentifier in req msg for %s", mac)
 
 		return nil, false
 	}
 
 	if ip4 := reqIP.To4(); ip4 == nil {
-		log.Debug("dhcpv4: bad OptionRequestedIPAddress in request message for %s", mac)
+		log.Debug("dhcpv4: bad OptionRequestedIPAddress in req msg for %s", mac)
 
 		return nil, false
 	}
@@ -685,15 +676,17 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needs
 
 			if l.IP.Equal(reqIP) {
 				lease = l
-			} else {
+
-				log.Debug(
+				return
-					`dhcpv4: mismatched OptionRequestedIPAddress `+
-						`in request message for %s`,
-					mac,
-				)
-				mismatch = true
 			}
 
+			log.Debug(
+				`dhcpv4: mismatched OptionRequestedIPAddress in req msg for %s`,
+				mac,
+			)
+
+			mismatch = true
+
 			return
 		}
 	}()
@@ -709,13 +702,12 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needs
 
 	if !lease.IsStatic() {
 		cliHostname := req.HostName()
-		lease.Hostname, err = s.validHostnameForClient(cliHostname, lease.Hostname, reqIP)
+		hostname := s.validHostnameForClient(cliHostname, reqIP)
-		if err != nil {
+		if hostname != lease.Hostname && s.leaseHosts.Has(hostname) {
-			log.Info(
+			log.Info("dhcpv4: hostname %q already exists", hostname)
-				"dhcpv4: warning: client hostname %q is invalid: %s",
+			lease.Hostname = ""
-				cliHostname,
+		} else {
-				err,
+			lease.Hostname = hostname
-			)
 		}
 
 		s.commitLease(lease)