Make NoPrivateNetworkHttpClient & proxy usage work better together

[mathroc]

Description

When using the NoPrivateNetworkHttpClient and there is a proxy set (eg: auto-discovered with the HTTPS_PROXY env var), if the proxy is local the request is blocked by the NoPrivateNetworkHttpClient.

I believe it would make sense to automatically authorize the proxy in this case, or skip the NoPrivateNetworkHttpClient entirely.

It's hard to figure out if we should skip adding NoPrivateNetworkHttpClient because the HttpClientInterface can't tell us if it's using a proxy or not. Even getting the inner client options and reproducing the logic it uses to figure out if and which proxy to use is not easy.

Ideally, the NoPrivateNetworkHttpClient could ask the inner client if it's using a proxy or not and not do anything when a proxy is used.

Example

No response

[nicolas-grekas]

NoPrivateNetworkHttpClient is opt-in, so that you should use it only on when running on a node where you need it.
The $subnet constructor argument could also be useful to whitelist the proxy.
Don't you know when you're using a proxy or not in your use case?

[mathroc]

In my use case, I learned that it was using a proxy when the code was deployed to production. 💥

now that I know that we removed the NoPrivateNetworkHttpClient wrapper and I hope the proxy will never be removed, so I don't mind too much whether this issue is fixed or not. Just wanted to suggest what, I hope, would be an improvement :)

[nicolas-grekas]

It makes sense yes. I think we don't need to have cooperation from decorated clients: the logic to retrieve the proxy is known (it's either in the options or in the env). Yes, the proxy can also be defined in the default options. But then, we could consider it's up to users to not wire this decorator when they configure a proxy.

In addition, we could define a set of hosts that should be rejected btw. This would allow forbidding internals hosts that could still be hit via the proxy. That would be just a regexp given as constructor argument.

Up to give it a try?

[mathroc]

the logic to retrieve the proxy is known (it's either in the options or in the env).

It's not that easy, there's HTTP_PROXY, HTTPS_PROXY, ALL_PROXY. Those, combined with redirections, would already mean that I can't know if the proxy will be used or not after a redirection if the scheme changes. And then there's also NO_PROXY which means I can't know if the proxy will be ignored before I know the URL (and its redirections)

In addition, we could define a set of hosts that should be rejected btw. This would allow forbidding internals hosts that could still be hit via the proxy. That would be just a regexp given as constructor argument.

Could we do the opposite: use the NoPrivateNetworkHttpClient but whitelist a few IPs for the proxies with a constructor argument? (array of IPs?)

[nicolas-grekas]

We can add an allow-list yes. It'd be up to users to properly configure this, but that's OK to me. As you wrote, auto-detection can be too hard.

[nicolas-grekas]

Another idea on this topic would be to take inspiration from proxy.pac functions. Maybe allowing something similar in PHP would provide the missing flexibility here.

[maxhelias]

Could we do the opposite: use the NoPrivateNetworkHttpClient but whitelist a few IPs for the proxies with a constructor argument? (array of IPs?)

Useful not just for the proxy, I think. I have 2 symfony applications that use the same private network to communicate with each other, only one of them is exposed to the public and for this one, I can't whitelist my private app without decorate NoPrivateNetworkHttpClient to authorize it.

If it's still a feature that we want to have, I can try to move things forwark.