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

Offer Duck Typing Fallback for X509TrustManagerExtensions on pre-Android N #2646

Closed
commonsguy opened this issue Jun 21, 2016 · 2 comments
Closed

Comments

@commonsguy
Copy link

On API Level 23 and below, X509TrustManagerExtensions only works if the X509TrustManager supplied to the X509TrustManagerExtensions constructor is the internal TrustManagerImpl implementation. Otherwise, it throws an IllegalArgumentException, with a message of tm is an instance of ... which is not a supported type of X509TrustManager. The Android N implementation uses duck typing, so it can correctly handle any X509TrustManager that supports the required methods (e.g., four-parameter checkServerTrusted()).

Right now, CertificateChainCleaner.get() tries to create an X509TrustManagerExtensions, and if that throws any Exception, falls back to BasicCertificateChainCleaner. Since the X509TrustManagerExtensions constructor fails for anything other than a TrustManagerImpl, this means that CertificateChainCleaner.get() falls back to BasicCertificateChainCleaner for anything other than a TrustManagerImpl.

I am trying to create a backport of the Android N network security configuration code. That requires a custom X509TrustManager called RootTrustManager. This works on N, because of the duck typing. It fails on Android 6.0, because RootTrustManager is not a TrustManagerImpl, and so OkHttp3 does not use the AndroidCertificateChainCleaner, since it fails to create the X509TrustManagerExtensions.

The only way that I can see of getting this to work is if CertificateChainCleaner.get() had an intermediate fallback option. If the X509TrustManagerExtensions constructor fails, try creating an instance of an alternative X509TrustManagerExtensions, forked from the Android N source, that implements the duck typing bit without the references to TrustManagerImpl (since that's an internal class). If that fails, use BasicCertificateChainCleaner. This way, CertificateChainCleaner.get() and AndroidCertificateChainCleaner could handle any X509TrustManager, not just TrustManagerImpl.

@commonsguy
Copy link
Author

While this might matter to others, it turns out that my real problem was another spot in Android where Android wants to work with TrustManagerImpl instead of any X509TrustManager. I have a prototype icky workaround using interceptors to allow for hostname-based checks from a regular X509TrustManager via the two-parameter checkServerTrusted().

I'll leave this open in case you think the issue might matter to others. Sorry to have bothered you prematurely.

@swankjesse
Copy link
Collaborator

Yeah, these APIs are really awkward to deal with. They’re overly general, incomplete, and difficult to use correctly. Happy to chat if you have ideas on how to make it work better. Mostly our goal is to make it do the right thing without any configuration, which as you can see requires a lot of work on our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants