dhcp: send 'server identifier' on request
The 'server identifier' option on a DHCPREQUEST must be the value of
the option sent by the server on the DHCPOFFER. Before this CL, we
were mistakenly using the siaddr field of the packet as the
identifier.
To cover this case in our unit tests, we fix our server to always
provide an siaddr of 0, and then require the client to send the
correct 'server identifier' option. (Just the server changes from
this CL cause the tests to fail.)
Also allow the DNS option to be any multiple of four, as DHCP
servers can send more than DNS server.
Change-Id: I40ff359f739c1129558e316edcee50de13e56446
diff --git a/dhcp/client.go b/dhcp/client.go
index b95b68f..4ba3fff 100644
--- a/dhcp/client.go
+++ b/dhcp/client.go
@@ -94,7 +94,7 @@
// On success, it adds a new address to this client's TCPIP stack.
// If the server sets a lease limit a timer is set to automatically
// renew it.
-func (c *Client) Request(ctx context.Context, requestedAddr tcpip.Address) error {
+func (c *Client) Request(ctx context.Context, requestedAddr tcpip.Address) (reterr error) {
if err := c.stack.AddAddress(c.nicid, ipv4.ProtocolNumber, "\xff\xff\xff\xff"); err != nil && err != tcpip.ErrDuplicateAddress {
return fmt.Errorf("dhcp: %v", err)
}
@@ -187,12 +187,16 @@
break
}
}
- if _, err := h.options(); err != nil {
+ opts, err := h.options()
+ if err != nil {
return fmt.Errorf("dhcp offer: %v", err)
}
var ack bool
var cfg Config
+ if err := cfg.decode(opts); err != nil {
+ return fmt.Errorf("dhcp offer: %v", err)
+ }
// DHCPREQUEST
addr := tcpip.Address(h.yiaddr())
@@ -202,9 +206,10 @@
}
}
defer func() {
- if !ack {
+ if !ack || reterr != nil {
c.stack.RemoveAddress(c.nicid, addr)
addr = ""
+ cfg = Config{Error: reterr}
}
c.mu.Lock()
@@ -227,7 +232,7 @@
h.setOptions([]option{
{optDHCPMsgType, []byte{byte(dhcpREQUEST)}},
{optReqIPAddr, []byte(addr)},
- {optDHCPServer, h.siaddr()},
+ {optDHCPServer, []byte(cfg.ServerAddress)},
})
if _, err := ep.Write([]byte(h), serverAddr); err != nil {
return fmt.Errorf("dhcp discovery write: %v", err)
@@ -250,7 +255,7 @@
break
}
}
- opts, err := h.options()
+ opts, err = h.options()
if err != nil {
return fmt.Errorf("dhcp ack: %v", err)
}
diff --git a/dhcp/dhcp.go b/dhcp/dhcp.go
index 09937b8..63d0184 100644
--- a/dhcp/dhcp.go
+++ b/dhcp/dhcp.go
@@ -16,6 +16,7 @@
// Config is standard DHCP configuration.
type Config struct {
+ Error error
ServerAddress tcpip.Address // address of the server
SubnetMask tcpip.AddressMask // client address subnet mask
Gateway tcpip.Address // client default gateway
@@ -27,7 +28,7 @@
*cfg = Config{}
for _, opt := range opts {
b := opt.body
- if l := opt.code.len(); l != -1 && l != len(b) {
+ if !opt.code.lenValid(len(b)) {
return fmt.Errorf("%s bad length: %d", opt.code, len(b))
}
switch opt.code {
@@ -41,7 +42,8 @@
case optDefaultGateway:
cfg.Gateway = tcpip.Address(b)
case optDomainNameServer:
- cfg.DomainNameServer = tcpip.Address(b)
+ // TODO(crawshaw): handle multiple DNS servers
+ cfg.DomainNameServer = tcpip.Address(b[:4])
}
}
return nil
@@ -179,17 +181,19 @@
optParamReq optionCode = 55
)
-func (code optionCode) len() int {
+func (code optionCode) lenValid(l int) bool {
switch code {
- case optSubnetMask, optDefaultGateway, optDomainNameServer,
+ case optSubnetMask, optDefaultGateway,
optReqIPAddr, optLeaseTime, optDHCPServer:
- return 4
+ return l == 4
case optDHCPMsgType:
- return 1
+ return l == 1
+ case optDomainNameServer:
+ return l%4 == 0
case optParamReq:
- return -1 // no fixed length
+ return true // no fixed length
default:
- return -1
+ return true // unknown option, assume ok
}
}
@@ -204,7 +208,7 @@
case optReqIPAddr:
return "option(request-ip-address)"
case optLeaseTime:
- return "option(least-time)"
+ return "option(lease-time)"
case optDHCPMsgType:
return "option(message-type)"
case optDHCPServer:
@@ -222,7 +226,7 @@
for _, opt := range opts {
if opt.code == optDHCPMsgType {
if len(opt.body) != 1 {
- return 0, fmt.Errorf("%s: bad length: %d", optDHCPMsgType, len(opt.body))
+ return 0, fmt.Errorf("%s: wrong length: %d", optDHCPMsgType, len(opt.body))
}
v := opt.body[0]
if v <= 0 || v >= 8 {
diff --git a/dhcp/dhcp_test.go b/dhcp/dhcp_test.go
index 1b9438b..9a23f78 100644
--- a/dhcp/dhcp_test.go
+++ b/dhcp/dhcp_test.go
@@ -134,11 +134,14 @@
var curAddr tcpip.Address
addrCh := make(chan tcpip.Address)
acquiredFunc := func(oldAddr, newAddr tcpip.Address, cfg Config) {
+ if err := cfg.Error; err != nil {
+ t.Fatalf("acquisition %d failed: %v", count, err)
+ }
if oldAddr != curAddr {
- t.Fatalf("aquisition %d: curAddr=%v, oldAddr=%v", curAddr, oldAddr)
+ t.Fatalf("aquisition %d: curAddr=%v, oldAddr=%v", count, curAddr, oldAddr)
}
if cfg.LeaseLength != time.Second {
- t.Fatalf("aquisition %d: lease length: %v, want %v", cfg.LeaseLength, time.Second)
+ t.Fatalf("aquisition %d: lease length: %v, want %v", count, cfg.LeaseLength, time.Second)
}
count++
curAddr = newAddr
diff --git a/dhcp/server.go b/dhcp/server.go
index 4f525e6..90ec04b 100644
--- a/dhcp/server.go
+++ b/dhcp/server.go
@@ -38,6 +38,9 @@
// NewServer creates a new DHCP server and begins serving.
// The server continues serving until ctx is done.
func NewServer(ctx context.Context, stack *stack.Stack, addrs []tcpip.Address, cfg Config) (*Server, error) {
+ if cfg.ServerAddress == "" {
+ return nil, fmt.Errorf("dhcp: server requires explicit server address")
+ }
s := &Server{
stack: stack,
addrs: addrs,
@@ -230,7 +233,21 @@
h.setOp(opReply)
copy(h.xidbytes(), hreq.xidbytes())
copy(h.yiaddr(), lease.addr)
- copy(h.siaddr(), s.cfg.ServerAddress)
+ copy(h.chaddr(), hreq.chaddr())
+ h.setOptions(opts)
+ s.ep.Write(buffer.View(h), &s.broadcast)
+}
+
+func (s *Server) nack(hreq header) {
+ // DHCPNACK
+ opts := options([]option{
+ {optDHCPMsgType, []byte{byte(dhcpNAK)}},
+ {optDHCPServer, []byte(s.cfg.ServerAddress)},
+ })
+ h := make(header, headerBaseSize+opts.len())
+ h.init()
+ h.setOp(opReply)
+ copy(h.xidbytes(), hreq.xidbytes())
copy(h.chaddr(), hreq.chaddr())
h.setOptions(opts)
s.ep.Write(buffer.View(h), &s.broadcast)
@@ -240,6 +257,21 @@
linkAddr := tcpip.LinkAddress(hreq.chaddr()[:6])
xid := hreq.xid()
+ reqopts, err := hreq.options()
+ if err != nil {
+ s.nack(hreq)
+ return
+ }
+ var reqcfg Config
+ if err := reqcfg.decode(reqopts); err != nil {
+ s.nack(hreq)
+ return
+ }
+ if reqcfg.ServerAddress != s.cfg.ServerAddress {
+ s.nack(hreq)
+ return
+ }
+
s.mu.Lock()
lease := s.leases[linkAddr]
switch lease.state {
@@ -267,7 +299,6 @@
h.setOp(opReply)
copy(h.xidbytes(), hreq.xidbytes())
copy(h.yiaddr(), lease.addr)
- copy(h.siaddr(), s.cfg.ServerAddress)
copy(h.chaddr(), hreq.chaddr())
h.setOptions(opts)
s.ep.Write(buffer.View(h), &s.broadcast)
