Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use requests rather than urllib3 #103

Merged
merged 12 commits into from
Jun 21, 2021
Merged

Use requests rather than urllib3 #103

merged 12 commits into from
Jun 21, 2021

Conversation

seizethedave
Copy link
Contributor

@seizethedave seizethedave commented Jun 17, 2021

I have been doing some research around the cert verification topic, and I came to the following points:

  1. mixpanel-python should strive to work without certificate hassles as much as possible. It is used in a wide variety of environments/servers/personal computers/diff't Python versions/etc.
  2. I asked in the urllib3 discord channel if:
    1. if I should begin to depend on urllib3[secure] rather than plain old urllib3. The answer was "no," as that extra option is being deprecated, and will do kinda "the wrong thing" w/r/t pyOpenSSL.
    2. if I should apply this fix from requests to my project: Don't use pyOpenSSL unless no SNI is detected psf/requests#5443. The answer was "yes," that it would probably help.
    3. if I should explicitly depend on certifi. (Included in urllib3[secure], but we aren't using that.) The answer was "yes." It will remove a lot of variability in root certs in the wild. (And indeed make having a local cert store optional.)

The current recommendation is that on modern Python versions (2.7.9+/3.4+) you're better off using the builtin Python ssl module because there's native SNI support. So that means not installing the [secure] extra and not injecting pyOpenSSL on those versions.
It's unfortunate that the old way of doing things was the default for a lot longer than it had to be and that the extra is named "secure", we're paying the distributed tech debt price by removing it as the default and trying to document this better.
Let me know if you have more questions :) I think the approach in the PR you mentioned is a good one but might be even better to force users to at least install Python 2.7.9+. Versions before that are really really old and insecure.

So I began to add items ii and iii to mixpanel-python. But then realized requests does both of those OOTB, and does a bunch of other sniffing around certifi/default certs/unzipping cert bundles/overriding certifi/etc. So to support point 1, I am just going to use requests.

mixpanel/__init__.py Show resolved Hide resolved
test_mixpanel.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants