Allow setting TLS max/min versions #138

jay0lee · 2019-05-01T16:53:19Z

Add httplib2.MAX_TLS_VERSION and httplib2.MIN_TLS_VERSION. Python 3.7+ and OpenSSL 1.1+ allow setting these:

https://docs.python.org/3/library/ssl.html#ssl.SSLContext.maximum_version

tests don't seem to be possible (yet) though as Travis Xenial image is using OpenSSL 1.0.

codecov · 2019-05-01T16:58:56Z

Codecov Report

Merging #138 into master will increase coverage by 0.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   73.57%   73.59%   +0.01%     
==========================================
  Files           8        8              
  Lines        2547     2556       +9     
==========================================
+ Hits         1874     1881       +7     
- Misses        673      675       +2

Impacted Files	Coverage Δ
python3/httplib2/__init__.py	`83.31% <80%> (-0.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd741d1...7ea85cc. Read the comment docs.

temoto · 2019-05-01T18:41:03Z

Great, thank you.

Think it could be done without global variable, per host?

You want to revert chmod on httplib2test.py or I can do it?

jay0lee · 2019-05-01T19:01:15Z

oops, fixed the chmod.

Regarding non-global way to do this, I couldn't find one.

jay0lee · 2019-05-21T16:31:05Z

friendly ping, anything else you need here?

temoto · 2019-05-22T06:41:26Z

Please try something like this

diff --git a/python3/httplib2/__init__.py b/python3/httplib2/__init__.py
index 73450be..7c72671 100644
--- a/python3/httplib2/__init__.py
+++ b/python3/httplib2/__init__.py
@@ -175,7 +175,8 @@ DEFAULT_TLS_VERSION = getattr(ssl, "PROTOCOL_TLS", None) or getattr(


 def _build_ssl_context(
-    disable_ssl_certificate_validation, ca_certs, cert_file=None, key_file=None
+    disable_ssl_certificate_validation, ca_certs, cert_file=None, key_file=None,
+    maximum_version=None, minimum_version=None,
 ):
     if not hasattr(ssl, "SSLContext"):
         raise RuntimeError("httplib2 requires Python 3.2+ for ssl.SSLContext")
@@ -185,6 +186,11 @@ def _build_ssl_context(
         ssl.CERT_NONE if disable_ssl_certificate_validation else ssl.CERT_REQUIRED
     )

+    if maximum_version is not None and hasattr(context, "maximum_version"):
+        context.maximum_version = getattr(ssl.TLSVersion, maximum_version)
+    if minimum_version is not None and hasattr(context, "minimum_version"):
+        context.minimum_version = getattr(ssl.TLSVersion, minimum_version)
+
     # check_hostname requires python 3.4+
     # we will perform the equivalent in HTTPSConnectionWithTimeout.connect() by calling ssl.match_hostname
     # if check_hostname is not supported.
@@ -1226,6 +1232,8 @@ class HTTPSConnectionWithTimeout(http.client.HTTPSConnection):
         proxy_info=None,
         ca_certs=None,
         disable_ssl_certificate_validation=False,
+        tls_maximum_version=None,
+        tls_minimum_version=None,
     ):

         self.disable_ssl_certificate_validation = disable_ssl_certificate_validation
@@ -1236,7 +1244,8 @@ class HTTPSConnectionWithTimeout(http.client.HTTPSConnection):
             self.proxy_info = proxy_info("https")

         context = _build_ssl_context(
-            self.disable_ssl_certificate_validation, self.ca_certs, cert_file, key_file
+            self.disable_ssl_certificate_validation, self.ca_certs, cert_file, key_file,
+            maximum_version=tls_maximum_version, minimum_version=tls_minimum_version,
         )
         super(HTTPSConnectionWithTimeout, self).__init__(
             host,
@@ -1384,6 +1393,8 @@ class Http(object):
         proxy_info=proxy_info_from_environment,
         ca_certs=None,
         disable_ssl_certificate_validation=False,
+        tls_maximum_version=None,
+        tls_minimum_version=None,
     ):
         """If 'cache' is a string then it is used as a directory name for
         a disk cache. Otherwise it must be an object that supports the
@@ -1411,6 +1422,8 @@ class Http(object):
         self.proxy_info = proxy_info
         self.ca_certs = ca_certs
         self.disable_ssl_certificate_validation = disable_ssl_certificate_validation
+        self.tls_maximum_version = tls_maximum_version
+        self.tls_minimum_version = tls_minimum_version
         # Map domain name to an httplib connection
         self.connections = {}
         # The location of the cache, for now a directory
@@ -1753,6 +1766,8 @@ a string that contains the response entity body.
                             proxy_info=self.proxy_info,
                             ca_certs=self.ca_certs,
                             disable_ssl_certificate_validation=self.disable_ssl_certificate_validation,
+                            tls_maximum_version=self.tls_maximum_version,
+                            tls_minimum_version=self.tls_minimum_version,
                         )
                     else:
                         conn = self.connections[conn_key] = connection_type(
@@ -1761,6 +1776,8 @@ a string that contains the response entity body.
                             proxy_info=self.proxy_info,
                             ca_certs=self.ca_certs,
                             disable_ssl_certificate_validation=self.disable_ssl_certificate_validation,
+                            tls_maximum_version=self.tls_maximum_version,
+                            tls_minimum_version=self.tls_minimum_version,
                         )
                 else:
                     conn = self.connections[conn_key] = connection_type(

temoto · 2019-05-23T16:40:06Z

@jay0lee it still has global variables. I believe this will be used to create bad surprises in the long run.

jay0lee · 2019-05-23T16:43:04Z

ah, I assumed your patch was additive. I can pull out the globals now then.

temoto

LGTM

@jay0lee thank you.

stinky2nine · 2019-05-30T00:57:49Z

python3/httplib2/__init__.py

@@ -185,6 +185,13 @@ def _build_ssl_context(
        ssl.CERT_NONE if disable_ssl_certificate_validation else ssl.CERT_REQUIRED
    )

+    # SSLContext.maximum_version and SSLContext.minimum_version are python 3.7+.


# SSLContext.maximum_version and SSLContext.minimum_version are Python 3.7+: # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.maximum_version

stinky2nine · 2019-05-30T00:58:31Z

python3/httplib2/__init__.py

@@ -185,6 +185,13 @@ def _build_ssl_context(
        ssl.CERT_NONE if disable_ssl_certificate_validation else ssl.CERT_REQUIRED
    )

+    # SSLContext.maximum_version and SSLContext.minimum_version are python 3.7+.
+    # source: https://docs.python.org/3/library/ssl.html#ssl.SSLContext.maximum_version
+    if maximum_version is not None and hasattr(context, "maximum_version"):


Can you also add code coverage?

Travis Python 3.7 uses OpenSSL 1.0 which won't support these attributes, the only way to do testing would be to pre-compile Python and OpenSSL on Travis and then run the tests against that. I can submit such a PR for .travis.yml (I do something like this for my own downstream project) but it will slow Travis testing down somewhat on that configuration.

@temoto Use tox-travis to force py37 and OpenSSL 1.1.0+? https://stackoverflow.com/questions/37016688/how-to-run-tox-with-travis-ci/

My suggestion to keep things organized is either:

full revamp of current config to take full advantage of tox-travis

just another job conforming to existing config

jobs: include: #... - stage: test env: _=py3-openssl-110 python: 3.7 install: - CUSTOM_OPENSSL_INSTALL_SCRIPT - pip install $pip_install_common 'codecov>=2.0.15' -r requirements-test.txt script: script/test -sv

I'd only slightly prefer not tox-travis route because it's many changes and you still need to install custom openssl.

CUSTOM_OPENSSL_INSTALL_SCRIPT should be noop with hot Travis cache.

pyopenssl actually tests something like this on Travis using MacOS which can use OpenSSL 1.1.1b (latest) via a brew update:

https://travis-ci.org/pyca/pyopenssl/jobs/537306843

I'm currently testing adding an additional job that would use MacOS, Python 3.7 and OpenSSL 1.1+ so we can actually test TLS min/max availability.

Ok, MacOS proved far to problematic so went with your suggestion of a custom script to compile.

I've added tests that are now passing

I removed Python 3.4 support as it's end of life and broke the tests.
Let me know if you have thoughts on better testing or anything else.

Python 3.7.3

stinky2nine · 2019-05-31T03:05:57Z

@temoto Please take another look and merge.

temoto · 2019-06-05T12:07:07Z

@jay0lee sorry it took so long. Please have a look here 890b822 and say if you like to put that version. I'm happy if you are.

Major change: only build results go into Travis cache, sources die in /tmp.
Minor changes: verify installed python/openssl versions, build without libpython library, subshell () to not care about current directory, small syntax things.

jay0lee · 2019-06-05T12:27:15Z

@temoto that looks good except that you're using 1.1.1c instead of $OPENSSL_VERSION in the check at the end. It should be possible for Bash to expand the variable before Python executes the inline script.

Do you want me to pull these changes into my PR fork?

temoto · 2019-06-05T16:35:33Z

@temoto that looks good except that you're using 1.1.1c instead of $OPENSSL_VERSION in the check at the end. It should be possible for Bash to expand the variable before Python executes the inline script.

Yeah, sorry. Just tested this syntax works: [[ $(python print ...) = ${openssl_version}* ]]

Do you want me to pull these changes into my PR fork?

It was done separately for maybe you would not like your work changed so much and keep it.
Now with your approval I'll just go ahead and merge. It's been too long already.

temoto · 2019-06-06T19:51:56Z

@jay0lee thank you very much. Your patch was just released in 0.13.0

…b2#138 commit e92155b85c719405fefe69b045f3bcf55d4fbcc1 Author: Sergey Shepelev <temotor@gmail.com> Date: Fri Jun 7 00:40:38 2019 +0500 v0.13.0 release commit 2a806cb4bc731058541ab902101ea7750f350282 Author: Jay Lee <jay0lee@gmail.com> Date: Thu Jun 6 02:59:21 2019 -0400 Allow setting TLS max/min versions httplib2/httplib2#138

jay0lee added 7 commits May 1, 2019 10:22

MAX_TLS_VERSION and MIN_TLS_VERSION

72030da

TLS min/max tests

f6d3300

test_external

63d7345

fix tests

d14079b

more fixes

6cacbc9

more fixes

282663a

undo test attempts

9a2114b

fix mode f httplib2test.py

f5e2d59

also allow per-host max/min

6ffd316

remove global vars

b721fcb

temoto approved these changes May 24, 2019

View reviewed changes

temoto requested review from stinky2nine, cdent and dims May 24, 2019 17:38

stinky2nine reviewed May 30, 2019

View reviewed changes

jay0lee added 8 commits May 30, 2019 12:22

Test on MacOS with OpenSSL 1.1.1b

225f6b0

Update .travis.yml

5b04e98

Python 3.7.3

brew install python and openssl

efcfad7

New test job with custom Python 3.7/OpenSSL 1.1 build

a7abdfa

build script cleanup

1f61d8e

fix --with-openssl

d35d50c

minimal apt work, fix SSL_INSTALL

0bb8f49

need to export

7c89231

jay0lee added 7 commits May 30, 2019 15:55

tests, fail if Python support isn't present

9128008

skip tests on 2.7

221d931

fix mark.skipif, leave cache intact

20cde98

fix SSLContext() and Http() parameters

dc2a43d

actually attempt connection

6f9a319

drop deprecated 3.4, pylint cleanup

acd58f1

more pylint stuff

a334892

stinky2nine approved these changes May 31, 2019

View reviewed changes

TEMP WIP trying to improve script/compile-py3-openssl11.sh

7ea85cc

temoto merged commit 2a806cb into httplib2:master Jun 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting TLS max/min versions #138

Allow setting TLS max/min versions #138

jay0lee commented May 1, 2019

codecov bot commented May 1, 2019 •

edited

Loading

temoto commented May 1, 2019

jay0lee commented May 1, 2019

jay0lee commented May 21, 2019

temoto commented May 22, 2019

temoto commented May 23, 2019

jay0lee commented May 23, 2019

temoto left a comment

stinky2nine May 30, 2019

stinky2nine May 30, 2019

jay0lee May 30, 2019

stinky2nine May 30, 2019

temoto May 30, 2019

jay0lee May 30, 2019

jay0lee May 30, 2019

stinky2nine commented May 31, 2019

temoto commented Jun 5, 2019

jay0lee commented Jun 5, 2019

temoto commented Jun 5, 2019

temoto commented Jun 6, 2019

Allow setting TLS max/min versions #138

Allow setting TLS max/min versions #138

Conversation

jay0lee commented May 1, 2019

codecov bot commented May 1, 2019 • edited Loading

Codecov Report

temoto commented May 1, 2019

jay0lee commented May 1, 2019

jay0lee commented May 21, 2019

temoto commented May 22, 2019

temoto commented May 23, 2019

jay0lee commented May 23, 2019

temoto left a comment

Choose a reason for hiding this comment

stinky2nine May 30, 2019

Choose a reason for hiding this comment

stinky2nine May 30, 2019

Choose a reason for hiding this comment

jay0lee May 30, 2019

Choose a reason for hiding this comment

stinky2nine May 30, 2019

Choose a reason for hiding this comment

temoto May 30, 2019

Choose a reason for hiding this comment

jay0lee May 30, 2019

Choose a reason for hiding this comment

jay0lee May 30, 2019

Choose a reason for hiding this comment

stinky2nine commented May 31, 2019

temoto commented Jun 5, 2019

jay0lee commented Jun 5, 2019

temoto commented Jun 5, 2019

temoto commented Jun 6, 2019

codecov bot commented May 1, 2019 •

edited

Loading