Endless loop in "_handle_control_chars()" if server sends EOF with zero bytes and endless loop in "channel_authenticate_telnet" in "async_channel.py" with same reason

[pertsevds]

Describe the bug
Bug in asynctelnet.
Code never returns from "open()" if server sends EOF.
When you connect to telnet server that sends EOFs, your code gets stuck in the endless loop in "_handle_control_chars()", because asyncio.StreamReader.read(1) will return an empty bytes object if EOF was received and the internal buffer is empty.

Here is a problem lines in "transport.py":

scrapli/scrapli/transport/plugins/asynctelnet/transport.py

Lines 127 to 133 in 062a5f4

	while True:
	try:
	c = await asyncio.wait_for(self.stdout.read(1), timeout=char_read_timeout)
	except asyncio.TimeoutError:
	return
	char_read_timeout = self._base_transport_args.timeout_socket / 10
	control_buf = self._handle_control_chars_response(control_buf=control_buf, c=c)

This can be easly fixed with

                if not c:
                    break

after the line number 129,

but there is another problem just right after that one - with authentication in "channel_authenticate_telnet" in "async_channel.py"

scrapli/scrapli/channel/async_channel.py

Lines 329 to 339 in 062a5f4

	async with self._channel_lock():
	while True:
	buf = await self.read()
	if not buf:
	current_iteration_time = datetime.now().timestamp()
	if (current_iteration_time - auth_start_time) > (
	return_interval * return_attempts
	):
	self.send_return()
	return_attempts += 1

I see increments in "return_attempts" but there is no limit for them. And it never stops.
I think we need to add some variable like "max_auth_return_retry" in "BaseChannelArgs".
And when return_attempts >= max_auth_return_retry - raise ScrapliAuthenticationFailed.

To Reproduce
Steps to reproduce the behavior:
pip install scrapli==2021.7.30a3

1. Your script

        conn_dict = {
            "host": ip,
            "timeout_socket": SCRAPLI_SOCKET_TIMEOUT,
            "timeout_transport": SCRAPLI_TRANSPORT_TIMEOUT,
            "timeout_ops": SCRAPLI_OPERATION_TIMEOUT,
            "comms_prompt_pattern": SCRAPLI_PROMPT_PATTERN,
            "port": 23,
            "transport": "asynctelnet",
            "auth_username": login,
            "auth_password": password
        }
            conn = AsyncGenericDriver(**conn_dict)
            try:
                await conn.open()
            except Exception as ex:
                logging.debug(str(ex))
            else:
                await conn.close()

2. What you're connecting to (vendor, platform, version)
   D-Link DES-1210-28/ME.

Expected behavior
Code returns from "open()" if server sends EOFs with ScrapliAuthenticationFailed.

Screenshots

OS (please complete the following information):

• OS: Windows 10
• scrapli version: 2021.7.30a3

Additional context
I need some guidance on how to implement limiting function for "return_attempts" in "channel_authenticate_telnet" in "async_channel.py".

[pertsevds]

Oh, and i see "sync_channel.py" must be fixed as well.

[carlmontanari]

Hey @davaeron thanks for opening this!

I see there is a PR as well but will just respond here and we can sort stuff out and then get things merged as needed.

When you connect to telnet server that sends EOFs, your code gets stuck in the endless loop in "_handle_control_chars()", because asyncio.StreamReader.read(1) will return an empty bytes object if EOF was received and the internal buffer is empty.

I was able to sorta recreate this by never returning from the _handle_control_chars method and then clearing the vty line on a switch. This caused the situation you described with the stream reader just returning an empty byte string. In testing the proposed fix I ran my functional tests against my XRV9K vm, and without bouncing around between the transports, it seems that the VM decides to not open telnet connections in quick succession and it acted in this same way -- having that c variable just be an empty byte string.

If we get an EOF here then I assume we should be raising ScrapliConnectionNotOpened not just returning from the _handle_control_chars method no? From what I've seen this is always a fatal condition so there would be no point in continuing on -- is that accurate? If so I think this may be appropriate:

c = await asyncio.wait_for(self.stdout.read(1), timeout=char_read_timeout)
if not c:
    raise ScrapliConnectionNotOpened("server returned EOF, connection not opened")

Thoughts?

but there is another problem just right after that one - with authentication in "channel_authenticate_telnet" in "async_channel.py"

I see increments in "return_attempts" but there is no limit for them. And it never stops.
I think we need to add some variable like "max_auth_return_retry" in "BaseChannelArgs".
And when return_attempts >= max_auth_return_retry - raise ScrapliAuthenticationFailed.

I dont believe this is actually an issue -- the channel_authenticate_telnet in both the sync and async channels are wrapped with the ChannelTimeout decorator which will raise a time out exception (governed by the timeout_ops attribute of the connection), so its not really unlimited. Unless there is a really good reason to track/care about how many times the return is sent I dont think I would want to modify how this part works, but I'm open to hearing what ya think?!

Carl

[pertsevds]

I think that

c = await asyncio.wait_for(self.stdout.read(1), timeout=char_read_timeout)
if not c:
    raise ScrapliConnectionNotOpened("server returned EOF, connection not opened")

will fix all problems.
Tried that with my switch and scrapli works as intended.

[carlmontanari]

Cool, will push that up to develop shortly. Thanks for opening this and for the help!!

Carl

[pertsevds]

Thank you for the help!