Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK 17 (JEP 403 Strongly Encapsulate JDK Internals) breaks okhttp without additional config #6694

Closed
sureshg opened this issue Jun 2, 2021 · 10 comments
Labels
bug Bug in existing code

Comments

@sureshg
Copy link

sureshg commented Jun 2, 2021

  • Version used
$ java --version                                                                                                                                      sgopal1@m-c02xd0kqjgh7
openjdk 17-loom 2021-09-14
OpenJDK Runtime Environment (build 17-loom+7-342)
OpenJDK 64-Bit Server VM (build 17-loom+7-342, mixed mode, sharing)

// OkHttp
implementation(platform("com.squareup.okhttp3:okhttp-bom:4.9.0"))
implementation("com.squareup.okhttp3:okhttp")

Throws the following error when trying to create an OkHttpClient on JDK17-ea build with strong encapsulation enabled.

java.lang.reflect.InaccessibleObjectException: Unable to make field private final sun.security.ssl.SSLContextImpl sun.security.ssl.SSLSocketFactoryImpl.context accessible: module java.base does not "opens sun.security.ssl" to unnamed module @3a44f074
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
        at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
        at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
        at okhttp3.internal.Platform.readFieldOrNull(Platform.java:417)
        at okhttp3.internal.Platform.trustManager(Platform.java:91)
        at okhttp3.OkHttpClient.<init>(OkHttpClient.java:184)
        at okhttp3.OkHttpClient.<init>(OkHttpClient.java:60)
        at okhttp3.OkHttpClient$Builder.build(OkHttpClient.java:718)
        at se.bjurr.gitchangelog.internal.integrations.github.GitHubServiceFactory.getGitHubService(GitHubServiceFactory.java:55)

Have to explicitly open this package for reflection in all (--add-​opens java.base/sun.security.ssl=ALL-UNNAMED) the programs for okhttp to work properly.

Source: https://github.com/square/okhttp/blob/master/okhttp/src/main/kotlin/okhttp3/internal/platform/Platform.kt#L93

@sureshg sureshg added the bug Bug in existing code label Jun 2, 2021
@yschimke
Copy link
Collaborator

yschimke commented Jun 2, 2021

Looks like a clean fix is to log and swallow this exception

          open fun trustManager(sslSocketFactory: SSLSocketFactory): X509TrustManager? {
            return try {
              // Attempt to get the trust manager from an OpenJDK socket factory. We attempt this on all
              // platforms in order to support Robolectric, which mixes classes from both Android and the
              // Oracle JDK. Note that we don't support HTTP/2 or other nice features on Robolectric.
              val sslContextClass = Class.forName("sun.security.ssl.SSLContextImpl")
              val context = readFieldOrNull(sslSocketFactory, sslContextClass, "context") ?: return null
              readFieldOrNull(context, X509TrustManager::class.java, "trustManager")
            } catch (e: ClassNotFoundException) {
              null
            }
          }

@yschimke
Copy link
Collaborator

yschimke commented Jun 2, 2021

We should add a JDK17 CI build at the same time.

@swankjesse
Copy link
Member

Thanks for the great report!

@yschimke
Copy link
Collaborator

@sureshg Something is off with the java files and line numbers in your stacktrace. Can you easily test if we give you a snapshot?

https://github.com/square/okhttp/blob/parent-4.9.0/okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt#L184

@yschimke
Copy link
Collaborator

Between Gradle, OpenJDK and Github Actions, testing this in a PR isn't much fun.

@sureshg
Copy link
Author

sureshg commented Jun 16, 2021

Something is off with the java files

@yschimke My bad, yes you are correct. The issue is due to an old OKHttp dependency used in the Gradle plugin. I did test the latest OKHttp release on JDK 18 and it's running smoothly without any issues/warnings 👍🏼 .
Here is the link JDK18 github action run logs on win/linux/macos - https://github.com/sureshg/okhttp-jdk-test/runs/2836840606?check_suite_focus=true

Again, really sorry for the inconvenience caused.

@sureshg sureshg closed this as completed Jun 16, 2021
@yschimke
Copy link
Collaborator

@sureshg No problem. My read is that we should still have #6707, we would still only use JDK9Platform when we support ALPN and don't opt into other security providers. So seems like the PR is probably beneficial. Thoughts?

@sureshg
Copy link
Author

sureshg commented Jun 16, 2021

The only place i could find where trustManager() is called is fun sslSocketFactory(sslSocketFactory: SSLSocketFactory), which already deprecated with DeprecationLevel as ERROR - https://github.com/square/okhttp/blob/master/okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt#L761

@yschimke
Copy link
Collaborator

yschimke commented Jul 4, 2021

Fix for 4.9.x #6742 (draft currently)

@yschimke
Copy link
Collaborator

yschimke commented Jul 17, 2021

I don't think it's trivial to test for this, as I suspect that the Gradle 7.2 + Junit code needs to avoid this anyway.

package okhttp3;

import org.junit.jupiter.api.Test;

import javax.net.ssl.SSLSocketFactory;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class Jdk17Test {
    @Test
    public void testDeprecatedSslSocketFactory() {
        // https://github.com/square/okhttp/issues/6694

        SSLSocketFactory factory = (SSLSocketFactory) SSLSocketFactory.getDefault();

        try {
            new OkHttpClient.Builder().sslSocketFactory(factory).build();
        } catch (UnsupportedOperationException uoe) {
            // expected
            assertEquals("clientBuilder.sslSocketFactory(SSLSocketFactory) not supported on " +
                    "JDK 8 (>= 252) or JDK 9+",
                    uoe.getMessage());
        }
    }
}

Is passing even if I disable the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

No branches or pull requests

3 participants