fideloper / TrustedProxy

Laravel Proxy Package for handling sessions when behind load balancers or other intermediaries.
MIT License
7.36k stars 126 forks source link

[Idea] Load the trusted proxies from an environment variable #81

Closed vitorbaptista closed 2 years ago

vitorbaptista commented 7 years ago

It would be useful to be able to pass a list of trusted proxies via an environment variable, for example TRUSTED_PROXIES=192.168.0.10,172.10.0.4.

My specific use case is running CachetHQ. They trust CloudFlare's IPs by default, but I need to add my local nginx reverse proxy IP to that list. My only option now is to change that file directly, which isn't ideal.

Although the reason I'd like this is very specific, there're other cases where it would be useful. For example, trusting different proxies in different environments (dev, qa, staging, production).

What do you think?

fideloper commented 7 years ago

Hi!

Answering from my phone, so no fun github formatting here, but here's my idea for now:

I'd suggest using a config() to set trusted proxies within the trusted proxy middleware. Then you can set that config value in your own config/proxy.php file, which can use env() to read from your .env file.

As you might know, the reason I say to use both config and env is because that lets you use config caching if you'd like (env() can only be used within config files when using config caching).

On Fri, Aug 25, 2017 at 07:47 Vitor Baptista notifications@github.com wrote:

It would be useful to be able to pass a list of trusted proxies via an environment variable, for example TRUSTED_PROXIES=192.168.0.10,172.10.0.4.

My specific use case is running CachetHQ https://github.com/CachetHQ/Cachet. They trust CloudFlare's IPs by default https://github.com/CachetHQ/Cachet/blob/613b7445fbfa020afc6c83bfcca16a2832807203/config/trustedproxy.php, but I need to add my local nginx reverse proxy IP to that list. My only option now is to change that file directly, which isn't ideal.

Although the reason I'd like this is very specific, there're other cases where it would be useful. For example, trusting different proxies in different environments (dev, qa, staging, production).

What do you think?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fideloper/TrustedProxy/issues/81, or mute the thread https://github.com/notifications/unsubscribe-auth/AAch0z5BnLcowZqeMtx8-CwBautnqaVsks5sbsJ1gaJpZM4PCnhw .

vitorbaptista commented 7 years ago

@fideloper Thanks for the hint! That sounds like a good approach in general. However, as it requires changing the code itself, it's not ideal for when you're using somebody's else code. Back to my example with CachetHQ, ideally I'd like to be able to configure it without having to change its code, so I could (for example) deploy it to Heroku without having to fork it to change its configurations.

You could argue that CachetHQ itself should implement it. However, I think this feature is useful for any of the many projects that use TrustedProxy, so would be better to implement here IMHO.

I'm not a PHP developer, but if you think it's useful, I'd be happy to give it a go and try submitting a PR.

fideloper commented 7 years ago

Oh that depends on your Laravel version - in 5.5 you wouldn't need to edit TrustedProxy code, you'd instead edit the middleware: https://github.com/laravel/laravel/blob/develop/app/Http/Middleware/TrustProxies.php#L15

In earlier Laravel, you could still put that logic inside of config/trustedproxy.php I think, however, and not edit any core code.

vitorbaptista commented 7 years ago

@fideloper As far as I understand, you'll still need to change a file. That's what I'd like to avoid. As the trusted proxies can potentially be different between environments, they're a good candidate to be extracted to an environment variable IMHO, following Heroku's 12 factor app

However, if I got it correctly, Laravel >=5.5 doesn't use your TrustedProxy anymore, but has this functionality in its core, right? In that case, it would be better to send this proposal to Laravel itself and see what they think.

Thanks a lot for your work and your time :v:

fideloper commented 7 years ago

I'm pretty sure what I said was a way to incorporate ENV vars. Otherwise you're editing a config file that's intended to be edited.

Laravel 5.5 uses TrustedProxy but incorporates it as a middleware instead of having you publish the vendor configuration, so it's a little different.

vitorbaptista commented 7 years ago

Sorry for the delay in responding.

I re-read our messages in this thread, and what I understood was that you suggested to create a custom config/trustedproxy.php file that loads the configuration from the environment. Something similar to the example on your README:

<?php

return [
    'proxies' => [
        // Instead of hardcoding the proxies here, load from env()
    ],
    // ...
];

?>

You talk about the need to have it inside a config(), but let's ignore these implementation issues for now.

Have I understood it correctly?

fideloper commented 7 years ago

Yeah - the package actually includes a config/trustedproxy.php file (which was created before TrustedProxy was implemented as a Middleware into the laravel/laravel project by Taylor). You should be able to publish that as shown in the readme file here.

Something like this:

<?php

return [
    'proxies' => explode(',', env('TRUSTED_PROXIES')),
    // ...
];

?>

And then, if you're on Laravel 5.5 which includes the TrustProxies middleware, you can set protected $proxies; to config('trustedproxy.proxies');

You might need to adjust that middleware class a bit and over-ride the constructor in order to set a value to protected $proxies; when the middleware is instantiated.

vitorbaptista commented 7 years ago

I see. So, this is what I'd like to avoid: editing a file. Back to my original example, I'm trying to deploy CachetHQ, but to do so I need to change its trusted proxies (i.e. change this configuration file). If I were to deploy it to Heroku, for example, where I can't change the files manually, I'd have to:

  1. Fork CachetHQ
  2. Edit the file in my fork
  3. Deploy my fork to Heroku

If I have multiple environments with different proxy configurations (prod, QA, etc), this needs to be repeated for each of them.

If, for example, TrustedProxy itself implemented something like your example, this wouldn't be necessary. I would just need to change the environment variable.

CachetHQ itself could implement this. However, as this issue isn't specific to CachetHQ, but any software that uses TrustedProxy, it feels more useful to implement in TrustedProxy itself. This way, any project that uses TrustedProxy would be able to be configured via env variables.

WDYT?

fideloper commented 7 years ago

Sure, I can definitely see that being a pain!

So it sounds like you want include the proxy package (is it already there in Cachet?) and avoid editing the core code for CachetHQ.

There’s a few issues there -

It’s possible you (or any user) might need to edit the middleware no matter what if you need any custom headers, since pulling in the array of headers to listen for env vars would be difficult at best (that’s a bit of a limitation with how symfony is setup, altho probably something that could be worked around).

One other stumbling block is that making this change a core change to the package actually also involves updating the laravel/laravel project (so the middleware listens for env vars) which I don’t have control over! It’s something we can maybe PR into laravel, however.

On Mon, Oct 9, 2017 at 17:19 Vitor Baptista notifications@github.com wrote:

I see. So, this is what I'd like to avoid: editing a file. Back to my original example, I'm trying to deploy CachetHQ, but to do so I need to change its trusted proxies (i.e. change this configuration file). If I were to deploy it to Heroku, for example, where I can't change the files manually, I'd have to:

  1. Fork CachetHQ
  2. Edit the file in my fork
  3. Deploy my fork to Heroku

If I have multiple environments with different proxy configurations (prod, QA, etc), this needs to be repeated for each of them.

If, for example, TrustedProxy itself implemented something like your example, this wouldn't be necessary. I would just need to change the environment variable.

CachetHQ itself could implement this. However, as this issue isn't specific to CachetHQ, but any software that uses TrustedProxy, it feels more useful to implement in TrustedProxy itself. This way, any project that uses TrustedProxy would be able to be configured via env variables.

WDYT?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/fideloper/TrustedProxy/issues/81#issuecomment-335305810, or mute the thread https://github.com/notifications/unsubscribe-auth/AAch04q9NmjQTmgA1r1W-HWBCoYzcledks5sqpwAgaJpZM4PCnhw .

vitorbaptista commented 7 years ago

So it sounds like you want include the proxy package (is it already there in Cachet?) and avoid editing the core code for CachetHQ.

It's already in CachetHQ. This all started because I had to edit their config to add local IP addresses, fo which I sent a PR to them (see https://github.com/CachetHQ/Cachet/pull/2694). Curiously, another person suggested loading via env variables in the PR as well.

It’s possible you (or any user) might need to edit the middleware no matter what if you need any custom headers, since pulling in the array of headers to listen for env vars would be difficult at best (that’s a bit of a limitation with how symfony is setup, altho probably something that could be worked around).

Certainly! There will still be cases where the user will need to edit the config file itself, as environment variables are limited. My intent here is to reduce this need, not eliminate it.

One other stumbling block is that making this change a core change to the package actually also involves updating the laravel/laravel project (so the middleware listens for env vars) which I don’t have control over! It’s something we can maybe PR into laravel, however.

I'm happy to take this issue to Laravel. I imagine they already have something in place to handle environment variables, so I'm hoping it won't be a big change.

What do you think is the best way to move this forward?

dirkam commented 4 years ago

Yeah - the package actually includes a config/trustedproxy.php file (which was created before TrustedProxy was implemented as a Middleware into the laravel/laravel project by Taylor). You should be able to publish that as shown in the readme file here.

Something like this:

<?php

return [
    'proxies' => explode(',', env('TRUSTED_PROXIES')),
    // ...
];

?>

And then, if you're on Laravel 5.5 which includes the TrustProxies middleware, you can set protected $proxies; to config('trustedproxy.proxies');

You might need to adjust that middleware class a bit and over-ride the constructor in order to set a value to protected $proxies; when the middleware is instantiated.

This is a very old post, but it's still open and it might be useful for others, too. With Laravel 6.x you don't even need to touch the Middleware or anything. It's enough to publish the config and change it to read the env variable, as stated in your example. Laravel will read and parse it.