RFR: 8326381: com.sun.net.httpserver.HttpsParameters and SSLStreams incorrectly handle needClientAuth and wantClientAuth

[John Jiang]

On Wed, 21 Feb 2024 06:56:01 GMT, Jaikiran Pai <jpai at openjdk.org> 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.

src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java line 91:

> 89:                     engine.setNeedClientAuth(true);
> 90:                 }
> 91:                 if (params.getWantClientAuth()) {

Assume the states of `wantClientAuth` and `needClientAuth` are correctly maintained with the changes in `HttpsParameters`, or it's impossible both of them are `true`.
Could here use `if-else if` clause, like the below?

if (params.getNeedClientAuth()) {
    engine.setNeedClientAuth(true);
} else if (params.getWantClientAuth()) {
    engine.setWantClientAuth(true);
}

test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 49:

> 47:      */
> 48:     @Test
> 49:     public void testClientAuth() throws Exception {

Just a suggestion.

Now that this test uses JUnit, why doesn't it define multiple test methods for the different HttpsParameters instances?
A single test method just focus on only one HttpsParameters instance (or test case).

If the checking on a HttpsParameters instance (or test case) fails, the remaining test cases still can be executed.

test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 72:

> 70:         assertFalse(wantClientAuthParams.getNeedClientAuth(),
> 71:                 "needClientAuth was expected to be false but wasn't");
> 72:     }

Is it necessary to check the states on the following cases?


HttpsParameters params = new Params();
params.setNeedClientAuth(true);
params.setWantClientAuth(false);



HttpsParameters params = new Params();
params.setWantClientAuth(true);
params.setNeedClientAuth(false);

test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 74:

> 72:     }
> 73: 
> 74: 

Nit: this blank line could be removed if you want.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497073469
PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497091356
PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497109813
PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497097393