Just like in the DHCPv4 client, packets have to go uniquely through the designated network interface

This can help compare and validate the original packet in cases where the packet is malformed (e.g. options after the END option in a DHCPv4 packet)

This uses the underlying netlink.AddrReplace, however it's adding an address. Replace it instead

I was refactoring some of the tests to take advantage of stretchr/testify/assert and ran into an interesting bug. It turns out the DHCPv4 options in dhcpv4/types.go are not all OptionCodes - only the first one is. The rest of them are ints due to the way the initialization works.

I forsee two fixes:

1. Prepend OptionCode before each =
2. Change the first line to OptionPad = iota and figure out what to do for the gaps in the range due to e.g. returned options.

This will allow finer-grained imports for who only needs a subset of the functionalities (e.g. no need for server or client)

Currently we have:

func (d *DHCPv4) UserClass() *UserClass {

I think this can be changed to:

func (d *DHCPv4) UserClass() []string {

The dhcpv4 and dhcpv6 packages use two different conventions for option parsing. In dhcpv4 the option is parsed along with option code and length, in dhcpv6 these are handled earlier, and the Parse* methods only handle the data, assuming that the option code is the same as the used option.

Let's uniform the two interfaces for consistency.

The sockets for broadcast should be created with a syscall instead of through standard ListenUDP function. The synchronous client already provides such functionality, but the async one can only send unicast requests.

This function is only used by the bsdp package, and is not in sync with sendReceive.

cc @get9

In the DHCPv6 client struct LocalAddr and RemoteAddr are declared to be of the net.Addr type, but wherever they are used the code attempts to cast them to net.UDPAddr and return an error on failure.

Isn't it better to change the struct and have the compiler do this check at compile time instead?

The standard client could be rewritten to use the asynchronous client:

1. open async client
2. send a packet
3. block on execution using WaitTimeout(...)

Just like in https://blog.golang.org/examples

As discussed in #71 , it is worth considering moving away from assertion libraries like the one we use, github.com/stretchr/testify.

In a recent PR I added a loop in dhcpv6/client.go to wait until LocalAddr != nil. While the risk is low, this loop doesn't have a break condition if the operation takes too much time. It's a good idea to add a deadline and break if passed.

CC @get9

https://blog.golang.org/examples

re #241

At the moment it tries with no pause and this is bad.

Label compression is not implemented, I have hit a DHCP server that responds with compressed labels and the library failed to parse the response

Seems like the dhcpv4 package isn't as fleshed-out as the dhcpv6 package. A couple improvements:

• dhcpv4.Client has a Timeout field... but doesn't actually do anything with it
• Couple things could be made as interfaces so they're a little more flexible.

Anything else?

Currently the clients only listen for 4 packets. In some networks there are multiple DHCP servers, so simply waiting for 4 packets is not robust, need to check for the message type until found what expected, or timed out

This is my first time delving into the internals of DHCP so forgive me if this is noise. I think I'm seeing a bug in this library. I have reserved an IP and hostname for a machine I have and using this library I am trying to set the hostname.

func Setup(platform string) (err error) {
	link, err := netlink.LinkByName("lo")
	if err != nil {
		return err
	}
	if err = netlink.LinkSetUp(link); err != nil {
		return err
	}

	ifname := "eth0"
	if _, err = netboot.IfUp(ifname, 5*time.Second); err != nil {
		return err
	}
	modifiers := []dhcpv4.Modifier{dhcpv4.WithRequestedOptions(dhcpv4.OptionHostName)}
	switch platform {
	case "GoogleCloud":
		modifiers = append(modifiers, dhcpv4.WithRequestedOptions(dhcpv4.OptionClasslessStaticRouteOption))
	}
	var netconf *netboot.NetConf
	if netconf, err = dhclient4(ifname, modifiers...); err != nil {
		return err
	}
	if err = netboot.ConfigureInterface(ifname, netconf); err != nil {
		return err
	}

	return nil
}

func dhclient4(ifname string, modifiers ...dhcpv4.Modifier) (*netboot.NetConf, error) {
	attempts := 10
	client := client4.NewClient()
	var (
		conv []*dhcpv4.DHCPv4
		err  error
	)
	for attempt := 0; attempt < attempts; attempt++ {
		log.Printf("requesting DHCP lease: attempt %d of %d", attempt+1, attempts)
		conv, err = client.Exchange(ifname, modifiers...)
		if err != nil && attempt < attempts {
			log.Printf("failed to request DHCP lease: %v", err)
			time.Sleep(1 * time.Second)
			continue
		}
		break
	}

	for _, m := range conv {
		log.Println(m.Summary())
		if m.OpCode == dhcpv4.OpcodeBootReply && m.MessageType() == dhcpv4.MessageTypeOffer {
			if m.YourIPAddr != nil {
				log.Printf("using IP address %s", m.YourIPAddr.String())
			}

			if m.HostName() != "" {
				log.Printf("using offered hostname: %s", m.HostName())
				if err = unix.Sethostname([]byte(m.HostName())); err != nil {
					return nil, err
				}
			} else {
				return nil, errors.New("a hostname was not offered from the DHCP server")
			}

			break
		}
	}

	netconf, _, err := netboot.ConversationToNetconfv4(conv)
	if err != nil {
		return nil, err
	}

	return netconf, err
}

results in

[   13.007555] [talos] [initramfs] DHCPv4 Message
[   13.007555]   opcode: BootRequest
[   13.007555]   hwtype: Ethernet
[   13.007555]   hopcount: 0
[   13.007555]   transaction ID: 0x7efdd29c
[   13.007555]   num seconds: 0
[   13.007555]   flags: Broadcast (0x8000)
[   13.007555]   client IP: 0.0.0.0
[   13.007555]   your IP: 0.0.0.0
[   13.007555]   server IP: 0.0.0.0
[   13.007555]   gateway IP: 0.0.0.0
[   13.007555]   client MAC: 52:54:00:a8:4c:e1
[   13.007555]   server hostname:
[   13.007555]   bootfile name:
[   13.007555]   options:
[   13.007555]     DHCP Message Type: DISCOVER
[   13.007555]     Parameter Request List: Subnet Mask, Router, Domain Name Server, Host Name, Domain Name
[   13.007555]
[   13.060144] [talos] [initramfs] DHCPv4 Message
[   13.060144]   opcode: BootReply
[   13.060144]   hwtype: Ethernet
[   13.060144]   hopcount: 0
[   13.060144]   transaction ID: 0x7efdd29c
[   13.060144]   num seconds: 0
[   13.060144]   flags: Broadcast (0x8000)
[   13.060144]   client IP: 0.0.0.0
[   13.060144]   your IP: 192.168.124.200
[   13.060144]   server IP: 192.168.124.1
[   13.060144]   gateway IP: 0.0.0.0
[   13.060144]   client MAC: 52:54:00:a8:4c:e1
[   13.060144]   server hostname:
[   13.060144]   bootfile name:
[   13.060144]   options:
[   13.060144]     Subnet Mask: ffffff00
[   13.060144]     Router: 192.168.124.1
[   13.060144]     Domain Name Server: 192.168.124.1
[   13.060144]     Host Name: localhost
[   13.060144]     Interface MTU: [5 220]
[   13.060144]     Broadcast Address: 192.168.124.255
[   13.060144]     IP Addresses Lease Time: 12h0m0s
[   13.060144]     DHCP Message Type: OFFER
[   13.060144]     Server Identifier: 192.168.124.1

Notice that the hostname is localhost. In my router it is master-1. If not a bug, any help would be much appreciated!

Consider making the dhcpv6.Future generic or using an external library

crypto/rand is blocking, while math/rand is not. This blocks the generation of the TransactionID until the machine has enough randomness in the pool. This has been the result of a fun troubleshooting :)

While working on a demo on qemu I found that the DHCPv6 client was apparently stuck. It worked sometimes, it didn't other times. SLAAC was fine and I could see traffic through it. It suddenly started working after a while. The function that generates the TransactionID depends on crypto/rand, that is blocking.

@hugelgupf suggests that Qemu's VirtIORNG could be used, to avoid swapping crypto/rand for math/rand , https://wiki.qemu.org/Features/VirtIORNG .

I would like to discuss in this issue whether it makes sense to enforce a cryptographic RNG or not for Transaction ID generation.

It would be nice to check for errors when applying modifiers.

For instance:

- solicit = mod(solicit)
+ solicit, err = mod(solicit)
+ if err != nil { return ..., err }

rfc5970 specifies that multiple client arch types can be specified.

The client MAY use this option to send a list of supported
architecture types to the server

The library is assuming only one is specified.

As explained at https://help.github.com/articles/signing-commits-using-gpg/

gometalinter has some pretty high signal about various things that can be improved/fixed, including lots of static analysis on the code. The only one that I think we should probably not run is the cyclomatic complexity linter because there are a lot of spurious warnings (particularly around the parsing func's that have a lot of switch branches).

Commit c6894ea broke my project https://github.com/rtr7/router7

I realize that currently, this repository does not have any tagged versions.

As per https://research.swtch.com/vgo-module, could you please start releasing a new major version for breaking changes?

Thanks for considering,

Can't seem to build or run this project on macOS/darwin due to the syscall.BindToDevice. Searching around leads me to the SO_BINDTODEVICE sockopt which isn't present on darwin. Instead, others have used a variety of workarounds:

• https://trac.transmissionbt.com/ticket/2313
• http://atastypixel.com/blog/the-making-of-talkie-multi-interface-broadcasting-and-multicast/

The exdhcp contains examples, but we also need some documentation on the main README

I am writing an example of dhcpv4 server for that library based on krolaw implementation and there are missing several base functions like:

NewOfferFromRequest
NewAckFromRequest

Actually I made them on top of the basic NewReplyFromRequest, but believe they should be inside of the library.

Do you have a plan to add them? Would you like to get a pull request?

P.S. There are missing NewOffer, NewAcknowledge function also mention in comments. But i don't care now about client.

The Go linter prefers CamelCase rather than UPPERCASE_AND_UNDERSCORE names. A lot of variables in the code follow the latter. Let's make golint happier

Similar to the dhcpv6 we may want to add the v4 asynchronous client

Just like in v6 (#38), do the same for dhcpv4

I have switched finally my test server implementation from https://github.com/krolaw/dhcp4/conn to this library and found that it is not serving incoming requests properly. Current implementation is strightforward derived from the example from book Network programming in Go. I looks like krolaw spent enough time to create working bindings for servers in different OS-es.

What is the problem? When you bind server using net.ListenUDP("udp4", &s.localAddr) with specific local address it cannot capture packet at all. If you set localAddr = 0.0.0.0 it can capture packets, but from all system's interfaces of course, which isn't acceptable for real operation. The workaround is to bind to all interfaces with address 0.0.0.0 but make a filter with interface ID and discard unnecessary packets on low level. Here is exactly that this code does: https://github.com/krolaw/dhcp4/blob/master/conn/filter.go

For linux they are using sys call option: syscall.SetsockoptString(s, syscall.SOL_SOCKET, syscall.SO_BINDTODEVICE, interfaceName) For FreeBSD there is a long story about introduction of IP_SENDIF option as well as IP_RECVIF which is not completed yet.

In general it is reasonable working approach, so I wonder if we can adopt it until find better solution?

Regarding this particular issue there are two question from me:

1. Could we resolve this issue: #231 without waiting any solutions for FreeBSD/MacOS?

2. Is it acceptable to implement krolaw/dhcp4/conn solution for server bindings?

Request messages are expected to have the same options as Solicit. This is not currently happening.

The approach I want to use is to pass optional modifier functions that modify DHCP messages, something like:

sol, err := NewSolicitForInterface("eth0",
    WithBootFileURL("..."),
    WithWhateverOtherThingIHaveInMind(...))
...

A single broken option can break the whole encoding/decoding workflow. All the options have to be rigorously tested to avoid this.

The rfc3315 mentions the following:

The data area of the user class option MUST contain one or more
instances of user class data. Each instance of the user class data
is formatted as follows:

  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-...-+-+-+-+-+-+-+
  |        user-class-len         |          opaque-data          |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-...-+-+-+-+-+-+-+

The user-class-len is two octets long and specifies the length of the
opaque user class data in network byte order.

I think the library is currently only considering the case of having only 1 user class data.

There is a bunch of duplicate code in functions like NewReplyFromRenew, NewReplyFromRequest, etc. Let's minimize the duplicate code.

As done in dhcpv6 and dhcpv4, let's add modifiers to the packet generation. Assigned to @get9 as our BSDP expert :)

I am only opening this to discuss whether we should move the project to github.com/coredhcp/dhcp.

The cons are that we already have the issues, stars and metrics in the current repo.

Shouldn't this be instead in the dhcpv4 package? cc @get9

There are several parts (client, server) that can't be easily tested with unit tests in Go. Let's add integration tests to fill the gap

The most used netlink library out there kind of plays fast and loose with unsafe pointers, sometimes leading to memory corruption.

(E.g. https://github.com/vishvananda/netlink/blob/master/ioctl_linux.go#L89 - should be using binary marshaling instead, since e may be freed anytime after - luckily, it gets returned and used after Ifreq is given to a syscall, but this kind of stuff is all over that codebase.)

It's also a huge grab-bag of netlink code, which means it's quite a large dependency in our embedded environment. I'd like to recommend switching to a netlink library based on https://github.com/mdlayher/netlink, such as https://godoc.org/github.com/jsimonetti/rtnetlink, which is more targeted (and has no memory corruption).

I'm happy to do this switch when I get to these items.

After the changes in PR #227, this won't work:

	for k := range pkt.Options {
		switch k {

		case dhcpv4.OptionDHCPMessageType:

This doesn't compile because the key of the map is a uint8.

Should the key be changed to an OptionCode?

cc @hugelgupf

go build
# github.com/insomniacslk/dhcp/dhcpv4
../../insomniacslk/dhcp/dhcpv4/client.go:125:25: undefined: unix.AF_PACKET
../../insomniacslk/dhcp/dhcpv4/client.go:125:68: undefined: unix.ETH_P_IP
../../insomniacslk/dhcp/dhcpv4/client.go:133:12: undefined: unix.SockaddrLinklayer

There are no constants in package golang.org/x/sys/unix for darwin_amd64

grep AF_PACKET *
grep: linux: Is a directory
syscall_linux.go:// SockaddrLinklayer implements the Sockaddr interface for AF_PACKET type sockets.
syscall_linux.go:	sa.raw.Family = AF_PACKET
syscall_linux.go:	case AF_PACKET:
zerrors_linux_386.go:	AF_PACKET                            = 0x11
zerrors_linux_amd64.go:	AF_PACKET                            = 0x11
zerrors_linux_arm.go:	AF_PACKET                            = 0x11
zerrors_linux_arm64.go:	AF_PACKET                            = 0x11
zerrors_linux_mips.go:	AF_PACKET                            = 0x11
zerrors_linux_mips64.go:	AF_PACKET                            = 0x11
zerrors_linux_mips64le.go:	AF_PACKET                            = 0x11
zerrors_linux_mipsle.go:	AF_PACKET                            = 0x11
zerrors_linux_ppc64.go:	AF_PACKET                            = 0x11
zerrors_linux_ppc64le.go:	AF_PACKET                            = 0x11
zerrors_linux_riscv64.go:	AF_PACKET                            = 0x11
zerrors_linux_s390x.go:	AF_PACKET                            = 0x11
zerrors_linux_sparc64.go:	AF_PACKET                        = 0x11
zerrors_solaris_amd64.go:	AF_PACKET                     = 0x20

This is a commit probably breaks compilation:
4eedccf

In BSD system AF_PACKET should be replaced by AF_LINK

BTW, why not to move client for separate package? I just removed file client.go from repo in order to compile server code.

I have been working on implementing RequestNetbootv4 and stumbled across some issues with the way sockets are used in the v4 part of the library.

If one starts from an interface that does not have any configuration (i.e. no IP, no routes), the attempt to broadcast the DISCOVER fails with ENETUNREACH. The client socket is an AF_INET, SOCK_RAW, IPPROTO_RAW socket, and without any route, even raw traffic gets dropped at layer 3. One possible way around this is to set SO_BINDTODEVICE on the device which is being configured, but this solves only the send part of the communication.

The receive socket, without any default route with link scope, does not see the incoming OFFER. I definitely see the OFFER coming up at layer 2 (tcpdump does see it), but it never gets to the socket.

Adding a default route with link scope before initiating the transaction fixes the problem.

I had a quick look at dhclient from ISC and from what I can see it uses PF_PACKET sockets, so it works at layer 2. I am sending a pull request with a RequestNetbootv4 implementation, but it's certainly not great as if a default route is not added beforehand on an unconfigured interface, the transaction fails with ENETUNREACH. As I mentioned above, even when binding the sender socket to the interface, the receiving end does not see the broadcasted OFFER. Possible ways to fix this:

• The library might try to be smart and check if a route is present, and if not add it (my RequestNetbootv4 implementation does not do it).
• Move to layer 2 sockets

The idea behind returning a "conversation" in the clients is that it will hold all the packets that have been sent (or tried to send) and received.
The DHCPv6 client doesn't return the Solicit packet if the transaction solicit/advertise fails, but it should.

I've wanted to rewrite my example DHCP server with recently added server.go implementation but haven't found no library code for sending packet back to network.

On the example code sending function was provided in user handler code, but actually this function is more complicated and should have a deal with different OSes, socket options and should be outside of user code in library implementation. So proposal is to make handler work only with packet manipulation, not network binding.

-- type Handler func(conn net.PacketConn, peer net.Addr, m *DHCPv4)

++ type Handler func(peer net.Addr, request *DHCPv4)(*DHCPv4, error) {

    r, err := dhcpv4.NewReplyFromRequest(request)
    msgType := request.MessageType()
    switch msgType {
        case dhcpv4.MessageTypeDiscover:
    ...
    }
-- if _, err := conn.WriteTo(r.ToBytes(), peer); err != nil {
-- log.Printf("Cannot reply to client: %v", err)
-- }
++ return r, err    
}

Rest part of initial proposal is omitted for now

The user class parsing added in PR #90 does not follow the RFC:

The value in UC_Len_i does not include the length field itself and MUST be non-zero.