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

Move to use okhttp4 due to a bug on IPV6 based cluster #2632

Closed
ppatierno opened this issue Nov 25, 2020 · 28 comments · Fixed by #6441
Closed

Move to use okhttp4 due to a bug on IPV6 based cluster #2632

ppatierno opened this issue Nov 25, 2020 · 28 comments · Fixed by #6441

Comments

@ppatierno
Copy link
Contributor

The current version of fabric8 Kubernetes client is using okhttp3 library which is affected by bug on hostname verification when connecting to the Kubernetes server API in IPV6 based cluster.
The same bug affects even the kubernetes-client Java library: kubernetes-client/java#1289
This issue was fixed in okhttp4 as you can see from this already merged PR:
square/okhttp#5889

We are facing this while using fabric8 in the Strimzi operator. There is a workaround as described here strimzi/strimzi-kafka-operator#4002 but it's just a workaround.
Is there any plan to update the fabric8 client to the okhttp4 library version?

@rohanKanojia
Copy link
Member

Isn't okhttp4 based on kotlin? Not sure if it can break any backward compatibility. @manusa @oscerd @iocanel @metacosm WDYT??

@oscerd
Copy link
Member

oscerd commented Nov 26, 2020

Yes, it is. It's a completely different library from 3.x

@ppatierno
Copy link
Contributor Author

So what's the solution you envisage? Back porting the fix about IPV6 from okhttp4 to okhttp3 and then updating fabric8? Right now fabric8 is actually bugged with IPV6 clusters due to this problem in okhttp3 :-(

@oscerd
Copy link
Member

oscerd commented Dec 1, 2020

Bumping to 4.x is something that require effort and time. I'm not sure they will backport the fix on 3.x anyway

@ppatierno
Copy link
Contributor Author

Well,, that could be a contribution to backport on okhttp3 if they won't do that. Otherwise, you should list this as a well-known bug on fabric8.

@manusa
Copy link
Member

manusa commented Dec 9, 2020

Bumping OkHTTP version to 4.x will not only require a lot of work in the client, but some work on the Kubernetes Client Quarkus extension too (I think).

I agree that we should eventually upgrade to 4.x, new issues are bound to happen in the future. But I'm not sure if the timing is right at the moment, since we need to deliver 5.0 ASAP.

Maybe for the time being contributing a backport of the fix to OkHttp 3.x is more cost-effective WDYT?

@metacosm
Copy link
Collaborator

metacosm commented Dec 9, 2020

Maybe for the time being contributing a backport of the fix to OkHttp 3.x is more cost-effective WDYT?

Are we confident the backport would be accepted, though? Or are we talking about maintaining a 3.x fork until the time when we decide to switch to 4?

@manusa
Copy link
Member

manusa commented Dec 10, 2020

My suggestion was just (for now) try to see if the back-port is accepted in the OkHttp project, since it's only a few lines (square/okhttp#5889 - haven't checked branch compatibility though).
Maintaining an OkHttp is not what I was implying. That would probably be a much greater effort than upgrading the dependency in the client.

@yschimke
Copy link

OkHttp 4.x is binary backwards compatible with OkHttp 3.x. The only breaking experience would be if you have kotlin clients building as it's not source compatible. Full description here https://square.github.io/okhttp/upgrading_to_okhttp_4/

It is not a whole new library, clients can upgrade and downgrade (and do, like in Retrofit) since the public API is maintained.

3.14.x is no longer maintained, so you should upgrade to 4.x unless you wanted legacy support with 3.12.12 (Java 7, Android < 5). See https://square.github.io/okhttp/security/

@maxandersen
Copy link

okhttp drags in kotlin runtime,right ? that brings its own set of challenges.

@yschimke
Copy link

I'm not aware of any actual runtime. It uses a kotlin stdlib library, but nothing like a JVM runtime.

https://mvnrepository.com/artifact/org.jetbrains.kotlin/kotlin-stdlib-jdk8/1.4.21-2

This is around 20kb.

@titisan
Copy link

titisan commented Feb 24, 2021

Any progress with this issue?
We are facing the same problem in a IPv6 Kubernetes cluster.

@stale
Copy link

stale bot commented May 25, 2021

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@maxandersen
Copy link

Still relevant

@Fabian-K
Copy link
Contributor

Hi,

is it already set in stone that fabric8 client replaces okhttp 3 with vert.x client? What are the main advantages from moving to vert.x client?

Sometimes I see transitive dependencies of okhttp as a motivation. I´m wondering where that comes from because when comparing the dependencies of io.vertx:vertx-web-client:4.0.3 and com.squareup.okhttp3:okhttp:4.9.1 the dependency trees look like this:

\--- com.squareup.okhttp3:okhttp:4.9.1
     +--- com.squareup.okio:okio:2.8.0
     |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.4.0 -> 1.4.10
     |    |    +--- org.jetbrains.kotlin:kotlin-stdlib-common:1.4.10
     |    |    \--- org.jetbrains:annotations:13.0
     |    \--- org.jetbrains.kotlin:kotlin-stdlib-common:1.4.0 -> 1.4.10
     \--- org.jetbrains.kotlin:kotlin-stdlib:1.4.10 (*)
\--- io.vertx:vertx-web-client:4.0.3
     +--- io.vertx:vertx-web-common:4.0.3
     |    \--- io.vertx:vertx-core:4.0.3
     |         +--- io.netty:netty-common:4.1.60.Final
     |         +--- io.netty:netty-buffer:4.1.60.Final
     |         |    \--- io.netty:netty-common:4.1.60.Final
     |         +--- io.netty:netty-transport:4.1.60.Final
     |         |    +--- io.netty:netty-common:4.1.60.Final
     |         |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    \--- io.netty:netty-resolver:4.1.60.Final
     |         |         \--- io.netty:netty-common:4.1.60.Final
     |         +--- io.netty:netty-handler:4.1.60.Final
     |         |    +--- io.netty:netty-common:4.1.60.Final
     |         |    +--- io.netty:netty-resolver:4.1.60.Final (*)
     |         |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |    \--- io.netty:netty-codec:4.1.60.Final
     |         |         +--- io.netty:netty-common:4.1.60.Final
     |         |         +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |         \--- io.netty:netty-transport:4.1.60.Final (*)
     |         +--- io.netty:netty-handler-proxy:4.1.60.Final
     |         |    +--- io.netty:netty-common:4.1.60.Final
     |         |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |    +--- io.netty:netty-codec:4.1.60.Final (*)
     |         |    +--- io.netty:netty-codec-socks:4.1.60.Final
     |         |    |    +--- io.netty:netty-common:4.1.60.Final
     |         |    |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    |    +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |    |    \--- io.netty:netty-codec:4.1.60.Final (*)
     |         |    \--- io.netty:netty-codec-http:4.1.60.Final
     |         |         +--- io.netty:netty-common:4.1.60.Final
     |         |         +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |         +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |         +--- io.netty:netty-codec:4.1.60.Final (*)
     |         |         \--- io.netty:netty-handler:4.1.60.Final (*)
     |         +--- io.netty:netty-codec-http:4.1.60.Final (*)
     |         +--- io.netty:netty-codec-http2:4.1.60.Final
     |         |    +--- io.netty:netty-common:4.1.60.Final
     |         |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |    +--- io.netty:netty-codec:4.1.60.Final (*)
     |         |    +--- io.netty:netty-handler:4.1.60.Final (*)
     |         |    \--- io.netty:netty-codec-http:4.1.60.Final (*)
     |         +--- io.netty:netty-resolver:4.1.60.Final (*)
     |         +--- io.netty:netty-resolver-dns:4.1.60.Final
     |         |    +--- io.netty:netty-common:4.1.60.Final
     |         |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    +--- io.netty:netty-resolver:4.1.60.Final (*)
     |         |    +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |    +--- io.netty:netty-codec:4.1.60.Final (*)
     |         |    +--- io.netty:netty-codec-dns:4.1.60.Final
     |         |    |    +--- io.netty:netty-common:4.1.60.Final
     |         |    |    +--- io.netty:netty-buffer:4.1.60.Final (*)
     |         |    |    +--- io.netty:netty-transport:4.1.60.Final (*)
     |         |    |    \--- io.netty:netty-codec:4.1.60.Final (*)
     |         |    \--- io.netty:netty-handler:4.1.60.Final (*)
     |         \--- com.fasterxml.jackson.core:jackson-core:2.11.3
     +--- io.vertx:vertx-auth-common:4.0.3
     |    \--- io.vertx:vertx-core:4.0.3 (*)
     \--- io.vertx:vertx-core:4.0.3 (*)

Also from a size point of view, its ~2.6 MB vs ~5.3 MB (results of building a fat jar with just okhttp or vert.x).

I agree that the project should replace okhttp 3 rather sooner than later. And if you prefer vert.x over okhttp, that's perfectly fine. Just interested in the reasons for it :)

@maxandersen
Copy link

One issue is having a dependency that requires a full kotlin build to rebuild from source. (Afaik)

Other is that by using vert.x we can share and optimize http stacks with frameworks like vert.x itself and quarkus.

Optimally the http client layer would be pluggable but not sure how feasible that is.

@yschimke
Copy link

What do you mean about full Kotlin build? And from source? OkHttp itself is near 100% Kotlin. But it's just a java jar when you use it as a dependency.

@rohanKanojia
Copy link
Member

rohanKanojia commented Jul 14, 2021

@ppatierno: Hello, I'm looking into this issue. I reproduced your issue by using KubernetesClient with a kind cluster with ipv6 configuration specified in kind docs[0]. Are you able to resolve your issue by excluding okhttp3 dependencies introduced by kubernetes-client jar and adding direct okhttp 4 dependencies?

I tested this on a simple reproducer project[1] on a ipv6 kind cluster and it seems to be working fine for me.

    <properties>
        <fabric8.version>5.5.0</fabric8.version>
        <okhttp.version>4.9.0</okhttp.version>
    </properties>

    <dependencies>
        <dependency>
            <groupId>io.fabric8</groupId>
            <artifactId>kubernetes-client</artifactId>
            <version>${fabric8.version}</version>
            <exclusions>
                <exclusion>
                    <groupId>com.squareup.okhttp3</groupId>
                    <artifactId>okhttp</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>com.squareup.okhttp3</groupId>
                    <artifactId>logging-interceptor</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>com.squareup.okhttp3</groupId>
            <artifactId>okhttp</artifactId>
            <version>${okhttp.version}</version>
        </dependency>
        <dependency>
            <groupId>com.squareup.okhttp3</groupId>
            <artifactId>logging-interceptor</artifactId>
            <version>${okhttp.version}</version>
        </dependency>
    </dependencies>

I also checked to upgrade Okhttp in KubernetesClient to v4.9.0:

I was able to get that reproducer project working on Kind ipv6 cluster with 5.6-SNAPSHOT(based on my branch). But I need to test it further to see if it's completely safe to upgrade.

I tested this with Quarkus KubernetesClient Extension as well, I'm able to get it working on a Kubernetes ipv6 cluster with the above exclusions. You can see the full project here.

[0] https://kind.sigs.k8s.io/docs/user/configuration/#ip-family
[1] https://github.com/rohankanojia-forks/fabric8-okhttp-ipv6-k8s-cluster-bug-reproducer

@ovolynets
Copy link

ovolynets commented Aug 4, 2021

Hello community!

I would also be interested in either upgrading okhttp or the migration to Vert.x - whatever is decided upon. Reason: CVE-2021-0341 reported by NexusIQ in okhttp in versions prior to 4.9.1, see square/okhttp#6724 for details.

The upgrade of okhttp to 4.9.1 or the most recent 5.0.0-alpha.2 would be what fixes the CVE but if you decide to move to Vert.x this would also solve the problem.

Who can make the decision or either ways to proceed with eliminating the CVE rather than implementing a workaround (e.g. as was suggested earlier for a different problem #2632 (comment))?

Thanks

UPD: 4.9.1 version of okhttp does NOT contain yet the necessary fix, only 5.0.0-alpha.2 does.

@yschimke
Copy link

yschimke commented Aug 4, 2021

@ovolynets the CVE isn't exploitable unless you take the HostnameVerifier from OkHttp and use it for other purposes. Unless you are doing something like that which would be really strange, it isn't exploitable at all.

@tomdw
Copy link

tomdw commented Nov 27, 2023

Are there still planes to upgrade to okhttp4? the 3.x version depends on an older junit dependency from kubernetes-server-mock through mockwebserver of okhttp3. And this gives CVE vulnerability warning in the IDE.

@maxandersen
Copy link

@manusa Kubernetes client does not rely on okhttp anymore - it's uses vertx.

Mockserver using okhttp is unfortunate. Not sure what the cure is here. For now you can manually override the version.

@manusa
Copy link
Member

manusa commented Nov 27, 2023

Hi @maxandersen

@manusa Kubernetes client does not rely on okhttp anymore - it's uses vertx.

I'm not sure if your intention was to tag me here or you wanted to reply to @tomdw instead.

Fabric8 Kubernetes Client relies on OkHttp for its kubernetes-httpclient-okhttp HttpClient implementation and the Kubernetes Mock Web Server.

@tomdw, as Max said, you should be able to override the OkHttp version for MockWebServer by forcing the dependency to a newer OkHttp version like documented here.

Mockserver using okhttp is unfortunate. Not sure what the cure is here.

Our kubernetes-server-mock and mockwebserver modules rely on OkHttp's mockwebserver as its backend.
One of the options I was considering was to replace this with a Vertx or a Jetty server. However, this will require additional work since right now everything is tightly coupled to OkHttp. (discussed in #5613)

@manusa
Copy link
Member

manusa commented Aug 14, 2024

As part of the #5778 initiative, we should no longer support OkHttp v3.

This issue is now the placeholder for all tasks related to the bump of OkHttp to the latest v4.

Changes should be smooth since OkHttp 4 should be binary and source compatible with OkHttp 3, for any incompatibilities please check https://square.github.io/okhttp/changelogs/upgrading_to_okhttp_4/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment