Skip to content
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

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

Closed
vitorbaptista opened this issue Aug 25, 2017 · 11 comments
Closed

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

vitorbaptista opened this issue Aug 25, 2017 · 11 comments

Comments

@vitorbaptista
Copy link

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
Copy link
Owner

fideloper commented Aug 25, 2017 via email

@vitorbaptista
Copy link
Author

@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
Copy link
Owner

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
Copy link
Author

@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 ✌️

@fideloper
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

fideloper commented Oct 7, 2017

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
Copy link
Author

vitorbaptista commented Oct 9, 2017 via email

@fideloper
Copy link
Owner

fideloper commented Oct 9, 2017 via email

@vitorbaptista
Copy link
Author

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 cachethq/cachet#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
Copy link

dirkam commented Jun 5, 2020

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.

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

No branches or pull requests

3 participants