Issue #276 - Support for TLS client certificates #1048

Elv1zz · 2023-01-27T19:53:29Z

This PR adds the new AdvancedX509KeyManager, which is used to select, manage, and apply a TLS client certificate to establish a secure connection to a server that requires a client certificate to connect. It includes an interactive component to allow the user to select the client certificate from the device's store.

The implementation is based on InteractiveKeyManager by Stephan Ritscher and was updated, modified, stripped-down, and now integrated into the nextcloud/android-library, as it was requested in previous PRs #1005 and #11099 to not add dependency on a third-party library for security related code.

One of the other comments in the previous PR was that there is already an implementation to support TLS client certificates in the nextcloud talk app. However, to my understanding, that approach seems to be too different to the nextcloud android app, since e.g. the talk app does not use the nextcloud android-library in the first place and has all the network related code in the app itself, so it has much stronger coupling between the components, which -- I think -- cannot be achieved in the separated architecture of the android-library and android app.

Since most logic and (minimalistic) UI is added to the library, integration into a client app requires only a few lines of code (corresponding PR to the nextcloud/android app will follow soon).

Thanks for reviewing :)

library/src/main/java/com/owncloud/android/lib/common/network/NetworkUtils.java

            SSLContext sslContext = getSSLContext();
-            sslContext.init(null, tms, null);
+            sslContext.init(kms, tms, null);


library/src/main/java/com/nextcloud/common/NextcloudClient.kt


            val sslContext = NetworkUtils.getSSLContext()

-            sslContext.init(null, arrayOf<TrustManager>(trustManager), null)
+            sslContext.init(arrayOf(keyManager), arrayOf<TrustManager>(trustManager), null)


library/src/main/java/com/nextcloud/common/PlainClient.kt


            val sslContext = NetworkUtils.getSSLContext()

-            sslContext.init(null, arrayOf<TrustManager>(trustManager), null)
+            sslContext.init(arrayOf(keyManager), arrayOf<TrustManager>(trustManager), null)


kushmittal2009

Hi, I tested your commits but a found an error. The error is that if you use apps in nextcloud like OpenOffice, it will fail to connect with http status error -1. Client cert not selected. The client certificate selector only works for the app it self and not the nextcloud apps inside the instantance of the nextcloud website in the app.

Elv1zz · 2023-01-30T19:38:17Z

Hi @kushmittal2009, thanks a lot for testing!

I am not sure, I understand your testcase correctly. You say:

The error is that if you use apps in nextcloud like OpenOffice, it will fail to connect with http status error -1. Client cert not selected. The client certificate selector only works for the app it self and not the nextcloud apps inside the instantance of the nextcloud website in the app.

By "app" do you refer to an app on your Android device or an app in your nextcloud instance? My changes here will only apply, if the client app also makes use of the new AdvancedX509KeyManager when requesting the auth token for the first time (without TLS client certificate support, there is no other way to establish a connection to the server, so that seems to be the only scenario where the client certificate selection can and should take place).

In the meantime I prepared a PR to the nextcloud android app as well (PR #11314), which shows what a client app using the android-library has to do to add TLS client support.

If that was what you already tested, please let me know how I can reproduce the error, so I can fix it.

I am using the nextcloud app with my modifications to the android-library for about two weeks now and so far it works without any trouble. But maybe I missed a usecase.

kushmittal2009 · 2023-01-30T23:53:19Z

Hi @Elv1zz , What I meant was, I tested your commits with the advancedX509keyManager and the compiled nextcloud app does seem to ask for certificate, sign in, and get the home screen of the app. However if you of apps like openoffice installed in your nextcloud server and you click a .doc file, in the logcat is says Client certificate not selected and the nextcloud app gets stuck showing a reload icon on the middle of the screen. The main issue is that if a WebView opens inside of a WebView, the other WebView can not use the saved CLient Certificate with the previous WebView.

Elv1zz · 2023-01-31T21:48:03Z

Hi @kushmittal2009, thanks for your detailed explanation! In fact you found an error in my implementation 👍 I implemented the TLS client certificate handling in a sub-class of NextcloudWebViewClient in AuthenticatorActivity, when it should have been implemented in NextcloudWebViewClient itself -- it worked fine for initial authentication, file up- and download, but not for other webviews, as you discovered.
It's now fixed (in the app) and now also works for your use case.

tobiasKaminsky · 2023-02-10T06:27:01Z

Tests are failing.
I am especially worried about:

java.lang.ClassCastException: AdvancedX509KeyManager context must be either Activity, Application, or Service!
at com.owncloud.android.lib.common.network.AdvancedX509KeyManager.init(AdvancedX509KeyManager.java:136)
at com.owncloud.android.lib.common.network.AdvancedX509KeyManager.<init>(AdvancedX509KeyManager.java:120)
at com.owncloud.android.lib.common.network.NetworkUtils.getAdvancedSslSocketFactory(NetworkUtils.java:121)
at com.owncloud.android.lib.common.network.NetworkUtils.registerAdvancedSslContext(NetworkUtils.java:105)
at com.owncloud.android.lib.common.OwnCloudClientFactory.createOwnCloudClient(OwnCloudClientFactory.java:145)
at com.owncloud.android.AbstractIT.beforeAll(AbstractIT.java:108)

Elv1zz · 2023-03-02T21:09:07Z

For some reason I missed the notification about your comment. I will have time to look into this again probably next week.

Tests are failing. I am especially worried about:
java.lang.ClassCastException: AdvancedX509KeyManager context must be either Activity, Application, or Service!

The AdvancedX509KeyManager needs an Activity, Application, or Service to be able to start its own Activity to select the client certificate. But if no such Context is passed, it should show a notification, which then can start that Activity... so the mentioned exception should not be thrown, but might be logged.
As I said, I'll have a look at it next week.

codecov · 2023-03-09T20:40:57Z

Codecov Report

Merging #1048 (355d84f) into master (605c5df) will decrease coverage by 2.10%.
The diff coverage is 20.43%.

❗ Current head 355d84f differs from pull request most recent head 40cb798. Consider uploading reports for the commit 40cb798 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1048      +/-   ##
============================================
- Coverage     52.56%   50.47%   -2.10%     
- Complexity      978      984       +6     
============================================
  Files           192      191       -1     
  Lines          7098     7327     +229     
  Branches        934      958      +24     
============================================
- Hits           3731     3698      -33     
- Misses         2851     3114     +263     
+ Partials        516      515       -1

Files	Coverage Δ
.../src/main/java/com/nextcloud/common/PlainClient.kt	`72.72% <100.00%> (+1.29%)`	⬆️
...loud/android/lib/common/OwnCloudClientFactory.java	`11.11% <100.00%> (ø)`
...cloud/android/lib/common/network/NetworkUtils.java	`66.17% <100.00%> (+0.50%)`	⬆️
...d/lib/common/network/AdvancedX509TrustManager.java	`52.38% <50.00%> (ø)`
...om/owncloud/android/lib/common/OwnCloudClient.java	`44.32% <63.63%> (+0.89%)`	⬆️
.../main/java/com/nextcloud/common/NextcloudClient.kt	`48.23% <63.63%> (-0.38%)`	⬇️
...network/SelectClientCertificateHelperActivity.java	`0.00% <0.00%> (ø)`
...oid/lib/common/network/AdvancedX509KeyManager.java	`16.81% <16.81%> (ø)`

... and 35 files with indirect coverage changes

functionpointer · 2023-03-28T13:10:41Z

These errors are just in the tests, right? So it can this already be used?

Elv1zz · 2023-03-31T15:52:35Z

These errors are just in the tests, right? So it can this already be used?

I am using my fork for quite a while now. So you should be able to use it, yes. And indeed it would be good to have more people using it so find any further potential bugs :)

So far there is one known issue left with the branding feature of Nextcloud, for which I hope to find time to look into soon.
If you find any problems related to TLS client certificates, just let me know.

This adds the new `AdvancedX509KeyManager`, which is used to select, manage, and apply a TLS client certificate to establish a secure connection to a server. It includes an interactive component to allow the user to select the client certificate from the device's store. The implementation is based on [`InteractiveKeyManager` by Stephan Ritscher](https://github.com/stephanritscher/InteractiveKeyManager) and was updated, modified, and integrated into the `nextcloud-android-library` by [Elv1zz](https://github.com/Elv1zz). Signed-off-by: Elv1zz <[email protected]>

During code review it was found that HTTP status 400 was only handled in `WebviewClient` based connections, but most connections are using `NextcloudClient`. So if a connection error occurs, most likely because of an expired certificate, it would not be handled correctly, and we would go on trying to use the expired certificate. So now a asic error handling was added. However, so far I did not find a way to handle this kind of error in `OwnCloudClient` due to missing `Context`. Signed-off-by: Elv1zz <[email protected]>

In case server returns an HTTP status code `400` (bad request), we remove the selected TLS client certificate for the corresponding host and port, since it might have expired or be rejected for another reason. For this, the `OwnCloudClient` constructor got an additional `Context` parameter and the class an additional field, since the `Context` is required to construct a new `AdvancedX509KeyManager`. Signed-off-by: Elv1zz <[email protected]>

Instead of using the magic number `400` for the HTTP status code which causes us to drop the selected TLS client certificate, the corresponding named constant from `HttpStatus` is used. Signed-off-by: Elv1zz <[email protected]>

Some replacements of `InteractiveKeyManager` to `AdvancedX509KeyManager` were missed. This was now fixed. Signed-off-by: Elv1zz <[email protected]>

While testing the TLS client certificate implementation I found that most `port`/`getPort` implementations returned `-1` when there was no port defined explicitly in the URL (e.g. `https://localhost/` instead of `https://localhost:443/`). This would lead to keys not being correctly removed. To fix this and making the usage of `removeKeys` more robust and simple, the `removeKeys(host, port)` method was made private, and several overloads were added, which accept all ways of representing an URL found in Nextcloud (and one extra way, which is not used (so far) accepting an `java.net.URI`). They all boil down to a `java.net.URL` which also returns `-1` for an implicit port, but provides the `getDefaultPort` for that case, allowing to get the protocols default port. Signed-off-by: Elv1zz <[email protected]>

To send notifications, we need the corresponding permission, so we have to add it to the `AndroidManifest.xml`. Signed-off-by: Elv1zz <[email protected]>

Before sending a notification, check if the required permission is granted. If it's not granted, we cannot request it, since we do not have an Actvity as that's the only time we try to send notifications. Signed-off-by: Elv1zz <[email protected]>

Forgot to update `NextcloudClient` creation to extended constructor signature, which also requires a `Context` instance. Signed-off-by: Elv1zz <[email protected]>

We try to get the application context from the `Context` instance passed to `AdvancedX509KeyManager`. This mainly fixed the `AbstractIT` instrumented test, but should make things more robust for future use. Signed-off-by: Elv1zz <[email protected]>

Added documentation to implementation of `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <[email protected]>

Use one line for each declaration, it enhances code readability. Signed-off-by: Elv1zz <[email protected]>

Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes. Signed-off-by: Elv1zz <[email protected]>

The method 'matches(AKMAlias)' has an NPath complexity of 540, current threshold is 200. This is an attempt to reduce the method's complexity. Signed-off-by: Elv1zz <[email protected]>

Added a brief documentation to empty method bodies which implement the `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <[email protected]>

Fixing broken format string where `%` and `$` were mixed up. Signed-off-by: Elv1zz <[email protected]>

The `AdvancedX509ExtendedKeyManager` now extends the `X509ExtendedKeyManager`, since the documentation recommends to not directly implement the `X509KeyManager` interface, but always extend the `X509ExtendedKeyManager`. Signed-off-by: Elv1zz <[email protected]>

There is no need to keep track of the top most `Activity` anymore, since we can just use any `Context` to start a new `Activity`. So we do not have to implement the `ActivityLifecycleCallbacks` and can drop the expectation of specific `Context` implementations (`Service`, `Activity` or `Application`). Signed-off-by: Elv1zz <[email protected]>

Lint issued some warnings, which were resolved now: - replaced `switch` statement with extended `switch` - explicitly typecasted variable could be replaced with pattern variable - added `<p>` to empty lines in Javadoc comment Signed-off-by: Elv1zz <[email protected]>

By using the `class.getName()` instead of `class.getCanonicalName()` the `TAG` should not become `null`. Signed-off-by: Elv1zz <[email protected]>

Added an unit test for the `AKMAlias.matches` method before refactoring it. Signed-off-by: Elv1zz <[email protected]>

This should reduce NPath complexity of `AKMAlias.matches` method without changing its behavior. This was verified using the previously added unit test that should cover all relevant cases. Signed-off-by: Elv1zz <[email protected]>

Codacy did not like my test in several points: - The function name may not have an underscore (`_`). - Magic number (`123`) should be replaced with a named constant. - File should end with a new line. Signed-off-by: Elv1zz <[email protected]>

Elv1zz · 2024-01-13T19:44:02Z

Tests are failing. I am especially worried about:

java.lang.ClassCastException: AdvancedX509KeyManager context must be either Activity, Application, or Service!
at com.owncloud.android.lib.common.network.AdvancedX509KeyManager.init(AdvancedX509KeyManager.java:136)
at com.owncloud.android.lib.common.network.AdvancedX509KeyManager.<init>(AdvancedX509KeyManager.java:120)
at com.owncloud.android.lib.common.network.NetworkUtils.getAdvancedSslSocketFactory(NetworkUtils.java:121)
at com.owncloud.android.lib.common.network.NetworkUtils.registerAdvancedSslContext(NetworkUtils.java:105)
at com.owncloud.android.lib.common.OwnCloudClientFactory.createOwnCloudClient(OwnCloudClientFactory.java:145)
at com.owncloud.android.AbstractIT.beforeAll(AbstractIT.java:108)

This part of the code was completely removed in the meantime, the AdvancedX509KeyManager now can use any Context, not only Activity, Application or Service. The code was simplified by this removal quite a bit.

So, is there anything I can do to get this PR forward?

functionpointer · 2024-01-17T20:33:02Z

Awesome work @Elv1zz!

I think tagging @tobiasKaminsky is appropriate as older PRs probably aren't checked for progress

tobiasKaminsky · 2024-01-23T07:48:24Z

@Elv1zz to get CI running, I would like to create this branch in our repo.
I hope this is okay for you.
I will also rebase it.

tobiasKaminsky · 2024-01-23T08:13:04Z

Superseeded by #1308

Elv1zz · 2024-01-24T17:30:41Z

@Elv1zz to get CI running, I would like to create this branch in our repo. I hope this is okay for you. I will also rebase it.

Yes, that should help get things up and running and is perfectly fine for me.
Thanks and see you in PR #1308

Elv1zz mentioned this pull request Jan 27, 2023

Issue #603 - Add support for TLS client certificates nextcloud/android#11314

Closed

szaimen added the 3. to review label Jan 27, 2023

github-advanced-security bot found potential problems Jan 27, 2023

View reviewed changes

kushmittal2009 reviewed Jan 30, 2023

View reviewed changes

kushmittal2009 previously approved these changes Feb 2, 2023

View reviewed changes

Elv1zz dismissed kushmittal2009’s stale review via ea39949 February 3, 2023 21:16

Elv1zz force-pushed the master branch from 64ad7a4 to b9826ce Compare May 21, 2023 18:31

Elv1zz force-pushed the master branch from d81c0a5 to a738d45 Compare June 1, 2023 21:25

Elv1zz force-pushed the master branch 3 times, most recently from 80f35a3 to 901fc9b Compare July 7, 2023 20:05

Elv1zz force-pushed the master branch from 901fc9b to 98f8dc9 Compare July 30, 2023 20:48

Elv1zz force-pushed the master branch from 98f8dc9 to 9802fb7 Compare September 18, 2023 18:24

Elv1zz force-pushed the master branch from 9802fb7 to 355d84f Compare October 27, 2023 19:01

Elv1zz added 8 commits January 10, 2024 21:17

Removing magic number in NextcloudClient

dcb3cea

Instead of using the magic number `400` for the HTTP status code which causes us to drop the selected TLS client certificate, the corresponding named constant from `HttpStatus` is used. Signed-off-by: Elv1zz <[email protected]>

Did some missed replacements

791c120

Some replacements of `InteractiveKeyManager` to `AdvancedX509KeyManager` were missed. This was now fixed. Signed-off-by: Elv1zz <[email protected]>

Added required permission

6114ed4

To send notifications, we need the corresponding permission, so we have to add it to the `AndroidManifest.xml`. Signed-off-by: Elv1zz <[email protected]>

Check for POST_NOTIFICATIONS permission

4dbea11

Before sending a notification, check if the required permission is granted. If it's not granted, we cannot request it, since we do not have an Actvity as that's the only time we try to send notifications. Signed-off-by: Elv1zz <[email protected]>

Elv1zz added 15 commits January 10, 2024 21:17

Fixing compilation error in unit tests

a80b92c

Forgot to update `NextcloudClient` creation to extended constructor signature, which also requires a `Context` instance. Signed-off-by: Elv1zz <[email protected]>

Get application context from context

ebffa49

We try to get the application context from the `Context` instance passed to `AdvancedX509KeyManager`. This mainly fixed the `AbstractIT` instrumented test, but should make things more robust for future use. Signed-off-by: Elv1zz <[email protected]>

Adding documentation

c19c24b

Added documentation to implementation of `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <[email protected]>

Codacy: One declaration per line

90b7812

Use one line for each declaration, it enhances code readability. Signed-off-by: Elv1zz <[email protected]>

Codacy: Declare fields at the top of the class

66e0fc0

Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes. Signed-off-by: Elv1zz <[email protected]>

Codacy: Try to reduce NPath complexity of method

d70d00f

The method 'matches(AKMAlias)' has an NPath complexity of 540, current threshold is 200. This is an attempt to reduce the method's complexity. Signed-off-by: Elv1zz <[email protected]>

Codacy: Document empty method bodies

c0f6ab8

Added a brief documentation to empty method bodies which implement the `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <[email protected]>

Fixing format string

a5f024c

Fixing broken format string where `%` and `$` were mixed up. Signed-off-by: Elv1zz <[email protected]>

Avoid TAG being null

a392523

By using the `class.getName()` instead of `class.getCanonicalName()` the `TAG` should not become `null`. Signed-off-by: Elv1zz <[email protected]>

Added unit test for AKMAlias.matches

6331c76

Added an unit test for the `AKMAlias.matches` method before refactoring it. Signed-off-by: Elv1zz <[email protected]>

Reduce NPath complexity of AKMAlias.matches

6b096b7

This should reduce NPath complexity of `AKMAlias.matches` method without changing its behavior. This was verified using the previously added unit test that should cover all relevant cases. Signed-off-by: Elv1zz <[email protected]>

Codacy issues in test

40cb798

Codacy did not like my test in several points: - The function name may not have an underscore (`_`). - Magic number (`123`) should be replaced with a named constant. - File should end with a new line. Signed-off-by: Elv1zz <[email protected]>

Elv1zz force-pushed the master branch from 355d84f to 40cb798 Compare January 10, 2024 20:52

tobiasKaminsky mentioned this pull request Jan 23, 2024

Client cert #1308

Merged

tobiasKaminsky closed this Jan 23, 2024

Elv1zz mentioned this pull request May 24, 2024

🔑 Support for client certificates pulsejet/memories#1169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #276 - Support for TLS client certificates #1048

Issue #276 - Support for TLS client certificates #1048

Elv1zz commented Jan 27, 2023

kushmittal2009 left a comment

Elv1zz commented Jan 30, 2023

kushmittal2009 commented Jan 30, 2023

Elv1zz commented Jan 31, 2023

tobiasKaminsky commented Feb 10, 2023

Elv1zz commented Mar 2, 2023

codecov bot commented Mar 9, 2023 •

edited

Loading

functionpointer commented Mar 28, 2023

Elv1zz commented Mar 31, 2023

Elv1zz commented Jan 13, 2024

functionpointer commented Jan 17, 2024

tobiasKaminsky commented Jan 23, 2024

tobiasKaminsky commented Jan 23, 2024

Elv1zz commented Jan 24, 2024

Issue #276 - Support for TLS client certificates #1048

Issue #276 - Support for TLS client certificates #1048

Conversation

Elv1zz commented Jan 27, 2023

kushmittal2009 left a comment

Choose a reason for hiding this comment

Elv1zz commented Jan 30, 2023

kushmittal2009 commented Jan 30, 2023

Elv1zz commented Jan 31, 2023

tobiasKaminsky commented Feb 10, 2023

Elv1zz commented Mar 2, 2023

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

functionpointer commented Mar 28, 2023

Elv1zz commented Mar 31, 2023

Elv1zz commented Jan 13, 2024

functionpointer commented Jan 17, 2024

tobiasKaminsky commented Jan 23, 2024

tobiasKaminsky commented Jan 23, 2024

Elv1zz commented Jan 24, 2024

codecov bot commented Mar 9, 2023 •

edited

Loading