On 5/24/19 8:51 AM, Xuelei Fan wrote:
SSLSessionContext.java
----------------------
As comment in the CSR review thread, I may not define the
jdk.tls.server.sessionTicketTimeout property, and use one session
timeout (SSLSessionContext.getSessionTimeout()) instead.
The ticket timeout may be not necessary, read more please.
SessionTicketExtension.java
---------------------------
As comment in the CSR review thread, I may not define the key timeout.
Instead, I may use key usage limit. As it is stateless server, I may
not use the stateless session timeout as well, for less states. The key
rotation scheme might be able to take place of timeout. For client
side, the session context session timeout and management may be
sufficient for the client side cache cleanup.
The spec says the keys need to be rotated regularly. Of course
"regularly" is up for interpretation. If a usage limit is implemented
and the server is not frequently used, it's possible to have the same
key used for the entire span of the session timeout. I don't feel that
is often enough.
StatelessKey.gcmspec:
If I read it right, the same key and iv are used for every ticket
encryption. This behavior is vulnerable. The IV should be unique for
each encryption.
GCM as a whole was probably the wrong choice
I would like to avoid to create thread in the fundamental API
implementation if possible. As the thread (KeyState.run()) is for
invalid key cleanup only, the cleanup can be moved to the get methods.
How is this thread in the API? It is only started when the key is
generated and not by a API call and it's the internal parts of the
implementation.
For each StatelessKey, one thread is created. I was wondering if there
are any chance that there are multiple threads to manage the stateless
keys, and potential memory leak?
Every time a new session key. Depending on the property the default is
once an hour. It's possible multiple threads can be used if the timing
is just right, but that only mean it runs through the list a few times.
Hardly a performance issue. Each key is put in the hashmap, so there is
no memory leak. Maybe I can trim it down to a static thread that runs
when needed, that would eliminate the multiple threads, but maybe add
more locks.
Anyway, I will try to avoid to use internal thread. I may use a key
rotation scheme similar to cookie manager (use two keys, one for legacy
and one for the current key, see HelloCookieManager.java).
The thread is one piece for clean up, if I remove it from the thread, I
will need to be put into the main code path.
The keys have a longer life, the defaults are 1 hour and can last up to
24hrs. Those can be changed with the properties in a way that would
require more keys to be stored. So rotating keys will involve some sort
of Map or storage object. HelloCookieManager just needs to remember the
previous secret and for a short period of time as the response from the
client could be only a few seconds.
As for the for the lock mechanism around the rotation. I don't see much
difference between you using the ReentrantLock vs the synchronized block
I have. I'm trying to minimize the amount of time in the lock
generating the key. I'm not so concerned about a few extra keys being
generated. Putting in more locks just means more chances for the main
code path getting stopped.
I don't see the significants to this other than being personal coding
styles.
If the key is invalid, an exception will be thrown and then fail back to
full handshake. Exception thrown and catch are expensive, I may not
exception for the failback to full handshake.
If there is an exception the full handshake is going to be more
expensive than the exception. I could probably code away the exception.
Let's discuss these issues firstly before we moving on with more code
review.
Hope it helps.
Thanks,
Xuelei
On 5/21/2019 5:35 PM, Anthony Scarpino wrote:
Hi all,
I’ve updated in-place some recent changes due to some additional testing
Tony
On May 16, 2019, at 2:30 PM, Anthony Scarpino
<anthony.scarp...@oracle.com> wrote:
I'm asking for a review of this rather large change to add support
stateless tickets in the TLS 1.3 5077 RFC.
https://bugs.openjdk.java.net/browse/JDK-8211018
http://cr.openjdk.java.net/~ascarpino/stateless/webrev.00
thanks
Tony
