Pull request: all: imp dhcp client hostname normalization

Updates #2952.
Updates #2978.

Squashed commit of the following:

commit 20e379b94ccf8140fd9056429315945c17f711a5
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 19 15:58:37 2021 +0300

    all: imp naming

commit ed300e0563fa37e161406a97991b26a89e23903a
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 19 15:43:09 2021 +0300

    all: imp dhcp client hostname normalization

CHANGELOG.md

@@ -33,7 +33,7 @@ and this project adheres to
 ### Changed
 
 - Quality of logging ([#2954]).
-- Normalization of hostnames with spaces sent by DHCP clients ([#2945]).
+- Normalization of hostnames sent by DHCP clients ([#2945], [#2952]).
 - The access to the private hosts is now forbidden for users from external
   networks ([#2889]).
 - The reverse lookup for local addresses is now performed via local resolvers
@@ -82,6 +82,7 @@ and this project adheres to
 [#2934]: https://github.com/AdguardTeam/AdGuardHome/issues/2934
 [#2945]: https://github.com/AdguardTeam/AdGuardHome/issues/2945
 [#2947]: https://github.com/AdguardTeam/AdGuardHome/issues/2947
+[#2952]: https://github.com/AdguardTeam/AdGuardHome/issues/2952
 [#2954]: https://github.com/AdguardTeam/AdGuardHome/issues/2954
 [#2961]: https://github.com/AdguardTeam/AdGuardHome/issues/2961
 

internal/aghnet/addr.go

@@ -10,6 +10,19 @@ import (
 	"golang.org/x/net/idna"
 )
 
+// IsValidHostOuterRune returns true if r is a valid initial or final rune for
+// a hostname label.
+func IsValidHostOuterRune(r rune) (ok bool) {
+	return (r >= 'a' && r <= 'z') ||
+		(r >= 'A' && r <= 'Z') ||
+		(r >= '0' && r <= '9')
+}
+
+// isValidHostRune returns true if r is a valid rune for a hostname label.
+func isValidHostRune(r rune) (ok bool) {
+	return r == '-' || IsValidHostOuterRune(r)
+}
+
 // ValidateHardwareAddress returns an error if hwa is not a valid EUI-48,
 // EUI-64, or 20-octet InfiniBand link-layer address.
 func ValidateHardwareAddress(hwa net.HardwareAddr) (err error) {
@@ -37,38 +50,32 @@ const maxDomainNameLen = 253
 
 const invalidCharMsg = "invalid char %q at index %d in %q"
 
-// isValidHostFirstRune returns true if r is a valid first rune for a hostname
-// label.
-func isValidHostFirstRune(r rune) (ok bool) {
-	return (r >= 'a' && r <= 'z') ||
-		(r >= 'A' && r <= 'Z') ||
-		(r >= '0' && r <= '9')
-}
-
-// isValidHostRune returns true if r is a valid rune for a hostname label.
-func isValidHostRune(r rune) (ok bool) {
-	return r == '-' || isValidHostFirstRune(r)
-}
-
 // ValidateDomainNameLabel returns an error if label is not a valid label of
 // a domain name.
 func ValidateDomainNameLabel(label string) (err error) {
-	if len(label) > maxDomainLabelLen {
+	l := len(label)
+	if l > maxDomainLabelLen {
 		return fmt.Errorf("%q is too long, max: %d", label, maxDomainLabelLen)
-	} else if len(label) == 0 {
+	} else if l == 0 {
 		return agherr.Error("label is empty")
 	}
 
-	if r := label[0]; !isValidHostFirstRune(rune(r)) {
+	if r := label[0]; !IsValidHostOuterRune(rune(r)) {
 		return fmt.Errorf(invalidCharMsg, r, 0, label)
+	} else if l == 1 {
+		return nil
 	}
 
-	for i, r := range label[1:] {
+	for i, r := range label[1 : l-1] {
 		if !isValidHostRune(r) {
 			return fmt.Errorf(invalidCharMsg, r, i+1, label)
 		}
 	}
 
+	if r := label[l-1]; !IsValidHostOuterRune(rune(r)) {
+		return fmt.Errorf(invalidCharMsg, r, l-1, label)
+	}
+
 	return nil
 }
 
@@ -86,7 +93,9 @@ func ValidateDomainName(name string) (err error) {
 	}
 
 	l := len(name)
-	if l == 0 || l > maxDomainNameLen {
+	if l == 0 {
+		return agherr.Error("domain name is empty")
+	} else if l > maxDomainNameLen {
 		return fmt.Errorf("%q is too long, max: %d", name, maxDomainNameLen)
 	}
 
@@ -138,7 +147,7 @@ func generateIPv6Hostname(ipv6 net.IP) (hostname string) {
 	return string(hnData)
 }
 
-// GenerateHostName generates the hostname from ip.  In case of using IPv4 the
+// GenerateHostname generates the hostname from ip.  In case of using IPv4 the
 // result should be like:
 //
 //   192-168-10-1
@@ -147,7 +156,7 @@ func generateIPv6Hostname(ipv6 net.IP) (hostname string) {
 //
 //   ff80-f076-0000-0000-0000-0000-0000-0010
 //
-func GenerateHostName(ip net.IP) (hostname string) {
+func GenerateHostname(ip net.IP) (hostname string) {
 	if ipv4 := ip.To4(); ipv4 != nil {
 		return generateIPv4Hostname(ipv4)
 	} else if ipv6 := ip.To16(); ipv6 != nil {

internal/aghnet/addr_test.go

@@ -88,6 +88,14 @@ func TestValidateDomainName(t *testing.T) {
 		name:       "success_idna",
 		in:         "пример.рф",
 		wantErrMsg: "",
+	}, {
+		name:       "success_one",
+		in:         "e",
+		wantErrMsg: "",
+	}, {
+		name:       "empty",
+		in:         "",
+		wantErrMsg: `domain name is empty`,
 	}, {
 		name: "bad_symbol",
 		in:   "!!!",
@@ -111,6 +119,11 @@ func TestValidateDomainName(t *testing.T) {
 		in:   "example.-aa.com",
 		wantErrMsg: `invalid domain name label at index 1:` +
 			` invalid char '-' at index 0 in "-aa"`,
+	}, {
+		name: "bad_label_last_symbol",
+		in:   "example-.aa.com",
+		wantErrMsg: `invalid domain name label at index 0:` +
+			` invalid char '-' at index 7 in "example-"`,
 	}, {
 		name: "bad_label_symbol",
 		in:   "example.a!!!.com",
@@ -166,7 +179,7 @@ func TestGenerateHostName(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			hostname := GenerateHostName(tc.ip)
+			hostname := GenerateHostname(tc.ip)
 			assert.Equal(t, tc.want, hostname)
 		})
 	}

internal/dhcpd/v4.go

@@ -558,34 +558,45 @@ func (o *optFQDN) ToBytes() []byte {
 	return b
 }
 
-// normalizeHostname normalizes and validates a hostname sent by the client.
+// normalizeHostname normalizes a hostname sent by the client.
-//
+func normalizeHostname(name string) (norm string, err error) {
-// TODO(a.garipov): Add client hostname uniqueness validations and rename the
-// method to validateHostname.
-func (s *v4Server) normalizeHostname(name string) (norm string, err error) {
 	if name == "" {
 		return "", nil
 	}
 
-	// Some devices send hostnames with spaces, but we still want to accept
+	parts := strings.FieldsFunc(name, func(c rune) (ok bool) {
-	// them, so replace them with dashes and issue a warning.
+		return c != '.' && !aghnet.IsValidHostOuterRune(c)
-	//
+	})
-	// See https://github.com/AdguardTeam/AdGuardHome/issues/2946.
+
-	norm = strings.ReplaceAll(name, " ", "-")
+	if len(parts) == 0 {
-	state := "non-normalized"
+		return "", fmt.Errorf("normalizing hostname %q: no valid parts", name)
-	if name != norm {
-		log.Debug("dhcpv4: normalized hostname %q into %q", name, norm)
-		state = "normalized"
 	}
 
-	err = aghnet.ValidateDomainName(norm)
+	norm = strings.Join(parts, "-")
-	if err != nil {
+	norm = strings.TrimSuffix(norm, "-")
-		return "", fmt.Errorf("validating %s hostname: %w", state, err)
-	}
 
 	return norm, nil
 }
 
+// validateHostname validates a hostname sent by the client.
+func (s *v4Server) validateHostname(name string) (err error) {
+	if name == "" {
+		return nil
+	}
+
+	err = aghnet.ValidateDomainName(name)
+	if err != nil {
+		return fmt.Errorf("validating hostname: %w", err)
+	}
+
+	// TODO(a.garipov): Add client hostname uniqueness validation either
+	// here or into method processRequest.  This is not as easy as it might
+	// look like, because the process of adding and releasing a lease is
+	// currently non-straightforward.
+
+	return nil
+}
+
 // Process Request request and return lease
 // Return false if we don't need to reply
 func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, ok bool) {
@@ -634,16 +645,29 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, ok bo
 	}
 
 	if !lease.IsStatic() {
+		cliHostname := req.HostName()
+
 		var hostname string
-		hostname, err = s.normalizeHostname(req.HostName())
+		hostname, err = normalizeHostname(cliHostname)
 		if err != nil {
 			log.Error("dhcpv4: cannot normalize hostname for %s: %s", mac, err)
 
-			return nil, false
+			// Go on and assign a hostname made from the IP.
+		}
+
+		if hostname != "" && cliHostname != hostname {
+			log.Debug("dhcpv4: normalized hostname %q into %q", cliHostname, hostname)
+		}
+
+		err = s.validateHostname(hostname)
+		if err != nil {
+			log.Error("dhcpv4: validating hostname for %s: %s", mac, err)
+
+			// Go on and assign a hostname made from the IP.
 		}
 
 		if hostname == "" {
-			hostname = aghnet.GenerateHostName(reqIP)
+			hostname = aghnet.GenerateHostname(reqIP)
 		}
 
 		lease.Hostname = hostname

internal/dhcpd/v4_test.go

@@ -290,7 +290,7 @@ func TestV4DynamicLease_Get(t *testing.T) {
 	})
 }
 
-func TestV4Server_normalizeHostname(t *testing.T) {
+func TestNormalizeHostname(t *testing.T) {
 	testCases := []struct {
 		name       string
 		hostname   string
@@ -311,25 +311,36 @@ func TestV4Server_normalizeHostname(t *testing.T) {
 		hostname:   "my device 01",
 		wantErrMsg: "",
 		want:       "my-device-01",
+	}, {
+		name:       "success_underscores",
+		hostname:   "my_device_01",
+		wantErrMsg: "",
+		want:       "my-device-01",
+	}, {
+		name:       "error_part",
+		hostname:   "device !!!",
+		wantErrMsg: "",
+		want:       "device",
+	}, {
+		name:       "error_part_spaces",
+		hostname:   "device ! ! !",
+		wantErrMsg: "",
+		want:       "device",
 	}, {
 		name:       "error",
 		hostname:   "!!!",
-		wantErrMsg: `validating non-normalized hostname: ` +
+		wantErrMsg: `normalizing hostname "!!!": no valid parts`,
-			`invalid domain name label at index 0: ` +
-			`invalid char '!' at index 0 in "!!!"`,
 		want:       "",
 	}, {
 		name:       "error_spaces",
 		hostname:   "! ! !",
-		wantErrMsg: `validating normalized hostname: ` +
+		wantErrMsg: `normalizing hostname "! ! !": no valid parts`,
-			`invalid domain name label at index 0: ` +
-			`invalid char '!' at index 0 in "!-!-!"`,
 		want:       "",
 	}}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			got, err := (&v4Server{}).normalizeHostname(tc.hostname)
+			got, err := normalizeHostname(tc.hostname)
 			if tc.wantErrMsg == "" {
 				assert.NoError(t, err)
 			} else {

internal/dnsforward/dns.go

@@ -140,20 +140,6 @@ func processInitial(ctx *dnsContext) (rc resultCode) {
 	return resultCodeSuccess
 }
 
-// Return TRUE if host names doesn't contain disallowed characters
-func isHostnameOK(hostname string) bool {
-	for _, c := range hostname {
-		if !((c >= 'a' && c <= 'z') ||
-			(c >= 'A' && c <= 'Z') ||
-			(c >= '0' && c <= '9') ||
-			c == '.' || c == '-') {
-			log.Debug("dns: skipping invalid hostname %s from DHCP", hostname)
-			return false
-		}
-	}
-	return true
-}
-
 func (s *Server) setTableHostToIP(t hostToIPTable) {
 	s.tableHostToIPLock.Lock()
 	defer s.tableHostToIPLock.Unlock()
@@ -169,6 +155,8 @@ func (s *Server) setTableIPToHost(t ipToHostTable) {
 }
 
 func (s *Server) onDHCPLeaseChanged(flags int) {
+	var err error
+
 	add := true
 	switch flags {
 	case dhcpd.LeaseChangedAdded,
@@ -190,8 +178,16 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
 		ll := s.dhcpServer.Leases(dhcpd.LeasesAll)
 
 		for _, l := range ll {
-			if len(l.Hostname) == 0 || !isHostnameOK(l.Hostname) {
+			// TODO(a.garipov): Remove this after we're finished
-				continue
+			// with the client hostname validations in the DHCP
+			// server code.
+			err = aghnet.ValidateDomainName(l.Hostname)
+			if err != nil {
+				log.Debug(
+					"dns: skipping invalid hostname %q from dhcp: %s",
+					l.Hostname,
+					err,
+				)
 			}
 
 			lowhost := strings.ToLower(l.Hostname)