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

Change Logging for Header mismatch #46

Closed mikes-gh closed 8 years ago

mikes-gh commented 8 years ago

@Tratcher Here a Debug Log is issued when header symmetry fails (there are other examples in this class)

https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.HttpOverrides/ForwardedHeadersMiddleware.cs#L80

The code considers the headers untrusted and returns without setting

request.Scheme = currentValues.Scheme;

The result is a redirect loop when

[HttpsRequired]

and

X-Forwarded-For count != X-Forwarded-Proto count

This is a common scenario for

This situation is not very discoverable and can leave one scratching your head. Why am I getting a re-direct loop?

Microsoft default LogLevel is Information , so if you look in the logs there is no indication of what actually went wrong aside from a log full of redirects. Its also a situation which doesn't recover (browser gives up).

At the very least can we change these header checks to _logger.LogError?

Is it a candidate for raising an exception?

Tratcher commented 8 years ago

We could raise the log level to improve visibility. It should not produce an exception as this is based on arbitrary user input.

mikes-gh commented 8 years ago

That would be great :+1: If requireheadersymmetry=true (default) I think it would be good to LogError if its not.

I still think requiring header symmetry for X-Forwarded-Proto is broken for anything more than one hop though. Its really only the first value that is important in determining whether the user arrived at the page through an SSL connection. so

so if X-Forwarded-Proto is https,http

the http bit is not important and often not added by load balancers etc.

Say you have nginx forwarding to kestrel on your Linux install and you are forwarding from your firewall. That wont work with requireheadersymmetry=true.

I am hoping to contribute towards the nginx docs as they are quite a bit off :smile:

mikes-gh commented 8 years ago

Actually Warn should be adequate

BrennanConroy commented 8 years ago

https://github.com/aspnet/BasicMiddleware/commit/d3816fa458b83603b9e0848baccb1772ab967402