forwardAuth.addHeadersToResponse

[iamolegga]

What does this PR do?

closes #3660

Motivation

described in #3660

More

• Added/updated tests
• Added/updated documentation

Additional Notes

Before reviewing this please read this comment in related issue for understanding desired behaviour.

This PR CAN be merged, it does pass headers from forwardAuth service to response, but if backend has the same header, it overwrites values of the first one (Set-Cookies headers will be set from both services if have different names, if both responses have same Set-Cookie's name value from backend will be set).

Desired behaviour from my point of view is when values of headers from both services will be concatenated (through Add). So my question is to someone from the core team: what is the best way to deal with it? Or we can merge it now as it is, but implementing desired behaviour in next iteration will be breaking change

Thanks 🙂

[iamolegga]

Looks like tests not related to current changes have failed

[SantoDE]

Hey hey,

thanks for your work on that!

We're currently trying to figure out an answer for your question and I think, adding the Add functionality would be great as its a non destructive approach and would probably help us to avoid strange behavior.

Do you have the time to contribute that as well?

[iamolegga]

Hi @SantoDE thanks for your response!

Do you have the time to contribute that as well?

Sure, I would like to! Just need a bit of instructions on best way to implement it 🙂

[iamolegga]

Any updates @SantoDE @jbdoumenjou ? 🙂

[jbdoumenjou]

Hi @iamolegga,

Thanks for your contribution and sorry for the delay :)

After analysis, as the RFC says :

Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. (See Section 5.2 for how user agents handle this case.)

To be compliant, the Set-Cookie header received from the authentication server response should only be applied after checking the Set-Cookie header value sent by the backend. The right way to do that would be to use the ModifyResponse mechanism from go. However, there is currently no way to keep track of the headers from the authentication server until the handling of the backend response.

This is why, to accept this Pull Request, a technical way to apply the header a posteriori must be found.

Unless you have a better technical proposal, we will unfortunately close this PR.
What do you think ?

[iamolegga]

Hi @jbdoumenjou

Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. (See Section 5.2 for how user agents handle this case.)

good catch, I didn't see that before, thanks 👍

To be compliant, the Set-Cookie header received from the authentication server response should only be applied after checking the Set-Cookie header value sent by the backend.

I think that passing set-cookie header from auth service is expected behaviour, if we set it, right?
But in case of same name that came from both auth and backend, it would be useful if backend could overwrite default value from auth. Imagine some business logic on backend that could extend default business logic on auth service. Wdyt? 🙂

[jbdoumenjou]

As I said before, without a technical proposal compatible with the RFC, we are forced to close this PR.
Feel free to discuss it on the corresponding issue.
Thanks anyway.

[iamolegga]

Why closing this PR? My proposition is totally compliant with RFC @jbdoumenjou

[iamolegga]

@jbdoumenjou why are you closing pr that is still in progress, what's the reason? Please read again my previous messages, I just wanted to know if described behaviour could be merged, then I could change this pr to technical proposal compatible with the RFC

[kvncrw]

Hi @iamolegga,

The team has discussed your proposal to determine if it is compliant with RFC and it appears that it is not. For example, if you copy the header Set-Cookie: name=value from the auth server response and the backend response also includes the header Set-Cookie: name=value2, both will set in the Traefik response which is not compliant as explained here.

Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name.

As mentioned by @jbdoumenjou, to implement said functionality would require logic to handle the auth header being copied only if the backend response does not contain a Set-Cookie header with the same name. One approach would be to use the ModifyResponse mechanism from Go and there is currently no way to keep track of the headers from the auth server before the handling of the backend response.

As there is no technical proposal compatible with the RFC we were forced to close this PR. If you have ideas on how to implement this without breaking the RFC, we would welcome the discussion in the aforementioned issue that relates to this PR.

It is also possible to implement this functionality in custom middleware plugin where it isn't necessarily required to be compliant with the RFC specifications. If you'd like to learn more about how to build such a plugin, please reference the plugindemo repository.

If you have any additional questions or comments, feel free to respond here or in the aforementioned issue.

[iamolegga]

Hello @notsureifkevin

Sorry, but I didn't get it, are we discussing now only set-cookie header behaviour, right? Because according to current tests, set-cookie set by backend overwrite set-cookie with the same name set by auth. And if there is set-cookie with unique name from auth or backend it's set too. It is desired behaviour, isn't it?

My original question was about concatinating header values from both auth and backend (of course we must exclude set-cookie with same name here), but that could be extra feature, and simple overwriting that is already implemented for now could be enough from my point of view.

[iamolegga]

@notsureifkevin please check this lines

[kvncrw]

@iamolegga according to the team, the response would include multiple set-cookie headers, both of which could include duplicate name's.

[iamolegga]

@notsureifkevin um, but tests pass 😐

[ldez]

Hello,

If you swap the lines L81 and L82 in your integration test, you will see that it is not working:

forwardauth_test.go:62:
    c.Assert(err, checker.IsNil)
... value *fmt.wrapError = &fmt.wrapError{msg:"try operation failed: for header Set-Cookie got values [Bar=Auth Bar=Backend], wanted [Foo=Auth Bar=Backend]", err:(*errors.errorString)(0xc000359ca0)} ("try operation failed: for header Set-Cookie got values [Bar=Auth Bar=Backend], wanted [Foo=Auth Bar=Backend]")

This is related to the fact that in your implementation, only the first value of Set-Cookie header is copying to the response.

If you replace your implementation by the following code (which copies all the values of a header), you will see that your test (without any modification) will not work:

for _, headerName := range fa.addHeadersToResponse {
	for _, v := range forwardResponse.Header.Values(headerName) {
		rw.Header().Add(headerName, v)
	}
}

forwardauth_test.go:62:
    c.Assert(err, checker.IsNil)
... value *fmt.wrapError = &fmt.wrapError{msg:"try operation failed: for header Set-Cookie got values [Foo=Auth Bar=Auth Bar=Backend], wanted [Foo=Auth Bar=Backend]", err:(*errors.errorString)(0xc00052e910)} ("try operation failed: for header Set-Cookie got values [Foo=Auth Bar=Auth Bar=Backend], wanted [Foo=Auth Bar=Backend]")

So in this case, it cannot be compliant with the RFC.

Thanks anyway.