duosecurity / duo_universal_java

Duo OIDC-based two-factor authentication for Java web applications
https://duo.com/docs/duoweb
Other
14 stars 19 forks source link

Upgrade OkHttp dependency to 4.3+ #23

Open gpfjeff opened 11 months ago

gpfjeff commented 11 months ago

The current version of duo_universal_java (1.1.3) is using OkHttp 3.14.9 under the hood. This version of OkHttp has a known issue with Tomcat applications where its internal thread pool cannot be shut down cleanly because it does not provide an API to signal OkHttp to shut them down. This was supposedly fixed in version 4.3.

We have been mandated to Duo as our corporate MFA solution, which we have successfully implemented and deployed to production. However, we are now seeing evidence of the OkHttp thread pool issue in our server logs:

03-Aug-2023 06:38:26.643 WARNING [Thread-290707] org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads The web application [XXXXXXX] appears to have started a thread named [OkHttp ConnectionPool] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread:
 java.base@11.0.18/java.lang.Object.wait(Native Method)
 java.base@11.0.18/java.lang.Object.wait(Object.java:462)
 okhttp3.internal.connection.RealConnectionPool.lambda$new$0(RealConnectionPool.java:62)
 okhttp3.internal.connection.RealConnectionPool$$Lambda$1771/0x00000008002bd440.run(Unknown Source)
 java.base@11.0.18/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
 java.base@11.0.18/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
 java.base@11.0.18/java.lang.Thread.run(Thread.java:829)

This isn't causing any critical issues for our apps, but it is a nuisance when we need to shutdown or restart an app.

Please investigate updating the OkHttp dependency to 4.3 or later to resolve this issue.

gpfjeff commented 11 months ago

For what it's worth, I tried overriding OkHttp in our local POM to bring it up to 4.3.1 (the latest in the 4.3 branch on Maven Central) and it didn't help. We're still getting the same error. Then again, I didn't think it would work anyway, as I suspect either Duo would need to do the thread shutdown itself, or (more likely) expose an API for the surrounding app to call the shutdown when it shuts down.

AaronAtDuo commented 11 months ago

We get the okhttp dependency from retrofit2 which annoyingly hasn't put out a new version for three years now. I'm wary of overriding the okhttp version manually but that might be what we need to do...

necouchman commented 10 months ago

I'm seeing this same issue trying to use 1.1.3 version of Duo Universal SDK. Any idea when this dependency issue will be resolved?

The issue I see in building this for the Guacamole project is that the Maven enforcer plugin fails to converge the dependencies:

[INFO] --- maven-enforcer-plugin:3.0.0-M3:enforce (enforce) @ guacamole-auth-duo ---
[WARNING] 
Dependency convergence error for com.squareup.okhttp3:okhttp:3.14.9 paths to dependency are:
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.duosecurity:duo-universal-sdk:1.1.3
    +-com.squareup.retrofit2:retrofit:2.9.0
      +-com.squareup.okhttp3:okhttp:3.14.9
and
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.duosecurity:duo-universal-sdk:1.1.3
    +-com.squareup.okhttp3:logging-interceptor:4.9.1
      +-com.squareup.okhttp3:okhttp:4.9.1

[WARNING] Rule 0: org.apache.maven.plugins.enforcer.DependencyConvergence failed with message:
Failed while enforcing releasability. See above detailed error message.
[WARNING] Rule 1: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for com.squareup.okhttp3:okhttp:3.14.9 paths to dependency are:
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.duosecurity:duo-universal-sdk:1.1.3
    +-com.squareup.retrofit2:retrofit:2.9.0
      +-com.squareup.okhttp3:okhttp:3.14.9
and
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.duosecurity:duo-universal-sdk:1.1.3
    +-com.squareup.okhttp3:logging-interceptor:4.9.1
      +-com.squareup.okhttp3:okhttp:4.9.1
AaronAtDuo commented 10 months ago

The concern is, we get the okhttp dependency indirectly via retrofit, which is now 3.5 years without an update. Overriding transitive dependency versions is a wonderful way to introduce instability and bugs :)

I'll see what we can do, but I'm not loving any of my options.

necouchman commented 10 months ago

Thanks @AaronAtDuo, totally understand the concerns with what you're trying to do. I'll have to see if there's a way I can get Maven Enforcer to bypass that particular check and continue the build, and see what happens. It seems to happen in all of the versions of the Duo Universal SDK for Java that I tried - 1.0.3 and 1.1.1 - 1.1.3, so I'm struggling to find a way to actually use the updated SDK at the moment.