SSL certificate CN and fingerprint verification support #1945

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

@ttanner
ttanner commented Mar 10, 2014

This PR adds support for SSL CommonName and fingerprint verification provided by urllib3.

In contrast to #1606 it is minimally invasive and does not require new API parameters. Instead, it allows "verify" to be a dictionary with extra parameters.

@Lukasa
Collaborator
Lukasa commented Mar 10, 2014

Thanks for this!

I have no strong opinions, it looks fine. =) +0.5. @sigmavirus24?

@sigmavirus24
Collaborator

I have a couple concerns. The first is that you're introducing yet another way for the user to set the CA bundle. You did not state that in your description. I assume you want this in order to specify the path that you would otherwise specify to verify as a plain string.

That noted, I'm not comfortable with verify having a trinary value (its current state) let alone having 4 possible values (True, False, '/path/to/bundle', and dictionary of connection options). Let's be clear, you are attaching these to the connection. We've held in the past that these are Transport Adapter concerns. Whilst being less invasive and a less severe API change, I'm still -0. I still feel that if you have all of the information for each URL you could structure it in such a way that a custom transport adapter could use it and attach it to the connection. The current API affords for what you want, just not in the simplest way possible. Especially given Kenneth's hesitancy to tie requests too closely to urllib3's API, I'm hesitant to be positive about this.

If @kennethreitz likes the change though my only request would be that you use #get on the verify dictionary like so:

cert_loc = verify.get('ca_bundle')
conn.assert_hostname = verify.get('hostname')
conn.assert_fingerprint = verify.get('fingerprint')

This is far better than first checking for the value first and then assigning it. Each of those assigned values will be None if in fact the parameter is not provided or the dictionary is simply empty.

@kennethreitz
Owner

-1, last time we did a mutating API like this (file upload api) it was a disaster.

This is why we have connection adapters. This configuration should be done there.

@sigmavirus24
Collaborator

Given @kennethreitz's -1 I'm going to close this.

@kennethreitz
Owner

@sigmavirus24 I can close pull requests.

@kennethreitz kennethreitz reopened this Mar 12, 2014
@kennethreitz
Owner

@ttanner thank you so much for this contribution! Unfortunately, we won't be accepting it at this time. Basically, we've went down the route of passing tuples and dicts to a few different APIs, and it got a bit hairy if it wasn't the primary interface.

It may be worth exploring other options (perhaps passing verify a class?), if you think these SSL features are important, though. I'd love to learn more about them.

@schlamar
Contributor

@kennethreitz Here is a use case regarding assert_hostname. I want to run a HTTPS web server with a self signed certificate and run some tests with requests against it. As this web server can run on multiple nodes it has no static host/IP.

If I set the requests CA_BUNDLE to my certificate it fails on ssl_match_hostname, because there is no hostname defined in the cert (requests.exceptions.SSLError: no appropriate commonName or subjectAltName fields were found ).

Right now, the only way of making this configuration to work is a) completely disable certificate verification or b) patch urllib3/requests to pass assert_hostname=False. Both options are not really nice.

Can you elaborate on your proposal to use a class? I guess I can help with a PR if you are more specific about what you expect.

@schlamar
Contributor

Or what about a function which has direct access to the connection object, so something like

if callable(verify):
    verifiy(conn)

Or a general API which allows you access to the connection object, e.g. a new keyword argument.

@sigmavirus24
Collaborator

Or a general API which allows you access to the connection object, e.g. a new keyword argument.

The answer to that is simple: No. I'm going to investigate the rest of this soon to see how I can help.

@sigmavirus24
Collaborator

@schlamar in this case, if I were you, I would do the following:

  1. Use @t-8ch's StackOverflow answer to create a new Adapter
  2. Register the adapter on your session like so:
mydomain = 'https://example.com'
s = requests.session()
s.mount(mydomain, HostNameIgnoringAdapter())
@piotr-dobrogost
Contributor

I took a look at code in SO question and it uses urllib3 which I thought was meant as an implementation detail of requests. If some parts of urllib3 are meant to be used by users they should be exposed explicitly by requests.

@sigmavirus24
Collaborator

@piotr-dobrogost as of this moment 98% (or more) of users can consider urllib3 an implementation detail. The rest of those users will need something from urllib3 and there are examples in the documentation (for the most common cases) which explicitly use urllib3. Regardless your comment is off-topic.

@schlamar
Contributor

if I were you, I would do the following

Thanks, that works perfectly. I would propose adding this to the docs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment