Issue #276 - Support for TLS client certificates #1048

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]>

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

Commits on Jan 10, 2024