Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: crypto/rand: Add an implementation for math/rand's Source #25531

Closed
wking opened this issue May 24, 2018 · 7 comments
Closed

proposal: crypto/rand: Add an implementation for math/rand's Source #25531

wking opened this issue May 24, 2018 · 7 comments

Comments

@wking
Copy link
Contributor

wking commented May 24, 2018

math/rand provides a more extensive API than crypto/rand. For folks who want the math/rand functionality fed by cryptographically secure random, it would be convenient if the crypto package exposed an implementation of math/rand's Source. I've stubbed out a proof-of-concept here.

There's tangentially-related source-swapping discussion #21835, although I don't see any discussion there about adjusting the Source interface itself (just the implementation). Having non-panic ways to return errors would be nice.

@gopherbot gopherbot added this to the Proposal milestone May 24, 2018
@cespare
Copy link
Contributor

cespare commented May 24, 2018

The usual objection to this historically is that cryptographically secure random integers, floats, etc are rarely needed in real cryptographic constructs, and so providing them in the standard library would only encourage people to abuse crypto/rand to do unsound and insecure things.

There has been previous discussion about this but I can't locate the relevant issue(s) at the moment.

/cc @FiloSottile

@wking
Copy link
Contributor Author

wking commented May 24, 2018

I'm coming at this because I don't see an easy way to implement "give me a slice with $n random characters from $charset" (kubernetes-retired/bootkube#984). math/rand's Int31n per-character would be secure, if not particularly efficient. And for the Bootkube use-case, efficiency is not terribly important.

To be more efficient, you'd need an Intns(max int, count int) []int (with a better name?), but one thing at a time ;).

@slrz
Copy link

slrz commented May 24, 2018

Read a slice of random bytes and translate them to the desired alphabet, using a base64-style approach? That is adaptable (and has been adapted) to all kinds of alphabets of different radices, including those not a power of two.

@uluyol
Copy link
Contributor

uluyol commented May 24, 2018

This is already very easy to implement using the standard library

func RandInt32() (int32, error) {
    var buf [4]byte
    _, err := rand.Read(buf[:])
    return int32(binary.LittleEndian.Uint32(buf[:])), err
}

@wking
Copy link
Contributor Author

wking commented May 24, 2018

int32(binary.LittleEndian.Uint32(buf[:])) 

This doesn't work as well with base 36 (or other bases that aren't powers of two). For those, as far as I can tell, you need to either throw out the remainder and re-draw (like this) or convert the base 256 input into your target base (e.g. via math/big, like this).

Re-drawing is inefficient, especially since the modulus based approach is already inefficient. You can end up drawing multiple bytes to get an unbiased draw from [0,36).

Converting to the target base is awkward too (unless there's some helper function that I'm missing?). See maintainer feedback on kubernetes-retired/bootkube#984 as evidence of that.

But in general, yeah, this could be handled outside of crypto/rand by a helper that converted bytes to your target base (which is basically what I have in kubernetes-retired/bootkube#984 now, although the current implementation swaps endian-ness). Where would that go in the stdlib? encoding/binary?

@wking
Copy link
Contributor Author

wking commented May 24, 2018

I've filed #25534 with a proposed Choose helper for random token generation in arbitrary bases. If this proposal is blocked on rarely needing a crypto source for the Rand methods, maybe we can table this until the Choose proposal settles?

@rsc
Copy link
Contributor

rsc commented Jun 4, 2018

Like in #25534, this seems fairly special-purpose. It is OK for functionality to exist in packages outside the standard library. Not every conceivable use has to be a function in the standard library.

Furthermore, I would push back on anything that further blurs the line between crypto/rand and math/rand. Adding math/rand.Reader was arguably already a mistake. Let's not make more.

@rsc rsc closed this as completed Jun 4, 2018
@golang golang locked and limited conversation to collaborators Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants