replace RequestsFetcher for Urllib3Fetcher

[NicholasTanz]

Description of the changes being introduced by the pull request:

Fixes #2751

[NicholasTanz]

Hi @jku, I implemented a FetcherInterface that utilizes urllib3, most of the implementation is quite similar to RequestsFetcher. For the tests, I swapped out the RequestsFetcher for the Urllib3Fetcher in test_fetcher_ng.py. (I haven't removed the requests_fetcher.py yet)

[jku]

On first pass this looks really good, thanks -- I was worried there might be some differences that makes this complicated but your work seems very promising. Still, we'll have to be careful not to change any details we don't intend to as this is very core client code that easily affects applications (I mention this in case the review takes a while: it's only because I know this is a potentially tricky change).

I think we can probably even drop the multiple poolmanagers: this manual connection pool management was added at a time when this wasn't part of the http library but now I believe the poolmanager handles exactly the issues the _get_poolmanager() code is working around. I'm going to review this better in the next days so I'll look into this.

[jku]

if we change the default fetcher already in this PR then we'd want to change the requirements as well (pyprojects.toml, requirements/main.txt and regenerate requirements/pinned.txt) but we can still discuss if there's benefit to shipping this as alternative (non-default) fetcher first. Maybe there isn't -- if we don't think anyone is going to use and test it as an alternative then our only real choice is to swap the defaults

However, I think this change in dependencies would remove certifi from the indirect dependencies -- we'll have to take look at that and decide what is correct for python-tuf:

• what should the default certificate source be (certifi is the status quo but we may need a few lines of code to get the connectionpool to use it)
• do we allow users to change this default? how?

[jku]

we may need a few lines of code to get the connectionpool to use it

this is correct: by default urllib3 uses the OS certificate store

[NicholasTanz]

I think it makes sense to swap out the defaults in this PR, if it isn't too much overhead for changing the requirements. For the certificate source, I think it makes sense to utilize certifi instead of the OS certificate source, based on this page it seems like a quick swap.

[NicholasTanz]

I think we can probably even drop the multiple poolmanagers: this manual connection pool management was added at a time when this wasn't part of the http library but now I believe the poolmanager handles exactly the issues the _get_poolmanager() code is working around. I'm going to review this better in the next days so I'll look into this.

sounds good, I was looking at this page and I think it is supported, so I can remove _get_poolmanager() / _poolManagers and just use one of them. I was wondering if I should pass in a different value for the num_pools parameter, or if the default amount of 10 works.

[jku]

Leaving some feedback now:

• let's leave the certificate store decision open for now: William has a good point that certifi is problematic in the linked sigstore-python issue...
• I believe we can drop the manual session management and just use a single PoolManager
• The timeouts need a bit of work: we are supposed to handle timeouts specifically so we can raise SlowRetrievalError... but it looks to me like that will actually mean little more work with urllib3 (since by default we get automatic retries which I believe means any timeouts are actually wapped in MaxRetryError)
  • I think we could add some error parsing in _fetch and _chunks to only raise SlowRetrievalError on these cases
  • but on the other hand I don't want complex code for a feature I don't really think is useful (defence against slow retrieval attacks is not really possible at library level IMO) -- so let's not overdo this

[jku]

For the record I was looking at the timeout results with these simple tests:

import urllib3
# Test connection timeout: MaxRetryError caused by NewConnectionError
urllib3.request("GET", "https://github.com", timeout=urllib3.Timeout(connect=0.0001))
# Test read timeout: MaxRetryError caused by ReadTimeoutError
urllib3.request("GET", "https://github.com", timeout=urllib3.Timeout(read=0.0001))

(note that this simple test reuses the default connection pool so a successful connection will prevent retesting the connection timeout without reloading the module)