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 Support based off existing PR's #3154

Closed
wants to merge 1 commit into from

Conversation

bmunro-peralex
Copy link

@bmunro-peralex bmunro-peralex commented Jan 19, 2023

Based off previous work by @pinpox and @m4w0lf
#2787
#2449

All config is now done in the environment variables, removed all unneeded calls.
Bitwarden removed the identify payload from the client so the first organization is always used when using a domain_hint

Currently Working:

  • Login from all web clients using sso
  • Creating MasterPassword on new SSO Login when no user exists.

Not Working:

  • Joining Organization link never fires accept so user never accepts invite during SSO login, normal login after the first SSO login that creates the account works
    The above has a workaround that can be enabled to accept all invites on login

How to test:
Add the following environment variables and have at least one organization created in your instance

`
SSO_ENABLED: "true"

SSO_CLIENT_ID: "111111111111111111111111111111111"

SSO_CLIENT_SECRET: "222222222222222222222222222222222222222222222"

SSO_AUTHORITY: "https://auth.example.com"

//Optional
SSO_ACCEPTALL_INVITES: "true"
`

The callback url currently is always:
Replace example.com with your vaultwarden domain.
https://example.com/identity/connect/oidc-signin

@sirux88
Copy link
Contributor

sirux88 commented Jan 19, 2023

Not Working:

  • Joining Organization link never fires accept so user never accepts invite during SSO login, normal login after the first SSO login that creates the account works

Is mailing enabled within your delevoping environment? If not, the necessary accept ( #[post("/organizations/<org_id>/users/<_org_user_id>/accept", data = "<data>")]) enpoint is never fired because accepting a new user is done wihin backend not by user frontend.

To test it you have to enable proper mailing

@Skiepp
Copy link

Skiepp commented Jan 20, 2023

Hi @bmunro-peralex, thanks a lot!! I can confirm it's working on Docker and Keycloak splendidly 👍

Do you think it will be possible to skip the "SSO Identifier" page?
(the form that pops up on #sso)

Ot is there a direct link to access SSO without input the organization first?

Thanks

@Timshel
Copy link
Contributor

Timshel commented Jan 20, 2023

Hey

Funny timing was working on it too, with the same idea of setting the SSO as a global conf.
Was not as advanced since had just rebased and made some cleanup to obtain a working login,

But I was more aggressive in deleting organization change (for example kept only the nonce in the sso_nonce table), I just integrated most of your change to test it :
You can see the diff here : https://github.com/Timshel/vaultwarden/compare/e84ca90..dd6947c (branch diff was wrong so used commit)
Only issue is when creating an account master password an error is displayed but can be ignored (the auto enroll call) and after submitting it returns to the default login page but navigating back to the sso login will then work.
Not pretty but I think maybe with some small change on the client (not sure on the policy around it) it should be possible to obtain something cleaner with no dependency to an organization.
Since vaultwarden is more for small scale deployment I believe it more logical. As a long term goal I would more want the Organizations to be created from the information returned by the sso and user association done in the backend.

For the sso I' m using keycloak was unable to make the discover_async work (not .well-known/openid-configuration ? ) so just end-up creating the metadata manually.

@Skiepp
Copy link

Skiepp commented Jan 20, 2023

Hey

Funny timing was working on it too, with the same idea of setting the SSO as a global conf. Was not as advanced since had just rebased and made some cleanup to obtain a working login,

But I was more aggressive in deleting organization change (for example kept only the nonce in the sso_nonce table), I just integrated most of your change to test it : You can see the diff here : https://github.com/Timshel/vaultwarden/compare/e84ca90..dd6947c (branch diff was wrong so used commit) Only issue is when creating an account master password an error is displayed but can be ignored (the auto enroll call) and after submitting it returns to the default login page but navigating back to the sso login will then work. Not pretty but I think maybe with some small change on the client (not sure on the policy around it) it should be possible to obtain something cleaner with no dependency to an organization. Since vaultwarden is more for small scale deployment I believe it more logical. As a long term goal I would more want the Organizations to be created from the information returned by the sso and user association done in the backend.

For the sso I' m using keycloak was unable to make the discover_async work (not .well-known/openid-configuration ? ) so just end-up creating the metadata manually.

Maybe it will help, the keycloak endpoint (SSO_AUTHORITY variable) it the "issuer" field in the well-known configuration

Also, I agree that it would be great to import orgs membership based on a SSO token claim (easily mapped to an LDAP attribute), it should go hand-in-hand with the skip of "SSO Identifier" page

Thanks again for the work 👍

@Timshel
Copy link
Contributor

Timshel commented Jan 20, 2023

I though I had tested it :D. thx @Skiepp.
Well anyway reverted the manual config.
So a cleaner diff in case : https://github.com/Timshel/vaultwarden/compare/e84ca90..3705df5

@LePresidente
Copy link

LePresidente commented Jan 21, 2023

On phone so this will be brief, also forgot I'm on my personal github account not my work one so excuse the name change

My main goal here was to remove the need for any patches to the web-vault front-end like previous PRS and just work with both web + client

The sso identifier page that saves the identifier is used by alot of calls during the sso process so I'm pretty sure it can be hidden by a web client patch but will break alot of the call process

This approach will mean the first org policies are the master for the sso as you can put anything into the identifier and the first org will be used.

I have no idea what's up with the org invite process though. Accept is never called from the front-end after the login process using the sso so I guess since the sso is linked to an org so the invite just gets accepted by the server since it should be already linked.

@potatoattack
Copy link

Hi

I'm trying to test this using Azure as the IdP and I can't get it to work.

I get Unable to find client from identifier in the browser and the following error in the syslog:

[2023-01-23 13:40:02.875][request][INFO] GET /identity/account/prevalidate?domainHint=vaultwarden
[2023-01-23 13:40:02.876][response][INFO] (prevalidate) GET /identity/account/prevalidate?<domainHint> => 200 OK
[2023-01-23 13:40:02.890][request][INFO] GET /identity/connect/authorize?client_id=web&redirect_uri=htt
[2023-01-23 13:40:03.020][vaultwarden::api::identity][ERROR] Unable to find client from identifier {}. Failed to discover OpenID provider
[2023-01-23 13:40:03.020][response][INFO] (authorize) GET /identity/connect/authorize?<redirect_uri>&<domain_hint>&<state> => 400 Bad Request

I have followed the bitwarden oidc docs for the azure config and the relevant vaultwarden config looks like this:

SSO_ENABLED=true
SSO_CLIENT_ID=<APPLICATION_ID>
SSO_CLIENT_SECRET=<CLIENT_SECRET>
SSO_AUTHORITY=https://login.microsoft.com/<TENANT_ID>/v2.0

I have tried the sso_authority with and without a trailing slash.

Anyone see what I'm doing wrong?

Thanks

@LePresidente
Copy link

Hi

I'm trying to test this using Azure as the IdP and I can't get it to work.

I get Unable to find client from identifier in the browser and the following error in the syslog:

[2023-01-23 13:40:02.875][request][INFO] GET /identity/account/prevalidate?domainHint=vaultwarden
[2023-01-23 13:40:02.876][response][INFO] (prevalidate) GET /identity/account/prevalidate?<domainHint> => 200 OK
[2023-01-23 13:40:02.890][request][INFO] GET /identity/connect/authorize?client_id=web&redirect_uri=htt
[2023-01-23 13:40:03.020][vaultwarden::api::identity][ERROR] Unable to find client from identifier {}. Failed to discover OpenID provider
[2023-01-23 13:40:03.020][response][INFO] (authorize) GET /identity/connect/authorize?<redirect_uri>&<domain_hint>&<state> => 400 Bad Request

I have followed the bitwarden oidc docs for the azure config and the relevant vaultwarden config looks like this:

SSO_ENABLED=true
SSO_CLIENT_ID=<APPLICATION_ID>
SSO_CLIENT_SECRET=<CLIENT_SECRET>
SSO_AUTHORITY=https://login.microsoft.com/<TENANT_ID>/v2.0

I have tried the sso_authority with and without a trailing slash.

Anyone see what I'm doing wrong?

Thanks

bitwarden documentation looks wrong, just tested with azure and it looks to be working.

the openid client uses the OpenID Connect metadata document so you need to put the base path minus .well-known/openid-configuration

So In azure click endpoints on the overview page you should see a list of end points, find the metadata document

will look something like this

https://login.microsoftonline.com/<TENANT_ID>/v2.0/.well-known/openid-configuration

Use in your compose file

SSO_AUTHORITY=https://login.microsoftonline.com/<TENANT_ID>/v2.0/

@bmunro-peralex bmunro-peralex changed the title [WIP] Sso Support based off existing PR's Sso Support based off existing PR's Jan 23, 2023
@bmunro-peralex
Copy link
Author

This is working, and i don't have time to look at this anymore.

The organization invite can probably be handled the same as registration, or for now accepting org invites just login with a master password to accept.

@ghost
Copy link

ghost commented Jan 23, 2023

@BlackDex any chance that this great work from @bmunro-peralex can be merged into the main branch?

@henobi
Copy link

henobi commented Jan 23, 2023

That would be awesome 👌
Great work!!

@Timshel
Copy link
Contributor

Timshel commented Jan 26, 2023

Had a look at trying to obtain a cleaner flow when registering with sso.
Was able to obtain something not too bad (Timshel/bitwarden_clients@5ceee91).
But it's still not optimum since afterward if you refresh the page and logout it will send you back to /#/login.

One option might be to just build a separate release of the front-end and modify the default redirection.

@LePresidente
Copy link

I wonder if it makes sense to move this to its own end point to not clutter the other code, so bitwarden.example.com/sso instead of reusing identity/connect/

@Timshel
Copy link
Contributor

Timshel commented Jan 27, 2023

Hey

Just tested the different flow and wanted to ensure I did not missed anything.
Run your latest version with a fresh DB sso, mail and SSO_ACCEPTALL_INVITES to true and frontend built with only a custom change to make the sso button visible (https://github.com/dani-garcia/bw_web_builds/blob/cffdfb06f9e1faa29f1aec33759b18b35d63bf8a/patches/v2023.1.1.patch#L344-L345)

Initial setup :

  • Need to create an account probably using either the invite from the admin panel since registering will be usually disabled.
  • Create an Organization

If those steps are not done the sso flow will crash on the /identity/connect/authorize endpoint and return raw json.

User account creation:

If user follow a link to /#/sso will start at step 3.

  1. user enter email then click continue (need to ignore the New around here? Create account at the bottom)
  2. need to ignore all option and click on Enterprise single sign-on
  3. enter an identifier (can be prefilled especially when using a direct link, will persist)
  4. Execute the external SSO flow (if already logged invisible)
  5. Back to the set password screen
    . A modal with "An error has occurred" is displayed for a short while (401 on invited user did I mess something up ?)
    . User input information
  6. Redirection to the login screen
    . a warning is visible for a short while : Logged Out / your session has expired
    . email is prefilled if user started on step 1 otherwise need to fill it
    . click continue
  7. . user can enter master password at this step
    . or ignore all option again and click on Enterprise single sign-on
  8. identifier page should be prefilled
  9. External sso flow should be invisible
  10. Enter master password to unlock

User account login:
If user follow a link to /#/sso will start at step 3.

  1. email is probably prefilled just need to click next
  2. . user can enter master password at this step
    . or click on Enterprise single sign-on
  3. identifier should be prefilled
  4. Execute the external SSO flow (should be invisible)
  5. Enter master password to unlock

On page refresh:
User end up on the lock page can just input master password.

On logout
User end up on the login page even if he used a link to directly arrive to sso.

Interrogations

Error on the set password screen, did I messed something up in my setup ?
On the require sso part I was not able to activate it.

Comments

Won't comment too much in case I messed something up but in general from a friends/familly setup point of view I find the flows to be quite long/complicated.
With some light front-end modifications the identifier part could be hidden but you mentioned that is was not part of your goal. Anyway it won't fix what I perceive as the main pain point : the default /#/login redirection for which the only solution I could find would mean a specific SSO front end build but I was told and completely understand is not an acceptable solution.

@bmunro-peralex
Copy link
Author

User account creation:

Point 5 the 401 error is the web client going to bitwarden.example.com/organizations/<org_id>/policies/invited-user?<_userId> with no auth header, I don't know why or if it's by design but vaultwarden blocks this request in the auth routine.

the return to login screen I'm not sure, possibly the set-password call is not returning everything, or its due to the sstamp reset.

The requiresso section I never tested and is from the existing PR's above.

SSO, in general, the flows will be long and over-complicated, there is the key connector to skip passwords but that is a whole nother process that requires SSO working before it's looked at which will reduce the complexity.

as i said I'm changing nothing bitwarden was doing already and this is pretty much their process where the password is always required even if SSO login, unless a seperate key-connector is configured to automate that part to a degree.

@Timshel
Copy link
Contributor

Timshel commented Jan 27, 2023

On the point 5 I insisted to ensure I was not wrong because if there if no solution to this error I think it would be better to just fail earlier by for example not even implementing things like get_auto_enroll_status or list_policies_invited_user.

This would allow to drop the requirement to have existing organization (could end up something like this).

For the return to the login screen after the set-password it's tricky I had a quick look and could not find a backend solution. I could find some front-end modifications which were still compatible with a non sso instance but :(.

For the key-connector part I don't see it as a solution since I view having a separate master password more as a feature which ensure that even an admin in-personification at the sso level cannot access someone vault (but no idea if key-connector has some protection against this).

@LePresidente
Copy link

On the point 5 I insisted to ensure I was not wrong because if there if no solution to this error I think it would be better to just fail earlier by for example not even implementing things like get_auto_enroll_status or list_policies_invited_user.

This would allow to drop the requirement to have existing organization (could end up something like this).

For the return to the login screen after the set-password it's tricky I had a quick look and could not find a backend solution. I could find some front-end modifications which were still compatible with a non sso instance but :(.

For the key-connector part I don't see it as a solution since I view having a separate master password more as a feature which ensure that even an admin in-personification at the sso level cannot access someone vault (but no idea if key-connector has some protection against this).

OK fixed both issues above, the policy stuff will probably rip out for now since it won't work with the current setup and probably will need to be done by global config or something.

@Timshel
Copy link
Contributor

Timshel commented Jan 28, 2023

Hey nice :).

Sorry will insist one last time ^^, it's possible to make the SSO logic work with no modification to the web-vault code while not requiring to create any org. I think it's better since it remove some setup, is more inline with the configuration being global and I don't believe the target is to ever integrate it at the org level as is done by the official server.
Anyway will open a PR against your branch, if you are unconvinced just close it ;).

For the front even if with the latest change the flow is quite nice I still made a custom web-vault which redirect to the sso page by default and remove the identifier field from it, if someone want to play with it : https://github.com/Timshel/oidc_web_builds/releases/tag/2023-01-28

@Timshel
Copy link
Contributor

Timshel commented Feb 1, 2023

@LePresidente hey sorry if you are just busy but in case you missed it, I opened two PR against your branch :

  • First one fix the build issues
  • Second one include the first, some fixes and additionally update the logic to use a setting to control forcing sso login.

@mvalois
Copy link
Contributor

mvalois commented Feb 7, 2023

Thank you for your contribution. I'm looking for writing the code that handles the SSO settings for an organisation in the webvault itself. Should the SSO be organisation-tied?

@BlackDex
Copy link
Collaborator

BlackDex commented Feb 7, 2023

Thank you for your contribution. I'm looking for writing the code that handles the SSO settings for an organisation in the webvault itself. Should the SSO be organisation-tied?

There already is a UI available, built-in into the web-vault.

@mvalois
Copy link
Contributor

mvalois commented Feb 7, 2023

Thank you for your contribution. I'm looking for writing the code that handles the SSO settings for an organisation in the webvault itself. Should the SSO be organisation-tied?

There already is a UI available, built-in into the web-vault.

yes, but whenever I click on it, I am redirected to my personal vault. Where is it handled?

@BlackDex
Copy link
Collaborator

BlackDex commented Feb 7, 2023

Thank you for your contribution. I'm looking for writing the code that handles the SSO settings for an organisation in the webvault itself. Should the SSO be organisation-tied?

There already is a UI available, built-in into the web-vault.

yes, but whenever I click on it, I am redirected to my personal vault. Where is it handled?

Looks like it calls a subscription endpoint. Maybe we are missing a correct org flag or something?

@mvalois
Copy link
Contributor

mvalois commented Feb 7, 2023

Looks like it calls a subscription endpoint. Maybe we are missing a correct org flag or something?

I see no API call from my web browser, is that normal?

@bmunro-peralex
Copy link
Author

The per org config doesn't work, its also being moved to the business portal so it should be hidden. This config above makes your whole instance support SSO using environment variables instead.

@mvalois
Copy link
Contributor

mvalois commented Feb 7, 2023

The per org config doesn't work, its also being moved to the business portal so it should be hidden. This config above makes your whole instance support SSO using environment variables instead.

Yeah, but I think we should mimic the way bitwarden does it. If it is organisation-wide we should keep it as it is. Do you confirm that on the bitwarden-side it is removed from the org and became a paid feature?

@BlackDex
Copy link
Collaborator

BlackDex commented Feb 7, 2023

There is no Business Console anymore. But there is a Bitwarden Licensed web-vault part, that isn't part of the OSS build.
Therefore those panels are not built in into our version.

Though, it looks like in master they are moved out of the bitwarden-licensed folder, in rc they are still in there.

@mvalois
Copy link
Contributor

mvalois commented Feb 7, 2023

Okay so for VW we don't need to put any web interface to configure SSO settings? Should I put them in the .env file and patch VW to handle them?

@LePresidente
Copy link

bitwarden/clients@cf972e7

so 18 hours this was done, i'll have a look at going per org again if they opening it up, but when I worked on the above the org settings panel did nothing.

@archef2000
Copy link

So i got the same result: it makes my whole system unresponsive to anything and i need to power cycle it.

@tribut
Copy link

tribut commented Sep 8, 2023

So i got the same result: it makes my whole system unresponsive to anything and i need to power cycle it.

You are likely running out of RAM.

@archef2000
Copy link

archef2000 commented Sep 8, 2023

How muth does it need to build it ? More than 6gb?
And is there a prebuild one?

@OckhamOdyssey
Copy link

Please focus this channel for the SSO topic. If you have RAM problems or problems building a custom image it is not the responsibility of this channel.

@archef2000
Copy link

I am trying to test the sso but can't get it to work

@avicoder
Copy link

avicoder commented Sep 9, 2023

@Timon321 , can we have dockerfile for deployment.

@archef2000
Copy link

I found an error while testing if using Authelia instead of Keycloak as it removes the trailing slash at the end of SSO_AUTHORITY=https://auth.domain.tld/ so you then only have https://auth.domain.tld and then the following error popsup when pressing SSO login in the web vault:

{"ErrorModel":{"Message":"Failed to discover OpenID provider: Validation error: unexpected issuer URI `https://auth.ntin.me` (expected `https://auth.ntin.me/`)","Object":"error"},"ExceptionMessage":null,"ExceptionStackTrace":null,"InnerExceptionMessage":null,"Message":"Failed to discover OpenID provider: Validation error: unexpected issuer URI `https://auth.ntin.me` (expected `https://auth.ntin.me/`)","Object":"error","ValidationErrors":{"":["Failed to discover OpenID provider: Validation error: unexpected issuer URI `https://auth.ntin.me` (expected `https://auth.ntin.me/`)"]},"error":"","error_description":""}

@Timshel
Copy link
Contributor

Timshel commented Sep 9, 2023

@archef2000 If I understand correctly it mean that setting SSO_AUTHORITY=https://auth.domain.tld should work ?
Might add something to remove the trailing slash to make it more robust.

@archef2000
Copy link

Yes that works it is just to strict with the trailing slash

@archef2000
Copy link

Also i needed to set the redirect url in Authelia to https://vault.domain.tld/identity/connect/oidc-signin

@Timshel
Copy link
Contributor

Timshel commented Sep 11, 2023

Spent some time on the issue of the organization invitation not working when using sso login.

From my understanding the org invite and url are kept in memory and lost during the external redirection of the sso flow (No idea how/if it works with the official server or how it could be fixed with only a back-end change).

Managed to make a front-end patch to fix it. Front-end build with it are available at timshel/oidc_web_builds.

Added :

@Timshel
Copy link
Contributor

Timshel commented Sep 14, 2023

Pushed to timshel/sso-support some improvement when using SSO_ACCEPTALL_INVITES :

  • Send a different email (without the accept link) to notify the user that he has been enrolled in an org.
  • When auto-accepting send either send_invite_accepted or send_invite_confirmed

Added invited_by_email to UserOrganization to keep track of it (it's in the JWT token of the accept url otherwise).
Though it made sense to keep the information even outside of this use case. It's in its own commit.

@tribut would be nice to integrate the latest change to the PR :); and since I'm not aware of any issue it might be ready for some feedback/review.

@gianlucapisati
Copy link

@Timshel (and whoever may be interested)

I was thinking about how to best manage frontend patching to enable SSO.
Do you think it would make sense to have two different pipelines or two different Docker files?

  • The first one that build the official assets.
  • The second one that, in addition to building the official assets, also applies the frontend patch.

In this way we don't have to mantain the frontend on a different repository but we will still have the official build with the official webvault.

What do you think?

@Timshel
Copy link
Contributor

Timshel commented Sep 15, 2023

Will depends on what the maintainers wants :) (@BlackDex ?). From what I remember/extrapolated from previous discussion :

  • They probably don't want two distributions, so any change not compatible with the non sso version I would not expect to be merged.
  • There is already a patch applied on the official source but it's limited to assets and styles. I expect they might not want any change to any Typescript : harder to maintain and introduce more security risk (Main reason why I kept SSO_ACCEPTALL_INVITES around).

@davidroler
Copy link

Hi there,

I've build the image using the branch called sso-support and added the env variables to enable SSO support, but I can't see the SSO button at the login page.

I can see the new version has been installed (Web vault) Version 2023.7.1.

Is there anything else that needs to be in order to get SSO support?

Thanks!

@Timshel
Copy link
Contributor

Timshel commented Sep 15, 2023

@davidroler you are running an unpatched front-end so on the screen asking for the Master password you will need to make the sso button visible with :

document.querySelector('a[routerlink="/sso"]').style.setProperty("display", "inline-block", "important");

If you want a version with a modified Dockerfile using a patched front-end you can have a look at : https://github.com/Timshel/vaultwarden/tree/main.

@gianlucapisati
Copy link

Will depends on what the maintainers wants :) (@BlackDex ?). From what I remember/extrapolated from previous discussion :

  • They probably don't want two distributions, so any change not compatible with the non sso version I would not expect to be merged.

As far I've understood, in the official Bitwarden release there's a licensed web-vault, that isn't part of the OSS build. Which means, if I'm not wrong, that they also manage this feature with two different distributions.

My point is that, from a user perspective, it makes little sense to merge this pull request into the main repository if you still need to dig into the code to enable SSO.

It's obviously only my point of view and I just want to make it easier to enjoy your hard work :)

@Timshel
Copy link
Contributor

Timshel commented Sep 15, 2023

My point is that, from a user perspective, it makes little sense to merge this pull request into the main repository if you still need to dig into the code to enable SSO.

@gianlucapisati I'm not sure which "code" you are referencing ?

With just the minimum patch to revert hiding the sso button (which is compatible with the non-sso version) there is the issue of the organization invitation link not working but that's why there is the SSO_ACCEPTALL_INVITES config. Outside of it I'm unaware of any other issue.

@davidroler
Copy link

@davidroler you are running an unpatched front-end so on the screen asking for the Master password you will need to make the sso button visible with :

document.querySelector('a[routerlink="/sso"]').style.setProperty("display", "inline-block", "important");

If you want a version with a modified Dockerfile using a patched front-end you can have a look at : https://github.com/Timshel/vaultwarden/tree/main.

I got it to work using your repo, thanks!

Is it possible to use Google Workspace as SSO provider instead of Azure? I don't see the option to use a certificate instead a client secret.

@gianlucapisati
Copy link

@gianlucapisati I'm not sure which "code" you are referencing ?

With just the minimum patch to revert hiding the sso button (which is compatible with the non-sso version) there is the issue of the organization invitation link not working but that's why there is the SSO_ACCEPTALL_INVITES config. Outside of it I'm unaware of any other issue.

With "digging into the code" I mean that if we merge the pull request as is (which means that the functionality is completed, working and tested) you still need to apply the patch to use it.

I think that many people will do the same "mistake" as davidroler and many users will open an issue here.

@Timshel
Copy link
Contributor

Timshel commented Sep 15, 2023

@gianlucapisati ? of course if it's merged a corresponding change will be done to bw_web_builds to as mentioned at minima revert hiding the SSO button. No PR is open since this one has yet to be reviewed so no need to split discussion.

@davidroler the underlying library used to handle the flow support multiple authentication method of which :

  • ClientSecretJwt : Client authentication using a JSON Web Token signed with the client secret used as an HMAC key.
  • PrivateKeyJwt : JSON Web Token signed with a private key whose public key has been previously registered with the OpenID Connect provider.

But I don't expect I'll work on it, if you want to have a look you'll need to replace the secret here.

@zeitue
Copy link

zeitue commented Sep 15, 2023

@gianlucapisati I'm not sure which "code" you are referencing ?
With just the minimum patch to revert hiding the sso button (which is compatible with the non-sso version) there is the issue of the organization invitation link not working but that's why there is the SSO_ACCEPTALL_INVITES config. Outside of it I'm unaware of any other issue.

With "digging into the code" I mean that if we merge the pull request as is (which means that the functionality is completed, working and tested) you still need to apply the patch to use it.

I think that many people will do the same "mistake" as davidroler and many users will open an issue here.

What about just having an environment variable such as SHOW_SSO=true and if it is set then apply the patch automatically.

@gianlucapisati
Copy link

@gianlucapisati ? of course if it's merged a corresponding change will be done to bw_web_builds to as mentioned at minima revert hiding the SSO button. No PR is open since this one has yet to be reviewed so no need to split discussion.

aaaah my bad sorry! I didn't get the web vault was in a separate repo 🤦

Sorry!

@BlackDex
Copy link
Collaborator

Hello all. I'm very busy with a lot of other stuff in my spare time, mostly Vaultwarden stuff but all other items. So, i do not have that much time to check all the PR's. And since this PR seems to still need some love what i see is being said by the people who work on it and test this, which is great, i'm putting my efforts on other items.

If there are any main guidelines needed, like, should we do this or that, please ask that in a straight question, like with bullet-points or something, so that the main contributors can look at them and give there opinion.

I also see a lot of people having issues with the web-vault not showing the button. This can be easily solved with just one single line of command, or, if you build a container from this PR and run that afterwards, you can have that done automatically during startup. Here is an example.

Create a file called vaultwarden.sh:

#!/usr/bin/env sh

# Remove CSS to hide SSO Link
sed -i 's#a\[routerlink="/sso"\],##' /web-vault/app/main.*.css

Add the following volume mount to your docker/docker-compose config: -v"${PWD}/vaultwarden.sh:/etc/vaultwarden.sh"
I used "${PWD}" here because i have that file located in the directory where i run the docker run command.
But just make sure the first part points to the correct location of the vaultwarden.sh file.

This file will then be called before vaultwarden starts, which modifies the CSS file to remove the CSS that hides the button.
You can also run that command on your local web-vault of course, just make sure it points to the correct app/main.*.css file.

@Timshel
Copy link
Contributor

Timshel commented Sep 16, 2023

And since this PR seems to still need some love

@BlackDex outside of merging timshel/sso-support (@tribut ? :), I'm unsure of what is needed ? (Of course I can understand you won't look at it while it's not in this PR just wanted to be sure I'm not missing anything else ^^)

For issues in need of answer, I can think of :

  • Organization invitation do not work with the SSO login (some information are lost during the redirection), solutions are :
    • A front-ent patch which change the Typecript is this acceptable ? (This is compatible with the non-sso logic).
    • A config SSO_ACCEPTALL_INVITES which change the logic to auto-accept pending invitation when a user log (a modified mail without invitation link notify the user of it). is it acceptable ?
  • Concerning the front-end build, can you confirm that all modifications should stay compatible with the non-sso version ? Some --build-arg could be used in the docker image but it still would probably result in a separate release at some point.

Edit: additionally I played with Playwright and wrote a simple scenario to test SSO unboarding then login using a Keycloak instance run with docker-compose. Is this something you would be interested in integrating and expanding ?

@BlackDex
Copy link
Collaborator

And since this PR seems to still need some love

@BlackDex outside of merging timshel/sso-support (@tribut ? :), I'm unsure of what is needed ? (Of course I can understand you won't look at it while it's not in this PR just wanted to be sure I'm not missing anything else ^^)

For a good review i need to have the full picture and able to test this. If I'm correct there is some documentation on how to do this so that will help when i come to it.

For issues in need of answer, I can think of :

* Organization invitation do not work with the SSO login (some information are lost during the redirection), solutions are :
  
  * A front-ent [patch](https://github.com/Timshel/oidc_web_builds/blob/master/oidc_invite.patch) which change the `Typecript` is this acceptable ? (This is compatible with the non-sso logic).

That frontend patch looks complicated to maintain. If they just adjust something to this logic, it's going to be a pain to test.
And, my fear is, that this will land on the current top maintainers, of which i think they probably are not going to use SSO at all, and thus a small buggy can sneak in quickly. I also just want to have minimal changes done, just some name changes and CSS additions and some small changes to have Vaultwarden work well, not too much.

  * A config `SSO_ACCEPTALL_INVITES` which change the logic to auto-accept pending invitation when a user log (a modified mail without invitation link notify the user of it). is it acceptable ?

I would have to see the logic, but there might be a different option, but for that, i would have to really dive into this PR and setup a test environment to test this whole flow including the mentioned issues.

* Concerning the front-end build, can you confirm that all modifications should stay compatible with the non-sso version ?   Some `--build-arg`  could be used in the docker image but it still would probably result in a separate release at some point.

The front-end should stay compatible with people who do not use SSO. So, any adjustments to the front-end which will break that are a no-go. But as mentioned above i would like to only have small adjustments done on the web-vault.
So, if a change would be to remove the hiding of the SSO login button, that is fine, because, if that is a working feature, then it's fine to have that from my point of view.

@Timshel
Copy link
Contributor

Timshel commented Sep 16, 2023

If I'm correct there is some documentation on how to do this so that will help when i come to it.

There is some written by @tribut, alternatively I can point you to the docker-compose I'm using.

@bmunro-peralex
Copy link
Author

Closing due to being superseded by #3899

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.