Pull request: 4863 fix dhcp request

Merge in DNS/adguard-home from 4863-fix-dhcp-request to master

Closes #4863.

Squashed commit of the following:

commit f8872015e315eab3b2ce0249e552d12cbcf72f63
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 20:34:35 2022 +0300

    dhcpd: imp code

commit b63c5d98c2055c3a3b76ff47737551840409f324
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 20:19:22 2022 +0300

    dhcpd: fix deadlock

commit 5c03b54a86ab05efde9716faef60b84ecab01d19
Merge: f076cf8f 8733f55c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 19:12:27 2022 +0300

    Merge branch 'master' into 4863-fix-dhcp-request

commit f076cf8fc13944613b7127aac86ca78f68009f93
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 19:08:04 2022 +0300

    dhcpd: imp code, names

commit a09540b6db6b86b80b8eb84c08187bfd9f960190
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 17:28:39 2022 +0300

    dhcpd: imp code, docs

commit 38b12235509aaf55fa130f820213410b6b3022bb
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 16:42:32 2022 +0300

    dhcpd: imp docs more

commit ff07c2f90f097754beb736fd5bd5cafc337ac65c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 16:41:42 2022 +0300

    dhcpd: fix docs

commit fafbc2ec2317f2320d8e1db167a1ae6c1c7c3790
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 16:35:42 2022 +0300

    dhcpd: imp code

commit 9fe30190a7f125fd640b58e17661a4c33c078eba
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 15:00:56 2022 +0300

    all: imp chlog

commit 1067fe95df5cb2252d1b9b70d2f3f8463997aea1
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 14:54:03 2022 +0300

    dhcpd: log changes

commit 20de395c2bdcfb8e0554bb1c45385c15d617be65
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Aug 30 14:49:58 2022 +0300

    dhcpd: impl rfc 2131 for req
This commit is contained in:
Eugene Burkov 2022-08-30 20:39:34 +03:00
parent 8733f55c2c
commit 5cc2a2cd0c
4 changed files with 212 additions and 84 deletions

View File

@ -32,6 +32,7 @@ and this project adheres to
### Changed
- The DHCPREQUEST handling is now closer to the [RFC 2131][rfc-2131] ([#4863]).
- The internal DNS client, used to resolve hostnames of external clients and
also during automatic updates, now respects the upstream mode settings for the
main DNS client ([#4403]).
@ -55,6 +56,9 @@ and this project adheres to
[#4535]: https://github.com/AdguardTeam/AdGuardHome/issues/4535
[#4745]: https://github.com/AdguardTeam/AdGuardHome/issues/4745
[#4850]: https://github.com/AdguardTeam/AdGuardHome/issues/4850
[#4863]: https://github.com/AdguardTeam/AdGuardHome/issues/4863
[rfc-2131]: https://datatracker.ietf.org/doc/html/rfc2131

View File

@ -71,12 +71,12 @@ type V4ServerConf struct {
// gateway.
subnet *net.IPNet
// notify is a way to signal to other components that leases have
// change. notify must be called outside of locked sections, since the
// notify is a way to signal to other components that leases have been
// changed. notify must be called outside of locked sections, since the
// clients might want to get the new data.
//
// TODO(a.garipov): This is utter madness and must be refactored. It
// just begs for deadlock bugs and other nastiness.
// TODO(a.garipov): This is utter madness and must be refactored. It just
// begs for deadlock bugs and other nastiness.
notify func(uint32)
}

View File

@ -94,9 +94,6 @@ 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)
@ -421,19 +418,19 @@ func (s *v4Server) RemoveStaticLease(l *Lease) (err error) {
return fmt.Errorf("validating lease: %w", err)
}
defer func() {
if err != nil {
return
}
s.conf.notify(LeaseChangedDBStore)
s.conf.notify(LeaseChangedRemovedStatic)
}()
s.leasesLock.Lock()
err = s.rmLease(l)
if err != nil {
s.leasesLock.Unlock()
defer s.leasesLock.Unlock()
return err
}
s.leasesLock.Unlock()
s.conf.notify(LeaseChangedDBStore)
s.conf.notify(LeaseChangedRemovedStatic)
return nil
return s.rmLease(l)
}
// addrAvailable sends an ICP request to the specified IP address. It returns
@ -545,21 +542,34 @@ func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease, err error) {
return l, nil
}
func (s *v4Server) commitLease(l *Lease) {
l.Expiry = time.Now().Add(s.conf.leaseTime)
// commitLease refreshes l's values. It takes the desired hostname into account
// when setting it into the lease, but generates a unique one if the provided
// can't be used.
func (s *v4Server) commitLease(l *Lease, hostname string) {
prev := l.Hostname
hostname = s.validHostnameForClient(hostname, l.IP)
func() {
s.leasesLock.Lock()
defer s.leasesLock.Unlock()
if s.leaseHosts.Has(hostname) {
log.Info("dhcpv4: hostname %q already exists", hostname)
s.conf.notify(LeaseChangedDBStore)
if l.Hostname != "" {
s.leaseHosts.Add(l.Hostname)
if prev == "" {
// The lease is just allocated due to DHCPDISCOVER.
hostname = aghnet.GenerateHostname(l.IP)
} else {
hostname = prev
}
}()
}
if l.Hostname != hostname {
l.Hostname = hostname
}
s.conf.notify(LeaseChangedAdded)
l.Expiry = time.Now().Add(s.conf.leaseTime)
if prev != "" && prev != l.Hostname {
s.leaseHosts.Del(prev)
}
if l.Hostname != "" {
s.leaseHosts.Add(l.Hostname)
}
}
// allocateLease allocates a new lease for the MAC address. If there are no IP
@ -581,8 +591,8 @@ func (s *v4Server) allocateLease(mac net.HardwareAddr) (l *Lease, err error) {
}
}
// processDiscover is the handler for the DHCP Discover request.
func (s *v4Server) processDiscover(req, resp *dhcpv4.DHCPv4) (l *Lease, err error) {
// handleDiscover is the handler for the DHCP Discover request.
func (s *v4Server) handleDiscover(req, resp *dhcpv4.DHCPv4) (l *Lease, err error) {
mac := req.ClientHWAddr
defer s.conf.notify(LeaseChangedDBStore)
@ -664,63 +674,177 @@ func (s *v4Server) checkLease(mac net.HardwareAddr, ip net.IP) (lease *Lease, mi
return nil, false
}
// processRequest is the handler for the DHCP Request request.
func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) {
// handleSelecting handles the DHCPREQUEST generated during SELECTING state.
func (s *v4Server) handleSelecting(
req *dhcpv4.DHCPv4,
reqIP net.IP,
sid net.IP,
) (l *Lease, needsReply bool) {
// Client inserts the address of the selected server in server identifier,
// ciaddr MUST be zero.
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
}
if !sid.Equal(s.conf.dnsIPAddrs[0]) {
log.Debug("dhcpv4: bad server identifier in req msg for %s: %s", mac, sid)
sid := req.ServerIdentifier()
if len(sid) != 0 && !sid.Equal(s.conf.dnsIPAddrs[0]) {
log.Debug("dhcpv4: bad OptionServerIdentifier in req msg for %s", mac)
return nil, false
} else if ciaddr := req.ClientIPAddr; ciaddr != nil && !ciaddr.IsUnspecified() {
log.Debug("dhcpv4: non-zero ciaddr in selecting req msg for %s", mac)
return nil, false
}
// Requested IP address MUST be filled in with the yiaddr value from the
// chosen DHCPOFFER.
if ip4 := reqIP.To4(); ip4 == nil {
log.Debug("dhcpv4: bad OptionRequestedIPAddress in req msg for %s", mac)
log.Debug("dhcpv4: bad requested address in req msg for %s: %s", mac, reqIP)
return nil, false
}
var mismatch bool
if lease, mismatch = s.checkLease(mac, reqIP); mismatch {
if l, mismatch = s.checkLease(mac, reqIP); mismatch {
return nil, true
}
if lease == nil {
} else if l == nil {
log.Debug("dhcpv4: no reserved lease for %s", mac)
}
return l, true
}
// handleInitReboot handles the DHCPREQUEST generated during INIT-REBOOT state.
func (s *v4Server) handleInitReboot(req *dhcpv4.DHCPv4, reqIP net.IP) (l *Lease, needsReply bool) {
mac := req.ClientHWAddr
if ip4 := reqIP.To4(); ip4 == nil {
log.Debug("dhcpv4: bad requested address in req msg for %s: %s", mac, reqIP)
return nil, false
}
// ciaddr MUST be zero. The client is seeking to verify a previously
// allocated, cached configuration.
if ciaddr := req.ClientIPAddr; ciaddr != nil && !ciaddr.IsUnspecified() {
log.Debug("dhcpv4: non-zero ciaddr in init-reboot req msg for %s", mac)
return nil, false
}
if !s.conf.subnet.Contains(reqIP) {
// If the DHCP server detects that the client is on the wrong net then
// the server SHOULD send a DHCPNAK message to the client.
log.Debug("dhcpv4: wrong subnet in init-reboot req msg for %s: %s", mac, reqIP)
return nil, true
}
if !lease.IsStatic() {
cliHostname := req.HostName()
hostname := s.validHostnameForClient(cliHostname, reqIP)
if lease.Hostname != hostname {
lease.Hostname = hostname
resp.UpdateOption(dhcpv4.OptHostName(hostname))
}
var mismatch bool
if l, mismatch = s.checkLease(mac, reqIP); mismatch {
return nil, true
} else if l == nil {
// If the DHCP server has no record of this client, then it MUST remain
// silent, and MAY output a warning to the network administrator.
log.Info("dhcpv4: warning: no existing lease for %s", mac)
s.commitLease(lease)
} else if lease.Hostname != "" {
// 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))
return nil, false
}
return l, true
}
// handleRenew handles the DHCPREQUEST generated during RENEWING or REBINDING
// state.
func (s *v4Server) handleRenew(req *dhcpv4.DHCPv4) (l *Lease, needsReply bool) {
mac := req.ClientHWAddr
// ciaddr MUST be filled in with client's IP address.
ciaddr := req.ClientIPAddr
if ciaddr == nil || ciaddr.IsUnspecified() || ciaddr.To4() == nil {
log.Debug("dhcpv4: bad ciaddr in renew req msg for %s: %s", mac, ciaddr)
return nil, false
}
var mismatch bool
if l, mismatch = s.checkLease(mac, ciaddr); mismatch {
return nil, true
} else if l == nil {
// If the DHCP server has no record of this client, then it MUST remain
// silent, and MAY output a warning to the network administrator.
log.Info("dhcpv4: warning: no existing lease for %s", mac)
return nil, false
}
return l, true
}
// handleByRequestType handles the DHCPREQUEST according to the state during
// which it's generated by client.
func (s *v4Server) handleByRequestType(req *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) {
reqIP, sid := req.RequestedIPAddress(), req.ServerIdentifier()
if sid != nil && !sid.IsUnspecified() {
// If the DHCPREQUEST message contains a server identifier option, the
// message is in response to a DHCPOFFER message. Otherwise, the
// message is a request to verify or extend an existing lease.
return s.handleSelecting(req, reqIP, sid)
}
if reqIP != nil && !reqIP.IsUnspecified() {
// Requested IP address option MUST be filled in with client's notion of
// its previously assigned address.
return s.handleInitReboot(req, reqIP)
}
// Server identifier MUST NOT be filled in, requested IP address option MUST
// NOT be filled in.
return s.handleRenew(req)
}
// handleRequest is the handler for a DHCPREQUEST message.
//
// See https://datatracker.ietf.org/doc/html/rfc2131#section-4.3.2.
func (s *v4Server) handleRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) {
lease, needsReply = s.handleByRequestType(req)
if lease == nil {
return nil, needsReply
}
resp.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeAck))
return lease, true
hostname := req.HostName()
isRequested := hostname != "" || req.ParameterRequestList().Has(dhcpv4.OptionHostName)
defer func() {
s.conf.notify(LeaseChangedAdded)
s.conf.notify(LeaseChangedDBStore)
}()
s.leasesLock.Lock()
defer s.leasesLock.Unlock()
if lease.IsStatic() {
if lease.Hostname != "" {
// 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))
}
return lease, needsReply
}
s.commitLease(lease, hostname)
if isRequested {
resp.UpdateOption(dhcpv4.OptHostName(lease.Hostname))
}
return lease, needsReply
}
// processRequest is the handler for the DHCP Decline request.
func (s *v4Server) processDecline(req, resp *dhcpv4.DHCPv4) (err error) {
// handleDecline is the handler for the DHCP Decline request.
func (s *v4Server) handleDecline(req, resp *dhcpv4.DHCPv4) (err error) {
s.conf.notify(LeaseChangedDBStore)
s.leasesLock.Lock()
@ -782,8 +906,8 @@ func (s *v4Server) processDecline(req, resp *dhcpv4.DHCPv4) (err error) {
return nil
}
// processRelease is the handler for the DHCP Release request.
func (s *v4Server) processRelease(req, resp *dhcpv4.DHCPv4) (err error) {
// handleRelease is the handler for the DHCP Release request.
func (s *v4Server) handleRelease(req, resp *dhcpv4.DHCPv4) (err error) {
mac := req.ClientHWAddr
reqIP := req.RequestedIPAddress()
if reqIP == nil {
@ -824,7 +948,7 @@ func (s *v4Server) processRelease(req, resp *dhcpv4.DHCPv4) (err error) {
// Return 1: OK
// Return 0: error; reply with Nak
// Return -1: error; don't reply
func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int {
func (s *v4Server) handle(req, resp *dhcpv4.DHCPv4) int {
var err error
// Include server's identifier option since any reply should contain it.
@ -836,9 +960,9 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int {
var l *Lease
switch mt := req.MessageType(); mt {
case dhcpv4.MessageTypeDiscover:
l, err = s.processDiscover(req, resp)
l, err = s.handleDiscover(req, resp)
if err != nil {
log.Error("dhcpv4: processing discover: %s", err)
log.Error("dhcpv4: handling discover: %s", err)
return 0
}
@ -848,7 +972,7 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int {
}
case dhcpv4.MessageTypeRequest:
var toReply bool
l, toReply = s.processRequest(req, resp)
l, toReply = s.handleRequest(req, resp)
if l == nil {
if toReply {
return 0
@ -856,16 +980,16 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int {
return -1 // drop packet
}
case dhcpv4.MessageTypeDecline:
err = s.processDecline(req, resp)
err = s.handleDecline(req, resp)
if err != nil {
log.Error("dhcpv4: processing decline: %s", err)
log.Error("dhcpv4: handling decline: %s", err)
return 0
}
case dhcpv4.MessageTypeRelease:
err = s.processRelease(req, resp)
err = s.handleRelease(req, resp)
if err != nil {
log.Error("dhcpv4: processing release: %s", err)
log.Error("dhcpv4: handling release: %s", err)
return 0
}
@ -939,7 +1063,7 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4
return
}
r := s.process(req, resp)
r := s.handle(req, resp)
if r < 0 {
return
} else if r == 0 {

View File

@ -143,7 +143,7 @@ func TestV4Server_leasing(t *testing.T) {
require.NoError(t, err)
resp = &dhcpv4.DHCPv4{}
res := s4.process(req, resp)
res := s4.handle(req, resp)
require.Positive(t, res)
require.Equal(t, dhcpv4.MessageTypeOffer, resp.MessageType())
@ -160,7 +160,7 @@ func TestV4Server_leasing(t *testing.T) {
))
require.NoError(t, err)
res := s4.process(req, resp)
res := s4.handle(req, resp)
require.Positive(t, res)
assert.Equal(t, aghnet.GenerateHostname(resp.YourIPAddr), resp.HostName())
@ -174,7 +174,7 @@ func TestV4Server_leasing(t *testing.T) {
))
require.NoError(t, err)
res := s4.process(req, resp)
res := s4.handle(req, resp)
require.Positive(t, res)
fqdnOptData := resp.Options.Get(dhcpv4.OptionFQDN)
@ -192,7 +192,7 @@ func TestV4Server_leasing(t *testing.T) {
))
require.NoError(t, err)
res := s4.process(req, resp)
res := s4.handle(req, resp)
require.Positive(t, res)
assert.NotEqual(t, staticIP, resp.YourIPAddr)
@ -328,7 +328,7 @@ func TestV4_AddReplace(t *testing.T) {
}
}
func TestV4Server_Process_optionsPriority(t *testing.T) {
func TestV4Server_handle_optionsPriority(t *testing.T) {
defaultIP := net.IP{192, 168, 1, 1}
knownIP := net.IP{1, 2, 3, 4}
@ -376,7 +376,7 @@ func TestV4Server_Process_optionsPriority(t *testing.T) {
resp, err = dhcpv4.NewReplyFromRequest(req)
require.NoError(t, err)
res := s.process(req, resp)
res := s.handle(req, resp)
require.Equal(t, 1, res)
o := resp.GetOneOption(dhcpv4.OptionDomainNameServer)
@ -431,7 +431,7 @@ func TestV4StaticLease_Get(t *testing.T) {
resp, err = dhcpv4.NewReplyFromRequest(req)
require.NoError(t, err)
assert.Equal(t, 1, s.process(req, resp))
assert.Equal(t, 1, s.handle(req, resp))
})
// Don't continue if we got any errors in the previous subtest.
@ -454,7 +454,7 @@ func TestV4StaticLease_Get(t *testing.T) {
resp, err = dhcpv4.NewReplyFromRequest(req)
require.NoError(t, err)
assert.Equal(t, 1, s.process(req, resp))
assert.Equal(t, 1, s.handle(req, resp))
})
require.NoError(t, err)
@ -513,7 +513,7 @@ func TestV4DynamicLease_Get(t *testing.T) {
resp, err = dhcpv4.NewReplyFromRequest(req)
require.NoError(t, err)
assert.Equal(t, 1, s.process(req, resp))
assert.Equal(t, 1, s.handle(req, resp))
})
// Don't continue if we got any errors in the previous subtest.
@ -547,7 +547,7 @@ func TestV4DynamicLease_Get(t *testing.T) {
resp, err = dhcpv4.NewReplyFromRequest(req)
require.NoError(t, err)
assert.Equal(t, 1, s.process(req, resp))
assert.Equal(t, 1, s.handle(req, resp))
})
require.NoError(t, err)