Make Scope ID RNG no longer use SecureRandom (fix #6178) #6179
Conversation
SecureRandom is a robust RNG for use in security-sensitive scenarios (like generating cryptographic/private keys). For something like a random Scope ID, it is overkill, as it's slower, and becomes a potentially blocking call if the machine hasn't been on for long or have enough entropy. It's much better to just use a normal ThreadLocalRandom instance, that will not experience contention issues even under high traffic.
|
I think you'll also want to check your usage of RandomStringUtils in I have definitely banned this class from my codebase as this irresponsible behavior make this class a foot-gun for future colleagues. |
There was a problem hiding this comment.
The prior version of this method returned strings that contained both upper and lowercase letters, while the new version uses only uppercase.
@filipelautert I think it would be best to add all lowercase letters to final String AB = "<...>". Thoughts?
The original code before using |
Impact
Description
SecureRandom is a robust RNG for use in security-sensitive scenarios (like generating cryptographic/private keys). For something like a random Scope ID, it is overkill, as it's slower, and becomes a potentially blocking call if the machine hasn't been on for long or have enough entropy.
I've looked into the original change (in commit 3daa688) and it seems to have been done to improve the performance of the Scope ID generation. While it clearly did improve things, it's much better to just use a normal ThreadLocalRandom instance, that will not experience contention issues even under high traffic, and will not have the problem with depending on system entropy.The original change (in commit 3daa688) already did this change implicitly, since RandomStringUtils up to version 3.14 was using ThreadLocalRandom. So from Liquibase's perspective, this maintains the improved performance profile without making it subject to the whims of Apache Commons Lang3.
Fixes #6178. I built a local
liquibase-coreJAR and linked into my CLI tool, and the changeset deployment no longer hangs.Things to be aware of
This makes the Scope ID generation not use a SecureRandom instance anymore, but it should really not be a problem as it should not require cryptographic-level randomness.This is just setting your Scope ID generation to use ThreadLocalRandom again, more explicitly, and in a way that cannot be arbitrarily changed by myopic library maintainers :)
Things to worry about
I think that the performance should actually be much better than the previous implemented fix (using Apache Commons), but I have not tested your use-case. I would recommend that you test it and see what the actual performance impact is.Actually, you did the change initially, to move away from
SecureRandomtoThreadLocalRandom, and the Apache Commons Lang3 team just changed that out from under you in v3.15, not just back toSecureRandombut all the way toSecureRandom.getInstanceStrong()which will make this block/take forever. So I see nothing more to worry about :)