Increase Tls1.2.0 support on Android (5.0.1 and below) #17841

tuncaulubilge · 2018-02-02T15:58:22Z

Motivation

On older Android devices (5.0.1 and below), TLS1.2 is not supported by default. There is already a fix in place for this on React Native, but it requires some improvements. Added a few extra details to cover wider range of devices:

Current fix covers SDK 19 and below, but same issue is reported up to api 21 for some Samsung devices. Increased version check for the fix
Current fix only forces Tlsv1.2 but we should keep the support for 1.1 and 1.0, so switched to default connection specs from OkHttpClient
sslSocketFactory method in OkHttpClient is deprecated. Applied the recommended fix from OkHttpClient documentations. Deprecated method uses reflection to access TrustManager, which requires an extra proguard rule on projects.

Test Plan

This PR is an improvement on an existing fix. All tests are running as expected.

Related PRs

Previous commit on the issue: 55ebb89

Release Notes

[ANDROID] [BUGFIX][OkHttpClientProvider.java] Fixed issues with TLSv1.2 on older devices

facebook-github-bot · 2018-02-02T15:59:01Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

pull-bot · 2018-02-02T15:59:19Z

	Warnings
⚠️	📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

facebook-github-bot · 2018-02-02T17:13:10Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

ide · 2018-02-02T17:28:02Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java


        ConnectionSpec cs = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
-                .tlsVersions(TlsVersion.TLS_1_2)
+                .tlsVersions(TlsVersion.TLS_1_0, TlsVersion.TLS_1_1, TlsVersion.TLS_1_2)


Not sure we want to enable older TLS, especially 1.0. Probably better to leave it at 1.2 only.

TLS 1.0 is still supported by Android platform and by Okhttp, so react native should not force to TLS1.2 just to fix an issue. For Lolipop+, tls 1.0 is already available, so I'd rather enable it on older devices too in order to keep version behaviour consistent. By adding all tls versions, we let the server make the decision

Is there a way to see which protocols the Android device supports by default and add only more secure ones?

TLS version is decided together by the client and the server. Server and client both share the list of tls versions supported and the highest version that is supported on both platforms is selected.

Adding TLS 1.0 here is only useful if the server doesn't support TLS 1.1 and 1.2, which is possible for a development server or a qa environment.

I have a PR #17309 that relates to this. It is important to recognize that some Android environments do not support TLS v1.0. Does this code require checking the list of supported protocols explicitly?

At this point why have a custom ConnectionSpec instance. As far as I see you can just use ConnectionSpec.MODERN_TLS as is, without creating a builder on top of it. It is already set to

.tlsVersions(TlsVersion.TLS_1_3, TlsVersion.TLS_1_2, TlsVersion.TLS_1_1, TlsVersion.TLS_1_0)

In addition ConnectionSpec.COMPATIBLE_TLS is built on top of ConnectionSpec.MODERN.TLS.

So if I am not misunderstanding the entire specs list could just be

List<ConnectionSpec> specs = new ArrayList<>(); specs.add(ConnectionSpec.MODERN_TLS); specs.add(ConnectionSpec.CLEARTEXT);

Would this not be far simpler (unless I misunderstand OkHttp/TLS API)?

We still need COMPATIBLE_TLS as a fallback on older devices but it is a good idea to use MODERN_TLS instead of a custom builder. Updated in a6d30f7

ide · 2018-02-05T14:47:56Z

@hramos Would it be possible to find someone especially familiar with the networking code and security to review this carefully? In particular, this commit affects TLS and also assumes the app has just one TrustManager.

tuncaulubilge · 2018-02-05T14:54:09Z

Here is the javadoc by Okhttp about trustmanagers. https://github.com/square/okhttp/blob/9cb3af2a03870f54a0b49c4b26042e1ec98d3513/okhttp/src/main/java/okhttp3/OkHttpClient.java#L673

hey99xx · 2018-02-21T18:46:24Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java

-        specs.add(cs);
+        specs.add(ConnectionSpec.MODERN_TLS);
        specs.add(ConnectionSpec.COMPATIBLE_TLS);
        specs.add(ConnectionSpec.CLEARTEXT);


Now you have same as OkHttpClient.DEFAULT_CONNECTION_SPECS.

static final List<ConnectionSpec> DEFAULT_CONNECTION_SPECS = Util.immutableList( ConnectionSpec.MODERN_TLS, ConnectionSpec.COMPATIBLE_TLS, ConnectionSpec.CLEARTEXT);

I'll test this with default specs and we can remove this if it works for all devices. Important part is the TlsSocketFactory, which enables TLS1_1 & TLS1_2 on older devices. Before this PR, we were disabling TLS1_1 and TLS1_0 for newer ones, but it's better to stick with default specs from okhttp

I removed the custom connection specs overall, and tested on KitKat, this seems to be working as expected.

perrosnk · 2018-02-22T14:00:46Z

I still have a problem connecting to my server which uses Let's encrypt certificate, even after using this code.
I have opened this issue #18032

tuncaulubilge · 2018-02-22T15:28:51Z

@perrosnk Have you tried it on a native Android app with only OkHttp installed? Might be an issue with okHttp

facebook-github-bot · 2018-03-24T17:06:53Z

@tuncaulubilge I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

…ction

facebook-github-bot · 2018-04-26T17:06:10Z

@tuncaulubilge I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

perrosnk · 2018-11-22T00:28:04Z

`Any updates?

cpojer · 2019-03-13T06:50:22Z

What is the status of this PR?

cpojer · 2019-03-26T17:40:16Z

It seems this PR is outdated and hasn't gotten any attention in a long time. Additionally, very few people seem to care a ton about making this change. I recognize that this is still valuable though so if anyone feels strongly about putting up a new PR with a proper implementation of this and is able to answer all the questions we may have, please make it happen. For now, I'm going to close this one.

tuncaulubilge changed the title ~~Fix/tls120~~ Increase Tls1.2.0 support on Android (5.0.1 and below) Feb 2, 2018

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Feb 2, 2018

ide reviewed Feb 2, 2018

View reviewed changes

hey99xx reviewed Feb 21, 2018

View reviewed changes

sdwilsh removed the cla signed label Mar 1, 2018

react-native-bot added Android Ran Commands One of our bots successfully processed a command. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Mar 14, 2018

janicduplessis removed the Android label Mar 18, 2018

react-native-bot added Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. labels Mar 18, 2018

janicduplessis removed the Android 🤖 label Mar 19, 2018

tuncaulubilge added 5 commits March 27, 2018 10:11

Enable Tls 1.2 on lolipop and below

ffca5b9

Enable all versions of tls instead of just 1.2

1ce2b76

Inject trustmanager into sslSocketFactory instead of relying on refle…

937e8dc

…ction

Use MODERN_TLS instead of custom specs builder

03d3723

Remove custom connection specs and use default specs instead

b49af68

tuncaulubilge force-pushed the fix/tls120 branch from e2fd8c1 to b49af68 Compare March 27, 2018 09:13

react-native-bot added the ✅Test Plan label May 15, 2018

cpojer closed this Mar 26, 2019

Increase Tls1.2.0 support on Android (5.0.1 and below) #17841

Increase Tls1.2.0 support on Android (5.0.1 and below) #17841

Conversation

tuncaulubilge commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Test Plan

Related PRs

Release Notes

Uh oh!

facebook-github-bot commented Feb 2, 2018

Uh oh!

pull-bot commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ide commented Feb 5, 2018

Uh oh!

tuncaulubilge commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perrosnk commented Feb 22, 2018

Uh oh!

tuncaulubilge commented Feb 22, 2018

Uh oh!

facebook-github-bot commented Mar 24, 2018

Uh oh!

facebook-github-bot commented Apr 26, 2018

Uh oh!

perrosnk commented Nov 22, 2018

Uh oh!

cpojer commented Mar 13, 2019

Uh oh!

cpojer commented Mar 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tuncaulubilge commented Feb 2, 2018 •

edited

Loading

pull-bot commented Feb 2, 2018 •

edited

Loading