Port of PR #4173 from v3 to v2 to fix issue #4391

[atzannes]

Fixes #4391

Problem

This code was failing to verify the SSL:

    def test_session_verify_not_stateful(self):
        insecure_url = "https://expired.badssl.com/"
        s = requests.Session()
        assert s.verify is True

        with pytest.raises(Exception):
            s.get(insecure_url, verify=True)

        s.get(insecure_url, verify=False)
        assert s.verify is True

        with pytest.raises(Exception):
            s.get(insecure_url, verify=True)  # <- This line was not raising bc we got a cached unverfied connection

The problem was that we were modifying the connection object returned to us by the poolmanager, which is illegal, causing us to get unverified connections when requesting verified ones.

Approach

I had come up with the same solution as PR #4173 independently, not having found the issue before trying to fix it, but my unit-tests were not as thurough, so I added those from PR #4173

The issue was closed in 2018 because it was fixed in v3 (which is still not released), and presumably because it was not fixable in v2, but perhaps this fix works for v2 (new urllib3 perhaps that allows it?)

[jeremycline]

The issue was that fixing it involved breaking an API in v2. I suppose a feature flag could be added, something like really_actually_do_verify which defaults to False but is documented as something you probably want, and the old behavior and API are available with that flag set to False. That'd make it easier to work around.

[atzannes]

Hi @jeremycline thanks for the feedback. Which API is broken in v2 by this change? Are you refering to get_gonnection in adapters.py which now takes two additional optional arguments, or the removal of cert_verify which was illegally modifying the connection object?

[jeremycline]

The removal of cert_verify which is a public method. If you kept that and switched behavior based on a feature flag it'd probably be more palatable.

[atzannes]

ok, great! I wasn't sure whether cert_verify was treated as public given the comment saying "This method should not be called from user code."

Having played around with it a bit, I have a suspicion that keeping that function in, even without a feature flag, will do the right thing in combination with building and passing the conn_kwargs to the poolmanager. I'll test that hypothesis and get a update up soon. Again, thanks for the quick feedback @jeremycline !

[jeremycline]

Oh, also, just to be clear I'm not a maintainer here, my feedback has no influence on the acceptance of such a pull request, is provided "as is", etc. :smile:

[atzannes]

@jeremycline I know you contributed the PR where I got most of the unit-tests from for this PR. And I was happy to see that the solution I had come up with was effectively identical with the one you had. By reading your PR, I inferred that you're not a maintainer here. But your feedback is very welcome and valuable nonetheless.

Ok, so I force-pushed and now there are two commits:

• the first where I've only added the _build_conn_kwargs function and where cert_verify (the member function and its call in send) are untouched. Tests pass even despite its shenanigans.
• the second where cert_verify has been deprecated (its body is replaced by pass), but we're still calling it from send, so that any code that has derived from HTTPAdapter and has not overriden send but has overriden cert_verify would continue working presumably.

I prefer having cert_verify deprecated because it was doing something illegal.

Thoughts? Concerns?

[atzannes]

@nateprewitt @Lukasa gentle ping. This is an API-compatible backport of @jeremycline's great fix for issue #4391.

• Added _build_conn_kwargs which mirrors what cert_verify does, but instead of modifying the conn argument in-place, it builds kwargs to be passed to the PoolManager so that it returns the right connection. The subsequent call to cert_verify should be a no-op if PoolManager honored the kwargs we passed.
• API change: get_connection in HTTPAdaper takes two new optional arguments: verify, and cert because it needs to call _build_conn_kwargs.

[atzannes]

Is there anybody out there?