Using middleware with "checkProxyHeaders" but without "trustedProxies" does not raise a warning

[SvenRtbg]

We used to use version 0.4 from the old repo, and upgraded to 0.6 and eventually 1.0 of this repo.

In the 0.6 release, there was a change that now forces the user to provide an IP list of "trustedProxies", otherwise the determined IP address would always be the "REMOTE_ADDR". Before, one could determine the supposed remote address without a list.

This effectively makes it useless to allow "checkProxyHeaders = true" with "trustedProxies = []". It will silently be switched back to "checkProxyHeaders = false" without any notice or warning.

This would force us to have a list of all trusted proxies of our DEV, TEST and PRODUCTION environment, and also reveal all these numbers in every other environment, or juggle with different configurations. Nobody should be able to sneak in from anywhere, which is guaranteed by our infrastructure setup. Effectively, we do not need to configure trusted proxies, we are fine with determining the IP from the headers as-is.

Would you consider reverting that change?

[SvenRtbg]

Im referring to this commit: https://github.com/akrabat/rka-ip-address-middleware/commit/50fdfc91fde2c92b2a361b4096de21ecbd4154cf#diff-582a89505c8047e89cd1ce2025aecfe3

[SvenRtbg]

Note that the current README does not mention any requirements for the constructor parameters at all:

Check proxy headers

Note that the proxy headers are only checked if the first parameter to the constructor is set to true. If set to false, then only $_SERVER['REMOTE_ADDR'] is used.

I set that parameter to true, headers should be evaluated.

Trusted Proxies

You can set a list of proxies that are trusted as the second constructor parameter. If this list is set, then the proxy headers will only be checked if the REMOTE_ADDR is in the trusted list.

"If this list is set" - it is not, I'd expect the result to always use the proxy headers.

[SvenRtbg]

In addition, this package "akrabat/ip-address-middleware" cannot easily be downgraded to the previously working package, because it replaces "akrabat/rka-ip-address-middleware" in ALL versions.

Note that the "replace" property in Composer is dangerous and always has side effects.

[SvenRtbg]

Ping @batumibiz

[batumibiz]

Proxy addresses are extracted from the HTTP header that is received from the client. Such data can not be trusted by default.

Therefore, it makes sense to assign an IP via Proxy address only if the address of the proxy ($_SERVER['REMOTE_ADDR']) is in the trusted list.

[batumibiz]

As for "warnings", I think in this case they should not be throwed. An empty list of trusted addresses is quite a working situation. For example, in the finished CMS, I deleted all trusted addresses from the admin panel ...

[SvenRtbg]

I'm aware of the security situation in general.

Let's discuss the middleware's interface for a minute: You can choose to use the forwarded headers by setting the parameter to true, and you can choose to not have a list of trusted proxies.

From the interface perspective, having two independent parameters to control only one setting is misleading.

If a user wants to read the forward headers, and the decision is made to only allow this by providing a list of trusted proxies, then only having the list as a parameter is way better, because it clearly communicates that forward headers will only be evaluated when giving a trusted proxy list. If one does not want forward headers evaluated, he gives no list.

On the other hand, not having to provide a list of trusted proxies would enable users to fetch the "first" IP address from the forward headers.

Even if you give a list of trusted proxies, the trust still depends on what these proxies do. For them to be fully trustworthy, they'd effectively have to remove any client side forward header and then add their own. So knowing the IP of a proxy is not enough, and making people think that this makes their environment more secure is wrong. They have to know what "trust" has to be.

In my case, I can trust my "anonymous" proxies even though they are not enumerated by IP address.

In essence, my proposal would be like this:

1. Revert the requirement to provide a list of trusted proxies when evaluating forward header. (see pull request #16) This would align the software with the documentation.

2. If that is unacceptable, I'd propose to change the interface of the middleware to this:

    public function __construct( 
        array $trustedProxies = [], 
        $attributeName = null, 
        array $headersToInspect = [] 
    ) 

The current first boolean parameter is not useful and should be removed. Default behavior does not change, when giving an array of trusted proxies, checking the forward header is automatically enabled.

The documentation must be changed accordingly.

[akrabat]

Just a heads up to let you know that I've seen a set of notifications come in for this project and will go through them this week.

[akrabat]

Okay,

@SvenRtbg I see your argument and it makes sense. How about this:

Change the constructor to:

public function __construct(
        $checkProxyHeaders = false,
        array $trustedProxies = null,
        $attributeName = null,
        array $headersToInspect = []
    ) 

i.e. set $trustedProxies to null by default.

Then, if $checkProxyHeaders is set to true and $trustedProxies is not set to an array, then throw an exception. Setting $trustedProxies to an empty array would then be allowed, but would have to be a conscious act.

We'll also need to be explicit about this in the README.

How does that sound?

[SvenRtbg]

It will deliver the feature I want, for sure.

Does it improve the general situation? How likely is it that an implementation will fail to set the trusted proxies array, without setting an empty array? Just think about reading the proxies from a file, and then using explode() - if the string won't be read correctly, explode would still create an empty array.

Also I don't see that trusted proxies feature to be of very high value in regards to security. Yes, clients may fake the headers. But proxies may also allow these headers to be passed from their client, and they'd add only one more bit of information, when in fact they should remove any possible fake header and create their own. So correctly configuring the outermost proxy is essential - and then not allowing any bypassing network connections. If you don't know what your proxy does, you cannot trust it.

I'd probably see this feature more like a feature toggle: In case your outside connection is going via a proxy, but the developers have direct access, they could "not" fake their IP when the trusted proxy array is set.

[SvenRtbg]

I've updated my pull request accordingly.

[akrabat]

Fixed by the merge of #16 into 1.0.1