On Fri, 23 Feb 2024 02:05:09 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which proposes to fix >> https://bugs.openjdk.org/browse/JDK-8326381? >> >> As noted in the JBS issue, the implementation in `setNeedClientAuth()` and >> `setWantClientAuth()` of `com.sun.net.httpserver.HttpsParameters` wasn't >> matching the API specification. The commit in this PR fixes that issue and >> it now matches the API specification as well as what is done in >> `javax.net.ssl.SSLParameters` class. >> >> Additionally, as noted in the JBS issue, the (internal class) >> `sun.net.httpserver.SSLStreams` had a bug where it could end up resetting >> the `needClientAuth` flag on the `SSLEngine` because of the way the >> `setNeedClientAuth()` and `setWantClientAuth()` methods were being called on >> the `SSLEngine`. This too has been fixed in this PR. >> >> A new jtreg test has been introduced to reproduce the issue in the >> `HttpsParameters` class and verify this fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > undo changes to SimpleSSLContext in test library test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 114: > 112: needClientAuthParams.setWantClientAuth(initialWantClientAuth); > 113: // needClientAuth = true and thus wantClientAuth = false > 114: needClientAuthParams.setNeedClientAuth(true); I just want to clarify that these new cases look not suggested by my previous comment. In my suggested cases, as shown as the below, setNeedClientAuth(true); setWantClientAuth(false); setWantClientAuth(true); setNeedClientAuth(false); this first setting takes a state (`needClientAuth` or `wantClientAuth`) to `true`, and the second setting takes both states to `false`. Before this fix, the second setting doesn't reset the `true` state specified by the first setting to `false`. Of course, if you think it's unnecessary to cover these corner cases, please feel free to ignore them. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1500171456
