-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Enable TLS 1.1 and TLS 1.2 on Android 4.1-4.4 #9840
Conversation
I've done some tests and the same issue also happens on some Samsung devices with API 21. Solved by applying the solution also for API 21 |
By analyzing the blame information on this pull request, we identified @AndrewJack to be a potential reviewer. |
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 - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
@gotev That's strange, since API 20 it should not be necessary. But maybe Samsung tampered a lot with their Android implementation. So maybe we should then drop the patches API check? Currently it is: if (Build.VERSION.SDK_INT >= 16 && Build.VERSION.SDK_INT < 20) |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@fkoester that's true for stock Android roms. I thought that all the other vendors were aligned, but maybe I was wrong, since the same problem arises on a Samsung rom with API 21. To solve that I simply changed the if statement: if (Build.VERSION.SDK_INT >= 16 && Build.VERSION.SDK_INT < 22) |
It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @AndrewJack as a potential reviewer. Could you take a look please or cc someone with more context? |
Please merge. Stripe api don't acept tls < 1.2 anymore |
Same issue Stripe do not accept tls < 1.2. Does anyone else have the same issue? |
cc @bestander or @mkonicek could you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few suggestions
|
||
client.connectionSpecs(specs); | ||
} catch (Exception exc) { | ||
Log.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use com.facebook.common.logging.FLog
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
See also https://github.com/facebook/react-native/issues/7192 | ||
*/ | ||
public static OkHttpClient.Builder enableTls12OnPreLollipop(OkHttpClient.Builder client) { | ||
if (Build.VERSION.SDK_INT >= 16 && Build.VERSION.SDK_INT < 20) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Build.VERSION_CODES for api levels
Build.VERSION_CODES.JELLY_BEAN
== 16
Build.VERSION_CODES.KITKAT_WATCH
== 20 - maybe use KITKAT
(v19) with a <=
for readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
available but not enabled by default. The following method | ||
enables it. | ||
|
||
See also https://github.com/facebook/react-native/issues/7192 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, I removed it.
It looks reasonable to me considering that it is off by default.
|
2b5b494
to
67ee28e
Compare
Credits to Alex Gotev (@gotev) for the nice implementation.
67ee28e
to
fc7d451
Compare
I implemented the suggestions made by @AndrewJack and rebased the PR to latest master. |
Andrew, shipit when ready |
@facebook-github-bot shipit |
@lacker has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: This is a proposed patch for issue facebook#7192. Android 4.1-4.4 has support for TLS 1.1 and 1.2 but it is disabled by default. Because of the known security issues and more and more servers switching to TLS 1.2 only, it would be nice for react-native to enable this support. I demonstrated a demo application which showcases the problem and can be used to test this patch. All sources and documentation for it can be found here: https://github.com/bringnow/react-native-tls-test Credits to Alex Gotev (gotev) for the nice implementation. Closes facebook#9840 Differential Revision: D4099446 Pulled By: lacker fbshipit-source-id: 94db320dce6d27f98169e63f834562360c00eef7
I don't understand: Also, this comment #7192 (comment) does not seem to work for me. Maybe it is only for api 16? We want to support 4.4 (api 19) and up. |
This is a proposed patch for issue #7192.
Android 4.1-4.4 has support for TLS 1.1 and 1.2 but it is disabled by default. Because of the known security issues and more and more servers switching to TLS 1.2 only, it would be nice for react-native to enable this support.
I demonstrated a demo application which showcases the problem and can be used to test this patch. All sources and documentation for it can be found here:
https://github.com/bringnow/react-native-tls-test
Credits to Alex Gotev (@gotev) for the nice implementation.