From d707f8b1d1be0ef201209ecfa30b4d43eabea937 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 19 Apr 2021 16:04:40 +0300 Subject: [PATCH] Pull request: all: imp dhcp client hostname normalization Updates #2952. Updates #2978. Squashed commit of the following: commit 20e379b94ccf8140fd9056429315945c17f711a5 Author: Ainar Garipov Date: Mon Apr 19 15:58:37 2021 +0300 all: imp naming commit ed300e0563fa37e161406a97991b26a89e23903a Author: Ainar Garipov Date: Mon Apr 19 15:43:09 2021 +0300 all: imp dhcp client hostname normalization --- CHANGELOG.md | 3 +- internal/aghnet/addr.go | 49 +++++++++++++++----------- internal/aghnet/addr_test.go | 15 +++++++- internal/dhcpd/v4.go | 66 ++++++++++++++++++++++++------------ internal/dhcpd/v4_test.go | 39 +++++++++++++-------- internal/dnsforward/dns.go | 28 +++++++-------- 6 files changed, 127 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7de24cee..dee6d8d2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/aghnet/addr.go b/internal/aghnet/addr.go index ff357c5b..a45c3c82 100644 --- a/internal/aghnet/addr.go +++ b/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 { diff --git a/internal/aghnet/addr_test.go b/internal/aghnet/addr_test.go index 53af1aac..d78e43d3 100644 --- a/internal/aghnet/addr_test.go +++ b/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) }) } diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index 8a0ec630..5d8f5359 100644 --- a/internal/dhcpd/v4.go +++ b/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. -// -// TODO(a.garipov): Add client hostname uniqueness validations and rename the -// method to validateHostname. -func (s *v4Server) normalizeHostname(name string) (norm string, err error) { +// normalizeHostname normalizes a hostname sent by the client. +func 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" + parts := strings.FieldsFunc(name, func(c rune) (ok bool) { + return c != '.' && !aghnet.IsValidHostOuterRune(c) + }) + + if len(parts) == 0 { + return "", fmt.Errorf("normalizing hostname %q: no valid parts", name) } - err = aghnet.ValidateDomainName(norm) - if err != nil { - return "", fmt.Errorf("validating %s hostname: %w", state, err) - } + norm = strings.Join(parts, "-") + norm = strings.TrimSuffix(norm, "-") 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 diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index b655bec9..8afbdf1d 100644 --- a/internal/dhcpd/v4_test.go +++ b/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 @@ -312,24 +312,35 @@ func TestV4Server_normalizeHostname(t *testing.T) { 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: "success_underscores", + hostname: "my_device_01", + wantErrMsg: "", + want: "my-device-01", }, { - name: "error_spaces", - hostname: "! ! !", - wantErrMsg: `validating normalized hostname: ` + - `invalid domain name label at index 0: ` + - `invalid char '!' at index 0 in "!-!-!"`, - want: "", + name: "error_part", + hostname: "device !!!", + wantErrMsg: "", + want: "device", + }, { + name: "error_part_spaces", + hostname: "device ! ! !", + wantErrMsg: "", + want: "device", + }, { + name: "error", + hostname: "!!!", + wantErrMsg: `normalizing hostname "!!!": no valid parts`, + want: "", + }, { + name: "error_spaces", + hostname: "! ! !", + wantErrMsg: `normalizing hostname "! ! !": no valid parts`, + 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 { diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index 11a01557..36cc2025 100644 --- a/internal/dnsforward/dns.go +++ b/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) { - continue + // TODO(a.garipov): Remove this after we're finished + // 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)