From a2a59b532b8a43c5a56ae22c62c14e159718278a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 19 Mar 2010 01:57:28 -0700 Subject: [PATCH] `vagrant ssh` will automatically fix permissions on the private key if necessary --- lib/vagrant/ssh.rb | 27 ++++++++++++++ templates/errors.yml | 6 ++++ test/vagrant/ssh_test.rb | 77 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/lib/vagrant/ssh.rb b/lib/vagrant/ssh.rb index 0f81772b2..50d52c621 100644 --- a/lib/vagrant/ssh.rb +++ b/lib/vagrant/ssh.rb @@ -22,6 +22,7 @@ module Vagrant options[param] = opts[param] || env.config.ssh.send(param) end + check_key_permissions(options[:private_key_path]) Kernel.exec "ssh -p #{port(opts)} -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i #{options[:private_key_path]} #{options[:username]}@#{options[:host]}".strip end @@ -67,6 +68,32 @@ module Vagrant error_and_exit(:vm_ssh_auth_failed) end + # Checks the file permissions for the private key, resetting them + # if needed, or on failure erroring. + def check_key_permissions(key_path) + # TODO: This only works on unix based systems for now. Windows + # systems will need to be investigated further. + stat = File.stat(key_path) + + if stat.owned? && file_perms(key_path) != "600" + logger.info "Permissions on private key incorrect, fixing..." + File.chmod(0600, key_path) + + error_and_exit(:ssh_bad_permissions, :key_path => key_path) if file_perms(key_path) != "600" + end + rescue Errno::EPERM + # This shouldn't happen since we verify we own the file, but just + # in case. + error_and_exit(:ssh_bad_permissions, :key_path => key_path) + end + + # Returns the file permissions of a given file. This is fairly unix specific + # and probably doesn't belong in this class. Will be refactored out later. + def file_perms(path) + perms = sprintf("%o", File.stat(path).mode) + perms.reverse[0..2].reverse + end + # Returns the port which is either given in the options hash or taken from # the config by finding it in the forwarded ports hash based on the # `config.ssh.forwarded_port_key` diff --git a/templates/errors.yml b/templates/errors.yml index 3039d0e3d..f01c48d0e 100644 --- a/templates/errors.yml +++ b/templates/errors.yml @@ -52,6 +52,12 @@ \nsince it describes the expected environment that vagrant is supposed \nto manage. Please create a `<%= Vagrant::Env::ROOTFILE_NAME %>` and place it in your project \nroot." +:ssh_bad_permissions: "The private key to connect to this box via SSH has invalid permissions + \nset on it. The permissions of the private key should be set to 0600, otherwise SSH will + \nignore the key. Vagrant tried to do this automatically for you but failed. Please set the + \npermissions on the following file to 0600 and then try running this command again: + + \n<%= key_path %>" :virtualbox_import_failure: "The VM import failed! Try running `VBoxManage import` on the box file manually for more verbose error output." :virtualbox_invalid_version: "Vagrant has detected that you have VirtualBox version <%= version %> installed! \nVagrant requires that you use at least VirtualBox version 3.1. Please install diff --git a/test/vagrant/ssh_test.rb b/test/vagrant/ssh_test.rb index 10e1fc0a6..b20fb934e 100644 --- a/test/vagrant/ssh_test.rb +++ b/test/vagrant/ssh_test.rb @@ -16,6 +16,14 @@ class SshTest < Test::Unit::TestCase context "connecting to external SSH" do setup do mock_ssh + @ssh.stubs(:check_key_permissions) + end + + should "check key permissions prior to exec" do + exec_seq = sequence("exec_seq") + @ssh.expects(:check_key_permissions).with(@env.config.ssh.private_key_path).once.in_sequence(exec_seq) + Kernel.expects(:exec).in_sequence(exec_seq) + @ssh.connect end should "call exec with defaults when no options are supplied" do @@ -38,7 +46,7 @@ class SshTest < Test::Unit::TestCase assert arg =~ /-p #{port}/ assert arg =~ /-i #{key_path}/ assert arg =~ /#{uname}@#{host}/ - # TODO options not tested for as they may be removed, they may be removed + # TODO options not tested for as they may be removed true end end @@ -144,4 +152,71 @@ class SshTest < Test::Unit::TestCase assert_equal "47", @ssh.port({ :port => "47" }) end end + + context "checking key permissions" do + setup do + mock_ssh + @ssh.stubs(:file_perms) + + @key_path = "foo" + + + @stat = mock("stat") + @stat.stubs(:owned?).returns(true) + File.stubs(:stat).returns(@stat) + end + + should "do nothing if the user is not the owner" do + @stat.expects(:owned?).returns(false) + File.expects(:chmod).never + @ssh.check_key_permissions(@key_path) + end + + should "do nothing if the file perms equal 600" do + @ssh.expects(:file_perms).with(@key_path).returns("600") + File.expects(:chmod).never + @ssh.check_key_permissions(@key_path) + end + + should "chmod the file if the file perms aren't 600" do + perm_sequence = sequence("perm_seq") + @ssh.expects(:file_perms).returns("900").in_sequence(perm_sequence) + File.expects(:chmod).with(0600, @key_path).once.in_sequence(perm_sequence) + @ssh.expects(:file_perms).returns("600").in_sequence(perm_sequence) + @ssh.expects(:error_and_exit).never + @ssh.check_key_permissions(@key_path) + end + + should "error and exit if the resulting chmod doesn't work" do + perm_sequence = sequence("perm_seq") + @ssh.expects(:file_perms).returns("900").in_sequence(perm_sequence) + File.expects(:chmod).with(0600, @key_path).once.in_sequence(perm_sequence) + @ssh.expects(:file_perms).returns("900").in_sequence(perm_sequence) + @ssh.expects(:error_and_exit).once.with(:ssh_bad_permissions, :key_path => @key_path).in_sequence(perm_sequence) + @ssh.check_key_permissions(@key_path) + end + + should "error and exit if a bad file perm is raised" do + @ssh.expects(:file_perms).with(@key_path).returns("900") + File.expects(:chmod).raises(Errno::EPERM) + @ssh.expects(:error_and_exit).once.with(:ssh_bad_permissions, :key_path => @key_path) + @ssh.check_key_permissions(@key_path) + end + end + + context "getting file permissions" do + setup do + mock_ssh + end + + should "return the last 3 characters of the file mode" do + path = "foo" + mode = "10000foo" + stat = mock("stat") + File.expects(:stat).with(path).returns(stat) + stat.expects(:mode).returns(mode) + @ssh.expects(:sprintf).with("%o", mode).returns(mode) + assert_equal path, @ssh.file_perms(path) + end + end end