Only use HTTPS #178

simmel · 2016-10-05T12:44:17Z

Closes #151

hugovk · 2016-10-05T13:51:23Z

There's a problem with Python 3.3: https://travis-ci.org/pylast/pylast/jobs/165224071#L216
and pypy: https://travis-ci.org/pylast/pylast/jobs/165224073#L216
and pypy3: https://travis-ci.org/pylast/pylast/jobs/165224074#L218

simmel · 2016-10-05T19:44:08Z

On Wed, 2016-10-05 at 06:51:23 -0700, Hugo wrote:

There's a problem with Python 3.3: https://travis-ci.org/pylast/pylast/jobs/165224071#L216
and pypy: https://travis-ci.org/pylast/pylast/jobs/165224073#L216
and pypy3: https://travis-ci.org/pylast/pylast/jobs/165224074#L218

Looks like create_default_context() doesn't exist in Python <3.4 and we
have to do:

Specify CA path
Check that the domain we want to connect is listed in the certificate
that the server we connect to is present.

3.3 is still supported another year https://en.wikipedia.org/wiki/CPython#Version_history .
Looks like Ubuntu precise http://packages.ubuntu.com/search?keywords=python3 (which expires 2017-04) and CentOS via SCL https://www.softwarecollections.org/en/scls/rhscl/python33/ still uses 3.3. Although 3.4 can be installed via SCL https://www.softwarecollections.org/en/scls/rhscl/rh-python34/ .

What should we do about the 3.3 (and pypy3, sadly) users? Logging a warn
and be done with it? This feels like a bad idea but I'm not that excited
of writing our own wrapper for it, while searching I found pymongo's SSL
wrapper which is almost 400 LoC and importing one of two different CA
stores.

I have some code on my machine which I'm sure can work. I'm getting it
ready to commit within the day or so.

A side note: It looks like the supported URL for libre.fm now is
libre.fm/2.0 and not alpha.libre.fm? alpha.libre.fm will never work with
SSL (with CN/host validation on).

hugovk · 2016-10-07T14:55:29Z

Let's use HTTPS where available, and HTTP otherwise. That's an improvement over the current situation.

There's still some usage of Python 3.3, but it's not huge in comparison:
http://www.randalolson.com/2016/09/03/python-2-7-still-reigns-supreme-in-pip-installs/
http://www.randalolson.com/2015/01/30/python-usage-survey-2014/

I'll update the Libre.fm URL.

simmel · 2016-10-13T19:51:33Z

On Fri, 2016-10-07 at 07:55:29 -0700, Hugo wrote:

Let's use HTTPS where available, and HTTP otherwise. That's an improvement over the current situation.

Where we can use it easily and securely by default?

Or where we can use it period? Because we can use it everywhere, it just
takes more code and dependencies.

I'll update the Libre.fm URL.

Nice!

simmel · 2016-10-19T10:42:56Z

Thoughts Hugo?

I have both ideas coded I just need to know which one to push :)

On Thu, 13 Oct 2016, at 21:51, Simon Lundström wrote:

On Fri, 2016-10-07 at 07:55:29 -0700, Hugo wrote:

Let's use HTTPS where available, and HTTP otherwise. That's an
improvement over the current situation.

Where we can use it easily and securely by default?

Or where we can use it period? Because we can use it
everywhere, it just
takes more code and dependencies.

I'll update the Libre.fm URL.

Nice!

hugovk · 2016-10-19T11:48:29Z

@simmel Everywhere sounds good!

simmel · 2016-10-23T12:57:51Z

@hugovk sadly I gave you false hope, using SSL securely everywhere isn't possible.

3.4 and >2.7.9 has sane defaults and just works(TM).

On 3.3 we have to use certifi and configure it explicitly to be secure. This sadly make all versions depend on certifi since I haven't found a way in setuptools or requirements files to only depend on something when running under a specific Python version.

I think I'm done with this, can you review it?

hugovk

About certifi, do any of these work? http://stackoverflow.com/q/21082091/724176

Alternatively, if you want to leave certifi out of setup.py, import certifi # pip install certifi is a good workaround for the install thing: if there's an ImportError on 3.3, it'll show this line with comment. That way non-3.3 don't need to install it at all.

hugovk · 2016-10-24T10:28:25Z

pylast/__init__.py

+# <=3.3 doesn't support create_default_context()
+# <2.7.9 and <3.2 never did any SSL verification
+# FIXME This can be removed after 2017-09 when 3.3 is no longer supported and
+# pypy3 uses 3.4 or later, see https://en.wikipedia.org/wiki/CPython#Version_history


pep8 thing: "E501 line too long (84 > 79 characters)" https://travis-ci.org/pylast/pylast/jobs/169897742#L196

hugovk · 2016-10-24T10:29:34Z

pylast/__init__.py

@@ -42,16 +42,33 @@
 def _deprecation_warning(message):
    warnings.warn(message, DeprecationWarning)

+
+def can_use_ssl_securely():


Rename as _can_use_ssl_securely if intended to be private?

hugovk · 2016-10-24T10:36:03Z

pylast/__init__.py

@@ -42,16 +42,33 @@
 def _deprecation_warning(message):
    warnings.warn(message, DeprecationWarning)

+
+def can_use_ssl_securely():
+    # 3.3 doesn't support create_default_context() but can be made to work


Let's explicitly make it's clear it's "Python 3.3".

hugovk · 2016-10-24T10:36:37Z

pylast/__init__.py

@@ -131,6 +148,32 @@ def _deprecation_warning(message):

 XML_ILLEGAL = re.compile(RE_XML_ILLEGAL)

+# <=3.3 doesn't support create_default_context()


Let's explicitly make it's clear it's "Python <=3.3"

simmel · 2016-10-24T20:00:16Z

Sweet! That SO link was exactly what I didn't find before! I settled for extras_require because it makes the code easier to remove and makes it stand out.

I hope I've fixed everything. This time, I ran pep8 two times before I commited ; )

hugovk · 2016-10-25T06:09:17Z

pylast/__init__.py

+
+# Python >3.4 and >2.7.9 has sane defaults
+elif sys.version_info > (3, 4) or (
+        sys.version_info < (3, 0) and sys.version_info > (2, 7, 9)):


Can this use _can_use_ssl_securely()?

The logic for _can_use_ssl_securely() is slightly different:

v > (3, 3) or (v < (3, 0) and v > (2, 7, 9))

Compared to here:

v > (3, 4) or (v < (3, 0) and v > (2, 7, 9))

Are they intentionally different? They have similar comments:

>3.4 and >2.7.9 has sane defaults so use SSL there

And:

Python >3.4 and >2.7.9 has sane defaults

If we cannot reuse _can_use_ssl_securely(), let's use interval comparison:

v > (3, 4) or ((2, 7, 9) < v < (3, 0))

No we can't since _can_use_ssl_securely() also includes 3.3.

Switched to clever code.

hugovk · 2016-10-25T06:24:34Z

pylast/__init__.py

+# FIXME This can be removed after 2017-09 when 3.3 is no longer supported and
+# pypy3 uses 3.4 or later, see
+# https://en.wikipedia.org/wiki/CPython#Version_history
+if sys.version_info < (3, 3, float("inf")) and sys.version_info > (3, 2):


So this matches 3.2.x. Let's use:

if sys.version_info[0] == 3 and sys.version_info[1] == 2:

3.3 actually, but yes let's.

hugovk · 2016-10-25T06:26:53Z

pylast/__init__.py

+    # <2.7.9 and <3.2 never did any SSL verification so don't do SSL there.
+    # >3.4 and >2.7.9 has sane defaults so use SSL there.
+    v = sys.version_info
+    return v > (3, 3) or (v < (3, 0) and v > (2, 7, 9))


We can use interval comparison:

return v > (3, 3) or ((2, 7, 9) < v < (3, 0))

Looks like clever code but I'll allow it. Interval comparison looks neat (but clever)!

simmel · 2016-10-25T18:12:10Z

On Wed, 2016-10-19 at 04:48:29 -0700, Hugo wrote:

@simmel Everywhere sounds good!

Ugh, sorry for spreading false hope.

<2.7.9 and <3.2 we can't do good how much we try. Should they use SSL
and hope for the best or just use HTTP there? Hope for the best is what
the code does now.

3.3 we can make sane via the certifi dependency and some
configuration, see the updated code. I'm looking into how to make us
ONLY depend on certifi when on Python 3.3.

3.4 and >2.7.9 all is good.

hugovk

Thanks for the updates!

I'm not expecting all the CI tests to pass due to #171, and so far 2.7 looks fine (and 3.4, pypy and pypy3 are still pending), but 3.3 complains:

    NetworkError: [SSL: NO_CIPHERS_AVAILABLE] no ciphers available (_ssl.c:548)
    [<TracebackEntry /home/travis/build/pylast/pylast/tests/test_pylast.py:37>, <TracebackEntry /home/travis/build/pylast/pylast/tests/test_pylast.py:120>, <TracebackEntry /home/travis/build/pylast/pylast/pylast/__init__.py:2640>, <TracebackEntry /home/travis/build/pylast/pylast/pylast/__init__.py:1370>, <TracebackEntry /home/travis/build/pylast/pylast/pylast/__init__.py:1196>, <TracebackEntry /home/travis/build/pylast/pylast/pylast/__init__.py:1178>]

https://travis-ci.org/pylast/pylast/jobs/170886479

What's up there?

simmel · 2016-10-27T06:11:06Z

I'm clueless. TravisCI uses precise, an older Ubuntu version so I thought it might be a problem with too old OpenSSL but that doesn't seem to be the case: https://travis-ci.org/pylast/pylast/jobs/170998686#L198

Might be that pypy3 doesn't seem to have OpenSSL statically build in either but I'll try to list the ciphers in pypy3 some how.

simmel · 2016-10-28T18:28:00Z

Ah yes, ancient OpenSSL https://travis-ci.org/pylast/pylast/builds/171464271#L210

simmel · 2016-10-28T18:31:08Z

Should work now.

simmel · 2016-10-29T07:53:32Z

@hugovk they passed. On pypy3 the tests timed out though, any idea why? Looks like there were ~20ish tests left.

Looks like it always takes that long in pull requests https://travis-ci.org/pylast/pylast/builds/167208560
Your PR worked though https://travis-ci.org/pylast/pylast/jobs/165842936 I'm guessing it's because you have access to the secrets?

hugovk · 2016-10-29T12:39:20Z

Not sure what caused that pypy3 timeout.

Yes, my PR's have access to the secrets. One way is for you to add your encrypted credentials like another contributor has (see f1e14f5), or if we do something from #171.

Now Python 3.3 complains:

NetworkError: [SSL: TLSV1_ALERT_DECODE_ERROR] tlsv1 alert decode error

https://travis-ci.org/pylast/pylast/jobs/171465889#L368

simmel · 2016-10-29T13:21:28Z

On Sat, 29 Oct 2016, at 14:39, Hugo wrote:

Not sure what caused that pypy3 timeout.
Yes, my PR's have access to the secrets. One way is for you to add
your encrypted credentials like another contributor has (see f1e14f5),
or if we do something from #171[1].
Just make those variables public

Sounds reasonable. In my "now playing"-script for irssi I've just made
them public since it's impossible not to. For me to require every user
to get an API-key is unreasonable.

Now Python 3.3 complains:
NetworkError: [SSL: TLSV1_ALERT_DECODE_ERROR] tlsv1 alert decode error
What the...? I'll look at it this evening. I didn't find that in the
pypy3 logs when I looked o_O

Links:

Tests fail for pull requests #171

hugovk · 2016-10-29T13:34:24Z

Thanks. By the way, that error is from Python 3.3, not PyPy3.

https://docs.python.org/2/library/ssl.html#best-defaults Deal with older Pythons which didn't do certificate validation, have sane defaults or even provided a cipher string.

simmel · 2016-10-29T22:30:57Z

@hugovk looks like this is why Mozillas SSL guide uses the expanded and not the compact version of the cipher strings.

hugovk · 2016-10-30T08:45:32Z

@simmel Thanks for all the work, merged!

hugovk · 2017-01-03T16:27:17Z

Released in pylast 1.7.0.

Update python_requires to specify the exact x.y.z 2.7.10 version required. (From HTTPS changes in #178.)

Alejandro Angulo added 2 commits October 15, 2016 12:44

don't want user creds in the repo

373109c

switch from HTTPConnection to HTTPSConnection

44c592d

hugovk reviewed Oct 24, 2016

View reviewed changes

hugovk reviewed Oct 25, 2016

View reviewed changes

hugovk approved these changes Oct 26, 2016

View reviewed changes

hugovk requested changes Oct 26, 2016

View reviewed changes

Use default SSL context when possible

99d5674

https://docs.python.org/2/library/ssl.html#best-defaults Deal with older Pythons which didn't do certificate validation, have sane defaults or even provided a cipher string.

hugovk approved these changes Oct 30, 2016

View reviewed changes

hugovk merged commit cdd441d into pylast:develop Oct 30, 2016

simmel deleted the https branch October 30, 2016 10:03

hugovk mentioned this pull request Sep 17, 2017

Drop support for Python 3.3 #208

Merged

hugovk mentioned this pull request Oct 30, 2017

SSL on Google Cloud instance #201

Closed

hugovk mentioned this pull request Dec 9, 2017

Update python_requires to 2.7.10+ #239

Closed

hugovk added a commit that referenced this pull request Dec 9, 2017

Update python_requires to 2.7.10+

9abb588

Update python_requires to specify the exact x.y.z 2.7.10 version required. (From HTTPS changes in #178.)

hugovk mentioned this pull request Dec 9, 2017

Update python_requires to 2.7.10+ #240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only use HTTPS #178

Only use HTTPS #178

simmel commented Oct 5, 2016

hugovk commented Oct 5, 2016

simmel commented Oct 5, 2016

hugovk commented Oct 7, 2016 •

edited

simmel commented Oct 13, 2016

simmel commented Oct 19, 2016

hugovk commented Oct 19, 2016

simmel commented Oct 23, 2016

hugovk left a comment

hugovk Oct 24, 2016

hugovk Oct 24, 2016

hugovk Oct 24, 2016

hugovk Oct 24, 2016

simmel commented Oct 24, 2016

hugovk Oct 25, 2016

simmel Oct 26, 2016

hugovk Oct 25, 2016

simmel Oct 26, 2016

hugovk Oct 25, 2016

simmel Oct 26, 2016

simmel commented Oct 25, 2016

hugovk left a comment

simmel commented Oct 27, 2016

simmel commented Oct 28, 2016

simmel commented Oct 28, 2016

simmel commented Oct 29, 2016

hugovk commented Oct 29, 2016

simmel commented Oct 29, 2016

hugovk commented Oct 29, 2016

simmel commented Oct 29, 2016

hugovk commented Oct 30, 2016

hugovk commented Jan 3, 2017

		@@ -131,6 +148,32 @@ def _deprecation_warning(message):

		XML_ILLEGAL = re.compile(RE_XML_ILLEGAL)

		# <=3.3 doesn't support create_default_context()

Only use HTTPS #178

Only use HTTPS #178

Conversation

simmel commented Oct 5, 2016

hugovk commented Oct 5, 2016

simmel commented Oct 5, 2016

hugovk commented Oct 7, 2016 • edited

simmel commented Oct 13, 2016

simmel commented Oct 19, 2016

hugovk commented Oct 19, 2016

simmel commented Oct 23, 2016

hugovk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simmel commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simmel commented Oct 25, 2016

hugovk left a comment

Choose a reason for hiding this comment

simmel commented Oct 27, 2016

simmel commented Oct 28, 2016

simmel commented Oct 28, 2016

simmel commented Oct 29, 2016

hugovk commented Oct 29, 2016

simmel commented Oct 29, 2016

hugovk commented Oct 29, 2016

simmel commented Oct 29, 2016

hugovk commented Oct 30, 2016

hugovk commented Jan 3, 2017

hugovk commented Oct 7, 2016 •

edited