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

[Jaikiran Pai]

On Mon, 26 Feb 2024 11:36:23 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.
>
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - John's review - set need/wantClientAuth to false and expect both need/wantClientAuth to be false
>  - assert that client auth was indeed initiated by server during TLS handshake

Hello Valentin,

> The fix for [8326233](https://bugs.openjdk.org/browse/JDK-8326233) uses two consecutive `if`s instead of `if`/`else if`: https://github.com/openjdk/jdk/pull/17923/files#diff-6fdccf370b8c2ed3fc9690a3f5ee9a7d65af933a84252d99709c8926c52de43dR628
> 
> This shouldn't make a difference except for invalid parameter objects (which could be created using reflection or by extending the class). I'm just asking if both places should use the same logic when copying the parameters?

Like you note it shouldn't make a difference functionally, especially with the tests we have now added. Ideally, using if/else in that other part of the code would have more clearly indicated the intention and expectation of that code. When we next touch that `HttpClient` code, I will update that part to use `if/else` to be consistent.

> Also (and this is out of scope for this PR) maybe it would be better to deprecate the separate want/need client auth fields in favor of an enum enum ClientAuth { NONE, WANT, NEED } and only one setter setClientAuth. This should be less error prone 

A similar suggestion has been made in https://bugs.openjdk.org/browse/JDK-8311114. It would need inputs from security-libs team about that proposed API.

The implementation changes, the deprecation and the test coverage changes are now complete. I will focus on the CSR text next.

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

PR Comment: https://git.openjdk.org/jdk/pull/17940#issuecomment-1963943783
PR Comment: https://git.openjdk.org/jdk/pull/17940#issuecomment-1963945286