github-starred/android
2
0
Fork 0
mirror of https://github.com/bitwarden/android.git synced 2026-03-14 14:16:23 -05:00
Code Issues 144 Packages Projects Releases 116 Wiki Activity

[PR #4486] [MERGED] [PM-16157] Support self-host servers using TLS with Client Authentication (mTLS) #4986

New Issue
Closed
opened 2025-11-26 23:52:29 -06:00 by GiteaMirror · 0 comments
GiteaMirror commented 2025-11-26 23:52:29 -06:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/bitwarden/android/pull/4486
Author: @rohm1
Created: 12/17/2024
Status: ✅ Merged
Merged: 2/10/2025
Merged by: @SaintPatrck

Base: main ← Head: PM-15537/mTLS-client-certificate

📝 Commits (10+)

  • 5ec0c10 #4416 add possibility to select a client certificate for mTLS
  • 4bd2fef Fix detekt issues
  • 9d08672 Merge remote-tracking branch 'origin/main' into PM-15537/mTLS-client-certificate
  • d5a2418 Migrate system certificate alias selection to KeyChainManager
  • 9682891 Remove manually added translations
  • 30f167a Revert unnecessary change to RootNavScreen.kt
  • 4b920e8 Merge remote-tracking branch 'refs/remotes/origin/main' into PM-15537/mTLS-client-certificate
  • bbbb009 Merge remote-tracking branch 'refs/remotes/origin/main' into PM-15537/mTLS-client-certificate
  • 43ad4f3 Replace KeyChainRepository and TLSHelper with SslManager
  • 1c717c5 (WIP) Implement certificate import and system certificate selection in EnvironmentScreen

📊 Changes

21 files changed (+1713 additions, -26 deletions)

View changed files

📝 app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/model/EnvironmentUrlDataJson.kt (+6 -0)
📝 app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt (+17 -0)
📝 app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt (+26 -0)
➕ app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/SslManager.kt (+20 -0)
➕ app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/SslManagerImpl.kt (+116 -0)
📝 app/src/main/java/com/x8bit/bitwarden/data/platform/manager/KeyManager.kt (+5 -1)
📝 app/src/main/java/com/x8bit/bitwarden/data/platform/manager/KeyManagerImpl.kt (+1 -1)
📝 app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/ImportPrivateKeyResult.kt (+1 -1)
📝 app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt (+144 -9)
📝 app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt (+362 -7)
📝 app/src/main/java/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt (+7 -0)
➕ app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManager.kt (+18 -0)
➕ app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerImpl.kt (+42 -0)
➕ app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/model/PrivateKeyAliasSelectionResult.kt (+17 -0)
📝 app/src/main/res/values/strings.xml (+16 -1)
📝 app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt (+105 -0)
➕ app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/SslManagerTest.kt (+218 -0)
📝 app/src/test/java/com/x8bit/bitwarden/data/platform/manager/KeyManagerTest.kt (+1 -1)
📝 app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt (+82 -3)
📝 app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModelTest.kt (+374 -2)

...and 1 more files

📄 Description

🎟️ Tracking

https://github.com/bitwarden/android/issues/4416

https://bitwarden.atlassian.net/browse/PM-17240
https://bitwarden.atlassian.net/browse/PM-16157

📔 Objective

This PR adds a "Client certificate" menu to the custom environment menu. By clicking on a "Choose" button, the native certificate selection menu will open. User can choose a certificate that will be used for connections to a self-hosted instance.

Other apps implement some dynamic logic: if the sever requires mTLS, the user is prompted to select a certificate. While this behavior is more flexible for the user, it is more complicated to implement. With the chosen approach, the app settings must be cleared and a new server must be set up. As users generally do not enable/disable mTLS on a regular basis, I think the tradeoff is acceptable. Additionally the only information that is saved is the certificate name (key alias). If a user update/renew a certificate, as long as the certificate name does not change, the new certificate will be used and there is no need to reset the app. This issue could also be worked around by adding a certificate selection menu to the settings.

📸 Screenshots

select-mtls-certificate-complete.webm

Manual tests I did

  • Upgrade from main does not crash and old settings are still saved/available/used
  • It is still possible to connect to server without mTLS
  • A certificate can be selected and saved in the settings. Certificate is used for connections to the server
  • Close the app and restart: Certificate setting has been persisted and connection is still using mTLS

Tested on Android 15 (Pixel 6) and Android 14 (Samsung Galaxy Note 20 Ultra 5G)

🦮 Reviewer guidelines

  • 📝 I am not an Android dev and this is my first time with Kotlin and contributing to the Bitwarden project. While I have tried to follow the architecture, I'm sure I missed some things and and others are not in the right place. I apologize in advance and look forward how to improve the code
  • ❓ The Android APIs use the key alias wording (KeyChain.choosePrivateKeyAlias), and this is also the wording I chose in the code. For the UI I chose to go with Client certificate. While this is generally bad to have two wordings for the same thing, key alias cannot be used in the UI because confusing for the user, and user certificate in code is kind of weird because that would be something else that the Android wording. Open for suggestions
  • 🌱 Following the current approach, we can add a Certificate selection screen to the settings. This way it is not necessary anymore to reset the app to choose a new certificate.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/bitwarden/android/pull/4486 **Author:** [@rohm1](https://github.com/rohm1) **Created:** 12/17/2024 **Status:** ✅ Merged **Merged:** 2/10/2025 **Merged by:** [@SaintPatrck](https://github.com/SaintPatrck) **Base:** `main` ← **Head:** `PM-15537/mTLS-client-certificate` --- ### 📝 Commits (10+) - [`5ec0c10`](https://github.com/bitwarden/android/commit/5ec0c1010104b6411b39d4badebbe1b8f59ee2e8) #4416 add possibility to select a client certificate for mTLS - [`4bd2fef`](https://github.com/bitwarden/android/commit/4bd2fef67f312b2009885c787433185c854c5db3) Fix detekt issues - [`9d08672`](https://github.com/bitwarden/android/commit/9d086724381afee5605cecaabb2b1e985c830c4d) Merge remote-tracking branch 'origin/main' into PM-15537/mTLS-client-certificate - [`d5a2418`](https://github.com/bitwarden/android/commit/d5a24180bb52090fb1077e2a2a784f3bac9b9630) Migrate system certificate alias selection to KeyChainManager - [`9682891`](https://github.com/bitwarden/android/commit/968289134bfec8b6f6f794b632baaf7a4e26e16e) Remove manually added translations - [`30f167a`](https://github.com/bitwarden/android/commit/30f167ae503313b2838591bec59439f263e87db7) Revert unnecessary change to RootNavScreen.kt - [`4b920e8`](https://github.com/bitwarden/android/commit/4b920e81a26faef8a2b31d114065b352dd220d5b) Merge remote-tracking branch 'refs/remotes/origin/main' into PM-15537/mTLS-client-certificate - [`bbbb009`](https://github.com/bitwarden/android/commit/bbbb0099b120056ce162a64c871037f6b1ec70f6) Merge remote-tracking branch 'refs/remotes/origin/main' into PM-15537/mTLS-client-certificate - [`43ad4f3`](https://github.com/bitwarden/android/commit/43ad4f39f729fa22338639283d3b729f9c72d129) Replace KeyChainRepository and TLSHelper with SslManager - [`1c717c5`](https://github.com/bitwarden/android/commit/1c717c5eea81b8d16dcf1691e3e38626d0b4cd73) (WIP) Implement certificate import and system certificate selection in EnvironmentScreen ### 📊 Changes **21 files changed** (+1713 additions, -26 deletions) <details> <summary>View changed files</summary> 📝 `app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/model/EnvironmentUrlDataJson.kt` (+6 -0) 📝 `app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt` (+17 -0) 📝 `app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt` (+26 -0) ➕ `app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/SslManager.kt` (+20 -0) ➕ `app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/SslManagerImpl.kt` (+116 -0) 📝 `app/src/main/java/com/x8bit/bitwarden/data/platform/manager/KeyManager.kt` (+5 -1) 📝 `app/src/main/java/com/x8bit/bitwarden/data/platform/manager/KeyManagerImpl.kt` (+1 -1) 📝 `app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/ImportPrivateKeyResult.kt` (+1 -1) 📝 `app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt` (+144 -9) 📝 `app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt` (+362 -7) 📝 `app/src/main/java/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt` (+7 -0) ➕ `app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManager.kt` (+18 -0) ➕ `app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerImpl.kt` (+42 -0) ➕ `app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/model/PrivateKeyAliasSelectionResult.kt` (+17 -0) 📝 `app/src/main/res/values/strings.xml` (+16 -1) 📝 `app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt` (+105 -0) ➕ `app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/SslManagerTest.kt` (+218 -0) 📝 `app/src/test/java/com/x8bit/bitwarden/data/platform/manager/KeyManagerTest.kt` (+1 -1) 📝 `app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt` (+82 -3) 📝 `app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModelTest.kt` (+374 -2) _...and 1 more files_ </details> ### 📄 Description ## 🎟️ Tracking https://github.com/bitwarden/android/issues/4416 https://bitwarden.atlassian.net/browse/PM-17240 https://bitwarden.atlassian.net/browse/PM-16157 ## 📔 Objective This PR adds a "Client certificate" menu to the custom environment menu. By clicking on a "Choose" button, the native certificate selection menu will open. User can choose a certificate that will be used for connections to a self-hosted instance. Other apps implement some dynamic logic: if the sever requires mTLS, the user is prompted to select a certificate. While this behavior is more flexible for the user, it is more complicated to implement. With the chosen approach, the app settings must be cleared and a new server must be set up. As users generally do not enable/disable mTLS on a regular basis, I think the tradeoff is acceptable. Additionally the only information that is saved is the certificate name (key alias). If a user update/renew a certificate, as long as the certificate name does not change, the new certificate will be used and there is no need to reset the app. This issue could also be worked around by adding a certificate selection menu to the settings. ## 📸 Screenshots [select-mtls-certificate-complete.webm](https://github.com/user-attachments/assets/f407776d-8ad4-44bf-b451-737afb1857ae) ## Manual tests I did - Upgrade from main does not crash and old settings are still saved/available/used - It is still possible to connect to server without mTLS - A certificate can be selected and saved in the settings. Certificate is used for connections to the server - Close the app and restart: Certificate setting has been persisted and connection is still using mTLS Tested on Android 15 (Pixel 6) and Android 14 (Samsung Galaxy Note 20 Ultra 5G) ## 🦮 Reviewer guidelines - 📝 I am not an Android dev and this is my first time with Kotlin and contributing to the Bitwarden project. While I have tried to follow the architecture, I'm sure I missed some things and and others are not in the right place. I apologize in advance and look forward how to improve the code - ❓ The Android APIs use the _key alias_ wording [(KeyChain.choosePrivateKeyAlias),](https://developer.android.com/reference/android/security/KeyChain#choosePrivateKeyAlias(android.app.Activity,%20android.security.KeyChainAliasCallback,%20java.lang.String[],%20java.security.Principal[],%20java.lang.String,%20int,%20java.lang.String)) and this is also the wording I chose in the code. For the UI I chose to go with _Client certificate_. While this is generally bad to have two wordings for the same thing, _key alias_ cannot be used in the UI because confusing for the user, and _user certificate_ in code is kind of weird because that would be something else that the Android wording. Open for suggestions - 🌱 Following the current approach, we can add a Certificate selection screen to the settings. This way it is not necessary anymore to reset the app to choose a new certificate. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
GiteaMirror added the pull-request label 2025-11-26 23:52:29 -06:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
app:authenticator
app:password-manager
bug
bug-passkey
customer-support
duplicate
enhancement
good first issue
help wanted
needs-reply
needs-response
pull-request
Mirrored from GitHub Pull Request
 question
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/android#4986
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?