Issue #603 - Add support for TLS client certificates

[Elv1zz]

This PR uses the new AdvancedX509KeyManager class which should be added by PR #1048 to the nextcloud/android-library to add support for servers that require a TLS client certificate to connect.

Since most logic is added to the android-library the changes to the app itself are rather small.

No tests were written, since I do not see how this can be tested automatically, as it would either require communication to a server requesting a client certificate -- which typically becomes very unreliable, and is also disregarded in the CONTRIBUTING guidelines -- or probably lots of mocked classes. If the second approach is required, I'd need some guidance on how to setup the test and what to mock.

Thanks for reviewing.

[tobiasKaminsky]

Thanks for this awesome PR! 👏
I am now trying to test this and set up NC with client side certs.
It is working within Firefox.
I now tried to install the p12 cert on Android, but it always states that the password is wrong.
I also tried to not set a password.
Do you have an idea?

I followed this tutorial: https://www.openlogic.com/blog/mutual-authentication-using-apache-and-web-client

[Elv1zz]

Thanks for this awesome PR! 👏

Gee, thanks 😊

I am now trying to test this and set up NC with client side certs. It is working within Firefox.

Okay, sounds good so far.

I now tried to install the p12 cert on Android, but it always states that the password is wrong. I also tried to not set a password. Do you have an idea?

Hm, I installed my key+cert quite a while ago... but I just checked: I also used a .p12 container for that.

I followed this tutorial: https://www.openlogic.com/blog/mutual-authentication-using-apache-and-web-client

Okay, I followed that guide (at least the part to export the keypair) and I get the same error as you when trying to import the key, although I chose a trivial password (to avoid typos... I know myself) and copied the file using Android Studio's device file manager (to avoid file corruption during transfer)...

But I found a solution: It seems, openssl uses a more modern encryption than Android supports so far. So you have to tell openssl to use the old encryption:

openssl pkcs12 -export -inkey KEY.pem -in CERT.pem -out Android.p12 -legacy

(The -legacy parameter at the end is the relevant part.) With that I was able to import my test key and certificate on my Android phone.

[tobiasKaminsky]

Awesome! Many thanks for pointing out how to do it!
It works!
We will soon review both PRs.

[kushmittal2009]

Hi @Elv1zz ,
I tested and revied these changes once more and found out that the branding feature does not work. I have the theming app installed in my nextcloud instance. The problem is, this app shoes the color I chose in the theming app, but it does not show the background and the logo of my nextcloud instance. It just shoes blue background with the text "Nextcloud" in the left side bar of the app. There is also another error of the app not working in background. When I swipe the app out of my recent open apps, the app stops auto uploading and if I take a photo at that time and open the app again, go to uploads, it would say "Upload error, Connection failed."

[Elv1zz]

Hi @Elv1zz , I tested and revied these changes once more and found out that the branding feature does not work. I have the theming app installed in my nextcloud instance. The problem is, this app shoes the color I chose in the theming app, but it does not show the background and the logo of my nextcloud instance. It just shoes blue background with the text "Nextcloud" in the left side bar of the app.

Hi @kushmittal2009, thanks for testing the PR again! Ok, I'll do not use the branding feature, so I have no experience with that and also did not test it. I hope, I can set it up. But I'll only have time for that not before next week.
But you could maybe check the logcat for some error messages related to that issue. Anything about connection errors, HTTP status 400, or something would be relevant. Because I assumed to have made all WebClients and connections aware of the TLS client certificate handling....
@tobiasKaminsky Maybe you have an idea, what I might have missed?

There is also another error of the app not working in background. When I swipe the app out of my recent open apps, the app stops auto uploading and if I take a photo at that time and open the app again, go to uploads, it would say "Upload error, Connection failed."

Erm, does this work as expected with a NextCloud server that does not require TLS client certificates? Because, if you kill the app, I would expect it to stop background tasks as well...
But same here: If you see something relevant in the logcat, please post it here, so I can have a look at it next week.

Thanks :)

[phishsticks]

Is there anything we can do to progress this? I have seen a lot of people in the community desperate for this feature :)

[Elv1zz]

Is there anything we can do to progress this? I have seen a lot of people in the community desperate for this feature :)

Depends :) There are two problems with the current implementation:

1. As reported by kushmittal2009, the branding feature does not work when the app uses TLS client certificates. I could reproduce the issue, but did not find time for further investigation so far. I assume that there is (at least) one more place where network connections are established, which also has to learn about client certificate handling.
2. The auto-upload does not work as reliable as without client certificates, when the app is in the background. This was also reported by kushmittal2009 and I observed it during my tests as well. This is probably also caused by the same or a similar issue as no. 1.

Regarding the failing tests, I started a while ago to address them, but could not finish (again, due to lack of time). I just pushed my latest improvements and a merge with upstream to stay in sync.

So, if you have any insights in the mentioned problems, that would be helpful and could create some progress. I hope to find time in the next weeks to look into this again -- I also really would like to have this feature in the official nextcloud client.

[nextcloud-android-bot]

master-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/8921-IT-master-22-59

[Elv1zz]

Dear reviewers, the current state of both PRs (this and PR 1048 towards the android-library) are ready for review. The upload problem mentioned above could be solved. The branding problem is not as big as it sounds: The branding information is correctly retrieved and applied. Only the header showing the custom logo and server name cannot be updated, since the used Glide instance uses its own HTTP(S) connection to retrieve the logo. Since that does know neither about the new AdvancedX509KeyManager nor about the AdvancedX509TrustManager it cannot handle the required TLS client certificates (and also should not be able to handle e.g. self-signed certificates). IMHO this issue is out of scope for this PR. Should I create a separate bug ticket for this?
However, I think the TLS client certificate handling will be very useful even without the branded server logo.

[avocadio]

@tobiasKaminsky - I was running @stephanritscher mTLS version - and this version is a significant improvement - the documents are syncing straight away.

I would recommend the QR scanning as it removes any hurdles during the configuration. I've noticed that @AlvaroBrey is no longer working full time on the project but is still assigned as reviewer.

[ippocratis]

@Elv1zz could you please add apk in actions artifacts or release in https://github.com/Elv1zz/nextcloud-android/actions?
Would love to try your additions with my nc instance until (if) this merged.
Thanks

[basharkey]

@Elv1zz could you please add apk in actions artifacts or release in https://github.com/Elv1zz/nextcloud-android/actions? Would love to try your additions with my nc instance until (if) this merged. Thanks

@ippocratis I've forked their repo and reconfigured the QA workflow so you can pull the apk from here:
https://github.com/basharkey/nextcloud-android/releases/download/untagged-a1c8d77666d536a61f32/qa-debug-1.apk

Related PR for transparency:
basharkey#1

[ippocratis]

@Elv1zz could you please add apk in actions artifacts or release in https://github.com/Elv1zz/nextcloud-android/actions? Would love to try your additions with my nc instance until (if) this merged. Thanks

@ippocratis I've forked their repo and reconfigured the QA workflow so you can pull the apk from here:
https://github.com/basharkey/nextcloud-android/releases/download/untagged-a1c8d77666d536a61f32/qa-debug-1.apk

Related PR for transparency:
basharkey#1

Thanks so much.
Keep up.

[Elv1zz]

@ippocratis I've forked their repo and reconfigured the QA workflow so you can pull the apk from here: https://github.com/basharkey/nextcloud-android/releases/download/untagged-a1c8d77666d536a61f32/qa-debug-1.apk

Related PR for transparency: basharkey#1

Thanks for fixing the workflow! I never managed to fix it -- and actually still do not understand why it broke 🤔
However, I think, I should include your fix. So, could you target your PR towards my repo?

[basharkey]

@Elv1zz I've created a PR against your repo. The QA workflow is what builds the APK and creates the comment containing the download URL. Unfortunately this only works for this repo and not forks because that workflow actually uses secrets we don't have access to, in order to upload the APK to @tobiasKaminsky Nextcloud server. My changes just create a Github artifact instead so that you can download it directly from Github!

[tobiasKaminsky]

Merged via #12408
Thanks for this awesome contribution!