On 6/5/2019 12:41 PM, Anthony Scarpino wrote:
http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02
SessionTicketExtension.java:
----------------------------
117 byte[] k = new byte[KEYLEN];
118 random.nextBytes(k);
119 key = new SecretKeySpec(k, "AES");
I think the stateless key is pretty important. Plaintext key may not
meet the security requirement in some circumstances.
What do you think if using KeyGenerator APIs?
While I would argue that if SecureRandom is broken there are a lot
bigger problems with the system, I'll use the API and have the old
random code as a backup in the catch because an NoSuchAlgorithmException
is not an acceptable or clean result to deal with in the code.
It's not only about the quality of SecureRandom. Some circumstances may
want the key is not extractable, in a token, etc.
I would not worry about the NoSuchAlgorithmException exception as AES is
the "required" algorithm for KeyGenerator. We don't support provider
that does not implement required algorithms.
166 synchronized (keystate) {
If needed, I would suggest to replace 'synchronized' with explicit
locks (java.util.concurrent.locks). For more information, please refer
to JDK-8221882 or Project Loom.
The questions is if it's necessary to change what maybe infrequently
locked code for Project Loom.
It's a good question. I don't know if it is really necessary. The
dependency could be very complicated. To easy the job, I would like to
cleanup all of the synchronized block in JSSE code, instead of make more
evaluation now and later if the use of the synchronized block could be a
problem or not. As would easy our life a little bit.
The only way around this is putting a
lock in SSLContextImpl and accessing it through hc.sslcontext. With all
this code being static the lock has to come through the context now.
Another less desirable option is one static lock for all contexts.
As you are locking the keyState, an instance variable. I think we could
add new lock object in the same instance. But, we may not need to do
that because we may need to adjust the code because of the following
commented issue.
>> 186 cleanup(getSessionTimeout(hc));
>> 191 static void cleanup(int sessionTimeout) {
>> 202 keystate.remove(i);
>> If different SSLContext use different session timeout, the key may be
>> valid in one context but not in another. If look the three lines
>> above together, the key cleanup method may not work as expected.
>>
>> I would suggest to use SSLContext sensible key state cache. For
>> example, put the cache in the SSLContextImpl instance.
Xuelei
