KnpLabs / Gaufrette

PHP library that provides a filesystem abstraction layer − will be a feast for your files!
http://knplabs.github.io/Gaufrette
MIT License
2.47k stars 355 forks source link

Phpseclib SFTP adapter isn't lazy loading #405

Open rodrigm opened 8 years ago

rodrigm commented 8 years ago

I started using Phpseclib SFTP adapter with KnpGaufretteBundle. By defining the service in Symfony, this is not lazy loading.

services:
    acme_test.sftp:
        class: phpseclib\Net\SFTP
        arguments: [%acme_test.ssh.host%]
        calls:
            - [login, [%acme_test.ssh.username%, %acme_test.ssh.password%]]
knp_gaufrette:
    adapters:
        foo:
            phpseclib_sftp:
                phpseclib_sftp_id: acme_test.sftp
                directory: /example/sftp

This makes it always have to be a valid for SFTP connection. This is quite complicated in environments like Travis, etc…

First, I thought it was an issue of KnpGaufretteBundle, but if we use this adapter will we fully dependent on a valid SFTP connection.

IMO, the solution would be to create LazyPhpseclibSftp similar Gaufrette/Adapter/LazyOpenCloud.php Adapter.

Can you come to mind of another solution?

A little related to https://github.com/KnpLabs/Gaufrette/pull/404

akerouanton commented 7 years ago

@rodrigm Symfony has a dedicated mechanism for lazy loading services. DId you try it?

It's recommended to use it any time you create a service representing an external connection, like SFTP instances in order to not face the problem you describe (external connections opened for every http requests). What you describe looks like single point of failure, and should be removed.

bartrail commented 7 years ago

I'm having the same issue. when called with a cli command, a connection is actually opened up lazy - meaning when it is used for the first time. but for some reason, now any web request takes another + 2.5 seconds to load. interestingly when we throw an exception in BackupSftpRsaKey we get a 500 return from the server on any request, but the output is just normal. any var_dump(123) is printed normally after any other output. when we remove the gaufrette config + services all requests respond with their normal quick response time.

actually we tried the lazy option because we needed to copy + compress a lot of data before sending with sftp to another server. without lazy the connection was closed when the data transfer should start. with lazy, it works like a charm. I don't get whats happening here.. any advice is highly appreciated.

this is my config:

config.yml:

knp_gaufrette:
    stream_wrapper:
        protocol: gaufrette
    adapters:
        export_sftp_adapter:
            phpseclib_sftp:
                phpseclib_sftp_id: uo.backup.sftp
                directory: /my/backup/dir
                create: true
services:
    uo.backup.sftp.rsa_key:
        lazy: true
        class: UO\Bundle\UtilityBundle\Service\BackupSftpRsaKey
        arguments: [%backup_sftp_private_key%,%backup_sftp_pass%]

    uo.backup.sftp:
        lazy: true
        class: phpseclib\Net\SFTP
        arguments: [%backup_sftp_host%]
        calls:
            - [login, [%backup_sftp_user%, "@=service('uo.backup.sftp.rsa_key').getRSA()"]]

my uo.backup.sftp.rsa_key class looks like this:

<?php
namespace UO\Bundle\UtilityBundle\Service;

use phpseclib\Crypt\RSA;

/**
 * Class BackupSftpRsaKey
 *
 * @package UO\Bundle\UtilityBundle\Service
 */
class BackupSftpRsaKey
{
    /**
     * @var RSA
     */
    private $rsa;

    /**
     * BackupSftpRsaKey constructor.
     *
     * @param String $keyFile
     * @param String|null $password
     */
    public function __construct(String $keyFile, String $password = null)
    {
        $this->rsa = new RSA();
        if (is_string($password)) {
            $this->rsa->setPassword($password);
        }
        $this->rsa->loadKey(file_get_contents($keyFile));
    }

    /**
     * @return RSA
     */
    public function getRSA()
    {
        return $this->rsa;
    }
}
michcald commented 7 years ago

@NiR- I have the same issue. I have tried defining the service lazy but had the same outcome.

l0wskilled commented 5 years ago

If you call a method lazy wont work. You could wrap it into a singleton which creates your instance and calls login to avoid this.

Remove and you'll see lazy works:

 calls:
            - [login, [%backup_sftp_user%, "@=service('uo.backup.sftp.rsa_key').getRSA()"]]
florianajir commented 4 years ago

Like @rodrigm I followed the doc https://github.com/KnpLabs/KnpGaufretteBundle/blob/master/Resources/docs/adapters/phpseclib_sftp.md and it define a call to login on instanciation.

please tell us how do you "wrap it into singleton" when gaufrette config accept only phpseclib\Net\SFTP service instance ?

l0wskilled commented 4 years ago

It's not a singleton, i just lazily called it that. I have a service with a public method that creates a new instance of the sftp. That instance only lives while i use it, as i never safe it in a variable. It looks like this.

I. e. im creating a new instance for each transaction, as the amount of things we upload/download ended up in the instance timing out or something and ssh connections were kept up in a weird state. It's kinda hacky though.

use Gaufrette\Adapter\PhpseclibSftp;
use Gaufrette\Filesystem;
use phpseclib\Net\SFTP;

/**
 * Class FilesystemFactory
 *
 * @package MainBundle\Factory
 */
class FilesystemFactory
{
    /**
     * @var string
     */
    protected $host;

    /**
     * @var string
     */
    protected $user;

    /**
     * @var string
     */
    protected $password;

    /**
     * @var string
     */
    protected $directory;

    /**
     * Factory constructor.
     *
     * @param $host
     * @param $user
     * @param $password
     * @param $directory
     */
    public function __construct(
        $host,
        $user,
        $password,
        $directory
    ) {
        $this->host = $host;
        $this->user = $user;
        $this->password = $password;
        $this->directory = $directory;
    }

    /**
     * Returns the filesystem
     *
     * @return Filesystem
     */
    public function getFilesystem()
    {
        return $this->initFilesystem();
    }

    /**
     * Initializes Filesystem and returns it
     *
     * @return Filesystem
     */
    protected function initFilesystem()
    {
        return new Filesystem($this->initAdapter());
    }

    /**
     * Initializes PhpseclibSftp Adapter and returns it
     *
     * @param bool $create if the directory should be created if necessary
     * @return PhpseclibSftp
     */
    protected function initAdapter($create = true)
    {
        return new PhpseclibSftp($this->initSFTP(), $this->directory, $create);
    }

    /**
     * Initializes SFTP and returns it
     *
     * @return SFTP
     * @throws \RuntimeException
     */
    protected function initSFTP()
    {
        $sftp = new SFTP($this->host);
        $loggedIn = $sftp->login($this->user, $this->password);
        if (!$loggedIn) {
            throw new \RuntimeException(
                sprintf(
                    "%s connection failed!",
                    $this->host
                )
            );
        }
        return $sftp;
    }
}
florianajir commented 4 years ago

Ok but I think this can't works with symfony bundle integration because of the gaufrette-bundle config (adapters part) need the injection of a service implementing logged-in phpseclib instance

i.e:

knp_gaufrette:
    adapters:
        legacy:
            phpseclib_sftp:
                phpseclib_sftp_id: legacy.sftp
                directory: '%env(LEGACY_SSH_DIRECTORY)%'
                create: false
    filesystems:
        legacy: # remote://legacy/
            adapter: legacy
    stream_wrapper:
        protocol: remote

services:
    legacy.sftp:
        class: phpseclib\Net\SFTP
        arguments: ['%legacy.ssh.host%']
        calls:
            - [login, ['%legacy.ssh.username%', '%legacy.ssh.password%']]
pdevillard commented 4 years ago

Well, i'm not proud of what I'm posting but this solved the situation for me. It was quite urgent since our hosting provider got quite disturbed after a DDOS attack. The server sent millions of connections. So this is what I did, this solution could help someone in deep help. But I would suggest to move to something more elegant for production...

The idea is basically to call twice login to actually log in.

<?php

namespace App\Gaufrette;

use phpseclib\Net\SFTP as BaseSftp;

class Sftp extends BaseSftp
{
    const STATE_INIT  = 1;
    const STATE_LOAD  = 2;
    const STATE_READY = 3;

    /**
     * @var string
     */
    private $state = self::STATE_INIT;

    /**
     * @var int
     */
    private $credentials = null;

    /**
     * @inheritdoc
     */
    public function login($username)
    {
        // First time is called in on the service loading
        if (self::STATE_INIT === $this->state) {
            $this->credentials = func_get_args();
            $this->state = self::STATE_LOAD;
            return;
        }

        // We are now ready
        $this->state = self::STATE_READY;

        return call_user_func_array(['parent', 'login'], func_get_args());
    }

    /**
     * @inheritdoc
     */
    public function pwd()
    {
        // If the service is ready, we act normally
        if (self::STATE_READY === $this->state) {
            return parent::pwd();
        }

        // Otherwise, it means that gaufrette is making the first move with "ensure directory exists", calling pwd...
        // This is where we will actually login
        call_user_func_array([$this, 'login'], $this->credentials);

        return $this->pwd();
    }
}

Then use this class in the services. No other changes are required from the standard procedure.

services:
    acme_test.sftp:
        class: App\Gaufrette\Sftp
        arguments: [%acme_test.ssh.host%]
        calls:
            - [login, [%acme_test.ssh.username%, %acme_test.ssh.password%]]
florianajir commented 4 years ago

Smart workaround @biliboo I avoided the problem migrating to https://github.com/thephpleague/flysystem-bundle 😅