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-3089] mTLS - Authenticate with Client Certificate #2629

Closed
wants to merge 19 commits into from
Closed

Conversation

oguzhane
Copy link
Mannequin

@oguzhane oguzhane mannequin commented Jul 19, 2023

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

This PR brings ability to login a bitwarden server by using a client certificate. The functionility has been implemented for Android.
User can import a X509 client certificate in pkcs#12 format to KeyStore or user can use existing certificates from keychain.

See: #582

Code changes

CertificateService.cs: responsible to interact with platform's native key management apis.
AdvancedPage.xaml: page where user setup the certificate.
AndroidHttpsHandler.cs: http handler that send the requests with client certificate

Screenshots

Demo

mtls-bitwarden.mp4

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@CLAassistant
Copy link
Mannequin

CLAassistant mannequin commented Jul 19, 2023

CLA assistant check
All committers have signed the CLA.

@bitwarden-bot
Copy link
Mannequin

bitwarden-bot mannequin commented Jul 19, 2023

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

@bitwarden-bot bitwarden-bot mannequin changed the title mTLS - Authenticate with Client Certificate [PM-3089] mTLS - Authenticate with Client Certificate Jul 19, 2023
@bitwarden-bot bitwarden-bot mannequin added the community-pr label Jul 19, 2023
@bitwarden-bot
Copy link
Mannequin

bitwarden-bot mannequin commented Jul 19, 2023

Logo
Checkmarx One – Scan Summary & Details65f876b0-b70f-4dcd-930b-7823dd7d4312

No New Or Fixed Issues Found


namespace Bit.iOS.Core.Services
{
public class CertificateService : ICertificateService
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I incorrectly assumed this had already been implemented on iOS, and we require platform parity (where possible) for community PRs. Are you planning to add this?

Copy link
Mannequin Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpbw2 I would be willing to add for iOS but i've no dev/test envrionment to implement for iOS

Copy link
Mannequin

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpbw2 I would be willing to add for iOS but i've no dev/test envrionment to implement for iOS

@oguzhane I'd be happy to send you a cheap/used iPhone from Swappa if you don't have one and you're willing/able to put the time into iOS support

If so, you can reach me at:

github@ mygithubusernamehere with the TLD of com

Either way, I appreciate the work you've done on this, if that's not obvious

I only need to know where to ship it; optionally, I may be able to find a gift card to send via e-mail, and you can handle ordering and shipping, if you have any privacy concerns

Copy link
Contributor

@mpbw2 mpbw2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comment about iOS.

Also our design team has requested the cert handling UI be moved to the self-hosted configuration page (instead of adding an Advanced menu during the login flow). Here's a mockup:

Screenshot 2023-07-23 at 10 34 25 PM

@oguzhane
Copy link
Mannequin Author

oguzhane mannequin commented Jul 25, 2023

See inline comment about iOS.

Also our design team has requested the cert handling UI be moved to the self-hosted configuration page (instead of adding an Advanced menu during the login flow). Here's a mockup:

Screenshot 2023-07-23 at 10 34 25 PM

Makes sense. I will be moving it to the self-hosted config page.

@oguzhane
Copy link
Mannequin Author

oguzhane mannequin commented Jul 29, 2023

See inline comment about iOS.
Also our design team has requested the cert handling UI be moved to the self-hosted configuration page (instead of adding an Advanced menu during the login flow). Here's a mockup:
Screenshot 2023-07-23 at 10 34 25 PM

Makes sense. I will be moving it to the self-hosted config page.

@mpbw2 i've moved it to environment page
image

@biscuit-316
Copy link
Mannequin

biscuit-316 mannequin commented Sep 9, 2023

I really would like this feature to be added. Especially because I notice that my Wireguard VPN does not work everywhere.

I saw here that you have an .apk file on your GitHub page. I've tried both this and this version. in both versions I found two problems.

The first problem is that the button to use system certificates does not work. When I press the button, I see a button pressed animation. but other than that nothing happens. The screen to select the certificate doesn't open.

The second problem is that the app does not accept the certificate if it is not encrypted with a password. To test the mutaul tls function I created a new certificate authority. For convenience I downloaded my client certificate without a password. But I kept getting the following error message. "Failed to import the certificate exception decrypting data -java". Even though the certificate is not encrypted, the app does ask for a password. Even if I leave the password field blank, I get this message.

@mzpqnxow
Copy link
Mannequin

mzpqnxow mannequin commented Sep 22, 2023

I really would like this feature to be added. Especially because I notice that my Wireguard VPN does not work everywhere.

Same here!

I saw here that you have an .apk file on your GitHub page. I've tried both this and this version. in both versions I found two problems.

I have iOS here but I may be able to guess your issue(s)

The first problem is that the button to use system certificates does not work. When I press the button, I see a button pressed animation. but other than that nothing happens. The screen to select the certificate doesn't open.

Seems like a non-ideal UX behavior, or bug. Sorry, can't help here. Do you not have any keypairs in your system store, I wonder? I imagine the behavior is still not desired- an error message would probably be better- but maybe that's an untested edge-case snd that's the behavior when there are no keypairs available. Perhaps some interface returns null or some exception/return code rather than raising the appropriate dialog, and the error is not handled in a helpful way

The second problem is that the app does not accept the certificate if it is not encrypted with a password. To test the mutaul tls function I created a new certificate authority. For convenience I downloaded my client certificate without a password. But I kept getting the following error message. "Failed to import the certificate exception decrypting data -java". Even though the certificate is not encrypted, the app does ask for a password. Even if I leave the password field blank, I get this message.

It sounds (based on the presence of "-java" in the error) that it's an improperly exported Java Keystore? Perhaps a raw JKS file?

I expect the application may want either PKCS12 or x509 format. It almost certainly doesn't support a JKS file

PKCS12 files (common extensions pfx, p12) are single password protected files with a standard/well-defined format that contain multiple files within them- certs, private keys, CRLs. The files inside should be x509 files

If the app has separate boxes for client certificate, client private key, and CA, it almost certainly wants x509 format, one for certificate, one for private key, one for CA. Optionally, it may accept the certificate and private key combined into PEM encoded x509, concatenated into one file. The CA should still be separate

If you have your certs/keys in a JKS, export them using keytool since JKS is not a standard format recognized outside of Java (Google "export jks x509" or "export jks x509 pem")

Note: Creation of a PKCS12 file will require you to specify a password when creating it and loading it into the app- it's required by the standard afaik. In that case, you should use the same password when loading it as when you created it. The private key inside the PKCS12 does not need a password, though

Remember to include the CA file, certificate and private key all in one PKCS12 (if using PKCS12) and do NOT create multiple PKCS12 files

This is my best guess, having not seen or tested the app yet. Hopefully it's helpful and I haven't misunderstood your issue

@oguzhane
Copy link
Mannequin Author

oguzhane mannequin commented Sep 22, 2023

Pkcs#12 is only supported format but it doesn't support chain of multiple certs in the file. i already tried to import with CA but didn't work. Hence, i had to register CA system wide.
i used openssl to generate pkcs#12. with that, i have been using the feature flawlessy over a month.
i tested importing from the system on emulator. Haven't tried on a real device.

@mzpqnxow
Copy link
Mannequin

mzpqnxow mannequin commented Sep 22, 2023

First, thanks for taking the time to work on this, and to clarify on this

Pkcs#12 is only supported format but it doesn't support chain of multiple certs in the file

Regarding the chain: it may want the CA + intermediates as a single PEM- the CA concatenated with the concatenated intermediate PEMs, all in one big PEM. Just an idea should you be interested in trying it

^-- Ignore this and see next post, it's a policy restriction in Android >= 11

Or maybe you meant it didn't like having a CA and a client cert in the PKCS12- in which case my only idea would be to ensure the client certificate doesn't have the CA bit set (and make sure the CA cert does)

There may be some policies introduced in Android preventing the CA from being recognized as such when in the PFX- something to do with certificate bits, the lifetime if the cert, etc. Though I would expect a clear error code if it was being rejected or ignored for such a reason. I'll look into the docs, though I'm not necessarily expecting you to take any action- as I said, fine with me. Mostly a curiousity 😆

Regardless, it's no bother to me either way, I'm thrilled either way, I was just chiming in to assist @biscuit-316 who seems to have much larger issues (that sound to me like possibly pilot error, the "-java" string is a bit suspect)

i already tried to import with CA but didn't work. Hence, i had to register CA system wide.

I wonder if iOS will be OK with this. I believe iOS prevents apps from utilizing client certificates from the system store. Hopefully it's not the same with CA certificates

On the topic of iOS certificate policies- I believe the "Import System Certificate" you implemented for Android will need to be conditionally left out for iOS, because of what I previously mentioned re: app access to the client cert system store. There's no privilege the user can grant to override/relax that control, either. I know you're not set up to test iOS so this isn't immediately useful to you, I'm mentioning it only as an FYI. I can probably find a reference on that, even if only to keep me honest in case I'm mistaken

i used openssl to generate pkcs#12. with that, i have been using the feature flawlessy over a month.
i tested importing from the system on emulator. Haven't tried on a real device.

👍

Thanks again!

@mzpqnxow
Copy link
Mannequin

mzpqnxow mannequin commented Sep 22, 2023

This may explain the CA issue- new policies in Android as of Android 11 (?) preventing apps from adding to even the user CA store

Additional source

Not exactly an authoritative source, but seems coherent and detailed enough to be trustworthy

@biscuit-316
Copy link
Mannequin

biscuit-316 mannequin commented Sep 22, 2023

It seems like a non-ideal UX behavior or a bug. Sorry, I can't help here. Don't have any key pairs in your system store, I wonder? I imagine the behavior is still not desired - an error message would probably be better - but perhaps this is an untested edge case and that is the behavior when no key pairs are available. Perhaps an interface returns null or an exception/return code instead of opening the proper dialog, and the error is not handled in a useful way

I copied the client certificate to my smartphone. I then installed the certificate using the "Install certificate from device storage" button. that button is in settings > Security & privacy > Other security settings > Install certificate from device storage.

When I open the URL in Chrome (the Android app), Chrome asks me which certificate to use. Only one certificate is installed, namely the one for bitwarden. When I select that certificate, the Vaultwarden web vault appears in the browser.

Since Chrome was able to use the certificate, I assumed I had installed it correctly.
That's why I assumed the button was the problem.

This certificate was already installed when I tried to use the button in the bitwarden app.

(My smartphone is not set to English, so the path in the settings may be different.)

It sounds (based on the presence of "-java" in the error) that it's an improperly exported Java Keystore? Perhaps a raw JKS file?

I expect the application may want either PKCS12 or x509 format. It almost certainly doesn't support a JKS file

PKCS12 files (common extensions pfx, p12) are single password protected files with a standard/well-defined format that contain multiple files within them- certs, private keys, CRLs. The files inside should be x509 files

If the app has separate boxes for client certificate, client private key, and CA, it almost certainly wants x509 format, one for certificate, one for private key, one for CA. Optionally, it may accept the certificate and private key combined into PEM encoded x509, concatenated into one file. The CA should still be separate

If you have your certs/keys in a JKS, export them using keytool since JKS is not a standard format recognized outside of Java (Google "export jks x509" or "export jks x509 pem")

Note: Creation of a PKCS12 file will require you to specify a password when creating it and loading it into the app- it's required by the standard afaik. In that case, you should use the same password when loading it as when you created it. The private key inside the PKCS12 does not need a password, though

Remember to include the CA file, certificate and private key all in one PKCS12 (if using PKCS12) and do NOT create multiple PKCS12 files

This is my best guess, having not seen or tested the app yet. Hopefully it's helpful and I haven't misunderstood your issue

My client certificate file is in PCKS#12 format. The certificate is generated and signed by my PFsense Firewall. In PFsense there are two options to download the certificate.
One of the options is: "Export PCKS#12 archive without encryption". This is the option I use to create a certificate without a password.

The certificate without password can be installed within the system certificate store without any problem.

@biscuit-316
Copy link
Mannequin

biscuit-316 mannequin commented Sep 22, 2023

Pkcs#12 is only supported format but it doesn't support chain of multiple certs in the file. i already tried to import with CA but didn't work. Hence, i had to register CA system wide.
i used openssl to generate pkcs#12. with that, i have been using the feature flawlessy over a month.
i tested importing from the system on emulator. Haven't tried on a real device.

The client certificate I used was in the format PKCS#12.
The certificate also worked with this version of the bitwarden app. When I use the "Import Certificate" button, and the certificate is password protected.

My client certificate is generated and signed by my PFsense Firewall. That's why I couldn't tell you what kind of files are in there.

@sandstormkeshav
Copy link
Mannequin

sandstormkeshav mannequin commented Dec 17, 2023

Hi @oguzhane is there anything else that needs to be done on this PR? Can't wait to see this get merged in!

@dbosompem
Copy link
Mannequin

dbosompem mannequin commented Jan 8, 2024

Hi All, looks like lots of or community members were interested to see this feature being implemented, but we have some upcoming changes coming soon for our mobile app which is pretty significant, and unfortunately this contribution doesn’t align with that direction, now. What we are working on could fix this, or we could possibly revisit this feature once that is done. Unfortunately this PR would have to be closed. Thank you!

@dbosompem dbosompem mannequin closed this Jan 8, 2024
@einot
Copy link
Mannequin

einot mannequin commented Feb 7, 2024

This is really sad to see. I was really hoping for this feature as it would greatly enhance the security of the system.

@Daniel-dev22
Copy link
Mannequin

Daniel-dev22 mannequin commented Apr 29, 2024

Hi All, looks like lots of or community members were interested to see this feature being implemented, but we have some upcoming changes coming soon for our mobile app which is pretty significant, and unfortunately this contribution doesn’t align with that direction, now. What we are working on could fix this, or we could possibly revisit this feature once that is done. Unfortunately this PR would have to be closed. Thank you!

Now that it's been a few months, was this added as part of the changes that were implemented?

@yarete03
Copy link
Mannequin

yarete03 mannequin commented May 1, 2024

It may sounds like a silly question. Is there a way to run in iOS this client version in order to be able to implement the cert feature?

@Daniel-dev22
Copy link
Mannequin

Daniel-dev22 mannequin commented May 20, 2024

As I know the bitwarden app is being rewritten into a different framework, it's worth mentioning the home assistant app written in swift for iOS and kotlin for Android supports MTLs and is open source which can potentially contain pointers on how to implement this in the bitwarden app.

Android
https://github.com/home-assistant/android/blob/3833e73dfeb42ba9fa336d4d1cc5bc22941bb7e8/common/src/main/java/io/homeassistant/companion/android/common/data/TLSHelper.kt#L29

iOS
https://github.com/home-assistant/iOS/blob/20a59306e20959d955e3bb0111132a00e0dcd016/Sources/App/Onboarding/API/Steps/OnboardingAuthStepConnectivity.swift#L87

@Daniel-dev22
Copy link

Now that the new native repos are up for Android and iOS maybe a contributor or bitwarden themselves might tackle implementing MTLs?

https://github.com/bitwarden/android
https://github.com/bitwarden/ios

As mentioned previously here's an example for Android/iOS on implementing MTLs.
#2629 (comment)

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.