Skip to content
This repository

Fix SSL3 error on Ubuntu 12.04 #799

Merged
merged 1 commit into from over 1 year ago

6 participants

Joseph McCullough Don't Add Me To Your Organization a.k.a The Travis Bot Kenneth Reitz Andrey Petrov Dustin Oprea Cory Benfield
Joseph McCullough

When attempting to request (get or post) to a resource responding only to SSL version 3, we'd get the following response on Ubuntu 12.04:


>>> import requests
>>> requests.post("https://sis-portal-prod.uttyler.edu/psp/TAPPRD/EMPLOYEE/EMPL/?&cmd=login")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests/api.py", line 98, in post
    return request('post', url, data=data, **kwargs)
  File "requests/safe_mode.py", line 39, in wrapped
    return function(method, url, **kwargs)
  File "requests/api.py", line 51, in request
    return session.request(method=method, url=url, **kwargs)
  File "requests/sessions.py", line 243, in request
    r.send(prefetch=prefetch)
  File "requests/models.py", line 630, in send
    raise SSLError(e)
requests.exceptions.SSLError: [Errno 1] _ssl.c:504: error:1407742E:SSL routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version

There's a good number of people having the error.

Fix taken from this thread. Specs pass. Even if you don't accept this into master, hopefully someone will see this pull request on Google.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request fails (merged 413d4b3d into b130b98).

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request fails (merged 413d4b3d into b130b98).

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 6ac66b3d into b130b98).

Joseph McCullough

Rebasing for a neater pull.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged c19650b into b130b98).

Kenneth Reitz
Owner

@joequery You should be making this pull request against urllib3, not requests :)

Kenneth Reitz kennethreitz merged commit 2e71497 into from August 19, 2012
Kenneth Reitz kennethreitz closed this August 19, 2012
Andrey Petrov
Collaborator

@kennethreitz :O You merged urllib3 pull request code into requests. Naughty.

Kenneth Reitz

god damn it, i hit the wrong button

Andrey Petrov
Collaborator

Consequences will never be the same. :P

Kenneth Reitz

Reverted

Joseph McCullough

Is there a known fix for this issue elsewhere? It looks like #606 is along the lines of what I'm looking for. However, requests was functioning fine on my mac, but not on Ubuntu 12.04, even after upgrading OpenSSL.

Ken Schwencke Schwanksta referenced this pull request in shazow/urllib3 December 12, 2012
Closed

Fix SSL3 error on Ubuntu 12.04 #127

Andrey Petrov
Collaborator

@Schwanksta The respective issue on urllib3 has been closed. :)

Dustin Oprea

I'm a little confused as to the status of what happened, here. My perception is that one change was made to urllib (allowing a SSL version to be passed) and that this PR was reverted, leaving only the @joequery fork of requests, where the only real change was to attempt SSL3 and then SSL23.

There still exists a problem where any error will be caught and another connection reattempted using SSL23, sometimes or always causing the "unknown protocol" error. This can simply be fixed by reraising the exception unless the message matches the TLS-version error:

    try:
      self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file,
                                  cert_reqs=self.cert_reqs,
                                  ca_certs=self.ca_certs,
                                  ssl_version=ssl.PROTOCOL_SSLv3)
    except ssl.SSLError as e:
      if str(e).endswith(':tlsv1 alert protocol version') is False:
        raise

      self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file,
                                  cert_reqs=self.cert_reqs,
                                  ca_certs=self.ca_certs,
                                  ssl_version=ssl.PROTOCOL_SSLv23)

I'd submit an issue, but apparently forks can't have issues.

@joequery I think your fork might still be the only solution. Please confirm. If so, please consider the fix, above.

Cory Benfield
Collaborator

@dsoprea Please think very carefully before commenting on two-year-old issues. There was definitely a better, less-noisy way to handle providing your input.

Your reading of this issue is wrong. This pull request contained only changes to urllib3, which is a separate module to requests, maintained elsewhere. We do not accept code changes in urllib3 in this repository, as we simply bring in urllib3 wholesale. Any urllib3 changes therefore need to be made at the core repository, shazow/urllib3. This means even if we wanted to, we could not consider this fix, because it's not our fix to make.

Kenneth accidentally merged this fix when he intended to close it, so he also reverted it.

If you'd carefully checked the internet, you'd know that there is an accepted way to choose the SSL version used by Requests. It is documented in this blog post (now more than a year old), this StackOverflow question, this Requests issue, and will be implemented in this open-source library.

Dustin Oprea
Cory Benfield
Collaborator

I did not intend for that message to seem like I was chastising you, or to seem abrasive: I'm sorry.

It's worth noting that, as I mentioned, this comes up very frequently. This means that a lot of developer time is spent responding to duplicate issues and comments on previous issues. Any GitHub comment is responded to as if it is urgent, because they often are. This means a comment or GitHub issue is an instant cost of our time (as I'm sure you can appreciate from your own projects).

This is what I meant when I said there were "lower-noise" ways of raising this issue. For example, Stack Overflow would have been a suitable place to ask. Alternatively, emailing one of the triagers would have been just as appropriate: both @sigmavirus24 and I have our email addresses published very clearly.

My response was born in part from frustration at repeatedly having to follow up on issues opened or commented on that could have been resolved by more care. I'm sure you just made a mistake, and as I said, I'm sorry for not taking care with my language.

Dustin Oprea
Cory Benfield
Collaborator

I'm glad we could both step back and talk this out. =) Gives me a little more faith in humanity. I'm sorry you've been encountering SSL issues. Let me know if the linked solution helps you out: if it doesn't, I'll happily spend some time looking at your problem further (just, you know, email me first!).

Dustin Oprea
Joseph McCullough

@dsoprea My fork existed back when documentation concerning choosing SSL versions wasn't as easy to find. I still use my fork from time to time when I deal with scraping sites which exclusively deal with sslv3, because I'm lazy and don't feel like doing that mounting process :P

I'll make the update you provided anyway, so thanks for that. I also didn't think your initial comment was rude or inconsiderate at all, and I don't see how commenting on an issue similar to your own is "noisy".

Dustin Oprea
Cory Benfield
Collaborator

@joequery As @dsoprea just said, we both managed to get our wires crossed and weren't entirely our best selves. These things happen, apologies all round and everyone's friends again. =)

@dsoprea You mention that you're not sure why the SSL version isn't available on the default HTTPAdapter. This is a fair question. The short answer is that we don't want to provide options on the HTTPAdapter for every single argument urllib3 takes on any of the functions we call. That will lead to a nightmare god-object, and worse, will lead to us duplicating the default values that are already in urllib3. That's a potential source of many many bugs.

With that in mind, we had to pick which attributes we thought were important enough to specify explicitly and which we thought weren't. This was an arbitrary decision, and Kenneth made it as he saw fit. In principle the ssl_version should never need to be set: it looks like the bug we're encountering here is related tangentially to buggy versions of OpenSSL (see a very recent discussion at #1847 and #1850, where the tone of the conversation provides an idea of how frustrating the maintainer + contributors find this SSL bugginess). This makes ssl_version a prime candidate to simply leave at its default urllib3 value.

This dovetails with the fact that the Transport Adapter interface is intended to be an inheritance-based interface, not a function-parameters one. Again, I've written briefly about this (though if you read the SSLAdapter post you'll find this retreads a lot of ground).

If you strongly disagree with this design decision, I can get Kenneth to weigh in on it, but we're strongly resisting changes to Requests' interfaces.

As for proposing an SSL Transport Adapter example in the documentation, I think we'd be happy to have it. =) I've documented it myself in a number of places that aren't the official documentation, and it's now present in what is likely to be a very popular companion library, but it also makes a good example.

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

Showing 1 unique commit by 1 author.

Aug 19, 2012
Joseph McCullough Fix SSL3 error on Ubuntu 12.04 c19650b
This page is out of date. Refresh to see the latest.
14  requests/packages/urllib3/connectionpool.py
@@ -96,9 +96,17 @@ def connect(self):
96 96
 
97 97
         # Wrap socket using verification with the root certs in
98 98
         # trusted_root_certs
99  
-        self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file,
100  
-                                    cert_reqs=self.cert_reqs,
101  
-                                    ca_certs=self.ca_certs)
  99
+        try:
  100
+          self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file,
  101
+                                      cert_reqs=self.cert_reqs,
  102
+                                      ca_certs=self.ca_certs,
  103
+                                      ssl_version=ssl.PROTOCOL_SSLv3)
  104
+        except ssl.SSLError:
  105
+          self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file,
  106
+                                      cert_reqs=self.cert_reqs,
  107
+                                      ca_certs=self.ca_certs,
  108
+                                      ssl_version=ssl.PROTOCOL_SSLv23)
  109
+
102 110
         if self.ca_certs:
103 111
             match_hostname(self.sock.getpeercert(), self.host)
104 112
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.