Tentative self signed ssl support #141

Merged
AmineL merged 12 commits from davidoskky/ReaderForSelfoss-multiplatform:self_ssl into master 2023-09-17 18:28:48 +00:00
Contributor

Types of changes

Idea on implementing the support for self signed certificates. This has to be done separately on Android and IOS due to the different http clients. I'm unsure if this approach currently trusts any certificate for any domain and thus even certificates from domains different from the one we are accessing or just trusts any authority but does check that the domain is the correct one.
@AmineL, let me know if this is an appropriate approach and I shall introduce a switch at login time to choose between the normal behavior and this.

## Types of changes Idea on implementing the support for self signed certificates. This has to be done separately on Android and IOS due to the different http clients. I'm unsure if this approach currently trusts any certificate for any domain and thus even certificates from domains different from the one we are accessing or just trusts any authority but does check that the domain is the correct one. @AmineL, let me know if this is an appropriate approach and I shall introduce a switch at login time to choose between the normal behavior and this.
davidoskky force-pushed self_ssl from 3ac8adfa5c to 7a5e2f1155 2023-04-16 15:59:13 +00:00 Compare
Owner

@davidoskky we should definitely have a switch at login for this.

The switch should also be pre-checked for api 24 users with a text below explaining why this is needed, or let the user handle things server side.

@davidoskky we should definitely have a switch at login for this. The switch should also be pre-checked for api 24 users with a text below explaining why this is needed, or let the user handle things server side.
davidoskky force-pushed self_ssl from 7a5e2f1155 to 4e0117877d 2023-07-13 12:52:45 +00:00 Compare
Author
Contributor

Eventually got around completing this...
I tested it on API 21 and it works, however the SSL problem is also present in Glide and thus images are not currently displayed with this fix for people who had this problem before.

I'm sorry for mixing in the gradle upgrade in this PR.

Eventually got around completing this... I tested it on API 21 and it works, however the SSL problem is also present in Glide and thus images are not currently displayed with this fix for people who had this problem before. I'm sorry for mixing in the gradle upgrade in this PR.
davidoskky force-pushed self_ssl from 4e0117877d to b793301706 2023-07-13 12:56:10 +00:00 Compare
davidoskky changed title from WIP: Tentative self signed ssl support to Tentative self signed ssl support 2023-07-13 12:59:49 +00:00
Owner

@davidoskky there is a build issue. Could you check this, please ?

> Task :androidApp:minifyGithubConfigReleaseWithR8
ERROR: Missing classes detected while running R8. Please add the missing classes or apply additional keep rules that are generated in /drone/src/androidApp/build/outputs/mapping/githubConfigRelease/missing_rules.txt.
ERROR: R8: Missing class org.bouncycastle.jsse.BCSSLParameters (referenced from: void okhttp3.internal.platform.BouncyCastlePlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List) and 1 other context)
Missing class org.bouncycastle.jsse.BCSSLSocket (referenced from: void okhttp3.internal.platform.BouncyCastlePlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List) and 5 other contexts)
Missing class org.bouncycastle.jsse.provider.BouncyCastleJsseProvider (referenced from: void okhttp3.internal.platform.BouncyCastlePlatform.<init>())
Missing class org.conscrypt.Conscrypt$Version (referenced from: boolean okhttp3.internal.platform.ConscryptPlatform$Companion.atLeastVersion(int, int, int))
Missing class org.conscrypt.Conscrypt (referenced from: boolean okhttp3.internal.platform.ConscryptPlatform$Companion.atLeastVersion(int, int, int) and 4 other contexts)
Missing class org.conscrypt.ConscryptHostnameVerifier (referenced from: okhttp3.internal.platform.ConscryptPlatform$DisabledHostnameVerifier)
Missing class org.openjsse.javax.net.ssl.SSLParameters (referenced from: void okhttp3.internal.platform.OpenJSSEPlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List))
Missing class org.openjsse.javax.net.ssl.SSLSocket (referenced from: void okhttp3.internal.platform.OpenJSSEPlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List) and 1 other context)
Missing class org.openjsse.net.ssl.OpenJSSE (referenced from: void okhttp3.internal.platform.OpenJSSEPlatform.<init>())
Missing class org.slf4j.impl.StaticLoggerBinder (referenced from: void org.slf4j.LoggerFactory.bind() and 3 other contexts)

> Task :androidApp:minifyGithubConfigReleaseWithR8 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':androidApp:minifyGithubConfigReleaseWithR8'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.R8Task$R8Runnable
   > Compilation failed to complete
@davidoskky there is a build issue. Could you check this, please ? ``` > Task :androidApp:minifyGithubConfigReleaseWithR8 ERROR: Missing classes detected while running R8. Please add the missing classes or apply additional keep rules that are generated in /drone/src/androidApp/build/outputs/mapping/githubConfigRelease/missing_rules.txt. ERROR: R8: Missing class org.bouncycastle.jsse.BCSSLParameters (referenced from: void okhttp3.internal.platform.BouncyCastlePlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List) and 1 other context) Missing class org.bouncycastle.jsse.BCSSLSocket (referenced from: void okhttp3.internal.platform.BouncyCastlePlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List) and 5 other contexts) Missing class org.bouncycastle.jsse.provider.BouncyCastleJsseProvider (referenced from: void okhttp3.internal.platform.BouncyCastlePlatform.<init>()) Missing class org.conscrypt.Conscrypt$Version (referenced from: boolean okhttp3.internal.platform.ConscryptPlatform$Companion.atLeastVersion(int, int, int)) Missing class org.conscrypt.Conscrypt (referenced from: boolean okhttp3.internal.platform.ConscryptPlatform$Companion.atLeastVersion(int, int, int) and 4 other contexts) Missing class org.conscrypt.ConscryptHostnameVerifier (referenced from: okhttp3.internal.platform.ConscryptPlatform$DisabledHostnameVerifier) Missing class org.openjsse.javax.net.ssl.SSLParameters (referenced from: void okhttp3.internal.platform.OpenJSSEPlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List)) Missing class org.openjsse.javax.net.ssl.SSLSocket (referenced from: void okhttp3.internal.platform.OpenJSSEPlatform.configureTlsExtensions(javax.net.ssl.SSLSocket, java.lang.String, java.util.List) and 1 other context) Missing class org.openjsse.net.ssl.OpenJSSE (referenced from: void okhttp3.internal.platform.OpenJSSEPlatform.<init>()) Missing class org.slf4j.impl.StaticLoggerBinder (referenced from: void org.slf4j.LoggerFactory.bind() and 3 other contexts) > Task :androidApp:minifyGithubConfigReleaseWithR8 FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':androidApp:minifyGithubConfigReleaseWithR8'. > A failure occurred while executing com.android.build.gradle.internal.tasks.R8Task$R8Runnable > Compilation failed to complete ```
Author
Contributor

I cannot reproduce the build error, it compiles correctly on my system. Did you try rebuilding the project? It definitely is something related with the gradle upgrade.

I cannot reproduce the build error, it compiles correctly on my system. Did you try rebuilding the project? It definitely is something related with the gradle upgrade.
Owner

I relaunched the build on the CI and the error is still here.

You can see that here

I relaunched the build on the CI and the error is still here. [You can see that here](https://build.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/643)
davidoskky force-pushed self_ssl from f900d456a1 to 042089e78a 2023-07-15 11:12:17 +00:00 Compare
davidoskky force-pushed self_ssl from 042089e78a to 3646658cb1 2023-07-15 11:26:01 +00:00 Compare
davidoskky added 1 commit 2023-07-15 11:48:41 +00:00
Add translations
All checks were successful
continuous-integration/drone/pr Build is passing
5eb5a5658f
Author
Contributor

Fixed the build problem

Fixed the build problem
AmineL reviewed 2023-07-15 18:38:34 +00:00
@ -60,3 +59,2 @@
// Flag to enable support for the new language APIs
sourceCompatibility = JavaVersion.VERSION_11
targetCompatibility = JavaVersion.VERSION_11
sourceCompatibility = JavaVersion.VERSION_17
Owner

Is the java version upgrade necessary for something ?

Is the java version upgrade necessary for something ?
Author
Contributor

I will check, I did upgrade for a reason related to the gradle upgrade but didn't note down the exact motivation.

I will check, I did upgrade for a reason related to the gradle upgrade but didn't note down the exact motivation.
Author
Contributor

I have checked, it is not necessary. I can revert it if it is preferable.

I have checked, it is not necessary. I can revert it if it is preferable.
Owner

Yes, please.

Yes, please.
AmineL marked this conversation as resolved
@ -83,3 +83,3 @@
val sourceGroup = binding.sourcesGroup
repository.getSourcesDetailsOrStats().forEach { source ->
repository.getSourcesDetailsOrStats().forEachIndexed { _, source ->
Owner

Why use the forEachIndexed, but don't use the index ? (same for the one just after)

Why use the `forEachIndexed`, but don't use the index ? (same for the one just after)
Author
Contributor

This had to do with forEach not being supported on older Android versions.

This had to do with forEach not being supported on older Android versions.
AmineL marked this conversation as resolved
@ -54,0 +55,4 @@
android:id="@+id/selfSigned"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="@string/disableSSL"
Owner

disableSSL should be disable_ssl

`disableSSL` should be `disable_ssl`
AmineL marked this conversation as resolved
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
Owner

Please add back all the deleted xmlns:tools="http://schemas.android.com/tools".

This will cause issues with the translation tool.

Please add back all the deleted `xmlns:tools="http://schemas.android.com/tools"`. This will cause issues with the translation tool.
AmineL marked this conversation as resolved
@ -130,4 +130,5 @@
<string name="update_source">Update source</string>
<string name="confirm_disconnect_title">Disconnect ?</string>
<string name="confirm_disconnect_description">You will be disconnected from your selfoss instance.</string>
<string name="disableSSL">Disabilita SSL</string>
Owner

Translations should be done on the translation tool. This will be overwritten when merged.

Translations should be done on the translation tool. This will be overwritten when merged.
AmineL marked this conversation as resolved
@ -0,0 +52,4 @@
.build()
}
}
install(ContentNegotiation) {
Owner

From this point on, everything should work the same for ios. Is there a way to handle this better ?

From this point on, everything should work the same for ios. Is there a way to handle this better ?
Author
Contributor

I believe I can refactor it so that most things stay in common code.

I believe I can refactor it so that most things stay in common code.
AmineL marked this conversation as resolved
AmineL requested changes 2023-07-15 18:39:23 +00:00
AmineL left a comment
Owner

These change break ios client handling. Please keep in mind to not break ios handling with the changed you make.

These change break ios client handling. Please keep in mind to not break ios handling with the changed you make.
Author
Contributor

Indeed, I'll try to see if there is any way to use a shared http client among Android and iOS; as far as I understood okHTTP can only be used in Android; I can check if any of the other ones supported by ktor is multiplatform.

Indeed, I'll try to see if there is any way to use a shared http client among Android and iOS; as far as I understood okHTTP can only be used in Android; I can check if any of the other ones supported by ktor is multiplatform.
davidoskky added 2 commits 2023-08-10 20:08:36 +00:00
davidoskky added 2 commits 2023-08-19 09:57:24 +00:00
Remove unused indexed function call
Some checks failed
continuous-integration/drone/pr Build is failing
24beee070c
Author
Contributor

I have reverted to Java 11. I have substituted forEachIndexed with forEach; it builds and works on emulated android 21 but the CI build fails.

I have reverted to Java 11. I have substituted forEachIndexed with forEach; it builds and works on emulated android 21 but the CI build fails.
Owner

The CI build was already failing before. Your changes were not the cause of this.

The CI build was already failing [before](https://build.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/649/1/2). Your changes were not the cause of this.
davidoskky added 1 commit 2023-08-21 21:26:34 +00:00
Use forEachIndexed to support older apis
Some checks failed
continuous-integration/drone/pr Build is failing
a67a2a0927
davidoskky added 1 commit 2023-08-22 14:56:22 +00:00
Downgrade gradle version
Some checks failed
continuous-integration/drone/pr Build is failing
42e1c1ebbd
davidoskky added 1 commit 2023-09-07 09:55:39 +00:00
Android gradle plugin
All checks were successful
continuous-integration/drone/pr Build is passing
0aac2cba0b
Author
Contributor

I'm now using the correct android gradle plugin version; that was the reason of the previously failed builds.
The CI build failed but didn't provide any reason for the failure.

FAILURE: Build failed with an exception.

* What went wrong:
Gradle build daemon disappeared unexpectedly (it may have been killed or may have crashed)
I'm now using the correct android gradle plugin version; that was the reason of the previously failed builds. The CI build failed but didn't provide any reason for the failure. ``` FAILURE: Build failed with an exception. * What went wrong: Gradle build daemon disappeared unexpectedly (it may have been killed or may have crashed) ```
Owner

@davidoskky I restarted the build, it's now working.

I'll re review it pretty soon

@davidoskky I restarted the build, it's now working. I'll re review it pretty soon
AmineL requested changes 2023-09-07 19:03:31 +00:00
@ -142,3 +144,4 @@
repository.refreshLoginInformation(url, login, password)
CoroutineScope(Dispatchers.Main).launch {
repository.updateApiInformation()
Owner

repository.updateApiInformation() is called in goToMain which is called at line 152. This should not be needed.

`repository.updateApiInformation()` is called in `goToMain` which is called at line 152. This should not be needed.
Author
Contributor

Updating the api information is required for the following login call. This can probably be fixed by restructuring the code of the login page.

Updating the api information is required for the following login call. This can probably be fixed by restructuring the code of the login page.
AmineL marked this conversation as resolved
@ -54,0 +55,4 @@
android:id="@+id/selfSigned"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="@string/disable_ssl"
Owner

@string/disable_ssl should be named @string/use_self_signed_cert

`@string/disable_ssl` should be named `@string/use_self_signed_cert`
Author
Contributor

As I understand it, this would be misleading because we are not just accepting self signed certificates but rather disregarding ssl validation all together. The SSL certificate may very well be for another domain or missing altogether.

As I understand it, this would be misleading because we are not just accepting self signed certificates but rather disregarding ssl validation all together. The SSL certificate may very well be for another domain or missing altogether.
AmineL marked this conversation as resolved
@ -130,4 +130,5 @@
<string name="update_source">Update source</string>
<string name="confirm_disconnect_title">Disconnect ?</string>
<string name="confirm_disconnect_description">You will be disconnected from your selfoss instance.</string>
<string name="disable_ssl">Disable SSL</string>
Owner

<string name="disable_ssl">Disable SSL</string> => <string name="use_self_signed_cert">Use a self signed certificate</string> in all the files.

` <string name="disable_ssl">Disable SSL</string> ` => ` <string name="use_self_signed_cert">Use a self signed certificate</string> ` in all the files.
AmineL marked this conversation as resolved
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
Owner

Please revert this change.

Please revert this change.
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<resources xmlns:tools="http://schemas.android.com/tools">
Owner

No idea why I added this file. Can you please delete it ?

No idea why I added this file. Can you please delete it ?
@ -27,0 +37,4 @@
fun createHttpClient(
appSettingsService: AppSettingsService,
api: SelfossApi
Owner

Why is this needed ? Can't it be replaced by this ?

Why is this needed ? Can't it be replaced by `this` ?
Owner

Why was gradle and all the dependencies updated in this PR ?

Updating gradle forces to use java 17, so I'm unable to test this.

Can you revert all the version updates ? They should be done in a separate PR.

Why was gradle and all the dependencies updated in this PR ? Updating gradle forces to use java 17, so I'm unable to test this. Can you revert all the version updates ? They should be done in a separate PR.
davidoskky force-pushed self_ssl from 0aac2cba0b to 4482234e1a 2023-09-10 19:19:10 +00:00 Compare
davidoskky added 1 commit 2023-09-10 19:33:36 +00:00
Move api client creation function within api class
All checks were successful
continuous-integration/drone/pr Build is passing
a029d8a7dc
Author
Contributor

I was for some reason convinced that the gradle upgrade was necessary due to some dependency. It was not. Since I've written all the upgrade related stuff already I'll submit the PR soon.

I was for some reason convinced that the gradle upgrade was necessary due to some dependency. It was not. Since I've written all the upgrade related stuff already I'll submit the PR soon.
AmineL reviewed 2023-09-11 18:46:52 +00:00
@ -60,2 +64,3 @@
dependencies {
implementation("io.ktor:ktor-client-okhttp:2.1.1")
implementation("com.squareup.okhttp3:okhttp:4.10.0")
implementation("io.ktor:ktor-client-okhttp:2.2.4")
Owner

There are still version changes. Can you please revert them ?

There are still version changes. Can you please revert them ?
AmineL reviewed 2023-09-11 18:48:36 +00:00
@ -36,4 +51,3 @@
prettyPrint = true
isLenient = true
ignoreUnknownKeys = true
explicitNulls = false
Owner

Why was this removed ?

Why was this removed ?
AmineL reviewed 2023-09-11 18:50:15 +00:00
@ -91,2 +101,2 @@
private fun shouldHavePostLogin() = appSettingsService.getApiVersion() != -1
private fun hasLoginInfo() =
fun shouldHavePostLogin() = appSettingsService.getApiVersion() != -1
fun hasLoginInfo() =
Owner

These two should be reverted to private.

These two should be reverted to private.
davidoskky added 3 commits 2023-09-11 22:39:33 +00:00
AmineL requested changes 2023-09-12 19:08:39 +00:00
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
Owner

Please revert this.

Please revert this.
@ -29,13 +29,17 @@ kotlin {
sourceSets {
val commonMain by getting {
dependencies {
implementation("io.ktor:ktor-client-core:2.1.1")
Owner

Please revert this

Please revert this
Author
Contributor

Do you mean the whole version upgrade?

Do you mean the whole version upgrade?
Owner

Yes please

Yes please
davidoskky added 2 commits 2023-09-17 10:02:01 +00:00
Revert xmlns changes
All checks were successful
continuous-integration/drone/pr Build is passing
056825aa0c
AmineL merged commit c458871569 into master 2023-09-17 18:28:48 +00:00
davidoskky deleted branch self_ssl 2023-09-25 00:19:46 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Louvorg/ReaderForSelfoss-multiplatform#141
No description provided.