On Sun, 18 Sep 2022 12:57:28 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this change which proposes to fix the > intermittent failures noted in https://bugs.openjdk.org/browse/JDK-8293657? > > There are two parts to this fix. One is straightforward fix in the test > configuration file where it uses an incompatible cipher suite with the > enabled protocols. The details of that are noted in my comment in the JBS > https://bugs.openjdk.org/browse/JDK-8293657?focusedCommentId=14524400&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14524400. > The fix for that resides solely in `management_ssltest07_ok.properties.in` > file. > > The more curious part was why this specific test configuration is ever > passing instead of always failing. That turned out to be a bug in the > `sun.management.jmxremote.HostAwareSslSocketFactory`. The (internal > implementation detail) in `sun.rmi.transport.tcp.TCPEndpoint` holds a cache > of `localEndpoints` which effectively are server and client socket factories > that get used when creating the corresponding socket/server sockets. The > cache uses a key, whose one component is the `HostAwareSslSocketFactory` > instance. This class extends from `javax.rmi.ssl.SslRMIServerSocketFactory` > which is where the `equals()` is implemented for the > `HostAwareSslSocketFactory` instances. The bug resides in the fact that the > current implementation of the `HostAwareSslSocketFactory` constructor doesn't > pass to its `super` the `enabledCipherSuites`, `enabledProtocols` and > `needClientAuth` values with which the `HostAwareSslSocketFactory` was > constructed with. Instead it stores these values as private me mbers and thus the `SslRMIServerSocketFactory` has its own version of these members as `null`. The `SslRMIServerSocketFactory` uses these values in its `equals` implementation and thus ends up computing a wrong output from the `equals` call. This ultimately ends up with the (internal) `TCPEndpoint` returning an incorrect (server) socket factory that gets used during the client/server communication. What this means is that the restrictions imposed on enabled protocols and enabled cipher suites in `management_ssltest07_ok.properties.in` aren't taken into account and instead a different set of (lenient) configurations for these attributes gets used (because of the cached socket factory constructed for a previous run of a different test configuration). > > The commit in this PR fixes the `HostAwareSslSocketFactory` to correctly pass > to its `super` the enabled protocols, enabled cipher suites and the client > auth mode, instead of saving that state (only) in `HostAwareSslSocketFactory`. > > Additionally the commit removes this test from the problem listing. I had a look at the RMIServerSocketFactory hierarchy and the proposed changes look reasonable. ------------- Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10323
