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

Goooler · 2023-08-11T10:11:16Z

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

rsinukov

Thanks for the PR.

Can you explain what is the current behaviour and what is the benefit of this change, please?

Goooler · 2023-08-15T13:29:40Z

If we are using CIO to request an HTTP URL on Android 9+, there is no exception thrown (an example here), but thrown with the use of OkHttp or Android engine, cause the isCleartextTrafficPermitted check had been implemented by them, like this, this is a suggested behavior on Android:

This flag is honored on a best effort basis because it's impossible to prevent all cleartext traffic from Android applications given the level of access provided to them.

We should catch up what Okhttp did on Android, to provide the same experience.

Goooler · 2023-08-15T13:30:20Z

ktor-client/ktor-client-cio/jvmAndNix/src/io/ktor/client/engine/cio/Endpoint.kt

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


Not very sure if we should put this check here.

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?

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?

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.

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

rsinukov · 2023-08-16T14:22:05Z

ktor-client/ktor-client-cio/jvmAndNix/src/io/ktor/client/engine/cio/Endpoint.kt

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


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?

yschimke · 2023-08-16T15:55:23Z

ktor-client/ktor-client-cio/jvm/src/io/ktor/client/engine/cio/Platform.kt

+     */
+    private val isAndroid: Boolean = System.getProperty("java.vm.name") == "Dalvik"
+
+    actual fun isCleartextTrafficPermitted(hostname: String): Boolean {


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.

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

Same for OkHttp. It's a compile only dependency

e5l

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?

Goooler · 2024-01-12T12:49:51Z

What do you mean? You mean I should move Platform into a new module under ktor-client/ktor-client-plugins?

e5l · 2024-01-12T12:59:25Z

Yep! We also can install this plugin by default in case the VM is Dalvik

Goooler · 2024-01-13T08:38:28Z

ktor-client/ktor-client-plugins/ktor-client-platform/common/src/Platforming.kt

+import io.ktor.client.request.host
+import io.ktor.utils.io.errors.UnknownServiceException
+
+public val Platforming: ClientPlugin<Any> = createClientPlugin("Platforming", ::Any) {


How can I install this plugin for CIO by default?

let's make this in the separate PR, it looks like it require service loader or some similar mechanism

Goooler · 2024-01-13T08:42:11Z

ktor-client/ktor-client-plugins/ktor-client-platform/nonJvm/src/Platform.kt

+
+package io.ktor.client.plugins.platform
+
+internal actual object Platform {


Seems nonJvm source set hasn't been recognized, how can I implement this actual object for non-jvm platforms?

Hey @Goooler, it will need to be implemented for js and posix source sets.

To check the implementation in ide, you can invert the flag ktor.ide.jvmAndCommonOnly in gradle.properties https://github.com/ktorio/ktor/blob/92ece71d433f6d96754ef7d18ab2691e1f36160a/gradle.properties#L7C1-L7C1 and import project again

Goooler · 2024-01-17T10:01:37Z

Seems the style check failure is not related to my change.

Goooler added 2 commits August 11, 2023 17:13

Add Platform

7d3fb01

Throw UnknownServiceException

1641149

rsinukov reviewed Aug 15, 2023

View reviewed changes

rsinukov self-assigned this Aug 15, 2023

Goooler commented Aug 15, 2023

View reviewed changes

rsinukov suggested changes Aug 16, 2023

View reviewed changes

Goooler added 2 commits August 16, 2023 22:26

Merge branch 'main' into KTOR-6182

f0733ef

Fix typo

ced616f

yschimke reviewed Aug 16, 2023

View reviewed changes

yschimke mentioned this pull request Sep 19, 2023

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

Closed

e5l requested changes Jan 12, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into KTOR-6182

044d2b6

Split Platform into a new plugin module

54df7e7

Goooler force-pushed the KTOR-6182 branch from 48c0726 to 54df7e7 Compare January 13, 2024 08:37

Goooler commented Jan 13, 2024

View reviewed changes

Goooler added 3 commits January 15, 2024 19:18

Split to js and posix source sets

9ff3467

More comments

e8dd071

Rename classes

cb1e09c

Goooler force-pushed the KTOR-6182 branch from 5dfca43 to cb1e09c Compare January 15, 2024 11:32

Goooler added 4 commits January 16, 2024 17:20

Merge remote-tracking branch 'origin/main' into KTOR-6182

ed199d2

Define UnknownServiceException for posix

e048727

Define UnknownServiceException for js

140c37c

Try to fix style

a4a1c11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Goooler commented Aug 11, 2023

rsinukov left a comment

Goooler commented Aug 15, 2023 •

edited

Goooler Aug 15, 2023

rsinukov Aug 16, 2023

Goooler Aug 16, 2023 •

edited

yschimke Aug 16, 2023

Goooler Aug 17, 2023

rsinukov Aug 16, 2023

yschimke Aug 16, 2023

Goooler Aug 17, 2023

yschimke Aug 17, 2023

e5l left a comment

Goooler commented Jan 12, 2024

e5l commented Jan 12, 2024

Goooler Jan 13, 2024

e5l Jan 15, 2024

Goooler Jan 13, 2024

e5l Jan 15, 2024

Goooler Jan 15, 2024

Goooler commented Jan 17, 2024


		package io.ktor.client.plugins.platform

		internal actual object Platform {

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

Are you sure you want to change the base?

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

Conversation

Goooler commented Aug 11, 2023

rsinukov left a comment

Choose a reason for hiding this comment

Goooler commented Aug 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Goooler Aug 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e5l left a comment

Choose a reason for hiding this comment

Goooler commented Jan 12, 2024

e5l commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Goooler commented Jan 17, 2024

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

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

Goooler commented Aug 15, 2023 •

edited

Goooler Aug 16, 2023 •

edited