Skip to content
This repository has been archived by the owner. It is now read-only.

RemoteIpAddress and X-Forwarded-For #190

Closed
Tratcher opened this issue Dec 12, 2016 · 25 comments
Closed

RemoteIpAddress and X-Forwarded-For #190

Tratcher opened this issue Dec 12, 2016 · 25 comments
Assignees

Comments

@Tratcher
Copy link
Member

@Tratcher Tratcher commented Dec 12, 2016

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...

  • Environment: Windows Server 2008 R2, IIS 7.5, ASP.NET Core 1.0.0 (still)
  • Requirement: it should work with direct callers as well as with clients that come over a corporate proxy.

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

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Dec 12, 2016

From @khellang on December 12, 2016 16:56

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

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

@Tratcher

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Dec 12, 2016

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

@JauernigIT

This comment has been minimized.

Copy link

@JauernigIT JauernigIT commented Dec 13, 2016

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

This comment has been minimized.

Copy link
Contributor

@khellang khellang commented Dec 13, 2016

@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

This comment has been minimized.

Copy link

@JauernigIT JauernigIT commented Dec 14, 2016

@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

This comment has been minimized.

Copy link
Contributor

@khellang khellang commented Dec 14, 2016

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:

public IList<IPAddress> KnownProxies { get; } = new List<IPAddress>() { IPAddress.IPv6Loopback };
/// <summary>
/// Address ranges of known proxies to accept forwarded headers from.
/// </summary>
public IList<IPNetwork> KnownNetworks { get; } = new List<IPNetwork>() { new IPNetwork(IPAddress.Loopback, 8) };

@muratg

This comment has been minimized.

Copy link
Contributor

@muratg muratg commented Jan 10, 2017

@blowdart thoughts?

@blowdart

This comment has been minimized.

Copy link
Member

@blowdart blowdart commented Jan 10, 2017

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

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Jan 10, 2017

@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

This comment has been minimized.

Copy link
Member

@blowdart blowdart commented Jan 10, 2017

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

@Tratcher

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Jan 10, 2017

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

@glennc

This comment has been minimized.

Copy link
Member

@glennc glennc commented Feb 9, 2017

@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

This comment has been minimized.

Copy link
Member

@blowdart blowdart commented Feb 10, 2017

Ugh. Yea, you can change the defaults.

@BillDines

This comment has been minimized.

Copy link

@BillDines BillDines commented Apr 6, 2017

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

This comment has been minimized.

Copy link

@taspeotis taspeotis commented Aug 1, 2017

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

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Aug 1, 2017

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

            services.Configure<ForwardedHeadersOptions>(options =>
            {
                ...
            });
@taspeotis

This comment has been minimized.

Copy link

@taspeotis taspeotis commented Aug 1, 2017

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

This comment has been minimized.

@rjpaulsen

This comment has been minimized.

Copy link

@rjpaulsen rjpaulsen commented Sep 29, 2017

@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

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Sep 29, 2017

@rjpaulsen share your request headers.

@khellang

This comment has been minimized.

Copy link
Contributor

@khellang khellang commented Sep 29, 2017

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

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Sep 29, 2017

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

@rjpaulsen

This comment has been minimized.

Copy link

@rjpaulsen rjpaulsen commented Sep 29, 2017

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

This comment has been minimized.

Copy link
Member Author

@Tratcher Tratcher commented Sep 29, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.