Make WordPress Core

Opened 13 years ago

Closed 7 years ago

#16925 closed defect (bug) (maybelater)

Move the WP_Filesystem_SSH2 class to a plugin

Reported by: dd32's profile dd32 Owned by: dd32's profile dd32
Milestone: Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: needs-patch
Focuses: Cc:

Description

I'd like to consider moving the WP_Filesystem_SSH2 class out of WordPress core and into a plugin.

The reasoning for this is simply due to the fact that the majority of users will not use it, It already requires the PHP extension to basically be custom-compiled (thanks to limited installations coming with the extension).

In my opinion, This would be better served in a plugin, potentially with a PHP-based version as well: #10348

Change History (16)

#1 @dd32
13 years ago

  • Keywords needs-patch added; 2nd-opinion removed
  • Owner set to dd32
  • Status changed from new to accepted

See Also: blazerlocal's comment which uses a PHP-based library to add SSH2 support

This is something I'm planning on working on before 3.3's release hopefully.

#2 @jnewmano
13 years ago

Why would you want to remove perfectly good code and take on the burden of trying to maintain a complicated plugin? The php ssh2 extension is several thousands of lines of c code and you want to re-implement and maintain it in php?

#3 @dd32
13 years ago

The idea wouldn't be to replace the usage of the PHP extension.

  • Move the SSH2 layers out of WordPress into a separate plugin (As it's a rarely used layer at present, and requires special configuration)
  • Support the PHP Extension if installed (Remembering, that this requires the extension to be custom compiled)
  • If the PHP Extension is not installed (as is the case with many environments, Not to mention the existing class is incompatible with the latest Extension version I believe) then fall back to using a PHP Implementation.

My thought is that this is better off in a community developed plugin, separate from WordPress release cycle, not only would it increase the visibility of the fact of a SSH2 layer existing (many people search, and can't work it out) as a plugin, but it would hopefully increase the number of people willing to help out on the development.

FYI, WordPress currently uses a 3rd party library to implement FTP in the cases where the PHP FTP extension is not available (A large chunk of installs)

#4 @knuthmorris
13 years ago

@jnewmano: It's already been done. And the plugin itself is pretty straight forward. The complicated part was creating a pure-PHP implementation of SSH / SFTP and that's already been done, too. blazerlocal elaborates why the php ssh2 extension is deficient:

http://core.trac.wordpress.org/ticket/10348#comment:19

In addition to the reasons blazerlocal elaborated upon phpseclib is also, quite simply, more feature packed. It can capture logs for you, lets you resume uploads, has its own implementation of expect, supporting both literal string matching and regular expression string matching, etc.

And let's not forget the biggest advantage of phpseclib: it works out of the box. And it's been working out of the box for four years.

Last edited 13 years ago by knuthmorris (previous) (diff)

#5 @knuthmorris
13 years ago

Also, honestly, I wouldn't even bother with implementing php ssh2 support. What do you get out of it that phpseclib doesn't provide? phpseclib works 100% of the time. php ssh2 works, what, 10% of the time?

Supporting php ssh2 makes about as much since as making sure WordPress themes work with Internet Explorer 4.0. You know - for that 0% of people that use it.

#6 follow-up: @dd32
13 years ago

Also, honestly, I wouldn't even bother with implementing php ssh2 support

Point is, It's already supported and the code written, as well as performing the SSH routines in PHP is bound to be slower than those implemented in C.

#7 in reply to: ↑ 6 @knuthmorris
13 years ago

Replying to dd32:

Also, honestly, I wouldn't even bother with implementing php ssh2 support

Point is, It's already supported and the code written, as well as performing the SSH routines in PHP is bound to be slower than those implemented in C.

Not according to this:

http://kevin.vanzonneveld.net/techblog/article/make_ssh_connections_with_php/#comment_3759

But hey - that post is over a year old so let's try it again, shall we? PHP 5.3.6, the version of phpseclib that's included in the Wordpress plugin, and 0.11.2 of php ssh. Here's what I got on three separate trials using the script in the above link:

Trial #1:
took 5.3246591091156 seconds
took 7.8901920318604 seconds

Trial #2:
took 5.2026791572571 seconds
took 8.3022320270538 seconds

Trial #3:
took 5.1897370815277 seconds
took 7.1662919521332 seconds

phpseclib consistently beats php ssh2 hands down. And there's a fairly wide margin too.

99% of the time the bottle neck in SSH is in how you're uploading data. The values you're using for the window size, the SFTP packet size, how many write packets you send before reading the read packets, etc. And programming in C as opposed to PHP isn't going to change that.

Maybe you're thinking that the bottle neck in PHP is the encryption. Encryption that phpseclib already offloads to mcrypt, if mcrypt is installed. mcrypt being a C extension. And for stuff like diffie-hellman key exchange phpseclib offloads to gmp and then to bcmath if either of those are installed.

Last edited 13 years ago by knuthmorris (previous) (diff)

#8 @knuthmorris
13 years ago

Oh - and let's try phpseclib without either gmp or bcmath. Doing so will require a slight modification to urdd's script:

<?php
include('Net/SFTP.php');
define('MATH_BIGINTEGER_MODE', MATH_BIGINTEGER_MODE_INTERNAL);
$sftp = new Net_SFTP('www.domain.tld', 22);
$sftp->login('user', 'pass');
$start = microtime(true);
$sftp->put('test', str_repeat('a', 256 * 1024));
$elapsed = microtime(true) - $start;
echo "took $elapsed seconds\r\n";
 
 
$connection = ssh2_connect('www.domain.tld', 22);
ssh2_auth_password($connection, 'user', 'pass');
$sftp = ssh2_sftp($connection);
$start = microtime(true);
$stream = fopen("ssh2.sftp://$sftp/home/user/test", 'w');
fputs($stream, str_repeat('a', 256 * 1024));
fclose($stream);
$elapsed = microtime(true) - $start;
echo "took $elapsed seconds\r\n";
?>

See the define('MATH_BIGINTEGER_MODE', MATH_BIGINTEGER_MODE_INTERNAL)? That's new. And here's what we get with that:

Trial #1:
took 6.8606338500977 seconds
took 7.5764989852905 seconds

Trial #2:
took 5.2222318649292 seconds
took 7.167053937912 seconds

Trial #3:
took 5.325345993042 seconds
took 7.7823340892792 seconds

Oh snap. phpseclib owns again.

Let's try it without mcrypt or gmp or bcmath. I ensure that mcrypt is not used by doing define('CRYPT_RC4_MODE', CRYPT_RC4_MODE_INTERNAL). Pure-PHP for everything.

Trial #1:
took 5.2221701145172 seconds
took 7.4742610454559 seconds

Trial #2:
took 5.2241580486298 seconds
took 7.8221480846405 seconds

Trial #3:
took 5.2212400436401 seconds
took 7.8840770721436 seconds

phpseclib wins again.

But just in case you believe I'm cooking the books and you're too lazy to verify the results for yourself, here's proof that the bottleneck is how the network packets are handled.

<?php
include('Crypt/RC4.php');
include('Crypt/Random.php');

define('CRYPT_RC4_MODE', CRYPT_RC4_MODE_INTERNAL);

$rc4 = new Crypt_RC4();

$key = '';
for ($i = 0; $i < 16; $i++) {
    $key.= pack('n', crypt_random(0, 0xFFFF));
}

$rc4->setKey($key);
$start = microtime(true);
$rc4->encrypt(str_repeat("\0", 1536));
$rc4->encrypt(str_repeat('a', 1024 * 1024));
$elapsed = microtime(true) - $start;
echo "took $elapsed seconds\r\n";

phpseclib uses arcfour256 by default hence my using Crypt_RC4 as well. str_repeat("\0", 1536) is encrypted because that's apparently what arcfour256 requires, per the source code comments in phpseclib. Also, note how we're encrypting 1MB instead of the 256KB that we're uploading via SFTP. And here are the times using phpseclib's pure-PHP RC4 implementation:

Trial #1:
took 2.6884229183197 seconds

Trial #2:
took 2.76646900177 seconds

Trial #3:
took 2.8742060661316 seconds

So, basically, it takes less time to encrypt 1MB then it takes to upload 256KB. The bottleneck in SFTP isn't solved by simply using C instead of PHP and phpseclib IS faster.

Don't believe me? Try it for yourself. I'm not including the source code to make my post bigger. I'm including it so you can verify my results instead of pulling random - and false - factoids out of thin air.

Last edited 13 years ago by knuthmorris (previous) (diff)

#9 @dd32
13 years ago

Don't believe me?

I think you've managed to prove my initial thinking wrong :)

Are you the author of that plugin? If so, Please submit it to the WordPress.org Extend Plugin directory and I'd be happy to suggest users use it, as well a give it a closer inspection/testing (another set of eyes is always useful).

The name of the plugin might be better served differently however, "phpseclib SFTP Updater" might not be found as easily as many others would expect, renaming it to "SSH SFTP Updater Support" for example might standout better (just food for thought) as well as be found via searches better (face it, most people call it SSH rather than SFTP anyway)

Version 0, edited 13 years ago by dd32 (next)

#10 @knuthmorris
13 years ago

I am indeed the author. I was kinda trying to conceal my identity lol. Figured a testimonial from a third party might carry more weight than a testimonial from the author of said plugin - that people might dismiss what I - the author - was saying simply because I am the author. I guess my break down of the code kinda gave my identity away. Well that and maybe an overly passionate response lol.

Anyway, I do think the alternative name you've proposed is better. I'll rename it tomorrow - it's kinda late in my tz and I figure I'll go to bed after this post.

I also did submit it to the WordPress.org Extend Plugin directory under the original name:

http://wordpress.org/extend/plugins/phpseclib-sftp-updater/

Should I resubmit? I gather it can take a while for things to get approved? I've written one other WordPress plugin but that was before there was an Extend Plugin directory.

#11 @dd32
13 years ago

I am indeed the author. I was kinda trying to conceal my identity lol.

Considering the plugin was released yesterday, kinda obvious :)

I also did submit it to the WordPress.org Extend Plugin directory

Approvals are done manully, being a long weekend in the US where most of the directory "managers" are, things will be delayed a bit over the weekend. I believe if you log in to http://wordpress.org/extend/plugins/about/ you can cancel the request, and add a new request with the other name if you wish. The initial name you request is what the plugin slug becomes, once it's approved you can modify the name freely..

Feel free to email me directly: wordpress [at ] dd32.id.au if you've got any questions/etc that isnt appropriate for trac, once you've got a copy on wordpress.org theres a few changes/cleanups i'm going to suggest to the code in the form of a patch :)

#12 @nacin
13 years ago

I've declined the phpseclib-sftp-updater plugin. Resubmit it under a new name and post a comment here when you've done so, and one of us will approve it.

#14 follow-up: @chriscct7
9 years ago

  • Keywords reporter-feedback added

It seems this issue was resolved with the plugin or am I missing something.

@dd32 can you clarify?

#15 in reply to: ↑ 14 @kitchin
9 years ago

The bug is to remove WP_Filesystem_SSH2.

Replying to chriscct7:

It seems this issue was resolved with the plugin or am I missing something.

@dd32 can you clarify?

#16 @dd32
7 years ago

  • Keywords reporter-feedback removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from accepted to closed

Closing this as maybelater the SSH support in core isn't doing any harm, although I highly recommend users use http://wordpress.org/extend/plugins/ssh-sftp-updater-support/ if they don't already have the SSH extension installed.

Note: See TracTickets for help on using tickets.