U4-9229 - Problems when using load balancing with SSL Offloading

Created by Simon Dingley 29 Nov 2016, 12:38:01 Updated by Angelbert David 31 Aug 2018, 15:44:55

Tags: PR Gold partner Consider for sprint

Relates to: U4-9165

Subtask of: U4-9609

The site is currently running under Umbraco 7.5.3.

We have a Citrix Netscaler Load Balancer with 2 servers behind it, one master and the other slave. SSL is offloaded to the Load Balancer and therefore the web servers only see the HttpRequest IsSecureConnection property as false. The Load Balancer adds a custom http header to indicate if the connection was over https.

We are trying to force SSL across the site and have the following redirect rule in the web.config:

The problem, however, is with the use of Request.IsSecureConnection in the Core in conjunction with having the umbracoUseSSL appsetting set to true. More specifically, if you look in BasePage.cs the Core has the following which creates an infinite loop of redirects in this situation:

if (!Request.IsSecureConnection && GlobalSettings.UseSSL) { string serverName = HttpUtility.UrlEncode(Request.ServerVariables["SERVER_NAME"]); Response.Redirect(string.Format("https://{0}{1}", serverName, Request.FilePath)); }

1 Attachments

Download SecureConnectionHttpModule.cs

Comments

Sebastiaan Janssen 29 Nov 2016, 12:53:16

Seems like all IsSecureConnection booleans need to be replaced with something like this: http://stackoverflow.com/a/27526290/5018


Jeffrey Schoemaker 29 Nov 2016, 13:02:54

Hi Simon, excellent point. Right now there's a method for getting the correct IP-address (even if it's behind a proxy / loadbalancer).

It's in Umbraco.Core\HttpContextExtensions and is called GetCurrentRequestIpAddress

public static string GetCurrentRequestIpAddress(this HttpContextBase httpContext) { // From: http://stackoverflow.com/a/740431/5018 try { var ipAddress = httpContext.Request.ServerVariables["HTTP_X_FORWARDED_FOR"];

            if (string.IsNullOrEmpty(ipAddress))
                return httpContext.Request.ServerVariables["REMOTE_ADDR"];

            var addresses = ipAddress.Split(',');
            if (addresses.Length != 0)
                return addresses[0];

            return httpContext.Request.ServerVariables["REMOTE_ADDR"];
        }
        catch (System.Exception ex)
        {
            //This try catch is to just always ensure that no matter what we're not getting any exceptions caused since
            // that would cause people to not be able to login
            return string.Format("Unknown, exception occurred trying to resolve IP {0}", ex);
        }
    }

I think there should also such a function for IsSecureConnection that should be used everywhere. We have such a function in our own projects that looks something like this:

public static bool IsHttps() { Boolean bReturn = false;

        try
        {
            if (!String.IsNullOrEmpty(System.Web.HttpContext.Current.Request.ServerVariables["HTTP_X_FORWARDED_PROTO"]))            

return System.Web.HttpContext.Current.Request.ServerVariables["HTTP_X_FORWARDED_PROTO"].ToLower() == "https"; else if (!String.IsNullOrEmpty(System.Web.HttpContext.Current.Request.Headers.Get("Front-End-Https")))

                return System.Web.HttpContext.Current.Request.Headers.Get("Front-End-Https") == "on";                               
            else
                return HttpContext.Current.Request.IsSecureConnection;
        }
        catch (Exception)
        {

        }

        return bReturn;
    }

As you see in this code we're looking for a header callled Front-End-Https or a servervar called HTTP_X_FORWARDED_PROTO. Those are two possible ways that we've seen in the past on different loadbalancers.

Or both these options should be implemented, or it should be an extendable so you can add your own logic into the function.

The same should be true for checking the IP-address. In the current setup is only looked for the variable "HTTP_X_FORWARDED_FOR", but we've seen implementations that set the header in a field called X-Forwarded-For.

Also that should be extendable per installation.

Hope my input helps,


Jeffrey Schoemaker 29 Nov 2016, 13:04:59

If HQ agrees with my suggested implementation I'm more than willing to honor the "Up for grabs"-priority :)


Sebastiaan Janssen 29 Nov 2016, 13:14:18

@jeffrey.schoemaker@perplex.nl Go for it!


Lee Gunn 29 Nov 2016, 13:20:57

Going forward I think Umbraco needs an overridable property like Request.WasSecureConnection.

The redirect code would then look like:

if (!Request.WasSecureConnection && !Request.IsSecureConnection && GlobalSettings.UseSSL) { // redirect }


Sebastiaan Janssen 29 Nov 2016, 13:24:43

What's the difference between that and making a method like the one Jeffrey posted above extensible ?


Lee Gunn 29 Nov 2016, 13:27:48

Mainly to avoid confusion. I would expect IsHttps() and Request.IsSecureConnection to always return the same value. Maybe it's just a matter of coming up with a suitable name. IsCurrentlyOrPreviouslyHttps() :)


Lee Gunn 29 Nov 2016, 13:32:06

Maybe I'm reading it wrong but does the code above not return true for IsHttps() if the client sends the header: "Front-End-Https" == "on"?

I think that's a vulnerability isn't it?

e.g.

  1. Bob makes a GET HTTP request for a login form.
  2. I intercept the request and append the header "Front-End-Https":"on"
  3. Server now thinks the request is secure and returns the login form over HTTP
  4. Bob then POSTs his username and password over HTTP
  5. I intercept his username and password


Morten Bock 29 Nov 2016, 14:38:32

Maybe there should be a distinction between configuring the client side code to use https, and setting up the server-to-server calls to be https.

If the load balancer terminates the https, then you probably want the server to request itself on http, and not through the load balancers https endpoint?

I'm thinking of usages like this: https://github.com/umbraco/Umbraco-CMS/blob/5397f2c53acbdeb0805e1fe39fda938f571d295a/src/Umbraco.Core/Sync/ConfigServerAddress.cs

Not sure if it is an actual problem, but might be worth testing out if this would be an issue when the actual servers do not run https.


Simon Dingley 29 Nov 2016, 14:47:15

@mortenbock It is an issue I see at the moment and I think the reason why the logs are being flooded with exceptions like this:

2016-11-29 13:21:10,358 [P8656/D3/T98] ERROR Umbraco.Web.Scheduling.ScheduledPublishing - Failed (at "https://staging.example.org.uk/umbraco"). System.Net.Http.HttpRequestException: An error occurred while sending the request. ---> System.Net.WebException: The underlying connection was closed: Could not establish trust relationship for the SSL/TLS secure channel. ---> System.Security.Authentication.AuthenticationException: The remote certificate is invalid according to the validation procedure.


Lee Gunn 29 Nov 2016, 15:03:59

@ProNotion in order to get round your particular problem right now, I think you're safe to just set GlobalSettings.UseSSL = false and then have some custom code that checks for the existence of the server header or IsSecureConnection and redirects to HTTPS if they are both false.

Umbraco will still be secure in the sense that it isn't directly connected to the world over HTTP.


Simon Dingley 29 Nov 2016, 15:08:52

@leeg If you're referring to the last message about the server to server communication I've added an entry in the host file on the servers to direct traffic via the Load Balancer. Not ideal but since I can't currently turn on GlobalSettings.UseSSL because of the infinite redirects it has to do for the moment until we can be satisfied this is working well enough for production.


Lee Gunn 29 Nov 2016, 15:26:16

@ProNotion ah sorry, forget my last reply. I forgot you were already redirecting with web.config.


Simon Dingley 29 Nov 2016, 15:28:50

We're currently working on removing the web.config redirect and forcing it on the LoadBalancer - this is not something I can do directly though so I'm working with the host on it.


Morten Bock 29 Nov 2016, 15:51:32

In case you want to do some testing locally, I think https://ngrok.com/ would be at nice way to set up an ssl terminating endpoint, forwarding requests to your local iis.


Sebastiaan Janssen 29 Nov 2016, 16:08:49

Just to note that umbracoUseSSL="false" is a bad idea if you want your backoffice to run over https. The authentication cookie will not be set as secure and thus it will be submitted over http (non-s) just fine. So it's easy to sniff.


Sebastiaan Janssen 29 Nov 2016, 16:19:00

@leeg Very good point, I wonder if this isn't a problem with all of this then, these checks seem to rely on header values..


Lee Gunn 29 Nov 2016, 16:28:07

@sebastiaan I think it's just the Request.Headers[] collection (used in the example above) that could cause problems as those headers are being sent from the client and therefore tweakable/hackable. Request.ServerVariables[] are set on the server so that should be fine (as used by load balancers)


Sebastiaan Janssen 29 Nov 2016, 17:07:44

Ah great, yes I misread it :-)


Morten Bock 29 Nov 2016, 17:10:22

On some solutions we have the load balancer forward https request to a different port. For example 10443. This would avoid the header spoofing.

But hopefully the load balancer would strip the secure flag headers from client request, and override with it's own value. And the servers should them not be accessible directly from the outside.


Lee Gunn 29 Nov 2016, 17:28:25

@mortenbock Oh yeah, from the msdn docs:

If a client request includes a header other than those specified in the IIS Server Variables table, you can retrieve the value of that header by preceding the header name with "HTTP_" in the call to Request.ServerVariables.

So, if client sends a request header of "X_FORWARDED_PROTO" : "true" and the load balancer doesn't handle the spoof properly then the web server will be fooled into thinking the request was secure.

Yikes, load balancers must surely already deal with this. Surely... :)

I need to find a load balancer to test this against!


Simon Dingley 17 Jan 2017, 16:08:01

I'm just revisiting this and came across the following [http://stackoverflow.com/questions/26904930/how-to-create-redirect-responses-on-iis-when-running-behind-ssl-terminating-reve, post on StackOverflow] which might be along the lines of a good solution to the problem - thoughts?


Sebastiaan Janssen 18 Jan 2017, 13:17:51

@ProNotion Promising, but it looks like you can't just set the allowedServerVariables without unlocking those first, so might not be viable.


Mark van Schaik 27 Mar 2017, 22:36:20

fix submitted, requires testing - https://github.com/umbraco/Umbraco-CMS/pull/1836

Also for people wanting https redirection for their websites, a better rule would be the following as it allows either normal https or use of the X-Forwarded-Proto header for load balancers/proxies.

<rule name="Force Https" stopProcessing="true">
  <conditions logicalGrouping="MatchAny">
    <add input="{HTTP_X_FORWARDED_PROTO}" pattern="https" negate="true" />
    <add input="{HTTPS}" pattern="on" negate="true" />
  </conditions>
  <action type="Redirect" url="https://{HTTP_HOST}{REQUEST_URI}" redirectType="Permanent" />
</rule>


Shannon Deminick 29 Mar 2017, 07:56:27

PR: https://github.com/umbraco/Umbraco-CMS/pull/1836/files


Richard Thompson 14 Sep 2017, 15:33:19

We are having the same issue. We have tried to enable umbracoUseSSL but the back office is infinitely redirecting. The site sits behind Varnish so the server variable HTTPS has a value of off but the server variable HTTP_X_HTTPS is set to on.

Would it be possible to update this change to support this?


Mark van Schaik 14 Sep 2017, 20:16:05

What's the issue implementing this? I submitted a pretty straightforward pull request fix over 6 months ago, it is a significant issue (affecting anyone using SSL over a variety of load balancers). The due version appears to be continually moved backwards.


Shannon Deminick 14 Sep 2017, 23:25:51

@Mark.van.Schaik like many of the above comments - all that is doing is making Umbraco think it's on SSL when it isn't simply by checking a header of which anyone that can access your site can pass in which means that anyone can fool your site into returning data over non-https which is not ideal. This is not a change we want to make for every user using Umbraco since that instantly opens up a security problem on their website. This is a tricky issue which is why we haven't just gone ahead and merged it.

It looks like different load balancers will specify different headers too, so there doesn't seem like one solution fits all.

There's a lot of articles online about various ways to do handle this with various load balancers and since we don't want to turn on header checks for default Umbraco installations that don't require this (thus leaving them vulnerable to header spoofing), the only way to resolve this is by either adding in some configuration values you can supply or by making the UmbracoRequireHttpsAttribute extensible (i.e. with policies that you can define on startup)


Ulrik Andersson 02 Oct 2017, 09:25:10

Also having this issue with AWS Load balancer. Seems to me the UseSSL setting does alot of stuff to make umbraco secure but it's only the redirect that's an issue? Would it be possible to add a new setting or override for just disabling the redirect?

I'm a bit worried that no ones assigned to this issue since umbraco is not secure now behind what it seems multiple different load balancers.


Simon Dingley 02 Oct 2017, 09:28:31

the only way to resolve this is by either adding in some configuration values you can supply or by making the UmbracoRequireHttpsAttribute extensible (i.e. with policies that you can define on startup)

I like the idea of this as it means Umbraco provides enough support for custom configurations without having to directly attempt to address a problem for which the solution is different for many people.


Wouter van der Beek 24 Oct 2017, 12:51:33

About two weeks ago we launched a new loadbalanced Umbraco website and we ran into massive problems due to the fact that our traffic between the load balancer and the webservers is HTTP. It is a multilingual installation where each root node is it's own website, but each with a different domain. The loadbalancer forwards the standard "HTTP_X_FORWARDED_PROTO" header to notify the webserver of the appropriate scheme.

All the domain names were specified without a scheme (ex. "foo.bar.com"). Because we enabled the umbraco.config option "useDomainPrefixes" (force Umbraco to render URL's with their full respective scheme+hostname instead of just the relative path) and because Umbraco regarded the request as non-secure, all URL's ended up being rendered as "http://..." instead of "https://...".

Our first attempt was to add the "https://" scheme in front of the domain name on the node, but this completely broke the website as Umbraco no longer could relate the incoming url "http://foo.bar.com" to the node's domain "https://foo.bar.com". This caused all traffic to end up on the same website (umbraco arbitrarily seems to pick the first website/node) and completely broke the website for all but one language.

We decided to dive into the Umbraco source code and we quickly found out that the UrlProvider seems to exclusively care for the current Url @ HttpContext.Request.Url. This means that regardless of any server headers (HTTPS="on", SERVER_PORT="443", SERVER_PORT_SECURE="1", HTTP_X_FORWARDED_PROTO="https") Umbraco will always return URL's with the http:// scheme.

For now we have solved the issue by building a custom HttpModule which detects the HTTPS header, and adds the HTTPS server variables when the secure connection is detected. It also "forcefully" changes the current HttpContext.Current.Request.Url to it's HTTPS scheme variant, which causes Umbraco to recognise the secure connection which fixes the issue for us for now. If anyone is interested in the solution, you can see the file I've attached in this post. (Note that this issue is exclusively intended to be used for a loadbalanced website). Don't forget to add the module in the web.config in the section and/or the section.

However this is but a "patch" to the real issue in Umbraco: the Core API should expose a service which can be overridden to determine if a connection is secure or what the current scheme is. We also have customers who use a completely different server header for their HTTPS forwarded calls, so I feel adding a service (as opposed to only checking the HTTP_X_FORWARDED_PROTO server variable) in the core is required.


Shannon Deminick 24 Oct 2017, 22:23:33

Thanks for all the info @wouter.vanderbeek@perplex.nl !

We definitely need to make something extensible/flexible for all of these scenarios - like a service as you suggest, or a callback, etc...

Maybe you could help us out with this statement

We decided to dive into the Umbraco source code and we quickly found out that the UrlProvider seems to exclusively care for the current Url @ HttpContext.Request.Url. This means that regardless of any server headers (HTTPS="on", SERVER_PORT="443", SERVER_PORT_SECURE="1", HTTP_X_FORWARDED_PROTO="https") Umbraco will always return URL's with the http:// scheme.

I haven't looked at this code for a while but regardless of the proxy headers, etc... what would you recommend is used as the default check for https instead of the HttpContext.Request.Url ?


Wouter van der Beek 25 Oct 2017, 09:00:57

I just sat down with Jeffrey to discuss possible options. We feel that the scenario with the non-secure traffic between the loadbalancer and the webserver is probably more of an exception rather than the norm. Therefor we would recommend using the Request.IsSecureConnection as the default to check for a secure connection.

Currently the DefaultUrlProvider simply uses "GetLeftPart" in most cases. This is a problem as the Current URL or the node's scheme could be non-HTTPS, even though the connection is secure (though this is only a problem for the scheme, not for the hostname). The URL's scheme should never be used (although currently you can force HTTPS url's by adding this scheme on the node's domain, so that "feature" would cease to function), and instead a specific check on whether or not the connection is secure should be used instead (which is a far better approach).

As for checking server variables: normally it would not make any sense to add loadbalancer-specific server variable checks in the Core. However there seems to be a precedent when it comes to checking loadbalancer server variables: The implementation for the verification of a user's IP address (see Umbraco.Core.HttpContextExtensions.GetCurrentRequestIpAddress) seems to check the HTTP_X_FORWARDED_FOR header, which is also a (semi-conventional) loadbalancer-specific server header. If it's Umbraco stance that this server variable should be supported, than I believe adding a check for the HTTP_X_FORWARDED_PROTO server variable should also be considered. In this case the HTTP_X_FORWARDED_PROTO server variable should be checked first, using Request.IsSecureConnection as the fallback (read:default) scenario.

If the above scenario were to be implemented, wrapped in an exposed service which can be replaced at the programmer's descretion, than anyone should be able to implmeent their own load-balancer specific solution fairly easily. If this service were to exist, adding the HTTP_X_FORWARDED_PROTO check into the default Umbraco service would just be a "nice to have" as the programmer could just check for this header themselves within their own implementation (although they would need to know a little bit about the load balancer setup, but they should anyway).


Shannon Deminick 31 Oct 2017, 06:58:43

I have started on this task but it is a deep dark rabbit hole actually and will take considerable testing as there are tons of cases to be considered. I'll post up my WIP PR tomorrow and will work with @zpqrtbnk on this as there is quite a lot of changes required in many places. Pretty much almost all GetLeftPart's need to be changed and an HttpRequestBase needs to be accessible in these places. So the change will be quite substantial and I don't think the changes can be made under a patch release and will need to wait until 7.8 (which is due out next month if all goes according to plan)


Shannon Deminick 31 Oct 2017, 07:09:37

WIP PR https://github.com/umbraco/Umbraco-CMS/pull/2275


Jeffrey Schoemaker 31 Oct 2017, 07:38:00

Hi @Shandem, thanks for the update. If you need any help / thoughts / etc, please let us know!


Jeffrey Schoemaker 23 Nov 2017, 14:12:05

Hi @Shandem, can you give an update by any chance? Is it still part of 7.8 and could I help you with any pre-beta testing?


Andy Felton 28 May 2018, 09:22:48

Hi @Shandem (@sebastiaan) just wondering if there is any news on this one, would certainly be happy to help with testing or in other ways if needed?


Jason Prothero 20 Jun 2018, 18:15:27

@wouter.vanderbeek@perplex.nl High 5 you rock! Thanks for posting your patch solution for this. It worked great once we added that header to the load balancer.

Perhaps there should be a Documentation page about this workaround until the core is updated?


Martijn de Wolf 18 Jul 2018, 09:34:15

We are running into the same problem at this moment on an Umbraco with version 7.7.8 We implemented the fix that @wouter.vanderbeek@perplex.nl suggested but i doenst seem to work for us.. We still receive the following errors in the backoffice; /umbraco/backoffice/UmbracoApi/UpdateCheck/GetCheck 417 (Missing token) /umbraco/backoffice/UmbracoApi/Section/GetSections 417 (Missing token)

Anyone else might have found a fix for this issue?


Sebastiaan Janssen 18 Jul 2018, 09:54:20

@mdewolf@haveanicedayonline.nl That's a different issue, check http://issues.umbraco.org/issue/U4-9873 for more details and possible solutions.


Angelbert David 31 Aug 2018, 15:44:55

Hi there, just checking in to see if there has been a fixed created for this yet? If so, what version of Umbraco would include it? thanks.


Priority: Up for grabs

Type: Bug

State: Review

Assignee:

Difficulty: Normal

Category: Security

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.5.3, 7.5.4, 7.6.11

Due in version:

Sprint:

Story Points: 3

Cycle: 9