Skip to content

[DX][RFC] Forcing to https causes a lot of troubles #27603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Aerendir opened this issue Jun 14, 2018 · 5 comments
Closed

[DX][RFC] Forcing to https causes a lot of troubles #27603

Aerendir opened this issue Jun 14, 2018 · 5 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@Aerendir
Copy link
Contributor

Symfony version(s) affected: from 2.8 I think, for sure from 3.4

Description
Forcing to https may lead to an infinite loop in some (unclear for me) circumstances.

It is something related to load balancers and trusted proxies.

How to reproduce

  1. Force to https, as described here, putting this in security.yaml
security:
    access_control:
        - { path: ^/, role: IS_AUTHENTICATED_ANONYMOUSLY, requires_channel: https }
  1. Deploy for example, to Heroku.
  2. Access the home page and you will get an error about too many redirects.

Possible Solution
There are two possible solutions to this problem and a required docs improvement:

  1. Manually adding the required $_SERVER variables:
// SF4: public/index.php
// SF 2||3: web/app.php

if($_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https')
{
    $_SERVER['HTTPS'] = 'on';
    $_SERVER['SERVER_PORT'] = 443;
}
  1. Adding trusted proxies to framework:
framework:
    trusted_proxies:  [127.0.0.1, ::1]
  1. Adding to the documentation a note about this common error and the possible solutions

WHAT I'M ASKING FOR

A simpler way to understand that there is a problem with the https redirection.

Maybe a clear exception in case of excessive number of redirects that tells the developer that it should either implement solution one or solution two.

This takes me 2 days to be solved now that I migrated to SF4 and takes some more days when I implemented https forcing using SF3.4 (I forgot about the problem, so I had to discover the solution two times, damn!).

Any opinions about this?

@javiereguiluz javiereguiluz added RFC RFC = Request For Comments (proposals about features that you want to be discussed) DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Jun 14, 2018
@stof
Copy link
Member

stof commented Jun 14, 2018

Maybe a clear exception in case of excessive number of redirects that tells the developer that it should either implement solution one or solution two.

the thing is, I don't think we can know about this excessive number of redirects, due to the stateless nature of HTTP (the browser can detect such case, as it is the one following redirection).

Among your possible solutions, the first proposal is not a solution. It is a hack trying to partial trust a proxy header without making it a trusted proxy (and if your app is accessible without going through the proxy, it might open a security hole in your app).
When deploying to heroku, the right solution is the second one: as heroku runs a proxy in front of your app (their load balancing router), you need to configure the proxy as trusted. and Heroku even documents it: https://devcenter.heroku.com/articles/getting-started-with-symfony#trusting-the-load-balancer

@Aerendir
Copy link
Contributor Author

@stof , you are right: the first solution is a hack and the preferred way of solving the problem is adding the trusted proxies.

Anyway, the problem is that, before understanding what is really going under the hood and from where it comes the redirection, you have to spend a lot of time.

I don't know if it is possible to intercept the loop: I cannot come up with a solution but maybe someone of you with more experience can :)

Anyway, if no solution is possible, the best approach would be to add to the docs a note about this problem that is known and intended, maybe also a link to the Heroku documentation or to the trusted_proxies... I don't know.

The only thing I know is that to discover the source of the loop I spent really a lot of time as this is something you set once and then forget.

Just as a side note, thank you for the link to the Heroku documentation: when I started with Heroku it didn't exist and I didn't find it searching for the issue on Google.

@colinodell
Copy link
Contributor

colinodell commented Jun 14, 2018

What about something like this in ChannelListener::handle()? (untested)

     public function handle(GetResponseEvent $event)
     {
         $request = $event->getRequest();
 
         list(, $channel) = $this->map->getPatterns($request);
 
         if ('https' === $channel && !$request->isSecure()) {
             if (null !== $this->logger) {
                 $this->logger->info('Redirecting to HTTPS.');
+                if ('https' === $request->headers->get('X-Forwarded-Proto')) {
+                    $this->logger->debug('Possible redirect loop - did you set the trusted_proxies correctly?');
+                }
             }

             $response = $this->authenticationEntryPoint->start($request);

             $event->setResponse($response);

             return;
         }

That would at least give you a hint in the logs.

Of course this is also one of those problems that once you (finally) figure it out the first time, it's easy to spot and fix again in the future.

Ninja-edit: revised the logger to use debug() and improved the log message.

@Aerendir
Copy link
Contributor Author

@colinodell , yes, but this is also something that yu set once and then forget about it.

If you only manage one application (like me), you forget about it.

I spotted the problem about 1 year ago the first time and now, upgrading to SF4 I had to spot it again as I completely forgot I used the hack...

More, as I didn't know anything about this problem, I used the hack, so my app was potentially vulnerable without I knowing nothing about.

For me a good approach would be to, other than logging the message like you shown, also to add a not in the documentation about the potential loop where is explained how to force https.

colinodell added a commit to colinodell/symfony that referenced this issue Jun 14, 2018
If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See symfony#27603
@colinodell
Copy link
Contributor

@Aerendir I have submitted a PR for both the logging message and documentation update

javiereguiluz added a commit to symfony/symfony-docs that referenced this issue Jun 18, 2018
…page (colinodell, javiereguiluz)

This PR was submitted for the 2.7 branch but it was merged into the 2.8 branch instead (closes #9924).

Discussion
----------

Link to proxy configuration docs from the routing scheme page

This should help developers avoid infinite redirect loops.

See symfony/symfony#27603

Commits
-------

671e13c Reword and added a note
c717e75 Link to proxy configuration docs from the routing scheme page
colinodell added a commit to colinodell/symfony that referenced this issue Jun 19, 2018
If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See symfony#27603
colinodell added a commit to colinodell/symfony that referenced this issue Jun 19, 2018
If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See symfony#27603
@fabpot fabpot closed this as completed Jun 20, 2018
symfony-splitter pushed a commit to symfony/security that referenced this issue Jun 20, 2018
If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See symfony/symfony#27603
symfony-splitter pushed a commit to symfony/security that referenced this issue Jun 20, 2018
…PS (colinodell)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[DX] Log potential redirect loops caused by forced HTTPS

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27603
| License       | MIT
| Doc PR        | n/a

If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See symfony/symfony#27603

Commits
-------

53048cec6d Log potential redirect loops caused by forced HTTPS
symfony-splitter pushed a commit to symfony/security-http that referenced this issue Jun 20, 2018
If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See symfony/symfony#27603
fabpot added a commit that referenced this issue Jun 20, 2018
…PS (colinodell)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[DX] Log potential redirect loops caused by forced HTTPS

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27603
| License       | MIT
| Doc PR        | n/a

If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.

See #27603

Commits
-------

53048ce Log potential redirect loops caused by forced HTTPS
Guikingone pushed a commit to Guikingone/symfony-docs that referenced this issue Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

5 participants