Add support for no_proxy and case insensitive env variables

[tzyl]

Changes proposed in this PR

• Add support for the no_proxy and NO_PROXY environment variables
  • Internally rely on urllib's proxy_bypass_environment
• Extract env variables using urllib's getproxies/getproxies_environment which supports lowercase + uppercase, preferring lowercase, except for HTTP_PROXY in a CGI environment

This does contain behaviour changes for consumers so making sure these are called out:

• no_proxy/NO_PROXY is now respected
• lowercase https_proxy is now allowed and taken over HTTPS_PROXY

Related to #9306 which also uses ProxyAgent

Signed-off-by: Timothy Leung tim95@hotmail.co.uk

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[clokep]

@tzyl Overall looks good, give a shout when you think this is ready for a review!

[tzyl]

@clokep I think this is ready for review! I have one question that I'm not sure how to solve though. I changed from using getproxies -> getproxies_environment in the last commit because it does some platform specific fallbacks which I don't think proxy_bypass_environment works with. However, in this case mypy doesn't have the types for it do you have any suggestions here? (assuming continuing to use *_environment from urllib)

synapse/rest/media/v1/preview_url_resource.py:28: error: Module 'urllib.request' has no attribute 'getproxies_environment'  [attr-defined]
--
  | synapse/server.py:38: error: Module 'urllib.request' has no attribute 'getproxies_environment'  [attr-defined]
  | Found 2 errors in 2 files (checked 379 source files)
  | ERROR: InvocationError for command /workdir/.tox/mypy/bin/mypy (exited with code 1)
  | ___________________________________ summary ____________________________________
  | ERROR:   mypy: commands failed
  | 🚨 Error: The command exited with status 1

[clokep]

@clokep I think this is ready for review! I have one question that I'm not sure how to solve though. I changed from using getproxies -> getproxies_environment

Is that a thing? It isn't documented: https://docs.python.org/3/library/urllib.request.html#urllib.request.getproxies

Can you point to documentation on this?

For mypy you can either ignore the types or manually specify them.

[tzyl]

The relevant source code for all the proxy functions is this block:
https://github.com/python/cpython/blob/bdb941be423bde8b02a5695ccf51c303d6204bed/Lib/urllib/request.py#L2487-L2776

Default: getproxies -> getproxies_environment
Darwin: getproxies -> getproxies_environment or getproxies_macosx_sysconf
NT: getproxies -> getproxies_environment or getproxies_registry

I was initially worried about those fallbacks and whether they would be incompatible with proxy_bypass_environment but looking a bit deeper they do return the same format as in the documented getproxies function so I think it should be ok to continue to use getproxies which is better as it is in the documentation!

For what it's worth getproxies_environment git blames to 13 years ago and is used in requests so I think although undocumented it would probably be worthy of a mypy type as I think it also types (lint doesn't complain?) some other undocumented functions like proxy_bypass_environment which is used in this PR:
https://github.com/python/cpython/blame/bdb941be423bde8b02a5695ccf51c303d6204bed/Lib/urllib/request.py#L2488
https://github.com/psf/requests/blob/8c211a96cdbe9fe320d63d9e1ae15c5c07e179f8/requests/compat.py#L59

[clokep]

I don't think we should be using undocumented methods from CPython, we have some users using PyPy for example so we really need to be sticking to what is available in the standard library.

[tzyl]

Totally reasonable and I defer to your expertise! So my main priorities I would like here are:

• Consistent behaviour with standard conventions at minimum within the python ecosystem (urllib, requests) and ideally with all software
• Doesn't reinvent the wheel and rewrite tried and tested code to risk unexpected inconsistencies in behaviour

In this case from the two functions being used in this PR getproxies and proxy_bypass_environment:

• getproxies is documented
• proxy_bypass* is not documented but mypy does not complain about this
  • mypy has a comment in their repo about undocumented proxy_bypass although I don't specifically see proxy_bypass_environment in that file
  • requests assumes these exist

If we're not comfortable using the undocumented proxy_bypass functions from urllib do you have any suggestions on what you would prefer? (e.g. reimplementing that logic in synapse)

[clokep]

• mypy has a comment in their repo about undocumented proxy_bypass although I don't specifically see proxy_bypass_environment in that file

Should we use proxy_bypass instead of proxy_bypass_environment then? This seems to match what you found above that it is probably just a passthru.

[tzyl]

Good question, unfortunately proxy_bypass_environment is the only one that accepts an optional input of proxies. The others take the hostname only and default to discovering the proxies internally. In that case we could use proxy_bypass only in ProxyAgent but it would mean we can't specify a custom no_proxy which is inconsistent with the behaviour of http_proxy and https_proxy being passed in as parameters to ProxyAgent.

[tzyl]

Hey @clokep, any more thoughts here? It feels like to me the two most obvious paths forward currently are:

• Continue using urllib's proxy_bypass_environment which mypy accepts but is not documented
• Reimplement this function in synapse

[clokep]

I haven't looked at this deeply enough to have an opinion unfortunately. I'm going to put this into the team's review queue for someone to look deeper into!

[anoadragon453]

While undocumented, PyPy does seem to implement urllib.request.proxy_bypass_environment, which is probably good enough?

(env) ➜  test_pypy python       
Python 3.7.9 (7e6e2bb30ac5, Nov 18 2020, 10:55:52)
[PyPy 7.3.3-beta0 with GCC 7.3.1 20180303 (Red Hat 7.3.1-5)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>> from urllib.request import proxy_bypass_environment
>>>> proxy_bypass_environment
<function proxy_bypass_environment at 0x00007f839d8aa7a0>
>>>> proxy_bypass_environment("test.com", proxies={"no": "test.com,unused.com"})
True
>>>> proxy_bypass_environment("test.com")
False

[ShadowJonathan]

Isn't urllib.request pure python? If so, then i think support can be pretty much guaranteed, documented or not.

[anoadragon453]

After discussing this a bit in #synapse-dev, we're happy to use the undocumented functions, given they should work fine in PyPy.

As for which function to use, things become a little clearer after looking at the source for these functions. proxy_bypass/getproxies_environment will pull known proxies from environment variables, whereas proxy_bypass/getproxies are platform-specific functions which will first search the environment for proxies, and then if none are found, it will try to pull known active proxies from the system.

So I'm happy about using *_environment methods here. The latter sounds useful for a future PR, but for this PR we should remain backwards compatible and just pull proxy information from environment variables. I could see us creating a config option for Synapse to attempt to use the system proxy, but that can be done in a separate PR.

[tzyl]

Thanks for the review and clear direction @anoadragon453! I'll work on making those changes when I find time this week :)

[tzyl]

@anoadragon453 ready for another pass! I've made the following changes as requested:

• Use getproxies_environment directly to avoid platform specific fallbacks for further breaks in behaviour
• Change to a single use_proxy flag rather than specify all proxy variables separately
• Change tests to mock patch env variables

Interestingly, mypy no longer complains about getproxies_environment which it did when I first opened this PR, I wonder what changed?

[tzyl]

Is there any point in using ProxyAgent without use_proxy=True? I kept this default here to match the same behaviour as before if you didn't specify either of http_proxy or https_proxy but seems like it may be redundant now that this is combined into a single parameter

[anoadragon453]

I think you're right. Outside of proxying (and a single debug log) ProxyAgent doesn't provide much on top of twisted's default Agent (as it shouldn't!).

I think we could try just using an Agent if use_proxy=False is passed to SimpleHttpClient.

Similarly in tests.

[tzyl]

I think I'd rather leave this out of this PR for now to avoid making more significant changes to the code but sounds like it could be worth a follow up!

[anoadragon453]

Sounds sensible, OK 🙂

[anoadragon453]

Thanks for making the changes. This is looking great! A few more things to clean up and I think this will be ready to merge.

[anoadragon453]

I think you're right. Outside of proxying (and a single debug log) ProxyAgent doesn't provide much on top of twisted's default Agent (as it shouldn't!).

I think we could try just using an Agent if use_proxy=False is passed to SimpleHttpClient.

Similarly in tests.

[anoadragon453]

Looks good to me. Thanks for iterating on it with us!