Enable forwarded headers when publishing projects by default

[DamianEdwards]

Given the primary deployment target for Aspire apps are container-based PaaSs that will very likely be using reverse proxies for ingress including TLS termination, etc., we should consider adding the forwarded headers middleware to the Aspire templates.

Thinking we put a method like the following in the ServiceDefaults project:

public static WebApplication UseDefaultForwardedHeaders(this WebApplication app)
{
    app.UseForwardedHeaders(new()
    {
        ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto | ForwardedHeaders.XForwardedHost
    });

    return app;
}

We'll still need to figure out what the story is for configuring the KnownProxies property so that it actually works once deployed.

[davidfowl]

I don't think this should be on by default..

[DamianEdwards]

Any thoughts on how we can improve this E2E?

[davidfowl]

Can we find out how pervasive it is (how many subsystems are affected)? If we add this middleware by default how do we handle the known proxies problem?

cc @Tratcher for advice

[DamianEdwards]

The target environment could inject the proxy address via environment variables to be read by either the middleware, template/app code, or a platform component. I believe this is similar to how we make this work behind IIS by default.

As far as subsystems affected by it, it's anything that relies on the request scheme or host to accurately reflect what the client sees. For HTML generating scenarios this can be important for link generation, redirects, etc. We hit this just now because of the change in Blazor rtm to render the current url in form elements' action attributes to correct a race condition when combining enhanced navigation and form handling with redirects. It's possible that specific scenario can be rectified with a change in Blazor to instead render root-relative urls (I believe this is what MVC/Razor Pages does today). I've logged dotnet/aspnetcore#51380 to follow up on that separately.

Other things impacted would include multi-tenancy or similar scenarios where differing logic is performed based on the host, e.g. geo-based regionalization via a frontend reverse proxy in front of the PaaS (admittedly these are more advanced). This is common enough that we added support for host-based routing in ASP.NET Core 3.

[Tratcher]

Remember ASPNETCORE_FORWARDEDHEADERS_ENABLED?
https://github.com/dotnet/aspnetcore/blob/30aad8fdfe32ac591039f665ba155a0dcb775dd2/src/DefaultBuilder/src/WebHost.cs#L269C1-L270
https://github.com/dotnet/aspnetcore/blob/30aad8fdfe32ac591039f665ba155a0dcb775dd2/src/DefaultBuilder/src/ForwardedHeadersOptionsSetup.cs#L22-L31

[davidfowl]

ah, we might be able to use this.

[DamianEdwards]

I don't think we have as strong a signal to do this now as we initially thought so moving to backlog.

[mip1983]

One vote for this from me, was certainly a tripping hazard that took a little while to figure out. Either including the env variable by default or just having something in the project template that’s commented out as a hint (like the lines for telemetry)

var apiservice = builder.AddProject<Projects.ecoDriverAnalytics_Api>("ecodriver-analytics-api")
.WithEnvironment("ASPNETCORE_FORWARDEDHEADERS_ENABLED", "true")

[DamianEdwards]

@davidfowl thoughts here? Should we handle this on the deployment side as that's where the required knowledge about the destination platform and its behavior resides? Do you recall how known proxies factors into this if we just use the environment variable to enable the middleware, e.g. do we also have to configure known proxy hosts and hop count?

[davidfowl]

No, I think we should make it a method on the app model that isn't automatic. I also think we need to move the ingress or not to the app model.

[DamianEdwards]

Be careful not to conflate ingress with external/internal access. ACA has ingress for all containers that do networking, but external access is optional. As I understand it, we'd want the forwarded headers on for all apps with HTTP endpoints in ACA, not just those exposed to the Internet. Are you saying you'd want all apps with endpoints to opt-in to declaring ingress in the app model in order to get the env var set? Seems like boilerplate.

[davidfowl]

Be careful not to conflate ingress with external/internal access.

They are independent flags. I just want to move them both into the apphost. I worry about baking it into azd where there's less user control to determine what happens.

Are you saying you'd want all apps with endpoints to opt-in to declaring ingress in the app model in order to get the env var set? Seems like boilerplate.

We can make it a one liner to have it apply to all projects if that is indeed the default.

[DamianEdwards]

preview 5 then?

[davidfowl]

Yes. We can make this a lifecycle hook that only applies during publish.

[DamianEdwards]

Moving into preview.5

We should update Aspire.Hosting to set the ASPNETCORE_FORWARDEDHEADERS_ENABLED environment variable to true when publishing project resources by default and add a way to opt-out of it, e.g. builder.AddProject<Projects.Frontend>().DisableFordwardedHeaders()

Relevant docs