http-response set-header does not work if http-request redirect applies about haproxy HOT 10 CLOSED

This is resolved with http-after-response set-header in 2.2.

from haproxy.

That's commit 6d0c3df ("MEDIUM: http: Add a ruleset evaluated on all responses just before forwarding"), and it's in v2.2-dev2.

@capflam @wtarreau How intrusive do you feel like this patch is? I'm wondering if we could backport this, maybe just to 2.1? This seems like an important feature, that people requested over the years.

If this is backported, we need to make sure the backported documentation explains when this feature has been introduced in that stable branch, otherwise people won't understand why it won't work for their older release (for example, the doc for the 2.1 branch should mention that this is supported since 2.1.4 - if we backport before that point release).

from haproxy.

I was unaware of the dependencies, I fully agree to not backport then.

It would have been different if the patch was enough standalone, but not with that amount of changes required.

from haproxy.

See also this mailing list discussion where the bug was reported first: https://www.mail-archive.com/[email protected]/msg29294.html

from haproxy.

A reg-test reproducing the issue is the following:

varnishtest "http-response set-header + http-request redirect"
feature ignore_unknown_macro

server s1 {
   rxreq
   txresp
} -start

haproxy h1 -conf {
  defaults
    mode http
    ${no-htx} option http-use-htx
    log global
    option httplog
    timeout connect         15ms
    timeout client          20ms
    timeout server          20ms

  frontend fe1
    bind "fd@${fe1}"
    http-response set-header foo bar
    http-request redirect location https://example.com if { path /redirect }

    default_backend be1

  backend be1
    server s1 ${s1_addr}:${s1_port}
} -start

client c1 -connect ${h1_fe1_sock} {
    txreq -url "/no-redirect"
    rxresp
    expect resp.status == 200
    expect resp.http.foo == bar

    txreq -url "/redirect"
    rxresp
    expect resp.status == 302
    expect resp.http.foo == bar
} -run

from haproxy.

Please note that this works exactly as designed and documented and this behavior will not change as this would break a lot of configurations. From the doc :

Header transformations only apply to traffic which passes through HAProxy,
and not to traffic generated by HAProxy, such as health-checks or error
responses.

We need to turn this issue into a feature request instead and to remove all the misleading "affects:" labels. I've personally added it to my TODO list a while ago and the problem is more a matter of configuration than anything : how do we want to make the extra header fields configurable on the redirect line ? I have some ideas on the subject but they cannot be developed in this issue as we'd lose them.

As such this issue is invalid. I don't know if we can change its description to turn it into a new feature request, or if we need to close it as invalid and open a new one.

from haproxy.

As such this issue is invalid. I don't know if we can change its description to turn it into a new feature request, or if we need to close it as invalid and open a new one.

I updated the description of the issue to word it as a feature request, updated the labels and hidden my previous comments. Thus the issue history is preserved, but it is no longer misleading.

how do we want to make the extra header fields configurable on the redirect line ? I have some ideas on the subject but they cannot be developed in this issue as we'd lose them.

I really wish that it does not require a new directive and / or duplication of configuration. Possibly a compatility_level setting similar to Postfix would allow users to opt into the changed behaviour of passing HAProxy generated traffic through the normal response (header) processing?

from haproxy.

I updated the description of the issue

thank you!

Possibly a compatility_level ...

No it's not related to this, it's a matter of how the whole stuff works. There simply is not any response at all to deal with while processing requests. A redirect is in fact an error which is pushed back to the client. It would be more than a major change to do this differently. It would require redirects to be performed only on the response path and to pretend there was a response while there was not. For example your http-response rules would not be able to block undesired server responses anymore without blocking your own pre-built responses. You'd get stickiness cookies emitted after your redirects, or entries counted into stick-tables as server errors while they're not. All of this would not make sense at all. And in addition this would only help for some isolated cases without dealing with them all, at the expense of breaking a lot of user useful stuff.

I've been thinking about a few possibilities instead. The first one was to move the redirect to an applet, just like we now do for stats. But then other users will complain that they can add headers to redirects and not to deny or to error responses. And we can obviously not move everything there, for the reason that these would not be usable when dealing with responses, that having such heavy processing when blocking requests under a DoS would instead amplify the DoS, and that it would still cause lots of confusion with headers processing rules having to apply both to servers and internally-generated traffic. By the way we already support "http-response redirect" and it couldn't be supported this way.

The other idea I was having on the subject, since it's most always a matter of adding headers, would be to implement the notion of headers list. These would work exactly like variables except that we would use them as a collection of headers to be added to a request or response at the end of the processing. This would allow to improve almost all request/response processing regardless of their direction and the fact that they are final rules or not.

It could look like this :
http-request add-header-list(res) Server Foo
http-request add-header-list(res) HSTS bar

and later, a final rule could optionally make use of them :
http-request redirect location /overthere hdr-list res
or :
http-request deny code 429 hdr-list come-back-later if [ condition ]

We could even imagine supporting them for addition at the end of a number of rules :
http-request add-headers-from-list list-name

We could later imagine having some specific lists always added with certain error responses, such as "error", "error403" etc. For errors it would still be limited by the fact that some errors are produced outside of the rules path. But we also know that there's been a long demand for error page templates so that could be addressed there.

So we need to put more thinking into this in my opinion, and this is a design discussion that should move to the list. We can keep this entry open so that we don't forget this item.

from haproxy.

Well, this commit directly depends on few others but looking more deeply, it is based on the refactoring of the HTTP actions and part of refactoring of the errorfiles. So to backport this feature, around twenty patches or more must be backported too. Honestly, it is too much pain :) The 2.2.0 is coming soon. It could be a good reason to upgrade.

Anyway, for now, I'm busy on the checks refactoring. But if there is really a demand for this feature in 2.1, the best will be to work on a specific patchset. But it should be discussed with Willy first :)

from haproxy.

I'm not against a backport into 2.1 however we must be careful not to break it. Knowing that it relies on 20+ patches doesn't reassure me. If we could have a "quick and dirty" alternative I'd be less worried. Also it's worth keeping in mind that the time invested on backports is not invested on development, so we must weigh this as well :-/

from haproxy.