feat(auth): reverse proxy authentication support - #176 #1152

ajgon · 2021-06-05T09:54:30Z

This is an attempt to address an issue described in #176 . Basically it allows auto-login user using forwarding proxy manager like authelia or vouch. This PR introduces two new config options:

ReverseProxyUserHeader - HTTP header containing user name from authenticated proxy. Default: Remote-User
ReverseProxyWhitelist - Coma separated lists of IP CIDRs which are allowed to use this feature - usually should contain reverse proxy IP with mask. Setting this to 0.0.0.0/0 would allow any connection to use the header (which is discouraged). By default everything is blacklisted, which means, this config parameter must be set to actually use the feature.

deluan · 2021-06-05T23:20:37Z

Thanks for this! Will take a look in the next couple of days.

deluan

This is great!! Some minor things here and there but very good overall.

Can you also make a PR to the documentation site, at least to add the config options? https://github.com/navidrome/website

Thanks!

deluan · 2021-06-07T22:40:03Z

conf/configuration.go

@@ -117,6 +120,11 @@ func Load() {
 		os.Exit(1)
 	}

+	if Server.ReverseProxyUserHeader == "" {


Defaults should be set with viper.SetDefault(). See examples on line ~200

deluan · 2021-06-07T22:41:26Z

conf/configuration.go

+	if Server.ReverseProxyUserHeader == "" {
+		Server.ReverseProxyUserHeader = "Remote-User"
+	}
+	Server.ReverseProxyUserHeader = http.CanonicalHeaderKey(Server.ReverseProxyUserHeader)


I'm pretty sure you don't need to call CanonicalHeaderKey, as Header.Get will match any case.

The Header.Get will do, but user may set it un-canonical way in config (i.e. remote-user). But I agree, it shouldn't go there, I'll perform the check where I actually need the header.

edit: Actually never-mind, you are completely right - I'm already doing Get where necessary, so this is not required here :)

deluan · 2021-06-07T22:49:55Z

server/app/auth.go

+func handleLoginFromHeaders(ds model.DataStore, r *http.Request) (payload map[string]interface{}, err error) {
+	var errIPNotInWhitelist = errors.New("IP is not whitelisted for reverse proxy login")
+	if !validateIPAgainstList(r.RemoteAddr, conf.Server.ReverseProxyWhitelist) {
+		log.Info(errIPNotInWhitelist.Error(), "ip", r.RemoteAddr)


The syntax for passing an error to log would be:

log.Error("message", key, value, key, value, err)

But in this case, I don't think you need to create the error variable. I'd use:

log.Error("IP is not whitelisted for reverse proxy login", "ip", r.RemoteAddr)

deluan · 2021-06-07T22:50:21Z

server/app/auth.go

+	userRepo := ds.User(r.Context())
+	user, err := userRepo.FindByUsername(username)
+	if user == nil || err != nil {
+		log.Info("User passed in header not found", "user", username)


This should be Warning or Error

deluan · 2021-06-07T22:50:53Z

server/app/auth.go

+
+	err = userRepo.UpdateLastLoginAt(user.ID)
+	if err != nil {
+		log.Error("Could not update LastLoginAt", "user", username)


You should add the err as a last parameter to the log in this case, to show in the logs what was the error

deluan · 2021-06-07T22:51:16Z

server/app/auth.go

+	bytes := make([]byte, 3)
+	_, err = rand.Read(bytes)
+	if err != nil {
+		log.Error("Could not create subsonic salt", "user", username)


You should add the err as a last parameter to the log in this case, to show in the logs what was the error

deluan · 2021-06-07T22:53:40Z

server/app/auth.go

@@ -40,6 +46,56 @@ func Login(ds model.DataStore) func(w http.ResponseWriter, r *http.Request) {
 	}
 }

+func handleLoginFromHeaders(ds model.DataStore, r *http.Request) (payload map[string]interface{}, err error) {


If you are explicitly ignoring the error of this function, we can remove it and in the case it is not a valid "login from headers", it just return a nil payload.

deluan · 2021-06-07T22:55:17Z

server/app/auth.go

+	}
+	ip = strings.Split(ip, ":")[0]
+	cidrs := strings.Split(comaSeparatedList, ",")
+	testedIP, _, err := net.ParseCIDR(fmt.Sprintf("%s/32", ip))


Question: does this handles IPv6? Also, why not make the ReverseProxyWhitelist a list of Host IPs, instead of Network IPs? Would it simplify things, for both the user and the code?

I'll fix IPv6 support, I completely forgot about it. Regarding Host IPs, I'm afraid I don't understand the question.

My understanding is, that r.RemoteAddr holds the IP of the connecting client (i.e. "raw" IP, not the one passed via X-* headers). So in this case it should be a proxy IP. We can check this proxy IP against whitelist to ensure, that no other party is trying to tamper the headers and log in bypassing authentication, by connecting directly to the app.

By "Host IP" I mean single address as opposed to a network address (with /size). Rephrasing my question: Why we have to specify the size of the network in each item of ReverseProxyWhitelist (like 192.168.1.1/32)? Why not just a list of IPs like 192.168.1.1,fdf8:f53b:82e4::53

Because sometimes (especially in docker containers) user may not be sure, what IP his proxy has. He only knows that it's in some specific subnet. Also, most of the projects (like gitea or grafana) which implements this feature, use CIDRs, so I believe it's somehow a standard.

deluan · 2021-06-07T22:56:56Z

server/app/serve_index.go

@@ -20,6 +20,8 @@ import (
 func serveIndex(ds model.DataStore, fs fs.FS) http.HandlerFunc {
 	policy := bluemonday.UGCPolicy()
 	return func(w http.ResponseWriter, r *http.Request) {
+		payload, _ := handleLoginFromHeaders(ds, r)


Small nitpick: payload here should be called auth

deluan · 2021-06-07T22:59:34Z

ui/src/authProvider.js

@@ -5,6 +5,20 @@ import { baseUrl } from './utils'
 import config from './config'
 import { startEventStream, stopEventStream } from './eventStream'

+if (
+  config.auth &&
+  /^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/.test(config.auth.id)


Instead of validating the ID, I think we should validate the JWT token, as it is more important for the whole authentication process. You could use the jwtDecode function, already imported. It throws an exception if the token is invalid

The problem is that jwtDecode would throw exception which may break module load. I'll keep this if condition for pre-check that we actually have auth to check, and I'll add jwtDecode inside of the if block.

You could do something like:

if (config.auth) { try { jwtDecode(config.auth.token) localStorage.setItem('token', config.auth.token) localStorage.setItem('userId', config.auth.id) ... } catch(e) { console.log(e) } }

This would not break the module load, right?

deluan · 2021-06-07T23:07:21Z

server/app/serve_index.go

@@ -37,6 +39,7 @@ func serveIndex(ds model.DataStore, fs fs.FS) http.HandlerFunc {
 			"version":                 consts.Version(),
 			"firstTime":               firstTime,
 			"baseURL":                 policy.Sanitize(strings.TrimSuffix(conf.Server.BaseURL, "/")),
+			"auth":                    payload,


One more thing: This now appears in debug logs:

DEBU[0004] UI configuration appConfig="map[auth:map[id:dfd95934-35fb-41e1-801e-710f6dafd9bd isAdmin:true message:User 'admin' authenticated successfully name:Dev Admin subsonicSalt:f6e4ae subsonicToken:c52d8017ba5b8791847e27785ad7a4af token:eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhZG0iOnRydWUsImV4cCI6MTYyMzE5MjEwMywiaWF0IjoxNjIzMTA1NzAzLCJpc3MiOiJORCIsInN1YiI6ImFkbWluIiwidWlkIjoiZGZkOTU5MzQtMzVmYi00MWUxLTgwMWUtNzEwZjZkYWZkOWJkIn0.8C7Pv-rwcWWBYWF7rORaxPb41r_ssTLiliiq_Rt7SYI username:admin] baseURL: defaultTheme:Dark devActivityPanel:true devEnableShare:false devFastAccessCoverArt:true enableDownloads:true enableFavourites:true enableStarRating:true enableTranscodingConfig:false enableUserEditing:false firstTime:false gaTrackingId: loginBackgroundURL:https://source.unsplash.com/collection/20072696/1600x900 losslessFormats:FLAC,WAV,ALAC,APE,DSF,WAV,SHN,WV,WVP version:dev welcomeMessage:Long long long long String <br/>Another String]"

Tokens (and the salt) must be redacted. You can find the redaction list here:

navidrome/log/log.go

Line 21 in 39d68e8

RedactionList: []string{

I added regexes to log/log.go but creds are still not redacted - am I doing something wrong? I even tested those regexes manually, and they should catch proper strings.

I'll take a look later tonight. Maybe it is a limitation of the redacting code

It was an issue with the redactor. I fixed it and pushed to your branch.

deluan

LGTM. Can you also please submit a PR to the docs site, to add the two new config options?

ajgon · 2021-06-10T07:33:57Z

Sorry, completely forgot about this :( Added documentation here navidrome/website#48 . English is not my primary language, so please ensure that what I wrote actually makes sense :)

deluan · 2021-06-12T03:16:40Z

Thanks! I'll review and do any changes to the docs before releasing.

henniaufmrenni · 2021-06-16T11:38:32Z

I've actually just tested this feature because it's super exciting to me and I'm really thankfull for the work, however somehow in my case the IP whitelisting doesn't take the right header into account. With the same reverse proxy setup - Traefik on VPS sending requests through a Wireguard Connection to a NAS running Navidrome via Docker (deluan/navidrome:sha-d3db41a) - I'm seeing this as the output of an exemplary whoami container

Hostname: e39fb27XXXX
IP: 127.0.0.1
IP: 172.24.0.4
RemoteAddr: 10.253.2.1:XXXXX
GET / HTTP/1.1
Host: whoami.example.com
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US;q=0.7,en;q=0.3
Authorization: Basic XXXXXXXXXXXXXXXXXXXX
Cache-Control: max-age=0
Cookie: authelia_session=XXXXXXXXXXXXXXXXXXXXXXX
Dnt: 1
Remote-Email: user@example.org
Remote-Groups: 
Remote-Name: 
Remote-User: user
Te: trailers
Upgrade-Insecure-Requests: 1
X-Forwarded-For: 46.114.XXX.XXX
X-Forwarded-Host: whoami.example.org
X-Forwarded-Port: 443
X-Forwarded-Proto: https
X-Forwarded-Server: dd1716fcb10a
X-Real-Ip: 46.114.XXX.XXX

My docker-compose looks the following:

version: "3"
services:
  navidrome:
    image: deluan/navidrome:sha-d3db41a
    user: 1000:1000 
    ports:
      - "5023:4533"
    restart: unless-stopped
    environment:
      ND_SCANSCHEDULE: 4h
      ND_LOGLEVEL: info
      ND_SESSIONTIMEOUT: 24h
      ND_REVERSEPROXYUSERHEADER: "Remote-User"
      ND_REVERSEPROXYWHITELIST: "10.253.2.0/24"

but the log output shows:
navidrome | time="2021-06-16T11:19:50Z" level=warning msg="IP is not whitelisted for reverse proxy login" ip=46.114.XXX.XXX

It seems its somehow not taking the RemoteAddr header, but rather X-Real-Ip or X-Forwarded-For into account. Setting ND_REVERSEPROXYWHITELIST to 0.0.0.0/0 works as expected.

ajgon · 2021-06-16T11:46:50Z

Drat, looks like it's problem with CHI middleware: https://github.com/go-chi/chi/blob/f71cd7d61909bbe19452b6104e76a4c4bf343e8e/middleware/realip.go#L32 .

Unfortunately I'm not familar with this framework - maybe @deluan can provide help here. I agree, it's a bug, only "raw IP" should be taken into consideration, without looking at X-... headers.

deluan · 2021-06-16T13:38:40Z

Humm, yeah, I'm pretty sure the realip middleware is the culprit. I'll disable it whenever this feature is used (ReverseProxyWhitelist != "")

…on feature Should fix #1152 (comment)

deluan · 2021-06-16T14:23:45Z

Hey @henniaufmrenni, can you try with the latest build?

henniaufmrenni · 2021-06-16T16:07:10Z

Hey just tested the latest build (sha-86271f0 ) and I can confirm that it's working 🎉


time="2021-06-16T15:58:22Z" level=debug msg="HTTP: GET http://example.org/api/keepalive/keepalive" elapsedTime="290.584µs" httpStatus=200 remoteAddr="10.253.2.1:43156" requestId=1a00644fd7a3/Lzt5bwPwPM-000001 responseSize=35 userAgent="Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0"
time="2021-06-16T15:58:22Z" level=debug msg="New broker client" client="8a35267d-6d7c-4022-ba8b-087ab8cc4131 (user - 10.253.2.1:43156 - 19de7235-bfeb-4698-9331-cf331992c0b3)" requestId=1a00644fd7a3/Lzt5bwPwPM-000002
time="2021-06-16T15:58:22Z" level=debug msg="Client added to event broker" newClient="8a35267d-6d7c-4022-ba8b-087ab8cc4131 (user - 10.253.2.1:43156 - 19de7235-bfeb-4698-9331-cf331992c0b3)" numClients=1

andrewzah · 2021-07-07T15:52:00Z

Are there docs for setting this up with Keycloak or Authelia?

ajgon · 2021-07-07T17:30:28Z

Are there docs for setting this up with Keycloak or Authelia?

Documenting this is out of the scope of navidrome, as every provider does it on its own way. I suggest, to refer to corresponding documentation of given identity provider. On navidrome side, you only need to set two configuration options (which are documented):

ND_REVERSEPROXYUSERHEADER (default value works for authelia, you may need to change it to X-Remote-User for some providers)
ND_REVERSEPROXYWHITELIST - should be set to your forwarding proxy IP, but if unsure at the beginning you can set it to 0.0.0.0/0 to allow everything

There is also a tutorial for vouch, it should provide some extra help how to start: https://old.reddit.com/r/navidrome/comments/oa8gkz/guide_how_to_use_a_sso_solution_in_front_of/

Good luck!

andrewzah · 2021-07-07T19:21:13Z

I see, thanks. I think it would be a good idea to add an example for Vouch in the docs so people get an idea.

For those of you using Keycloak + External Auth Server + Traefik, I had to do the following:

// /home/eas/app/bin/generate-config-token.js
let config_token = {
  eas: {
    plugins: [
      // firstly whitelist /rest/* paths for subsonic client apps
      {
        type: "request_js",
        snippet: "if (parentReqInfo.parsedUri.path.startsWith('/rest') == true) res.statusCode = 200;"
      },
      {
        type: "oidc",
        ...
         custom_service_headers: {
            "X-Auth-Username": {
                source: "userinfo",
                query_engine: "jp",
                query: "$.preferred_username",
                query_opts: {
                   single_value: true
              },
         }
      }
...

# docker-compose.yml
services:
  navidrome:
    ...
    environment:
      ND_REVERSEPROXYWHITELIST: "172.19.0.0/16"
      ND_REVERSEPROXYUSERHEADER: "X-Auth-Username"
    ...
    labels:
      - "traefik.enable=true"
      - "traefik.http.routers.navidrome.rule=Host(`example.com`)"
      - "traefik.http.routers.navidrome.entrypoints=websecure"
      - "traefik.http.routers.navidrome.tls.certresolver=myresolver"
      - "traefik.http.routers.navidrome.middlewares=navidrome-auth"
      
      # auth middleware
      - "traefik.http.middlewares.navidrome-auth.forwardauth.trustForwardHeader=true"
      - "traefik.http.middlewares.navidrome-auth.forwardauth.authResponseHeaders=X-Userinfo, X-Auth-Username, X-Id-Token, X-Access-Token, Authorization"
      - "traefik.http.middlewares.navidrome-auth.forwardauth.address=https://auth.example.com/verify?config_token=<token generated from running 'node generate-config-token.js'>"

edit: For the time being I've disabled this. I'm not sure if it's an EAS setting but I constantly have to log in with my external auth and Navidrome doesn't handle this well; I just have to refresh when I notice Navidrome showing errors about loading songs.

hscells · 2021-10-11T10:59:04Z

Hi, I have managed to get this working with my own setup, so thank you!

However, I was just wondering if it is possible to create a user if one doesn't already exist?

By this, I mean I am getting this error for a user that successfully authenticates using my reverse proxy authentication, but does not exist in the navidrome database:

navidrome/server/auth.go

Line 286 in aa1571e

log.Warn(r, "User passed in header not found", "user", username)

ajgon · 2021-10-11T20:09:25Z

Currently it's not possible, and I'm not sure if it's a good idea either. Registration should be a conscious act (either of a user or of an admin). And to be honest, I've never seen setup like that. I think it's more a question to @deluan - how the app should behave in this case.

andrewzah · 2021-10-12T01:18:56Z

And to be honest, I've never seen setup like that

This is very common for software that integrates LDAP or OIDC flows. Off the top of my head, I can say that Airflow, Jupyterhub, and Gitea support this flow either from configuring LDAP alone or configuring LDAP + some config settings.

Those also allow setting user roles or admin status via LDAP filters. This is also known as RBAC - Role Based Authentication.

So I don't think it's unusual to allow creating a user + setting roles here via http headers, so long as the source is trusted via the whitelist.

Registration should be a conscious act (either of a user or of an admin)

That is already handled by LDAP/OIDC. Right now you would have to 1) set up the user in Keycloak and then 2) set up the user, again, in Navidrome. It's redundant and, in my experience, confuses users because they have to register twice.

What I would recommend for Navidrome is having an environment variable like ALLOW_EXTERNAL_REGISTRATION to explicitly enable this behavior.

ajgon · 2021-10-12T06:23:49Z

Sounds reasonable - I'll take a look into that on a weekend 👍 .

deluan · 2021-10-13T00:22:09Z

The issue with creating a user without a password (or an auto-generated one) is that the Subsonic API will not work....

There is some initial work (#1262) from @mikhail5555 that is the first step towards the goal to allow each client/player to have their own password, but it is not possible yet. While we don't have this ability, I wouldn't spend time working on this ALLOW_EXTERNAL_REGISTRATION.

By the way, what about creating a new issue to track this feature request? Let's avoid discussing new features on closed PRs/issues

github-actions · 2023-03-12T02:03:04Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

feat(auth): reverse proxy authentication support - #176

776bc06

deluan requested changes Jun 7, 2021

View reviewed changes

deluan reviewed Jun 7, 2021

View reviewed changes

ajgon and others added 2 commits June 8, 2021 20:44

address PR remarks

bd93707

Fix redaction of UI appConfig

5796bb5

deluan approved these changes Jun 9, 2021

View reviewed changes

ajgon mentioned this pull request Jun 10, 2021

Add reverse proxy authentication documentation navidrome/website#48

Closed

deluan merged commit 6bd4c0f into navidrome:master Jun 12, 2021

deluan mentioned this pull request Jun 13, 2021

Refactor routing #1171

Merged

deluan added a commit that referenced this pull request Jun 16, 2021

Disable realip middleware when using the reverse proxy authenticati…

521d1ff

…on feature Should fix #1152 (comment)

This was referenced Jun 17, 2021

Passwords are stored in clear text #202

Closed

Possible to remove authentication? #176

Closed

NyaomiDEV mentioned this pull request Sep 16, 2021

[Request] Handle SSO/OAuth/OIDC proxies/middlewares (like Authelia and Vouch) pufferpanel/pufferpanel#1056

Open

selfhoster1312 mentioned this pull request Jan 13, 2023

Enable SSO auth (does not prevent Subsonic API on 'public' mode) YunoHost-Apps/navidrome_ynh#85

Closed

2 tasks

selfhoster1312 mentioned this pull request Jan 13, 2023

Create accounts automatically when authenticating from HTTP header #2087

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2023

feat(auth): reverse proxy authentication support - #176 #1152

feat(auth): reverse proxy authentication support - #176 #1152

Conversation

ajgon commented Jun 5, 2021 • edited Loading

deluan commented Jun 5, 2021

deluan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgon Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluan Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluan Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluan left a comment

Choose a reason for hiding this comment

ajgon commented Jun 10, 2021

deluan commented Jun 12, 2021

henniaufmrenni commented Jun 16, 2021 • edited Loading

ajgon commented Jun 16, 2021

deluan commented Jun 16, 2021 • edited Loading

deluan commented Jun 16, 2021

henniaufmrenni commented Jun 16, 2021

andrewzah commented Jul 7, 2021

ajgon commented Jul 7, 2021 • edited Loading

andrewzah commented Jul 7, 2021 • edited Loading

hscells commented Oct 11, 2021

ajgon commented Oct 11, 2021

andrewzah commented Oct 12, 2021 • edited Loading

ajgon commented Oct 12, 2021

deluan commented Oct 13, 2021

github-actions bot commented Mar 12, 2023

ajgon commented Jun 5, 2021 •

edited

Loading

ajgon Jun 8, 2021 •

edited

Loading

deluan Jun 8, 2021 •

edited

Loading

deluan Jun 7, 2021 •

edited

Loading

henniaufmrenni commented Jun 16, 2021 •

edited

Loading

deluan commented Jun 16, 2021 •

edited

Loading

ajgon commented Jul 7, 2021 •

edited

Loading

andrewzah commented Jul 7, 2021 •

edited

Loading

andrewzah commented Oct 12, 2021 •

edited

Loading