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

New X-Forwarded-Prefix headers should contain route path only #6190

Closed
rainest opened this issue Aug 5, 2020 · 5 comments
Closed

New X-Forwarded-Prefix headers should contain route path only #6190

rainest opened this issue Aug 5, 2020 · 5 comments

Comments

@rainest
Copy link
Contributor

rainest commented Aug 5, 2020

Summary

See #5620 and specifically https://github.com/Kong/kong/pull/5620/files#diff-8cb7654e2e5f4b45e64e3cd2afdcfcd9R239-R271 for current behavior.

Based on the report in Kong/kubernetes-ingress-controller#784 and a brief reading of that code, Kong will either send an existing X-Forwarded-Prefix header from a trusted sender or will generate a new X-Forwarded-Prefix header containing the entire path.

Though unclear, I believe the headers that Kong generates itself should contain the route path only, i.e. run https://docs.konghq.com/2.1.x/pdk/kong.router/#kongrouterget_route, extract the path, and set X-Forwarded-Prefix to that.

Steps To Reproduce

  1. Create a route+service using an echo service (httpbin or similar).
  2. Send a request through the route, and inspect the X-Forwarded-Prefix header received upstream

Additional Details & Logs

From the original report, CE 2.0.4 running on EKS 1.17. Exact route config used is available in Kong/kubernetes-ingress-controller#784 but I believe this should affect any route equally; exact config shouldn't matter.

The correct behavior here is unclear. Based on the header name alone, I agree with the controller report: this should send the Kong route path prefix only, or the portion of the request matching it in the case of regex paths. However, this header is non-standard, and I cannot find anything that appears to be an authoritative source on its behavior. There are several open questions:

  • If we receive an X-Forwarded-Prefix header from a trusted downstream, should we combine it with our route prefix in any way? As-is we use the downstream value exclusively.
  • For regex paths, should we forward the matched string, or the original regex?
@rainest
Copy link
Contributor Author

rainest commented Aug 5, 2020

@jerfer do you know of an authoritative source for this header's behavior, or absent that, any source that describes this header's behavior? We should probably be able to learn more about it by reviewing the source of other proxies, but it'd be useful to know if there is something close to an authoritative source.

@jerfer
Copy link

jerfer commented Aug 5, 2020

@rainest Here you go, I found one of each.

Does that work for you?

@bungle
Copy link
Member

bungle commented Aug 6, 2020

This is duplicate of #6132

@bungle
Copy link
Member

bungle commented Aug 7, 2020

@rainest / @jerfer,

There is now a PR #6201 to fix this. Can you please try it out and report any issues / give feedback, thank you!

bungle added a commit that referenced this issue Aug 7, 2020
…warded_prefix

### Summary

The PR #5620 implemented `X-Forwarded-Prefix` wrong as reported in these:
- #6190
- Kong/kubernetes-ingress-controller#784
- #6132

And also by some other sources.

This commit re-implements `X-Forwarded-Prefix`:

1. Is there trusted `X-Forwarded-Prefix` header in request? If yes, then set
   that as the `X-Forwarded-Prefix` (note: at this point we don't support
   multi-value `X-Forwarded-Prefix` headers, e.g. appending. We don't support
   multi-value in `X-Forwarded-Host/-Port/-Proto` either.
2. Has Kong router stripped the prefix of the request URI before proxying, if
   yes, then set `X-Forwarded-Prefix` to match the stripped prefix.
3. If 1 or 2 didn't set the header, set it to `/`.

### Issues Resolved

Fix #6190
Fix Kong/kubernetes-ingress-controller#784
Fix #6132
@gszr
Copy link
Member

gszr commented Aug 12, 2020

Closing this issue, let's please keep the discussion in #6201. Thanks for reporting, @rainest!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants