From bdcf345155207a25e48872175de9ddcbbc44af62 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Tue, 23 Aug 2022 18:22:49 +0300 Subject: [PATCH] Pull request: 4745 Fix DHCP hostnames Merge in DNS/adguard-home from 4745-fix-dhcp-hostnames to master Closes #4745. Squashed commit of the following: commit fe03c8eda6c8ee35a10eb5f5a8e4d4d0c7373246 Author: Eugene Burkov Date: Tue Aug 23 18:16:16 2022 +0300 dhcpd: imp code, naming commit 7a7129268917d99ba16781b7f2e9bfb7ae84ff3e Author: Eugene Burkov Date: Tue Aug 23 18:10:12 2022 +0300 dhcpd: add tests commit bb14a4a62df1eed6492d30f622c3e22da9a6f4be Author: Eugene Burkov Date: Tue Aug 23 14:58:29 2022 +0300 dhcpd: imp code, docs commit 2ada39f994cb9dbb2208d47a588eb72056bb5306 Author: Eugene Burkov Date: Tue Aug 23 14:44:35 2022 +0300 all: log changes commit cbd3ed254865921be09376097dac9f5926b2349a Author: Eugene Burkov Date: Tue Aug 23 14:40:54 2022 +0300 dhcpd: imp option 81 commit 64dabb52560f5edc08f17aadaa43172a5d74463d Author: Eugene Burkov Date: Tue Aug 23 14:10:15 2022 +0300 dhcpd: fix empty hostname in static lease commit 0df5d10d0d94863b9bbab28129bcc3436fb71222 Author: Eugene Burkov Date: Tue Aug 23 13:34:31 2022 +0300 dhcpd: report dupl hostnames of static lease --- CHANGELOG.md | 2 + internal/dhcpd/v4.go | 123 ++++++++++++++------------------ internal/dhcpd/v4_test.go | 146 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65cc777c..b5edd91b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,9 +30,11 @@ and this project adheres to ### Fixed +- Dynamic leases created with empty hostnames ([#4745]). - Unnecessary logging of non-critical statistics errors ([#4850]). [#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993 +[#4745]: https://github.com/AdguardTeam/AdGuardHome/issues/4745 [#4850]: https://github.com/AdguardTeam/AdGuardHome/issues/4850 diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index 17881d2c..4fb85552 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -20,6 +20,7 @@ import ( "github.com/go-ping/ping" "github.com/insomniacslk/dhcp/dhcpv4" "github.com/insomniacslk/dhcp/dhcpv4/server4" + "golang.org/x/exp/slices" //lint:ignore SA1019 See the TODO in go.mod. "github.com/mdlayher/raw" @@ -93,6 +94,9 @@ func (s *v4Server) validHostnameForClient(cliHostname string, ip net.IP) (hostna if hostname == "" { hostname = aghnet.GenerateHostname(ip) + } else if s.leaseHosts.Has(hostname) { + log.Info("dhcpv4: hostname %q already exists", hostname) + hostname = aghnet.GenerateHostname(ip) } err = netutil.ValidateDomainName(hostname) @@ -252,11 +256,11 @@ func (s *v4Server) rmLeaseByIndex(i int) { // Remove a dynamic lease with the same properties // Return error if a static lease is found func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { - for i := 0; i < len(s.leases); i++ { - l := s.leases[i] + for i, l := range s.leases { + isStatic := l.IsStatic() - if bytes.Equal(l.HWAddr, lease.HWAddr) { - if l.IsStatic() { + if bytes.Equal(l.HWAddr, lease.HWAddr) || l.IP.Equal(lease.IP) { + if isStatic { return errors.Error("static lease already exists") } @@ -268,20 +272,7 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { l = s.leases[i] } - if l.IP.Equal(lease.IP) { - if l.IsStatic() { - return errors.Error("static lease already exists") - } - - s.rmLeaseByIndex(i) - if i == len(s.leases) { - break - } - - l = s.leases[i] - } - - if l.Hostname == lease.Hostname { + if !isStatic && l.Hostname == lease.Hostname { l.Hostname = "" } } @@ -289,6 +280,10 @@ func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { return nil } +// ErrDupHostname is returned by addLease when the added lease has a not empty +// non-unique hostname. +const ErrDupHostname = errors.Error("hostname is not unique") + // addLease adds a dynamic or static lease. func (s *v4Server) addLease(l *Lease) (err error) { r := s.conf.ipRange @@ -304,13 +299,17 @@ func (s *v4Server) addLease(l *Lease) (err error) { return fmt.Errorf("lease %s (%s) out of range, not adding", l.IP, l.HWAddr) } - s.leases = append(s.leases, l) - s.leasedOffsets.set(offset, true) - if l.Hostname != "" { + if s.leaseHosts.Has(l.Hostname) { + return ErrDupHostname + } + s.leaseHosts.Add(l.Hostname) } + s.leases = append(s.leases, l) + s.leasedOffsets.set(offset, true) + return nil } @@ -365,10 +364,11 @@ func (s *v4Server) AddStaticLease(l *Lease) (err error) { return fmt.Errorf("validating hostname: %w", err) } - // Don't check for hostname uniqueness, since we try to emulate - // dnsmasq here, which means that rmDynamicLease below will - // simply empty the hostname of the dynamic lease if there even - // is one. + // Don't check for hostname uniqueness, since we try to emulate dnsmasq + // here, which means that rmDynamicLease below will simply empty the + // hostname of the dynamic lease if there even is one. In case a static + // lease with the same name already exists, addLease will return an + // error and the lease won't be added. l.Hostname = hostname } @@ -523,11 +523,7 @@ func (s *v4Server) findExpiredLease() int { // reserveLease reserves a lease for a client by its MAC-address. It returns // nil if it couldn't allocate a new lease. func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease, err error) { - l = &Lease{ - HWAddr: make([]byte, len(mac)), - } - - copy(l.HWAddr, mac) + l = &Lease{HWAddr: slices.Clone(mac)} l.IP = s.nextIP() if l.IP == nil { @@ -620,33 +616,25 @@ func (s *v4Server) processDiscover(req, resp *dhcpv4.DHCPv4) (l *Lease, err erro return l, nil } -type optFQDN struct { - name string -} +// OptionFQDN returns a DHCPv4 option for sending the FQDN to the client +// requested another hostname. +// +// See https://datatracker.ietf.org/doc/html/rfc4702. +func OptionFQDN(fqdn string) (opt dhcpv4.Option) { + optData := []byte{ + // Set only S and O DHCP client FQDN option flags. + // + // See https://datatracker.ietf.org/doc/html/rfc4702#section-2.1. + 1<<0 | 1<<1, + // The RCODE fields should be set to 0xFF in the server responses. + // + // See https://datatracker.ietf.org/doc/html/rfc4702#section-2.2. + 0xFF, + 0xFF, + } + optData = append(optData, fqdn...) -func (o *optFQDN) String() string { - return "optFQDN" -} - -// flags[1] -// A-RR[1] -// PTR-RR[1] -// name[] -func (o *optFQDN) ToBytes() []byte { - b := make([]byte, 3+len(o.name)) - i := 0 - - b[i] = 0x03 // f_server_overrides | f_server - i++ - - b[i] = 255 // A-RR - i++ - - b[i] = 255 // PTR-RR - i++ - - copy(b[i:], []byte(o.name)) - return b + return dhcpv4.OptGeneric(dhcpv4.OptionFQDN, optData) } // checkLease checks if the pair of mac and ip is already leased. The mismatch @@ -679,6 +667,8 @@ func (s *v4Server) checkLease(mac net.HardwareAddr, ip net.IP) (lease *Lease, mi // processRequest is the handler for the DHCP Request request. func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) { mac := req.ClientHWAddr + // TODO(e.burkov): The IP address can only be requested in DHCPDISCOVER + // message. reqIP := req.RequestedIPAddress() if reqIP == nil { reqIP = req.ClientIPAddr @@ -711,24 +701,17 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needs if !lease.IsStatic() { cliHostname := req.HostName() hostname := s.validHostnameForClient(cliHostname, reqIP) - if hostname != lease.Hostname && s.leaseHosts.Has(hostname) { - log.Info("dhcpv4: hostname %q already exists", hostname) - lease.Hostname = "" - } else { + if lease.Hostname != hostname { lease.Hostname = hostname + resp.UpdateOption(dhcpv4.OptHostName(hostname)) } s.commitLease(lease) } else if lease.Hostname != "" { - o := &optFQDN{ - name: lease.Hostname, - } - fqdn := dhcpv4.Option{ - Code: dhcpv4.OptionFQDN, - Value: o, - } - - resp.UpdateOption(fqdn) + // TODO(e.burkov): This option is used to update the server's DNS + // mapping. The option should only be answered when it has been + // requested. + resp.UpdateOption(OptionFQDN(lease.Hostname)) } resp.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeAck)) @@ -851,7 +834,7 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int { // TODO(a.garipov): Refactor this into handlers. var l *Lease - switch req.MessageType() { + switch mt := req.MessageType(); mt { case dhcpv4.MessageTypeDiscover: l, err = s.processDiscover(req, resp) if err != nil { diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index 3c241a4a..e720d113 100644 --- a/internal/dhcpd/v4_test.go +++ b/internal/dhcpd/v4_test.go @@ -8,7 +8,9 @@ import ( "net" "strings" "testing" + "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghnet" "github.com/AdguardTeam/golibs/stringutil" "github.com/AdguardTeam/golibs/testutil" "github.com/insomniacslk/dhcp/dhcpv4" @@ -23,6 +25,7 @@ var ( DefaultRangeStart = net.IP{192, 168, 10, 100} DefaultRangeEnd = net.IP{192, 168, 10, 200} DefaultGatewayIP = net.IP{192, 168, 10, 1} + DefaultSelfIP = net.IP{192, 168, 10, 2} DefaultSubnetMask = net.IP{255, 255, 255, 0} ) @@ -39,6 +42,7 @@ func defaultV4ServerConf() (conf V4ServerConf) { GatewayIP: DefaultGatewayIP, SubnetMask: DefaultSubnetMask, notify: notify4, + dnsIPAddrs: []net.IP{DefaultSelfIP}, } } @@ -54,6 +58,148 @@ func defaultSrv(t *testing.T) (s DHCPServer) { return s } +func TestV4Server_leasing(t *testing.T) { + const ( + staticName = "static-client" + anotherName = "another-client" + ) + + staticIP := net.IP{192, 168, 10, 10} + anotherIP := DefaultRangeStart + staticMAC := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA} + anotherMAC := net.HardwareAddr{0xBB, 0xBB, 0xBB, 0xBB, 0xBB, 0xBB} + + s := defaultSrv(t) + + t.Run("add_static", func(t *testing.T) { + err := s.AddStaticLease(&Lease{ + Expiry: time.Unix(leaseExpireStatic, 0), + Hostname: staticName, + HWAddr: staticMAC, + IP: staticIP, + }) + require.NoError(t, err) + + t.Run("same_name", func(t *testing.T) { + err = s.AddStaticLease(&Lease{ + Expiry: time.Unix(leaseExpireStatic, 0), + Hostname: staticName, + HWAddr: anotherMAC, + IP: anotherIP, + }) + assert.ErrorIs(t, err, ErrDupHostname) + }) + + t.Run("same_mac", func(t *testing.T) { + wantErrMsg := "dhcpv4: adding static lease: removing " + + "dynamic leases for " + anotherIP.String() + + " (" + staticMAC.String() + "): static lease already exists" + + err = s.AddStaticLease(&Lease{ + Expiry: time.Unix(leaseExpireStatic, 0), + Hostname: anotherName, + HWAddr: staticMAC, + IP: anotherIP, + }) + testutil.AssertErrorMsg(t, wantErrMsg, err) + }) + + t.Run("same_ip", func(t *testing.T) { + wantErrMsg := "dhcpv4: adding static lease: removing " + + "dynamic leases for " + staticIP.String() + + " (" + anotherMAC.String() + "): static lease already exists" + + err = s.AddStaticLease(&Lease{ + Expiry: time.Unix(leaseExpireStatic, 0), + Hostname: anotherName, + HWAddr: anotherMAC, + IP: staticIP, + }) + testutil.AssertErrorMsg(t, wantErrMsg, err) + }) + }) + + t.Run("add_dynamic", func(t *testing.T) { + s4, ok := s.(*v4Server) + require.True(t, ok) + + discoverAnOffer := func( + t *testing.T, + name string, + ip net.IP, + mac net.HardwareAddr, + ) (resp *dhcpv4.DHCPv4) { + testutil.CleanupAndRequireSuccess(t, func() (err error) { + return s.ResetLeases(s.GetLeases(LeasesStatic)) + }) + + req, err := dhcpv4.NewDiscovery( + mac, + dhcpv4.WithOption(dhcpv4.OptHostName(name)), + dhcpv4.WithOption(dhcpv4.OptRequestedIPAddress(ip)), + dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte{1, 2, 3, 4, 5, 6, 8})), + dhcpv4.WithGatewayIP(DefaultGatewayIP), + ) + require.NoError(t, err) + + resp = &dhcpv4.DHCPv4{} + res := s4.process(req, resp) + require.Positive(t, res) + require.Equal(t, dhcpv4.MessageTypeOffer, resp.MessageType()) + + resp.ClientHWAddr = mac + + return resp + } + + t.Run("same_name", func(t *testing.T) { + resp := discoverAnOffer(t, staticName, anotherIP, anotherMAC) + + req, err := dhcpv4.NewRequestFromOffer(resp, dhcpv4.WithOption( + dhcpv4.OptHostName(staticName), + )) + require.NoError(t, err) + + res := s4.process(req, resp) + require.Positive(t, res) + + assert.Equal(t, aghnet.GenerateHostname(resp.YourIPAddr), resp.HostName()) + }) + + t.Run("same_mac", func(t *testing.T) { + resp := discoverAnOffer(t, anotherName, anotherIP, staticMAC) + + req, err := dhcpv4.NewRequestFromOffer(resp, dhcpv4.WithOption( + dhcpv4.OptHostName(anotherName), + )) + require.NoError(t, err) + + res := s4.process(req, resp) + require.Positive(t, res) + + fqdnOptData := resp.Options.Get(dhcpv4.OptionFQDN) + require.Len(t, fqdnOptData, 3+len(staticName)) + assert.Equal(t, []uint8(staticName), fqdnOptData[3:]) + + assert.Equal(t, staticIP, resp.YourIPAddr) + }) + + t.Run("same_ip", func(t *testing.T) { + resp := discoverAnOffer(t, anotherName, staticIP, anotherMAC) + + req, err := dhcpv4.NewRequestFromOffer(resp, dhcpv4.WithOption( + dhcpv4.OptHostName(anotherName), + )) + require.NoError(t, err) + + res := s4.process(req, resp) + require.Positive(t, res) + + assert.NotEqual(t, staticIP, resp.YourIPAddr) + }) + }) +} + func TestV4Server_AddRemove_static(t *testing.T) { s := defaultSrv(t)