[release/9.0] Forwarded Headers Middleware: Ignore XForwardedHeaders from Unknown Proxy

[github-actions]

Backport of #61530 to release/9.0

/cc @BrennanConroy @yannic-hamann-abb

Forwarded Headers Middleware: Ignore XForwardedHeaders from Unknown Proxy

Description

If the ForwardedHeadersMiddleware middleware is used without using XForwardedFor then the KnownNetworks and KnownProxies checks are skipped.

Fixes #61449

Customer Impact

Expectations for KnownNetworks and KnownProxies settings are not always met. If you aren't careful with configuring your app (careful meaning aware of this issue), you can end up allowing traffic you didn't intend to allow.

Regression?

• Yes
• No

Risk

• High
• Medium
• Low

Runs a check that was already there but runs it in more cases.

Verification

• Manual (required)
• Automated

Packaging changes reviewed?

• Yes
• No
• N/A

[wtgodbe]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[danports]

This seems like a significant behavior change, especially for a patch release, and I was very surprised to see that this was not at least called out in the 9.0.6 release notes. We had to revert an upgrade from 9.0.5 today after this broke a production environment which has Kestrel fronted by AWS ALB.

[andygjp]

This also impacted my production environment!

In my case its an Azure WebApp, using EasyAuth, hosting a custom image. I use the 9.0-noble tag, so I guess thats how I ended up with this change.

Short of going back to 9.0.5, what changes do I need to make to restore previous functionality?

[BrennanConroy]

What are your setups? This should only affect you if you didn't use XForwardedFor and set a value for KnownNetworks or KnownProxies. And if you do have that specific setup, then it should be fixing an issue where KnownNetworks and KnownProxies weren't being checked properly.

[doeringp]

There's loopback set as default for KnownNetworks and KnownProxies in ForwardedHeadersOptions.

ForwardedHeadersOptions.cs:

/// <summary>
/// Addresses of known proxies to accept forwarded headers from.
/// </summary>
public IList<IPAddress> KnownProxies { get; } = new List<IPAddress>() { IPAddress.IPv6Loopback };

/// <summary>
/// Address ranges of known proxies to accept forwarded headers from.
/// </summary>
public IList<IPNetwork> KnownNetworks { get; } = new List<IPNetwork>() { new IPNetwork(IPAddress.Loopback, 8) };

So this unfortunately is a breaking change for us, as we do not use XForwardedFor and didn't configure KnownNetworks and KnownProxies explicitly.

app.UseForwardedHeaders(new ForwardedHeadersOptions
{
    ForwardedHeaders = ForwardedHeaders.XForwardedProto
});

[yannic-hamann-abb]

@doeringp in your setup, what is the actual IP of the proxy forwarding the request?

[doeringp]

@yannic-hamann-abb I'm not sure yet, but I know it's not localhost since the application is running in Kubernetes behind an Nginx Ingress Controller.

[yannic-hamann-abb]

In your setup you have to explicitly add the proxy IP or proxy network to the ForwardedHeadersOptions via KnownProxies or KnownNetworks.

var options = new ForwardedHeadersOptions { ForwardedHeaders.XForwardedProto };
options.KnownNetworks.Add(...);

[mayerraphael]

For us it was enough to override the defaults of KnownProxies and KnownNetworks.

app.UseForwardedHeaders(new ForwardedHeadersOptions
{
    ForwardedHeaders = ForwardedHeaders.XForwardedProto,
    KnownNetworks = {},
    KnownProxies = {},
});

With this UseForwardedHeaders sets the correct HttpContext.Request.Scheme again provided by X-Forwarded-Proto.

Still a breaking change. Thankfully we discovered it in a testing environment.

[yannic-hamann-abb]

@mayerraphael with that config you are bypassing the known proxy checks entirely.

[doeringp]

@yannic-hamann-abb I agree that this change makes sense from a security standpoint. However, it's also a breaking change in behavior that could impact customers like us who haven't used the X-Forwarded-For header and haven't configured KnownNetworks or KnownProxies.

[andygjp]

I have an OData API that uses server paging and a SPA client which depends upon the nextLink, returned in the API response, to get the next page of customers. There have been no code or environment changes to the API for many weeks, then on Wednesday morning my users cannot page through their customers - the schema has changed from https to http and the browser won’t allow the request.

I saw this change and assumed the nightly build of the container, which uses v9 of the SDK, had used v9.0.301 and that had changed the response. (I hadn’t changed the versions of the packages consumed by my API - all Microsoft.* packages are still at v9.0.5.) I fixed the version to 9.0.203 and it didn’t fix it.

I checked my configuration and I already was using app.UseForwardedHeaders() and services.Configure<ForwardedHeadersOptions>(ops => ops.ForwardedHeaders = ForwardedHeaders.XForwardedProto).

I use EasyAuth and though maybe it was that proxy that was affecting the response, so I turned that off (in a test environment) and that still didn’t fix it.

I think there is still another proxy in front of my Azure Web App and its that which has a dependency upon this library, because with...

• all Microsoft.* packages at v9.0.5
• app.UseForwardedHeaders() removed
• self-contained app published using v9.0.203 SDK
• running inside a container that uses the runtime-deps:9.0.5 image
• EasyAuth turned off

It still doesn’t work.

But if I add ASPNETCORE_FORWARDEDHEADERS_ENABLED environment variable to the Azure Web App, it fixes the issue, ie the nextLink schema is https. I assume that proxy uses that environment variable.

[danports]

@BrennanConroy Our setup is app.UseForwardedHeaders(new ForwardedHeadersOptions { ForwardedHeaders = ForwardedHeaders.XForwardedProto }) - we did not explicitly set a value for KnownNetworks or KnownProxies but as @doeringp pointed out there are default values for those properties. Similar to @andygjp's scenario, what broke after updating to 9.0.6 was link generation - all links were HTTP rather than HTTPS as they should have been.

[yannic-hamann-abb]

And if you do have that specific setup, then it should be fixing an issue where KnownNetworks and KnownProxies weren't being checked properly.
@danports

If you had set the option in 9.0.5 to ForwardedHeaders.All or added ForwardedHeaders.XForwardedFor, you would have broken the app in the exact same way.

This change ensures consistent handling of forwarded headers how it is described in the docs.
Unfortunate to see that this had a negative impact for your setup.

[danports]

Yes @yannic-hamann-abb, I don't think anyone is arguing that your original PR was a bad idea - what's unfortunate is that this backport PR was a breaking change made in a patch release with no warnings in the release notes.