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

SSO using OpenID Connect #3899

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

Timshel
Copy link
Contributor

@Timshel Timshel commented Sep 18, 2023

This is based on previous PR (#2787, #2449 and #3154) with work done by @pinpox, @m4w0lf, @Sheap, @bmunro-peralex, @tribut and others I probably missed sorry.

This PR add support for OpenId Connect to handle authentication to an external SSO.
This introduce another way to control who can use the vault without having to use invitation or an LDAP.

A master password is still required and not controlled by the SSO (depending on your point of view this might be a feature ;).

Bitwarden key connector is not supported and due to the license it's highly unlikely that it will ever be:

2.1 Commercial Module License. Subject to Your compliance with this Agreement, Bitwarden hereby grants to You a limited, non-exclusive, non-transferable, royalty-free license to use the Commercial Modules for the sole purposes of internal development and internal testing, and only in a non-production environment.

Usage

This should be agnostic to the SSO used as long as it supports client secret authentication and expose an OpenID Connect Discovery endpoint. (I'm testing it with Keycloak at the moment, a demo test stack is available README.md)

Added some documentation at the root of the project SSO.md that could be later moved to the wiki.

I made some additional modification in my main branch to allow for easier testing (modified Docker image to use prebuilt patched front-end).

On front-end modification, I made patched versions available at Timshel/oidc_web_builds. Two versions are available :

  • One contains the change expected to be merged (named button); all change needs to be compatible with the non-sso version.
  • Second one set #sso as the default redirect url.

Issues

As mentioned in the previous PR one of the main issue is the inability for the organization invitation to work with the SSO redirection. To fix it a patch to the front-end is needed.

⚠️⚠️ ⚠️ If you have issues or need help testing the PR ⚠️ ⚠️ ⚠️

Please open issues in Timshel/vaultwarden in order to keep the discussion here focused on merging this work.
Of course if you believe your issue is important mention this PR so a reference will be visible.

But please try to keep commenting in this PR to a minimum to keep it legible, the previous one has over 200 comments ...

@derfabianpeter
Copy link

Super happy to see this PR being worked on. We (ayedo.de) would be willing to offer a sponsoring to prioritize this PR if that helps! Just reach out.

@Timshel Timshel force-pushed the sso-support branch 2 times, most recently from c86e481 to d5f78b4 Compare September 28, 2023 17:06
@Timshel
Copy link
Contributor Author

Timshel commented Sep 28, 2023

Just added a configuration example for Gitlab which might be one of easiest way to test this PR :).

@AkechiShiro
Copy link

AkechiShiro commented Sep 29, 2023

Hi @Timshel, thanks for your amazing and prolonged work on this feature, is this PR close to be in a ready merge-able state or is there a lot of work left?
I see the latest commit is about documentation, so, all issues mentioned at the beginning were fixed in some way or another ? Or there are still issue to fix ?

@Timshel
Copy link
Contributor Author

Timshel commented Sep 29, 2023

Mainly waiting for maintainer review/feedback now :).

@ruben-herold
Copy link

@Timshel thx for your work!!! Hope this will be integrated soon

@pellux-network
Copy link

Hoping this gets merged soon!

@AkechiShiro
Copy link

AkechiShiro commented Oct 4, 2023

Tagging some maintainers for review on this PR, if they have the available time resource to do so @BlackDex @dani-garcia

EDIT: I don't understand the thumbs-down, because tagging maintainers doesn't mean they have time to handle the PR or review it, it's just a way to mention them, if they don't answer/go MIA, or whatever, feel free to fork on this PR and maintain your own forks, no one is entitled to do any work, they don't want to.

@BlackDex
Copy link
Collaborator

BlackDex commented Oct 4, 2023

I do not have much time actually.

Also, I'm a bit puzzled with all the different SSO PR's.
And I am a bit hesitant to merge one if that for some reason could break the other or has a totally different way of working.
I'm not sure what to do here because i see people want something like this, but there are multiple ways of getting this working it looks like.

One way would be to create a semi-supported release branch which contains SSO support, but that could get messy keeping it up-to-date. What do you think @dani-garcia ?

@Timshel
Copy link
Contributor Author

Timshel commented Oct 4, 2023

? As mentioned this is the continuation of the previous PRs, it all rely on openidconnect. All of those PR are based on the previous ones when the previous PR owner stopped maintaining it.

I can´t speak for the owner of previous PRs but I believe this make all the others redundant. You could probably close the previous one referencing this one and encourage their owner to reopen if something is missing.

Thanks @bmunro-peralex for closing his PR to make things more legible and of course for his work which is present in this PR :).

@xoxys
Copy link
Contributor

xoxys commented Oct 4, 2023

Why not finally add at least one way to support OIDC? You can also flag it as preview feature or something like this to get feedback from the community, but not getting this feature into Vaultwarden after multiple PRs were provided by the community without a review or without getting merged for months until the authors then gave up feels wrong to me for an open source project.

@BlackDex
Copy link
Collaborator

BlackDex commented Oct 4, 2023

Why not finally add at least one way to support OIDC? You can also flag it as preview feature or something like this to get feedback from the community, but not getting this feature into Vaultwarden after multiple PRs were provided by the community without a review or without getting merged for months until the authors then gave up feels wrong to me for an open source project.

Well, because One way could be a different way then the others, or could cause a lot of other changes needed to be done if they do not match, or maybe even could overlap and do something totally different. 49 FIles are changed, so I'm not going to be happy if there needs to be major rework done because of adding this feature which is not fully working/supported.

You have to keep in mind that this could break other code in some way. But as said before, i do not have much time to check and validate this. And this is a huge PR and a lot of testing needs to be done, and i this is not specifically on my prio list for now actually. That is why i mentioned a special branch, which builds this version with a different tag and not fully supported in terms of issues with the login from my side.

@Timshel
Copy link
Contributor Author

Timshel commented Oct 4, 2023

Well, because One way could be a different way then the others, or could cause a lot of other changes needed to be done if they do not match, or maybe even could overlap and do something totally different.

@BlackDex I'll insist but there is no other way (At least not in the currently opened PRs). All those PR are based on the previous ones. They got more refined each time as someone picked-it up.

@tschuyebuhl
Copy link

is there any way one can help with testing? or anything that can be done to help get this merged?

@isaiah-v
Copy link

isaiah-v commented Oct 4, 2023

I've been watching the progress of this feature. I can't wait for it, but out of curiosity, how does decryption work with this feature? Is it still client side? How do you now decrypt without knowing the password?

@Timshel
Copy link
Contributor Author

Timshel commented Oct 4, 2023

@isaiah-v as mentioned a master password is still required. There is no change on this point.

@Timshel
Copy link
Contributor Author

Timshel commented Oct 6, 2023

@BlackDex thinking on it I don´t think the semi-supported branch is a good idea.

Main issue for people running this branch is that there might be some change in the migrations that might force to correct DB state manually. Even if it's not difficult (cf Timshel/vaultwarden#db-migration), integrating in a separate branch would not help with this.

Additionally unless you grant me commit rights it means that this would make it more complicated for me to support it and if you have no time for review I can't see how you would semi-support it.

It's important to note that the SSO_ENABLED config act as feature flag, the impact on the non sso version is quite low so merging this should have a low risk for the non sso users.

In the end if people are not running it at the moment it might be because they are waiting for an easier way to run this (but I made updates on main@Timshel/vaultwarden to make it easier) but I would expect it's mainly because they are waiting for it to be reviewed, a solution without any review would not be worth much ...

Since I'm running this myself I will maintain this branch/PR, and will continue to update main@Timshel/vaultwarden with anything I can think of to help people running it. As mentioned before if you have any question don't hesitate but please open it on Timshel/vaultwarden to prevent spamming here (of course mention this PR if you think your issue is important).

In my opinion the next step is for it to be reviewed and then integrated (maybe without being promoted at first).

@AkechiShiro
Copy link

I will definitely try to host the branch of your fork that contains sso-support and see if I run into any issues, I will report them on your repo @Timshel

@dandanthedev
Copy link

+1, please merge!

@griefie
Copy link

griefie commented Oct 10, 2023

It seems that there is a lot of hesitation on investing time into reviewing this and i can understand this. However - the longer the delay the bigger the diff guys. The branch clearly works and simply needs a bit more love. Besides it already looks like a lot of work went into this and the older preceding branches. Why not make it a beta build? Even 2.0.0-beta? The closer it is to the main stream, the quicker will be the feedback and the improvement. Let's not forget this is open source, where ideas thrive and not corporate where ideas die ;)

@derfabianpeter
Copy link

We're still happy to sponsor this PR if it helps

@Timshel
Copy link
Contributor Author

Timshel commented Oct 11, 2023

Rebased and added the @BlackDex suggestion in #3154 (comment) to make the SSO button visible when running the docker-compose.

@Timshel
Copy link
Contributor Author

Timshel commented Jan 9, 2025

Updated to include 2025.1.0 based web-vault.

@gjuuz
Copy link

gjuuz commented Jan 12, 2025

So I also need to enter the master password when signing in with this SSO?

@zeddD1abl0
Copy link

So I also need to enter the master password when signing in with this SSO?

Yep. Unless you have another mechanism, this is just another layer of security over the top of the Master Password.

@gjuuz
Copy link

gjuuz commented Jan 13, 2025

So I also need to enter the master password when signing in with this SSO?

Yep. Unless you have another mechanism, this is just another layer of security over the top of the Master Password.

Thanks but the i don't get the benefit of this. Security is always important but in case you provide secure authentication within the sso IDP the master password is not needed anymore to be honest.
Would be great if this is possible any day.

@bmunro-peralex
Copy link

I'll relook at the keyconnector stuff i did when i was working on this, though there is no point looking at it till this is merged

@rossigee
Copy link

Thanks but the i don't get the benefit of this.

The benefit is that SSO becomes a kind of 2FA, which is useful in the event that someone is somehow able to obtain your master passphrase.

For example, my SSO service is configured to require hardware keys. If I understand it correctly, this feature means I should now be able to configure VaultWarden to ensure that I have not only my master passphrase (needed to unlock the vault) but also another stronger form of authentication too.

A key connector would improve convenience, but likely lower overall security. Whether you choose to use one or not would depend on your risk appetite.

@gjuuz
Copy link

gjuuz commented Jan 13, 2025

I'll relook at the keyconnector stuff i did when i was working on this, though there is no point looking at it till this is merged

That would be great !

@gjuuz
Copy link

gjuuz commented Jan 13, 2025

kind of 2FA, which is useful in the event that someone is somehow able to obtain your master passphrase.

Yea but 2FA is also available within Vaultwarden itself so why implementing a IDP ?

Dont understand me wrong its definetly more secure using this extra layer but in larger organizations its not really helpful.

@nlseven
Copy link

nlseven commented Jan 13, 2025

kind of 2FA, which is useful in the event that someone is somehow able to obtain your master passphrase.

Yea but 2FA is also available within Vaultwarden itself so why implementing a IDP ?

Dont understand me wrong its definetly more secure using this extra layer but in larger organizations its not really helpful.

It simply means that instead of an org user needing three passwords (regular account used for corp services, Vaultwarden account password for syncing, and Vaultwarden master password) it brings it down to just the regular corp account and a master password.

Importantly this brings it closer to feature-parity with Bitwarden.

@zeddD1abl0
Copy link

Dont understand me wrong its definetly more secure using this extra layer but in larger organizations its not really helpful.

I'm not sure what level of "larger organisation" you're specifically talking about, but from my perspective, this gives me 3 controls I can add with relative ease:

  1. Enforce login for specific users from specific locations. IDP with country checks? Done.
  2. Enforce allowed groups. If the user isn't part of an allowed group their access is gone regardless of their Master Password
  3. User decommissioning. If the user can't authenticate to the IDP, they can't log in. Doesn't matter if they know their password or not.

I will admit, the full picture (IDP providing master password) is pretty awesome, but even in its current format, this will provide some very useful tooling for me.

@micolous
Copy link

Friendly reminder (again): there are 101 people watching this PR, and every comment emails everyone. The same questions and comments are coming up repeatedly, which is drowning out status updates.

This PR implements SSO the same way BitWarden does, with all the same functional constraints. If you have questions about how that works, you're better off asking BitWarden.

If you have questions or comments about this functionality as it relates to Vaultwarden, please open a discussion - it's makes it much easier to find your questions, and means people following this PR can focus on status updates.

If you have questions about how SSO works, contact your identity provider's support or sales engineering team.

Thank you. ❤️

@Timshel
Copy link
Contributor Author

Timshel commented Jan 15, 2025

@stefan0xC had another look at Membership::confirm_user_invitations and I'm a bit unsure why this in fact exists ?
I believe for admin invitation there is no membership in database and for organization invitation it should be handled in Organizations::confirm_invite ?
Edit: A yes it's only in use when email are disabled.
Additionally, it seems with the new OrganizationId change the guard raise an error for an invalid id such as _ used in the admin invitation ?

Different subject for those using Entra I don't remember if there were some trouble with the email_verified claim. Could you give some feedback/help in Timshel#85 (comment)

Copy link
Contributor

@stefan0xC stefan0xC left a comment

Choose a reason for hiding this comment

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

Additionally, it seems with the new OrganizationId change the guard raise an error for an invalid id such as _ used in the admin invitation ?

Ah, I was wondering about your code why you've disabled the guard.
Just tested it and the error is more pronounced (very ugly) but registering an account still seems to work. But yeah, we will have to check if we could make it less ugly. (Maybe by using a fake UUID instead of _)

src/api/core/organizations.rs Outdated Show resolved Hide resolved
src/api/core/organizations.rs Outdated Show resolved Hide resolved
src/db/models/organization.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.