Closing connection using TLS

[dododedodonl]

When closing a connection on a server using TLS, eg:

<?php

require __DIR__ . '/vendor/autoload.php';

$server = new React\Socket\TcpServer($port = 9900);
$server = new React\Socket\SecureServer($server, null, [
    'local_cert' => 'server.pem',
    'passphrase' => 'secret'
]);

$server->on('connection', function (React\Socket\ConnectionInterface $connection) {
    echo '[' . $connection->getRemoteAddress() . ' connected]' . PHP_EOL;

    // do stuff
    
    $connection->end();
});

the connection is not closed properly, as

socket/src/Connection.php

Lines 123 to 133 in e04478a

	public function handleClose()
	{
	if (!\is_resource($this->stream)) {
	return;
	}
	// Try to cleanly shut down socket and ignore any errors in case other
	// side already closed. Underlying Stream implementation will take care
	// of closing stream resource, so we otherwise keep this open here.
	@\stream_socket_shutdown($this->stream, \STREAM_SHUT_RDWR);
	}

uses https://www.php.net/manual/en/function.stream-socket-shutdown.php.

Note comment https://www.php.net/manual/en/function.stream-socket-shutdown.php#125659 which states tls does not get shut down properly that way.

Solution seems to be to replace @\stream_socket_shutdown($this->stream, \STREAM_SHUT_RDWR); with \fclose($this->stream).

Other solution seems to be to call @\stream_socket_enable_crypto($this->stream, false); before the socket shutdown as mentioned in comment https://www.php.net/manual/en/function.stream-socket-shutdown.php#126303

[clue]

[…] tls does not get shut down properly that way.

What problem are you seeing exactly? May I ask you to provide a gist to reproduce this?

Note that this call will be followed immediately by fclose() in the Stream component.

[dododedodonl]

What problem are you seeing exactly? [...]

When the connection is closed by the server, the client outputs an error:

Error: GnuTLS error -110 in gnutls_record_recv: The TLS connection was non-properly terminated.
Server did not properly shut down TLS connection
Error: Could not read from socket: ECONNABORTED - Connection aborted
Error: Disconnected from server

Replacing

socket/src/Connection.php

Lines 123 to 133 in e04478a

	public function handleClose()
	{
	if (!\is_resource($this->stream)) {
	return;
	}
	// Try to cleanly shut down socket and ignore any errors in case other
	// side already closed. Underlying Stream implementation will take care
	// of closing stream resource, so we otherwise keep this open here.
	@\stream_socket_shutdown($this->stream, \STREAM_SHUT_RDWR);
	}

with

    public function handleClose()
    {
        if (!\is_resource($this->stream)) {
            return;
        }

        // Try to cleanly shut down socket and ignore any errors in case other
        // side already closed. Underlying Stream implementation will take care
        // of closing stream resource, so we otherwise keep this open here.
        if ($this->encryptionEnabled) {
            @\stream_socket_enable_crypto($this->stream, false);
        }
        @\stream_socket_shutdown($this->stream, \STREAM_SHUT_RDWR);
    }

resolves the issue.

From what I understand, using stream_socket_shutdown() causes the sockets to get closed before the by TLS protocol required "close_notify" message is send. This does not get resolved by a later call to fclose() because the socket is already closed by then.

[clue]

What problem are you seeing exactly? [...]

When the connection is closed by the server, the client outputs an error:

Error: GnuTLS error -110 in gnutls_record_recv: The TLS connection was non-properly terminated.
Server did not properly shut down TLS connection
Error: Could not read from socket: ECONNABORTED - Connection aborted
Error: Disconnected from server

What you're saying may be entirely correct, but we need to double check which "client outputs an error" under what circumstances exactly. I would love to add some tests to confirm correct behavior and avoid future regressions, as we can't randomly change this logic without having some safety nets in place.

Perhaps you can help us by giving some clear instructions how we can reproduce the specific problem you're seeing locally?

[dododedodonl]

What you're saying may be entirely correct, but we need to double check which "client outputs an error" under what circumstances exactly. I would love to add some tests to confirm correct behavior and avoid future regressions, as we can't randomly change this logic without having some safety nets in place.

That is very sound and I would agree with that logic :)

Perhaps you can help us by giving some clear instructions how we can reproduce the specific problem you're seeing locally?

I am tinkering around with a rudimentary implementation of ftps with implicit tls in PHP using reactphp. I ran into this problem when using filezilla as client. At some point, it issues the LIST command which requests data over a second connection. I send this data, and then shut down the data connection as per ftps spec. The shutdown triggers the mentioned error. I was not able to reduce it down to a simple reproduction with eg. server.php and client.php solely using reactphp (the behaviour is not there). It seems to be dependent on the client's connection logic.