Client cert #1308
Merged
Client cert #1308
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tobiasKaminsky
force-pushed
the
clientCert
branch
from
40cb798
to
c9b913b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| SSLContext sslContext = getSSLContext(); | ||
| sslContext.init(null, tms, null); | ||
| sslContext.init(kms, tms, null); |
Check failure
Code scanning / CodeQL
`TrustManager` that accepts all certificates High
|
|
||
| val sslContext = NetworkUtils.getSSLContext() | ||
|
|
||
| sslContext.init(null, arrayOf<TrustManager>(trustManager), null) | ||
| sslContext.init(arrayOf(keyManager), arrayOf<TrustManager>(trustManager), null) |
Check failure
Code scanning / CodeQL
`TrustManager` that accepts all certificates High
|
|
||
| val sslContext = NetworkUtils.getSSLContext() | ||
|
|
||
| sslContext.init(null, arrayOf<TrustManager>(trustManager), null) | ||
| sslContext.init(arrayOf(keyManager), arrayOf<TrustManager>(trustManager), null) |
Check failure
Code scanning / CodeQL
`TrustManager` that accepts all certificates High
Codecov Report
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
|
|
/rebase |
nextcloud-command
force-pushed
the
clientCert
branch
from
c9b913b
to
ffb7f56
Compare
SpotBugsSpotBugs increased! |
|
/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 <[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]>
nextcloud-command
force-pushed
the
clientCert
branch
from
ffb7f56
to
66aa603
Compare
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: tobiasKaminsky <[email protected]>
|
Some nice improvements and fixes, thanks 👍 |
|
At least for optional client certificate support there are still open issues, e.g #nextcloud/android#12931 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR from #1048 to allow CI