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

Add SSO auth support #2040

Closed
wants to merge 6 commits into from
Closed

Conversation

knrdl
Copy link

@knrdl knrdl commented Jan 16, 2023

This PR adds SSO support. The workflow looks like this:

  1. user wants to open mealie, sends a request
  2. therefore the RP (reverse proxy) forwards the request to the IDP (identity provider)
  3. IDP handles user auth and provides user info via http headers to RP (user info can also be empty, if desired)
  4. RP forwards the request (with additional headers) to mealie
  5. Mealie frontend gets application info from endpoint /api/app/about on load. It contains a new bool field "sso_login_available" which is true if sso is enabled in settings and at least the "Remote-User" header is present
    • if sso_login_available=false the regular login form is presented to the user
    • if sso_login_available=true the frontend fires an authentication request (without username or password) to get a session token from the backend. The backend checks whether it is fulfilling a SSO login or credentials must be checked.
  6. user gets logged in and can use mealie
  7. to logout a custom logout link to the IDP can be provided.

This PR is mostly based on the work of @tribut in #1622
However there are some notable differences:

  1. in the version by @tribut SSO was exclusive (either SSO xor internal auth/ldap). This PR allows internal/ldap as fallback
  2. if the user is not logged in there is no redirect to the IDP (identity provider). The reverse proxy could handle this initial redirect instead if desired
  3. instead of only "Remote-User" also the headers "Remote-Name" and "Remote-Email" are supported
  4. no duplication of config parameter between frontend and backend

Please have look!

podman requires absolute paths to some docker images on docker hub.
prevent parsing of string values as other types
@tribut
Copy link

tribut commented Jan 17, 2023

Thanks for picking this up!

I currently don't have time to test this, but afair the reason why the redirect was necessary was due to mealie being a PWA, the browser will not always request the login page from the proxy. Have you tested if having a redirect in the proxy works reliably?

@knrdl
Copy link
Author

knrdl commented Jan 19, 2023

Good point @tribut, I haven't thought about this edge case.
Assuming the following: Website is installed as PWA and user is logged in. The mealie session is valid but the IDP session is expired. The next http request will result in a 401 by the reverse proxy. So maybe redirect to the IDP URL on every 401 response is a solution? or am i missing something?

@knrdl knrdl marked this pull request as draft January 19, 2023 18:58
Comment on lines +119 to +120
"password": "SSO",
# Fill the next two values with something unique and vaguely

Choose a reason for hiding this comment

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

Could this be a security risk? Could you log in through the login form using "SSO" for this user? In that would be the case, maybe a randomly generated password could add some security.

@@ -61,7 +61,8 @@ def get_logged_in_user(self):
@user_router.put("/password")
def update_password(self, password_change: ChangePassword):
"""Resets the User Password"""
if not verify_password(password_change.current_password, self.user.password):
# when logged in via SSO, do not check old password
if self.user.password != "SSO" and not verify_password(password_change.current_password, self.user.password):
Copy link

@namelivia namelivia Feb 25, 2023

Choose a reason for hiding this comment

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

Could reusing the password field with a specific value be a problem in the future? It is giving the password field two meanings, maybe a boolean value for the user could be a safer option.

Copy link
Author

Choose a reason for hiding this comment

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

Could reusing the password field with a specific value be a problem in the future? It is giving the password field two meanings, maybe a boolean value for the user could be a safer option.

Passwords are not stored as plaintext. They are always hashed, and only the hash value gets persisted. Currently bcrypt is in use as hashing algorithm, see https://github.com/hay-kot/mealie/blob/39012adcc1b431c59ff9d83f75100971ffa540e0/mealie/core/security/hasher.py#L27
The hash values have a fixed size which is always longer than 3 characters (e.g. "SSO"). So the value "SSO" is an invalid hash value. Hashing always produces valid hash values. So there is no chance of overlapping values.

TL;DR no security problem here

@namelivia
Copy link

Nice! It would be very nice to have this in Mealie, if you need any kind of help testing or whatever is needed to help you push this forward let me know.

@knrdl
Copy link
Author

knrdl commented Feb 25, 2023

Nice! It would be very nice to have this in Mealie, if you need any kind of help testing or whatever is needed to help you push this forward let me know.

Implementing 99% of this feature is easy and done by this pull request. But the last 1% are nasty and non trivial. That's why I marked the PR as draft.

As I chose Tandoor Recipes instead of Mealie for now, I am currently not pushing this any further. Maybe others can help.

@namelivia
Copy link

Nice! It would be very nice to have this in Mealie, if you need any kind of help testing or whatever is needed to help you push this forward let me know.

Implementing 99% of this feature is easy and done by this pull request. But the last 1% are nasty and non trivial. That's why I marked the PR as draft.

As I chose Tandoor Recipes instead of Mealie for now, I am currently not pushing this any further. Maybe others can help.

I see, well, thanks for your work!

@aroberts
Copy link

@knrdl can you please list the issues you currently see that aren't handled by PR (that last 1%), so someone else can pick up the work?

@knrdl
Copy link
Author

knrdl commented Feb 27, 2023

Sure, the PWA/ServiceWorker aspect in conjunction with the Reverse Proxy is not fully handled or tested yet:

  • The SSO relevant endpoints (app_about, get_token) must not be cached by the SW
  • When the user is offline and the session expires, a redirect to the IDP cannot take place. Otherwise a Server Not Found / Timeout or similar page would be shown.
  • If the IDP session expires but the Mealie session is still valid, 401(or 403) api responses will be encountered. These should be handled by a logout. The action triggered by the user will then be lost(?)
  • If SSO login is mandatory (the only allowed auth method) the reverse proxy must enforce this. But in a PWA setup the reverse proxy is not involved at all. Therefore the fallback described by @tribut in Add SSO auth support #2040 (comment) could be applied by introducing an addition frontend env var like SSO_MANDATORY_LOGIN_URL. So mandatory SSO would not be initiated by the RP but Mealie instead.

These are fewer problems than I thought there remained. If I find the time maybe I will take another look into it. Or I forgot some of the edge cases, who knows :)

@aroberts
Copy link

thanks for the rundown!

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.

5 participants