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

SecureRandom update to match Ruby 2.3 #4656

Merged
merged 5 commits into from Jun 12, 2017
Merged

Conversation

kares
Copy link
Member

@kares kares commented Jun 8, 2017

securerandom.rb imported from MRI's 2.3 stable line

re-implemented random_number (native in C-Ruby now) in Java to handle ranges

resolves #4607

@kares kares added this to the JRuby 9.1.11.0 milestone Jun 8, 2017
@kares kares requested a review from headius June 8, 2017 12:05
@headius
Copy link
Member

headius commented Jun 8, 2017

Looks good to me. How big is the diff with MRI stdlib securerandom now?

I'm guessing this is a library they might plan to gemify, so we'll probably have to address that soon. Perhaps this should be registered as an internal extension for now, so we can just require "securerandom.jar"?

@kares
Copy link
Member Author

kares commented Jun 9, 2017

yeah, but its confusing the stdlib repo is a few years stale and contains an old securerandom.rb
... which JRuby had, the new library .rb had to be dig out from the ruby repo directly, the diff is :

3,6c3,5
< begin
<   require 'openssl'
< rescue LoadError
< end
---
> 
> require 'jruby'
> org.jruby.ext.securerandom.SecureRandomLibrary.load JRuby.runtime
50,80d48
< module SecureRandom
<   if defined?(OpenSSL::Random) && /mswin|mingw/ !~ RUBY_PLATFORM
<     def self.gen_random(n)
<       @pid = 0 unless defined?(@pid)
<       pid = $$
<       unless @pid == pid
<         now = Process.clock_gettime(Process::CLOCK_REALTIME, :nanosecond)
<         ary = [now, @pid, pid]
<         OpenSSL::Random.random_add(ary.join("").to_s, 0.0)
<         seed = Random.raw_seed(16)
<         if (seed)
<           OpenSSL::Random.random_add(seed, 16)
<         end
<         @pid = pid
<       end
<       return OpenSSL::Random.random_bytes(n)
<     end
<   else
<     def self.gen_random(n)
<       ret = Random.raw_seed(n)
<       unless ret
<         raise NotImplementedError, "No random device"
<       end
<       unless ret.length == n
<         raise NotImplementedError, "Unexpected partial read from random device: only #{ret.length} for #{n} bytes"
<       end
<       ret
<     end
<   end
< end
< 
176d143
< =begin
194,219c161,162
<     if 0 < n
<       if defined? OpenSSL::BN
<         OpenSSL::BN.rand_range(n).to_i
<       else
<         hex = n.to_s(16)
<         hex = '0' + hex if (hex.length & 1) == 1
<         bin = [hex].pack("H*")
<         mask = bin[0].ord
<         mask |= mask >> 1
<         mask |= mask >> 2
<         mask |= mask >> 4
<         begin
<           rnd = random_bytes(bin.length)
<           rnd[0] = (rnd[0].ord & mask).chr
<         end until rnd < bin
<         rnd.unpack("H*")[0].hex
<       end
<     else
<       # assumption: Float::MANT_DIG <= 64
<       if defined? OpenSSL::BN
<         i64 = OpenSSL::BN.rand(64, -1).to_i
<       else
<         i64 = random_bytes(8).unpack("Q")[0]
<       end
<       Math.ldexp(i64 >> (64-Float::MANT_DIG), -Float::MANT_DIG)
<     end
---
>     # implemented natively in JRuby (just like in MRI)
>     raise NotImplementedError("#{__method__}(#{n})")
221d163
< =end
250a193,197
> # NOTE: JRuby's SecureRandom native part implement some of the methods from Random::Formatter
> # - SecureRandom.random_bytes(n=nil)
> # - SecureRandom.hex(n=nil)
> # - SecureRandom.uuid
> # - SecureRandom.random_number(n=0)

... is it preferable if removals are put back (JRuby had some modifications previous so I did not care much)?

@headius
Copy link
Member

headius commented Jun 9, 2017

I think we should consider MRI repo the canonical source for these libraries until they've officially completed gemifying each library. I just brought it up to keep in mind.

This particular library diverges so much from MRI (at least in impl) that I'm not concerned about maintaining a diff.

@kares kares merged commit 17ff9fe into jruby:master Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SecureRandom.random_number doesn't accept a range
2 participants