traefik icon indicating copy to clipboard operation
traefik copied to clipboard

add forwardAuth.addAuthCookiesToResponse

Open tgunsch opened this issue 3 years ago • 1 comments

What does this PR do?

Add the possibility to configure cookie names on ForwardAuth middleware, which gets copied from auth response to the client response. This will solve the issue, that the auth server can't refresh cookies, as described in #3660

Motivation

Based on the work from @iamolegga in PR #6893 and the comment https://github.com/traefik/traefik/issues/3660#issuecomment-896031575 in #3660 I implemented this solution.

More

  • [x] Added/updated tests
  • [x] Added/updated documentation

Additional Notes

  • As all posts in the original issue #3660 "only" needs the Set-cookie header, and not any other header from auth server response, the PR only copy cookies.
  • As a new cookie from auth server can be security relevant (i.e. a changed session cookie), this cookie has always a higher priority and will overwrite a cookie with same name from a backend.

Thomas Gunsch [email protected] Mercedes-Benz Tech Innovation GmbH (ProviderInformation)

tgunsch avatar Apr 06 '22 14:04 tgunsch

Hello @tgunsch,

Thanks for your interest in Traefik and for your contribution!

This PR is labeled as needs-design-review, as you can see in our contribution guide, this means that we have to take a longer look to evaluate how it would interact with the other parts of Traefik.

We will come back to you once the design review iteration is done.

rtribotte avatar Jul 20 '22 12:07 rtribotte

Shouldn't be taken as this being accepted, and I can't talk on behalf of the authors, however this is a better pattern as it reduces complexity and indentation in my opinion. Also the check becomes part of the newup of the ForwardAuth rather than on every request.

Was just passing through and noticed this.

Hi @james-d-elliott , thanks for you feedback. I was on vacation last three weeks and we will have a look on it next few days.

tgunsch avatar Oct 13 '22 08:10 tgunsch

Have the same needs https://github.com/traefik/traefik/issues/3660 and am trying to understand where this PR is at and how I could be of help. Is it still awaiting design review?

nferch avatar Mar 06 '23 21:03 nferch

I just wanted to let everyone know that we are interested in moving forward with this, but we are currently prioritizing a different focus.

If you want to help us move this forward faster, we would love help with a review. How it would work, assessments on what the risks are, what areas will be impacted, and writing tests to evaluate this would be a huge help. @nmengin can give more detailed information if you are interested.

tfny avatar Mar 27 '23 14:03 tfny

I just wanted to let everyone know that we are interested in moving forward with this, but we are currently prioritizing a different focus.

If you want to help us move this forward faster, we would love help with a review. How it would work, assessments on what the risks are, what areas will be impacted, and writing tests to evaluate this would be a huge help. @nmengin can give more detailed information if you are interested.

Happy to help here. I've tested the changes for my specific use case but can do a more comprehensive review, please let me know what I should be looking at.

nferch avatar Apr 03 '23 16:04 nferch

Hello @tgunsch,

Thanks for the contribution and sorry for the delay, we are moving this pull request to the status/2-needs-review state, to have it an upcoming Traefik version.

To move this pull request forward, can we take over it to make some changes?

kevinpollet avatar Jan 08 '24 15:01 kevinpollet

Hello @tgunsch,

Thanks for the contribution and sorry for the delay, we are moving this pull request to the status/2-needs-review state, to have it an upcoming Traefik version.

To move this pull request forward, can we take over it to make some changes?

Hi @kevinpollet ,

perfect, thank you very much. For sure you can take the PR over. We waiting eagerly for the upcoming version :partying_face:

tgunsch avatar Jan 08 '24 15:01 tgunsch