From 773f02cf7d3edaa2f4cf594142a6cb3b6c4a90d7 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Tue, 13 Apr 2021 16:00:09 +0300 Subject: [PATCH] Pull request: dhcpd: normalize client host Updates #2946. Squashed commit of the following: commit f830c03ddee65f7e86c43baa00a7dcc022091d93 Merge: df6c83d7 327e76cd Author: Ainar Garipov Date: Tue Apr 13 15:55:58 2021 +0300 Merge branch 'master' into 2946-norm-dhcp-host commit df6c83d7d117b718110a035216e708d5131bf71c Author: Ainar Garipov Date: Tue Apr 13 15:49:03 2021 +0300 dhcpd: imp docs commit 1407e9bd7b7694bd3069349faba3ec9ff5eb468b Author: Ainar Garipov Date: Tue Apr 13 15:27:38 2021 +0300 dhcpd: normalize client host --- CHANGELOG.md | 2 ++ internal/dhcpd/nullbool_test.go | 8 +++-- internal/dhcpd/v4.go | 42 ++++++++++++++++++++++++-- internal/dhcpd/v4_test.go | 53 +++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b9541d5..c54857dd 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dhcpd/nullbool_test.go b/internal/dhcpd/nullbool_test.go index 2570dd44..44d1b223 100644 --- a/internal/dhcpd/nullbool_test.go +++ b/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) }) } diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index a4ff66fb..3ad76036 100644 --- a/internal/dhcpd/v4.go +++ b/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) { - var lease *Lease +func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, ok bool) { + 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{ diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index 9afe5fbb..d84a704a 100644 --- a/internal/dhcpd/v4_test.go +++ b/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) + }) + } +}