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

Support the new "Forwarded" header (RFC 7239) #5978

Open
cwe1ss opened this issue Jan 27, 2016 · 18 comments
Open

Support the new "Forwarded" header (RFC 7239) #5978

cwe1ss opened this issue Jan 27, 2016 · 18 comments
Labels
affected-medium This issue impacts approximately half of our customers area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-forwarded-headers This issue is related to forwarded headers severity-minor This label is used by an internal tool
Milestone

Comments

@cwe1ss
Copy link
Contributor

cwe1ss commented Jan 27, 2016

There is now a standard for the most common X-Forwarded-* headers. It was introduced with RFC 7239: "Forwarded HTTP Extension".

Example: Forwarded: for=192.0.2.60;proto=http;by=203.0.113.43

It would be great if this could be supported.

@Tratcher
Copy link
Member

Yes, we've seen it. Are you aware of any proxies that actually set that new header?

@cwe1ss
Copy link
Contributor Author

cwe1ss commented Jan 27, 2016

No - but I haven't looked either. I recently built a small ASP.NET gateway proxy for Azure Service Fabric and that's why I stumbled upon this RFC. I guess this definitely isn't high priority since there won't be anyone who's setting just this header for a looong time. It would just be good to support it someday since it is a standard and will eventually get more common.

@lantrix
Copy link

lantrix commented Mar 3, 2016

I have a scenario whereby we have a reverse proxy (internally) passing requests to backend api’s and web apps; and one of the api's is ASP.NET 5 and using the BasicMiddleware classes to handle the headers from the upstream proxies. The team had implemented the newer RFC so BasicMiddleware can't handle Forwarded headers from the RFC.

@asbjornu
Copy link
Member

asbjornu commented Jan 3, 2018

We're using Forwarded and plan to implement it in the proxies we use, such as NGINX. The header is becoming more mainstream and it would be good of Microsoft to advocate for its use over the (non-standard) X-Forwarded-* headers.

@damianh
Copy link

damianh commented Dec 2, 2018

Would also like to have ability to extend the 'Forwared' header with a PathBase parameter as well as support for that with the current implementation i.e. X-Forwarded-PathBase

(somewhat) Related: https://github.com/aspnet/Hosting/issues/1120

@Tratcher
Copy link
Member

Tratcher commented Dec 2, 2018

@damianh do you expect PathBase to be different per request? That Hosting issue is about getting one value at startup.

@damianh
Copy link

damianh commented Dec 2, 2018

Yes, I would certainly like to be able to do so on a per-request basis. This will allow two scenarios where generated URLs and paths (fully qualified, rooted, relative) would "just work":

  1. Re-configuring the proxy to a new path base (e.g. http://proxy/foo/ -> http://backend/ to http://proxy/bar/ -> http://backend/ ) without having to reconfigure and restart the backend server.

  2. Having one backend server responding on two separate paths (e.g. http://proxy/foo/ | http://proxy/bar/ -> http://backend/). Useful if "migrating" to a new route without breaking clients in situ.

@damianh
Copy link

damianh commented Dec 2, 2018

@Tratcher This is what I use near top of my Configure(IApplicationBuilder app):

        var forwardedHeadersOptions = new ForwardedHeadersOptions
        {
            ForwardedHeaders = ForwardedHeaders.All
        };
        app.UseForwardedHeaders(forwardedHeadersOptions);
        app.Use((context, next) => {
            if (context.Request.Headers.TryGetValue("X-Forwarded-PathBase", out var pathBases))
            {
                context.Request.PathBase = pathBases.First();
            }
            return next();
        });

@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 19, 2018
@aspnet-hello aspnet-hello transferred this issue from aspnet/BasicMiddleware Dec 19, 2018
@aspnet-hello aspnet-hello added area-middleware enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Dec 19, 2018
@colgreen
Copy link

"Are you aware of any proxies that actually set that new header?"

Arguably that point is somewhat circular, i.e. maybe if ASP.NET Core supported the 'Forwarded' header there would be more pressure on the reverse proxy implementations to support it. It is after all the only approach that actually has a formal specification (rfc7239).

@damianh
Copy link

damianh commented Apr 30, 2020

Would also like to have ability to extend the 'Forwared' header with a PathBase parameter as well as support for that with the current implementation i.e. X-Forwarded-PathBase

Just to mention that I've shipped a package that supports this https://github.com/ProxyKit/HttpOverrides

@apeeters
Copy link

apeeters commented Jul 7, 2020

"Are you aware of any proxies that actually set that new header?"

Arguably that point is somewhat circular, i.e. maybe if ASP.NET Core supported the 'Forwarded' header there would be more pressure on the reverse proxy implementations to support it. It is after all the only approach that actually has a formal specification (rfc7239).

Indeed. If no one starts implementing the standard, it will never gain traction. Also: Spring has support for this Forwarded header since 2016...

@colgreen
Copy link

colgreen commented Jul 7, 2020

Perhaps someone could reach out to the YARP team to get their views, i.e. e.g. are they planning to implement rfc7239

microsoft/reverse-proxy

@NatMarchand
Copy link
Contributor

Yes, we've seen it. Are you aware of any proxies that actually set that new header?

@Tratcher, it is the documented way for Azure APIM : https://docs.microsoft.com/en-us/azure/api-management/policies/set-header-to-enable-backend-to-construct-urls

@Tratcher
Copy link
Member

Tratcher commented Aug 3, 2020

Perhaps someone could reach out to the YARP team to get their views, i.e. e.g. are they planning to implement rfc7239

microsoft/reverse-proxy

YARP is being built by this team. We did add support for the new header in YARP but it's off by default. See https://microsoft.github.io/reverse-proxy/articles/transforms.html#forwarded

Design note: this probably should be implemented in AspNetCore as a whole new middleware, it takes very different options. I'm also not sure what to do with the randomized client ids, stash them in a new request feature?

@JonAnders
Copy link

Design note: this probably should be implemented in AspNetCore as a whole new middleware, it takes very different options.

@Tratcher I think there is value in keeping it in the same middleware, if possible, as the headers are conceptually the same. A class called ForwardedHeadersMiddleware is where I would expect to find the standards-compliant implementation, I wouldn't keep looking for a different middleware.

What if Forwarded was added to the ForwardedHeaders enum? To restrict the processing, in line with how X-Forwarded-* is configured, the enum could include ForwardedBy, ForwardedFor, ForwardedHost and ForwardedProto as well.

Support for both Forwarded and X-Forwarded-* could then be configured together, such as:

app.UseForwardedHeaders(new ForwardedHeadersOptions
{
    ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.ForwardedFor
});

There are some options that only make sense for the X-Forwarded-* headers, but is that a problem? They can either be ignored or cause an exception if they are set when only support for the Forwarded header is configured.

I'm also not sure what to do with the randomized client ids, stash them in a new request feature?

Would it be a plausible alternative to just provide a class for parsing the Forwarded header and leave the rest up to the developer when using randomized client ids?

@Tratcher
Copy link
Member

Support for both Forwarded and X-Forwarded-* could then be configured together, such as:

app.UseForwardedHeaders(new ForwardedHeadersOptions
{
    ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.ForwardedFor
});

That's a good example of why these shouldn't be combine into one middleware. If a request has both headers then what order should they be processed in? It's ambiguous.

@Tratcher Tratcher added affected-medium This issue impacts approximately half of our customers severity-minor This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@Tratcher Tratcher modified the milestones: Backlog, .NET 7 Planning Oct 28, 2021
@ghost
Copy link

ghost commented Sep 13, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@asbjornu
Copy link
Member

That's a good example of why these shouldn't be combine into one middleware. If a request has both headers then what order should they be processed in? It's ambiguous.

I'm not disagreeing with you @Tratcher, but I do think it's easy to argue that Forwarded should take precedence over X-Forwarded-*, both due to chronology and precision.

Chronology

Since X-Forwarded-* existed before Forwarded, it is most likely that Forwarded was added after X-Forwarded-* in order to accommodate middleware that supports Forwarded. Middleware that doesn't support Forwarded will just ignore it and process X-Forwarded-* as before.

If adding Forwarded to an HTTP request doesn't do anything because X-Forwarded-* takes precedence, it makes it very hard to evolve beyond the status quo since every new header will have to be added as a clean break with existing infrastructure.

Precision

Forwarded also has a precisely defined processing model, which I assume most implementers prefer over the undefined behavior of X-Forwarded-*.

What is a proxy supposed to do when there are an unequal amount of items added to X-Forwarded-For, X-Forwarded-Host and X-Forwareded-Proto, for instance? Which item corresponds to which between the three headers? Such situations may occur if a reverse proxy doesn't support all 3 headers and only modifies one or two of them.

There's also the question of whether new items should be appended or prepended to existing headers. There are numerous edge cases that are unresolved since these headers have been invented separately and never thoroughly specified in terms of a single, coherent standard.

Since Forwarded groups for, by, host and proto together for each item and clearly defines that items should be appended to an existing header, it doesn't suffer from these problems. If issues with Forwarded arises, IETF has a clearly defined process to improve upon the published RFC, which will be made available in a canonical RFC that all implementers can use and reference.

Because of all of these benefits, I think it's obvious that Forwarded should take precedence over X-Forwarded-*, when both exist in an HTTP request.

@mitchdenny mitchdenny added the feature-forwarded-headers This issue is related to forwarded headers label Mar 27, 2023
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-medium This issue impacts approximately half of our customers area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-forwarded-headers This issue is related to forwarded headers severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests