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

replace apache http w/okhttp, enable TLS1.2 for Android API<=21 #5658

Merged
merged 1 commit into from
Dec 27, 2019

Conversation

mikehardy
Copy link
Member

@mikehardy mikehardy commented Dec 12, 2019

Pull Request template

Purpose / Description

AnkiWeb will only support TLS1.2 beginning 1/1/2020, which is objectively the right thing to do (@dae gave us a heads up in #5623, thank you @dae)

We were also using an old old old style of HTTP communication.

This updates us to use OkHTTP v3 (works down to API15) and uses TLS1.2 where possible

Fixes

Fixes #5623

Approach

I examined #3626 a while back and determined depending on a 3rd party library was if anything, preferable to tracking platform. So OkHTTP was fine.

We already had a dependency even with the analytics work, so I'm comfortable with their versioning etc.

First I proved I could swap to their Apache HTTP API implementation with a test. It worked.

Then I took the internal implementation of that and used it as a template to use the (more correct / flexible) underlying OkHTTP Request/Response APIS. It worked.

Finally, I implemented the twin custom bits of "enable TLS1.2 on API<=21" per stackoverflow code, and "track progress while uploading" also via stackoverflow, so we had feature parity with the original implementation, and forward-ported to AnkiWeb's pending TLS level.

How Has This Been Tested?

I used an API17 and API29 emulator, and did lots and lots of force syncing up and down back and forth as well as regular syncing, with some Timber debugging enabled to trace execution and print status. Everything seems fine?

I also built APKs people can test, linked them below and put a call for testers out to the mailing list and on the original issue.

There is future opportunity to use the file copy Utils functions from Compat in here, but I wanted to stay focused.

We need to release this live + public before 1/1/2020, so this needs to hit beta ASAP and make it into release.

Checklist

Please, go through these checks before submitting the PR.

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code

@mikehardy mikehardy assigned mikehardy and unassigned mikehardy Dec 12, 2019
@mikehardy mikehardy force-pushed the okhttp-tls12 branch 3 times, most recently from 6448ba1 to bd70cd4 Compare December 12, 2019 22:51
@mikehardy mikehardy changed the title WIP replace apache http w/okhttp, enable TLS1.2 for Android API<=21 replace apache http w/okhttp, enable TLS1.2 for Android API<=21 Dec 12, 2019
@mikehardy mikehardy marked this pull request as ready for review December 12, 2019 22:52
@mikehardy
Copy link
Member Author

This is ready for review now @timrae - it's rough though, I did it as fast as possible to beat the Jan 1 deadline for TLS1.2. All seems to work locally on API17 and API29 emulator

@mikehardy
Copy link
Member Author

@timrae I resolved the unused imports, but left the "too long method" - refactoring that method is out of scope. Getting rid of apache HTTP while doing TLS1.2 was already a little scope creep and I want to leave it at this so we can get it out the door before 1/1/2020

@mikehardy mikehardy added Priority-High Review High Priority Request for high priority review labels Dec 13, 2019
@mikehardy
Copy link
Member Author

mikehardy commented Dec 14, 2019

I have produced 2 test builds for anyone interested to use alongside (not in place of) your regular builds. Using them should not affect your normal installation provided you go to "Advanced Preferences" and change the storage directory to "/sdcard/AnkiDroid.E"

Here is one for people on 2.9.1 built off latest 2.9beta: https://github.com/mikehardy/Anki-Android/releases/download/v2.9beta4/AnkiDroid-2.9.2beta4-TLS12.parallel.E.apk

Here is one for people on cutting-edge 2.10alpha's: https://github.com/mikehardy/Anki-Android/releases/download/v2.9beta4/AnkiDroid-2.10alpha17-TLS12.parallel.E.apk

Both were built by applying the single commit in this branch to the relevant branch and running ./tools/parallel-release.sh <branch-name>-TLS12, if that's interesting.

@mikehardy
Copy link
Member Author

re-read it today and there was one response body buffer remaining unclosed, I close it now. And I added some comments indicating when certain workarounds could be removed as OkHTTP API forward-porting happens during future minSdkVersion increases

@ankidroid ankidroid deleted a comment from mikehardy Dec 14, 2019
@dotancohen
Copy link

I can confirm that AnkiDroid-2.10alpha17-TLS12.parallel.E.apk syncs fine in both directions on an Android 4.4 (API 19) Barnes & Noble Nook Glow3 E-ink device. Thank you!

@mikehardy
Copy link
Member Author

@dotancohen I was curious how that test was going :-), thanks so much for reporting back.

Worst case you are personally covered then, with that install, past 1/1/2020 but we will try to get 2.9.2 out with this change prior to the new year so everyone is covered

@mikehardy
Copy link
Member Author

mikehardy commented Dec 19, 2019

A long time ago I had an automated test for this - I should re-add it 03387ef

Just checked - it is actually still there! And it's working, so even without the warning about the new requirement this would have actually begun to fail in CI, giving us a heads-up as well, just with an emergency instead of a plan. Either way it will verify things still work after 1/1/2020 assuming the PR is merged.

Worth noting that it fails on API15 though. TLS1.2 is simply not available on the platform so the ~300 users there will be locked out of syncing after 1/1/2020, and the most recent AndroidX updates have started requiring minSdkVersion 16 anyway, so we'll have to move soon.

@timrae
Copy link
Member

timrae commented Dec 19, 2019

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
- Added 1
           

Complexity increasing per file
==============================
- AnkiDroid/src/main/java/com/ichi2/libanki/sync/CountingFileRequestBody.java  2
- AnkiDroid/src/main/java/com/ichi2/libanki/sync/FullSyncer.java  1
- AnkiDroid/src/main/java/com/ichi2/libanki/sync/Tls12SocketFactory.java  7
- AnkiDroid/src/main/java/com/ichi2/libanki/sync/HttpSyncer.java  2
         

See the complete overview on Codacy

@ankidroid ankidroid deleted a comment from mikehardy Dec 19, 2019
return req(method, fobj, comp, null);
}


private org.apache.http.HttpResponse req(String method, InputStream fobj, int comp, JSONObject registerData) throws UnknownHttpResponseException {
private Response req(String method, InputStream fobj, int comp, JSONObject registerData) throws UnknownHttpResponseException {
Copy link
Member

Choose a reason for hiding this comment

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

@timrae timrae merged commit a91aead into ankidroid:master Dec 27, 2019
@timrae
Copy link
Member

timrae commented Dec 27, 2019

Thanks @mikehardy I only skimmed over the code, but let's test it!

@timrae
Copy link
Member

timrae commented Jan 30, 2020

Sorry I was hoping to get out a 2.10 release before this TLS deadline but I haven't had much time for AnkiDroid recently. I've just cherry-picked this onto the 2.9.2 hotfix branch and pushed out a new beta.

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

Successfully merging this pull request may close these issues.

AnkiWeb will require TLS1.2 moving forward
4 participants