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

feat: Add support for forward auth #1341

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

sgmitchell
Copy link

Description

Adds support for automatically authenticating as the user specified in the X-Forwarded-User header when the setting enableForwardAuth is set.

This is basically the same as #580 (the first 2 commits are cherry picked from that PR) but rebased and with suggestions applied

Screenshot (if UI-related)

image

To-Dos

  • Successful build pnpm build
  • Translation keys pnpm i18n:extract

Issues Fixed or Closed

@ishanjain28
Copy link

Hii, I don't see any role/group here. Does this only work for imported users ?
If not, how does it distinguish between jellyfin admins and jellyfin users ?

@sgmitchell
Copy link
Author

Hii, I don't see any role/group here. Does this only work for imported users ?

Yes. Any user would need to already be created for this to work. It doesn't provision users automatically.

Copy link
Author

Choose a reason for hiding this comment

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

All the changes in this file are just copied straight from #580. I'm not sure how this file is used or if there is a better way to generate this spec

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add this inside the securitySchemes section of components (line 1956), with something like this inside:

components:
  ...
  securitySchemes:
    ...
    forwardAuth:
      type: apiKey
      in: header
      name: X-Forwarded-User

I'm not an expert in this kind of spec either, so if someone could confirm it would be nice.

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha. So since that's already in place, I think this file is probably good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is too. But it doesn't hurt to have another check.

@gauthier-th gauthier-th mentioned this pull request Feb 11, 2025
1 task
server/middleware/auth.ts Outdated Show resolved Hide resolved
src/utils/localRequestHelper.ts Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

LGTM.
I'd like a few poeple to test this and report it's working well before merging though, as I'm not using forwardAuth myself.
I'll create a preview tag soon.

@tsallou
Copy link

tsallou commented Feb 13, 2025

Hello, thank you for this timely contribution! I was actually looking for ways to authenticate my setup.

I tested your code in a test environment using Authentik and Nginx Proxy Manager. It works as expected, which is great, and it's quite simple to set up.

I did notice one issue with the username in the header. I have a user with an email address set up in Jellyseer, and authentication failed for this user. It only worked for users without an email address. I think we need to make it work in all cases, as I use LDAP with Jellyfin to create users in Jellyseer, and they don't have emails by default (though my admin account does).

From a security perspective, I believe merging this PR as-is poses a significant risk. There's nothing to warn the user (either in the UI or the documentation) that enabling this feature without a properly configured reverse proxy exposes the instance completely to anyone who knows a username or email address used in the application.

At the very least, we should clearly state in the UI that enabling this option can seriously compromise the instance's security.

Some more permanent solutions could be:

  • Hiding the setting in a configuration file (this is what Sonarr/Radarr do for similar parameters).
  • Enforcing that the parameter is only considered when the request comes from a trusted proxy IP.

@sgmitchell
Copy link
Author

It only worked for users without an email address.

Hmmm. I have it tested working with both local and jellyfin users with and without emails. When a user is added in the UI and no email is set, it will default the email to the username (src). Additionally if you dump the schema by running something like sqlite3 /app/config/db/db.sqlite3 '.schema User --indent', email is the only field with a unique constraint so either that or the integer id are the only 2 fields that I think would be safe to use.

I'm not sure what your ldap plugin is doing but could you inspect your local database and see if your LDAP client is following the process as jellyseerr's source?

There's nothing to warn the user (either in the UI or the documentation)
...
At the very least, we should clearly state in the UI that enabling this option can seriously compromise the instance's security.

I've put a screenshot showing the advanced label and extra text stating Only enable when secured behind a trusted proxy. Do you have a suggestion as to better wording to make that clear?

@jcbxz
Copy link

jcbxz commented Feb 14, 2025

Hi 👋

Thanks for picking this up, I'm also interested in this functionality.

I'm chiming in with a slightly different setup, I am not using Authentik but a combo of Authelia and Traefik. It seems as though Authelia will provide slightly different headers compared to Authentik: https://www.authelia.com/integration/trusted-header-sso/introduction/#response-headers.

Would it possible for the header name to be configurable so I can have Jellyseerr look for the appropriate header for Authelia rather than requiring header rewrites in Traefik?

Another fairly minor thing I've noticed is that every page refresh will redirect to the /login before redirecting to the dashboard, so you can lose your place in the app. eg. if you refresh on /discover/tv it will take you back to the dashboard instead of where you were.

@ishanjain28
Copy link

@jcbxz These headers are returned by authelia in the forward auth response and they have to be copied to the X-... header in trafeik from the forward auth request. It would be nice to have this as a configurable option but it doesn't change any thing. You still need to copy those headers in the reverse proxy.

@jcbxz
Copy link

jcbxz commented Feb 15, 2025

@jcbxz These headers are returned by authelia in the forward auth response and they have to be copied to the X-... header in trafeik from the forward auth request. It would be nice to have this as a configurable option but it doesn't change any thing. You still need to copy those headers in the reverse proxy.

Sure, I have it configured to copy the headers from authelia in the traefik middleware with the label they recommend in their example, eg:

labels:
    traefik.enable: 'true'
    traefik.http.routers.authelia.rule: 'Host(`example.com`)'
    traefik.http.middlewares.authelia.forwardAuth.address: 'http://authelia:9091/api/authz/forward-auth'
    traefik.http.middlewares.authelia.forwardAuth.trustForwardHeader: 'true'
    traefik.http.middlewares.authelia.forwardAuth.authResponseHeaders: 'Remote-User,Remote-Groups,Remote-Email,Remote-Name'

The problem is this behavior can only copy headers, not rename them - I can't just put X-Forwarded-User there and expect it to work because Authelia doesn't provide that header. As far as I'm aware X-Forwarded-User isn't a standardized name so perhaps it shouldn't be assumed that's where the header will always be.

but it doesn't change any thing

It changes whether a traefik plugin to modify response headers is required for Jellyseerr. It's not impossible, but it's a bit unfortunate if the feature doesn't cater towards different headers like those Authelia provides.

Just to throw out an example of another app that offers this functionality, this is a screenshot from the Authelia docs of Organizr's configuration page:

image

My 2c, but would it be an option to keep it fairly simple and change the toggle on/off in the settings into an input field which accepts the header name?

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.

Support for Forward Auth
6 participants