Client cert #1308

tobiasKaminsky · 2024-01-23T07:51:01Z

PR from #1048 to allow CI

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)


codecov · 2024-01-23T08:12:04Z

Codecov Report

Merging #1308 (fe5b7b4) into master (5d74d6f) will decrease coverage by 1.67%.
The diff coverage is 20.67%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1308      +/-   ##
============================================
- Coverage     51.00%   49.33%   -1.67%     
- Complexity      978      990      +12     
============================================
  Files           198      200       +2     
  Lines          7298     7697     +399     
  Branches        945      989      +44     
============================================
+ Hits           3722     3797      +75     
- Misses         3065     3383     +318     
- Partials        511      517       +6

Files	Coverage Δ
.../src/main/java/com/nextcloud/common/PlainClient.kt	`68.96% <100.00%> (+1.10%)`	⬆️
...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.44% <63.63%> (+0.91%)`	⬆️
.../main/java/com/nextcloud/common/NextcloudClient.kt	`50.00% <61.53%> (-0.62%)`	⬇️
...network/SelectClientCertificateHelperActivity.java	`0.00% <0.00%> (ø)`
...oid/lib/common/network/AdvancedX509KeyManager.java	`16.76% <16.76%> (ø)`

... and 2 files with indirect coverage changes

tobiasKaminsky · 2024-01-23T10:01:29Z

/rebase

github-actions · 2024-01-23T10:12:20Z

SpotBugs

SpotBugs increased!

github-actions · 2024-01-23T10:58:35Z

SpotBugs

Category	Base	New
Bad practice	7	8
Correctness	34	34
Dodgy code	26	27
Internationalization	6	6
Malicious code vulnerability	49	50
Multithreaded correctness	3	5
Performance	5	8
Total	130	138

SpotBugs increased!

tobiasKaminsky · 2024-01-24T06:12:30Z

/rebase

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 <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

Some replacements of `InteractiveKeyManager` to `AdvancedX509KeyManager` were missed. This was now fixed. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

To send notifications, we need the corresponding permission, so we have to add it to the `AndroidManifest.xml`. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

Forgot to update `NextcloudClient` creation to extended constructor signature, which also requires a `Context` instance. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

Added documentation to implementation of `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Use one line for each declaration, it enhances code readability. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

Added a brief documentation to empty method bodies which implement the `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Fixing broken format string where `%` and `$` were mixed up. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

By using the `class.getName()` instead of `class.getCanonicalName()` the `TAG` should not become `null`. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Added an unit test for the `AKMAlias.matches` method before refactoring it. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

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 <elv1zz.git@gmail.com>

github-actions · 2024-01-24T06:14:49Z

SpotBugs

Category	Base	New
Bad practice	7	8
Correctness	34	34
Dodgy code	26	27
Internationalization	6	6
Malicious code vulnerability	49	50
Multithreaded correctness	3	5
Performance	5	8
Total	130	138

SpotBugs increased!

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

github-actions · 2024-01-24T07:16:21Z

SpotBugs

Category	Base	New
Bad practice	7	7
Correctness	34	34
Dodgy code	26	26
Internationalization	6	6
Malicious code vulnerability	49	50
Multithreaded correctness	3	3
Performance	5	5
Total	130	131

SpotBugs increased!

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

github-actions · 2024-01-24T07:22:40Z

SpotBugs

Category	Base	New
Bad practice	7	7
Correctness	34	34
Dodgy code	26	26
Internationalization	6	6
Malicious code vulnerability	49	49
Multithreaded correctness	3	3
Performance	5	5
Total	130	130

Elv1zz · 2024-01-25T20:12:38Z

Some nice improvements and fixes, thanks 👍
Looking forward to see it in production

ne20002 · 2024-06-05T15:31:19Z

At least for optional client certificate support there are still open issues, e.g #nextcloud/android#12931

tobiasKaminsky force-pushed the clientCert branch from 40cb798 to c9b913b Compare January 23, 2024 07:54

tobiasKaminsky added the 3. to review label Jan 23, 2024

github-advanced-security bot found potential problems Jan 23, 2024

View reviewed changes

tobiasKaminsky mentioned this pull request Jan 23, 2024

Issue #276 - Support for TLS client certificates #1048

Closed

nextcloud-command force-pushed the clientCert branch from c9b913b to ffb7f56 Compare January 23, 2024 10:08

Elv1zz added 20 commits January 24, 2024 06:12

Removing magic number in NextcloudClient

32fa627

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 <elv1zz.git@gmail.com>

Did some missed replacements

9dc2c24

Some replacements of `InteractiveKeyManager` to `AdvancedX509KeyManager` were missed. This was now fixed. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Added required permission

39ec832

To send notifications, we need the corresponding permission, so we have to add it to the `AndroidManifest.xml`. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Fixing compilation error in unit tests

da3921a

Forgot to update `NextcloudClient` creation to extended constructor signature, which also requires a `Context` instance. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Adding documentation

478ceea

Added documentation to implementation of `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Codacy: One declaration per line

f6d87a0

Use one line for each declaration, it enhances code readability. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Codacy: Declare fields at the top of the class

5198fb7

Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Codacy: Try to reduce NPath complexity of method

982fe5f

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 <elv1zz.git@gmail.com>

Codacy: Document empty method bodies

24ef5b0

Added a brief documentation to empty method bodies which implement the `ActivityLifecycleCallbacks` interface. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Fixing format string

63d9a2b

Fixing broken format string where `%` and `$` were mixed up. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Avoid TAG being null

c98b142

By using the `class.getName()` instead of `class.getCanonicalName()` the `TAG` should not become `null`. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Elv1zz added 3 commits January 24, 2024 06:12

Added unit test for AKMAlias.matches

46c8382

Added an unit test for the `AKMAlias.matches` method before refactoring it. Signed-off-by: Elv1zz <elv1zz.git@gmail.com>

Reduce NPath complexity of AKMAlias.matches

8ca4642

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 <elv1zz.git@gmail.com>

Codacy issues in test

66aa603

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 <elv1zz.git@gmail.com>

nextcloud-command force-pushed the clientCert branch from ffb7f56 to 66aa603 Compare January 24, 2024 06:12

CI

cd72cda

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

do not use context as field

fe5b7b4

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky merged commit 1da2336 into master Jan 25, 2024
15 of 19 checks passed

delete-merged-branch bot deleted the clientCert branch January 25, 2024 11:42

tobiasKaminsky mentioned this pull request Jan 25, 2024

🔑 Support for client certificates nextcloud/android#603

Closed

joshtrichards mentioned this pull request Mar 20, 2024

Support mutual TLS auth #276

Open

Elv1zz mentioned this pull request May 24, 2024

🔑 Support for client certificates pulsejet/memories#1169

Open

iDontWantAUsername mentioned this pull request Sep 10, 2024

[Feature Request] Support for Client Certificates (mTLS) hegocre/NextcloudPasswords#126

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client cert #1308

Client cert #1308

tobiasKaminsky commented Jan 23, 2024 •

edited

Loading

codecov bot commented Jan 23, 2024 •

edited

Loading

tobiasKaminsky commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

tobiasKaminsky commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

Elv1zz commented Jan 25, 2024

ne20002 commented Jun 5, 2024

Client cert #1308

Client cert #1308

Conversation

tobiasKaminsky commented Jan 23, 2024 • edited Loading

codecov bot commented Jan 23, 2024 • edited Loading

Codecov Report

tobiasKaminsky commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

SpotBugs

SpotBugs increased!

github-actions bot commented Jan 23, 2024

SpotBugs

SpotBugs increased!

tobiasKaminsky commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

SpotBugs

SpotBugs increased!

github-actions bot commented Jan 24, 2024

SpotBugs

SpotBugs increased!

github-actions bot commented Jan 24, 2024

SpotBugs

Elv1zz commented Jan 25, 2024

ne20002 commented Jun 5, 2024

tobiasKaminsky commented Jan 23, 2024 •

edited

Loading

codecov bot commented Jan 23, 2024 •

edited

Loading