Merge SecureRandom into Random and Random::Secure #4894

ysbaddaden · 2017-08-28T13:21:33Z

As per #4882 (comment) keeping SecureRandom is a source of duplication and of confusion. Despite the explicit naming, there are contradictory understandings as what should be the source for secure random numbers in different contexts: SecureRandom, Random::System or Crystal::System::Random?

In order to avoid this pitfall, I propose to clarify that:

Random::Secure is the source for cryptographically secure random numbers (renaming Rendom::System, see discussion below);
Crystal::System::Random is an internal abstraction to feed Random::System (i.e. nothing else must use it).
SecureRandom shall... be removed, and its usage replaced with Random::System throughout stdlib.

Following these, this patch:

moves #base64, #urlsafe_base64 and #uuid methods from SecureRandom into Random;
implements generic Random#random_bytes methods;
renames Random::System as Random::Secure;
moves the #random_bytes(buf) method from SecureRandom into Random::Secure;
replaces SecureRandom and Crystal::System::Random usages in stdlib to their equivalent calls on Random::Secure.

A benefit is that all PRNG implementations may benefit from #urlsafe_base64 or #uuid for example, but these methods should only be used with a cryptographically secure PRNG only (e.g. Random::Secure, Random::ISAAC, ChaCha20, ...). This is stated obviously in the documentation of Random and each concerned method.

That being said, using an insecure but very fast and deterministic PRNG may be suitable in test suites, which is now possible, just by switching the PRNG. Switching from the system source, using a cryptographically secure PRNG (correctly seeded from the system source) is now also possible.

ysbaddaden · 2017-08-28T13:22:46Z

I deeply apologize to @oprypin for not understand the reality of #3434 before 🙇‍♂️

RX14 · 2017-08-28T16:46:24Z

Looks like the formatter failed.

ysbaddaden · 2017-08-28T17:11:15Z

Indeed. I pushed again.

oprypin · 2017-08-28T17:17:26Z

I'm not sure if it makes much sense to split this change into two commits. Future readers will just get confused by a commit that adds a bunch of stuff and a commit that deletes a bunch of stuff, when really it's mostly being moved.

ysbaddaden · 2017-08-28T17:30:21Z

Having 2 commits may help reviewing each patch, avoiding remove/renamings noise. Yet, I agree we should "squash and merge" into a single one if we merge this.

ysbaddaden · 2017-08-28T17:34:04Z

src/random.cr

+  # Random.new.random_bytes(slice)
+  # slice # => [217, 118, 38, 196]
+  # ```
+  def random_bytes(buf : Bytes) : Nil


The one method I added is this one: a generic random_bytes(buf) that depends on next_u size, to consume as few numbers as possibles from the PRNG.

I'm not very confident in the filling remaining bytes part (when buf.size isn't a multiple of sizeof(next_u)), and would like some review.

I don't think, it is endianness really matters for 'random_bytes'.
And with this assumption, your 'tail filling' is perfectly correct.
Note, some PRNG can have more efficient implementation of 'random_bytes'.
Chacha20 generates 64 bytes at once, for example.
May be it is better to check 'respond_to?(:random_bytes)' first?

:-( I see: method should be called differently. random_bytes_native?

This code is in the base Random class. PRNGs are free to override random_bytes, the only bad thing is that they'd have to reimplement the buffer filling logic.

@oprypin , ok, I see.

@ysbaddaden The logic seems good to me :-)

@asterite, but were you reviewing the old code that these comments are on, or the new code?

@oprypin I only see one code, the same I see when I check out the code:

def random_bytes(buf : Bytes) : Nil ptr = buf.to_unsafe finish = buf.to_unsafe + buf.size while ptr < finish random = next_u rand_ptr = pointerof(random).as(UInt8*) if IO::ByteFormat::SystemEndian != IO::ByteFormat::LittleEndian rand_ptr.to_slice(sizeof(typeof(next_u))).reverse! end rand_ptr.copy_to(ptr, {finish - ptr, sizeof(typeof(next_u))}.min) ptr += sizeof(typeof(next_u)) end end

ysbaddaden · 2017-08-28T17:36:42Z

Linux 32-bit crashed again on Travis?

RX14

I don't mind this being rebased instead of squashed. I like having an explanatory commit log.

RX14 · 2017-08-28T18:40:31Z

src/random.cr

+  # ```
+  def random_bytes(buf : Bytes) : Nil
+    n = buf.size / sizeof(typeof(next_u))
+    remaining = buf.size - n * sizeof(typeof(next_u))


Couldn't this be replaced by divmod?

RX14 · 2017-08-28T18:47:24Z

src/random.cr

+      remaining.times do |i|
+        bits = i * 8
+        mask = typeof(next_u).new(0xff << bits)
+        buf[-i - 1] = UInt8.new((bytes & mask) >> bits)


The -i - 1 is cryptic. It needs a note at least.

I'd prefer to go for the simple method of creating a base = n * sizeof(typeof(next_u)) and using base + i to index.

RX14 · 2017-08-28T18:52:03Z

src/random.cr

+  #
+  # It is recommended to use the secure `Random::System` as a source or another
+  # cryptographically quality PRNG such as `Random::ISAAC` or ChaCha20.
+  def uuid : String


I'd like UUID to be moved into it's own struct with methods before 1.0 so hopefully this method should disappear and be replaced by a constructor on that. Doesn't help with the hex/base64 methods though. I don't think that using Random::DEFAULT.hex will be a world-ending issue in Crystal anyway (not that it's usually the correct thing to do) because PCG32 has fairly strong guarantees.

RX14 · 2017-08-28T18:54:30Z

src/http/multipart/builder.cr

@@ -1,5 +1,3 @@
-require "secure_random"


oops, my bad.

oprypin · 2017-08-28T18:40:09Z

src/random.cr

+    remaining = buf.size - n * sizeof(typeof(next_u))
+
+    slice = buf.to_unsafe.as(typeof(next_u)*).to_slice(n)
+    slice.each_index { |i| slice[i] = next_u }


This gives different results depending on endianness. May be worth using IO::ByteFormat::LittleEndian

Also seems like the last few bytes are big-endian while the rest are (on most systems) little endian.
I'm suggesting a different implementation in ysbaddaden#2

oprypin · 2017-08-28T19:27:36Z

spec/std/random_spec.cr

+      bytes = Bytes.new(2000)
+      TestRNG.new(RNG_DATA_32).random_bytes(bytes)
+      bytes.size.should eq 2000
+      bytes[1990, 10].should_not eq(Bytes.new(10))


These specs don't really check anything.
Needs something like https://github.com/crystal-lang/crystal/pull/3434/files#diff-0e9f78d10caafce0823cedec53f1c6f0
I'm suggesting an implementation in ysbaddaden#3

akzhan · 2017-09-02T17:27:11Z

Is this Pull request breaks seed code on #4675?

funny-falcon · 2017-09-03T12:30:55Z

Probably it will break, if will be merged before.
But it will be easy to fix.

oprypin · 2017-09-04T15:29:12Z

Thanks for accepting my changes!
Now someone should take a look at random_bytes again.

As a note, if these commits are squashed during merge, authorship info will be lost. So better apply whatever level of squashing is wanted manually.

asterite · 2017-09-11T20:59:09Z

I like this, but why not call it Random::Secure instead of Random::System?

ysbaddaden · 2017-09-12T12:31:43Z

I kept the Random::System naming since that's what it is: use the system PRNG that happens to be secure. I guess libsodium using this naming also influenced my into keeping it, but Random::Secure sounds good.

If others agree, I'll change the naming, I shall change the specs to expect a fixed result (for base64, urlsafe_base64) too.

ysbaddaden · 2017-09-12T12:32:42Z

If someone could review @oprypin's rewrite of the generic Random#random_bytes that would be sweet.

Sija · 2017-09-12T12:37:37Z

src/random.cr

+      random = next_u
+      rand_ptr = pointerof(random).as(UInt8*)
+      if IO::ByteFormat::SystemEndian != IO::ByteFormat::LittleEndian
+        rand_ptr.to_slice(sizeof(typeof(next_u))).reverse!


Would it be possible to save result of typeof(next_u) call into a variable? In this way we wouldn't need to call it 3 more times.

These are not actual calls, it's a fully compile-time construct. Using a variable could prevent some compiler optimizations.
This is pure speculation though.

funny-falcon · 2017-09-12T12:43:33Z

src/random.cr

+    while ptr < finish
+      random = next_u
+      rand_ptr = pointerof(random).as(UInt8*)
+      if IO::ByteFormat::SystemEndian != IO::ByteFormat::LittleEndian


Is it needed for anything except tests?

This is needed to produce the same results for the same seed, on different systems.
Note that this code is NOT only used for Random::System

asterite · 2017-09-12T12:45:07Z

I just suggest using Random::Secure because if I wanted a secure RNG source I would search "secure" and would never think "system" implies that. If we are sure all "system" RNGs are secure, then I'd say we use the name Secure. Otherwise we can use System but say "system should be secure, but it's not guaranteed".

RX14 · 2017-09-12T17:14:40Z

I think providing the system RNG is pointless if it's not secure (i.e. we can do better in userspace), so we should call it Random::Secure and always use the most secure RNG on each system. For 99% of platforms that should be the OS's RNG but I don't think we should make the guarantee that it's the OS rng.

oprypin · 2017-09-25T15:53:06Z

@ysbaddaden, it seems like the rename to Random::Secure is agreed upon, and not much else is blocking this.
So, could you rename that? And rebase this to remove merge commits and squash the commits in the preferred way, because I don't think GitHub will automatically do the right thing in this case

SecureRandom has been dropped in favor to Random::Secure (renamed from Random::System) which is now the secure source for random numbers suitable for cryptography. Moves SecureRandom#random_bytes(Bytes) into Random::Secure, and implements a generic Random#random_bytes(Bytes) to fill a Bytes with random numbers from any PRNG. Moves SecureRandom methods such as #base64 and #uuid into Random, so any PRNG implementation may use these methods; they should only be used with a secure PRNG thought (except maybe in test suites). This change allows to swap PRNG at will. For example swap Random::Secure for Random::ISAAC (seeded from Random::Secure), or use a cryptographically secure but slow PRNG in production, and an insecure but fast and determinable PRNG in test suites, for example.

ysbaddaden · 2017-09-25T17:13:48Z

All done. Waiting for CI to run.

ysbaddaden requested a review from RX14 August 28, 2017 13:23

ysbaddaden added the pr:needs-review label Aug 28, 2017

ysbaddaden force-pushed the std-merge-secure-random-into-random branch from d1e9d82 to af8393f Compare August 28, 2017 17:11

ysbaddaden commented Aug 28, 2017

View reviewed changes

RX14 reviewed Aug 28, 2017

View reviewed changes

oprypin requested changes Aug 28, 2017

View reviewed changes

oprypin mentioned this pull request Sep 11, 2017

Introduce Crystal::Hasher for computing a hash value #4946

Merged

Sija reviewed Sep 12, 2017

View reviewed changes

funny-falcon reviewed Sep 12, 2017

View reviewed changes

ysbaddaden and others added 2 commits September 25, 2017 19:07

Reimplement random_bytes to be consistent with endianness

79d0358

ysbaddaden force-pushed the std-merge-secure-random-into-random branch from e5394d7 to 79d0358 Compare September 25, 2017 17:10

ysbaddaden changed the title ~~Merge SecureRandom into Random and Random::System~~ Merge SecureRandom into Random and Random::Secure Sep 25, 2017

mverzilli added this to the Next milestone Sep 25, 2017

oprypin approved these changes Sep 25, 2017

View reviewed changes

RX14 approved these changes Sep 25, 2017

View reviewed changes

asterite approved these changes Sep 25, 2017

View reviewed changes

ysbaddaden removed the pr:needs-review label Sep 26, 2017

ysbaddaden merged commit 78b4355 into crystal-lang:master Sep 26, 2017

ysbaddaden deleted the std-merge-secure-random-into-random branch September 26, 2017 13:08

RX14 mentioned this pull request Oct 8, 2017

UUID continuation #4453

Merged

akzhan mentioned this pull request Oct 10, 2017

Use Random::Secure with Crystal::Hasher. #5097

Merged

Merge SecureRandom into Random and Random::Secure #4894

Merge SecureRandom into Random and Random::Secure #4894

Conversation

ysbaddaden commented Aug 28, 2017 • edited

ysbaddaden commented Aug 28, 2017

RX14 commented Aug 28, 2017

ysbaddaden commented Aug 28, 2017

oprypin commented Aug 28, 2017

ysbaddaden commented Aug 28, 2017

Choose a reason for hiding this comment

funny-falcon Sep 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Aug 28, 2017

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oprypin Aug 28, 2017 • edited

Choose a reason for hiding this comment

oprypin Aug 28, 2017 • edited

Choose a reason for hiding this comment

akzhan commented Sep 2, 2017

funny-falcon commented Sep 3, 2017 • edited

oprypin commented Sep 4, 2017

asterite commented Sep 11, 2017

ysbaddaden commented Sep 12, 2017

ysbaddaden commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Sep 12, 2017

RX14 commented Sep 12, 2017 • edited

oprypin commented Sep 25, 2017

ysbaddaden commented Sep 25, 2017

ysbaddaden commented Aug 28, 2017 •

edited

funny-falcon Sep 12, 2017 •

edited

oprypin Aug 28, 2017 •

edited

oprypin Aug 28, 2017 •

edited

funny-falcon commented Sep 3, 2017 •

edited

RX14 commented Sep 12, 2017 •

edited