Page MenuHomeDevCentral
Differential D25

Allow to serve https:// links behind a front-end server with SSL termination
ClosedPublic
Actions

Authored by dereckson on Jul 13 2015, 20:53.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 06:46
Unknown Object (File)
Mon, Nov 18, 15:22
Unknown Object (File)
Tue, Nov 12, 00:47
Unknown Object (File)
Sun, Nov 10, 04:43
Unknown Object (File)
Sun, Nov 10, 04:42
Unknown Object (File)
Thu, Nov 7, 17:51
Unknown Object (File)
Tue, Nov 5, 06:43
Unknown Object (File)
Fri, Nov 1, 14:04
View All Files
Subscribers
security

Details

Reviewers
fauve
xcombelle
Maniphest Tasks
T492: When using a front-end server with SSL termination, back-end serves http:// links.
Commits
rGROVE0f370cd5307d: Allow to serve https:// links behind a front-end server with SSL termination
Summary

A new TrustProxy middleware allows to set a trust strategy about
potentially spoofable headers like HTTP_X_FORWARDED_PROTO and
HTTP_X_FORWARDED_FOR. This solves the proxy HTTPS problem.

We an app.proxy configuration option defined like this:

Auth Grove can handle proxy headers like HTTP_X_FORWARDED_PROTO
according your configuration.

  • To always trust forward headers, adds a star entry: ['*']
  • To never trust any server, use an empty array: []
  • To specify the proxies servers, create an array with each IP.

If you put Auth Grove on an back-end application server, with a
front-end nginx responsible for SSL termination,
you can set the front-end IPs or blindly trust any remote
address with a magic entry '*'.

Fixes T492.

Test Plan
  • Run Auth Grove on an Apache server, serving HTTPS
  • Access https:// served by a nginx server, with proxy_pass
  • See we have http:// if app.proxy is []
  • See we have https:// if app.proxy is ['*'] or the nginx server IP.

Diff Detail

Repository
rGROVE Auth Grove
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dereckson updated this revision to Diff 51.Jul 13 2015, 20:53
dereckson retitled this revision from to Allow to serve https:// links behind a front-end server with SSL termination.
dereckson updated this object.
dereckson edited the test plan for this revision. (Show Details)
dereckson added reviewers: fauve, xcombelle.
dereckson added a task: T492: When using a front-end server with SSL termination, back-end serves http:// links..
dereckson added a subscriber: security.
xcombelle requested changes to this revision.Jul 14 2015, 09:37
xcombelle edited edge metadata.

The official doc for trusted proxy looks like https://github.com/fideloper/TrustedProxy

according to this doc it looks like lacking of this part https://github.com/fideloper/TrustedProxy#add-the-service-provider

I think according to the docs
proxy => '*' and proxy => [ip1,ip2] should handle both TrustAll and Trust Some case
I think even if proxy => [] should handle the TrustNone case.

If my suppositions are right the code could be simplified to

public function handle($request, Closure $next)
  {
    $proxy = Config::get('app.proxy',default=[]);

    $request->setTrustedProxies($proxy);
   
    return $next($request);
  }

and the app part would become

/*
|--------------------------------------------------------------------------
| Proxies serving requests
|--------------------------------------------------------------------------
|
| Auth Grove can handle proxy headers like HTTP_X_FORWARDED_PROTO according
| your configuration.
|
|  - To always trust forward headers, proxy => '*'
|   - To never trust any server, proxy =>   []
|   - To specify the proxies servers, use an array with each IP.
|
| If you put Auth Grove on an back-end application server, with a front-end
| nginx responsible for SSL termination, you can set the front-end IPs or
| blindly trust any remote address with a magic entry '*'.
|
*/

'proxy' => '*',

My second part of concern is that the config is not safe by default so the value in the default config file should be trust none.

My third and major part of concern is that there is none automated test.

A bonus would be an early check of validity of the proxy value.

This revision now requires changes to proceed.Jul 14 2015, 09:37
dereckson added a comment.EditedJul 14 2015, 16:29

Thanks for the review.

Composer package fideloper/TrustedProxy

In D25#413, @xcombelle wrote:

The official doc for trusted proxy looks like https://github.com/fideloper/TrustedProxy

according to this doc it looks like lacking of this part https://github.com/fideloper/TrustedProxy#add-the-service-provider

Let's clarify this is not an official Laravel documentation, but a third party package offering the same functionality than we plan to add to our code.

They claim to offer a Laravel Proxy Package for handling sessions when behind load balancers or other intermediaries.

It offers interesting things, like a list of trusted proxies (e.g. Cloudshare) and less interestings like a service provider for Laravel 4.

Commit message should be amended to credit them by the way, if we're looking their code as inspiration.

Simplication

Well caught this idea of simplification, we're trying that. If confirmed, we should decide if we plan to offer Enum in the future or revert the commit offering these Enum.

I'm going to inquire that this evening.

Security

Valid concern.

My rationale were:

  • We'll deploy behind a trusted proxy (our front-end nginx), default install will be suggested through Docker, and so most usually behind also a Nginx or other load balancer / front-end server.
  • Worst case is https when it should use http. So it would post to the default https site of the server is site is not mirrored.
  • But as it handles authentication, it should always be deployed to use https.

You're right, to put our assertions and use of the product in the generic config isn't ideal.

The administrative cost to note ['*'] or the front-end IP(s) is rather cheap.

Unit testing

That's testable, indeed. I'm adding something like:

  1. Create a request with HTTP_X_FORWARDED_PROTO to 'https', without trusted proxy -> gets the port, 80 expected
  2. Same in ['*'] -> gets the port, 443 expected

See for example https://github.com/symfony/HttpFoundation/tree/master/Tests for such approach.

Early check of validity of the proxy value

Aye, that will avoid strange behavior for a missed dot.

Let's fail loudly if anything other than an IP is set.

xcombelle added a comment.Jul 14 2015, 17:10

about unit test

I think also the cases:

  • a proxy is set but a different is used should also be tested
  • if a check is added to ip values, the fail loudly should also be tested
  • the ipv6 should be also tested either it is supported or not (if not supported it should fail)
dereckson mentioned this in T839: Current URL detection code in views aren't coherent.May 21 2016, 22:24
dereckson added a comment.May 31 2016, 18:48

To address security concerns, I'm going to change this, so no proxy is trusted by default.

We'll allow Docker configuration to request [*].

As the configuration is cached by Laravel, this is satisfactory to introduce a ternary here.

config/app.php
39

Whitespaces seem suspect

49

[] by default, but some .env option TRUST_ALL_PROXIES to set ['*']

dereckson updated this revision to Diff 915.May 31 2016, 19:14
dereckson edited edge metadata.

Rebased against master.

dereckson updated this revision to Diff 916.May 31 2016, 19:21
dereckson edited edge metadata.

Adressed security concerns:

  • Proxies won't be trusted by default
  • README updated to document the -e TRUST_ALL_PROXIES=1 for Docker
dereckson updated this revision to Diff 924.Jun 1 2016, 17:40
  • Tests have been added for TrustProxy None/All. It runs independantly of the main test suite, as the configuration doesn't seem to be refreshed between tests, even using refreshApplication method.
  • REMOTE_ADDR is now handled through Symfony Request component
  • Add a link to Symfony trust proxy method documentation
dereckson updated this revision to Diff 927.Jun 1 2016, 17:43

Rebased against 0d90fbb8ff2c.

dereckson updated this revision to Diff 930.EditedJun 1 2016, 18:10
  • Rebased against 2cb53eabe
  • Fixed typo (found by arc lint spelling)
dereckson marked 2 inline comments as done.Jun 1 2016, 18:12
dereckson updated this revision to Diff 931.Jun 1 2016, 18:30

Test for some proxy scheme.

dereckson updated this revision to Diff 932.Jun 1 2016, 20:36

Ready to merge. @xcombelle Could you check that looks good to you?

xcombelle added a comment.Jun 1 2016, 23:26

@dereckson Yes it's ok I would just nitpick that the line in the example of the README -e TRUST_ALL_PROXIES=1 \ is an unecessary example of unsecure use and a better line would be -e TRUST_ALL_PROXIES=0\ But that is really too much nitpicking for saying no to the patch

xcombelle accepted this revision.Jun 1 2016, 23:27
xcombelle edited edge metadata.
This revision is now accepted and ready to land.Jun 1 2016, 23:27
dereckson added a comment.Jun 2 2016, 04:36
In D25#6159, @xcombelle wrote:

@dereckson Yes it's ok I would just nitpick that the line in the example of the README -e TRUST_ALL_PROXIES=1 \ is an unecessary example of unsecure use and a better line would be -e TRUST_ALL_PROXIES=0\ But that is really too much nitpicking for saying no to the patch

What do we call proxies in this discussion? Any intermediary web server serving the request. This could be a web server with the responsibility to dispatch requests elsewhere or this could be a cache like Varnish.

For example, for Nasqueron the request will have the following life:

client browser > Dwellers front end nginx > Docker container nginx > Docker php-fpm

The client will request https://login.nasqueron.org and that will reach the front end nginx.

The front end configuration is here:

P196 Server block for login.nasqueron.org
1 # Force a redirection from http:// to https://
2 server {
3 listen 80;
4 listen [::]:80;
5 server_name login.nasqueron.org;
6
7 include letsencrypt;
8
9 return 301 https://$host$request_uri;
10 }
11
12 server {
13 server_name login.nasqueron.org;
14
15 include ssl_params;
16 ssl_certificate /data/letsencrypt/etc/live/login.nasqueron.org/fullchain.pem;
17 ssl_certificate_key /data/letsencrypt/etc/live/login.nasqueron.org/privkey.pem;
18
19 # Docker container: login
20 include proxy_params;
21 location / { proxy_pass http://localhost:25080; }
22
23 # 502 error
24 root /var/wwwroot-502/$server_name;
25 error_page 502 /502.html;
26 location /502.html {}
27 }

At this moment, nginx will call the container nginx with the information it's for an https. This is this information we're trusting.

In such Docker scenario, as long as Amazon EC2 + load balancer, GCE, Heroku, OpenShift and other PaaS we'll always be in this configuration.

Yet, for everyone of these scenario, this will be totally secure, as long as the front end web server is well configured. And the good news is the default Apache or nginx configuration correctly sanitize: the web servers remove every HTTP_ header submitted by the user

For example, if we look the proxy_params file shipped with the Debian package for nginx, we see the following entries:

Header set for back endSourceData
Hostoriginal requestHost
X-Real-IPfront-end serverTCP connexion IP
X-Forwarded-Forfront-end serverfrom proxy_pass
X-Forwarded-Forfront-end serverfrom proxy_pass

As we can see, only Host: is trusted from the original request, which makes sense, as this is the requested site. All other HTTP headers from the original request will be discarded. Others values are derived from the front-end configuration, so trusted.

If the system administrator forgets to include or write manually these rules, this is still not a problem, it will get strange http://<value given to proxy_pass> links, probably not usable.

So this is secure, because the web servers do a very secure job to sanitize headers.

The only case it's not secure is when we expose directly the application to the world, but in this case, Docker README isn't interesting, and default TRUST_ALL_PROXIES value is false, so all is fine.

The Symfony documentation agrees with this analysis: http://symfony.com/doc/current/cookbook/request/load_balancer_reverse_proxy.html#but-what-if-the-ip-of-my-reverse-proxy-changes-constantly recommends to specify when possible the proxy address, but to transfer the headers security responsibility when the address is dynamic and will change.

dereckson updated this revision to Diff 933.Jun 2 2016, 04:58
dereckson edited edge metadata.

Improve Docker command, so out of the box Docker installation won't expose the port to world.

Closed by commit rGROVE0f370cd5307d: Allow to serve https:// links behind a front-end server with SSL termination (authored by dereckson, committed by dereckson). · Explain WhyJun 2 2016, 04:59
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
5 lines
5 lines
app/
Enums/
16 lines
Http/
1 line
Middleware/
59 lines
config/
20 lines
1 line
tests/
standalone/
53 lines

Diff 934

View Options

Makefile

Loading...
View Options

README.md

Loading...
View Options

app/Enums/TrustProxyConfigurationMode.php

Loading...
View Options

app/Http/Kernel.php

Loading...
View Options

app/Http/Middleware/TrustProxy.php

Loading...
View Options

config/app.php

Loading...
View Options

phpunit.xml

Loading...
View Options

tests/standalone/ProxyTest.php

Loading...
Nasqueron DevCentral · If it had been much bigger the moon would have had a core of ice. · Powered by Phabricator