-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
Can you explain what is the current behaviour and what is the benefit of this change, please?
If we are using
We should catch up what Okhttp did on Android, to provide the same experience. |
if (!Platform.isCleartextTrafficPermitted(host)) { | ||
throw UnknownServiceException( | ||
"CLEARTEXT communication to $host not permitted by network security policy" | ||
) | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.execute
method. Also, don't you need to check that the traffic is clear text?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ktor-client/ktor-client-cio/jvm/src/io/ktor/client/engine/cio/Platform.kt
Outdated
Show resolved
Hide resolved
if (!Platform.isCleartextTrafficPermitted(host)) { | ||
throw UnknownServiceException( | ||
"CLEARTEXT communication to $host not permitted by network security policy" | ||
) | ||
} |
There was a problem hiding this comment.
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.execute
method. Also, don't you need to check that the traffic is clear text?
*/ | ||
private val isAndroid: Boolean = System.getProperty("java.vm.name") == "Dalvik" | ||
|
||
actual fun isCleartextTrafficPermitted(hostname: String): Boolean { |
There was a problem hiding this comment.
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.
It's way simpler than the reflection.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Goooler, thank you for the PR.
It looks like this code is Android specific, it also could be helpful not only for CIO.
Could you please extract this check to the separate client plugin?
What do you mean? You mean I should move |
Yep! We also can install this plugin by default in case the VM is Dalvik |
import io.ktor.client.request.host | ||
import io.ktor.utils.io.errors.UnknownServiceException | ||
|
||
public val Platforming: ClientPlugin<Any> = createClientPlugin("Platforming", ::Any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I install this plugin for CIO by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this in the separate PR, it looks like it require service loader or some similar mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ktor-client/ktor-client-plugins/ktor-client-platform/nonJvm/src/Platform.kt
Outdated
Show resolved
Hide resolved
Seems the style check failure is not related to my change. |
# Conflicts: # ktor-io/common/src/io/ktor/utils/io/errors/Errors.kt # ktor-io/js/src/io/ktor/utils/io/errors/IOException.kt # ktor-io/jvm/src/io/ktor/utils/io/errors/IOException.kt # ktor-io/posix/src/io/ktor/utils/io/errors/IOException.kt
https://youtrack.jetbrains.com/issue/KTOR-6182
This part had been implemented in Okhttp & HttpURLConnection, we just need to patch it for CIO.
Staled some code from:
https://github.com/square/okhttp/blob/735ed1a6e5a15042f6d7e8f100fb3c8376f6aa27/okhttp/src/jvmMain/kotlin/okhttp3/internal/connection/RealRoutePlanner.kt#L205-L208
https://github.com/square/okhttp/blob/550cc11989e5117ed3d8afaf20c7b6c419b5351f/okhttp/src/main/kotlin/okhttp3/internal/platform/AndroidPlatform.kt#L105-L148