Using middleware with "checkProxyHeaders" but without "trustedProxies" does not raise a warning about ip-address-middleware HOT 12 CLOSED

Im referring to this commit: akrabat/rka-ip-address-middleware@50fdfc9#diff-582a89505c8047e89cd1ce2025aecfe3

from ip-address-middleware.

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.

from ip-address-middleware.

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.

from ip-address-middleware.

Ping @batumibiz

from ip-address-middleware.

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.

from ip-address-middleware.

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 ...

from ip-address-middleware.

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.

from ip-address-middleware.

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.

from ip-address-middleware.

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?

from ip-address-middleware.

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.

from ip-address-middleware.

I've updated my pull request accordingly.

from ip-address-middleware.

Fixed by the merge of #16 into 1.0.1

from ip-address-middleware.