dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.4k stars 10k forks source link

ForwardedHeadersMiddleware isn't setting X-Original-Proto correctly #54904

Closed RobSiklos closed 6 months ago

RobSiklos commented 6 months ago

Is there an existing issue for this?

Describe the bug

When using the ForwardedHeaders middleware, and the initial request uses HTTPS but the backend connection is HTTP, the X-Original-Proto header shows http instead of https.

That is to say, when the connection to ASP.NET Core is HTTP, but the X-Forwarded-Proto header contains https, the X-Original-Proto header shows http instead of https.

Expected Behavior

The X-Original-Proto header shows https if X-Forwarded-Proto is https

Steps To Reproduce

Set up a new ASP.NET Core WebAPI project in Visual Studio 2022 with the following program also available at https://github.com/RobSiklos/ForwardedHeadersWebTest):

public class Program
{
    public static void Main(string[] args)
    {
        var builder = WebApplication.CreateBuilder(args);

        // Add services to the container.

        builder.Services.AddControllers();

        builder.Services.Configure<ForwardedHeadersOptions>(options =>
        {
            options.ForwardedHeaders = ForwardedHeaders.All;
            options.ForwardLimit = null;

            // Trust all known networks, for testing.
            options.KnownProxies.Clear();
            options.KnownNetworks.Clear();
        });

        var app = builder.Build();

        // Configure the HTTP request pipeline.
        app.Use((context, next) =>
        {
            Console.WriteLine(context.Request.Scheme); // http
            Console.WriteLine(context.Request.Headers["X-Forwarded-Proto"]); // https
            Console.WriteLine("----");
            return next();
        });

        app.UseForwardedHeaders();

        app.Use((context, next) =>
        {
            Console.WriteLine(context.Request.Scheme); // https
            Console.WriteLine(context.Request.Headers["X-Forwarded-Proto"]); // empty

            // *** HERE IS THE PROBLEM ***
            Console.WriteLine(context.Request.Headers["X-Original-Proto"]); // http *** SHOULD BE https ***

            Console.WriteLine("====================");
            return next();
        });

        app.MapControllers();

        app.Run();
    }
}

Now send a request via http:// to the web app similar to the following:

curl --location 'http://localhost:50301/weatherforecast' \
--header 'X-Forwarded-Proto: https' \
--header 'X-Forwarded-For: 1.2.3.4'

Exceptions (if any)

No response

.NET Version

8.0.100

Anything else?

Tried with .NET 6.0 and .NET 8.0 - same result.

martincostello commented 6 months ago

Just to rule it out, do you get the same problem if you also update KnownProxies in the same way as you've done for KnownNetworks?

RobSiklos commented 6 months ago

@martincostello KnownProxies takes single IP addresses, while KnownNetworks takes ranges, so I can't update it in the same way. By adding "everything" to KnownNetworks, it should rule out any possibility that the proxy isn't "known". Further, if the middleware was rejecting the proxy, the request.Scheme wouldn't be updated from http to https, right?

martincostello commented 6 months ago

I think if you call Clear() on either, it'll allow everything too the same way as allowing the whole network.

RobSiklos commented 6 months ago

@martincostello I don't think that's true, but it doesn't matter. Either way, it's not a factor because it's clear that the middleware in my sample IS processing the request, since it's updating the request.Scheme property.

martincostello commented 6 months ago

That's how I interpret this code:

https://github.com/dotnet/aspnetcore/blob/7f4a3ef02a6a713d1d69831572bddbe7dfcafd6d/src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs#L216

If both are empty, then the networks shouldn't stop it processing the header or not.

RobSiklos commented 6 months ago

Ok, you're correct. I've changed the sample to clear both KnownProxies and KnownNetworks, but the issue remains.

joegoldman2 commented 6 months ago

There is no bug here. X-Original-Proto is not supposed to contain the value of X-Forwarded-Proto. X-Original-Proto is there to store the original value of HttpContext.Request.Scheme (the original scheme on which the request was made) to avoid losing it as it will be overridden by the ForwardedHeadersMiddleware middleware. If you need to access to the "forwarded" scheme, you can use HttpContext.Request.Scheme after the middleware.

The same rule applies to:

RobSiklos commented 6 months ago

Thanks @joegoldman2 - that makes a lot of sense. In that case, the documentation at https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer?view=aspnetcore-8.0 needs to be updated, because it states:

When processed, X-Forwarded-{For|Proto|Host} values are moved to X-Original-{For|Proto|Host}.

martincostello commented 6 months ago

Docs issues can be filed in dotnet/AspNetCore.Docs.