Default to math/rand in all places, add RandRead option

[stapelberg]

I am aware of PR #13 and the proposed branch, but they don’t go far enough: not only the crypto/rand usage in pktsock_linux.go needs to go, but also the crypto/rand reads for generating xids.

See the commit messages for more detailed explanations, but I’d like to stress two things in the discussion of this PR:

1. this problem is made significantly worse with Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=43838a23a05fbd13e47d750d3dfd77001536dd33 (present in Linux ≥ 4.16), which results in crypto/rand blocking on early boot until the CRNG is securely initialized. On my machine, the DHCP client would hang for minutes until sending out the first request.

2. I don’t see the need for an option on the pktconn as per the explanation in my commit message: given that the IP identifier field is only used for fragmentation, which DHCP never uses, we might as well set a static value. Using math/rand is strictly better, accepting an option to specify this behavior seems overkill to me :)

Thanks for your consideration!

[stapelberg]

I didn’t see commit https://github.com/d2g/dhcp4client/commit/6e570ed0a266b730a860ba1068090f683b2c213a (its title made me believe it only gofmt'ed) earlier, so let me rebase this first.

[stapelberg]

Updated the PR to make generateXID fall back to a seeded (*math/rand.Rand).Read, so this should be ready for review now.

[d2g]

Really like the idea, I hadn't thought about including the mac address in the seed.

By using the non global version of the Rand do we introduce any issues where we have concurrency generating an XID? I've not thought of the implications. If you've considered it let me know otherwise is it safer to mutex it?

On the other branch, have you had a look through? The majority of it is operating as it does currently although I've not have much success with renewals.

Thanks for the feedback on the commit message. Between that the lack of documentation etc I must do better. Thanks for helping us get there.

[stapelberg]

By using the non global version of the Rand do we introduce any issues where we have concurrency generating an XID? I've not thought of the implications. If you've considered it let me know otherwise is it safer to mutex it?

I wasn’t sure about the goroutine safeness of dhcp4client.Client — is the intention to have it goroutine-safe? I wasn’t sure since it wasn’t explicitly called out (we should address that after reaching a conclusion), but also I can’t really see the use-case for manipulating the Client from multiple goroutines, so I just assumed it was not goroutine-safe.

On the other branch, have you had a look through? The majority of it is operating as it does currently although I've not have much success with renewals.

Do you mean https://github.com/d2g/dhcp4client/commits/%2313LinuxRandUpdate? The only change is the introduction of the random function to pktconn, AFAICT. As I mentioned earlier, I don’t quite see the benefit of that flexibility, so I’d suggest to keep it out until we have an actual use-case.

Thanks for the feedback on the commit message. Between that the lack of documentation etc I must do better. Thanks for helping us get there.

Thanks, much appreciated! If you’d like, I could send another PR to add some more godoc.

By the way, I see some commits which fix compilation on Linux. Given Go’s excellent cross-compilation support, maybe you’d like to run GOOS=linux go build github.com/d2g/dhcp4client/... as part of your testing to catch these issues locally :)

[d2g]

I wasn’t sure about the goroutine safeness

Me neither. Although generating a xid currently is might as well keep it that way?

Thanks, much appreciated! If you’d like, I could send another PR to add some more godoc.

All PR are welcome. I really appreciate it as I don't get much time to move things forward.

[stapelberg]

Me neither. Although generating a xid currently is might as well keep it that way?

Fair enough! Done.