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

Fix SNI with latest Google Play Sevices #542

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pateljunaid
Copy link

No description provided.

@koush
Copy link
Owner

koush commented Jun 20, 2017

I know I had this exact code at some point, and then reverted. Couldn't use that method because some version of android would crap out. If I recall, I think it was hostname verification that failed on wildcards? I honestly can't remember. I need to do a regression test back to API 9 and see what it was.

@koush
Copy link
Owner

koush commented Jun 20, 2017

But if you are using conscrypt manually, you can insert a middleware that does this in AndroidAsync as well.

koush/ion@4c8529d

@mkonecny
Copy link
Contributor

@koush I believe SNI is broken for all devices without this changes. Wouldn't that have higher priority than keeping compatibility on older Android devices?

BTW, this PR is almost an exact replica of #538 but with merge conflict resolved. It's tested working on our 4.3 and 4.4 devices.

@mkonecny
Copy link
Contributor

mkonecny commented Jun 20, 2017

@koush with further testing, it appears that this PR breaks for all devices that are not using ProviderInstall.installIfNeeded. Was that the issue you had experienced?

Perhaps we need to run ProviderInstall.installIfNeeded in the AndroidAsync library if we detect it hasn't been run yet. Your thoughts?

@koush
Copy link
Owner

koush commented Jun 20, 2017

@mkonecny I believe this code should only be used when conscrypt is the underlying SSL stack. That means Android 5.0+ (I think), and obviously GPS conscrypt. I'll need look through the android release history and see exactly when they switched SSL stacks.

@koush
Copy link
Owner

koush commented Jun 20, 2017

I'm a bit conflicted about introducing GPS Conscrypt as an optional dependency to AndroidAsync (where it is right now in ion).

@@ -69,7 +69,7 @@ protected SSLEngine createConfiguredSSLEngine(GetSocketData data, String host, i
}

if (sslEngine == null)
sslEngine = sslContext.createSSLEngine();
sslEngine = sslContext.createSSLEngine(host, port);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

808

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

Successfully merging this pull request may close these issues.

4 participants