okhttp 4.3.0 crash with custom X509TrustManager

[laalto]

Android 10 specific.

Custom X509TrustManager in use, extending from X509ExtendedTrustManager, installed with OkHttpClient.Builder#sslSocketFactory().

Updating okhttp from 4.2.2 to 4.3.0 causes a crash when installing the trust manager:

java.lang.IllegalArgumentException: Required method checkServerTrusted(X509Certificate[], String, String, String) missing        
        at android.net.http.X509TrustManagerExtensions.<init>(X509TrustManagerExtensions.java:71)
        at okhttp3.internal.platform.android.Android10CertificateChainCleaner.<init>(Android10CertificateChainCleaner.kt:36)
        at okhttp3.internal.platform.Android10Platform.buildCertificateChainCleaner(Android10Platform.kt:62)
        at okhttp3.internal.tls.CertificateChainCleaner$Companion.get(CertificateChainCleaner.kt:42)
        at okhttp3.OkHttpClient$Builder.sslSocketFactory(OkHttpClient.kt:735)

Looking up the commits this seems to be introduced by https://github.com/square/okhttp/commit/a41361efcb0d4ed2e7f09313c9e9fcc3d72e837b that cut down on reflective calls on the trust manager.

Fixed this by adding an implementation of the missing method in the custom trust manager. Note the Android platform exception message is off-by-one String arg.

        private X509ExtendedTrustManager delegate;
        private Method checkServerTrusted;

        CustomExtendedTrustManager(@NonNull X509ExtendedTrustManager delegate) {
            this.delegate = delegate;

            try {
                checkServerTrusted = delegate.getClass().getMethod("checkServerTrusted",
                        X509Certificate[].class,
                        String.class,
                        String.class);
            } catch (NoSuchMethodException ignored) {
            }
        }

        @SuppressWarnings("unused")
        @Keep
        public List<X509Certificate> checkServerTrusted(X509Certificate[] chain, String authType, String host) throws CertificateException {
            if (checkServerTrusted == null) {
                throw new CertificateException("checkServerTrusted(X509Certificate[], String, String) not implemented in delegate");
            }

            List<X509Certificate> list;
            try {
                //noinspection unchecked
                list = (List<X509Certificate>) checkServerTrusted.invoke(delegate, chain, authType, host);
            } catch (IllegalAccessException e) {
                throw new CertificateException("Failed to call checkServerTrusted", e);
            } catch (InvocationTargetException e) {
                if (e.getCause() instanceof CertificateException) {
                    throw (CertificateException) e.getCause();
                }
                if (e.getCause() instanceof RuntimeException) {
                    throw (RuntimeException) e.getCause();
                }
                throw new CertificateException("checkServerTrusted failed", e.getCause());
            }

            return list;
        }

With this in place the app is no longer crashing and not really expecting okhttp to change here. Posting this issue as a reference to others who might have the same issue.

[yschimke]

Thanks, I'll add a test specific for this on android.

[swankjesse]

@laalto this is one of the nicest issue reports I’ve seen. Thanks!

[swankjesse]

And a warning in our API doc: https://github.com/square/okhttp/pull/5685

[swankjesse]

(whoops, leaving open in anticipation of @yschimke’s test!)

[yschimke]

OK so the behaviour change here is that prior to Android 10 (Q) we ran this code

    fun build(trustManager: X509TrustManager): AndroidCertificateChainCleaner? = try {
      val extensionsClass = Class.forName("android.net.http.X509TrustManagerExtensions")
      val constructor = extensionsClass.getConstructor(X509TrustManager::class.java)
      val extensions = constructor.newInstance(trustManager)
      val checkServerTrusted = extensionsClass.getMethod(
          "checkServerTrusted", Array<X509Certificate>::class.java, String::class.java,
          String::class.java)
      AndroidCertificateChainCleaner(trustManager, extensions, checkServerTrusted)
    } catch (_: Exception) {
      null
    }

So on any failure like this we defaulted to non Android Platform version.

  override fun buildCertificateChainCleaner(trustManager: X509TrustManager): CertificateChainCleaner =
        AndroidCertificateChainCleaner.build(trustManager) ?: super.buildCertificateChainCleaner(trustManager)

Should we revert to that behaviour? Or require people add the method?

[yschimke]

Sorry but not sorry for breaking this. We should probably be a lot more intentional here and then release a 4.3.1? this weekend?

Fortunately this only shows up when you upgrade and there is a workaround. Unfortunately, it may not flag in local testing before releasing unless you run with Android 10.

[yschimke]

PR for a fix here https://github.com/square/okhttp/pull/5688

[swankjesse]

Nice turnaround. I’ll cut a 4.3.1 when this lands.

[yschimke]

There is a secondary bug, it still fails if you have logging e.g. the existing client test rule EventListener installed. I'll make a repro and file a bug for that.

[swankjesse]

FYI, OkHttp 4.3.1 is released.

[laalto]

Thanks, much appreciated!