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

KTOR-6182 Support throwing UnknownServiceException if there is no cleartext traffic permitted #3725

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
@@ -0,0 +1,61 @@
/*
* Copyright 2014-2023 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.cio

import java.lang.reflect.InvocationTargetException

internal actual object Platform {
/**
* This explicit check avoids activating in Android Studio with Android specific classes available when running plugins inside the IDE.
*/
private val isAndroid: Boolean = System.getProperty("java.vm.name") == "Dalvik"

actual fun isCleartextTrafficPermitted(hostname: String): Boolean {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't the full story for OkHttp.

We compile against robolectric android-all in order to call these APIs directly.

https://github.com/square/okhttp/blob/okhttp_4x/okhttp/src/main/kotlin/okhttp3/internal/platform/AndroidPlatform.kt

It's way simpler than the reflection.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I borrowed these code from the old implementation, cause I see there is no Android Runtime applied in CIO module.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for OkHttp. It's a compile only dependency

if (!isAndroid) return true
return try {
val networkPolicyClass = Class.forName("android.security.NetworkSecurityPolicy")
val getInstanceMethod = networkPolicyClass.getMethod("getInstance").apply { isAccessible = true }
val networkSecurityPolicy = getInstanceMethod.invoke(null)
api24IsCleartextTrafficPermitted(hostname, networkPolicyClass, networkSecurityPolicy)
} catch (_: ClassNotFoundException) {
true
} catch (_: NoSuchMethodException) {
true
} catch (e: IllegalAccessException) {
throw AssertionError("unable to determine cleartext support", e)
} catch (e: IllegalArgumentException) {
throw AssertionError("unable to determine cleartext support", e)
} catch (e: InvocationTargetException) {
throw AssertionError("unable to determine cleartext support", e)
}
}

@Throws(InvocationTargetException::class, IllegalAccessException::class)
private fun api24IsCleartextTrafficPermitted(
hostname: String,
networkPolicyClass: Class<*>,
networkSecurityPolicy: Any
): Boolean = try {
val isCleartextTrafficPermittedMethod = networkPolicyClass
.getMethod("isCleartextTrafficPermitted", String::class.java)
.apply { isAccessible = true }
isCleartextTrafficPermittedMethod.invoke(networkSecurityPolicy, hostname) as Boolean
} catch (_: NoSuchMethodException) {
api23IsCleartextTrafficPermitted(networkPolicyClass, networkSecurityPolicy)
}

@Throws(InvocationTargetException::class, IllegalAccessException::class)
private fun api23IsCleartextTrafficPermitted(
networkPolicyClass: Class<*>,
networkSecurityPolicy: Any
): Boolean = try {
val isCleartextTrafficPermittedMethod = networkPolicyClass
.getMethod("isCleartextTrafficPermitted")
.apply { isAccessible = true }
isCleartextTrafficPermittedMethod.invoke(networkSecurityPolicy) as Boolean
} catch (_: NoSuchMethodException) {
true
}
}
Expand Up @@ -16,6 +16,7 @@ import io.ktor.util.*
import io.ktor.util.date.*
import io.ktor.utils.io.*
import io.ktor.utils.io.core.*
import io.ktor.utils.io.errors.UnknownServiceException
import kotlinx.atomicfu.*
import kotlinx.coroutines.*
import kotlinx.coroutines.channels.*
Expand Down Expand Up @@ -199,6 +200,12 @@ internal class Endpoint(
connections.incrementAndGet()

try {
if (!Platform.isCleartextTrafficPermitted(host)) {
throw UnknownServiceException(
"CLEARTEXT communication to $host not permitted by network security policy"
)
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very sure if we should put this check here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do it earlier, before creating connection. Maybe even in the CIOEngine.executemethod. Also, don't you need to check that the traffic is clear text?

Copy link
Author

@Goooler Goooler Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can, but I see this check had been seated at a simular position in Okhttp, not very sure why.

@yschimke Can you take a look?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know enough about CIO to comment, is this HTTP only here? Or could it be HTTPS also?

Probably worth adding tests to validate the behaviour for both cases.

Some extra context for OkHttp. We position it

  • After application interceptors have a chance to modify the request, in case they have an AlwaysSecureInterceptor configured.
  • Before we connect via various options - proxies, or IPv4/IPv6 fallback, which might involve connecting to an IP instead of a host. So we avoid working around the platform restrictions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be HTTP or HTTPS here. Thanks for your explanation, especially for the second point.


repeat(connectAttempts) {
val address = InetSocketAddress(host, port)

Expand Down
@@ -0,0 +1,10 @@
/*
* Copyright 2014-2023 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.cio

internal expect object Platform {

fun isCleartextTrafficPermitted(hostname: String): Boolean
}
@@ -0,0 +1,9 @@
/*
* Copyright 2014-2023 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.cio

internal actual object Platform {
actual fun isCleartextTrafficPermitted(hostname: String): Boolean = true
}
2 changes: 2 additions & 0 deletions ktor-io/common/src/io/ktor/utils/io/errors/Errors.kt
Expand Up @@ -5,3 +5,5 @@ public expect open class IOException(message: String, cause: Throwable?) : Excep
}

public expect open class EOFException(message: String) : IOException

public expect open class UnknownServiceException(message: String) : IOException
2 changes: 2 additions & 0 deletions ktor-io/jvm/src/io/ktor/utils/io/errors/IOException.kt
Expand Up @@ -3,3 +3,5 @@ package io.ktor.utils.io.errors
public actual typealias IOException = java.io.IOException

public actual typealias EOFException = java.io.EOFException

public actual typealias UnknownServiceException = java.net.UnknownServiceException