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

Templates do not work in Antares Linux due to missing scheme forwarders #4135

Closed
Tratcher opened this issue Apr 3, 2018 · 34 comments
Closed
Assignees
Labels
accepted This issue has completed "acceptance" testing (including accessibility) area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@Tratcher
Copy link
Member

Tratcher commented Apr 3, 2018

The 2.1 templates include UseHttpsRedirection and UseHsts by default. These stop working and put your site into an infinite loop if you deploy to Antares Linux (or behind any other reverse proxy besides IIS). The problem is that TLS is terminated by the reverse proxy and Kestrel doesn't know the correct request scheme. OAuth and OIDC also fail in this configuration as they generate incorrect redirects.

The fix is to add and configure the ForwardedHeaders middleware to fix up the scheme as forwarded from the proxy. UseIISIntegration adds and configures this middleware when running behind IIS but we have no matching lightup and config for Linux e.g. UseApacheIntegration, UseNginxIntegration, etc..

Related:
https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer
dotnet/AspNetCore.Docs#5806

Proposal:
See what it would take to detect and light up in Antares Linux, and add that code to CreateDefaultBuilder. Also see if this can be generalized to other Apache and NGinx scenarios.

@DamianEdwards for 2.1 rc1 consideration

@muratg
Copy link
Contributor

muratg commented Apr 12, 2018

Moving out due to schedule constraints. @DamianEdwards @glennc FYI.

@muratg
Copy link
Contributor

muratg commented Oct 1, 2018

@shirhatti Could you please sync up with @Tratcher on this one?

@Tratcher
Copy link
Member Author

Tratcher commented Nov 4, 2018

Another hit: aspnet/Security#1901

@shirhatti
Copy link
Contributor

cc @bradygaster

@natemcmaster natemcmaster transferred this issue from aspnet/AzureIntegration Nov 20, 2018
@natemcmaster natemcmaster added Needs: Design This issue requires design work before implementating. cost: S labels Nov 20, 2018
@natemcmaster natemcmaster added this to the 3.0.0-preview2 milestone Nov 20, 2018
@muratg muratg modified the milestones: 3.0.0-preview2, 3.0.0-preview3 Jan 30, 2019
@muratg
Copy link
Contributor

muratg commented Jan 30, 2019

Moving out since preview2 already shipped.

@bradygaster
Copy link
Member

@muratg i tried this tonight using 2.1 templates to antares linux and it lit up right away (hero path was clean). i will also try on AKS, as that uses, if memory serves me correctly, nginx, and update this with the result. if this also works i'd like to make sure i understand what's awry to help make sure the experiences (kubernetes with nginx, or an AKS out-of-the-box deployment and app service linux).

@muratg
Copy link
Contributor

muratg commented Feb 7, 2019

@bradygaster interesting. The template does have UseHttpsRedirection, right? I wonder if there's no reverse proxy involved in antares linux then (would be surprising on its own.)

On AKS, I believe you'd pass through Azure load balancer. (You may also install an internal reverse proxy, e.g. via nginx-ingress.) Not sure if those would be broken or not.

@bradygaster
Copy link
Member

@muratg yes, they do have UseHttpsRedirection in Startup.cs. The e2e "hero scenario" to go from dotnet new razor to a working https://dotnetcoreonlinux.azurewebsites.net/ (real url) took about 10 minutes. i'll try it again for kicks now using the new az webapp up command, just for kicks. 👍

Worked on getting our demo aks cluster working tonight with nginx per this article tonight and didn't get too far, so i'm going to rebuild the cluster and try via draft up with a few yaml tweaks, with no customization to the cluster - just out-of-the-box AKS. will report back on that tomorrow by eod.

@Tratcher
Copy link
Member Author

Tratcher commented Feb 7, 2019

Did you echo out the scheme to make sure? HTTPS can be enforced on the front end, but that doesn't help redirect generation for things like auth.

@bradygaster
Copy link
Member

@Tratcher I'll want to talk to you and @muratg to understand the scenario. :) The out-of-the-box scenario works, so I presume i'm missing some setup steps that would expose the issue on Antares linux.

@Tratcher
Copy link
Member Author

Another hit: MicrosoftDocs/azure-docs#11104

@bradygaster
Copy link
Member

reproduced this issue, and with feedback from @cephalin was able to validate a potential solution, which is represented by code from Startup.cs in a working project below.

This has been validated in a few new web apps running with a home-built docker image of a .net core 2.2 app.

In the Configure(IApplicationBuilder app, IHostingEnvironment env) method of Startup.cs, we'd need to add code like this that would effectively enable HTTP in a container on App Service:

var options = new ForwardedHeadersOptions {
      ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto    
};

options.KnownProxies.Add(IPAddress.Parse("::ffff:172.19.0.1"));

app.UseForwardedHeaders(options);

A proper fix for this would be simple, but we need to answer questions on how the argument(s) passed to IPAddress.Parse should be loaded or discovered. Another concern is preloading these with values appropriate for App Service customers, Kubernetes customers, and potential variations of these scenarios.

@bradygaster
Copy link
Member

@cephalin the nice people who build the container images will be setting that EV for us. Estimate is early summer for release of the images inclusive with that EV.

@analogrelay
Copy link
Contributor

@cephalin FYI we don't plan to backport this to 2.x builds, but the code is very simple (just enabling the forwarded headers middleware when an environment variable is set). @bradygaster will be writing up a blog post on how to enable this on 2.x builds.

@cephalin
Copy link

cephalin commented May 3, 2019

@bradygaster
Copy link
Member

Yes, @cephalin that would be fantastic for you to complement the upcoming blog post (once we fix this) with the doc update. thanks so much.

@analogrelay analogrelay added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 7, 2019
@Tratcher
Copy link
Member Author

Azure-App-Service/dotnetcore-template#1 ASPNETCORE_FORWARDEDHEADERS_ENABLED=true added to the docker files.

@Tratcher Tratcher added 2 - Working and removed Needs: Design This issue requires design work before implementating. labels May 15, 2019
@Tratcher Tratcher added Done This issue has been fixed and removed 2 - Working labels May 17, 2019
@Tratcher
Copy link
Member Author

@bradygaster let's sync up about the 2.x blog post.

@Tratcher
Copy link
Member Author

Recommended 2.x mitigation that cleanly upgrades to 3.0:

                // ConfigureServices
                if (string.Equals("true", hostingContext.Configuration["ForwardedHeaders_Enabled"], StringComparison.OrdinalIgnoreCase))
                {
                    services.Configure<ForwardedHeadersOptions>(options =>
                    {
                        options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto;
                        // Only loopback proxies are allowed by default. Clear that restriction because forwarders are
                        // being enabled by explicit configuration.
                        options.KnownNetworks.Clear();
                        options.KnownProxies.Clear();
                    });
                }

                // Configure:
                app.UseForwardedHeaders();

@analogrelay
Copy link
Contributor

analogrelay commented May 21, 2019

Closing, since this is done for 3.0. Let's make getting the blog post written part of "accepting" the issue.

@Tratcher
Copy link
Member Author

Tratcher commented Jun 3, 2019

Verified with 3.0.100-preview6-012234. Note we don't have the new container images so you need to add ASPNETCORE_FORWARDEDHEADERS_ENABLED=true as an app setting.

@bradygaster, how's the blog post?

@Tratcher Tratcher added the accepted This issue has completed "acceptance" testing (including accessibility) label Jun 3, 2019
@bradygaster
Copy link
Member

in progress amidst conf prep and post-move. will have a review to you two by EOW

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted This issue has completed "acceptance" testing (including accessibility) area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

10 participants