OIDC SSO Re: Issue #246 #1955
OIDC SSO Re: Issue #246 #1955
Conversation
|
Hi @Sheap |
|
Done |
| name TEXT NOT NULL, | ||
| billing_email TEXT NOT NULL | ||
| uuid VARCHAR(40) NOT NULL PRIMARY KEY, | ||
| name TEXT NOT NULL, |
There was a problem hiding this comment.
These should be added in a separate migration, something like this one https://github.com/dani-garcia/vaultwarden/blob/main/migrations/sqlite/2021-05-11-205202_add_hide_email/up.sql
There was a problem hiding this comment.
I would even say this should be in a separate table by it self and have foreign keys on the org id.
This so the org table keeps clean, and what if bitwarden will support multiple providers for example.
src/api/core/organizations.rs
Outdated
| @@ -200,6 +213,35 @@ fn post_organization( | |||
|
|
|||
| org.name = data.Name; | |||
| org.billing_email = data.BillingEmail; | |||
| org.identifier = match data.Identifier { | |||
There was a problem hiding this comment.
| org.identifier = match data.Identifier { | |
| org.identifier = data.Identifier.unwrap_or_default(); |
src/api/identity.rs
Outdated
| @@ -102,6 +169,15 @@ fn _password_login(data: ConnectData, conn: DbConn, ip: &ClientIp) -> JsonResult | |||
| err!("This user has been disabled", format!("IP: {}. Username: {}.", ip.ip, username)) | |||
| } | |||
|
|
|||
| // Check if org policy prevents password login | |||
| let user_orgs = UserOrganization::find_by_user_and_policy(&user.uuid, OrgPolicyType::RequireSso, &conn); | |||
| if user_orgs.len() == 1 && user_orgs[0].atype >= 2 { | |||
There was a problem hiding this comment.
| if user_orgs.len() == 1 && user_orgs[0].atype >= 2 { | |
| if user_orgs.len() >= 1 && user_orgs[0].atype < UserOrgType::Admin { |
There was a problem hiding this comment.
I went with a version which better reflects the documentation. Specifically:
if user_orgs.len() >= 1 && user_orgs[0].atype != UserOrgType::Owner && user_orgs[0].atype != UserOrgType::Admin {
src/api/identity.rs
Outdated
| err_msg += &format!("\n caused by: {}", cause); | ||
| cur_fail = cause.source(); | ||
| } | ||
| panic!("{}", err_msg); |
There was a problem hiding this comment.
We shouldn't panic during the normal server operation, it's better to return a Result instead.
src/db/models/organization.rs
Outdated
| @@ -131,13 +138,20 @@ impl Organization { | |||
| billing_email, | |||
| private_key, | |||
| public_key, | |||
| identifier: String::from(""), | |||
| use_sso: false, | |||
| callback_path: String::from("http://localhost/#/sso/"), | |||
There was a problem hiding this comment.
These should all use String::new(), or ideally be an Option when not defined.
src/api/identity.rs
Outdated
|
|
||
| #[get("/account/prevalidate?<domainHint>")] | ||
| #[allow(non_snake_case)] | ||
| fn prevalidate(domainHint: &RawStr, conn: DbConn) -> JsonResult { |
There was a problem hiding this comment.
Why not use a String domainHint? Using a String it should be percent decoded already. Same goes for authorize()
src/api/identity.rs
Outdated
| @@ -432,3 +513,151 @@ fn _check_is_some<T>(value: &Option<T>, msg: &str) -> EmptyResult { | |||
| } | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn invalid_json(error_message: &str, exception: bool) -> JsonResult { | |||
There was a problem hiding this comment.
This function is simple enough that it could be inlined. Also it always uses exception == false, as far as I can see.
src/api/identity.rs
Outdated
|
|
||
| // TODO store the nonce for validation on authorization token exchange - unclear where to store | ||
| // this | ||
| let (mut authorize_url, _csrf_state, _nonce) = client |
There was a problem hiding this comment.
It's not the same, but as an example, any nonce/challenges we use for the two factor authentication we save in its database table, check the type TwoFactorType, the values >= 1000 are used for this kind of temp authentication values and they are deleted once used.
In your case SSO is not two factor authentication, but you could create a new database table just for those.
Cargo.toml
Outdated
| @@ -137,6 +137,8 @@ backtrace = "0.3.60" | |||
|
|
|||
| # Macro ident concatenation | |||
| paste = "1.0.5" | |||
| openidconnect = "2.0.1" | |||
| urlencoding = "1.1.1" | |||
There was a problem hiding this comment.
This urlencoding library doesn't seem to be used.
Running a docker instance of keycloak seems like the simplest option: docker run -p 8080:8080 -e KEYCLOAK_USER=admin -e KEYCLOAK_PASSWORD=admin quay.io/keycloak/keycloak:15.0.2I'm trying to test this PR locally, but haven't succeeded yet. The code builds and runs, but I don't see the "Login with SSO" button. @Sheap What config ( |
|
I manged to apply the patch, but am running into build errors. This is what I did: Build output: Build output |
|
@pinpox sorry for the delay in getting back to you - I was busy last week, then had a long weekend diff --git a/.dockerignore b/.dockerignore
index 69f51d2..6bc2dbd 100644
--- a/.dockerignore
+++ b/.dockerignore
@@ -3,7 +3,6 @@ target
# Data folder
data
-.env
.env.template
.gitattributes
@@ -24,4 +23,4 @@ hooks
tools
# Web vault
-web-vault
\ No newline at end of file
+#web-vault
diff --git a/docker/amd64/Dockerfile b/docker/amd64/Dockerfile
index 70d9c8f..8737ad5 100644
--- a/docker/amd64/Dockerfile
+++ b/docker/amd64/Dockerfile
@@ -22,7 +22,6 @@
# $ docker image inspect --format "{{.RepoTags}}" vaultwarden/web-vault@sha256:29a4fa7bf3790fff9d908b02ac5a154913491f4bf30c95b87b06d8cf1c5516b5
# [vaultwarden/web-vault:v2.21.1]
#
-FROM vaultwarden/web-vault@sha256:29a4fa7bf3790fff9d908b02ac5a154913491f4bf30c95b87b06d8cf1c5516b5 as vault
########################## BUILD IMAGE ##########################
FROM rust:1.53 as build
@@ -101,7 +100,7 @@ EXPOSE 3012
# and the binary from the "build" stage to the current stage
WORKDIR /
COPY Rocket.toml .
-COPY --from=vault /web-vault ./web-vault
+COPY ./web-vault/build ./web-vault
COPY --from=build /app/target/release/vaultwarden .
COPY docker/healthcheck.sh /healthcheck.sh@dani-garcia Thanks for the comments - I've addressed the simpler points already, but I didn't have time today to look into the ones which required more consideration. I'll hopefully get to them this week. |
|
@Sheap I just wanted to say thank you for getting the ball rolling on this. It's amazing! |
|
@Sheap I got it to work, but am running into a panic when trying to Might be a configuration error, but a panic seems a bit harsh? |
|
It's not an error I've seen. Removing the panics in favour of graceful errors is one of the things I'm looking into at the moment. |
|
Okay, big changes 😪. I think I've addressed all of the above points now. |
|
The newest patch fails to apply again: Are these files missing? |
|
I've got a version with the login process working now. I get redirected correctly to the authentication page of the authentication provider) and can login. However, I've not been able to find out what values are needed for What do I have to set for the |
|
I think, those fields should be read only. In a vanilla bitwarden they are |
That's true, but they have to be set to something, otherwise the login process fails as far as I could find out. I've been continuing to work on this PR. Not sure if @Sheap is still planning on working on it, should I open a second on (maybe as draft?) to track progress? I've rebased against the current master so that it builds with the updated web vault, but still am running into a few bugs and some feedback would be great to help me get it in a mergable state. @BlackDex let me know what way you prefer. |
|
If the comments above are addressed i don't mind how this will be getting to a mergeable PR. |
|
@pinpox looking over my code, I believe those fields should be |
|
I'm running into a problem here: The code seems to work fine now, I can login and get redirected to the URL I specify. However
Where is that defined and can it be changed to something else that does not include |
|
@pinpox The path is defined in webvault If ADFS don't allow fragments or queries, then the only option I can think of would be to set up a subdomain. But I don't know how to do that in this context. |
|
@Sheap @pinpox @BlackDex @dani-garcia There are two open PRs regarding (pseudo) SSO in vaultwarden (#1955 and #2449). Both seem to be stuck in the review stage for many weeks. I know how hard it is to find time for these projects – totally understandable. If we (the user community) got together and collected some money to be split among all of you, would that speed up to process? Probably won't be much, but still more than nothing, and might be enough to buy a nice dinner for everyone. Are you open to the idea of such a bounty model? I'd also appreciate any reactions by the community, to show you'd be willing to donate some money to this cause (just to get an idea about how many people are willing to pay some money for this feature). |
|
@pReya I am still actively working on my PR, but progress is a bit slow. I currently have the time to work on it and money is not the show stopper, what I would need to speed this up would be some technical assistance in questions specific to this project and to rust. I'm currently in the process of applying all the fixes from the review that was made on my PR, but it took so long that I have since changed quite a few things and have to adapt a lot. I'll try the matrix channel later with a few questions I still have to get this finished. Would be nice to speed up the feedback loop. |
|
@Sheap did this work for you? I added a print statement to see the token, it is: Which according to jwt.io decodes to the payload: |
|
@pinpox I think that the |
This is not right. Required claims are listed here |
|
Ok @Lurkars you are right about the theory, but neither |
|
I think Gitea's Oauth2 provider implementation is only meant to allow third party apps to access Gitea's API. It's doesn't look like it's really meant to authenticate users on a external service. |
The format of an access_token is not defined, it can also be a random string and not containing any encoded data. So don't rely any requirements on getting information from an access_token. It seems like Gitea generates access_tokens as JWTs, I don't know why, but this is not OAuth2 standard and cannot be assumed for other providers. Claims like email, sub inside an ID Token are the additions OpenID Connect added to the OAuth2 spec. That's why we are asking for OpenID support and not only OAuth2. The Gitea documentation is a bit confusing (https://docs.gitea.io/en-us/oauth2-provider/) because, they claim to support OpenID Connect, but sample token response does not contain an ID Token. I will now try to test if Gitea can be used as valid OpenID Connect provider. Anyways, I am 0% into Rust, so cannot contribute directly here. But I am very deep into OpenID Connect protocol. So if you need any help here, ping me! Edit: I just tested with a gitea instance. So it returns a valid ID token, so seems fine to me. |
|
A quick look at the code, I found this So it seems to try to read the access_token as ID token. Please use id_token instead. Every OpenID Provider will return id_token next to access_token in Token Response! Taking a quick look at library example: https://github.com/ramosbugs/openidconnect-rs/blob/main/examples/google.rs#L215 seems like you can get claims directly from this and don't need to decode JWT yourself! So I would recommend to simple use the library to get email and sub etc. |
@Kab1r This can't be correct. I have an account on bitwarden.com and have tested the configuration there. Gitea works perfectly with an official bitwarden account.
Can confirm.
Perfect, thanks that was what I was looking for! Indeed using the Logging it seems to work now via OIDC, but I still don't know what the callback path is supposed to be. Currently it just redirect to the login screen again after login. If anyone (with more front-end) knowledge) can find out what the callback path is supposed to be, that would be of great help! |
|
Wow, that's a wonderful development! 🤩 |
I haven't used authelia personally, but their website says it supports OpenID connect in the beta version currently. If it can operate as OIDC provider, it should work. |
@pinpox According to the Bitwarden Docs the correct callback path should be https://your.domain.com/sso/oidc-signin, I believe. I could be wrong though, I'm not an expert in OIDC (yet ;)). |
|
Is this still in progress or has this been abandoned? |
Still in progress. See #2449 |
|
Hi this pr is in progress? |
I think the most recent progress is tracked… there: #3899 |
|
Closing this PR in favor of #3899 to keep overview of it all. |
This branch adds the minimal required changes to allow OIDC SSO (at least with keycloak. There are many options/features of the process which I did not address, which may rule out other providers).
This is so far my biggest contact point with rust in a web server, as well as in implementing OIDC, so I wouldn't be surprised if there were places where I strayed from the path. But I have tested it, and at least it works. I'd welcome any feedback here. My greatest concern is that I'm currently ignoring the nonce. I believe it should be checked against, but I'm not sure where would be a sensible place to store it between generating it alongside the auth url, and consuming it when exchanging the code for an access token.
Note: There are associated changes required to the webvault here - I wasn't sure how I should link them, so to keep things simple for now, I've simply included a .patch file in this repo.