SSO using OpenID Connect #3899
SSO using OpenID Connect #3899
Conversation
Timshel
force-pushed
the
sso-support
branch
2 times, most recently
from
e344cad to
ecbe5e3
Compare
Timshel
force-pushed
the
sso-support
branch
from
ecbe5e3 to
77fe2b3
Compare
|
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
force-pushed
the
sso-support
branch
2 times, most recently
from
c86e481 to
d5f78b4
Compare
|
Just added a configuration example for Gitlab which might be one of easiest way to test this PR :). |
|
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? |
|
Mainly waiting for maintainer review/feedback now :). |
|
@Timshel thx for your work!!! Hope this will be integrated soon |
|
Hoping this gets merged soon! |
|
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. |
|
I do not have much time actually. Also, I'm a bit puzzled with all the different SSO PR's. 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 ? |
|
? 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 :). |
|
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. |
@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. |
|
is there any way one can help with testing? or anything that can be done to help get this merged? |
|
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? |
|
@isaiah-v as mentioned a master password is still required. There is no change on this point. |
|
@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 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). |
|
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 |
|
+1, please merge! |
|
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 ;) |
|
We're still happy to sponsor this PR if it helps |
|
Rebased and added the @BlackDex suggestion in #3154 (comment) to make the |
@oe3gwu probably but it's not really the point of this PR. I believe OpenId Connect reached its goal and is well enough defined that a client implementation can expect to work with almost all providers without having to rely on an additional component to deploy. |
|
please please please make this real!! Im waiting this for so looong |
|
The real question is if it's actual SSO like with Bitwarden's Key Connector or SSO that still requires a master password like on @Timshel's forked repo |
sign-on and decryption should be independently keyed in my opinion, but maybe there could be another option to link them? in any case it should be possible to 'not trust' the SSO service. |
|
It's actually not about opinions but the way the Bitwarden web frontend handles decryption without having to maintain a separate fork – this has been brought up multiple times when people tried to implement / asked for SSO support previously; e.g. via LDAP Actually reading the very first comment of this PR it clearly states the master password will still be required – just in case someone was nagging for a merge because they just read the headline;
|
|
I'll add that when I created the issue I was not aware that the license of Bitwarden key connector is quite restrictive.
|
There was a problem hiding this comment.
Well, here's my review of this PR. While I have not found any glaring issues from reading the code I was wondering about some stuff. Some of which I think should be done differently (especially confirm_user_invitation)...
I have not looked at the playwright tests again. I think it's okay to merge them as is and improve them once they are merged.
| @@ -730,6 +732,17 @@ impl UserOrganization { | |||
| }} | |||
| } | |||
|
|
|||
| pub async fn confirm_user_invitations(user_uuid: &str, conn: &mut DbConn) -> EmptyResult { | |||
There was a problem hiding this comment.
I don't think this should be done via a database call.
There was a problem hiding this comment.
Wrote-it to avoid having to run two queries with:
for user_org in UserOrganization::find_invited_by_user(&user.uuid, &mut conn).await.iter_mut() {
user_org.status = UserOrgStatus::Accepted as i32;
user_org.save(&mut conn).await?;
}And it's now used in the sso login too when mail are disabled.
There was a problem hiding this comment.
Since it's used in two different place I would tend to keep the function.
Would you prefer that I rewrite the function with find/save ?
src/api/identity.rs
Outdated
| } | ||
| ) | ||
| } | ||
| Some((user, Some(sso_user))) if sso_user.identifier != user_infos.identifier => { |
There was a problem hiding this comment.
Isn't the returned sso_user found trough the users_infos.identifier so this should never be the case?
There was a problem hiding this comment.
We return the user matching either the email or the identifier.
If someone change his email but has not yet updated it in Vaultwarden then someone else could use the old email with the provider and try to usurp the Vaultwarden account.
Storing and using the identifier is part of the mitigation to prevent this sort of cve.
There was a problem hiding this comment.
Get User by email, get SsoUser by identifier. If the user_uuid matches or both are None everything is fine. If not there are three cases:
a) an SsoUser for the identifier does not yet exist <- depends whether we allow it (SSO_SIGNUPS_MATCH_EMAIL).
b) a User account for the mail does not yet exist (None) but an SsoUser exists (the user_uuid should point to a different existing User). <- User should change their mail back to match the Sso provided one.
c) a User account for the mail exists and SsoUser exists but the user_uuid points to a different User.
If I just compare by email and identifier I don't think I'd catch the third case, I'd get the User per mail and join it with the SsoUser by identifier? Your check would not tell me I've got the wrong User, I think.
There was a problem hiding this comment.
In the return type the <User, Option<SsoUser>> always share the same user_uuid the join is done in database.
If the union find a match on the email and on the identifier then only the identifier match is returned.
So then the 3 case left are:
- I have a user without any SSO association -> do we allow the association or not ?
- I have a user with an SSO association:
- If it matches the identifier: we found the correct user
- If the identifier does not match, then it means it's new user (identifier is not in db) but the email is already used so we will block login.
I think it's clear that what I wrote initially as a small optimization to make only one query has become way too complicated especially since it relies on part of the check being done in the db.
As such as I mentioned in the other comment I will rewrite this by always searching by identifier first and fetch by email only if it's missing.
Since it will be missing only during onboarding it means for most login it will have no impact and the code logic should be cleaner.
There was a problem hiding this comment.
Thanks for the explanation. I think I understand your optimization a bit but I still think you have a flaw in your logic: because if "only the identifier match is returned" and you want to differentiate between the cases you cannot check if the identifier is equal but if the email is equal, since the identifier match has been returned.
With a simplified database (where users only has uuid and email and sso_users has only user_uuid and identifier). If I have a user 1 with email A and associated Identifier X and a second user 2 with email B and no associated identifier, your union might return me the following:
1|A|1|X
2|B||
So I'd need to check if A==B to determine whether the User relation matches.
There was a problem hiding this comment.
Not that's really important (I mean since I will rewrite it ^^) but in your example if you query with :
- identifier X email A -> return
1|A|1|X-> login ok - identifier X email B -> return
1|A|1|X-> login ok (email sent to notify that email has changed). - identifier Y email A -> return
1|A|1|X-> login fail identifier mismatch - identifier Y email B -> 2|B|| -> login ok if association is allowed
There was a problem hiding this comment.
Removed the find_by_identifier_or_email it's a bit better.
src/api/core/accounts.rs
Outdated
| kdf_upgrade(&mut user, &data.master_password_hash, &mut conn).await?; | ||
|
|
||
| Ok(Json(json!({ | ||
| "MasterPasswordPolicy": {}, // Required for SSO login with mobile apps |
There was a problem hiding this comment.
Would it be okay to return the CONFIG.sso_master_password_policy() here?
There was a problem hiding this comment.
This would make the SSO_MASTER_PASSWORD_POLICY not specific to SSO, but I think it would make sense since the policy is global.
Would probably rename the setting to DEFAULT_MASTER_PASSWORD_POLICY, might open a separate PR for it.
There was a problem hiding this comment.
In my previous comment I made the supposition it would apply to login without sso since the method existed before this PR, but I'm not sure when it's called outside of the SSO flow.
Additionally, when testing it with SSO the enforceOnLogin part does not appear to work since the returned policy appeared to be ignored.
Still I decided to return the defined policy but only when the SSO is activated.
| pub trait AuthMethodScope { | ||
| fn scope_vec(&self) -> Vec<String>; | ||
| fn scope(&self) -> String; | ||
| fn check_scope(&self, scope: Option<&String>) -> ApiResult<String>; | ||
| } |
There was a problem hiding this comment.
Is there a need to implement this as a trait?
There was a problem hiding this comment.
It's mainly used in LoginJwtClaims::default to prevent having to define/pass the scope_vec in all the callers.
I don't know for a need, but I believe it makes for cleaner code.
|
@stefan0xC pushed some fixes (in separate commits for now). Should I close the conversion where we were in agreement, or should I let you do it ? ALTER TABLE users_organizations DROP COLUMN invited_by_email;The added column had a default value so even an unmodified Vaultwarden should run without issue but better to return to a clean state. For those running off other branches / distribution for now no change, I'll probably add a migration to remove it if needed. |
|
@stefan0xC made an additional modification around signups to disable it if the |
Co-authored-by: Pablo Ovelleiro Corral <[email protected]> Co-authored-by: Stuart Heap <[email protected]> Co-authored-by: Alex Moore <[email protected]> Co-authored-by: Brian Munro <[email protected]> Co-authored-by: Jacques B. <[email protected]>
|
@stefan0xC updated the playwright tests to work with For those wanting to test the web-vault in version Edit: Sorry if pinged you @BlackDex, but the PR status was mentioning some change requested that I couldn't really find (all I can find is marked as resolved). |
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:
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 :
button); all change needs to be compatible with the non-sso version.#ssoas 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.
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 ...