On 6/4/19 9:02 PM, Xuelei Fan wrote:
Hi Tony,
There are a lot of pretty good designs in the update, for example, the
cooperation of the session timeout and key rotation timeout.
My following comments are mainly about the issues I can find. Most of
them are minors.
On 6/3/2019 5:42 PM, Anthony Scarpino wrote:
http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02
SessionTicketExtension.java:
----------------------------
81 private static int currentKey = new SecureRandom().nextInt();
When I read the code, the first question came to me was: if a integer is
strong enough to be a key. It took me a while to understand that
currentKey is a key id rather than a key. Would you mind rename the
filed with a straightforward name? For example currentKeyId?
Sure, I can see how that could be confusing
83 // Time in milliseconds until key is invalid and will be
destroyed.
Looks like there is no code related to this comment line. Is it > duplicated
line?
It was an orphaned comment
93 keyTimeout = TIMEOUT_DEFAULT;
Nice to have a debug log for incorrect setting.
sure
78 private static int keyTimeout = TIMEOUT_DEFAULT;
I did not find the assignment to this filed other than the
initialization static block. Could this filed be 'final'?
ok
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.
80 private static final HashMap<Integer, StatelessKey> keystate =
new HashMap<>();
This 'keystate' is a global variable. As means different SSLContext
shares the same stateless keys. Different SSLContext could use
different session timeout. I would prefer to make it a SSLContext
sensible variable, and then when SSLContext object is
releases/destroyed, and stateless keys will be released/destroyed as well.
Yeah, that shouldn't be static
108 static final class StatelessKey {
Could this class be private?
yes
149 StatelessKey ssk = getKey(hc, currentKey);
The above line will check if the key is invalid via getKey() method.
Because of the implementation details of isExpired() and isInvalid(), I
think it should be sufficient if not checking isInvalid(). Otherwise, I
was little bit confusing about the logic here.
yes
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. 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.
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.
BTW, would you mind limit each line no more than 80 characters?
I will review the rest of this class tomorrow.
Thanks,
Xuelei
