ssh: add ability to enable compression (for SCP/SFTP) #1735

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

vszakats commented Aug 5, 2017

Add the ability to enable compression for SCP/SFTP.

The required low-level logic was already available as part of libssh2 (via LIBSSH2_FLAG_COMPRESS libssh2_session_flag() option.)

This patch adds the new libcurl option CURLOPT_SSH_COMPRESSION (boolean) and the new curl command-line option --compressed-ssh to request this libssh2 feature. To have compression enabled, it is required that the SSH server supports a (zlib) compatible compression method and that libssh2 was built with zlib support enabled.

Ref: #1732


Use case: When transferring well-compressible (f.e. plaintext) data which is either large and/or the bandwidth is relatively low. For small transfers and/or fast connections, it may even result in slightly worse performance. See also: https://askubuntu.com/questions/6770/does-ssh-use-any-compression

@vszakats, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Lekensteyn, @bagder and @yangtse to be potential reviewers.

@vszakats vszakats added the SCP/SFTP label Aug 5, 2017

@vszakats vszakats changed the title from add ability to enable SSH compression to add ability to enable SSH compression (for SCP/SFTP) Aug 5, 2017

@vszakats vszakats changed the title from add ability to enable SSH compression (for SCP/SFTP) to ssh: add ability to enable compression (for SCP/SFTP) Aug 5, 2017

@vszakats vszakats requested a review from jay Aug 5, 2017

Contributor

vszakats commented Aug 5, 2017

I've created the patch assuming it would make it only into 7.56.0. That is to say I'd be quite glad if it could be included in 7.55.0 next week. Given, of course that all eyes find it alright.

@vszakats vszakats deleted a comment from coveralls Aug 5, 2017

@vszakats vszakats deleted a comment from coveralls Aug 5, 2017

Owner

bagder commented Aug 5, 2017

We only merge features when the feature window is open. That's 4 weeks following a release, so too late for 7.55.0

Contributor

vszakats commented Aug 5, 2017

Ok, it's clearly too late for that, thanks for clarifying!

vszakats added a commit to vszakats/harbour-core that referenced this pull request Aug 9, 2017

2017-08-09 09:23 UTC Viktor Szakats (vszakats users.noreply.github.com)
  * contrib/hbcurl/easy.c
  * contrib/hbcurl/hbcurl.ch
    + add tentative support for CURLOPT_SSH_COMPRESSION when built
      against upper than libcurl 7.55.0
      curl/curl#1735

vszakats added a commit to vszakats/curl-for-win that referenced this pull request Aug 9, 2017

@vszakats vszakats removed the request for review from jay Aug 9, 2017

Contributor

vszakats commented Aug 9, 2017

My real world tests were conclusively positive. Here are test binaries (for Windows) with this patch applied including a libssh2 library with zlib enabled: https://bintray.com/vszakats/generic/curl-test/7.55.1

(NOTE: these binaries are not for production use or distribution, their sole purpose is testing this specific feature on that platform.)

Contributor

vszakats commented Aug 15, 2017

Any suggestions or anything missing to have this approved?

bagder approved these changes Aug 15, 2017

Looks great!

src/tool_operate.c
@@ -1091,6 +1091,10 @@ static CURLcode operate_do(struct GlobalConfig *global,
to fail if we are not talking to who we think we should */
my_setopt_str(curl, CURLOPT_SSH_HOST_PUBLIC_KEY_MD5,
config->hostpubmd5);
+
+ /* new in libcurl 7.56.0 */
+ my_setopt_str(curl, CURLOPT_SSH_COMPRESSION,
@jay

jay Aug 16, 2017

Owner

this should be my_setopt and you have to pass it a long like if(config->ssh_compression) my_setopt(curl, CURLOPT_SSH_COMPRESSION, 1L). Also the example should use 1L

@vszakats

vszakats Aug 16, 2017

Contributor

Thanks!

@vszakats

vszakats Aug 16, 2017

Contributor

Fixed both.

+SFTP retrieval
+ </name>
+ <command>
+--key curl_client_key --pubkey curl_client_key.pub -u %USER: --compressed-ssh sftp://%HOSTIP:%SSHPORT%PWD/log/file642.txt --insecure
@jay

jay Aug 16, 2017

Owner

the test doesn't do anything to see if the compression is enabled, is that right?

@vszakats

vszakats Aug 16, 2017

Contributor

That's right. It tests if download is still (transparently) working.

@vszakats

vszakats Aug 16, 2017

Contributor

I've found no obvious way to verify if compression is indeed used in a libcurl transfer. It also requires support from the server-side, not sure if the test server supports it. For local, manual tests, I was timing the transfers and watching the process with a network monitor.

ssh: add the ability to enable compression (for SCP/SFTP)
The required low-level logic was already available as part of
`libssh2` (via `LIBSSH2_FLAG_COMPRESS` `libssh2_session_flag()`[1]
option.)

This patch adds the new `libcurl` option `CURLOPT_SSH_COMPRESSION`
(boolean) and the new `curl` command-line option `--compressed-ssh`
to request this `libssh2` feature. To have compression enabled, it
is required that the SSH server supports a (zlib) compatible
compression method and that `libssh2` was built with `zlib` support
enabled.

[1] https://www.libssh2.org/libssh2_session_flag.html
Ref: #1732
Contributor

vszakats commented Aug 16, 2017

Squashed and rebased on current master.

Contributor

vszakats commented Aug 16, 2017

Slightly off, but the Travis CI queue may be eased a bit by enabling both 'Auto cancel' options on https://travis-ci.org/curl/curl/settings. This would drop any pending commits from the queue if a newer commit lands.

There is a similar option on AppVeyor CI, called 'Rolling builds', which could be enabled for a similar effect, but in this case, by default it will even cancel running builds on the same branch on any new commit (this can be disabled to match Travis CI.)

With these set, there would be less strain on the CI servers and the queue could finish faster.

What do you think about it?

Coverage Status

Coverage increased (+0.01%) to 73.158% when pulling fa7b3a1 on vszakats:ssh_compress into 870d849 on curl:master.

@vszakats vszakats deleted a comment from coveralls Aug 16, 2017

@jay jay closed this in b7b4dc0 Aug 17, 2017

Owner

jay commented Aug 17, 2017

Thanks. I landed this with a slight change so that the error message will be shown even if libssh2_session_flag isn't available. Also note infof should end in a linefeed

  if(data->set.ssh_compression) {
#if LIBSSH2_VERSION_NUM >= 0x010208
    if(libssh2_session_flag(ssh->ssh_session, LIBSSH2_FLAG_COMPRESS, 1) < 0)
#endif
      infof(data, "Failed to enable compression for ssh session\n");
  }
Contributor

vszakats commented Aug 17, 2017

Good idea, thank you @jay! (Noted the infof() issue – what happened is that I copied that line from the preceding failf() which doesn't need the \n)

@vszakats vszakats deleted the vszakats:ssh_compress branch Aug 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment