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

OIDC SSO #2449

Closed
wants to merge 1 commit into from
Closed

OIDC SSO #2449

wants to merge 1 commit into from

Conversation

pinpox
Copy link

@pinpox pinpox commented May 2, 2022

Continuing the PR by @Sheap #1955. Work in progress.

struct SsoOrganizationData {
// authority: Option<String>,
// clientId: Option<String>,
// clientSecret: Option<String>,
Copy link

Choose a reason for hiding this comment

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

Commented out properties. Can probably be removed, can't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Depends, if those items are sent by the web-client or other clients then having those as an Option would be a better choice.
If they are not needed at all, then removing them is the better option.

Copy link
Author

Choose a reason for hiding this comment

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

They were added in newer versions of the web-client. They are not used/needed anywhere, I assume they are for other kinds of SSO configuration. The older web-client had a lot less options and inputs than the current version.

@technowhizz
Copy link

@pinpox is this PR ready for review?

@pinpox
Copy link
Author

pinpox commented May 31, 2022

@pinpox is this PR ready for review?

I'm sure there is still stuff to do, but it would be great if you could could give me a first review, so I know where to continue on working.

@Kab1r
Copy link

Kab1r commented Jun 27, 2022

Does this implementation include support for key-connector or would that be a separate feature request / PR?
Are users still required to have a master password?

@pinpox
Copy link
Author

pinpox commented Jun 29, 2022

Does this implementation include support for key-connector or would that be a separate feature request / PR?

The key-connector is not part of this PR.

Are users still required to have a master password?

Yes, the password is used to encrypt the vault and still required.

@BlackDex BlackDex mentioned this pull request Jun 29, 2022
61 tasks
src/api/identity.rs Outdated Show resolved Hide resolved
@BlackDex
Copy link
Collaborator

First, sorry for not checking this earlier.

I did some quick checking. And i probably missed some important stuff to nag about 😉
Overall i think it looks nice. There are some comments here and there which need to be removed etc...
All the commits should be squashed into one i think, and it needs a re-base and probably a new clippy run.

The patch which is currently in this PR is probably also out-of-date.
If it is only needed to remove some CSS hide options, maybe a simple sed would be enough as a comment so that people can try?

What I would also like, is some small howto on how i can test this my self?
Maybe a simple docker-compose example to start and run a LDAP server with some users seeded to it, or something like that.
That would save a lot of work for me and @dani-garcia if we need to test this.

@williamdes
Copy link
Contributor

About LDAP servers, I am building a LDAP server with a test suite in full docker. Feel free to steal anything you may need: https://github.com/sudo-bot/docker-openldap
Else I recommend this docker image that is unmaintained: https://github.com/osixia/docker-openldap
Here you will find an example LDAP server for emails: https://github.com/datacenters-network/mails/blob/250432abe68c5b56f8c916f5dbffbfd68498db54/docker-compose.yml#L121-L183
Feel free to ping me for LDAP docker things, I will try to help if I can

@pReya pReya mentioned this pull request Jul 26, 2022
@pinpox
Copy link
Author

pinpox commented Jul 27, 2022

Hey, thanks for looking into it!

Overall i think it looks nice. There are some comments here and there which need to be removed etc...
All the commits should be squashed into one i think, and it needs a re-base and probably a new clippy run.

The PR is still marked as Draft for this reason, it needs still some cleanup before it can be merged but I'm trying to get it working first.

What I would also like, is some small howto on how i can test this my self?
I'm currently using a gitea instance to test this. This doesn't have anything to do with the git functinality, but gitea can act as OIDC provider and you can start an instance easily without much configuration. I tried keycloak aswell, but it's way more complicated as it supportes a ton of features we don't need here.

Here are the basic steps to test this, keep in mind it's not all working yet.

Start a gitea (e.g. with the docker instructions from their site) and add an application under `https://mygitea.tld/user/settings/applications/oauth2/1` like this

image

Login to vaultwarden (the normal way) and create a organization, set the identifier to the organization name you set before

image

Then, in the web-vault go to `Enterprise single sign-on" and login with the identifier you created

image

@pinpox
Copy link
Author

pinpox commented Jul 28, 2022

In case someone can help out or knows how to fix it, this is the current blocker: #1955 (comment)

EDIT:
#1955 (comment)

@gal
Copy link

gal commented Aug 2, 2022

Might be easier to use something made for SSO authentication - https://keycloak.org. Bit clunky but it works

@pinpox
Copy link
Author

pinpox commented Aug 2, 2022

Might be easier to use something made for SSO authentication - https://keycloak.org. Bit clunky but it works

I have tried keycloak but it is quite complex for the very few options we need to test this. It is for sure "the right tool" but I recommend testing with gitea just because it's simpler to set up in a minute.

@shreyasajj
Copy link

In case someone can help out or knows how to fix it, this is the current blocker: #1955 (comment)

EDIT: #1955 (comment)

Is this https://your.domain.com/sso/oidc-signin. I got it from https://bitwarden.com/help/oidc-azure/ https://bitwarden.com/help/oidc-okta/

@pinpox
Copy link
Author

pinpox commented Aug 8, 2022

I have got OIDC login working now! 🥳

I still need to look in to an issue with the front-end (Currently configuring it by setting the values in the DB manually) and some other minor issues.

In case someone can help out or knows how to fix it, this is the current blocker: #1955 (comment)
EDIT: #1955 (comment)

Is this https://your.domain.com/sso/oidc-signin. I got it from https://bitwarden.com/help/oidc-azure/ https://bitwarden.com/help/oidc-okta/

The current implementation indeed uses /#/sso for both paths, but I'm not sure we should leave it like that, because e.g. ADFS does not support anchors.

@shreyasajj
Copy link

Ah this the only thing left to make sure my mom can use it without her forgetting her password. Just curious how far are you?

@BlackDex
Copy link
Collaborator

Ah this the only thing left to make sure my mom can use it without her forgetting her password. Just curious how far are you?

This will not prevent forgetting a password, since you still need a master password which can be the same as the linked sso of course, but that will not be secure of course.

@shreyasajj
Copy link

Wait it will be an extra layer. So you will need oidc then login with master password then you will access your vault

@Kab1r
Copy link

Kab1r commented Aug 17, 2022

That's what key-connector is for.

@pinpox
Copy link
Author

pinpox commented Aug 18, 2022

Just curious how far are you?

This branch is already functional in the backend, you can login with SSO already. What is missing is fixing the front-end part, currently you have to configure it on the DB itself by just setting the values in sso_config. For some reason I have not found out yet, the frontend does not show the configured values, you can set new ones though, even though they are not shown. The actual OIDC authentication is working. If someone wants to help out with the front-end part, let me know.

Wait it will be an extra layer. So you will need oidc then login with master password then you will access your vault

That is correct. The master password is used to derive the encryption key for your vault, so it needs to be provided either by you or by the key-connector.

That's what key-connector is for.

The connector isn't part of this PR and I have not looked into it yet. Is that something we can even support? Is that a separate piece of software or is it part of the backend?

@pReya
Copy link

pReya commented Aug 18, 2022

The connector isn't part of this PR and I have not looked into it yet. Is that something we can even support? Is that a separate piece of software or is it part of the backend?

The key connector is described here: https://bitwarden.com/help/about-key-connector/
Repository for the key connector can be found here: https://github.com/bitwarden/key-connector

@BlackDex
Copy link
Collaborator

Just curious how far are you?

This branch is already functional in the backend, you can login with SSO already. What is missing is fixing the front-end part, currently you have to configure it on the DB itself by just setting the values in sso_config. For some reason I have not found out yet, the frontend does not show the configured values, you can set new ones though, even though they are not shown. The actual OIDC authentication is working. If someone wants to help out with the front-end part, let me know.

A quick look at the code (without having it compiled etc..) i think that the response for the get_organization_sso function is incorrect.

It should look like this, not sure if that is what you currently get, or that it is wrapped into another layer.

{
    "enabled": true,
    "data":
    {
        "configType": 2,
        "keyConnectorEnabled": false,
        "keyConnectorUrl": "",
        "...CUT...": "...CUT..."
    },
    "urls":
    {
        "callbackPath": "https://vw.domain.tld/sso/oidc-signin",
        "signedOutCallbackPath": "https://vw.domain.tld/sso/oidc-signedout",
        "spEntityId": "https://vw.domain.tld/sso/saml2",
        "spMetadataUrl": "https://vw.domain.tld/sso/saml2/{org_uuid}",
        "spAcsUrl": "https://vw.domain.tld/sso/saml2/{org_uuid}Acs"
    },
    "object": "organizationSso"
}

@hellerbarde
Copy link

Thank you to everyone working on this.

Currently, src/db/models/organization.rs has some code in it that probably does not belong there. @pinpox 's other branch squash-rebase-oidc looks better.

Is there something we can do to help with this? Can we test something? Thanks and cheers!

@gianklug
Copy link

Bump

@pinpox
Copy link
Author

pinpox commented Nov 16, 2022

Hey, sorry have been a bit busy lately.

Is there something we can do to help with this? Can we test something? Thanks and cheers!

If anyone with rust experience wants to help, I'm happy to integrate pull requests on my branch and consolidate the work in some form we can get merged.

@KramNamez
Copy link

I'm trying to understand what's still necessary.

Do I understand right that basically on the backend the squash-rebase-oidc branch is ready to go, but what's required is applying web-vault-sso.patch to the web-vault repo, making sure that it allows for all the necessary configuration to be possible and then opening a PR against that, too?

@olsn
Copy link

olsn commented Dec 7, 2022

I'm trying to understand what's still necessary.

Do I understand right that basically on the backend the squash-rebase-oidc branch is ready to go, but what's required is applying web-vault-sso.patch to the web-vault repo, making sure that it allows for all the necessary configuration to be possible and then opening a PR against that, too?

I've had a look at this and it looks like that web-vault-sso.patch is very outdated. From what I have seen, the sources of the web-client, that contain the sso-configuration-pages are not licensed under AGPL, but under the Bitwarden License, which basically prevents anyone from using/copying them. So the Todo here would be for someone to code a custom SSO-configuration page for the official bitwarden web-client. (But please correct me, if i'm wrong here!)
It's not a huge task, but definitely more than just a few true/false-tweaks in the web-client.

Backend-wise, from what I've seen there also seem to be some open ends:

  • adjust current backend-changes to the use of diesel2
  • the oicd-callback-paths are currently hardcoded to localhost, that would need to be adjusted to reflect the proper host+path (which I have not found out yet)

@KramNamez
Copy link

Thanks for taking a look too and I agree. Unfortunately, as far as I can tell, this entire branch is rather behind the main branch now, since this has been dragging on for so long.

At this point, I'm wondering if it might be more efficient to make a new branch off of main and try and port Pinpox' work into that and at the same time prepare the changes necessary to the web-vault. Unfortunately, as I'm not experienced with Rust, it seems that the work required here is above my level.

Pinpox has prepared and given me a state of the code that runs and looked relatively clean (if based on old VW code), so I'm sure that they're willing to help anyone else getting a running state to work off of.

@pr0ton11
Copy link

Bump, would also be interested in this to work.
Maybe as a suggestion, instead of keycloak which is bulky and complicated, there is also another OSS-project called authentik, which could make testing a bit easier

@maaft
Copy link

maaft commented Dec 24, 2022

+1 for authentik.

Although I think that SSO should definitely not be implemented in a way that requires a specific SSO provider

@Avsynthe
Copy link

Another +1 for Authentik.

Moved from FreeIPA + Keycloak/Authelia for this as they just don't keep up anymore and Authentik will be big eventually as it just evolved into a proper company with staff a few months back.

However I don't believe that by implementing OIDC or SAML we will be locking anyone into a specific platform

@KramNamez
Copy link

KramNamez commented Dec 28, 2022

The problem isn't the SSO instance. Keycloak, Authentik, it's all the same in the end. If you find it easier to work with, that's nice and if that lets you help with the code, awesome!

But the issue is really just that currently the code of Pinpox' branch has diverged from main, and someone who understands Rust well enough needs to take Pinpox' work and rebase or port it onto main. On top of that, someone would need to untangle the webvault and add all the necessary changes into that to both allow logging in with SSO as well as to allow configuring all of this without having to manually edit the database.

@billyjbryant
Copy link

I would love to test this out, I use JumpCloud + AWS Identity Center for SSO. Is there a ready made docker image or do I need to burn my own?

@BlackDex
Copy link
Collaborator

I would love to test this out, I use JumpCloud + AWS Identity Center for SSO. Is there a ready made docker image or do I need to burn my own?

Well, this current PR isn't up-to-date with the present code, and it seems to have some conflicts.
But if those are solved, burning a custom image shouldn't be that hard.

@pr0ton11
Copy link

Would be up to sponsor this feature, if someone needs incentive to implement

@matbgn
Copy link

matbgn commented Jan 14, 2023

I would also be happy to contribute in the area of sponsorship. ;-)

@senbax-admin
Copy link

I would also provide some sponsorship :)

@Preisschild
Copy link

Really hope the Vaultwarden SSO implementation is generic so it can be used with any provider. We use Keycloak.

Yes, of course it is. OIDC is a standard. The discussion originally was just how to test this easily. Stop recommending auth providers, this is completely off topic for this.

@cvalka4
Copy link

cvalka4 commented Mar 16, 2023

I find it extremely strange that a password manager for teams does not support SSO.

@modzilla99
Copy link

@pinpox do you still work on this MR? I'd like to help, but as my knowledge of Rust is very limited, I'm afraid I could only help financially or by testing.

@pinpox
Copy link
Author

pinpox commented Mar 20, 2023

@pinpox do you still work on this MR? I'd like to help, but as my knowledge of Rust is very limited, I'm afraid I could only help financially or by testing.

The work is being continued here: #3154

@BlackDex
Copy link
Collaborator

@pinpox Maybe close this PR then?

@modzilla99
Copy link

modzilla99 commented Mar 20, 2023

@pinpox do you still work on this MR? I'd like to help, but as my knowledge of Rust is very limited, I'm afraid I could only help financially or by testing.

The work is being continued here: #3154

Ups, all those MRs got me confused, thanks and sorry for bothering you!

@Blackclaws
Copy link

Blackclaws commented Nov 9, 2023

@pinpox Maybe close this PR then?

@BlackDex Since @pinpox itself has pointed to another PR which again has been superseeded by #3899 maybe close this PR if @pinpox doesn't do it? Would certainly help to clean up the confusion about too many SSO merge requests that are basically just building off of one another :)

@BlackDex
Copy link
Collaborator

Closing this PR in favor of #3899 to keep overview of it all.

@BlackDex BlackDex closed this Dec 19, 2023
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.