Skip to content

Conversation

@risforrob
Copy link
Contributor

to no longer use specifically TLSv1 but instead use the defualt TLS and require no sslV2 and V3.

@posita
Copy link
Contributor

posita commented Oct 25, 2017

So this is basically to make more future-friendly as we update OpenSSL implementations? If that's accurate, then LGTM in theory. 😁

Background

From https://wiki.openssl.org/index.php/SSL_and_TLS_Protocols#Security :

It is recommended to run TLSv1.0, 1.1 or 1.2 and fully disable SSLv2 and SSLv3 that have protocol weaknesses.

But see https://www.howsmyssl.com/s/about.html#version :

TLS 1.0 is the first version of TLS, is fairly common in the world, and requires workarounds in both the client and server to work securely for all cipher suites. TLS 1.0 is also unable to use modern cipher suites that offer greater security and efficiency. Clients using it will be marked as Bad.

cert_reqs=ssl.CERT_REQUIRED,
ca_certs=_TRUSTED_CERT_FILE,
ssl_version=ssl.PROTOCOL_TLSv1,
ssl_version=(ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen this approach before. Can these be used alone (i.e., without any base PROTOCOL_ constant)? Is there an example of this usage? It's not clear in the docs (see ssl.OP_ALL for py2 et seq. and ssl.OP_ALL for py3 et seq.)

block=block,
cert_reqs=ssl.CERT_REQUIRED,
ca_certs=_TRUSTED_CERT_FILE,
ssl_version=ssl.PROTOCOL_TLSv1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is what you want? From https://docs.python.org/2/library/ssl.html#ssl.wrap_socket:

The parameter ssl_version specifies which version of the SSL protocol to use. … If not specified, the default is PROTOCOL_SSLv23; it provides the most compatibility with other versions.

@risforrob
Copy link
Contributor Author

As was pointed out to me

  1. my ssl_version setting was missing a protocol version, which made the argument value useless
  2. This is already correctly set in urllib3 https://github.com/shazow/urllib3/blob/1.22/urllib3/util/ssl_.py#L219

Copy link
Contributor

@posita posita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nice. OpenSSL stuff is often…counterintuitive. 😒 Thanks for the education! 😁

LGTM. 👍 Squash commits before rebase-and-merge?

@risforrob risforrob merged commit e3a489c into master Nov 1, 2017
@risforrob risforrob deleted the update_tls branch November 1, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants