From f717776c640ce31ffbc742144164edffc24c22e6 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Thu, 6 May 2021 13:02:48 +0300 Subject: [PATCH] Pull request: dhcpd: imp normalization, validation Updates #3056. Squashed commit of the following: commit 875954fc8d59980a39b03032007cbc15d87801ea Author: Ainar Garipov Date: Wed May 5 19:54:24 2021 +0300 all: imp err msgs commit c6ea471038ce28f608084b59d3447ff64124260f Author: Ainar Garipov Date: Wed May 5 17:55:12 2021 +0300 dhcpd: imp normalization, validation --- CHANGELOG.md | 8 + internal/aghnet/addr.go | 16 +- internal/aghnet/addr_test.go | 36 ++- internal/dhcpd/dhcpd.go | 10 +- internal/dhcpd/dhcpd_test.go | 24 +- internal/dhcpd/v4.go | 414 ++++++++++++------------- internal/dhcpd/v4_test.go | 41 ++- internal/dnsforward/clientid.go | 4 +- internal/dnsforward/clientid_test.go | 14 +- internal/dnsforward/dnsforward_test.go | 5 +- 10 files changed, 299 insertions(+), 273 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e297dacc..d68dd3c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,14 @@ and this project adheres to ## [v0.106.2] - 2021-05-17 (APPROX.) --> +### Fixed + +- Uniqueness validation for dynamic DHCP leases ([#3056]). + +[#3056]: https://github.com/AdguardTeam/AdGuardHome/issues/3056 + + + ## [v0.106.1] - 2021-04-30 ### Fixed diff --git a/internal/aghnet/addr.go b/internal/aghnet/addr.go index a45c3c82..ba81733f 100644 --- a/internal/aghnet/addr.go +++ b/internal/aghnet/addr.go @@ -48,32 +48,32 @@ const maxDomainLabelLen = 63 // See https://stackoverflow.com/a/32294443/1892060. const maxDomainNameLen = 253 -const invalidCharMsg = "invalid char %q at index %d in %q" - // ValidateDomainNameLabel returns an error if label is not a valid label of // a domain name. func ValidateDomainNameLabel(label string) (err error) { + defer agherr.Annotate("validating label %q: %w", &err, label) + l := len(label) if l > maxDomainLabelLen { - return fmt.Errorf("%q is too long, max: %d", label, maxDomainLabelLen) + return fmt.Errorf("label is too long, max: %d", maxDomainLabelLen) } else if l == 0 { return agherr.Error("label is empty") } if r := label[0]; !IsValidHostOuterRune(rune(r)) { - return fmt.Errorf(invalidCharMsg, r, 0, label) + return fmt.Errorf("invalid char %q at index %d", r, 0) } else if l == 1 { return nil } for i, r := range label[1 : l-1] { if !isValidHostRune(r) { - return fmt.Errorf(invalidCharMsg, r, i+1, label) + return fmt.Errorf("invalid char %q at index %d", r, i+1) } } if r := label[l-1]; !IsValidHostOuterRune(rune(r)) { - return fmt.Errorf(invalidCharMsg, r, l-1, label) + return fmt.Errorf("invalid char %q at index %d", r, l-1) } return nil @@ -87,6 +87,8 @@ func ValidateDomainNameLabel(label string) (err error) { // TODO(a.garipov): After making sure that this works correctly, port this into // module golibs. func ValidateDomainName(name string) (err error) { + defer agherr.Annotate("validating domain name %q: %w", &err, name) + name, err = idna.ToASCII(name) if err != nil { return err @@ -96,7 +98,7 @@ func ValidateDomainName(name string) (err error) { 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) + return fmt.Errorf("too long, max: %d", maxDomainNameLen) } labels := strings.Split(name, ".") diff --git a/internal/aghnet/addr_test.go b/internal/aghnet/addr_test.go index d78e43d3..514f8706 100644 --- a/internal/aghnet/addr_test.go +++ b/internal/aghnet/addr_test.go @@ -95,40 +95,46 @@ func TestValidateDomainName(t *testing.T) { }, { name: "empty", in: "", - wantErrMsg: `domain name is empty`, + wantErrMsg: `validating domain name "": domain name is empty`, }, { name: "bad_symbol", in: "!!!", - wantErrMsg: `invalid domain name label at index 0: ` + - `invalid char '!' at index 0 in "!!!"`, + wantErrMsg: `validating domain name "!!!": invalid domain name label at index 0: ` + + `validating label "!!!": invalid char '!' at index 0`, }, { name: "bad_length", in: longDomainName, - wantErrMsg: `"` + longDomainName + `" is too long, max: 253`, + wantErrMsg: `validating domain name "` + longDomainName + `": too long, max: 253`, }, { name: "bad_label_length", in: longLabelDomainName, - wantErrMsg: `invalid domain name label at index 0: "` + longLabel + - `" is too long, max: 63`, + wantErrMsg: `validating domain name "` + longLabelDomainName + `": ` + + `invalid domain name label at index 0: validating label "` + longLabel + + `": label is too long, max: 63`, }, { - name: "bad_label_empty", - in: "example..com", - wantErrMsg: `invalid domain name label at index 1: label is empty`, + name: "bad_label_empty", + in: "example..com", + wantErrMsg: `validating domain name "example..com": ` + + `invalid domain name label at index 1: ` + + `validating label "": label is empty`, }, { name: "bad_label_first_symbol", in: "example.-aa.com", - wantErrMsg: `invalid domain name label at index 1:` + - ` invalid char '-' at index 0 in "-aa"`, + wantErrMsg: `validating domain name "example.-aa.com": ` + + `invalid domain name label at index 1: ` + + `validating label "-aa": invalid char '-' at index 0`, }, { name: "bad_label_last_symbol", in: "example-.aa.com", - wantErrMsg: `invalid domain name label at index 0:` + - ` invalid char '-' at index 7 in "example-"`, + wantErrMsg: `validating domain name "example-.aa.com": ` + + `invalid domain name label at index 0: ` + + `validating label "example-": invalid char '-' at index 7`, }, { name: "bad_label_symbol", in: "example.a!!!.com", - wantErrMsg: `invalid domain name label at index 1:` + - ` invalid char '!' at index 1 in "a!!!"`, + wantErrMsg: `validating domain name "example.a!!!.com": ` + + `invalid domain name label at index 1: ` + + `validating label "a!!!": invalid char '!' at index 1`, }} for _, tc := range testCases { diff --git a/internal/dhcpd/dhcpd.go b/internal/dhcpd/dhcpd.go index 59be06a0..8fcb4310 100644 --- a/internal/dhcpd/dhcpd.go +++ b/internal/dhcpd/dhcpd.go @@ -27,13 +27,13 @@ var webHandlersRegistered = false // Lease contains the necessary information about a DHCP lease type Lease struct { + // Expiry is the expiration time of the lease. The unix timestamp value + // of 1 means that this is a static lease. + Expiry time.Time `json:"expires"` + + Hostname string `json:"hostname"` HWAddr net.HardwareAddr `json:"mac"` IP net.IP `json:"ip"` - Hostname string `json:"hostname"` - - // Lease expiration time - // 1: static lease - Expiry time.Time `json:"expires"` } // IsStatic returns true if the lease is static. diff --git a/internal/dhcpd/dhcpd_test.go b/internal/dhcpd/dhcpd_test.go index 0371878a..b07ae411 100644 --- a/internal/dhcpd/dhcpd_test.go +++ b/internal/dhcpd/dhcpd_test.go @@ -37,30 +37,34 @@ func TestDB(t *testing.T) { SubnetMask: net.IP{255, 255, 255, 0}, notify: testNotify, }) - require.Nil(t, err) + require.NoError(t, err) s.srv6, err = v6Create(V6ServerConf{}) - require.Nil(t, err) + require.NoError(t, err) leases := []Lease{{ - IP: net.IP{192, 168, 10, 100}, - HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, - Expiry: time.Now().Add(time.Hour), + Expiry: time.Now().Add(time.Hour), + Hostname: "static-1.local", + HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 100}, }, { - IP: net.IP{192, 168, 10, 101}, - HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xBB}, + Hostname: "static-2.local", + HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xBB}, + IP: net.IP{192, 168, 10, 101}, }} srv4, ok := s.srv4.(*v4Server) require.True(t, ok) err = srv4.addLease(&leases[0]) - require.Nil(t, err) - require.Nil(t, s.srv4.AddStaticLease(leases[1])) + require.NoError(t, err) + + err = s.srv4.AddStaticLease(leases[1]) + require.NoError(t, err) s.dbStore() t.Cleanup(func() { - assert.Nil(t, os.Remove(dbFilename)) + assert.NoError(t, os.Remove(dbFilename)) }) s.srv4.ResetLeases(nil) s.dbLoad() diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index cb59fcb4..0a769648 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -36,7 +36,7 @@ type v4Server struct { // leases contains all dynamic and static leases. leases []*Lease - // leasesLock protects leases and leasedOffsets. + // leasesLock protects leases, leaseHosts, and leasedOffsets. leasesLock sync.Mutex } @@ -49,6 +49,68 @@ func (s *v4Server) WriteDiskConfig4(c *V4ServerConf) { func (s *v4Server) WriteDiskConfig6(c *V6ServerConf) { } +// normalizeHostname normalizes a hostname sent by the client. If err is not +// nil, norm is an empty string. +func normalizeHostname(hostname string) (norm string, err error) { + defer agherr.Annotate("normalizing %q: %w", &err, hostname) + + if hostname == "" { + return "", nil + } + + norm = strings.ToLower(hostname) + parts := strings.FieldsFunc(norm, func(c rune) (ok bool) { + return c != '.' && !aghnet.IsValidHostOuterRune(c) + }) + + if len(parts) == 0 { + return "", fmt.Errorf("no valid parts") + } + + norm = strings.Join(parts, "-") + norm = strings.TrimSuffix(norm, "-") + + return norm, nil +} + +// validHostnameForClient accepts the hostname sent by the client and returns +// either a normalized version of that hostname or a new hostname generated from +// the client's IP address. If this new hostname is different from the provided +// previous hostname, additional uniqueness check is performed. +// +// hostname is always a non-empty valid hostname. If err is not nil, it +// describes the issues encountered when normalizing cliHostname. +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 { + log.Info("dhcpv4: normalized hostname %q into %q", cliHostname, hostname) + } + + return hostname, err +} + // ResetLeases - reset leases func (s *v4Server) ResetLeases(leases []*Lease) { var err error @@ -62,9 +124,13 @@ func (s *v4Server) ResetLeases(leases []*Lease) { s.leases = nil for _, l := range leases { - l.Hostname, err = s.validHostnameForClient(l.Hostname, l.IP) + l.Hostname, err = s.validHostnameForClient(l.Hostname, l.Hostname, l.IP) if err != nil { - log.Info("dhcpv4: warning: previous hostname %q is invalid: %s", l.Hostname, err) + log.Info( + "dhcpv4: warning: previous hostname %q is invalid: %s", + l.Hostname, + err, + ) } err = s.addLease(l) @@ -204,7 +270,7 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { if bytes.Equal(l.HWAddr, lease.HWAddr) { if l.IsStatic() { - return fmt.Errorf("static lease already exists") + return agherr.Error("static lease already exists") } s.rmLeaseByIndex(i) @@ -215,9 +281,9 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { l = s.leases[i] } - if net.IP.Equal(l.IP, lease.IP) { + if l.IP.Equal(lease.IP) { if l.IsStatic() { - return fmt.Errorf("static lease already exists") + return agherr.Error("static lease already exists") } s.rmLeaseByIndex(i) @@ -227,54 +293,31 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { return nil } -func (s *v4Server) addStaticLease(l *Lease) (err error) { - if sn := s.conf.subnet; !sn.Contains(l.IP) { - return fmt.Errorf("subnet %s does not contain the ip %q", sn, l.IP) - } - - s.leases = append(s.leases, l) - +// addLease adds a dynamic or static lease. +func (s *v4Server) addLease(l *Lease) (err error) { r := s.conf.ipRange - offset, ok := r.offset(l.IP) - if ok { - s.leasedOffsets.set(offset, true) - } + offset, inOffset := r.offset(l.IP) - s.leaseHosts.Add(l.Hostname) - - return nil -} - -func (s *v4Server) addDynamicLease(l *Lease) (err error) { - r := s.conf.ipRange - offset, ok := r.offset(l.IP) - if !ok { + if l.IsStatic() { + if sn := s.conf.subnet; !sn.Contains(l.IP) { + return fmt.Errorf("subnet %s does not contain the ip %q", sn, l.IP) + } + } else if !inOffset { return fmt.Errorf("lease %s (%s) out of range, not adding", l.IP, l.HWAddr) } s.leases = append(s.leases, l) - s.leaseHosts.Add(l.Hostname) s.leasedOffsets.set(offset, true) + if l.Hostname != "" { + s.leaseHosts.Add(l.Hostname) + } + return nil } -// addLease adds a dynamic or static lease. -func (s *v4Server) addLease(l *Lease) (err error) { - err = s.validateLease(l) - if err != nil { - return err - } - - if l.IsStatic() { - return s.addStaticLease(l) - } - - return s.addDynamicLease(l) -} - -// Remove a lease with the same properties -func (s *v4Server) rmLease(lease Lease) error { +// rmLease removes a lease with the same properties. +func (s *v4Server) rmLease(lease Lease) (err error) { if len(s.leases) == 0 { return nil } @@ -296,7 +339,7 @@ func (s *v4Server) rmLease(lease Lease) error { // AddStaticLease adds a static lease. It is safe for concurrent use. func (s *v4Server) AddStaticLease(l Lease) (err error) { - defer agherr.Annotate("dhcpv4: %w", &err) + defer agherr.Annotate("dhcpv4: adding static lease: %w", &err) if ip4 := l.IP.To4(); ip4 == nil { return fmt.Errorf("invalid ip %q, only ipv4 is supported", l.IP) @@ -304,11 +347,28 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) { l.Expiry = time.Unix(leaseExpireStatic, 0) - l.Hostname, err = normalizeHostname(l.Hostname) + err = aghnet.ValidateHardwareAddress(l.HWAddr) if err != nil { return err } + var hostname string + hostname, err = normalizeHostname(l.Hostname) + if err != nil { + return err + } + + err = aghnet.ValidateDomainName(hostname) + if err != nil { + return fmt.Errorf("validating hostname: %w", err) + } + + if s.leaseHosts.Has(hostname) { + return agherr.Error("hostname exists") + } + + l.Hostname = hostname + // Perform the following actions in an anonymous function to make sure // that the lock gets unlocked before the notification step. func() { @@ -372,16 +432,19 @@ func (s *v4Server) RemoveStaticLease(l Lease) (err error) { return nil } -// Send ICMP to the specified machine -// Return TRUE if it doesn't reply, which probably means that the IP is available -func (s *v4Server) addrAvailable(target net.IP) bool { +// addrAvailable sends an ICP request to the specified IP address. It returns +// true if the remote host doesn't reply, which probably means that the IP +// address is available. +// +// TODO(a.garipov): I'm not sure that this is the best way to do this. +func (s *v4Server) addrAvailable(target net.IP) (avail bool) { if s.conf.ICMPTimeout == 0 { return true } pinger, err := ping.NewPinger(target.String()) if err != nil { - log.Error("ping.NewPinger(): %v", err) + log.Error("dhcpv4: ping.NewPinger(): %s", err) return true } @@ -393,20 +456,24 @@ func (s *v4Server) addrAvailable(target net.IP) bool { pinger.OnRecv = func(_ *ping.Packet) { reply = true } - log.Debug("dhcpv4: Sending ICMP Echo to %v", target) + + log.Debug("dhcpv4: sending icmp echo to %s", target) err = pinger.Run() if err != nil { - log.Error("pinger.Run(): %v", err) + log.Error("dhcpv4: pinger.Run(): %s", err) + return true } if reply { - log.Info("dhcpv4: IP conflict: %v is already used by another device", target) + log.Info("dhcpv4: ip conflict: %s is already used by another device", target) + return false } - log.Debug("dhcpv4: ICMP procedure is complete: %v", target) + log.Debug("dhcpv4: icmp procedure is complete: %q", target) + return true } @@ -481,58 +548,69 @@ func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease, err error) { func (s *v4Server) commitLease(l *Lease) { l.Expiry = time.Now().Add(s.conf.leaseTime) - s.leasesLock.Lock() - s.conf.notify(LeaseChangedDBStore) - s.leasesLock.Unlock() + func() { + s.leasesLock.Lock() + defer s.leasesLock.Unlock() + + s.conf.notify(LeaseChangedDBStore) + s.leaseHosts.Add(l.Hostname) + }() s.conf.notify(LeaseChangedAdded) } -// Process Discover request and return lease +// processDiscover is the handler for the DHCP Discover request. func (s *v4Server) processDiscover(req, resp *dhcpv4.DHCPv4) (l *Lease, err error) { mac := req.ClientHWAddr + err = aghnet.ValidateHardwareAddress(mac) + if err != nil { + return nil, err + } + s.leasesLock.Lock() defer s.leasesLock.Unlock() - // TODO(a.garipov): Refactor this mess. l = s.findLease(mac) - if l == nil { - toStore := false - for l == nil { - l, err = s.reserveLease(mac) - if err != nil { - return nil, fmt.Errorf("reserving a lease: %w", err) - } - - if l == nil { - log.Debug("dhcpv4: no more ip addresses") - if toStore { - s.conf.notify(LeaseChangedDBStore) - } - - // TODO(a.garipov): Return a special error? - return nil, nil - } - - toStore = true - - if !s.addrAvailable(l.IP) { - s.blocklistLease(l) - l = nil - - continue - } - - break - } - - s.conf.notify(LeaseChangedDBStore) - } else { + if l != nil { reqIP := req.RequestedIPAddress() if len(reqIP) != 0 && !reqIP.Equal(l.IP) { log.Debug("dhcpv4: different RequestedIP: %s != %s", reqIP, l.IP) } + + resp.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeOffer)) + + return l, nil + } + + needsUpdate := false + defer func() { + if needsUpdate { + s.conf.notify(LeaseChangedDBStore) + } + }() + + leaseReady := false + for !leaseReady { + l, err = s.reserveLease(mac) + if err != nil { + return nil, fmt.Errorf("reserving a lease: %w", err) + } + + if l == nil { + log.Debug("dhcpv4: no more ip addresses") + + return nil, nil + } + + needsUpdate = true + + if s.addrAvailable(l.IP) { + leaseReady = true + } else { + s.blocklistLease(l) + l = nil + } } resp.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeOffer)) @@ -569,115 +647,14 @@ func (o *optFQDN) ToBytes() []byte { return b } -// normalizeHostname normalizes a hostname sent by the client. If err is not -// nil, norm is an empty string. -func normalizeHostname(name string) (norm string, err error) { - if name == "" { - return "", nil - } - - norm = strings.ToLower(name) - parts := strings.FieldsFunc(norm, func(c rune) (ok bool) { - return c != '.' && !aghnet.IsValidHostOuterRune(c) - }) - - if len(parts) == 0 { - return "", fmt.Errorf("normalizing hostname %q: no valid parts", name) - } - - 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) { - defer agherr.Annotate("validating hostname: %s", &err) - - if name == "" { - return nil - } - - err = aghnet.ValidateDomainName(name) - if err != nil { - return err - } - - if s.leaseHosts.Has(name) { - return agherr.Error("hostname exists") - } - - return nil -} - -// validHostnameForClient accepts the hostname sent by the client and returns -// either a normalized version of that hostname or a new hostname generated from -// the client's IP address. -// -// hostname is always a non-empty valid hostname. If err is not nil, it -// describes the issues encountered when normalizing cliHostname. -func (s *v4Server) validHostnameForClient( - cliHostname string, - ip net.IP, -) (hostname string, err error) { - hostname, err = normalizeHostname(cliHostname) - if err == nil { - err = s.validateHostname(hostname) - if err != nil { - // Go on and assign a hostname made from the IP below, - // returning the error that we've got. - hostname = "" - } - } - - if hostname == "" { - hostname = aghnet.GenerateHostname(ip) - } - - if hostname != cliHostname { - log.Info("dhcpv4: normalized hostname %q into %q", cliHostname, hostname) - } - - return hostname, err -} - -// validateLease returns an error if the lease is invalid. -func (s *v4Server) validateLease(l *Lease) (err error) { - defer agherr.Annotate("validating lease: %s", &err) - - if l == nil { - return agherr.Error("lease is nil") - } - - err = aghnet.ValidateHardwareAddress(l.HWAddr) - if err != nil { - return err - } - - err = s.validateHostname(l.Hostname) - if err != nil { - return err - } - - if sn := s.conf.subnet; !sn.Contains(l.IP) { - return fmt.Errorf("subnet %s does not contain the ip %q", sn, l.IP) - } - - r := s.conf.ipRange - if !l.IsStatic() && !r.contains(l.IP) { - return fmt.Errorf("dynamic lease range %s does not contain the ip %q", r, l.IP) - } - - 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) { - var err error - +// processDiscover 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) + if err != nil { + return nil, false + } + reqIP := req.RequestedIPAddress() if reqIP == nil { reqIP = req.ClientIPAddr @@ -696,34 +673,49 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, ok bo return nil, false } - s.leasesLock.Lock() - for _, l := range s.leases { - if bytes.Equal(l.HWAddr, mac) { - if !l.IP.Equal(reqIP) { - s.leasesLock.Unlock() - log.Debug("dhcpv4: mismatched OptionRequestedIPAddress in request message for %s", mac) + mismatch := false + func() { + s.leasesLock.Lock() + defer s.leasesLock.Unlock() - return nil, true + for _, l := range s.leases { + if !bytes.Equal(l.HWAddr, mac) { + continue } - lease = l + if l.IP.Equal(reqIP) { + lease = l + } else { + log.Debug( + `dhcpv4: mismatched OptionRequestedIPAddress `+ + `in request message for %s`, + mac, + ) + mismatch = true + } - break + return } + }() + if mismatch { + return nil, true } - s.leasesLock.Unlock() if lease == nil { - log.Debug("dhcpv4: no lease for %s", mac) + log.Debug("dhcpv4: no reserved lease for %s", mac) return nil, true } if !lease.IsStatic() { cliHostname := req.HostName() - lease.Hostname, err = s.validHostnameForClient(cliHostname, reqIP) + lease.Hostname, err = s.validHostnameForClient(cliHostname, lease.Hostname, reqIP) if err != nil { - log.Info("dhcpv4: warning: client hostname %q is invalid: %s", cliHostname, err) + log.Info( + "dhcpv4: warning: client hostname %q is invalid: %s", + cliHostname, + err, + ) } s.commitLease(lease) diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index 303b6cb8..daece0a8 100644 --- a/internal/dhcpd/v4_test.go +++ b/internal/dhcpd/v4_test.go @@ -30,8 +30,9 @@ func TestV4_AddRemove_static(t *testing.T) { // Add static lease. l := Lease{ - IP: net.IP{192, 168, 10, 150}, - HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + Hostname: "static-1.local", + HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 150}, } err = s.AddStaticLease(l) @@ -76,11 +77,13 @@ func TestV4_AddReplace(t *testing.T) { require.True(t, ok) dynLeases := []Lease{{ - IP: net.IP{192, 168, 10, 150}, - HWAddr: net.HardwareAddr{0x11, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + Hostname: "dynamic-1.local", + HWAddr: net.HardwareAddr{0x11, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 150}, }, { - IP: net.IP{192, 168, 10, 151}, - HWAddr: net.HardwareAddr{0x22, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + Hostname: "dynamic-2.local", + HWAddr: net.HardwareAddr{0x22, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 151}, }} for i := range dynLeases { @@ -89,11 +92,13 @@ func TestV4_AddReplace(t *testing.T) { } stLeases := []Lease{{ - IP: net.IP{192, 168, 10, 150}, - HWAddr: net.HardwareAddr{0x33, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + Hostname: "static-1.local", + HWAddr: net.HardwareAddr{0x33, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 150}, }, { - IP: net.IP{192, 168, 10, 152}, - HWAddr: net.HardwareAddr{0x22, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + Hostname: "static-2.local", + HWAddr: net.HardwareAddr{0x22, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 152}, }} for _, l := range stLeases { @@ -129,8 +134,9 @@ func TestV4StaticLease_Get(t *testing.T) { s.conf.dnsIPAddrs = []net.IP{{192, 168, 10, 1}} l := Lease{ - IP: net.IP{192, 168, 10, 150}, - HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + Hostname: "static-1.local", + HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}, + IP: net.IP{192, 168, 10, 150}, } err = s.AddStaticLease(l) require.NoError(t, err) @@ -269,7 +275,12 @@ func TestV4DynamicLease_Get(t *testing.T) { assert.Equal(t, dhcpv4.MessageTypeAck, resp.MessageType()) assert.Equal(t, mac, resp.ClientHWAddr) assert.True(t, s.conf.RangeStart.Equal(resp.YourIPAddr)) - assert.True(t, s.conf.GatewayIP.Equal(resp.Router()[0])) + + router := resp.Router() + require.Len(t, router, 1) + + assert.Equal(t, s.conf.GatewayIP, router[0]) + assert.True(t, s.conf.GatewayIP.Equal(resp.ServerIdentifier())) assert.Equal(t, s.conf.subnet.Mask, resp.SubnetMask()) assert.Equal(t, s.conf.leaseTime.Seconds(), resp.IPAddressLeaseTime(-1).Seconds()) @@ -329,12 +340,12 @@ func TestNormalizeHostname(t *testing.T) { }, { name: "error", hostname: "!!!", - wantErrMsg: `normalizing hostname "!!!": no valid parts`, + wantErrMsg: `normalizing "!!!": no valid parts`, want: "", }, { name: "error_spaces", hostname: "! ! !", - wantErrMsg: `normalizing hostname "! ! !": no valid parts`, + wantErrMsg: `normalizing "! ! !": no valid parts`, want: "", }} diff --git a/internal/dnsforward/clientid.go b/internal/dnsforward/clientid.go index ab7af437..6f6debe7 100644 --- a/internal/dnsforward/clientid.go +++ b/internal/dnsforward/clientid.go @@ -2,6 +2,7 @@ package dnsforward import ( "crypto/tls" + "errors" "fmt" "path" "strings" @@ -15,7 +16,8 @@ import ( func ValidateClientID(clientID string) (err error) { err = aghnet.ValidateDomainNameLabel(clientID) if err != nil { - return fmt.Errorf("invalid client id: %w", err) + // Replace the domain name label wrapper with our own. + return fmt.Errorf("invalid client id %q: %w", clientID, errors.Unwrap(err)) } return nil diff --git a/internal/dnsforward/clientid_test.go b/internal/dnsforward/clientid_test.go index 35474d20..9394a9ab 100644 --- a/internal/dnsforward/clientid_test.go +++ b/internal/dnsforward/clientid_test.go @@ -117,8 +117,8 @@ func TestProcessClientID(t *testing.T) { hostSrvName: "example.com", cliSrvName: "!!!.example.com", wantClientID: "", - wantErrMsg: `client id check: invalid client id: invalid char '!' ` + - `at index 0 in "!!!"`, + wantErrMsg: `client id check: invalid client id "!!!": ` + + `invalid char '!' at index 0`, wantRes: resultCodeError, strictSNI: true, }, { @@ -128,9 +128,9 @@ func TestProcessClientID(t *testing.T) { cliSrvName: `abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmno` + `pqrstuvwxyz0123456789.example.com`, wantClientID: "", - wantErrMsg: `client id check: invalid client id: "abcdefghijklmno` + - `pqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789" ` + - `is too long, max: 63`, + wantErrMsg: `client id check: invalid client id "abcdefghijklmno` + + `pqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789": ` + + `label is too long, max: 63`, wantRes: resultCodeError, strictSNI: true, }, { @@ -238,8 +238,8 @@ func TestProcessClientID_https(t *testing.T) { name: "invalid_client_id", path: "/dns-query/!!!", wantClientID: "", - wantErrMsg: `client id check: invalid client id: invalid char '!' ` + - `at index 0 in "!!!"`, + wantErrMsg: `client id check: invalid client id "!!!": ` + + `invalid char '!' at index 0`, wantRes: resultCodeError, }} diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go index fdd703e1..91949351 100644 --- a/internal/dnsforward/dnsforward_test.go +++ b/internal/dnsforward/dnsforward_test.go @@ -1166,8 +1166,9 @@ func TestNewServer(t *testing.T) { in: DNSCreateParams{ LocalDomain: "!!!", }, - wantErrMsg: `local domain: invalid domain name label at index 0: ` + - `invalid char '!' at index 0 in "!!!"`, + wantErrMsg: `local domain: validating domain name "!!!": ` + + `invalid domain name label at index 0: ` + + `validating label "!!!": invalid char '!' at index 0`, }} for _, tc := range testCases {