Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reenable TLS1.1 and 1.2 while leaving SSLv3 disabled #2385

Merged
merged 1 commit into from
Jun 28, 2016
Merged

Reenable TLS1.1 and 1.2 while leaving SSLv3 disabled #2385

merged 1 commit into from
Jun 28, 2016

Conversation

chennin
Copy link
Contributor

@chennin chennin commented Jun 28, 2016 •
edited
Loading

Description:
#2375 successfully deprecated SSLv2 and v3, but also disabled TLSv1.1 and v1.2. This reenables TLSv1.1 and 1.2 while leaving SSLv2 and v3 disabled. It also prevents CRIME because it can.

Details:
eventlet.wrap_ssl is just a wrapper for ssl.wrap_socket, which is less flexible than SSLContext.wrap_socket(). So this creates an SSL context and sets the same options (cert, key, ciphers, server_side) while setting the TLS versions more forward-compatibly (ssl.PROTOCOL_SSLv23 selects the highest available version).

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

@danieljkemp
Copy link
Contributor

My understanding of the Python ssl library used
https://docs.python.org/3.5/library/ssl.html#ssl.wrap_socket is that one
version must be specified. I selected tlsv1.0 so we'd follow the guidelines
to the letter.

I'd be in favor of adding tls1.1/tls1.2 . Prior to this update the
ssl.wrap_socket interface simply defaulted to ssl.PROTOCOL_SSLv23 which
selects the highest the client supports which allows for downgrade attacks.

I don't see an option to allow for negotiation that also bans sslv2/3. Any
suggestions here? I'm not entirely sure we can default to supporting only
tls1.2 even if it would be the most secure.

On Tue, Jun 28, 2016, 16:32 AlucardZero [email protected] wrote:

Description:

#2375 #2375
successfully deprecated SSLv2 and v3, but also disabled
https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLSv1_1 TLSv1.1
and v1.2. This reenables TLSv1.1 and 1.2 while leaving SSLv2 and v2
disabled. It also prevents CRIME
https://en.wikipedia.org/wiki/CRIME#Prevention because it can.
Details:
eventlet.wrap_ssl is just
https://github.com/eventlet/eventlet/blob/2cd5f1d9aea53efb4526e7185017bdcc84732588/eventlet/convenience.py#L127
a wrapper for ssl.wrap_socket, which is less flexible
https://docs.python.org/3/library/ssl.html#socket-creation than
SSLContext.wrap_socket(). So this creates an SSL context and sets the same
options (cert, key, ciphers, server_side) while setting the TLS versions
more forward-compatibly (ssl.PROTOCOL_SSLv23
https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_SSLv23 selects
the highest available version).

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged
    unless tests pass

You can view, comment on, or merge this pull request online at:

#2385
Commit Summary

  • Reenable TLS1.1 and 1.2 while leaving SSLv3 disabled

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2385, or mute the
thread
https://github.com/notifications/unsubscribe/AJzfuReyBx1ilwivk2rxdnPid9h-mODdks5qQYS8gaJpZM4JAgYK
.

@danieljkemp
Copy link
Contributor

Got it. Looks like we need to create a ssl.Context object, then set the
options flag to disable ssl v2/3. I'll work on a pr asap.

On Tue, Jun 28, 2016, 17:14 Dan Kemp [email protected] wrote:

My understanding of the Python ssl library used
https://docs.python.org/3.5/library/ssl.html#ssl.wrap_socket is that one
version must be specified. I selected tlsv1.0 so we'd follow the guidelines
to the letter.

I'd be in favor of adding tls1.1/tls1.2 . Prior to this update the
ssl.wrap_socket interface simply defaulted to ssl.PROTOCOL_SSLv23 which
selects the highest the client supports which allows for downgrade attacks.

I don't see an option to allow for negotiation that also bans sslv2/3. Any
suggestions here? I'm not entirely sure we can default to supporting only
tls1.2 even if it would be the most secure.

On Tue, Jun 28, 2016, 16:32 AlucardZero [email protected] wrote:

Description:

#2375 #2375
successfully deprecated SSLv2 and v3, but also disabled
https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLSv1_1
TLSv1.1 and v1.2. This reenables TLSv1.1 and 1.2 while leaving SSLv2 and v2
disabled. It also prevents CRIME
https://en.wikipedia.org/wiki/CRIME#Prevention because it can.
Details:
eventlet.wrap_ssl is just
https://github.com/eventlet/eventlet/blob/2cd5f1d9aea53efb4526e7185017bdcc84732588/eventlet/convenience.py#L127
a wrapper for ssl.wrap_socket, which is less flexible
https://docs.python.org/3/library/ssl.html#socket-creation than
SSLContext.wrap_socket(). So this creates an SSL context and sets the same
options (cert, key, ciphers, server_side) while setting the TLS versions
more forward-compatibly (ssl.PROTOCOL_SSLv23
https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_SSLv23 selects
the highest available version).

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged
    unless tests pass

You can view, comment on, or merge this pull request online at:

#2385
Commit Summary

  • Reenable TLS1.1 and 1.2 while leaving SSLv3 disabled

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2385, or mute
the thread
https://github.com/notifications/unsubscribe/AJzfuReyBx1ilwivk2rxdnPid9h-mODdks5qQYS8gaJpZM4JAgYK
.

@balloob
Copy link
Member

balloob commented Jun 28, 2016

@danieljkemp this pr fixes it right?

@chennin
Copy link
Contributor Author

chennin commented Jun 28, 2016

@danieljkemp right - this is actually a PR that does that so you don't have to

@danieljkemp
Copy link
Contributor

Sorry! Issues and PRs look too similar on this device, the fix I threw
together (looks pretty much identical) was just practice! Less git on
cellphone may be a good plan...

On Tue, Jun 28, 2016, 17:39 AlucardZero [email protected] wrote:

@danieljkemp https://github.com/danieljkemp right - this is actually a
PR that does that so you don't have to

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#2385 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AJzfua2bPQAQekE4MK3Bjujki_q2zpYGks5qQZSPgaJpZM4JAgYK
.

@balloob
Copy link
Member

balloob commented Jun 28, 2016

Now that everyone is happy, I'll merge it ! 👍 🐬 🍺

@balloob balloob merged commit 31d2a5d into home-assistant:dev Jun 28, 2016
@chennin chennin deleted the more-tls-improvements branch June 28, 2016 23:56
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chennin @danieljkemp @balloob