search

konklone / congress-android

Congress for Android, an app for tracking Congress.
https://play.google.com/store/apps/details?id=com.sunlightlabs.android.congress
Other
158 stars 43 forks source link

Upgrade to OkHttp 3.12.2 #763

Open smpeters opened 5 years ago

smpeters commented 5 years ago

Along with this package upgrade, it also requires a minimum Android SDK Version of 21 to get updated TLS security.

My changes for #762 are included as well.

konklone commented 5 years ago

This generally looks great. However, the minimum API version is a significant change, which impacts device compatibility. Current stats suggest this will cut out about 10% of active Android devices across the ecosystem: https://developer.android.com/about/dashboards I could look at the stats specific to this app inside my own publisher dashboard, but if anything I'd expect it to be more severe.

Could that part be removed from this PR, and the rest accepted? What's the rationale to move to 21 now?

smpeters commented 5 years ago

That's the minimum API level for OkHttp. There's still some Heartbeat related SSL/TLS issues with lower Android API versions.

On Sun, Mar 17, 2019 at 5:03 PM Eric Mill notifications@github.com wrote:

This generally looks great. However, the minimum API version is a significant change, which impacts device compatibility. Current stats suggest this will cut out about 10% of active Android devices across the ecosystem: https://developer.android.com/about/dashboards I could look at the stats specific to this app inside my own publisher dashboard, but if anything I'd expect it to be more severe.

Could that part be removed from this PR, and the rest accepted? What's the rationale to move to 21 now?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/konklone/congress-android/pull/763#issuecomment-473719530, or mute the thread https://github.com/notifications/unsubscribe-auth/AACYUhtSdfPmnWiN7xvf0APKgFqSQ2YLks5vXruUgaJpZM4b4Z5B .

konklone commented 5 years ago

Help me understand the impact here - is it required in order to enable TLS v1.2, or is it about mitigating weaknesses with arbitrary servers? The app has a defined set of services it can connect to, so it doesn't have to worry about connecting to known-weak services.

smpeters commented 5 years ago

AWS, Microsoft, and Google cloud services run at TLS v1.2 by default. All major browsers will stop supporting anything before TLS v1.2 within the next year. If ProPublica or congressional servers providing the images (Picasso uses OkHttp) upgrade to servers using TLS v1.2 at a minimum, the app will stop functioning on devices without TLS v1.2. Depending on how the current versions of OkHttp in the application work, the app could stop working on newer devices as well.

On the downside, going to a Lollipop minimum will drop some supported devices on the new version of the app. That would exclude phones and device produced before November 2014 and some after. Although, with the SSL concerns around that time, most carriers and manufacturers upgraded even earlier devices.

On Mon, Mar 18, 2019 at 8:54 AM Eric Mill notifications@github.com wrote:

Help me understand the impact here - is it required in order to enable TLS v1.2, or is it about mitigating weaknesses with arbitrary servers? The app has a defined set of services it can connect to, so it doesn't have to worry about connecting to known-weak services.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/konklone/congress-android/pull/763#issuecomment-473919208, or mute the thread https://github.com/notifications/unsubscribe-auth/AACYUhON_PZDQuKhR4sVZv5JgLUD5fozks5vX5qigaJpZM4b4Z5B .

konklone commented 5 years ago

Reading the OkHttp blog post on this, it seems like what we want for now is the 3.12 stable branch for which they'll backport fixes for a while. I'm all about improving our TLS security, but I don't see any reason to drop support for these devices until servers do. The app will still be preferring TLS v1.2 over 1.0 by moving to a recent 3.x version.

Would you mind updating the PR to use 3.12, and to not bump the API level? The work you did in the PR to update the network classes and their use of the OkHTTP API is extremely helpful, and I'd love to merge that in to the app and move us to 3.x.

smpeters commented 5 years ago

OK, changes made to use in OkHttp 3.12.2 and minimum API lowered back to 16.

TacoTheDank commented 5 years ago

@smpeters @konklone Bump, I believe this can be merged now. @smpeters Great work :)

smpeters commented 5 years ago

Are there any concerns here that I can resolve?

TacoTheDank commented 5 years ago

I'm waiting for this to be merged so I can start a light refactoring of the code, I think everything is good right now

TacoTheDank commented 4 years ago

Anything on this? The merging repo doesn't seem to exist anymore? :/

smpeters commented 4 years ago

Yep, I deleted that repo. I'm going a different direction with my repo. Sitting for a year on a patch to merge is more than I really care to do.