From a69ff5054dec829e496e95ff6fec48def554cb42 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 2 Apr 2018 17:33:35 -0700 Subject: [PATCH] Update generated ssh private key file permissions on create This updates the permissions on the automatically generated private key file to only be readable by the user. Includes support for file permission modification on Windows platform. --- lib/vagrant.rb | 6 + plugins/communicators/ssh/communicator.rb | 15 ++ .../communicators/ssh/communicator_test.rb | 135 +++++++++++++++++- vagrant.gemspec | 2 + 4 files changed, 154 insertions(+), 4 deletions(-) diff --git a/lib/vagrant.rb b/lib/vagrant.rb index ca48f40c8..3f978a1a7 100644 --- a/lib/vagrant.rb +++ b/lib/vagrant.rb @@ -65,6 +65,12 @@ require 'i18n' # there are issues with ciphers not being properly loaded. require 'openssl' +# If we are on Windows, load in File helpers +if Vagrant::Util::Platform.windows? + require "win32/file" + require "win32/file/security" +end + # Always make the version available require 'vagrant/version' global_logger = Log4r::Logger.new("vagrant::global") diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index 149566483..7b7f44e5b 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -193,6 +193,21 @@ module VagrantPlugins f.write(priv) end + # Adjust private key file permissions + if Vagrant::Util::Platform.windows? + priv_path = @machine.data_dir.join("private_key").to_s + priv_owner = File.owner(priv_path) + new_perms = File.get_permissions(priv_path).map do |p_user, perm| + if p_user != priv_owner + perm = File::DELETE + end + [p_user, perm] + end + File.set_permissions(priv_path, Hash[new_perms]) + else + @machine.data_dir.join("private_key").chmod(0600) + end + # Remove the old key if it exists @machine.ui.detail(I18n.t("vagrant.inserting_remove_key")) @machine.guest.capability( diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index 50510445a..cb754a428 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -16,11 +16,13 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do guest_port: 5986, pty: false, keep_alive: false, - insert_key: false, + insert_key: insert_ssh_key, export_command_template: export_command_template, shell: 'bash -l' ) end + # Do not insert public key by default + let(:insert_ssh_key){ false } # Configuration mock let(:config) { double("config", ssh: ssh) } # Provider mock @@ -35,6 +37,8 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do ui: ui ) end + # SSH information of the machine + let(:machine_ssh_info){ {host: '10.1.2.3', port: 22} } # Subject instance to test let(:communicator){ @communicator ||= described_class.new(machine) } # Underlying net-ssh connection mock @@ -60,12 +64,14 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do # Mock for net-ssh scp let(:scp) { double("scp") } + # Setup for commands using the net-ssh connection. This can be reused where needed # by providing to `before` connection_setup = proc do allow(connection).to receive(:closed?).and_return false allow(connection).to receive(:open_channel). and_yield(channel).and_return(channel) + allow(connection).to receive(:close) allow(channel).to receive(:wait).and_return true allow(channel).to receive(:close) allow(command_channel).to receive(:send_data) @@ -74,10 +80,10 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do and_yield(nil, command_stdout_data) allow(command_channel).to receive(:on_extended_data). and_yield(nil, nil, command_stderr_data) - allow(machine).to receive(:ssh_info).and_return(host: '10.1.2.3', port: 22) - expect(channel).to receive(:exec).with(core_shell_cmd). + allow(machine).to receive(:ssh_info).and_return(machine_ssh_info) + allow(channel).to receive(:exec).with(core_shell_cmd). and_yield(command_channel, '').and_return channel - expect(command_channel).to receive(:on_request).with('exit-status'). + allow(command_channel).to receive(:on_request).with('exit-status'). and_yield(nil, exit_data) # Return mocked net-ssh connection during setup allow(communicator).to receive(:retryable).and_return(connection) @@ -121,6 +127,127 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do expect{ communicator.ready? }.to raise_error Vagrant::Errors::SSHInvalidShell end end + + context "with insert key enabled" do + before do + allow(machine).to receive(:guest).and_return(guest) + allow(guest).to receive(:capability?).with(:insert_public_key). + and_return(has_insert_cap) + allow(guest).to receive(:capability?).with(:remove_public_key). + and_return(has_remove_cap) + allow(communicator).to receive(:insecure_key?).with("KEY_PATH").and_return(true) + allow(ui).to receive(:detail) + end + + let(:insert_ssh_key){ true } + let(:has_insert_cap){ false } + let(:has_remove_cap){ false } + let(:machine_ssh_info){ + {host: '10.1.2.3', port: 22, private_key_path: ["KEY_PATH"]} + } + let(:guest){ double("guest") } + + context "without guest insert_ssh_key or remove_ssh_key capabilities" do + it "should raise an error" do + expect{ communicator.ready? }.to raise_error(Vagrant::Errors::SSHInsertKeyUnsupported) + end + end + + context "without guest insert_ssh_key capability" do + let(:has_remove_cap){ true } + + it "should raise an error" do + expect{ communicator.ready? }.to raise_error(Vagrant::Errors::SSHInsertKeyUnsupported) + end + end + + context "without guest remove_ssh_key capability" do + let(:has_insert_cap){ true } + + it "should raise an error" do + expect{ communicator.ready? }.to raise_error(Vagrant::Errors::SSHInsertKeyUnsupported) + end + end + + context "with guest insert_ssh_key capability and remove_ssh_key capability" do + let(:has_insert_cap){ true } + let(:has_remove_cap){ true } + let(:new_public_key){ :new_public_key } + let(:new_private_key){ :new_private_key } + let(:openssh){ :openssh } + let(:private_key_file){ double("private_key_file") } + let(:path_joiner){ double("path_joiner") } + + before do + allow(ui).to receive(:info) + allow(Vagrant::Util::Keypair).to receive(:create). + and_return([new_public_key, new_private_key, openssh]) + allow(private_key_file).to receive(:open).and_yield(private_key_file) + allow(private_key_file).to receive(:write) + allow(private_key_file).to receive(:chmod) + allow(guest).to receive(:capability) + allow(File).to receive(:chmod) + allow(machine).to receive(:data_dir).and_return(path_joiner) + allow(path_joiner).to receive(:join).and_return(private_key_file) + allow(guest).to receive(:capability).with(:insert_public_key) + allow(guest).to receive(:capability).with(:remove_public_key) + end + + after{ communicator.ready? } + + it "should create a new key pair" do + expect(Vagrant::Util::Keypair).to receive(:create). + and_return([new_public_key, new_private_key, openssh]) + end + + it "should call the insert_public_key guest capability" do + expect(guest).to receive(:capability).with(:insert_public_key, openssh) + end + + it "should write the new private key" do + expect(private_key_file).to receive(:write).with(new_private_key) + end + + it "should set private key file as user readable only" do + expect(private_key_file).to receive(:chmod).with(0600) + end + + it "should remove the default public key" do + expect(guest).to receive(:capability).with(:remove_public_key, any_args) + end + + context "on windows platform" do + let(:owner){ "owner" } + let(:permissions){ {} } + + before do + allow(private_key_file).to receive(:to_s).and_return("PRIVATE_KEY_PATH") + allow(File).to receive(:owner).and_return(owner) + allow(File).to receive(:get_permissions).and_return(permissions) + allow(File).to receive(:set_permissions) + allow(Vagrant::Util::Platform).to receive(:windows?).and_return(true) + stub_const('File::DELETE', :delete) + end + + it "should extract owner username from file" do + expect(File).to receive(:owner).with("PRIVATE_KEY_PATH") + end + + it "should get set new permissions on private key file" do + expect(File).to receive(:set_permissions).with("PRIVATE_KEY_PATH", {}) + end + + context "with multiple permissions on file" do + let(:permissions){ {owner => :permission, "other" => :permission, "all" => :permission} } + + it "should delete all non-owner permissions" do + expect(File).to receive(:set_permissions).with("PRIVATE_KEY_PATH", + owner => :permission, "other" => :delete, "all" => :delete) + end + end + end + end + end end describe ".execute" do diff --git a/vagrant.gemspec b/vagrant.gemspec index edc12f032..4096fd684 100644 --- a/vagrant.gemspec +++ b/vagrant.gemspec @@ -28,6 +28,8 @@ Gem::Specification.new do |s| s.add_dependency "rb-kqueue", "~> 0.2.0" s.add_dependency "rest-client", ">= 1.6.0", "< 3.0" s.add_dependency "wdm", "~> 0.1.0" + s.add_dependency "win32-file", "~> 0.8.1" + s.add_dependency "win32-file-security", "~> 1.0.10" s.add_dependency "winrm", "~> 2.1" s.add_dependency "winrm-fs", "~> 1.0" s.add_dependency "winrm-elevated", "~> 1.1"