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 support to forward header with oidc token #100

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gcavalcante8808
Copy link

@gcavalcante8808 gcavalcante8808 commented Apr 14, 2020

Scenario

For whom that want to use the user claims in their application or other layers (OPA I'm looking to you :D), this PR add the header X-Oidc-Token to the forwarded headers supported by thomseddon / traefik-forward-auth; with that, subsequent middlewares or applications can decrypt and use the information inside the token.

Inspired by istio/istio#11108

What has been done

  • Added oidc token to AuthCookie.
  • Added X-Oidc-Token header.
  • Updated Docs to reflect the new header.
  • Manual Tests with Auth0/OIDC.

@gcavalcante8808 gcavalcante8808 changed the title Forward Header with oidc token Add support to forward header with oidc token Apr 14, 2020
@thomseddon
Copy link
Owner

I like this - I wonder if there's a more generic way to handle forwarding parameters from providers? There's an earlier request for the access token with the google provider in #30 as well the OIDC claims.

@thomseddon thomseddon added enhancement New feature or request under review labels Apr 14, 2020
@gcavalcante8808
Copy link
Author

gcavalcante8808 commented Apr 14, 2020

@thomseddon, Ideally we should have id_token , access_token and refresh_token available, but I don't know if there is security implications for access_token.

Looking at some solutions like AWS ALB, it makes those tokens available as forwarded headers:

image

This PR can be this first step; acess and refresh token can and should be implemented in future PRs. If we going for this path, we should use X-Oidc-IdToken here instead of X-oidc-token and add X-Oidc-AccessToken and X-RefreshToken for other tokens when the respective PRs arrives; what do you think?

@thomseddon
Copy link
Owner

This was actually raised in #30 and I think it would make sense to use the same generic header as oauth2_proxy:X-Forwarded-Access-Token

To prevent tampering, we will also need to include this in the hash that's prepended in the cookie, and verify the hash matches in ValidateCookie.

I'm also wondering if we should encrypt the value rather than leaving it in plain text in the cookie, I will have to do some reading in the specs/rfcs on that one and will update here when that's clear. Would you be willing to make the above changes?

@ImpThomasHein
Copy link

ImpThomasHein commented May 28, 2020

Hi,
thanks for work and effort.
When do you think you will finish the work on this?

@gcavalcante8808
Copy link
Author

@thomseddon I'm willing to finish this PR. Do you have some update about the spec reading?

@thomseddon
Copy link
Owner

Hey - I've done a bit more reading and I'm pretty positive we should be encrypting the tokens before storing them in the cookie (perhaps the email should be encrypted too?), please let me know if you think this may be unnecessary?

This shouldn't be too difficult to do, I'd also like to ensure the implementation is backwards compatible - so users who still have a cookie generated using the previous technique will not suddenly be flagged as having an invalid cookie.

That all sounds good?

@thomseddon thomseddon added this to the 2.3 milestone Aug 22, 2020
@thimiosgr
Copy link

thimiosgr commented Oct 8, 2020

Hi,

you have done an amazing work on the whole project.
Are you going to add the extra header for the access token? Is there another way that the username of the user is included in the response from the authentication authority?

@passi246
Copy link

passi246 commented Dec 2, 2020

Hey any updates here? Would be pretty helpful if this can be merged somehow soon.

@mnp
Copy link

mnp commented Aug 25, 2021

Is this PR going to move forward or should I look to maybe a Lua filter if I want to log fields from inside a token?

@gcavalcante8808
Copy link
Author

Hey - I've done a bit more reading and I'm pretty positive we should be encrypting the tokens before storing them in the cookie (perhaps the email should be encrypted too?), please let me know if you think this may be unnecessary?

This shouldn't be too difficult to do, I'd also like to ensure the implementation is backwards compatible - so users who still have a cookie generated using the previous technique will not suddenly be flagged as having an invalid cookie.

That all sounds good?

I wasn't sure about the changes needed to work, but it seems more clear now.

I feel that the last part of your comment can be considered as an answer to that other comment that you did, that said: I wonder if there's a more generic way to handle forwarding parameters from providers?.

What I Think: Maybe the allow list of forwarded headers + cookie structure, can be the generic way to avoid hard-coded allowed headers and, by allowing the user to configure the headers, you can pass on the responsibility about the cookie structure as well.

Copy link

@fdevillard fdevillard left a comment

Choose a reason for hiding this comment

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

I was passing by and saw the typo.

Cheers :)

@@ -320,6 +320,9 @@ Note, if you pass `whitelist` then only this is checked and `domain` is effectiv

The authenticated user is set in the `X-Forwarded-User` header, to pass this on add this to the `authResponseHeaders` config option in traefik, as shown [here](https://github.com/thomseddon/traefik-forward-auth/blob/master/examples/docker-compose-dev.yml).

The OIDC/JWT token is set in the `X-Oidc-Token` header, to pass this on add this to the `authResponseHeaders` config option in traefik; the value is encrypt,

Choose a reason for hiding this comment

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

Typo: encrypted*

@jdelker
Copy link

jdelker commented Oct 31, 2021

What exactly is preventing this PR to get merged?

@metaben
Copy link

metaben commented Dec 4, 2021

If I understand this feature correctly, I think I may be another person who will benefit.

I currently use Forward Auth with Auth0, where I have custom metadata fields in the user profile in the access token which are used to determine things like what URL a User is redirected to after logging in to auth.domain.com. Forward Auth sits in front of each subdomain so I'd like to be able to access those fields so that the bearer of an access token who was routed to x.domain.com can't access y.domain.com or z.domain.com.

Or is there a way I can use something like Cookie Domain? Thanks.

@gcaracuel
Copy link

I guess this is waiting to address this comment before it is merged #100 (comment) but at least in my case this is preventing my project to use this tool.

@gcavalcante8808 as it has been commented before I would use standard header introduced by oauth2_proxy X-Forwarded-Access-Token

@radicand
Copy link

radicand commented Nov 1, 2022

I'd also like to see support for passing the token, but with an option for a more standard Authorization: Bearer <token> format. I am migrating from a legacy forwardauth solution, and need this for Kubernetes Dashboard and other applications to pick up the authorization correctly.

@mzndr
Copy link

mzndr commented May 23, 2023

Any updates on this? I think this project does a pretty awesome job and is a pretty clean solution for people that want to use Traefik as an API gateway, but this feature is crucial for my project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.