Skip to content

Conversation

@evermind-zz
Copy link
Contributor

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Could not load images with Picasso. This problem only surfaced as I maintain a fork of NewPipe that supports the video platform Rumble and the thumbnails stopped to load. The reason is that NewPipe moved from UniversalImageLoader to Picasso. This PR fixes that. It might fix other platforms on those devices too.

  • enable TLSv1.1/1.2 for OkHttpClient.Builder for Picasso on <=KitKat devices that support those protocols but have not been enabled.

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

I think this should be solved/done by #7175.

Btw: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA and TLS_RSA_WITH_AES_256_CBC_SHA are considered weak.

@evermind-zz
Copy link
Contributor Author

I think this should be solved/done by #7175.

Btw: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA and TLS_RSA_WITH_AES_256_CBC_SHA are considered weak.

thanks for pointing that out. I will try that the NetCipher route #7175

@evermind-zz
Copy link
Contributor Author

evermind-zz commented Nov 5, 2021

I've checked NetCipher and get it up and running -- but they do NOT enable TLSv1.1 and TLSv1.2. So NetCipher is no solution for me as the server sp.rmbl.ws only supports TLSv1.2. Maybe I miss something to enable it there. Any hints appreciated. Thank you!

Edit: the TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA and TLS_RSA_WITH_AES_256_CBC_SHA are the only protocols that android 4.x can use with TLSv1.2 in combination with that server.

@litetex
Copy link
Member

litetex commented Nov 5, 2021

Okay, I reviewed the PR again and I don't understand the problems at all:
As stated in the javadoc:
https://www.javadoc.io/doc/com.squareup.okhttp3/okhttp/3.3.1/okhttp3/OkHttpClient.Builder.html#sslSocketFactory-javax.net.ssl.SSLSocketFactory-javax.net.ssl.X509TrustManager-

Sets the socket factory and trust manager used to secure HTTPS connections. If unset, the system defaults will be used.

We already overwrite the system default on devices that run Android 4.4 (API 19).
So in theory there should be no problems as TLS 1.1. and 1.2 are already enabled for Picasso.

I think the problem here is that PicassoHelper gets initialized before the TLS is configured.
What happens if you move

// enable TLS1.1/1.2 for kitkat devices, to fix download and play for media.ccc.de sources
if (Build.VERSION.SDK_INT == Build.VERSION_CODES.KITKAT) {
TLSSocketFactoryCompat.setAsDefault();
}
to ?

Does this fix your problems?

@evermind-zz evermind-zz force-pushed the enable-tls-related-protocols-for-picasso-on-kitkat branch from 768014a to 5c7afbf Compare November 6, 2021 14:00
@evermind-zz
Copy link
Contributor Author

evermind-zz commented Nov 6, 2021

This was my first try and it did not work. Does OkHttp3 build on top of HttpsURLConnection? As the documentation states:

Sets the default SSLSocketFactory inherited by new instances of this class.

The socket factories are used when creating sockets for secure https URL connections.

I verified the OkHttpClient object is not using the TLSSocketFactoryCompat class in PicassoHelper but OpenSSLSocketFactoryImpl. Only the protocols ["TLSv1", "SSLv3"] are enabled

So I am not sure if this setting helps at all:

// enable TLS1.1/1.2 for kitkat devices, to fix download and play for media.ccc.de sources
if (Build.VERSION.SDK_INT == Build.VERSION_CODES.KITKAT) {
TLSSocketFactoryCompat.setAsDefault();
}

For me also media.ccc.de is not showing pictures on Android 4.4 emulator without this PR

In DownloaderImpl the TLSSocketFactoryCompat and additional Ciphers are explicitly set for its OkHttpClient.Builder().

I changed the PR now to not duplicate code and moved the DownloaderImpl enableModernTLS() method to a helper class and reuse that code to configure the PicassoHelper OkHttpClient.Builder() instance in the same way.

EDIT:
I was wrong about NetCipher. They enable TLSv1.1 and TLSv.1.2 but only If I set the compatible flag to false in NetCipher.getTlsOnlySocketFactory(false); I thought true would be more compatible. So I will test NetCiper again!

@evermind-zz
Copy link
Contributor Author

I created a new pull request using NetCipher which solves my problems

@evermind-zz evermind-zz closed this Nov 8, 2021
@evermind-zz evermind-zz deleted the enable-tls-related-protocols-for-picasso-on-kitkat branch November 8, 2021 06:46
@evermind-zz evermind-zz restored the enable-tls-related-protocols-for-picasso-on-kitkat branch November 12, 2021 07:32
evermind-zz added a commit to bravepipeproject/BravePipe that referenced this pull request Dec 25, 2021
- sync with TeamNewPipe v0.21.15
- fix for Picasso to enable TLSv1.1/1.2 on KitKat devices.
  (fixes Rumble and media.ccc.de displaying of thumbnails)
  This fix will not be available in NewPipe see TeamNewPipe#7350
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.

2 participants