Skip to content

Commit

Permalink
This a followup commit for PR #1296
Browse files Browse the repository at this point in the history 
It adds changes related to Cookies as to why they are in the deny list.
It mentions the correct override behavior when setting same headers on
Ingress Controller and Route.
It also adds an alternative solution for the override behavior.
  • Loading branch information
@miheer
miheer committed Jul 27, 2023
1 parent 1b69d46 commit bf77874
Showing 1 changed file with 70 additions and 28 deletions.
98 changes: 70 additions & 28 deletions enhancements/ingress/set-delete-http-headers.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -735,26 +735,9 @@ another [option](https://docs.openshift.com/container-platform/4.12/networking/i
but have actual use-cases (which aren't covered by the existing option). They will be allowed, with notes in the API and product documentation,
and possibly a warning on admission.

For example: If someone creates an Ingress Controller with the following:
```sh
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
httpHeaders:
forwardedHeaderPolicy: Replace
```
Or if someone add a route annotation `haproxy.router.openshift.io/set-forwarded-headers`,
then the `set-header` stanzas will be added in `haproxy.config` using the [haproxy-config.template](https://github.com/openshift/router/blob/0cd9a58dff77127562f7851ccfa5cda79b8cef48/images/router/haproxy/conf/haproxy-config.template#L565-L580)

However, if someone wants to set custom values for these then they need to use the API of this EP.

The custom values set using `spec.httpHeaders.actions` will take over the value defined in annotation `haproxy.router.openshift.io/set-forwarded-headers` or `spec.httpHeaders.forwardedHeaderPolicy`.

Please note that the setting of forwardedHeaderPolicy happens at the plain backend section of haproxy.config i.e. per route level and not at the front-end section
of the haproxy.config level as per the above-mentioned link.
The custom HTTP response value of `x-forwarded` header set using `spec.httpHeaders.actions` of the IngressController will take over the value defined in annotation `haproxy.router.openshift.io/set-forwarded-headers` of Route, `spec.httpHeaders.forwardedHeaderPolicy` of Ingress Controller
or `spec.httpHeaders.actions` of Route as in case of HTTP response headers, Haproxy overrides the value of the backend of same header name which
was present in the frontend section.

Another use case is deleting the header which forwardedHeaderPolicy does not provide but this EP does which is the reason why we will allow users to set/delete
`x-forwarded-for` headers using this API.
Expand Down Expand Up @@ -782,6 +765,26 @@ For this reason, the `host` header is in the deny list of headers that are prohi
Please note that a custom `host` header may be specified via RouteSpec. Please refer to `Open Questions` for more details.
### Cookie and Set-Cookie
There are three reasons we blocked `Cookie` and `Set-Cookie`:
1. There were no use cases mentioned for these headers in [RFE](https://issues.redhat.com/browse/RFE-464).
2. The cookies that HAProxy sets are used for session tracking, to map a client connection to a particular
backend server.
The cookie name is an MD5 hash value derived from the backend's name (similarly, the cookie value is a hashed value
derived from backend server's endpoint).
So the cookie names that we would want to forbid are all the possible values
of the MD5 hashing algorithm, but it's extremely unlikely that someone would accidentally choose the same cookie name
that HAProxy used.

However, if there is an interference caused by setting these then either the HAProxy's session affinity might not work correctly,
or HAProxy might overwrite the cookie.
3. It was thought that it would be better to have a structured API for it.
The following table summarizes which headers are allowed to be customized via the Route API or IngressController API:
| Header name | Configurable using IngressController `spec.httpHeaders.actions` | Configurable using Route `spec.httpHeaders.actions` | Configurable using another API |
Expand All @@ -791,6 +794,7 @@ The following table summarizes which headers are allowed to be customized via th
| `strict-transport-security` | No | No | `haproxy.router.openshift.io/hsts_header` Route annotation |
| `x-forwarded-for` | Yes | Yes | `haproxy.router.openshift.io/set-forwarded-headers` Route annotation or IngressController `spec.httpHeaders.forwardedHeaderPolicy` API field |
| `x-ssl` | Yes | Yes | No |
| `cookie` or `set-cookie` | No | No | No |
### Drawbacks
Expand Down Expand Up @@ -889,7 +893,7 @@ FIELD: forwardedHeaderPolicy <string>
Answer: Yes, please refer to the `Risks and Mitigations` section for more details.

#### Can we please make sure to only add the `cache-control: private` header if there is no `cache-control: public` header already present in the response?
Answer: No. Many applications serve mixed content: static assets (images, js, css) and dynamic content (html, json).
Answer: No. Many applications serve mixed content: static assets (images, js, css) and dynamic content (html, json).

They usually want to cache the static assets on the client and do not care about session stickiness for those requests.

Expand Down Expand Up @@ -1002,16 +1006,27 @@ This route can be annotated with `haproxy.router.openshift.io/disable_cookies=tr
IngressController.
#### Why does a custom header value defined in RouteSpec override IngressControllerSpec value ?
Answer: It's quite common to have a global setting with an escape hatch locally to override the global setting.
The purpose of this EP is to define custom headers. So, there are two reasons we need such override:
1. A route owner shall have the choice to define what headers he wants as per his application requirements(as they might be varying)
than the ones admin has set cluster-wide.
2. The IngressController and Route might be owned by the same person who wants to specify some global configuration
and then override that global configuration on a small subset of routes.
Answer: In case of HTTP response headers, the headers set in spec.httpHeaders.actions of IngressController will override
the headers with same name in Route's in spec.httpHeaders.actions or Route annotation.
In case of HTTP request headers, the headers set in Route using spec.httpHeaders.actions or annotation will override the
headers with same name present in the IngressController's spec.httpHeaders.actions.
This is because of the way Haproxy is designed to work. We cannot change the processing order; we're constrained by HAProxy's design:
the frontend receives the request and dispatches it to the backend, and then the frontend gets the
response from the backend.
In case of HTTP response, Haproxy overrides the header value of same header name in backend with
the value of the header defined in frontend. So, [the backend rules are applied first, followed by the frontend's rules.](https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#4.2-http-response)
The `http-reponse set or delete` stanzas are applied and interpreted in this way by the haproxy.
However, in case of the HTTP request, haproxy overrides the values of the header value of the same header name present in the frontend with the value of the
header defined in backend. [The rules are evaluated in their declaration order when they are met in a frontend,
listen or backend section.](https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#4.2-http-request)
So, the `http-request set or delete` stanzas are applied and interpreted this way by the haproxy.
#### Does a custom header value defined in RouteSpec override the route annotation ?
Answer: Yes. A value defined in RouteSpec overrides the route annotation.
For Example: Please refer to `X-Forwarded-For` section under `Risks and Mitigations` for more details.
#### So, we allowed there to be an additional way of specifying x-forwarded-for as well? Are there any plans to deprecate the first way?
Answer: We don't have any plans to deprecate the existing way of setting forwarded headers using route annotation and ingress controller.
Expand Down Expand Up @@ -1123,6 +1138,33 @@ We have multiple options for quoting header values in `haproxy.config`, as [desc
2. Wrap the entire string in single-quotes.
The HAProxy documentation refers to this as _strong quoting_. Using strong quoting allows us to keep the quoting logic simple and `haproxy.config` more human-readable compared to the alternatives.
### Proposed Alternative Solution for overriding the HTTP response headers defined in the IngressController by the Route
As per HAProxy's design: the frontend receives the request and dispatches it to the backend, and then the frontend
gets the response from the backend.
So, the override behavior happens the way mentioned in question `Why does a custom header value defined in RouteSpec override IngressControllerSpec value ?`
under section `Open Questions` where HTTP Response headers defined in the IngressController are override by headers
defined in the Route.
However, if we want the same behavior of override for HTTP response headers as that of HTTP request headers then we will have
to inject the set-header from the IngressController config into every backend and not the frontend.
The downside is that it would grow the generated haproxy.config significantly.
The upside is:
It would give us better control over the ordering.
It would eliminate the issue with the host header so, we could allow it to be set via IngressController.
The reason for this proposal was that it's quite common to have a global setting with an escape hatch locally to
override the global setting.
The purpose of this EP is to define custom headers. So, there are two reasons we need such override:
1. A route owner shall have the choice to define what headers he wants as per his application requirements(as they might be varying)
than the ones admin has set cluster-wide.
2. The IngressController and Route might be owned by the same person who wants to specify some global configuration
and then override that global configuration on a small subset of routes.
## Infrastructure Needed
This feature requires no special infrastructure.

0 comments on commit bf77874

Please sign in to comment.