Unify Random and SecureRandom

[oprypin]

Followup to #3402

• Random now has a random_bytes method.
• SecureRandom now extends the Random module, so it can use all the normal RNG operations based on the cryptographic source.
• In order to have decent performance for this (to avoid calling urandom like 10 times for each generated number), now reimplementing random_bytes can be an alternative to implementing next_u.

[RX14]

It would be ideal if this worked the same was as IO#read.

[oprypin]

I don't understand

[RX14]

IO#read takes a slice of a fixed size and fills it. It's designed so that you can reuse the slice efficiently.

This takes a size and returns a slice of that size, which means that the allocation isn't controlled by the caller. I know why you do this for performance when filling it with integers, but I dislike the inconsistency.

[oprypin]

@RX14 oh, I've considered doing this (I think avoiding allocations could give even better performance actually), but in the end I think simplicity is more important. The consistency is with SecureRandom.random_bytes.

Anyway, I don't mind changing this, but I'm not sure if it's best. Leaving as is for now.

[lbguilherme]

Some buffering would be interesting here too. random_bytes could always get at least.. say.. 32 random bytes from the source. And then successive calls would use this buffer until it is depleted and it needs to be repopulated again. Anyway, this is an improvement for the future. +1 for this pull request.

[oprypin]

Buffering would solve many problems that were kind of worked around. But... that just seems not... secure. So I avoided it on purpose.

[RX14]

I think SecureRandom should become Random::Secure to be more consistent, now that it's a Random implementation.

[RX14]

I think it's a bit weird that SecureRandom doesn't include Random like Random::MT19937. I guess it's just because it doesn't need a seed?

[oprypin]

It's a module which can't be instantiated. I was really surprised that because extend is used, the SecureRandom module itself can be used exactly like an instance of a class including Random

[ysbaddaden]

I think there is a dichotomy.

SecureRandom is meant for cryptography, to generate a bunch of random bytes. For example to generate keys, tokens or unique identifiers that can't be forged.

Random is meant for generating a random number (not many bytes), having numbers informally distributed, and do it fast. For example to pick a random entry in an Array, or flush a deck of card.

I really don't see any use case for SecureRandom.rand(max) for example. Its pointless at best, maybe dangerous at worst (it's no longer secure).

I'm not against unifying the implementations, or to rename the module as Random::Secure, but I'm not sure considering urandom as Random implementation is a good idea.

[oprypin]

@ysbaddaden

What if someone wants to shuffle a deck of cards really well for an important game? What if someone wants a lot of random bytes for noise but the use case is not related to cryptography? Should they reimplement this functionality from scratch just because you say it's pointless?

it's no longer secure

?

[ysbaddaden]

First of all, thanks for improving Random. Really. I don't mean to be rude or anything. I'm concerned with security or the impression of security given by SecureRandom. Everything it provides must be cryptographically secure (hence why I reworked it to always use urandom, and will rewprk it to use arc4random on OpenBSD).

What if someone wants to shuffle a deck of cards really well for an important game?

I would seed a PRNG from a secure source, and choose a PRNG that returns uniformally distributed numbers. Scratch that, I would use a sort method that uniformally distributes cards in a deck.

?

Taking random bytes and applying a transformation to it makes it less secure, because it's now more predictable than it was before (you reduce the possibilities). Let's say you had a 0..65535 range, you reduced it to 0..10000, hence possibilities were divided by 6.5!

[RX14]

@ysbaddaden The "secure" from secure random comes from it being a sufficiently good source of entropy to perform cryptography with. I don't think that secure random implies that it's results are only to be used for cryptography or other security-like use cases where the amount of entropy matters.

In short I think that Random::Secure should ensure that the quality of entropy is secure, not attempt to enforce that the amount of entropy you draw out is "enough".

[oprypin]

I would seed a PRNG from a secure source, and choose a PRNG that returns uniformally distributed numbers.

Sure, that's what's done usually. But the PRNGs have small state (so results' randomness is limited) and produce only, say, 32-bit numbers (that's why this whole Random module is needed in the first place).

Let's say you had a 0..65535 range, you reduced it to 0..10000, hence possibilities were divided by 6.5!

No, if I have a 0...65536 range, I accept 0...60000 when the 0...10000 range is requested (well that was in the other merged PR)

[ysbaddaden]

I don't think that secure random implies that it's results are only to be used for cryptography

No, but cryptography quality trumps other usages.

[rayhamel]

Perhaps SecureRandom should be renamed to something else? HighEntropyRandom, RobustRandom, UnpredictableRandom or what have you. Cryptography is an area where you either know exactly what you're doing, or you really don't; a name like "SecureRandom" gives the latter group a false sense of "security" while they go about using secure functions insecurely.

[oprypin]

https://docs.python.org/3/library/random.html#random.SystemRandom

[oprypin]

@asterite Status?

[asterite]

@BlaXpirit At oredev, so I'll take a look at this next week :-)

[zatherz]

"I'll take a look at this next week"

[Sija]

@asterite ping?

[RX14]

It seems asterite has all but left the project recently :(

[ysbaddaden]

I'm as strongly opposed to this change as I used to be.

Please provide a Random::System alternative PRNG that will read bytes from the best system source, add Random#random_bytes(Slice). These are great improvements, which I'll merge right away. But pretty please don't touch SecureRandom, thanks!

[spalladino]

I back @ysbaddaden on this one, better to keep SecureRandom as is.

[oprypin]

Let me just say that I'm in total disbelief on the core team's response.

I do not intend to rework this into something that makes no sense, namely, providing access to system's random source through 2 different modules with an arbitrary split and with some overlapping functionality.

The revelation I had when writing extend Random was kinda worth it, but in the end missing such an opportunity just makes me sad.

[ysbaddaden]

I apologize for hard feelings, but usages are different.

System random doesn't mean secure (e.g. arc4random is secure on OpenBSD but isn't on FreeBSD).

Secure means suitable for cryptography usages. We musn't change the bytes fetched from the secure random source in any way. I prefer to be safe than sorry because algorithms were supposed to be good enough, but actually someone found later that it's not.

Hence my strong opinion on the topic.

[RX14]

If we do keep SecureRandom separate from Random, we should at least build SecureRandom on an instance of Random. Users must be able to access the exact same secure random source used for SecureRandom as a Random implementation. Sure we can document the class saying that just because you use the class doesn't mean your random code is secure, but it should be available to the people who do know what they're doing.

[ysbaddaden]

Yes, Random::System could preferably use a secure source.

[oprypin]

Superseded by #4450