Issue #1169: Support for TLS client certificates #1216
Conversation
This adds classes from nextcloud's `android-library` project which are used to manage TLS client certificates and handle corresponding requests.
Because of the different communication paths of the *memories* app (using "local" API calls resulting in network requests), the selection of the TLS client certificate has to be done in a different way than in the *nextcloud* app. *If* the login request would be the first request sent to the server using the webview, things might work out differently, but since the API describe endpoint is queried first, certificate selection has not been done so far, resulting in a connection error. By using an additional step opening the user entered URL in the webview, we can trigger the certificate selection and afterwards use the selected certificate for the real authentication.
For the video player to be able to connect to the server using the selected client certificate, we have to use a different `HttpDataSource` that takes into account the configuration of the `OkHttpClient`. Therefore instead of the `DefaultHttpDataSource`, we now use `OkHttpDataSource` which can be given the configured `OkHttpClient`.
New string resources were first added as empty placeholders, now the corresponding values were added to guide the user in selecting a client certificate.
Only removing keys for specific URL given to `requestClientCertFor` method instead of all stored keys. (Even though, this should not make a difference for the given application, since there should always be only one server connection configured.) Added comment *why* we navigate the webview to the given URL.
Removed commented code that did not work. Extended explanatory comment.
android/app/src/main/java/gallery/memories/service/HttpService.kt
Outdated
Show resolved
Hide resolved
|
First off: Thank you very much for your work @Elv1zz :) Here my test results for your PR: Works perfectly:
Works not:
Open Questions:
|
By mistake the `client` field in `HttpService` holding an `OkHttpClient` was made a `val`, while it should be updated later in the code. So it became a `var` with `private set` instead. Thanks Waler for pointing this out! Co-authored-by: Waler <Waler89@googlemail.com>
😊
Yes, sadly it is needed (as far as I could figure out): The problem is that before the You can verify that the issue happens for you too, when you delete all app data again, start the app, enter the URL of your server, and then just try to login. This should fail with an SSL or HTTP error -- if your server requires client certificates as my setup does. If you have optional client certificates (e.g. used for authentication) then it would work without the button, I guess. An option I thought about could be to only show the button after such an error occurred, so the user can then select the required certificate.
Yes, that's the way it should work currently 👍
Hm, I don't know, but I don't think so. The external browser app manages the client certificates on its own (while it of course also should get them from the system's certificate store), but we cannot "instruct" it which certificate to use (which probably is a good idea in terms of security). |
|
Looks good to me overall, thanks! If you think it's ready to merge based on your testing just mark the PR as ready, then I'll go ahead with my local testing. Since the download manager doesn't work, sharing won't work either. This is a major pain; there seems to be no way around this sadly, it just needs a rewrite to not use the system dl manager at all (this might end up being cleaner though). |
|
Sorry for disappearing for quite some time, but I was busy with other things. During this time I was using my fork of the memories app. Since everything worked as expected and Github does not show any merge conflcts, I think it should be ready for review now. 🙂 |
|
When will this change be merged? :) |
|
@pulsejet please take a look above |
This adds support for TLS client certificates to the Memories Android app
At the moment there are three things to discuss/decide with this PR:
AdvancedX509KeyManagerandSelectClientCertificateHelperActivityclass from nextcloud android-library, so this app has more control over the code (this was a major concern in the nextcloud project). However, any progress that is made in the android-library, will not automatically be reflected here. So by adding nextcloud's android-library as dependency, at least theAdvancedX509KeyManagerandSelectClientCertificateHelperActivitycould be removed again and directly used from that library. If that library should be used in the future for more, that probably should be the way to go.AdvancedX509KeyManagercode: The client certificate request sent by the server is handled by theWebView(or theonReceivedClientCertRequestmethod of theWebViewClientto be more precise). I could not find any other way to handle this request. But maybe this is okay, since it only has to be done once on connection setup?X509KeyManagerto theDownloadManageror itsRequest. Maybe theandroix.media3.exoplayer.DownloadManagercould be used instead, as its constructor can be given anDataSource.Factoryas is already done for theExoPlayer(so, yes, video playback already does work 😄). But since that would be a rather big chance to the app and I am not an expert for bothDownloadManagers, this has not been done so far.So after all the negative points, I can confirm that the main features of the app do work as expected and I can view my images and videos, browse, create and modify albums, see detected people, etc., and I am happily using the app for about a week now without any issues. However, since I have not used the Android app without my modifications, I do not know if there are any performance issues or any other problems (apart from the already mentioned above).
Looking forward to your feedback.