bpo-29970: Time out SSL handshake if not complete after 10 seconds by mocmocamoc · Pull Request #4825 · python/cpython

mocmocamoc · 2017-12-12T22:43:20Z

Currently if a SSL server is awaiting a handshake and never receives any data at all, it will remain stuck in that state indefinitely. This means that a client can open an unlimited number of connections to the server and not send any data, eventually exhausting the server's file handle limit if this continues.

This change is to add a timeout event that will abort a connection if the handshake does not complete within ten seconds of starting.

https://bugs.python.org/issue29970

the-knights-who-say-ni · 2017-12-12T22:43:22Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

mocmocamoc · 2017-12-13T10:35:05Z

Please could the CLA check be rerun?

asvetlov

Test is required

bedevere-bot · 2017-12-13T11:36:10Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mocmocamoc · 2017-12-13T19:13:11Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-13T19:13:13Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

asvetlov · 2017-12-13T20:15:26Z

I have a feeling that handshake timeout should be configurable.
Passing it as a parameter for create_server/create_connection is not perfect -- we need adding it for streams also and probably to UNIX sockets.
Moreover shutdown timeout appears just after merging the PR.
Loop attrs are even worse: they are global for every connections but different peers require different handshake timeouts.

@1st1 do you have a suggestion?

1st1 · 2017-12-13T20:21:17Z

Yes, we need a way to configure it. Adding 10 seconds delay to asyncio test suite is not acceptable too.

mocmocamoc · 2017-12-13T20:59:39Z

I've added handshake_timeout as a constructor argument, and reduced the timeout to 0.1s for the purposes of the tests.

asvetlov · 2017-12-13T21:03:22Z

Parameter in SSLProtocol.__init__ is not enough: the class is not exposed to user, there is no way to configure the param by asyncio public API.

1st1 · 2017-12-13T22:04:23Z

                            reuse_address=None,
-                            reuse_port=None):
+                            reuse_port=None,
+                            handshake_timeout=10.0):


The parameter should be called ssl_handshake_timeout

1st1 · 2017-12-13T22:06:21Z

It would be nice to research how long is handshake timeout in other frameworks. Quick google gave me this: https://support.f5.com/csp/article/K13834 -- which might deserve to be mentioned in a comment. Please try to find more resources on this.

…eout

mocmocamoc · 2017-12-15T12:06:42Z

@1st1 @asvetlov Please could you review the latest changes?

asvetlov · 2017-12-15T20:46:26Z

    def _make_ssl_transport(self, rawsock, protocol, sslcontext, waiter=None,
                            *, server_side=False, server_hostname=None,
-                            extra=None, server=None):
+                            extra=None, server=None, ssl_handshake_timeout=10.0):


Add SSL_HANDSHAKE_TIMEOUT = 10.0 to constants.py and reuse the constant everywhere.

I mean everywhere you have 10.0 constant.

mocmocamoc · 2017-12-17T20:37:22Z

I've moved the constant into constants.py as SSL_HANDSHAKE_TIMEOUT.

1st1 · 2017-12-18T03:55:07Z

            sock.listen(backlog)
            sock.setblocking(False)
-            self._start_serving(protocol_factory, sock, ssl, server, backlog)
+            self._start_serving(protocol_factory, sock, ssl, server, backlog, ssl_handshake_timeout)


Please make sure all lines are shorter than 79 characters (PEP 8).

1st1

Please also fix line lengths to be < 79

1st1 · 2017-12-18T03:56:11Z

        # the SSL handshake
        self._write_backlog.append((b'', 1))
        self._loop.call_soon(self._process_write_backlog)
+        self._loop.call_later(self._ssl_handshake_timeout, self._check_handshake_timeout)


You should save a reference to this handle (self._handshake_timeout_handle = self._loop.call_later(...)) and cancel it (self. _handshake_timeout_handle.cancel()) if the handshake is successful.

1st1 · 2017-12-18T03:59:04Z

            server_side=False,
-            server_hostname='python.org')
+            server_hostname='python.org',
+            ssl_handshake_timeout=ANY)


Using ANY kind of defeats the purpose of the test. Do this:

handshake_timeout = object()

and then pass it instead of ANY and assert calls with that handshake_timeout object.

bedevere-bot · 2017-12-18T04:01:39Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mocmocamoc · 2017-12-18T20:03:31Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-18T20:03:33Z

Thanks for making the requested changes!

@asvetlov, @1st1: please review the changes made to this pull request.

1st1 · 2017-12-18T20:26:53Z

+    async def _create_connection_transport(
+            self, sock, protocol_factory, ssl,
+            server_hostname, server_side=False,
+            ssl_handshake_timeout=constants.SSL_HANDSHAKE_TIMEOUT


Move the ): one line up: ssl_handshake_timeout=constants.SSL_HANDSHAKE_TIMEOUT):

1st1 · 2017-12-18T20:27:42Z

+            self, protocol_factory, sock,
+            sslcontext=None, server=None, backlog=100,
+            ssl_handshake_timeout=constants.SSL_HANDSHAKE_TIMEOUT
+            ):


Move ): one line up, don't let it hanging there alone :)

1st1

Aside from code-formatting nits (please fix ): in otherwise empty lines) LGTM.

asvetlov · 2017-12-19T09:32:12Z

The code is perfect but documentation should be updated to reflect new parameter.
Please add .. versionadded:: 3.7 tags as well.

mocmocamoc · 2017-12-19T18:12:02Z

Documentation updated.

asvetlov · 2017-12-19T19:45:49Z

Thanks!

Time out SSL handshake if not complete after 10 seconds

1e85836

the-knights-who-say-ni added the CLA not signed label Dec 12, 2017

bedevere-bot added the awaiting review label Dec 12, 2017

asvetlov requested changes Dec 13, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 13, 2017

asvetlov removed the CLA not signed label Dec 13, 2017

the-knights-who-say-ni added the CLA signed label Dec 13, 2017

asvetlov added the topic-asyncio label Dec 13, 2017

Neil Aspinall added 3 commits December 13, 2017 18:57

Add test for SSL handshake timeout

b7467e4

Add news and ACKS entries

f655c5c

Fixing whitespace issue

85c9d7a

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 13, 2017

Make handshake_timeout configurable, use short timeout for tests

3168967

Expose handshake_timeout via start/create_server

0b2d3cc

mocmocamoc requested a review from 1st1 as a code owner December 13, 2017 22:02

1st1 reviewed Dec 13, 2017

View reviewed changes

Neil Aspinall added 2 commits December 13, 2017 22:34

Rename handshake_timeout to ssl_handshake_timeout, add documentation

ef373ca

Fix whitespace issue

4ca9f14

Merge remote-tracking branch 'upstream/master' into ssl_handshake_tim…

654f974

…eout

asvetlov reviewed Dec 15, 2017

View reviewed changes

Added SSL_HANDSHAKE_TIMEOUT to constants.py

b80e768

1st1 reviewed Dec 18, 2017

View reviewed changes

1st1 requested changes Dec 18, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Dec 18, 2017

Cancel SSL handshake check once successful, PEP8 and test fixes

6d5ed76

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 18, 2017

1st1 reviewed Dec 18, 2017

View reviewed changes

1st1 approved these changes Dec 18, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 18, 2017

Fix lonely parentheses

ec887cd

Add ssl_handshake_timeout to asyncio documentation

fa4249e

asvetlov merged commit f7686c1 into python:master Dec 19, 2017

bedevere-bot removed the awaiting merge label Dec 19, 2017

mocmocamoc deleted the ssl_handshake_timeout branch December 19, 2017 19:49

asvetlov mentioned this pull request Dec 20, 2017

bpo-29970: Make ssh_handshake_timeout None by default #4939

Merged

mocmocamoc mentioned this pull request Dec 20, 2017

TCP handle leakage for "stale" connections kyuupichan/electrumx#94

Closed

Uh oh!

Conversation

mocmocamoc commented Dec 12, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Dec 12, 2017

Uh oh!

mocmocamoc commented Dec 13, 2017

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 13, 2017

Uh oh!

mocmocamoc commented Dec 13, 2017

Uh oh!

bedevere-bot commented Dec 13, 2017

Uh oh!

asvetlov commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Dec 13, 2017

Uh oh!

mocmocamoc commented Dec 13, 2017

Uh oh!

asvetlov commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 13, 2017

Uh oh!

mocmocamoc commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mocmocamoc commented Dec 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 18, 2017

Uh oh!

mocmocamoc commented Dec 18, 2017

Uh oh!

bedevere-bot commented Dec 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Dec 19, 2017

Uh oh!

mocmocamoc commented Dec 19, 2017

Uh oh!

asvetlov commented Dec 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mocmocamoc commented Dec 12, 2017 •

edited by bedevere-bot

Loading

asvetlov commented Dec 13, 2017 •

edited

Loading