-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Unify Random and SecureRandom #3434
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| require "random" | ||
| require "base64" | ||
|
|
||
| {% if flag?(:linux) %} | ||
|
|
@@ -10,16 +11,23 @@ require "base64" | |
| # | ||
| # Examples: | ||
| # ```crystal | ||
| # SecureRandom.base64 # => "LIa9s/zWzJx49m/9zDX+VQ==" | ||
| # SecureRandom.hex # => "c8353864ff9764a39ef74983ec0d4a38" | ||
| # SecureRandom.uuid # => "c7ee4add-207f-411a-97b7-0d22788566d6" | ||
| # SecureRandom.random_bytes(8) # => Bytes[141, 161, 130, 39, 247, 150, 68, 233] | ||
| # SecureRandom.base64 # => "LIa9s/zWzJx49m/9zDX+VQ==" | ||
| # SecureRandom.hex # => "c8353864ff9764a39ef74983ec0d4a38" | ||
| # 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
| extend Random | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a bit weird that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 self | ||
|
|
||
| @@initialized = false | ||
|
|
||
| # Generates *n* random bytes that are encoded into Base64. | ||
|
|
@@ -29,7 +37,7 @@ module SecureRandom | |
| # ```crystal | ||
| # SecureRandom.base64(4) # => "fK1eYg==" | ||
| # ``` | ||
| def self.base64(n : Int = 16) : String | ||
| def base64(n : Int = 16) : String | ||
| Base64.strict_encode(random_bytes(n)) | ||
| end | ||
|
|
||
|
|
@@ -42,7 +50,7 @@ module SecureRandom | |
| # SecureRandom.urlsafe_base64(8, true) # => "vvP1kcs841I=" | ||
| # SecureRandom.urlsafe_base64(16, true) # => "og2aJrELDZWSdJfVGkxNKw==" | ||
| # ``` | ||
| def self.urlsafe_base64(n : Int = 16, padding = false) : String | ||
| def urlsafe_base64(n : Int = 16, padding = false) : String | ||
| Base64.urlsafe_encode(random_bytes(n), padding) | ||
| end | ||
|
|
||
|
|
@@ -54,17 +62,21 @@ module SecureRandom | |
| # SecureRandom.hex # => "05f100a1123f6bdbb427698ab664ff5f" | ||
| # SecureRandom.hex(1) # => "1a" | ||
| # ``` | ||
| def self.hex(n : Int = 16) : String | ||
| def hex(n : Int = 16) : String | ||
| random_bytes(n).hexstring | ||
| end | ||
|
|
||
| def next_u : UInt8 | ||
| random_bytes(1)[0] | ||
| end | ||
|
|
||
| # Generates a slice filled with *n* random bytes. | ||
| # | ||
| # ```crystal | ||
| # SecureRandom.random_bytes # => [145, 255, 191, 133, 132, 139, 53, 136, 93, 238, 2, 37, 138, 244, 3, 216] | ||
| # SecureRandom.random_bytes(4) # => [217, 118, 38, 196] | ||
| # ``` | ||
| def self.random_bytes(n : Int = 16) : Slice(UInt8) | ||
| def random_bytes(n : Int = 16) : Slice(UInt8) | ||
| if n < 0 | ||
| raise ArgumentError.new "negative size: #{n}" | ||
| end | ||
|
|
@@ -155,7 +167,7 @@ module SecureRandom | |
| # ```crystal | ||
| # SecureRandom.uuid # => "a4e319dd-a778-4a51-804e-66a07bc63358" | ||
| # ``` | ||
| def self.uuid : String | ||
| def uuid : String | ||
| bytes = random_bytes(16) | ||
| bytes[6] = (bytes[6] & 0x0f) | 0x40 | ||
| bytes[8] = (bytes[8] & 0x3f) | 0x80 | ||
|
|
||
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#readtakes 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.
Uh oh!
There was an error while loading. Please reload this page.
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_bytescould 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.