Allow getRemoteAddress to return the PID of a connecting Unix connection

[gdejong]

Currently when dealing with a UnixServer, its connections do not return a value for getRemoteAddress, the returned value is actually NULL. I`d like it to return the PID of the connecting client. Is that something you would integrate in this library?

public function getRemoteAddress()
{
    if($this->unix){
        $socket = socket_import_stream($this->stream);

        return socket_get_option($socket, SOL_SOCKET, 17 /*SO_PEERCRED*/);
    }
    return $this->parseAddress(@stream_socket_get_name($this->stream, true));
}

See http://php.net/manual/en/function.socket-get-option.php#101380

[clue]

Absolutely! I like the idea of returning the PID and can see some very valid use cases (local authentication etc.).

If we can integrate this into this library without introducing a BC break and/or a hard dependency on ext-sockets, then I see no reason why we wouldn't want to get this in :+1: PRs with added tests and documentation would be much appreciated :shipit:

[gdejong]

I'd like to ask a few questions before I try to create PR for this :)

• The getRemoteAddress method from the ConnectionInterface interface should return the remote address (URI) or null if unknown. In this case only the PID is known, which is an integer. What should we return? There isn't really a URI scheme for a PID as far as I know.
• Another solution would be to add another method to a Connection, like getUnixPid (name definitely to be decided). For all non-unix connections this would return null, and for unix connections it would return the pid as an integer.

What are your thoughts on this?

[clue]

Very good points!

Interestingly SO_PEERCRED socket option should return a ucred structure with pid, uid and gid. Right now, PHP does not really support this constant and simply seems to return the first element only.

A single accessor may work for now, but I can clearly see some value in the remaining values once PHP actually implements this structure.

As an alternative, what do you think about exposing this as part of the query string?

$client->getRemoteAddress();
unix:///tmp/server.sock?pid=1&uid=2&gid=3

[kelunik]

Depending on the parser you can also expose the PID as port number.

[gdejong]

I have created the most basic Unix server/client to test this feature with. Leaving it here for future reference.

server.php

<?php

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

use React\EventLoop\Factory;
use React\Socket\ConnectionInterface;
use React\Socket\UnixServer;

$loop = Factory::create();

$unix_server = new UnixServer("unix:///tmp/server.sock", $loop);

$unix_server->on("connection", function (ConnectionInterface $connection) {
    echo "New connection from (local): " . $connection->getLocalAddress() . PHP_EOL;
    echo "New connection from (remote): " . $connection->getRemoteAddress() . PHP_EOL;
});

echo "Server is listening..." . PHP_EOL;

$loop->run();

Note: after exiting the server you have to manually remove the /tmp/server.sock file from your file system.

client.php

<?php

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

use React\Socket\ConnectionInterface;
use React\EventLoop\Factory;

$loop = Factory::create();

$connector = new React\Socket\UnixConnector($loop);

$connector->connect('/tmp/server.sock')->then(function (ConnectionInterface $connection) {
    echo "Connected as (local): " . $connection->getLocalAddress() . PHP_EOL;
    echo "Connected as (remote): " . $connection->getRemoteAddress() . PHP_EOL;
    $connection->close();
});

$loop->run();

Current output:

php server.php
Server is listening...
New connection from (local): 'unix:///tmp/server.sock'
New connection from (remote): NULL

php client.php
Connected as (local): NULL
Connected as (remote): 'unix:///tmp/server.sock'

[gdejong]

Small update:

After some trial and error I think adding the PID to getRemoteAddress in not a good idea. A Connection can be either server > client or client > server. In the former case getRemoteAddress returns nothing, in the latter case it returns the Unix socket descriptor (as you can see in my previous post). Adding the PID to nothing and to the Unix socket descriptor feels too hacky.

I feel like adding a separate method (like getUnixPid) is the best solution (it does not affect current behavior and people can use it only when they really want to). Next challenge I encounter: do we add this method to the ConnectionInterface?

Another possibility is to create a new implementation of the ConnectionInterface, like a UnixConnection and have the UnixServer return an instance of this class in its handleConnection. That would even allow the public $unix variable to be removed from the Connection.

Looking forward to hearing your thoughts.

Regards,

[dkrieger]

Following, I was about to enter a similar issue.

[clue]

@gdejong Thank you very much for looking into this!

After some trial and error I think adding the PID to getRemoteAddress in not a good idea. A Connection can be either server > client or client > server. In the former case getRemoteAddress returns nothing, in the latter case it returns the Unix socket descriptor (as you can see in my previous post). Adding the PID to nothing and to the Unix socket descriptor feels too hacky.

Very good input, sounds reasonable.

I feel like adding a separate method (like getUnixPid) is the best solution (it does not affect current behavior and people can use it only when they really want to). Next challenge I encounter: do we add this method to the ConnectionInterface?

I'm not entirely opposed to the idea, but looking at the suggest PR in #153 makes me wonder how this API is going to be consumed by downstream projects depending on this project.

The SO_PEERCRED socket option is platform dependent and I'm not sure how we can reliably integrate this into this library. This seems to be Linux only(?) and FreeBSD (and Mac?) seems to use LOCAL_PEERCRED instead? This also required ext-sockets, which may or may not be available.

As an alternative, how about we expose the stream resource in way so that downstream packages can use this to manually call this in their code? This can be as simple as this pseudo code:

$connector->connect('unix://server.sock')->then(function (ConnectionInterface $conn) {
    if (isset($conn->stream) && function_exists('stream_socket_import')) {
        $socket = stream_socket_import($conn->stream);
        $pid = socket_get_option($socket, SOL_SOCKET, SO_PEERCRED));
        echo 'Connected to PID ' . $pid . PHP_EOL;
    }
    $conn->close();
});

[gdejong]

Fair points, indeed interoperability could be a problem.

I had actually already used your alternative. Currently that works well.

The Connection class currently has a public $stream; marked as internal. If this ever is made private, that alternative no longer works. How do you feel about either making a getStream() method or removing the @internal? (so it is more explicit that the stream can be used outside the Connection class itself?)