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

allow preserving the originally requested path after the authentication redirect #335

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

colearendt
Copy link

In most (all?) cases, traefik makes the forwardAuth request at the root of the server (/). This means that the user's requested path is lost in the authentication redirect. However, the prefix is sent along with the X-Forwarded-Prefix header, so we use that in addition to the requested path if the requested path is empty.

https://github.com/traefik/traefik/blob/c57876c116ebc375becaff476fbcf6f25c4db7f3/pkg/middlewares/auth/forward.go#L57

https://github.com/traefik/traefik/blob/c57876c116ebc375becaff476fbcf6f25c4db7f3/pkg/middlewares/auth/forward.go#L102

close #323
close #325

internal/auth.go Outdated
func returnUrl(r *http.Request) string {
return fmt.Sprintf("%s%s", redirectBase(r), r.URL.Path)
fwdUri := r.URL.Path
Copy link

Choose a reason for hiding this comment

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

I'm currently running into a related issue. For us we have in r.URL the correct page that we initially try to access. however if that URL has a query parameter, e.g. /myhorus/editor?open=123 then that ?open=123 is ommited and not there after going through authentication. Have you tried to r.URL.RequestURI for surch cases?

Copy link
Author

@colearendt colearendt Dec 3, 2022

Choose a reason for hiding this comment

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

I noticed that as a missing component 😞

Unfortunately I don't think that will work without traefik changes. The problem is that traefik doesn't send along that information, so the traefik-forward-auth can't put it in the redirect and therefore the redirect can't restore it.

The problem is illustrated in the two links above. traefik just uses the "configured" address directly and does not include information about the original request.

My suspicion is that to fix the query parameter issue, traefik would either need to:

  1. Append the Path / Query Params onto the GET request for the forward auth check
  2. Add a header that forwards the query parameters so that traefik-forward-auth can grab and use those headers

The other option is that it may be possible to write a custom forward auth middleware using the existing as a baseline, now that traefik has a mechanism for doing so. Forking traefik and building (2) would be pretty straightforward, I would think - I would expect (1) may be trickier.

Copy link

Choose a reason for hiding this comment

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

I can not confirm this. For me traefik is sending the actually requested URL to the traefik-forward-auth. I further changed the line as follows:

  •   return fmt.Sprintf("%s%s", redirectBase(r), r.URL.Path)
    
  •   return fmt.Sprintf("%s%s", redirectBase(r), r.URL.RequestURI())
    

Then the query part is also preserved.
I think you're lacking something else in your setup if it doesn't work for you. This is with traefik 2.6.6 for me

Copy link
Author

@colearendt colearendt Dec 4, 2022

Choose a reason for hiding this comment

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

Good to know!! I'll do a bit more testing. If that is the case generally, it will be welcome news to me! 😄

Copy link
Author

@colearendt colearendt Dec 5, 2022

Choose a reason for hiding this comment

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

You're right! URL.RequestURI() preserves the query... but not the path 😑 I'll try to see if I can figure out a hybrid approach

URL.Path: /
RequestURI: /
URL.RequestURI(): /?query=hi
X-Forwarded-Prefix: /prefix

Copy link
Author

@colearendt colearendt Dec 5, 2022

Choose a reason for hiding this comment

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

Actually... it looks like RequestURI() can maintain some of the Request. Perhaps we have another middleware monkeying around here:

URL.Path: /hello/friend/
RequestURI: /
RequestURI(): /hello/friend/?query=hello
X-Forwarded-Prefix: /prefix

EDIT: Confirmed. We had a StripPrefix middleware set up as higher priority before Auth. RequestURI() looks like the way to go!! And Path would have been working had we not had that middleware set up 🙈

URL.Path: /daily-workbench
RequestURI: /
RequestURI(): /daily-workbench?query=test
X-Forwarded-Prefix: 

Copy link

Choose a reason for hiding this comment

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

If it helps, this is what we do in the middleware:
image

maybe it works for us because we do not strip anything away but just path the prefix through

When redirecting, URL query parameters are currently dropped
by referencing URL.Path. This PR changes to use RequestURI() instead.
@Menschomat
Copy link

Will this ever lead to a new release?

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