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

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

Conversation

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

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!

Please could the CLA check be rerun?

Choose a reason for hiding this comment

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

Test is required

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.

Neil Aspinall added 3 commits

December 13, 2017 18:57

I have made the requested changes; please review again

Thanks for making the requested changes!

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

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?

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

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

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.

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

Choose a reason for hiding this comment

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

The parameter should be called ssl_handshake_timeout

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.

Neil Aspinall added 2 commits

December 13, 2017 22:34

		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):

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

I mean everywhere you have 10.0 constant.

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

		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)

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Please also fix line lengths to be < 79

		# 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)

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

I have made the requested changes; please review again

Thanks for making the requested changes!

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

1st1 approved these changes Dec 18, 2017

Choose a reason for hiding this comment

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

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

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

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels