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

Merge SecureRandom into Random and Random::Secure #4894

Merged

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Aug 28, 2017

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
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Aug 28, 2017

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

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 28, 2017

Looks like the formatter failed.

@ysbaddaden ysbaddaden force-pushed the std-merge-secure-random-into-random branch from d1e9d82 to af8393f Compare Aug 28, 2017
@ysbaddaden
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Aug 28, 2017

Indeed. I pushed again.

@oprypin
Copy link
Member

@oprypin oprypin commented Aug 28, 2017

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
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Aug 28, 2017

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.

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

@ysbaddaden ysbaddaden Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@funny-falcon funny-falcon Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@funny-falcon funny-falcon Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@oprypin oprypin Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@funny-falcon funny-falcon Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oprypin , ok, I see.

Copy link
Member

@asterite asterite Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ysbaddaden The logic seems good to me :-)

Copy link
Member

@oprypin oprypin Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@asterite asterite Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Aug 28, 2017

Linux 32-bit crashed again on Travis?

Copy link
Contributor

@RX14 RX14 left a comment

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

src/random.cr Outdated
# ```
def random_bytes(buf : Bytes) : Nil
n = buf.size / sizeof(typeof(next_u))
remaining = buf.size - n * sizeof(typeof(next_u))
Copy link
Contributor

@RX14 RX14 Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be replaced by divmod?

src/random.cr Outdated
remaining.times do |i|
bits = i * 8
mask = typeof(next_u).new(0xff << bits)
buf[-i - 1] = UInt8.new((bytes & mask) >> bits)
Copy link
Contributor

@RX14 RX14 Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/random.cr Outdated
#
# 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
Copy link
Contributor

@RX14 RX14 Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1,5 +1,3 @@
require "secure_random"
Copy link
Contributor

@RX14 RX14 Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, my bad.

src/random.cr Outdated
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 }
Copy link
Member

@oprypin oprypin Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@oprypin oprypin Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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))
Copy link
Member

@oprypin oprypin Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@akzhan akzhan commented Sep 2, 2017

Is this Pull request breaks seed code on #4675?

@funny-falcon
Copy link
Contributor

@funny-falcon funny-falcon commented Sep 3, 2017

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

@oprypin
Copy link
Member

@oprypin oprypin commented Sep 4, 2017

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
Copy link
Member

@asterite asterite commented Sep 11, 2017

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

@ysbaddaden
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Sep 12, 2017

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
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Sep 12, 2017

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

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!
Copy link
Contributor

@Sija Sija Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@oprypin oprypin Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@funny-falcon funny-falcon Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed for anything except tests?

Copy link
Member

@oprypin oprypin Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@asterite asterite commented Sep 12, 2017

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
Copy link
Contributor

@RX14 RX14 commented Sep 12, 2017

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
Copy link
Member

@oprypin oprypin commented Sep 25, 2017

@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

ysbaddaden and others added 2 commits Sep 25, 2017
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 ysbaddaden force-pushed the std-merge-secure-random-into-random branch from e5394d7 to 79d0358 Compare Sep 25, 2017
@ysbaddaden ysbaddaden changed the title Merge SecureRandom into Random and Random::System Merge SecureRandom into Random and Random::Secure Sep 25, 2017
@mverzilli mverzilli added this to the Next milestone Sep 25, 2017
@ysbaddaden
Copy link
Contributor Author

@ysbaddaden ysbaddaden commented Sep 25, 2017

All done. Waiting for CI to run.

RX14
RX14 approved these changes Sep 25, 2017
@ysbaddaden ysbaddaden merged commit 78b4355 into crystal-lang:master Sep 26, 2017
2 checks passed
@ysbaddaden ysbaddaden deleted the std-merge-secure-random-into-random branch Sep 26, 2017
@RX14 RX14 mentioned this pull request Oct 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants