Python ssl fix here

[wyhuang]

The new version of Python broke https connections in SickBeard. A simple two line correction will fix this by removing the ssl verification.

[imajes]

This is a really bad idea. Switching to unverified SSL is not a fix, but instead it's just creating risk.

[spidercensus]

Yup. Not a solution. What specifically broke? Is it some provider with a
bad cert?

On Wed, Jul 22, 2015 at 10:25 PM, James Cox [email protected]
wrote:

This is a really bad idea. Switching to unverified SSL is not a fix, but
instead it's just creating risk.

—
Reply to this email directly or view it on GitHub
#954 (comment).

[wyhuang]

You will find most newsgroups api request use https, they almost all use bad certs. If you head over to SickRage's SickBeard.py file you will find they have used a similar fix. Yes it's a risk, but at the moment a key feature is unusable.

[wyhuang]

Also does sickbeard require security? Isn't it better to not use / disable remote https login altogether? News Group website api requests do not work at the moment and from the user perspective it is more important then remote https login due to the nature of how SickBeard works, you setup a show once and don't worry about it anymore, so a remote login is almost redundant 99.99% of the time.

[imajes]

The issue isn't that it's just a small request, but that forever more, ALL SSL requests from sickbeard will never be verified. This sort of code sits there and creates risk forever. It's a silent killer.

[imajes]

Also, can you demonstrate a bad API SSL cert?

[arucard21]

Not sure where you're still experiencing problems with SSL certificate verification, but this has been fixed in #912 months ago (the discussion about whether unverified SSL is acceptable can also be found there). This was fixed by providing a helper method that allows you to retrieve the contents of a URL. You can specify whether you want the SSL certificate to be verified or not and provides additional benefits by standardizing URL access across sickbeard (See https://github.com/midgetspy/Sick-Beard/blob/development/sickbeard/helpers.py#L186).

As comment on this PR, I'd have to agree with @imajes that disabling SSL certificate verification entirely is not a good idea. If the SSL certificate is (or should be) valid, then you want to verify it, e.g. for public websites like PushBullet or EZRSS. But you also want to be able to skip verification if you know that it will fail, e.g. for self-signed sites like when you run your own SABnzbd, Newznab, etc.

While this PR is the simplest code modifications, it has far too great an impact on the functionality (which is already explained by @imajes ). If you'll look at https://www.python.org/dev/peps/pep-0476/#opting-out you'll see that monkeypatching the ssl module is highly discouraged.

I hope this bit of feedback was helpful to you for any other PR's you might wish to submit.

[derooy]

Let me know if this is not the appropriate channel.

I was unable to download torrents at some https endpoints (silently), after recent sickbeard changes.
upgrading my python from 2.7.5 or 2.7.6 to 2.7.10 via the deadsnakes-python2.7 ppa fixed the issue.

Hopefully that helps you @wyhuang

[arucard21]

@derooy The change in python that made SSL certificate validation the default behavior was introduced in python 2.7. If you were running on python 2.7.5 or 2.7.6 without problems before you suddenly had problems with an https endpoint, then this probably doesn't apply to you. The problem would have had to occur as soon as you updated to 2.7.x. What also indicates to me that your problem is not related to this certificate verification, is that it would not have been fixed by upgrading to 2.7.10 since that version still validates certificates by default (and the deadsnakes repo doesn't seem to change that, at first glance).

[derooy]

@arucard21 I agree with everything you say, certainly my situation doesn't
make sense and a laymen's (my) observations should not carry much weight.
However I thought it best to document it just in case. Thanks for the help
(:
On 27 Jul 2015 4:38 am, "arucard21" [email protected] wrote:

@derooy https://github.com/derooy The change in python that made SSL
certificate validation the default behavior was introduced in python 2.7.
If you were running on python 2.7.5 or 2.7.6 without problems before you
suddenly had problems with an https endpoint, then this probably doesn't
apply to you. The problem would have had to occur as soon as you updated to
2.7.x. What also indicates to me that your problem is not related to this
certificate verification, is that it would not have been fixed by upgrading
to 2.7.10 since that version still validates certificates by default (and
the deadsnakes repo doesn't seem to change that, at first glance).

—
Reply to this email directly or view it on GitHub
#954 (comment).

[thezoggy]

brief overview of the issue and fix at hand:

python 2.7.9 (https://www.python.org/downloads/release/python-279/) changed things because of the whole poodle attack. They (python community) backported python 3.x ssl and went security focused, as they disabled SSLv3 and enabled https cert checking by default (vs before the programmer had to specifically enable it). Thus this 'fix' to disable the verification globally does nothing but restore the pre python 2.7.9 action (as sb never really checked certs).. now with that said this may not be the best thing as we all know how many exploits SSL has had in the recent year.. but sickbeard honestly has never been so security focused... they allow sharing what you download to social media (twitter), it only uses basic auth, stores passwords in clear text, logs show apikey+pw+etc. Now for the whole point of using https for your indexers.. its not like your sharing personal info and if your apikey gets leaked.. just change it and move along? most people would rather it just work than be secure.. The only real way to 'secure' sb is to do a reverse proxy to try and keep some of it in house, dont expose sb externally to the world.. use vpn / run your downloader on the same machine.. etc.

The true problem with sb is that people have tried to submit code to deal with the mismatch of 'defaults' of pre 2.7.9 vs post 2.7.9.. which have in turn broken the notifiers/indexers/etc.. then others came in and tried to fix it by rewriting code.. thus fixing it for some but breaking it further for others. The lack of unit testing in sb and the fact no one is doing code review, means its up to the person coding the fix to go test each and every part of sb.. which obviously isnt done and hard to expect.

[phit42]

Thx for the summary thezoggy, but where does that leave us for the master branch?
I actually liked arucard21 approach to put all calls into a geturl helper function, but it appears the switch to urllib2 together with the changes to its calling parameters (data object vs request) raises some issues with some notifiers. I am thinking it might need a 100%-backwards compatible helper function that works with all notifiers like before, roll it out, then later notifier classes can be switched one by one.

[phit42]

BTW I had the issue that urlopen with the create_unverified_context did not return at all and did not even throw an exception when I was using https for sabnzbd (even in the first getSabAccesMethod call with the url https://localhost:9095/api?mode=auth). I needed to use the highly discouraged code under https://www.python.org/dev/peps/pep-0476/#opting-out in my sab.py module for the moment.