Renewal packet sometimes goes to wrong server

[tomkcook]

In client.go:308, the server address for a renewal request is set to the SIAddr field from the previous request. In certain cases, this is wrong.

The SIAddr field is the server address to use for the next step in DHCP, which after a DISCOVER/OFFER/REQUEST/ACK sequence is to retrieve a PXE boot image. So if a network uses different hosts for DHCP and PXEboot, the ACK packet's SIAddr field will point to the PXEboot server, not the DHCP server, and any renewal request sent to this server will fail.

The correct thing would be to record the address the UDP packet containing the ACK came from and use that as the server address for renewal.

[tomkcook]

Actually, scratch that, SIAddr should not be set at all in RENEW state. See RFC2131 ss 4.3.2:

 o DHCPREQUEST generated during RENEWING state:

  'server identifier' MUST NOT be filled in, 'requested IP address'
  option MUST NOT be filled in, 'ciaddr' MUST be filled in with
  client's IP address. In this situation, the client is completely
  configured, and is trying to extend its lease. This message will
  be unicast, so no relay agents will be involved in its
  transmission.  Because 'giaddr' is therefore not filled in, the
  DHCP server will trust the value in 'ciaddr', and use it when
  replying to the client.

So lines 308, 312 and 313 of client.go are all contrary to the specification.

[d2g]

Agreed.

I think this has already been addressed in V2 branch (WIP)

[tomkcook]

Confirmed that the below works (at least with VirtualBox's NAT DHCP server):

    dhcp4client.Broadcast(false)(client)
    renewPkt := dhcp4.NewPacket(dhcp4.BootRequest)
    renewPkt.SetCHAddr(ack.CHAddr())
    mid, err := generateMessageId()
    if err != nil {
            return nil, nil, err
    }
    renewPkt.SetXId(mid)
    renewPkt.SetCIAddr(ack.YIAddr())
    renewPkt.AddOption(dhcp4.OptionDHCPMessageType, []byte{byte(dhcp4.Request)})
    renewPkt.PadToMinSize()

    err = client.SendPacket(renewPkt)
    if err != nil {
            DEBUG.Printf("Error sending renewal packet: %s\n", err.Error())
            return nil, nil, err
    }

    newAck, err := client.GetAcknowledgement(&renewPkt)
    if err != nil {
            DEBUG.Printf("Error getting renewal acknowledgement: %s\n", err.Error())
            return newAck, nil, err
    }

    options = newAck.ParseOptions()

[d2g]

Have you managed to get this working? Renewals have generally been causing me issues.

[d2g]

Switched the default branch to version 2 which has renews working based on the server identifier in the DHCP packet.