RFR: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93) [v3]

[Xue-Lei Andrew Fan]

On Fri, 7 May 2021 12:01:16 GMT, Alexey Bakhtin <abakhtin at openjdk.org> wrote:

>> Hello All,
>> 
>> Could you please review the fix for the JDK-8241248?
>> The issue happens during the TLSv1.3 handshake without server stateless session resumption in case of server receives several parallel requests with the same pre_shared_key.
>> The main idea of the fix is to remove resuming session from the session cache in the early stage.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8241248
>> Webrev 8u: http://cr.openjdk.java.net/~abakhtin/8241248/webrev.v0/
>> 
>> The test from the bug report using OpenSSL is passed ( -Djdk.tls.server.enableSessionTicketExtension=false )
>> javax/net/ssl and sun/security/ssl jtreg tests passed
>> 
>> Regards
>> Alexey
>
> Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add Cache.pull method

Thank you for take my comments.  The code looks nice to me, except a few trivial comments.

src/java.base/share/classes/sun/security/ssl/PreSharedKeyExtension.java line 377:

> 375:                     // If we are keeping state, see if the identity is in the cache
> 376:                     if (requestedId.identity.length == SessionId.MAX_LENGTH) {
> 377:                         s = sessionCache.pull(requestedId.identity);

Would you please add a comment here so as we know why a pull method could be used here?  For example:


-                    // If we are keeping state, see if the identity is in the cache
+                    // If we are keeping state, see if the identity is in the
+                    // cache.  Note that for TLS 1.3, we would also clean
+                    // up the cached session if it is not rejoinable.

src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 183:

> 181:         if (id != null)
> 182:             return sessionCache.pull(new SessionId(id));
> 183:         return null;

As this is an internal method, it should be safe to assume that the id is non-null.  I'm fine if you want to keep the non-null check, but please use braces for if-statement (see also https://www.oracle.com/java/technologies/javase/codeconventions-statements.html#449).

src/java.base/share/classes/sun/security/util/Cache.java line 430:

> 428:             return null;
> 429:         }
> 430:         V value;

I may add a blank line before this line.

src/java.base/share/classes/sun/security/util/Cache.java line 442:

> 440:         entry.invalidate();
> 441:         return value;
> 442:     }

I may adjust the lines a little bit so as to avoid duplicated operations (see the implementation code of isValid()).


         long time = (lifetime == 0) ? 0 : System.currentTimeMillis();
         if (entry.isValid(time)) {
              V value = entry.getValue();
              entry.invalidate();
              return value;
          } else {
              if (DEBUG) {
                  System.out.println("Ignoring expired entry");
              }
              return null;
          }

-------------

PR: https://git.openjdk.java.net/jdk/pull/3664