Allow SFTP to be used for upload!/download! instead of SCP #524

mattbrictson · 2023-12-22T18:08:14Z

SSHKit can now use SFTP for file transfers. The default is still SCP, but this can be changed to SFTP per host:

host = SSHKit::Host.new('user@example.com')
host.transfer_method = :sftp

or globally:

SSHKit::Backend::Netssh.configure do |ssh|
  ssh.transfer_method = :sftp
end

Fixes #15

mattbrictson · 2023-12-22T18:10:16Z

@JasonPoll this PR is still a work in progress, but it should be ready for you to test. Let me know if this branch works with SFTP in your environment. Also would appreciate any suggestions/feedback on the implementation. Thanks!

This will make it easier to swap in SFTP in a later commit.

will-in-wi

Code looks reasonable to me. Haven't tried it (I don't tend to use these methods…).

mattbrictson · 2023-12-27T21:43:53Z

lib/sshkit/backends/netssh.rb

@@ -2,7 +2,6 @@
 require 'strscan'
 require 'mutex_m'
 require 'net/ssh'
-require 'net/scp'


🗒️ I moved this to the scp_transfer.rb strategy, so that net/scp is only loaded if that strategy is being used.

mattbrictson · 2023-12-27T21:44:30Z

lib/sshkit/backends/netssh.rb

        attr_writer :ssh_options

+        def initialize
+          self.transfer_method = :scp


🗒️ In a future release, we can consider changing the default to :sftp (which would be a breaking change).

mattbrictson · 2023-12-27T21:47:49Z

lib/sshkit/backends/netssh.rb

-        with_ssh do |ssh|
-          ssh.scp.upload!(local, remote, options, &summarizer)
+        with_transfer(summarizer) do |transfer|
+          transfer.upload!(local, remote, options)


🗒️ My goal with these changes was to leave upload!, download! and transfer_summarizer code unmodified as much as possible. The difference is that rather than using ssh.scp directly, the code now uses a transfer strategy provided by the new with_transfer helper. The transfer strategy uses scp or sftp based on configuration.

In other words, the Netssh class stays mostly the same, but the upload/download implementation can be swapped between scp and sftp.

As someone who works in a 10+ year old code base as part of my day-job, I appreciate the approach you've taken here -- minimal changes to the long-established core of the codebase, refactoring previous functionality into a sort of configuration-based 'plugin' design, and adding new functionality with that same plugin functionality, all the while retaining backwards compatibility.

💯, good sir.

mattbrictson · 2023-12-27T21:48:22Z

lib/sshkit/backends/netssh.rb

@@ -105,7 +121,7 @@ def transfer_summarizer(action, options = {})
        last_percentage = nil
        proc do |_ch, name, transferred, total|
          percentage = (transferred.to_f * 100 / total.to_f)
-          unless percentage.nan?
+          unless percentage.nan? || percentage.infinite?


🗒️ To be safe, I added this check to guard against divide-by-zero.

mattbrictson · 2023-12-27T21:49:22Z

lib/sshkit/backends/netssh.rb

@@ -183,6 +199,20 @@ def with_ssh(&block)
        )
      end

+      def with_transfer(summarizer)


🗒️ This helper checks whether scp or sftp is configured (it can be specified globally or overridden per host), and instantiates and yields the appropriate transfer strategy class.

mattbrictson · 2023-12-27T21:54:47Z

lib/sshkit/backends/netssh/sftp_transfer.rb

+          ssh.sftp.connect!
+          ssh.sftp.upload!(local, remote, options)
+        ensure
+          ssh.sftp.close_channel


🗒️ We have to close the sftp channel when we are done. Otherwise subsequent ssh commands will hang.

mattbrictson · 2023-12-27T21:58:38Z

lib/sshkit/backends/netssh/sftp_transfer.rb

+
+        def download!(remote, local, options)
+          options = { progress: self }.merge(options || {})
+          destination = local ? local : StringIO.new.tap { |io| io.set_encoding('BINARY') }


🗒️ This workaround is due to a surprising difference between net-scp and net-sftp. In net-scp, calling download! without an explicit local path will return the download bytes as a binary-encoded string object.

However, net-sftp returns the string object as UTF-8 encoded. That means that binary files will be corrupted.

Work around this limitation by explicitly specifying a binary encoded StringIO object, to make net-sftp be consistent with the behavior of net-scp.

mattbrictson · 2023-12-27T22:03:09Z

lib/sshkit/backends/netssh/sftp_transfer.rb

+        def on_get(download, entry, offset, data)
+          entry.size ||= download.sftp.file.open(entry.remote, &:size)
+          summarizer.call(nil, entry.remote, offset + data.bytesize, entry.size)
+        end


🗒️ net-sftp calls on_get to provide download progress. This information is much different than how net-scp provides progress. In net-sftp, offset refers to the offset of the local destination, before the bytes have been transferred. So some arithmetic is needed to calculate the progress: offset + data.bytesize.

Furthermore, the total size of the file being transferred, which is supposed to be provided via entry.size, is nil. To work around this, I had to explicitly make an sftp open call to read the file size.

mattbrictson · 2023-12-27T22:04:01Z

lib/sshkit/backends/netssh/sftp_transfer.rb

+        def on_put(_upload, file, offset, data)
+          summarizer.call(nil, file.local, offset + data.bytesize, file.size)
+        end


🗒️ net-sftp calls on_put to provide upload progress. For uploads, file.size seems to be correct, so no workaround is needed.

mattbrictson · 2023-12-27T22:04:41Z

test/functional/backends/netssh_transfer_tests.rb

+
+module SSHKit
+  module Backend
+    module NetsshTransferTests


🗒️ I extracted the upload/download tests into a module, so the same tests can easily be reused for both scp and sftp.

JasonPoll · 2023-12-29T23:10:54Z

lib/sshkit/backends/netssh.rb

@@ -23,10 +22,27 @@ module SSHKit
  module Backend

    class Netssh < Abstract
+      def self.assert_valid_transfer_method!(method)
+        return if [nil, :scp, :sftp].include?(method)


nil?

I suppose it's inconsequential when transfer_method defaults to :scp along with the behaviors of #with_transfer.

@JasonPoll Host#transfer_method is initially set to nil (which means "use the global default"), so I wanted to make sure that nil is an allowable value.

...oh wait...I think I understand. nil is considered a valid value because per-host configuration could be nil and global configuration could be what is specifying :scp or :sftp.

Even in the case when both host and global configuration are assigned a transfer_value of nil, the #with_transfer method still embodies the previous behavior of transfers being performed via SCP.

Maybe a more correct implementation would be to have assert_valid_transfer_method! strictly only allow :scp and :sftp, with a workaround in Host#transfer_method= to allow nil.

# netssh.rb def self.assert_valid_transfer_method!(method) return if [:scp, :sftp].include?(method) raise ArgumentError, "#{method.inspect} is not a valid transfer method. Supported methods are :scp, :sftp." end

# host.rb def transfer_method=(method) Backend::Netssh.assert_valid_transfer_method!(method) unless method.nil? @transfer_method = method end

Addressed in 9bbcc2c

JasonPoll · 2023-12-30T00:19:02Z

EXAMPLES.md

@@ -162,6 +162,23 @@ end
 In this case the `recursive: true` option mirrors the same options which are
 available to [`Net::{SCP,SFTP}`](http://net-ssh.github.io/net-scp/).


confirmed this older documentation about recursive: true is still true with SFTP enabled.

JasonPoll

I performed some functionality tests with a couple VMs, one with SCP enabled, and one without. My tests performed as expected. Detailed copy/pasted notes below.

To keep my notes a bit more succinct, this is quick setup and testing method I used for testing #upload! and #download!:

require 'sshkit'
require 'sshkit/dsl'
include SSHKit::DSL
   
def doit(ip, &block)
 puts "- global transfer method: :#{SSHKit::Backend::Netssh.config.transfer_method}"
   on [ip] do |host|
     yield host if block_given? 
        
     as :vagrant do 
       within '/home/vagrant/tmp' do 
         upload! 'junk.dat', 'junk.dat'
         download! 'junk.dat', 'junk_downloaded.dat'
       end 
     end 
   end 
 end

To further make my notes succinct, redundant lines are eliminated/ellipsed.

With everything set to SSHKit defaults, on 192.168.69.70 (a host with SCP enabled) uploading and downloading via SCP works as expected:

doit('192.168.69.70')

- global transfer method: :scp
  INFO Uploading junk.dat 12.21%
  ...
  INFO Uploading junk.dat 100.0%
  INFO Downloading /home/vagrant/tmp/junk.dat 12.21%
  ...
  INFO Downloading /home/vagrant/tmp/junk.dat 100.0%

With everything set to defaults, on 192.168.69.42 (a host with SCP disabled) it errors as expected when attempting to upload:

doit('192.168.69.42')

- global transfer method: :scp
#<Thread:0x00007fc2d022f1d8 /home/vagrant/code/sshkit/lib/sshkit/runners/parallel.rb:10 run> terminated with exception (report_on_exception is true): 
...
SSHKit::Runner::ExecuteError: Exception while executing on host 192.168.69.42: SCP did not finish successfully (255): 
from /home/vagrant/code/sshkit/lib/sshkit/runners/parallel.rb:15:in `rescue in block (2 levels) in execute'
Caused by Net::SCP::Error: SCP did not finish successfully (255): 
from /home/vagrant/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-scp-4.0.0/lib/net/scp.rb:365:in `block (3 levels) in start_command'

Changing the global transfer_method to :sftp and the file uploads/downloads as expected:

SSHKit::Backend::Netssh.configure{|ssh| ssh.transfer_method = :sftp}
=> :sftp
doit('192.168.69.42')

- global transfer method: :sftp
  INFO Uploading junk.dat 10.94%
  ...
  INFO Uploading junk.dat 100.0%
  INFO Downloading junk_downloaded.dat 10.94%
  ...
  INFO Downloading junk_downloaded.dat 100.0%

Changing the transfer_method to :sftp on a per-host basis and the file uploads/downloads as expected:

doit('192.168.69.42') { |host| host.transfer_method = :sftp }

- global transfer method: :scp
  INFO Uploading junk.dat 10.94%
  ...
  INFO Uploading junk.dat 100.0%
  INFO Downloading junk_downloaded.dat 10.94%
  ...
  INFO Downloading junk_downloaded.dat 100.0%

This looks good to me!

mattbrictson · 2023-12-31T16:30:44Z

Thanks @JasonPoll and @will-in-wi for the reviews! I plan on releasing this as part of sshkit 1.22.0 in mid-January, after 1.21.7 has some time in the wild. In the meantime, you can take advantage of the new sftp functionality by pointing your Gemfile at the master branch:

gem "sshkit", github: "capistrano/sshkit", branch: "master"

mattbrictson added the ✨ Feature Adds a new feature label Dec 22, 2023

mattbrictson added 4 commits December 22, 2023 14:57

Extract SCP logic into separate strategy class

aa2e02b

This will make it easier to swap in SFTP in a later commit.

Guard against divide-by-zero

3015787

Add net-sftp gem dependency

d963bda

Allow :sftp or :scp transfer method to be configured

0d65d88

mattbrictson force-pushed the features/sftp branch from 5d98e4a to 984b374 Compare December 22, 2023 21:05

Implement SFTP transfer strategy

ced903d

mattbrictson force-pushed the features/sftp branch 2 times, most recently from e75d894 to 84f822e Compare December 22, 2023 21:22

mattbrictson added 2 commits December 22, 2023 15:26

Extract upload/download tests into module for reuse

cf985cf

Test SFTP transfer method

0306386

mattbrictson force-pushed the features/sftp branch from 84f822e to 0306386 Compare December 22, 2023 21:27

will-in-wi approved these changes Dec 26, 2023

View reviewed changes

mattbrictson added 3 commits December 27, 2023 13:12

Merge branch 'master' into features/sftp

c78f4bf

Test that transfer_method can be configured

1a6b3cd

Test that expected implementation is actually used

53ca44e

mattbrictson force-pushed the features/sftp branch from e8cfbef to 53ca44e Compare December 27, 2023 21:42

mattbrictson marked this pull request as ready for review December 27, 2023 21:42

mattbrictson commented Dec 27, 2023

View reviewed changes

JasonPoll reviewed Dec 29, 2023

View reviewed changes

Don't allow global transfer_method to be nil

9bbcc2c

JasonPoll reviewed Dec 30, 2023

View reviewed changes

JasonPoll approved these changes Dec 30, 2023

View reviewed changes

JasonPoll mentioned this pull request Dec 30, 2023

EXAMPLES.md corrections #527

Merged

mattbrictson merged commit 2ab75aa into master Dec 31, 2023
16 checks passed

mattbrictson deleted the features/sftp branch December 31, 2023 16:31

mattbrictson mentioned this pull request Dec 31, 2023

SFTP transfer does not work with Ruby 2.0 #528

Closed

JasonPoll mentioned this pull request Jan 4, 2024

Support older net-sftp API for Ruby 2.0 #529

Merged

juan-apa mentioned this pull request Jan 23, 2024

require missing net/scp dependency basecamp/kamal#645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SFTP to be used for upload!/download! instead of SCP #524

Allow SFTP to be used for upload!/download! instead of SCP #524

mattbrictson commented Dec 22, 2023

mattbrictson commented Dec 22, 2023

will-in-wi left a comment

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023

JasonPoll Dec 30, 2023

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023 •

edited

Loading

mattbrictson Dec 27, 2023

mattbrictson Dec 27, 2023

JasonPoll Dec 29, 2023

mattbrictson Dec 29, 2023

JasonPoll Dec 29, 2023

mattbrictson Dec 29, 2023

mattbrictson Dec 29, 2023

JasonPoll Dec 30, 2023

JasonPoll left a comment

mattbrictson commented Dec 31, 2023

		@@ -162,6 +162,23 @@ end
		In this case the `recursive: true` option mirrors the same options which are
		available to [`Net::{SCP,SFTP}`](http://net-ssh.github.io/net-scp/).

Allow SFTP to be used for upload!/download! instead of SCP #524

Allow SFTP to be used for upload!/download! instead of SCP #524

Conversation

mattbrictson commented Dec 22, 2023

mattbrictson commented Dec 22, 2023

will-in-wi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbrictson Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonPoll left a comment

Choose a reason for hiding this comment

mattbrictson commented Dec 31, 2023

mattbrictson Dec 27, 2023 •

edited

Loading