Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Dec 31, 2023
17 changes: 17 additions & 0 deletions EXAMPLES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


## Set the upload/download method (SCP or SFTP).

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

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

or globally:

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

## Setting global SSH options

Setting global SSH options, these will be overwritten by options set on the
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ you can pass the `strip: false` option: `capture(:ls, '-l', strip: false)`
### Transferring files

All backends also support the `upload!` and `download!` methods for transferring files.
For the remote backend, the file is transferred with scp.
For the remote backend, the file is transferred with scp by default, but sftp is also
supported. See [EXAMPLES.md](EXAMPLES.md) for details.

```ruby
on '1.example.com' do
Expand Down
42 changes: 36 additions & 6 deletions lib/sshkit/backends/netssh.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
require 'strscan'
require 'mutex_m'
require 'net/ssh'
require 'net/scp'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


module Net
module SSH
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil?

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 9bbcc2c


raise ArgumentError, "#{method.inspect} is not a valid transfer method. Supported methods are :scp, :sftp."
end

class Configuration
attr_accessor :connection_timeout, :pty
attr_reader :transfer_method
attr_writer :ssh_options

def initialize
self.transfer_method = :scp
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

def transfer_method=(method)
Netssh.assert_valid_transfer_method!(method)

@transfer_method = method
end

def ssh_options
default_options.merge(@ssh_options ||= {})
end
Expand Down Expand Up @@ -64,16 +80,16 @@ def assign_defaults
def upload!(local, remote, options = {})
summarizer = transfer_summarizer('Uploading', options)
remote = File.join(pwd_path, remote) unless remote.to_s.start_with?("/") || pwd_path.nil?
with_ssh do |ssh|
ssh.scp.upload!(local, remote, options, &summarizer)
with_transfer(summarizer) do |transfer|
transfer.upload!(local, remote, options)
Comment on lines -67 to +84
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

def download!(remote, local=nil, options = {})
summarizer = transfer_summarizer('Downloading', options)
remote = File.join(pwd_path, remote) unless remote.to_s.start_with?("/") || pwd_path.nil?
with_ssh do |ssh|
ssh.scp.download!(remote, local, options, &summarizer)
with_transfer(summarizer) do |transfer|
transfer.download!(remote, local, options)
end
end

Expand Down Expand Up @@ -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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

message = "#{action} #{name} #{percentage.round(2)}%"
percentage_r = (percentage / log_percent).truncate * log_percent
if percentage_r > 0 && (last_name != name || last_percentage != percentage_r)
Expand Down Expand Up @@ -183,6 +199,20 @@ def with_ssh(&block)
)
end

def with_transfer(summarizer)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ 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.

transfer_method = host.transfer_method || self.class.config.transfer_method
transfer_class = if transfer_method == :sftp
require_relative "netssh/sftp_transfer"
SftpTransfer
else
require_relative "netssh/scp_transfer"
ScpTransfer
end

with_ssh do |ssh|
yield(transfer_class.new(ssh, summarizer))
end
end
end
end

Expand Down
26 changes: 26 additions & 0 deletions lib/sshkit/backends/netssh/scp_transfer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require "net/scp"

module SSHKit
module Backend
class Netssh < Abstract
class ScpTransfer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ The scp implementation is trivial, because the existing Netssh implementation was modeled around the net-scp API. So this strategy is just a thin wrapper.

def initialize(ssh, summarizer)
@ssh = ssh
@summarizer = summarizer
end

def upload!(local, remote, options)
ssh.scp.upload!(local, remote, options, &summarizer)
end

def download!(remote, local, options)
ssh.scp.download!(remote, local, options, &summarizer)
end

private

attr_reader :ssh, :summarizer
end
end
end
end
46 changes: 46 additions & 0 deletions lib/sshkit/backends/netssh/sftp_transfer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
require "net/sftp"

module SSHKit
module Backend
class Netssh < Abstract
class SftpTransfer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ The sftp implementation is more complicated, because net-sftp behaves in subtly different ways compared to net-scp, despite have a similar public-facing API.

def initialize(ssh, summarizer)
@ssh = ssh
@summarizer = summarizer
end

def upload!(local, remote, options)
options = { progress: self }.merge(options || {})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ To receive upload/download progress callbacks, we have to give net-sftp a :progress object. This is different from net-scp which takes a block. The :progress object will receive on_put (for upload) and on_get (for download) callbacks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the notes here -- when I had originally looked at this PR before heading out for the holidays I figured that I'd need to take a detour through Net::SFTP to figure out how the summarizer was getting the transfer progress information.

ssh.sftp.connect!
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ To be able to send ssh commands and sftp commands over the same ssh session, we have to explicitly close the sftp channel when we are done. That means that if sftp was previously used, it will exist but be in a closed state. So we have to explicitly reopen it before we use it. If this is the first time using sftp, it will already be open and this is a no-op.

ssh.sftp.upload!(local, remote, options)
ensure
ssh.sftp.close_channel
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ 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.


ssh.sftp.connect!
ssh.sftp.download!(remote, destination, options)
local ? true : destination.string
ensure
ssh.sftp.close_channel
end

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
Comment on lines +31 to +34
Copy link
Member Author

@mattbrictson mattbrictson Dec 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗒️ 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.


def on_put(_upload, file, offset, data)
summarizer.call(nil, file.local, offset + data.bytesize, file.size)
end
Comment on lines +36 to +38
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private

attr_reader :ssh, :summarizer
end
end
end
end
7 changes: 7 additions & 0 deletions lib/sshkit/host.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module SSHKit
class Host

attr_accessor :password, :hostname, :port, :user, :ssh_options
attr_reader :transfer_method

def key=(new_key)
@keys = [new_key]
Expand Down Expand Up @@ -41,6 +42,12 @@ def initialize(host_string_or_options_hash)
end
end

def transfer_method=(method)
Backend::Netssh.assert_valid_transfer_method!(method)

@transfer_method = method
end

def local?
@local
end
Expand Down
1 change: 1 addition & 0 deletions sshkit.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency('mutex_m')
gem.add_runtime_dependency('net-ssh', '>= 2.8.0')
gem.add_runtime_dependency('net-scp', '>= 1.1.2')
gem.add_runtime_dependency('net-sftp', '>= 2.1.2')

gem.add_development_dependency('danger')
gem.add_development_dependency('minitest', '>= 5.0.0')
Expand Down
83 changes: 83 additions & 0 deletions test/functional/backends/netssh_transfer_tests.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
require 'securerandom'

module SSHKit
module Backend
module NetsshTransferTests
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def setup
super
@output = String.new
SSHKit.config.output_verbosity = :debug
SSHKit.config.output = SSHKit::Formatter::SimpleText.new(@output)
end

def a_host
VagrantWrapper.hosts['one']
end

def test_upload_and_then_capture_file_contents
actual_file_contents = ""
file_name = File.join("/tmp", SecureRandom.uuid)
File.open file_name, 'w+' do |f|
f.write "Some Content\nWith a newline and trailing spaces \n "
end
Netssh.new(a_host) do
upload!(file_name, file_name)
actual_file_contents = capture(:cat, file_name, strip: false)
end.run
assert_equal "Some Content\nWith a newline and trailing spaces \n ", actual_file_contents
end

def test_upload_within
file_name = SecureRandom.uuid
file_contents = "Some Content"
dir_name = SecureRandom.uuid
actual_file_contents = ""
Netssh.new(a_host) do |_host|
within("/tmp") do
execute :mkdir, "-p", dir_name
within(dir_name) do
upload!(StringIO.new(file_contents), file_name)
end
end
actual_file_contents = capture(:cat, "/tmp/#{dir_name}/#{file_name}", strip: false)
end.run
assert_equal file_contents, actual_file_contents
end

def test_upload_string_io
file_contents = ""
Netssh.new(a_host) do |_host|
file_name = File.join("/tmp", SecureRandom.uuid)
upload!(StringIO.new('example_io'), file_name)
file_contents = download!(file_name)
end.run
assert_equal "example_io", file_contents
end

def test_upload_large_file
size = 25
fills = SecureRandom.random_bytes(1024*1024)
file_name = "/tmp/file-#{size}.txt"
File.open(file_name, 'wb') do |f|
(size).times {f.write(fills) }
end
file_contents = ""
Netssh.new(a_host) do
upload!(file_name, file_name)
file_contents = download!(file_name)
end.run
assert_equal File.open(file_name, 'rb').read, file_contents
end

def test_upload_via_pathname
file_contents = ""
Netssh.new(a_host) do |_host|
file_name = Pathname.new(File.join("/tmp", SecureRandom.uuid))
upload!(StringIO.new('example_io'), file_name)
file_contents = download!(file_name)
end.run
assert_equal "example_io", file_contents
end
end
end
end
66 changes: 0 additions & 66 deletions test/functional/backends/test_netssh.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
require 'helper'
require 'securerandom'
require 'benchmark'

module SSHKit
Expand Down Expand Up @@ -136,71 +135,6 @@ def test_test_does_not_raise_on_non_zero_exit_status
end.run
end

def test_upload_and_then_capture_file_contents
actual_file_contents = ""
file_name = File.join("/tmp", SecureRandom.uuid)
File.open file_name, 'w+' do |f|
f.write "Some Content\nWith a newline and trailing spaces \n "
end
Netssh.new(a_host) do
upload!(file_name, file_name)
actual_file_contents = capture(:cat, file_name, strip: false)
end.run
assert_equal "Some Content\nWith a newline and trailing spaces \n ", actual_file_contents
end

def test_upload_within
file_name = SecureRandom.uuid
file_contents = "Some Content"
dir_name = SecureRandom.uuid
actual_file_contents = ""
Netssh.new(a_host) do |_host|
within("/tmp") do
execute :mkdir, "-p", dir_name
within(dir_name) do
upload!(StringIO.new(file_contents), file_name)
end
end
actual_file_contents = capture(:cat, "/tmp/#{dir_name}/#{file_name}", strip: false)
end.run
assert_equal file_contents, actual_file_contents
end

def test_upload_string_io
file_contents = ""
Netssh.new(a_host) do |_host|
file_name = File.join("/tmp", SecureRandom.uuid)
upload!(StringIO.new('example_io'), file_name)
file_contents = download!(file_name)
end.run
assert_equal "example_io", file_contents
end

def test_upload_large_file
size = 25
fills = SecureRandom.random_bytes(1024*1024)
file_name = "/tmp/file-#{size}.txt"
File.open(file_name, 'wb') do |f|
(size).times {f.write(fills) }
end
file_contents = ""
Netssh.new(a_host) do
upload!(file_name, file_name)
file_contents = download!(file_name)
end.run
assert_equal File.open(file_name, 'rb').read, file_contents
end

def test_upload_via_pathname
file_contents = ""
Netssh.new(a_host) do |_host|
file_name = Pathname.new(File.join("/tmp", SecureRandom.uuid))
upload!(StringIO.new('example_io'), file_name)
file_contents = download!(file_name)
end.run
assert_equal "example_io", file_contents
end

def test_interaction_handler
captured_command_result = nil
Netssh.new(a_host) do
Expand Down
Loading