Skip to content

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).


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