aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

RemoteIpAddress and X-Forwarded-For #190

Closed Tratcher closed 7 years ago

Tratcher commented 7 years ago

From @JauernigIT on December 12, 2016 12:54

We struggled for some time now to find a way to reliable find out the client IP address in ASP.NET Core...

First we activated UseIISIntegration() and app.UseForwardedHeaders(new ForwardedHeadersOptions { ForwardedHeaders = ForwardedHeaders.XForwardedFor }) in the startup phase.

Then we thought that we could use httpContext.Connection.RemoteIpAddress for this scenario and yes, it works when we have direct callers - in this case it returns the right client IP... But: unfortunately it doesn't work when the client is behind our corporate proxy. In this case, Connection.RemoteIpAddress everytime returns 127.0.0.1.

Doing some more investigation we found out that the X-Forwarded-For request header was correctly set for clients that come over the proxy: X-Forwarded-For: <client-ip>, 127.0.0.1, <proxy-ip:port>. Hence, we are able to manually grab the right client IP from the request. But we thought that httpContext.Connection.RemoteIpAddress is exactly doing this for us?

Copied from original issue: aspnet/HttpAbstractions#742

Tratcher commented 7 years ago

From @khellang on December 12, 2016 16:56

I think this might be the wrong repo? ForwardedHeadersMiddleware belongs in https://github.com/aspnet/BasicMiddleware :smile:

Anyway, have you tried setting the ForwardLimit to something other than 1 (the default) on the ForwardedHeadersOptions?

Tratcher commented 7 years ago

There are a number of other ForwardedHeadersOptions properties to adjust like ForwardLimit and KnownProxies

JauernigIT commented 7 years ago

We tried to set ForwardLimit = null, as result we got the IP address of the proxy back, but not the real client IP...

How is httpContext.Connection.RemoteIpAddress intended to work? Is it evaluating the number of IP addresses in the X-Forwarded-For request header depending on ForwardedHeadersOptions.ForwardLimit? How does it know, which is the outermost IP in the XFF header? As said above, we have an XFF of <client-ip>, 127.0.0.1, <proxy-ip:port>, which has no clear order (but first entry is the client IP). We're using some 3rd party solution for proxying/load balancing, perhaps this doesn't behave in conformity.

khellang commented 7 years ago

@JauernigIT Did you try adding the proxy IP to KnownProxies as well? If the MW doesn't know it's a proxy, it'll assume it's the client IP.

The middleware will process the comma-separated values from right to left. This means that in your case, ForwardLimit should be set to (at least) 3 or null and either the specific proxy IP or an IP-range (which includes the proxy IP) must be added to KnownProxies or KnownNetworks, respectively.

JauernigIT commented 7 years ago

@khellang Thanks, we will test setting KnownProxies on our next deployment. It's not ideal since our network infrastructure changes from time to time, so perhaps we will evaluate X-Forwarded-For manually anyway. Is 127.0.0.1 excluded by default when getting RemoteIpAddress from an XFF chain?

khellang commented 7 years ago

Is 127.0.0.1 excluded by default when getting RemoteIpAddress from an XFF chain?

@JauernigIT Yep. Both IPv4 and IPv6 loopback is in the default list of KnownProxies and KnownNetworks: https://github.com/aspnet/BasicMiddleware/blob/46adbc3e8f2158801d491995477f8fae91da5c15/src/Microsoft.AspNetCore.HttpOverrides/ForwardedHeadersOptions.cs#L63-L68

muratg commented 7 years ago

@blowdart thoughts?

blowdart commented 7 years ago

Not sure what I'm supposed to be thinking about here. You have to have some knowledge of your network layer to get this configured correctly, there's no way around that.

Tratcher commented 7 years ago

@blowdart The root question on this and related issues is whether or not the algorithm is too complex and requires too many settings for common usage patterns.

blowdart commented 7 years ago

Ah. Maybe. For common use it's usually correct through, it's once you get nested proxies it becomes weird.

Tratcher commented 7 years ago

The defaults only work for IIS/ANCM. Limit one proxy, loopback address, symmetrical headers. It doesn't even work in Azure.

Tratcher commented 7 years ago

https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.HttpOverrides/ForwardedHeadersOptions.cs#L51-L74

glennc commented 7 years ago

@blowdart sounds like the idea here is to discuss changing our defaults, like symmetrical headers, to allow it to work in some places better by default. As well as getting our docs for this prioritized. What do you think about the defaults? Would changing the symmetrical headers default be a reasonable lessening of the trust level, or do we need to leave it locked down and rely on docs?

blowdart commented 7 years ago

Ugh. Yea, you can change the defaults.

BillDines commented 7 years ago

We have also run up against this. Changing defaults so it works in different deployment scenarios without requiring code changes would certainly help us. Currently we plan to use configuration so we can change the settings without needing code changes. Its worrying that changes in the way your application is deployed can break it with very little information as to why. I'm very much looking forward to the documentation as this is not an area where I have a lot of experience!

taspeotis commented 7 years ago

First we activated UseIISIntegration() and app.UseForwardedHeaders(new ForwardedHeadersOptions { ForwardedHeaders = ForwardedHeaders.XForwardedFor }) in the startup phase.

One problem I have encountered but not been able to solve is that UseIISIntegration sets RequireHeaderSymmetry = true, at least in v1.1.2 (removed in later versions).

https://github.com/aspnet/IISIntegration/blob/rel/1.1.2/src/Microsoft.AspNetCore.Server.IISIntegration/WebHostBuilderIISExtensions.cs#L65

So when you do app.UseForwardedHeaders(new ... { ..., RequireHeaderSymmetry = false }) your settings are lost. (It also sets ForwardedHeaders a few lines up, too, so you can't set that either. This is not removed in later versions.)

For now we're fishing X-Forwarded-For out of Headers and waiting for v2.

Tratcher commented 7 years ago

Don't call UseForwardedHeaders, UseIISIntegration has already added that. Instead do the following to reconfigure the forwarded headers:

            services.Configure<ForwardedHeadersOptions>(options =>
            {
                ...
            });
taspeotis commented 7 years ago

Hi, thanks for the reply.

UseIISIntegration doesn't call UseForwardedHeaders as far as I can see, so you need to call it at least once (either overload). It just adds the Configure<ForwardedHeaderOptions> which sits there to overwrite whatever values for ForwardedHeaders and RequireHeaderSymmetry are configured or specified by UseForwardedHeaders(options).

https://github.com/aspnet/IISIntegration/blob/rel/1.1.2/src/Microsoft.AspNetCore.Server.IISIntegration/WebHostBuilderIISExtensions.cs#L26

I tried services.Configure myself before and after the UseIISIntegration call but I was unsuccessful in overriding whatever UseIISIntegration set.

Also I recognise this is not the repository for IIS Integration, I just left my comment to hopefully be useful to the next guy that stumbles across this issue.

Tratcher commented 7 years ago

It's over here: https://github.com/aspnet/IISIntegration/blob/dev/src/Microsoft.AspNetCore.Server.IISIntegration/IISSetupFilter.cs

rjpaulsen commented 6 years ago

@Tratcher did you get anything (like HttpContext.Connection.RemoteIpAddress) working, or are you still pulling the IP from X-Forwarded-For ? I've got the same exact issue and I'm going bonkers trying to figure this out.

Tratcher commented 6 years ago

@rjpaulsen share your request headers.

khellang commented 6 years ago

I also spent an extremely unnecessary amount of time setting up forwarded headers behind NGINX. Surely there must be a better way to handle misconfigurations here? Or ease the defaults to not be as strict? In my case, I'd much rather have it throw than silently continue the request without a proper remote address...

Tratcher commented 6 years ago

For 2.0 the restrictions were eased to address some common scenarios. Are you still having trouble on 2.0?

rjpaulsen commented 6 years ago

I've got a year-long project being deployed Monday so I wasn't brave enough to switch to 2.0 by the time it was ready. In the meantime, I'm just pulling it from the header. I was hoping I was just missing some line of code that you were going to send me. :-)

A 2.0 upgrade will be coming soon, so I'll assume it will magically start working.

Tratcher commented 6 years ago

RequireHeaderSymmetry = false is the most common workaround. That's the new default for 2.0.