ELY-2731 Use SecureRandom instead of ThreadLocalRandom

[TomasHofman]

Issue: https://issues.redhat.com/browse/ELY-2731

[ivassile]

@TomasHofman Can you make the requested changes? Thanks!

[TomasHofman]

Sorry, lost track of this. I updated the PR.

One thing - I'm looking at the wildfly-common Assert.assertTrue() method:

    public static boolean assertTrue(boolean expr) {
        assert expr : CommonMessages.msg.expectedBoolean(expr);
        return expr;
    }

and then the CommonMessages.msg.expectedBoolean() method:

    @Message(id = 1003, value = "Internal error: Assertion failure: Expected boolean value to be %s")
    String expectedBoolean(boolean expr);

It looks to me the printed message is wrong, and prints the opposite of the expected value. In the assertTrue() method we should call CommonMessages.msg.expectedBoolean(true); rather than CommonMessages.msg.expectedBoolean(expr);..?

Or maybe it's just to early for me.

[darranl]

Looking at the JavaDoc for SecureRandom, yes the class itself is thread safe and can be accessed by multiple threads concurrently:

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/security/SecureRandom.html

BUT if the underlying Provider does not declare it is thread safe it will introduce locking.on accessing the provider.

I don't know if it will be better or worse but I wonder if Elytron could have it's on ThreadLocalSecureRandom - the reason for the ThreadLocal model is you know your instance is only going to be accessed by a single thread so you are not waiting on locks held by other threads. The down side being you end up with one SecureRandom per thread but is this an issue?

[TomasHofman]

I updated the PR - added a publicly accessible ThreadLocalSecureRandom class.

I guess since the ThreadLocal model was there in the first place, it's probably safer to stick to it.

I think you are able to asses the performance implication better than me. My feeling originally was that the random bytes generation in practice would not be as heavily used for this to cause performance issues, but whether that's true or not, there's no reason to introduce bottlenecks that do not need to be there.

[darranl]

@ivassile if you can add a review I think with one more change from @TomasHofman I can merge today

[darranl]

I have replaced this with #2189 to get us some better CI results.