Skip to content
This repository has been archived by the owner on Jun 30, 2020. It is now read-only.

HTTPS terminated at load balancer #41

Closed
mdevine82 opened this issue Sep 28, 2016 · 9 comments
Closed

HTTPS terminated at load balancer #41

mdevine82 opened this issue Sep 28, 2016 · 9 comments

Comments

@mdevine82
Copy link

So I found the following issue when implementing my site. I had originally tested it on servers that were using local self-signed certs, so everything was great.

$app->add(Middleware::TrailingSlash(false)->redirect(301));
$app->add(Middleware::Https());

HTTPS
Because I moved to a system that was terminating the SSL on the load balancer the HTTPS middleware was causing infinite redirects. Its not really a bug but took me a hot second to figure out what the heck was going on.

TrailingSlash
Then because the SSL is terminated when TrailingSlash would fire, it would send the redirect back but with an http:// protocol because that is how the request came in. As far as I'm aware the following headers are somewhat of a standard for this type of situation. Do you think it would be prudent to look for these headers and if the they exist change the URI? You could maybe if just add it as a chained method that would checkHTTPSForward or something.

HTTP_X_FORWARDED_PROTO: https
HTTP_X_FORWARDED_PORT: 443
@oscarotero
Copy link
Owner

Interesting. I think this should be handled by Https middleware in the following way:

  • Https should be executed before trailingSlash
  • If the request is http, but includes one of these headers, and the checkHttpsForward option is enabled, do not redirect
  • And if the response received is a redirect (status 301, 302) and the location header is http, change to https.

What do you think?

@mdevine82
Copy link
Author

Yeah I think that sounds like it would work well.

@oscarotero
Copy link
Owner

I've made a commit with this. Can you test it before release a new tag?

@mdevine82
Copy link
Author

Just tested it out, works as I would expect it to.

@mdevine82
Copy link
Author

@oscarotero just wanted to just in on if you were planning on tagging and releasing this?

@oscarotero
Copy link
Owner

Yep, I just need to change the headers name, because the names cannot be HTTP_X_FORWARDED_PROTO or HTTP_X_FORWARDED_PORT (as they are in $_SERVER), but are X-Forwarded-Proto and X-Forwarded-Port. (are you sure that worked?)

oscarotero added a commit that referenced this issue Oct 11, 2016
@oscarotero
Copy link
Owner

Changed.
Can you test it again, please? If it's ok, I'll release the 3.17.0 version

@mdevine82
Copy link
Author

Your way is actually the better way to do it but it actually did work before. I was curious as to why that was and its because you are using the getHeaderLine() function. So here is the basic call stack:

$req->getHeaderLine('HTTP_X_FORWARDED_PORT');
Headers Class ->has();
Headers Class ->normalizeKey()

So Slim actually parses the headers and strips the HTTP_ as part of the normalizeKey function. It does this both when it builds the Headers object and when you run getHeaderLine() as a means to make it more idiot resistant (nothing is idiot proof). So note the strpos and substr methods below that strip the 'http-' (they convert the _ to a dash just above)

    public function normalizeKey($key)
    {
        $key = strtr(strtolower($key), '_', '-');
        if (strpos($key, 'http-') === 0) {
            $key = substr($key, 5);
        }

        return $key;
    }

So in a nutshell, it works now and it works in a more understandable way. Kudos.

P.S. Sorry for the long response

@oscarotero
Copy link
Owner

Ok, got it (I didn't know slim does that).
v3.17.0 released

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants