Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rohm1
Copy link

@rohm1 rohm1 commented Dec 17, 2024

🎟️ Tracking

#4416

📔 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

Screenshot (Dec 18, 2024 19_40_38)

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.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-16157

@bitwarden-bot bitwarden-bot changed the title #4416 PM-15037 mTLS [PM-16157] #4416 PM-15037 mTLS Dec 17, 2024
@rohm1 rohm1 changed the title [PM-16157] #4416 PM-15037 mTLS [PM-16157] #4416 mTLS Dec 18, 2024
@vvolkgang vvolkgang linked an issue Dec 18, 2024 that may be closed by this pull request
1 task
@vvolkgang vvolkgang changed the title [PM-16157] #4416 mTLS [PM-16157] Support self-host servers with TLS with Client Authentication (mTLS) Dec 18, 2024
@vvolkgang vvolkgang changed the title [PM-16157] Support self-host servers with TLS with Client Authentication (mTLS) [PM-16157] Support self-host servers using TLS with Client Authentication (mTLS) Dec 18, 2024
@vvolkgang
Copy link
Member

vvolkgang commented Dec 18, 2024

👋🏾 Looking great, thanks for your contribution! We may only be able to action after the holiday season but have some preliminary questions:

  1. Can you provide a screen recording of the user interaction?
  2. Just to confirm, KeyChainCallback handles both choosing an existing system certificate or importing a new one to the system KeyChain correct?
  3. Did you consider also support in-app importing to a KeyStore like it was done here? Mostly curious if you tried it and bumped into any issues but I understand it's a bigger lift that may not be needed for your usecase.
  4. 💭 For system certificates we may need to store more than just the alias to reduce the surface of unintentional certificate swaps.

@rohm1 rohm1 force-pushed the PM-15537/mTLS-client-certificate branch from 67060f7 to 5cf6919 Compare December 18, 2024 18:46
@rohm1
Copy link
Author

rohm1 commented Dec 18, 2024

Hey @vvolkgang ,

thanks for the quick feedback! To your questions:

  1. Screen recording when user has a certificate installed. Clicking "select" will take the value, clicking "Deny" will clear the input box
screen-20241218-193902.mp4

Screen recording when no client certificate is installed. In this case Android displays nothing. To avoid confusing the user too much, I updated the description text.

screen-20241218-194202.mp4
  1. Are your referring to Android's KeyChainAliasCallback or to my ChoosePrivateKeyAliasCallback (which is a wrapper for the Android object)? anyway, the current implementation can only choose a certificate that has previously been installed, no import
  2. No I did no try to implement the in-app import. Should we update the description text to make it clear?
  3. Do you mean certificates installed/managed by the system? As you can see in the screen recording, they are not available in the selection menu, so nothing more to do?

@rohm1 rohm1 force-pushed the PM-15537/mTLS-client-certificate branch from 5cf6919 to 61b9170 Compare December 18, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connecting to a server with TLS Client Authentication crashes app
3 participants