Pull request: dhcpd: normalize client host

Updates #2946.

Squashed commit of the following:

commit f830c03ddee65f7e86c43baa00a7dcc022091d93
Merge: df6c83d7 327e76cd
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 13 15:55:58 2021 +0300

    Merge branch 'master' into 2946-norm-dhcp-host

commit df6c83d7d117b718110a035216e708d5131bf71c
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 13 15:49:03 2021 +0300

    dhcpd: imp docs

commit 1407e9bd7b7694bd3069349faba3ec9ff5eb468b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 13 15:27:38 2021 +0300

    dhcpd: normalize client host

CHANGELOG.md

@@ -30,6 +30,7 @@ and this project adheres to
 
 ### Changed
 
+- Normalization of hostnames with spaces sent by DHCP clients ([#2945]).
 - 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
@@ -72,6 +73,7 @@ and this project adheres to
 [#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
 [#2889]: https://github.com/AdguardTeam/AdGuardHome/issues/2889
 [#2927]: https://github.com/AdguardTeam/AdGuardHome/issues/2927
+[#2945]: https://github.com/AdguardTeam/AdGuardHome/issues/2945
 [#2947]: https://github.com/AdguardTeam/AdGuardHome/issues/2947
 
 

internal/dhcpd/nullbool_test.go

@@ -46,9 +46,10 @@ func TestNullBool_UnmarshalText(t *testing.T) {
 			var got nullBool
 			err := got.UnmarshalJSON(tc.data)
 			if tc.wantErrMsg == "" {
-				assert.Nil(t, err)
+				assert.NoError(t, err)
 			} else {
-				require.NotNil(t, err)
+				require.Error(t, err)
+
 				assert.Equal(t, tc.wantErrMsg, err.Error())
 			}
 
@@ -63,7 +64,8 @@ func TestNullBool_UnmarshalText(t *testing.T) {
 		}
 
 		err := json.Unmarshal([]byte(`{"A":true}`), &got)
-		require.Nil(t, err)
+		require.NoError(t, err)
+
 		assert.Equal(t, want, got.A)
 	})
 }

internal/dhcpd/v4.go

@@ -6,6 +6,7 @@ import (
 	"bytes"
 	"fmt"
 	"net"
+	"strings"
 	"sync"
 	"time"
 
@@ -557,10 +558,39 @@ func (o *optFQDN) ToBytes() []byte {
 	return b
 }
 
+// normalizeHostname normalizes and validates a hostname sent by the client.
+//
+// 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
+	// them, so replace them with dashes and issue a warning.
+	//
+	// See https://github.com/AdguardTeam/AdGuardHome/issues/2946.
+	norm = strings.ReplaceAll(name, " ", "-")
+	state := "non-normalized"
+	if name != norm {
+		log.Debug("dhcpv4: normalized hostname %q into %q", name, norm)
+		state = "normalized"
+	}
+
+	err = aghnet.ValidateDomainName(norm)
+	if err != nil {
+		return "", fmt.Errorf("validating %s hostname: %w", state, err)
+	}
+
+	return norm, 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, bool) {
+func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, ok bool) {
-	var lease *Lease
+	var err error
+
 	mac := req.ClientHWAddr
 	reqIP := req.RequestedIPAddress()
 	if reqIP == nil {
@@ -604,7 +634,13 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (*Lease, bool) {
 	}
 
 	if !lease.IsStatic() {
-		lease.Hostname = req.HostName()
+		lease.Hostname, err = s.normalizeHostname(req.HostName())
+		if err != nil {
+			log.Error("dhcpv4: cannot normalize hostname for %s: %s", mac, err)
+
+			return nil, false
+		}
+
 		s.commitLease(lease)
 	} else if len(lease.Hostname) != 0 {
 		o := &optFQDN{

internal/dhcpd/v4_test.go

@@ -269,3 +269,56 @@ func TestV4DynamicLease_Get(t *testing.T) {
 		assert.Equal(t, mac, ls[0].HWAddr)
 	})
 }
+
+func TestV4Server_normalizeHostname(t *testing.T) {
+	testCases := []struct {
+		name       string
+		hostname   string
+		wantErrMsg string
+		want       string
+	}{{
+		name:       "success",
+		hostname:   "example.com",
+		wantErrMsg: "",
+		want:       "example.com",
+	}, {
+		name:       "success_empty",
+		hostname:   "",
+		wantErrMsg: "",
+		want:       "",
+	}, {
+		name:       "success_spaces",
+		hostname:   "my device 01",
+		wantErrMsg: "",
+		want:       "my-device-01",
+	}, {
+		name:     "error",
+		hostname: "!!!",
+		wantErrMsg: `validating non-normalized hostname: ` +
+			`invalid domain name label at index 0: ` +
+			`invalid char '!' at index 0 in "!!!"`,
+		want: "",
+	}, {
+		name:     "error_spaces",
+		hostname: "! ! !",
+		wantErrMsg: `validating normalized hostname: ` +
+			`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)
+			if tc.wantErrMsg == "" {
+				assert.NoError(t, err)
+			} else {
+				require.Error(t, err)
+
+				assert.Equal(t, tc.wantErrMsg, err.Error())
+			}
+
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}