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.

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

Disable the use of session tickets on TLSv1.2 by default #1970

Merged
merged 3 commits into from Sep 25, 2020

Conversation

PleasantMachine9
Copy link
Contributor

See #1886 and #1898

Since currently session resumption is not supported by urllib3,
there is no reason to request tickets from the server.
It takes up extra bytes in transit (~200 bytes), and raises
some minor security concerns.

See also: https://blog.filippo.io/we-need-to-talk-about-session-tickets/

Hopefully we can add support for resumption, but until then, this flag (NO_TICKET) has no reason to be off.

Since currently session resumption is not supported by urllib3,
there is no reason to request tickets from the server.
It takes up extra bytes in transit (~200 bytes), and raises
some minor security concerns.

See also: https://blog.filippo.io/we-need-to-talk-about-session-tickets/
@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #1970 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1970   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         2187      2189    +2     
=========================================
+ Hits          2187      2189    +2     
Impacted Files Coverage Δ
src/urllib3/util/ssl_.py 100.00% <100.00%> (ø)

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 d560e21...1bda47f. Read the comment docs.

@PleasantMachine9
Copy link
Contributor Author

test_ssl_failed_fingerprint_verification
where MaxRetryError("HTTPSConnectionPool(host='localhost', port=49868): Max retries exceeded with url: / (Caused by NewConne...ection.HTTPSConnection object at 0x10c3e5190>: Failed to establish a new connection: [Errno 61] Connection refused'))") = .value

This looks like a flaky test case to me? I don't see how my change would affect this TC.

@sethmlarson
Copy link
Member

This looks like a flaky test case to me? I don't see how my change would affect this TC.

Our CI is a bit flaky sometimes :) might not have been caused by your change.

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

It may not say so in the Python documentation, but according to OpenSSL OP_NO_TICKET is meant for server-side sockets only?

https://www.openssl.org/docs/man1.1.1/man3/SSL_set_options.html

@sethmlarson
Copy link
Member

So I confirmed in Wireshark that for TLS 1.2 specifically setting OP_NO_TICKET does indeed disable the session_ticket (35) extension from being sent with ClientHello.

@PleasantMachine9
Copy link
Contributor Author

Practically speaking it wouldn't make sense for it to be server-side only, so it's probably a documentation issue in openssl.

As per the RFC detailing tickets for TLS 1.2, the client is the first party to indicate whether they support tickets, by including the empty extension.

So it makes sense that we should be able to tell openssl not to send that extension.

@PleasantMachine9
Copy link
Contributor Author

Trying again for the TCs

sethmlarson
sethmlarson previously approved these changes Sep 25, 2020
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This looks good to me now, thanks for putting this together!

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.

None yet

2 participants