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

Is OkHttp too eager in its blocking of non-HTTPS traffic on localhost? #8031

Closed
bartekpacia opened this issue Sep 19, 2023 · 4 comments
Closed
Labels
bug Bug in existing code

Comments

@bartekpacia
Copy link

bartekpacia commented Sep 19, 2023

Hi! First of all, thanks to all creators & maintainers of this awesome library.

Use case

I'm building a testing framework on top of JUnit4, AndroidJUnitRunner, and Android Test Orchestrator. Now as you probably know, two apps are involved during instrumentation testing on Android – the app under test and the instrumentation app, and they run in the same process.

Since I need two-way communication between these two apps, I went with gRPC (which works over HTTP/2 by default and uses OkHttp under the hood – correct me if I'm wrong). It has worked fine.

Problem

Because of reasons not important to this issue, I had to replace the existing gRPC networking with "plain" HTTP clients and servers. I chose to use OkHttp for the client running in the instrumentation app.

instrumentation app (OkHttp client) -- HTTP GET ---> app under test (Ktor server)

Here's how I build the client:

val client = OkHttpClient.Builder()
                .connectionSpecs(listOf(ConnectionSpec.CLEARTEXT))
                .connectTimeout(timeout, timeUnit)
                .readTimeout(timeout, timeUnit)
                .writeTimeout(timeout, timeUnit)
                .callTimeout(timeout, timeUnit)
                .build()

then I'm making a simple GET request to http://localhost:8081.

Unfortunately, when the client tries to make the request, the call fails because of Android security policy. Now, I know about <network-security-config> and that I can set android:usesCleartextTraffic="true" in AndroidManifest – but I really can't do that, since it'd be a breaking change for users of my testing framework. And it's quite ugly.

Stacktrace
09-19 09:56:39.514  5330  5349 I TestRunner: run started: 1 tests
09-19 09:56:39.515  5330  5349 I TestRunner: started: initializationError(pl.leancode.patrol.example.MainActivityTest)
09-19 09:56:39.515  5330  5349 E TestRunner: failed: initializationError(pl.leancode.patrol.example.MainActivityTest)
09-19 09:56:39.515  5330  5349 E TestRunner: ----- begin exception -----
09-19 09:56:39.517  5330  5349 E TestRunner: java.net.UnknownServiceException: CLEARTEXT communication to localhost not permitted by network security policy
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.RealConnection.connect(RealConnection.kt:188)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.ExchangeFinder.findConnection(ExchangeFinder.kt:226)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.ExchangeFinder.findHealthyConnection(ExchangeFinder.kt:106)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.ExchangeFinder.find(ExchangeFinder.kt:74)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.RealCall.initExchange$okhttp(RealCall.kt:255)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.kt:32)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.kt:95)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.kt:83)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:76)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at okhttp3.internal.connection.RealCall.execute(RealCall.kt:154)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.http4k.client.OkHttp$invoke$1.invoke(OkHttp.kt:39)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.http4k.client.OkHttp$invoke$1.invoke(OkHttp.kt:36)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at pl.leancode.patrol.contracts.PatrolAppServiceClient.performRequest(PatrolAppServiceClient.kt:47)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at pl.leancode.patrol.contracts.PatrolAppServiceClient.performRequest$default(PatrolAppServiceClient.kt:31)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at pl.leancode.patrol.contracts.PatrolAppServiceClient.listDartTests(PatrolAppServiceClient.kt:22)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at pl.leancode.patrol.PatrolAppServiceClient.listDartTests(PatrolAppServiceClient.kt:32)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at pl.leancode.patrol.PatrolJUnitRunner.listDartTests(PatrolJUnitRunner.java:114)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at pl.leancode.patrol.example.MainActivityTest.testCases(MainActivityTest.java:17)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at java.lang.reflect.Method.invoke(Native Method)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.Parameterized$RunnersFactory.allParameters(Parameterized.java:424)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.Parameterized$RunnersFactory.<init>(Parameterized.java:375)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.Parameterized$RunnersFactory.<init>(Parameterized.java:360)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.Parameterized.<init>(Parameterized.java:303)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at java.lang.reflect.Constructor.newInstance0(Native Method)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at java.lang.reflect.Constructor.newInstance(Constructor.java:343)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.internal.builders.AnnotatedBuilder.buildRunner(AnnotatedBuilder.java:104)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.internal.builders.AnnotatedBuilder.runnerForClass(AnnotatedBuilder.java:86)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.internal.runner.junit4.AndroidAnnotatedBuilder.runnerForClass(AndroidAnnotatedBuilder.java:63)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:70)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:37)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.internal.runner.AndroidRunnerBuilder.runnerForClass(AndroidRunnerBuilder.java:149)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.internal.runner.AndroidLogOnlyBuilder.runnerForClass(AndroidLogOnlyBuilder.java:93)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.internal.runner.ScanningTestLoader.doCreateRunner(ScanningTestLoader.java:50)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.internal.runner.TestLoader.getRunnersFor(TestLoader.java:64)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.internal.runner.TestRequestBuilder.build(TestRequestBuilder.java:835)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.runner.AndroidJUnitRunner.buildRequest(AndroidJUnitRunner.java:650)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at androidx.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:418)
09-19 09:56:39.517  5330  5349 E TestRunner: 	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2361)
09-19 09:56:39.517  5330  5349 E TestRunner: ----- end exception -----
09-19 09:56:39.518  5330  5349 I TestRunner: finished: initializationError(pl.leancode.patrol.example.MainActivityTest)

After looking into the stacktrace, I found this code:

if (route.address.sslSocketFactory == null) {
if (ConnectionSpec.CLEARTEXT !in route.address.connectionSpecs) {
throw UnknownServiceException("CLEARTEXT communication not enabled for client")
}
val host = route.address.url.host
if (!Platform.get().isCleartextTrafficPermitted(host)) {
throw UnknownServiceException(
"CLEARTEXT communication to $host not permitted by network security policy"
)
}

And I think it's too eager in blocking unencrypted HTTP traffic? Shouldn't it be Android OS to decide whether to throw the SecurityException, not OkHttp?

This use case (HTTP traffic on localhost) was working fine when using gRPC.

Questions

  • Any ideas why calling localhost worked when OkHttp was used as a transport for gRPC, but fails when used standalone?
  • I also tried using the Apache HTTP client, and it doesn't throw this exception. Is it because it doesn't depend on Android's HttpURLConnection, which since Android 5.0 is OkHttp under the hood?
@yschimke
Copy link
Collaborator

I think we are doing the right thing, it shouldn't be up to libraries to decide which hosts are safe to connect to. The app developer can configure this.

Shouldn't it be Android OS to decide whether to throw the SecurityException, not OkHttp?

It can't really, when you make a connection, you get a list of Dns hosts, then make socket connections. It's not really guaranteed to be correct if the OS makes socket level decisions about when to allow connections. Since OkHttp makes a raw socket connection, and then applies TLS on top, when would Android make this decision?

HTTP Client libraries need to implement the isCleartextTrafficPermitted check themselves, otherwise they are routing around Android security.

An example from Ktor ktorio/ktor#3725

Is it because it doesn't depend on Android's HttpURLConnection, which since Android 5.0 is OkHttp under the hood?

This isn't the same OkHttp. Both Grpc and Android have old forked copies of OkHttp 2.

@bartekpacia
Copy link
Author

bartekpacia commented Sep 19, 2023

Thank you very much for quick response @yschimke! I learn a lot from it.

I guess I'll try using the old OKHttp2 for now.

In the long term, would it make sense to add an exclusion (in the snippet above) to OkHttp for e.g. localhost? Or does it make no sense?

@yschimke
Copy link
Collaborator

No, I don't think OkHttp should override the network-security-config that the developer has put in place.

@swankjesse
Copy link
Member

Instead, your users should configure that thing to permit cleartext connections to localhost.

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