On Thu, 28 Sep 2023 01:38:15 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to fix the issue >> reported in https://bugs.openjdk.org/browse/JDK-8301686? >> >> The internal implementation of SSLContext caches SSLSession(s). These >> sessions are for a particular combination or peer host and port. When a TLS >> handshake completes successfully, the session is then stored in this cache. >> If/when subsequent handshake attempts against the same host/port combination >> happens, using this same SSLContext instance, then the internal >> implementation triggers a session resumption, which is allowed by the TLS >> RFC. During session resumption, the client then uses the pre-shared key from >> the previous successful handshake and sends it as part of the `ClientHello` >> message. >> >> One other part of the TLS handshake is the `server_name` extension. The >> client sends a `SNI` in the handshake which the server side can either >> reject or accept. To facilitate this matching on the server side, the >> `javax.net.ssl.SNIMatcher` can be configured on the (server side) >> `SSLParameters`. Setting of `SNIMatcher` is optional. >> >> If a successful handshake session (that was cached by the client) used a SNI >> name which the server accepted, then this SNI name is sent as part of the >> session resumption `ClientHello` along with the pre-shared key. The current >> issue is that, during session resumption, on the server side, if the >> `SNIMatcher` is no longer present, then the server rightly aborts the >> session resumption, but still ends up sending the pre-shared key extension >> as part of the `ServerHello` message. The client, upon seeing this >> pre-shared key extension in the `ServerHello` considers that the session >> resumption succeeded and ends up using that pre-shared key to derive the >> early secret. On the server side though, the server has already rejected >> this resumption request and thus when the client next sends data, the server >> will no longer be able to decode the data and will fail with >> `javax.crypto.AEADBadTagException: Tag mismatch` as noted in the JBS issue. >> >> The change in this PR, removes the pre-shared key extension data from the >> `ServerHello` message, when the server side notices that the session >> resumption is being aborted. >> >> A new jtreg test has been added which reproduces the issue and verifies the >> fix. Existing tests in tier1, tier2 and tier3 continue to pass with this >> change. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - limit line length in test case to 80 chars > - Brad's suggestion - replace "the" with "this" > - merge latest from master branch > - review comment - use SSLContextTemplate for SSLContext creation in test > - 8301686: TLS 1.3 handshake fails if server_name doesn't match resuming > session Marked as reviewed by wetmore (Reviewer). src/java.base/share/classes/sun/security/ssl/ServerNameExtension.java line 347: > 345: shc.resumingSession = null; > 346: // this server is disallowing this session > resumption, > 347: // so don't include the pre-shared key in the > ServerHello handshake message If you wouldn't mind <= 80 here also. The other changes look great. No need for another full test cycle. Thanks for considering. ------------- PR Review: https://git.openjdk.org/jdk/pull/13669#pullrequestreview-1649425699 PR Review Comment: https://git.openjdk.org/jdk/pull/13669#discussion_r1340495893
