Use crypto/rand for random id generation.

[jsha]

Notably, if crypto/rand returns an error (very unlikely) this will panic.

Fixes #1043 and #1037

[codecov-io]

Codecov Report

Merging #1044 into master will decrease coverage by 0.07%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
- Coverage   55.05%   54.98%   -0.08%     
==========================================
  Files          41       41              
  Lines        9879     9872       -7     
==========================================
- Hits         5439     5428      -11     
- Misses       3419     3422       +3     
- Partials     1021     1022       +1

Impacted Files	Coverage Δ
msg.go	76.55% <60%> (-0.95%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22cda6d...a509d69. Read the comment docs.

[FiloSottile]

crypto/rand basically never returns an error, unless someone set up a super-strict sandbox, in which case they probably have opinions that can be encoded as a custom Id function. I'd just go ahead and make it a panic, that's what I did in the version in the Google monorepo.

[tmthrgd]

I like this approach and it's a nice simplification. Just a few minor nits.

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Use crypto/rand for..." ]

tmthrgd requested changes on this pull request. I like this approach and it's a nice simplification. Just a few minor nits. > - // seeding idRand upon the first call to id. - - var seed int64 - var buf [8]byte - - if _, err := crand.Read(buf[:]); err == nil { - seed = int64(binary.LittleEndian.Uint64(buf[:])) - } else { - seed = rand.Int63() - } - - idRand = rand.New(rand.NewSource(seed)) + var v uint16 + err := binary.Read(crand.Reader, binary.BigEndian, &v) + if err != nil { + panic(fmt.Sprintf("failed to get randomness: %s", err)) This panic message is off. It should be something like: `panic("dns: reading random id failed: " + err.Error())`. > - if idRand == nil { - // This (partially) works around - // golang/go#11833 by only - // seeding idRand upon the first call to id. - - var seed int64 - var buf [8]byte - - if _, err := crand.Read(buf[:]); err == nil { - seed = int64(binary.LittleEndian.Uint64(buf[:])) - } else { - seed = rand.Int63() - } - - idRand = rand.New(rand.NewSource(seed)) + var v uint16 I think `var id uint16` would be better, but that's very minor. > @@ -15,10 +15,8 @@ import ( "encoding/binary" "fmt" "math/big" - "math/rand" With this import gone, we can import crypto/rand without renaming it. So `crand "crypto/rand"` -> `"crypto/rand"`. > @@ -73,53 +71,23 @@ var ( ErrTime error = &Error{err: "bad time"} // ErrTime indicates a timing error in TSIG authentication. ) -// Id by default, returns a 16 bits random number to be used as a -// message id. The random provided should be good enough. This being a -// variable the function can be reassigned to a custom function. -// For instance, to make it return a static value: +// Id by default, returns a 16 bit random number (from crypto/rand) to +// be used as a message id. This being a variable the function can be I think I'd rather this be worded something like: "Id by default returns a 16-bit random number to be used as a message id. The number is drawn from a cryptographically secure random number generator."

Can you post a follow up PR with these changes? (I can do it as well). Thanks!

[yongtang]

Looks like all review comments have been covered, @miekg @tmthrgd can you check to see if this PR is ready to go?

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Use crypto/rand for..." ]

Looks like all review comments have been covered, @miekg @tmthrgd can you check to see if this PR is ready to go?

Ah, let me fix the nits from Tom. I though I merged, but I only pressed 'lgtm', my mistake

[miekg]

merging and doing a small follow up PR; this allows me to tag a release where this at least is fixed.