-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Unify Random and SecureRandom #3434
Conversation
@@ -56,6 +56,14 @@ module Random | |||
# The integers must be uniformly distributed between 0 and the maximal value for the chosen type. | |||
abstract def next_u : UInt | |||
|
|||
# Generates a slice filled with *n* random bytes. | |||
def random_bytes(n : Int) : Bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal if this worked the same was as IO#read
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buffering would solve many problems that were kind of worked around. But... that just seems not... secure. So I avoided it on purpose.
# SecureRandom.uuid # => "c7ee4add-207f-411a-97b7-0d22788566d6" | ||
# | ||
# SecureRandom.rand(10000) # => 4264 | ||
# [1, 2, 3].shuffle(SecureRandom) # => [1, 3, 2] | ||
# ``` | ||
# | ||
# The implementation follows the | ||
# [libsodium sysrandom](https://github.com/jedisct1/libsodium/blob/6fad3644b53021fb377ca1207fa6e1ac96d0b131/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c) | ||
# implementation and uses `getrandom` on Linux (when provided by the kernel), | ||
# then tries to read from `/dev/urandom`. | ||
module SecureRandom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SecureRandom
should become Random::Secure
to be more consistent, now that it's a Random
implementation.
# ``` | ||
# | ||
# The implementation follows the | ||
# [libsodium sysrandom](https://github.com/jedisct1/libsodium/blob/6fad3644b53021fb377ca1207fa6e1ac96d0b131/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c) | ||
# implementation and uses `getrandom` on Linux (when provided by the kernel), | ||
# then tries to read from `/dev/urandom`. | ||
module SecureRandom | ||
extend Random |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 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. |
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?
? |
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).
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! |
@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 |
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
No, if I have a |
No, but cryptography quality trumps other usages. |
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. |
@asterite Status? |
@BlaXpirit At oredev, so I'll take a look at this next week :-) |
"I'll take a look at this next week" |
@asterite ping? |
It seems asterite has all but left the project recently :( |
I'm as strongly opposed to this change as I used to be. Please provide a |
I back @ysbaddaden on this one, better to keep SecureRandom as is. |
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 |
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. |
If we do keep |
Yes, |
Superseded by #4450 |
Followup to #3402
Random
now has arandom_bytes
method.SecureRandom
now extends theRandom
module, so it can use all the normal RNG operations based on the cryptographic source.urandom
like 10 times for each generated number), now reimplementingrandom_bytes
can be an alternative to implementingnext_u
.