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 tls configuration with NetCipher for all Android versions #7372

Conversation

evermind-zz
Copy link
Contributor

@evermind-zz evermind-zz commented Nov 8, 2021

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

  • use NetCipher for all android versions for TLS configuration
  • enable TLS configuration for picasso's OkHttpClient
  • remove obsolete TLS releatd code

PR is tested on Android 4.4 and Android 7.1.2
EDIT: The PR works also on Android 9 but it fails on Android 10. See #issuecomment-966884189

This Pull request will render the following pull requests obsolete:

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

@XiangRongLin XiangRongLin added bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality labels Nov 9, 2021
@evermind-zz evermind-zz force-pushed the replace-tls-configuration-with-netCipher branch from 4224165 to e43c760 Compare November 10, 2021 06:11
@evermind-zz
Copy link
Contributor Author

evermind-zz commented Nov 12, 2021

The PR works also on Android 9 but it fails on Android 10.

I figured out that OkHttp 3.12.13 is calling the method getApplicationProtocol that has been introduced in Android 10 javax/net/ssl/SSLSocket#getApplicationProtocol() but as NetCipher directly inherit from SSLSocket in DelegateSSLSocket class and does not overwrite getApplicationProtocol() SSLSocket.getApplicationProtocol() is called which throws UnsupportedOperationException.

So I came up with adding my own getApplicationProtocol() into NetCiphers DelegateSSLSocket that will use reflection to delegate the call. TlsOnlySocketFactory.DelegateSSLSocket.getApplicationProtocol():

public String getApplicationProtocol() {
    String result = null; 
    try {
        result = (String) delegate
                .getClass()
                .getMethod("getApplicationProtocol")
                .invoke(delegate);
    } catch (Exception e) {
        e.printStackTrace();
    }

    return result; 
}

In the end to get it working we would have to either upgrade OkHttp or convince NetCipher to fix something in their code.
Updating OkHttp will result in losing Android 4.x support. I guess in a month or two NewPipe will nevertheless drop Android 4.x because of no longer security updates for OkHttp 3.12.*

In short NetCipher will not be a solution for now! I guess I have to reopen #7350
Added NetCipher merge request

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.

Code looks - on the first impression - good.

However this has to be tested with some devices to ensure nothing gets broken.

Some minor changes:

@litetex
Copy link
Member

litetex commented Nov 12, 2021

Did some quick testing and the YT kiosk immediately throws an error on my emulator:

Exception

  • User Action: requested kiosk
  • Request: Start loading: https://www.youtube.com/feed/trending
  • Content Country: US
  • Content Language: en-US
  • App Language: en_US
  • Service: YouTube
  • Version: 0.21.13
  • OS: Linux Android 11 - 30
Crash log

java.lang.UnsupportedOperationException
	at javax.net.ssl.SSLSocket.getApplicationProtocol(SSLSocket.java:1467)
	at okhttp3.internal.platform.Android10Platform.getSelectedProtocol(Android10Platform.java:63)
	at okhttp3.internal.connection.RealConnection.connectTls(RealConnection.java:347)
	at okhttp3.internal.connection.RealConnection.establishProtocol(RealConnection.java:284)
	at okhttp3.internal.connection.RealConnection.connect(RealConnection.java:169)
	at okhttp3.internal.connection.StreamAllocation.findConnection(StreamAllocation.java:258)
	at okhttp3.internal.connection.StreamAllocation.findHealthyConnection(StreamAllocation.java:135)
	at okhttp3.internal.connection.StreamAllocation.newStream(StreamAllocation.java:114)
	at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.java:42)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.java:127)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:257)
	at okhttp3.RealCall.execute(RealCall.java:93)
	at org.schabi.newpipe.DownloaderImpl.execute(DownloaderImpl.java:166)
	at org.schabi.newpipe.extractor.downloader.Downloader.post(Downloader.java:131)
	at org.schabi.newpipe.extractor.downloader.Downloader.post(Downloader.java:114)
	at org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.areHardcodedClientVersionAndKeyValid(YoutubeParsingHelper.java:347)
	at org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.getClientVersion(YoutubeParsingHelper.java:434)
	at org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.prepareDesktopJsonBuilder(YoutubeParsingHelper.java:819)
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeTrendingExtractor.onFetchPage(YoutubeTrendingExtractor.java:60)
	at org.schabi.newpipe.extractor.Extractor.fetchPage(Extractor.java:54)
	at org.schabi.newpipe.extractor.kiosk.KioskInfo.getInfo(KioskInfo.java:57)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$getKioskInfo$11(ExtractorHelper.java:184)
	at org.schabi.newpipe.util.ExtractorHelper$$ExternalSyntheticLambda10.call(Unknown Source:4)
	at io.reactivex.rxjava3.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:43)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.single.SingleDoOnSuccess.subscribeActual(SingleDoOnSuccess.java:35)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.rxjava3.core.Scheduler$DisposeTask.run(Scheduler.java:614)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:65)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:56)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.lang.Thread.run(Thread.java:923)


@evermind-zz
Copy link
Contributor Author

Jup! This is the problem with NetCipher not yet implementing those methods. See #issuecomment-966884189

@Redirion
Copy link
Member

but looks like the owner of the repository is willing to merge a PR containing the missing methods added in https://developer.android.com/reference/javax/net/ssl/SSLSocket

the missing methods are:
SDK 29:
getApplicationProtocol
getHandshakeApplicationProtocol
getHandshakeApplicationProtocolSelector
setHandshakeApplicationProtocolSelector
SDK24:
getHandshakeSession
SDK9:
getSSLParameters
setSSLParameters

@evermind-zz evermind-zz marked this pull request as draft November 14, 2021 14:24
@evermind-zz evermind-zz force-pushed the replace-tls-configuration-with-netCipher branch 2 times, most recently from 5ccfd2b to 2f09ee2 Compare November 14, 2021 21:49
Let NetCipher sort out the TlsVersions and CipherSuites that it considers save.
@evermind-zz evermind-zz force-pushed the replace-tls-configuration-with-netCipher branch from 2f09ee2 to 6dcfe72 Compare November 14, 2021 21:54
@evermind-zz
Copy link
Contributor Author

evermind-zz commented Nov 14, 2021

Hope everything is fixed now! Now we just have to wait until a new NetCipher version is released.

If someone wants to test with my NetCipher changes applied just replace below line in app/build.gradle

-    implementation "info.guardianproject.netcipher:netcipher:${netcipherVersion}"
+    implementation 'com.gitlab.evermind-zz.NetCipher:netcipher:c6fa69b'

@litetex
Copy link
Member

litetex commented Nov 15, 2021

Jup! This is the problem with NetCipher not yet implementing those methods.

Maybe we should also add an option to enable/disable this in the preferences.
In case Android 13+ breaks NetCipher and we don't want to wait for an update.

@litetex
Copy link
Member

litetex commented Mar 15, 2022

Closing this for now:

Can be reopened / recreated when the situation changes.

@litetex litetex closed this Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants