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 configuring Forwarded/X-Forwarded-* headers #134

Conversation

Miciah
Copy link
Contributor

@Miciah Miciah commented Jun 3, 2020

Allow configuring Forwarded/X-Forwarded-* headers

Implement the ROUTER_SET_FORWARDED_HEADERS environment variable and haproxy.router.openshift.io/set-forwarded-headers route annotation. The annotation takes precedence over the environment variable. The environment variable and annotation may specify any of the following values to control when HAProxy sets the Forwarded and X-Forwarded-* headers:

  • append to append to existing headers.
  • replace to replace existing headers.
  • if-none to set the headers if they are absent.
  • never to never set headers.

The default setting is append.

  • images/router/haproxy/conf/haproxy-config.template: Add a new variable, setForwardedHeadersPattern, with a regular expression that matches the values listed above. Add a new variable, setForwardedHeadersAnnotation, with the new annotation. Add a new variable, setForwardedHeadersDefaultValue, with value of ROUTER_SET_FORWARDED_HEADERS if specified or else the default value append. Use these variables to control when the Forwarded and X-Forwarded-* headers are set for the backend.

This change is related to NE-343.

Delete stray comment in haproxy-config.template

Delete the "Add x-forwarded-for header." comment that was added in openshift/origin@d74ebe6.

  • images/router/haproxy/conf/haproxy-config.template: Delete comment.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2020
@Miciah Miciah changed the title Allow configuring Forwarded/X-Forwarded-* headers Allow configuring Forwarded/X-Forwarded-* headers Jun 3, 2020
@Miciah Miciah changed the title Allow configuring Forwarded/X-Forwarded-* headers Allow configuring Forwarded/X-Forwarded-* headers Jun 3, 2020
Implement the ROUTER_SET_FORWARDED_HEADERS environment variable and
haproxy.router.openshift.io/set-forwarded-headers route annotation.  The
annotation takes precedence over the environment variable.  The environment
variable and annotation may specify any of the following values to control
when HAProxy sets the Forwarded and X-Forwarded-* headers:

* "append" to append to existing headers.
* "replace" to replace existing headers.
* "if-none" to set the headers if they are absent.
* "never" to never set headers.

The default setting is "append".

This commit is related to NE-343.

https://issues.redhat.com/browse/NE-343

* images/router/haproxy/conf/haproxy-config.template: Add a new variable,
setForwardedHeadersPattern, with a regular expression that matches the
values listed above.  Add a new variable, setForwardedHeadersAnnotation,
with the new annotation.  Add a new variable,
setForwardedHeadersDefaultValue, with value of ROUTER_SET_FORWARDED_HEADERS
if specified or else the default value "append".  Use these variables to
control when the Forwarded and X-Forwarded-* headers are set for the
backend.
Delete the "Add x-forwarded-for header." comment that was added in
<openshift/origin@d74ebe6>.

* images/router/haproxy/conf/haproxy-config.template: Delete comment.
@Miciah Miciah force-pushed the allow-configuring-forwarded-and-x-forwarded-headers branch from be00278 to 447fe9f Compare July 9, 2020 21:20
@sgreene570
Copy link

🚀
Looks good!
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, sgreene570

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 18, 2020

@Miciah: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-upgrade 447fe9f link /test e2e-aws-upgrade

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit afcf368 into openshift:master Jul 18, 2020
@merusso
Copy link

merusso commented Oct 2, 2020

@Miciah, I'm curious about this implementation behavior for X-Forwarded-Host/Port/Proto and append mode.

Since append doesn't make sense for these headers (they can only have one value), append acts the same as replace for these headers. In other words, it does not honor "existing headers" and instead overrides them. Am I reading this right?

Wouldn't it be better if append worked like if-none for these headers?

@Miciah
Copy link
Contributor Author

Miciah commented Oct 6, 2020

@Miciah, I'm curious about this implementation behavior for X-Forwarded-Host/Port/Proto and append mode.

Since append doesn't make sense for these headers (they can only have one value), append acts the same as replace for these headers. In other words, it does not honor "existing headers" and instead overrides them. Am I reading this right?

I'm not sure that's true: Why couldn't an intermediate proxy could modify the host, port, or protocol?

Wouldn't it be better if append worked like if-none for these headers?

Could you provide an example scenario where appending would be the wrong thing to do? Unfortunately the x- headers are nonstandard, so we have to go by what makes sense, and here, I think treating the x- headers and the standard Forwarded header consistently and honoring the behavior specified by the ROUTER_SET_FORWARDED_HEADERS environment variable or haproxy.router.openshift.io/set-forwarded-headers annotation makes the most sense.

@merusso
Copy link

merusso commented Oct 14, 2020

These headers are meant to preserve the values that are sent from the client's originating request. There may be layers of load balancers between a client and the downstream server that handles a request, and this server relies on these headers to know the values sent by the client. References:

Could you provide an example scenario where appending would be the wrong thing to do?

So imagine the following:

  • We have a setup where there's a load balancer sitting between clients and the OpenShift cluster
  • This load balancer strips HTTPS, sending HTTP to the cluster but preserving the client values with X-Forwarded-* headers
  • There's an application running in OpenShift which handles requests for the public-facing endpoint "https://myapp.openshift.company.com"

When receiving requests, this app wants to know what protocol/port a client uses to connect to this endpoint. It can know this by looking at the X-Forwarded-Proto/Port headers passed to it from upstream systems, but this is only true if all upstream systems preserve these values passed by the client. In this router implementation, the load balancer will pass "X-Forwarded-Proto: https, X-Forwarded-Port: 443", but the router will overwrite these headers with "X-Forwarded-Proto: http, X-Forwarded-Port: 80", losing the real values passed by the originating client to the public-facing endpoint, so the application won't know if the client is actually connecting over HTTP or HTTPS.

Instead, if the router preserved these headers, the app would receive "X-Forwarded-Proto: https, X-Forwarded-Port: 443" and know that the client connected to the public-facing endpoint via HTTPS:443.

You might say that we should use the mode if-none instead of append in this scenario, but we would also like for X-Forwarded-For to append, which is the expected behavior for this header. IMO, the ideal default behavior is for all "Forwarded" headers to first and foremost preserve the values from the originating client request. In other words, X-Forwarded-For appends and X-Forwarded-Host/Port/Proto preserves.

@Miciah
Copy link
Contributor Author

Miciah commented Oct 14, 2020

These headers are meant to preserve the values that are sent from the client's originating request. There may be layers of load balancers between a client and the downstream server that handles a request, and this server relies on these headers to know the values sent by the client. References:

These references contradict each other in various small ways, which reinforces my point that these headers are non-standard and that implementations are inconsistent. MDN mentions several variants of X-Forwarded-Proto, Pivotal makes the seemingly bogus claim that X-Forwarded-Proto is "part of the HTTP standard", AWS says that X-Forwarded-For is single-valued, and Nginx says that it doesn't properly implement multi-valued Forwarded. All this goes to show that it is difficult to make strong claims or determinations about how these headers should be set.

Could you provide an example scenario where appending would be the wrong thing to do?

So imagine the following:

  • We have a setup where there's a load balancer sitting between clients and the OpenShift cluster
  • This load balancer strips HTTPS, sending HTTP to the cluster but preserving the client values with X-Forwarded-* headers
  • There's an application running in OpenShift which handles requests for the public-facing endpoint "https://myapp.openshift.company.com"

When receiving requests, this app wants to know what protocol/port a client uses to connect to this endpoint. It can know this by looking at the X-Forwarded-Proto/Port headers passed to it from upstream systems, but this is only true if all upstream systems preserve these values passed by the client. In this router implementation, the load balancer will pass "X-Forwarded-Proto: https, X-Forwarded-Port: 443", but the router will overwrite these headers with "X-Forwarded-Proto: http, X-Forwarded-Port: 80", losing the real values passed by the originating client to the public-facing endpoint, so the application won't know if the client is actually connecting over HTTP or HTTPS.

The router only replaces the header values if the policy is set to "replace".

Granted, in your example, the server would need to use the first value of each header; it wouldn't work properly with the "append" policy if the server used the last value. Is that the concern?

Instead, if the router preserved these headers, the app would receive "X-Forwarded-Proto: https, X-Forwarded-Port: 443" and know that the client connected to the public-facing endpoint via HTTPS:443.

You might say that we should use the mode if-none instead of append in this scenario, but we would also like for X-Forwarded-For to append, which is the expected behavior for this header. IMO, the ideal default behavior is for all "Forwarded" headers to first and foremost preserve the values from the originating client request. In other words, X-Forwarded-For appends and X-Forwarded-Host/Port/Proto preserves.

What would be a reasonable way to approach this? Change the behavior of "append" to append only Forwarded and X-Forwarded-For but preserve the other headers (which would make it inconsistent with the other policies), change the behavior of all policies (which might surprise users and not be what they want), add a new policy (any suggestions for names?), or something else?

@merusso
Copy link

merusso commented Oct 14, 2020

These references contradict each other in various small ways, which reinforces my point that these headers are non-standard and that implementations are inconsistent. MDN mentions several variants of X-Forwarded-Proto, Pivotal makes the seemingly bogus claim that X-Forwarded-Proto is "part of the HTTP standard", AWS says that X-Forwarded-For is single-valued, and Nginx says that it doesn't properly implement multi-valued Forwarded. All this goes to show that it is difficult to make strong claims or determinations about how these headers should be set.

Great points. MDN calls these headers "de-facto standard", i.e. not an official standard but a common practice. The official standard Forwarded header was added later, isn't supported everywhere, and doesn't have a replacement for X-Forwarded-Port.

The router only replaces the header values if the policy is set to "replace".

Granted, in your example, the server would need to use the first value of each header; it wouldn't work properly with the "append" policy if the server used the last value. Is that the concern?

This is true for X-Forwarded-For (source IPs), but X-Forwarded-Host/Port/Proto aren't being appended as you describe, if I'm reading this PR correctly, and I've never heard of any server implementation that would expect multiple values in these headers.

What would be a reasonable way to approach this? Change the behavior of "append" to append only Forwarded and X-Forwarded-For but preserve the other headers (which would make it inconsistent with the other policies), change the behavior of all policies (which might surprise users and not be what they want), add a new policy (any suggestions for names?), or something else?

Great question. First off, are we in agreement that "append" doesn't apply to X-Forwarded-Host/Port/Proto? Specifically it applies to X-Forwarded-For and the for directive in the Forwarded header.

So "append" currently means:

  • For X-Forwarded-For and Forwarded: for=, append values
  • For other forwarded headers, behave the same as replace

I think a better and less surprising behavior would be:

  • For X-Forwarded-For and Forwarded: for=, append values
  • For other forwarded headers, behave the same as if-none

I may be short-sighted here, but I don't think anyone will want the current append behavior for X-Forwarded-Host/Port/Proto (same as replace). IMO, the spirit of these headers is to preserve the URL values coming from the original client request. When intermediate load balancers / reverse proxies are added between the client and server, the original client values are lost. This is why I think the default behavior should be to preserve these headers.

If you really don't want to change the current behavior, though, could we perhaps split "append" into 2 policies?

  1. append-replace uses the current behavior
  2. append-preserve uses the append+if-none behavior

@merusso
Copy link

merusso commented Oct 19, 2020

Hey @Miciah , any thoughts on the changes I suggested above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants