On Mon, 26 Feb 2024 11:36:23 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 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
