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

FR: serve: Add the X-Forwarded-Proto header #7061

Closed
Zaba opened this issue Jan 25, 2023 · 9 comments · Fixed by #8224
Closed

FR: serve: Add the X-Forwarded-Proto header #7061

Zaba opened this issue Jan 25, 2023 · 9 comments · Fixed by #8224
Assignees
Labels
fr Feature request funnel Relating to Tailscale Funnel https://tailscale.com/blog/introducing-tailscale-funnel/ L2 Few Likelihood P2 Aggravating Priority level T0 New feature Issue type

Comments

@Zaba
Copy link

Zaba commented Jan 25, 2023

What are you trying to do?

I've tried to use tailscale serve to simplify my setup since I don't have any complex reverse proxying requirements, however I found that my backend really wants to have the X-Forwarded-Proto header set to https if the proxy terminated TLS on its behalf.

How should we solve this?

Since the Tailscale server knows whether it had terminated TLS, it seems like it should be fairly easy for it to insert the X-Forwarded-Proto header with the appropriate value when reverse-proxying the requests.

What is the impact of not solving this?

Some picky backends want to see the X-Forwarded-Proto: https header and will display warnings, etc., if it's missing.

Anything else?

No response

@Zaba Zaba added fr Feature request needs-triage labels Jan 25, 2023
@DentonGentry DentonGentry added funnel Relating to Tailscale Funnel https://tailscale.com/blog/introducing-tailscale-funnel/ L2 Few Likelihood P2 Aggravating Priority level T0 New feature Issue type and removed needs-triage labels Feb 12, 2023
@Zaba
Copy link
Author

Zaba commented Feb 22, 2023

I think this may be fairly simple to fix by adding a Rewrite hook that calls SetXForwarded to the ReverseProxy somewhere in https://github.com/tailscale/tailscale/blob/main/ipn/ipnlocal/serve.go#L436. Not sure if that will handle X-Forwarded-Proto correctly without any other changes though.

@DentonGentry
Copy link
Contributor

I believe this was fixed two weeks ago in 3177cca

@jamesez
Copy link

jamesez commented Apr 11, 2023

I see X-Forwarded-For but not X-Forwarded-Proto in that commit. I don't see -Proto in the source for serve.go at all; can you re-open?

@DentonGentry

@DentonGentry DentonGentry reopened this Apr 11, 2023
@jamesez
Copy link

jamesez commented Apr 11, 2023

A use-case for this feature is when testing OIDC authentication to a Rails application. Rails (in particular) can be fussy about whether it believes its on https or not and will redirect to https if it's not, and OIDC providers often won't let you have a redirection URL that is non-SSL if it's not 'localhost'.

Thank you!

@Multiply
Copy link

We could also use this option specifically for X-Forwarded-Proto but X-Forwarded-Host would also be a nice addition

@iandees
Copy link

iandees commented Apr 14, 2023

As another usecase for this, the Flask Python web framework (really I think Werkzeug under the covers?) uses this header to decide how to build URLs when using url_for(..., _external=True).

@zofrex zofrex self-assigned this Apr 14, 2023
@zofrex
Copy link
Contributor

zofrex commented Apr 14, 2023

We could also use this option specifically for X-Forwarded-Proto but X-Forwarded-Host would also be a nice addition

@Multiply could you expand a little on why X-Forwarded-Host would be a useful addition for you? My understanding is that this is only needed when a proxy rewrites the original Host header, which we aren't doing — the contents of the X-Forwarded-Host header would be the same as the Host header — but if there's a use-case for this we can look into it.

@Multiply
Copy link

Multiply commented Apr 15, 2023

We could also use this option specifically for X-Forwarded-Proto but X-Forwarded-Host would also be a nice addition

@Multiply could you expand a little on why X-Forwarded-Host would be a useful addition for you? My understanding is that this is only needed when a proxy rewrites the original Host header, which we aren't doing — the contents of the X-Forwarded-Host header would be the same as the Host header — but if there's a use-case for this we can look into it.

I'd assume it'd include the port, if it's not 80 for http, or 443 for https, so we that way can properly generate the 'correct' URLs, no matter how many layers of proxies one have, and for all supported ports.

Edit: I might have to read up on "standards", but X-Forwarded-Port also seems like an option, but I've seen most support for X-Forwarded-Host simply copying the Host header.

mKeRix added a commit to mKeRix/tailscale that referenced this issue May 27, 2023
This adds replicates the headers also sent by the golang reverse proxy
by default.

Fixes tailscale#7061

Signed-off-by: Heiko Rothe <me@heikorothe.com>
mKeRix added a commit to mKeRix/tailscale that referenced this issue May 27, 2023
This replicates the headers also sent by the golang reverse proxy by
default.

Fixes tailscale#7061

Signed-off-by: Heiko Rothe <me@heikorothe.com>
@mKeRix
Copy link
Contributor

mKeRix commented May 27, 2023

I also ran into this issue for a use case that I've been working on and ended up providing a PR that implements both the X-Forwarded-Host and X-Forwarded-Proto header: #8224.

mKeRix added a commit to mKeRix/tailscale that referenced this issue Jun 4, 2023
This replicates the headers also sent by the golang reverse proxy by
default.

Fixes tailscale#7061

Signed-off-by: Heiko Rothe <me@heikorothe.com>
DentonGentry pushed a commit that referenced this issue Jun 4, 2023
This replicates the headers also sent by the golang reverse proxy by
default.

Fixes #7061

Signed-off-by: Heiko Rothe <me@heikorothe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fr Feature request funnel Relating to Tailscale Funnel https://tailscale.com/blog/introducing-tailscale-funnel/ L2 Few Likelihood P2 Aggravating Priority level T0 New feature Issue type
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants