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

Add support to reverse proxies #2

Closed
fume opened this issue Apr 15, 2022 · 2 comments · Fixed by #29
Closed

Add support to reverse proxies #2

fume opened this issue Apr 15, 2022 · 2 comments · Fixed by #29
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@fume
Copy link
Collaborator

fume commented Apr 15, 2022

We actually generate the AssertionConsumerServiceUrl using the HTTP Request host. This won't work in scenarios where reverse proxies are used, since the request host will be different from the "public" host that users can reach.

We basically need to change the following line

public string GetAttributeConsumerService()
{
return $"https://{_httpContextAccessor.HttpContext.Request.Host}/proxy/assertionconsumer";
}

We could use the x-forwarded-host header (https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer?view=aspnetcore-6.0) or, eventually, just put the right host in config.

@fume fume added enhancement New feature or request good first issue Good for newcomers labels Apr 15, 2022
@fume
Copy link
Collaborator Author

fume commented Jun 21, 2022

I just noticed that the ForwardedHeadersMiddleware can be enabled as simply as setting the env variable ASPNETCORE_FORWARDEDHEADERS_ENABLED to true (ForwardedHeaderStartupFilter). Doing so, the default values for the ForwardedHeadersOptions are going to be used except for the overridden options in the (ForwardedHeadersOptionsSetup). Basically, only the x-forwarded-proto and x-forwarded-for headers are going to be used, which is not enough for us since we also need x-forwarded-host.

I see two options here:

  • We check for the ASPNETCORE_FORWARDEDHEADERS_ENABLED env variable, and if set to true we configure the ForwardedHeadersOptions as we desire
  • We add the ForwardedHeaders middleware on our own

@fume
Copy link
Collaborator Author

fume commented Jun 21, 2022

I have an implementation proposal on the support-reverse-proxies branch.

In Program.cs

if (string.Equals(builder.Configuration["ASPNETCORE_FORWARDEDHEADERS_ENABLED"], "true", StringComparison.OrdinalIgnoreCase))
{
ConfigureForwardedHeadersOptions();
}

Where ConfigureForwardedHeadersOptions() is
void ConfigureForwardedHeadersOptions()
{
builder.Services.Configure<ForwardedHeadersOptions>(options =>
{
var section = builder.Configuration.GetSection("ForwardedHeaders");
section.Bind(options);
var knownProxies = section.GetValue<string>("KnownProxies")?.Split(',');
var knownNetworks = section.GetValue<string>("KnownNetworks")?.Split(',');
if (knownProxies?.Length > 0)
{
options.KnownProxies.Clear();
foreach (var ip in knownProxies)
{
options.KnownProxies.Add(IPAddress.Parse(ip));
}
}
if (knownNetworks?.Length > 0)
{
options.KnownNetworks.Clear();
foreach (var network in knownNetworks)
{
var networkSplit = network.Split('/');
var prefix = networkSplit[0];
var prefixLength = int.Parse(networkSplit[1]);
options.KnownNetworks.Add(new IPNetwork(IPAddress.Parse(prefix), prefixLength));
}
}
});
}

Then in the appsettings we can define the ForwardedHeaders section and set all the required options. The ForwardedHeaders are defaulted to All (Which means X-Forwarded-For | X-Forwarded-Proto | X-Forwarded-Host)

"ForwardedHeaders": {
"ForwardedHeaders": "All"
}

@MarcoZama , @tommasodotNET , @PaoloCastAway , any thoughts?

@fume fume pinned this issue Jun 22, 2022
@fume fume self-assigned this Jul 2, 2022
@fume fume closed this as completed in #29 Sep 16, 2022
@fume fume unpinned this issue Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant