-1
\$\begingroup\$

I've written a simple PHP SSH2 wrapper class. Please review the code and point out mistakes.

class Ssh 
{
    private $sshHandler = false;
    public function __construct ()
    {
        if (!function_exists('ssh2_connect'))
        {
            throw new Exception("Server doesn't have SSH2 extension!");
        }
    }

    public function connect ($host = '127.0.0.1', $port = 22, $methods = null, $callbacks = null)
    {
        $this->sshHandler = ssh2_connect($host, $port, $methods, $callbacks);
        if($this->sshHandler == false)
            return false;
        return true;
    }

    public function isConnected ()
    {
        return $this->sshHandler != false ? true : false;
    }

    public function auth ($method, $data)
    {
        if (!isset($method) || $this->sshHandler == false)
            return false;

        switch($method)
        {
            case 'password':
                if (!isset($data))
                    return false;
                if (!isset($data['username']) || !isset($data['password']))
                    return false;
                if(ssh2_auth_password($this->sshHandler, $data['username'], $data['password']) == true)
                    return true;
                return false;
            break;
            case 'auth_agent':
                if (!isset($data))
                    return false;
                if (!isset($data['username']))
                    return false;
                if(ssh2_auth_agent($this->sshHandler, $data['username']) == true)
                    return true;
                return false;
            break;
            case 'hostbased_file':
                if (!isset($data))
                    return false;
                if (!isset($data['username']) || !isset($data['hostname']) || !isset($data['pubkeyfile']) || !isset($data['privkeyfile']))
                    return false;
                if (!isset($data['passphrase']))
                    $data['passphrase'] = null;
                if (!isset($data['local_username']))
                    $data['local_username'] = null;
                if(ssh2_auth_hostbased_file($this->sshHandler, $data['username'], $data['hostname'], $data['pubkeyfile'], $data['privkeyfile'], $data['passphrase'], $data['local_username']) == true)
                    return true;
                return false;
            break;
            case 'none':
                if (!isset($data))
                    return false;
                if (!isset($data['username']))
                    return false;
                return ssh2_auth_none($this->sshHandler, $data['username']);
            break;
            case 'pubkey_file':
                if (!isset($data))
                    return false;
                if (!isset($data['username']) || !isset($data['pubkeyfile']) || !isset($data['privkeyfile']))
                    return false;
                if (!isset($data['passphrase']))
                    $data['passphrase'] = null;
                if(ssh2_auth_pubkey_file($this->sshSession, $data['username'], $data['pubkeyfile'], $data['privkeyfile'], $data['passphrase']) == true)
                    return true;
                return false;
            break;
        }
    }

    public function executeCommand ($command, $returnString = true)
    {
        if (!isset($command) || $this->sshHandler == false)
            return false;
        $stream = ssh2_exec($this->sshHandler, $command);
        if (is_resource($stream) == false)
            return false;

        if ($returnString == false)
            return $stream;

        stream_set_blocking($stream, true);
        $streamOut = ssh2_fetch_stream($stream, SSH2_STREAM_STDIO);

        return stream_get_contents($stream);
    }

    public function getServerFingerprint ($flags = SSH2_FINGERPRINT_MD5 | SSH2_FINGERPRINT_HEX)
    {
        if ($this->sshHandler == false)
            return false;
        return ssh2_fingerprint($this->sshHandler);
    }

    public function getNegotiatedMethods ()
    {
        if ($this->sshHandler == false)
            return false;
        return ssh2_methods_negotiated($this->sshHandler);
    }

    public function disconnect ()
    {
        fclose($this->sshHandler);
        $this->sshHandler = false;
        return true;
    }
    public function getHandler ()
    {
        return $this->sshHandler;
    }
}
\$\endgroup\$
0
\$\begingroup\$

Not sure about the overall utility of this class, in that it limits some of the functionalities of the underlying SSH2 functions.

Let's look at the executeCommand() method for example. You have now restricted the ability to do things like pass in variables to be set on the environment. You also do nothing there to monitor STDERR stream.

If your main intent is to manage the way in which connections are made (i.e. authentication and such), then perhaps this class might be sensibly refactored into a factory class that can take inputs for host, auth settings, environmental variables, callbacks, etc. and simply return an object of another class representing the SSH resource with appropriately set-up mechanisms for handling STDOUT and STDERR streams which would be suitable for passing around as a dependency to code needing SSH to that particular target server.

Your concrete class could be similar to what you have now, allowing for various ssh2_* operations against the resource and providing proper error/response handling around the executed commands. Consider naming something like Ssh2 to clearly indicate you are working with underlying ssh2_* functions.

Example usage with this approach:

try {
    $mySsh = Ssh2Provider::getSshConnection(...);
    $foo = new SomeClassNeedingSsh($mySsh);
} catch (Exception $e) {
   ...
}

// inside SomeClassNeedingSsh
public function __construct(Ssh2 $ssh) {
    // maybe passed object to property
    $this->ssh2 = $ssh;

    // do some stuff
    $this->ssh2->executeCommand(...);
}

For something as important as a remote server connection, I would want to make sure that you have covered the appropriate magic methods such as __destruct() and __clone() to make sure you have enforced behaviors to close connections when the object is de-referenced, prevent attempted object cloning, etc.


Your auth method would probably be best broken up to several auth_password and similar methods. You can easily eliminate your switch statement here, and when you think about it, it doesn't make logical sense to have this sort of dynamic branching at run time, as you will always exactly only one of these methods for any given connection. You could still perhaps have a general auth method that takes authentication type parameter, but this method would then just hand off to the selected method for performing authentication.


Throughout your code, you seem to default to loose comparisons (==, et al). I would like to see you get in the habit of using strict comparisons (===, et al) as default comparison approach, only using loose comparisons when there is some special reason to do so (for example, you TRULY want to consider empty arrays, empty strings, etc. as the same). I think over time this leads to code that is less buggy and easier to debug when you do need to debug. Loose comparisons are always top candidates to introduce unexpected behavior in your code.


Perhaps consider renaming: $sshHandler => $sshResource


public function __construct ()
{
    if (!function_exists('ssh2_connect'))
    {
        throw new Exception("Server doesn't have SSH2 extension!");
    }
}

Does it even make sense to have a public constructor for this class? Why would one ever want to instantiate an object of this class without having appropriate settings for host, port, auth, etc? You could perhaps end up with half-set-up object being passed around your system.

If you want to have public constructor instantiation, consider removing the connect() method and making sure the dependencies for that method are passed to constructor and that the constructor actually performs the connection successfully.

Also, I would think you should have a failure somewhere MUCH earlier in your call stack that a core dependency like ssh2_* functions is not being met. To me, this might be more of an application set-up concern rather than something deferred until object instantiation. If you are on PHP 5.6+ you might consider placing use ssh2_connect before your class definition to both make clear the dependency and to enforce that this dependency exists at compile time.


public function connect ($host = '127.0.0.1', $port = 22, $methods = null, $callbacks = null)
{
    $this->sshHandler = ssh2_connect($host, $port, $methods, $callbacks);
    if($this->sshHandler == false)
        return false;
    return true;
}

You are not doing ANY validation or type-hinting related to the parameters being passed here. For public functions, you should typically validate passed parameters for at a minimum, things like data types, but possibly also to make sure the dependencies make sense to work with. Here for example, you could try a DNS lookup against the passed $host variable to see if that host is even reachable from the server the code is running on.

When validation fails, you should throw an Exception or otherwise give caller feedback as to the failure.

Also in this connect() method, I wonder whether these default values should be specified here in the method signature rather than in class constants or properties. Perhaps your method signature should be something like this:

public function connect (
    $host = Ssh::DEFAULT_HOST,
    $port = Ssh::DEFAULT_PORT,
    array $methods = Ssh::defaultConnectMethods,
    array $callbacks = Ssh::defaultCallbacks
) {

You have a few cases where your lines of code are getting to be pretty long. You should strive to keep code under 80 characters per line, breaking up code across lines as necessary. This helps make your code easier to read.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the tips, i will use them in the next class \$\endgroup\$ – Dante383 Nov 2 '16 at 15:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.