almarklein / timetagger

Tag your time, get the insight
https://timetagger.app
GNU General Public License v3.0
1.11k stars 99 forks source link

Reverse Proxy authentication trust doesn't work as expected #344

Open cfstras opened 1 year ago

cfstras commented 1 year ago

I've come across two issues when looking at reverse proxy authentication:

  1. https://github.com/encode/uvicorn/issues/1068 This is mostly only an issue when f.ex. testing with localhost. As a workaround, one can use the LAN IP to access the proxy instead.

More importantly:

  1. The application server used, uvicorn, has its own logic for parsing the X-Forwarded-For header. Combined with the functionality in timetagger, this will mangle/break the list of forwarded IPs for incoming requests, potentially even trusting fake headers sent by a client! To fix this, one has to export FORWARDED_ALLOW_IPS="" to disable the uvicorn parsing. See uvicorn docs.

For reference, I'm passing

--proxy_auth_header="X-Forwarded-User"
--proxy_auth_trusted="127.0.0.1"
--proxy_auth_enabled=True
almarklein commented 1 year ago

Sorry for responding so late, this slipped my attention. I haven't looked looked into proxy authentication that much, to be honest. Are there things in TimeTagger that we can do to resolve these issues? PR's welcome :)

Also cc @Rynoxx @mtn-mathi @foorschtbar

cfstras commented 1 year ago

My first instinct would be to remove the X-Forwarded-For code from timetagger, trust the functionality in uvicorn and configure it automatically and/or document how to. Though I‘m not sure why it was added in the first place if uvicorn was always used, or, in which usecases it was tested, so we can make sure not to break these.

foorschtbar commented 1 year ago

Sorry for responding so late, this slipped my attention. I haven't looked looked into proxy authentication that much, to be honest. Are there things in TimeTagger that we can do to resolve these issues? PR's welcome :)

Also cc @Rynoxx @mtn-mathi @foorschtbar

For me, the current solution works at the moment.

cfstras commented 1 year ago

@foorschtbar Could you please describe the setup you’re using, the flags and env vars, and which headers are passed by your reverse proxy? In my tests, using oauth2-proxy, timetagger (without the described workaround) would always ignore the X-Forwarded-User header. Upon debugging, I found that uvicorn already parses the X-Forwarded-For, and clears it, and then timetagger tries to check it again and decides to skip proxy auth handling because it‘s missing.

Rynoxx commented 1 year ago

@cfstras This config works for me in kuberenetes behind Authentik as the proxy-auth provider. I'm not seeing any differences in behavior between having FORWARDED_ALLOW_IPS="" or not, also tried *.

But this might also be because authentik is using a custom header (X-authentik-username) for it and not a X-Forwarded-* header?

          env:
            - name: TIMETAGGER_BIND
              value: "0.0.0.0:80"
            - name: TIMETAGGER_CREDENTIALS
              value: "<redacted>"
            - name: TIMETAGGER_LOG_LEVEL
              value: "info"
            - name: TIMETAGGER_DATADIR
              value: "/root/_timetagger"
            - name: TIMETAGGER_PROXY_AUTH_ENABLED
              value: "True"
            - name: TIMETAGGER_PROXY_AUTH_HEADER
              value: "X-authentik-username"
            - name: TIMETAGGER_PROXY_AUTH_TRUSTED
              value: "10.0.0.1/8"
            - name: TZ
              value: "Europe/Stockholm"
cfstras commented 1 year ago

Ah, that explains it. Just checked the uvicorn code: https://github.com/encode/uvicorn/blob/87387273945624452c1f0e797bcf2a539b0c9211/uvicorn/middleware/proxy_headers.py#L28

The behavior there seems to only happen when the reverse proxy uses 127.0.0.1.

It‘s not clear to me why setting it to * doesn’t trigger the bug for you though… Maybe the config is not getting through for some reason?

IMO we either should disable that default in uvicorn, or just pass the TIMETAGGER_PROXY_AUTH_TRUSTED to the uvicorn middleware config instead of manual handling? The latter could make the code a lot simpler.

foorschtbar commented 1 year ago

@foorschtbar Could you please describe the setup you’re using, the flags and env vars, and which headers are passed by your reverse proxy? In my tests, using oauth2-proxy, timetagger (without the described workaround) would always ignore the X-Forwarded-User header. Upon debugging, I found that uvicorn already parses the X-Forwarded-For, and clears it, and then timetagger tries to check it again and decides to skip proxy auth handling because it‘s missing.

I use Authelia (no setup needed for Timetagger) and Traefik:

Treafik:

- "traefik.http.middlewares.timetagger-auth.forwardauth.address=http://authelia:9091/api/verify?rd=https%3A%2F%2Fauth.xxx.tld"
- "traefik.http.middlewares.timetagger-auth.forwardAuth.trustForwardHeader=true"
- "traefik.http.middlewares.timetagger-auth.forwardAuth.authResponseHeaders=Remote-Email"

Timetagger:

- TIMETAGGER_PROXY_AUTH_ENABLED=True
- TIMETAGGER_PROXY_AUTH_TRUSTED=127.0.0.1,172.xxx.xxx.xxx/24
- TIMETAGGER_PROXY_AUTH_HEADER=Remote-Email
cfstras commented 1 year ago

Hm, @foorschtbar I‘d guess that in your case the traefik requests are coming to timetagger from 172.x.x.x, so you‘re not triggering this bug.