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

Backport securerandom #4149

Merged
merged 4 commits into from Sep 14, 2016

Conversation

Projects
None yet
5 participants
@headius
Member

headius commented Sep 12, 2016

This PR backports recent SecureRandom entropy fixes from 9k to 1.7. See #1405.

We had a 1.7 user in #jruby today with entropy problems.

kares and others added some commits Mar 10, 2016

avoid eager secure random initialization on every new Ruby thread all…
…ocation

shaves off 10-15% of Thread.new/start :

```
Rehearsal ----------------------------------------------------------
Thread.new [100000x]    12.800000   6.850000  19.650000 ( 10.392600)
Thread.start [100000x]  12.670000   5.680000  18.350000 (  9.625846)
------------------------------------------------ total: 38.000000sec

                             user     system      total        real
Thread.new [100000x]    12.780000   5.760000  18.540000 (  9.620000)
Thread.start [100000x]  12.490000   5.810000  18.300000 (  9.524975)
```

```
Rehearsal ----------------------------------------------------------
Thread.new [100000x]    11.040000   5.560000  16.600000 (  8.708792)
Thread.start [100000x]  10.740000   5.520000  16.260000 (  8.400990)
------------------------------------------------ total: 32.860000sec

                             user     system      total        real
Thread.new [100000x]    10.840000   5.620000  16.460000 (  8.506339)
Thread.start [100000x]  10.460000   5.560000  16.020000 (  8.323819)
```

@headius headius added this to the JRuby 1.7.27 milestone Sep 12, 2016

Actually settle on SHA1PRNG if it's available. #1405
I botched the previous patch a bit by unconditionally re-assigning
the secureRandom local to a default JDK new SecureRandom. This
could cause systems without the default preferred PRNG
(NativePRNGNonBlocking, Java 8+) to have slower thread startup and/or
random number generation.
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 13, 2016

Member

isn't 8fb4b93 "dangerous" to back-port (changing a previously initialized field on ThreadContext to null) ?

Member

kares commented Sep 13, 2016

isn't 8fb4b93 "dangerous" to back-port (changing a previously initialized field on ThreadContext to null) ?

Show outdated Hide outdated core/src/main/java/org/jruby/util/cli/Options.java
@@ -146,6 +146,7 @@
public static final Option<Boolean> REIFY_VARIABLES = bool(MISCELLANEOUS, "reify.variables", false, "Attempt to expand instance vars into Java fields");
public static final Option<Boolean> PREFER_IPV4 = bool(MISCELLANEOUS, "net.preferIPv4", true, "Prefer IPv4 network stack");
public static final Option<Boolean> FCNTL_LOCKING = bool(MISCELLANEOUS, "file.flock.fcntl", true, "Use fcntl rather than flock for File#flock");
public static final Option<String> PREFERRED_PRNG = string(MISCELLANEOUS, "preferred.prng", "NativePRNGNonBlocking", "Maintain children static scopes to support scope dumping.");

This comment has been minimized.

@kares

kares Sep 13, 2016

Member

the help message for the option seems like a copy 🍝 left over

@kares

kares Sep 13, 2016

Member

the help message for the option seems like a copy 🍝 left over

This comment has been minimized.

@headius

headius Sep 13, 2016

Member

Indeed...I'll fix that on both master and in this PR.

@headius

headius Sep 13, 2016

Member

Indeed...I'll fix that on both master and in this PR.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 13, 2016

Member

@kares: isn't 8fb4b93 "dangerous" to back-port (changing a previously initialized field on ThreadContext to null) ?

A little dangerous, I agree. However the internals of ThreadContext have never been a blessed API. I know there might be people poking at them, but we've never shown people examples that use TC internals and we have generally discouraged messing with it.

I'm not too concerned, especially since it helps thread startup times (important for Fiber and Enumerator#next as well). @enebo?

Member

headius commented Sep 13, 2016

@kares: isn't 8fb4b93 "dangerous" to back-port (changing a previously initialized field on ThreadContext to null) ?

A little dangerous, I agree. However the internals of ThreadContext have never been a blessed API. I know there might be people poking at them, but we've never shown people examples that use TC internals and we have generally discouraged messing with it.

I'm not too concerned, especially since it helps thread startup times (important for Fiber and Enumerator#next as well). @enebo?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 13, 2016

Member

@takanuva This is the backported SecureRandom changes I mentioned. Since you have an environment with entropy problems, perhaps you could test out a HEAD build of JRuby 1.7 for us once we merge?

It would be REALLY nice to know that this actually fixes your issue without installing an entropy-generator. Up to now we've just assumed it does, since it's hard to simulate an environment with entropy-starvation problems.

Member

headius commented Sep 13, 2016

@takanuva This is the backported SecureRandom changes I mentioned. Since you have an environment with entropy problems, perhaps you could test out a HEAD build of JRuby 1.7 for us once we merge?

It would be REALLY nice to know that this actually fixes your issue without installing an entropy-generator. Up to now we've just assumed it does, since it's hard to simulate an environment with entropy-starvation problems.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 14, 2016

Member

@bbrowning or @mkristian can you see a problem with this change to threadcontext not having securerandom initialized? I personally think the potential for this breaking stuff could only happen in a deep extension but not very likely.

Member

enebo commented Sep 14, 2016

@bbrowning or @mkristian can you see a problem with this change to threadcontext not having securerandom initialized? I personally think the potential for this breaking stuff could only happen in a deep extension but not very likely.

@bbrowning

This comment has been minimized.

Show comment
Hide comment
@bbrowning

bbrowning Sep 14, 2016

Contributor

@enebo I don't know of anything that uses securerandom directly like this. I agree it would have to be an extension really digging deep to get hit by this. While it is possible that one exists somewhere, a quick GitHub search didn't turn up anything obvious.

Contributor

bbrowning commented Sep 14, 2016

@enebo I don't know of anything that uses securerandom directly like this. I agree it would have to be an extension really digging deep to get hit by this. While it is possible that one exists somewhere, a quick GitHub search didn't turn up anything obvious.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 14, 2016

Member

... no (more) worries from me than - thanks everyone for looking into it!

Member

kares commented Sep 14, 2016

... no (more) worries from me than - thanks everyone for looking into it!

@headius headius merged commit b9bcb01 into jruby:jruby-1_7 Sep 14, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@headius headius deleted the headius:backport_securerandom branch Sep 14, 2016

@takanuva

This comment has been minimized.

Show comment
Hide comment
@takanuva

takanuva Sep 15, 2016

Sure, @headius, just give me a few days and I'll gladly test that for you guys. 😄

takanuva commented Sep 15, 2016

Sure, @headius, just give me a few days and I'll gladly test that for you guys. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment