Marek Posolda commented on Bug PLINK-258

@Pedro I have few comments:
1) I am seeing that default keyLength is 0, which means that SecureRandom is initialized just with SecureRandom.getInstance("SHA1PRNG") without calling setSeed. It should be secure, however it may have performance impact, which could be bad for unit tests (I am not worried too much about it as I won't be running pl unit tests so often as you )

Another thing is that SecureRandomProvider is not shared among CredentialHandlers (unless provided in credentialHandlerOptions, but that's not the case by default), so both PasswordCredentialHandler and TOTPCredentialHandler will use separate instances of DefaultSecureRandomProvider and SecureRandom needs to be initialized twice because of this. This is something, which I addressed in my PR (I did it that first CredentialHandler will add initialized SecureRandomProvider back into credentialHandlerOptions map, so it can be reused by second CredentialHandler. Minor drawback is that I needed to change the credentialHandlerOptions to not be unmodifiable map)

2) I am seeing that renewRandomNumberGeneratorInterval is 0 by default, which means that SecureRandom instances are not periodically recreated. Shouldn't we use some non-0 value instead? And possibly try to generateSeed from previous secureRandom to avoid expensive initialization again?

3) Possible thread-safety issue (unless I am missing something)

I think that variables "secureRandom" and "lastRenewTime" should be better declared as volatile. The code like:

        if (isSecureRandomOutDated()) {
            if (this.lock.tryLock()) {
                try {
                    this.secureRandom = createSecureRandom();
                    this.lastRenewTime = new Date();
                } finally {
                    this.lock.unlock();
                }
            }
        }

means that variables "secureRandom" and "lastRenewTime" are re-created atomically in same block, but in case that Thread2 will call "isSecureRandomOutDated()" it could possibly see updated value of "lastRenewTime" but old (and maybe partially broken) instance of secureRandom as these variables are not volatile and thread2 doesn't have any lock protection and never access synchronized block (but maybe I am missing something here...)

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira