From 051c7a78238d5d28d8a3fcf9c76ab1f4c986e690 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 2 Aug 2018 15:13:27 -0700 Subject: [PATCH] Fix sudo usage in exports write within linux host cap Only move new exports file to destination without sudo when the file has write access and the directory has write access. Always use sudo when changing file ownership. --- plugins/hosts/linux/cap/nfs.rb | 9 ++- test/unit/plugins/hosts/linux/cap/nfs_test.rb | 72 +++++++++++++++++-- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/plugins/hosts/linux/cap/nfs.rb b/plugins/hosts/linux/cap/nfs.rb index 1d407d52e..630dc0f4e 100644 --- a/plugins/hosts/linux/cap/nfs.rb +++ b/plugins/hosts/linux/cap/nfs.rb @@ -173,16 +173,14 @@ module VagrantPlugins def self.nfs_write_exports(new_exports_content) if(nfs_exports_content != new_exports_content.strip) begin + exports_path = Pathname.new(NFS_EXPORTS_PATH) + # Write contents out to temporary file new_exports_file = Tempfile.create('vagrant') new_exports_file.puts(new_exports_content) new_exports_file.close new_exports_path = new_exports_file.path - # Only use "sudo" if we can't write to /etc/exports directly - sudo_command = "" - sudo_command = "sudo " if !File.writable?(NFS_EXPORTS_PATH) - # Ensure new file mode and uid/gid match existing file to replace existing_stat = File.stat(NFS_EXPORTS_PATH) new_stat = File.stat(new_exports_path) @@ -190,7 +188,7 @@ module VagrantPlugins File.chmod(existing_stat.mode, new_exports_path) end if existing_stat.uid != new_stat.uid || existing_stat.gid != new_stat.gid - chown_cmd = "#{sudo_command}chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}" + chown_cmd = "sudo chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}" result = Vagrant::Util::Subprocess.execute(*Shellwords.split(chown_cmd)) if result.exit_code != 0 raise Vagrant::Errors::NFSExportsFailed, @@ -200,6 +198,7 @@ module VagrantPlugins end end # Always force move the file to prevent overwrite prompting + sudo_command = "sudo " if !exports_path.writable? || !exports_path.dirname.writable? mv_cmd = "#{sudo_command}mv -f #{new_exports_path} #{NFS_EXPORTS_PATH}" result = Vagrant::Util::Subprocess.execute(*Shellwords.split(mv_cmd)) if result.exit_code != 0 diff --git a/test/unit/plugins/hosts/linux/cap/nfs_test.rb b/test/unit/plugins/hosts/linux/cap/nfs_test.rb index 144041924..575035ab9 100644 --- a/test/unit/plugins/hosts/linux/cap/nfs_test.rb +++ b/test/unit/plugins/hosts/linux/cap/nfs_test.rb @@ -269,14 +269,72 @@ EOH expect{ described_class.nfs_write_exports("new content") }.to raise_error(Vagrant::Errors::NFSExportsFailed) end - it "should retain existing file owner and group IDs" do - pending("investigate using a simulated FS to test") - test_with_simulated_fs - end + context "exports file modification" do + let(:tmp_stat) { double("tmp_stat", uid: 100, gid: 100, mode: tmp_mode) } + let(:tmp_mode) { 0 } + let(:exports_stat) { double("stat", uid: exports_uid, gid: exports_gid, mode: exports_mode) } + let(:exports_uid) { -1 } + let(:exports_gid) { -1 } + let(:exports_mode) { 0 } + let(:new_exports_file) { double("new_exports_file", path: "/dev/null/exports") } - it "should raise custom exception when chown fails" do - pending("investigate using a simulated FS to test") - test_with_simulated_fs + before do + allow(File).to receive(:stat).with(new_exports_file.path).and_return(tmp_stat) + allow(File).to receive(:stat).with(tmp_exports_path.to_s).and_return(exports_stat) + allow(new_exports_file).to receive(:puts) + allow(new_exports_file).to receive(:close) + allow(Vagrant::Util::Subprocess).to receive(:execute).and_return(Vagrant::Util::Subprocess::Result.new(0, "", "")) + allow(Tempfile).to receive(:create).with("vagrant").and_return(new_exports_file) + end + + it "should retain existing file owner and group IDs" do + expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args| + expect(args).to include("sudo") + expect(args).to include("chown") + }.and_return(Vagrant::Util::Subprocess::Result.new(0, "", "")) + described_class.nfs_write_exports("new content") + end + + it "should raise custom exception when chown fails" do + expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args| + expect(args).to include("sudo") + expect(args).to include("chown") + }.and_return(Vagrant::Util::Subprocess::Result.new(1, "", "")) + expect { described_class.nfs_write_exports("new content") }.to raise_error(Vagrant::Errors::NFSExportsFailed) + end + + context "when user has write access to exports file" do + let(:file_writable?) { true } + let(:dir_writable?) { false } + let(:exports_pathname) { double("exports_pathname", writable?: file_writable?, dirname: exports_dir_pathname) } + let(:exports_dir_pathname) { double("exports_dir_pathname", writable?: dir_writable?) } + + before do + allow(File).to receive(:stat).and_return(exports_stat) + allow(File).to receive(:exist?).and_return(false) + allow(Pathname).to receive(:new).with(tmp_exports_path.to_s).and_return(exports_pathname) + end + + it "should use sudo when moving new file" do + expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args| + expect(args).to include("sudo") + expect(args).to include("mv") + }.and_return(Vagrant::Util::Subprocess::Result.new(0, "", "")) + described_class.nfs_write_exports("new content") + end + + context "and write access to exports parent directory" do + let(:dir_writable?) { true } + + it "should not use sudo when moving new file" do + expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args| + expect(args).not_to include("sudo") + expect(args).to include("mv") + }.and_return(Vagrant::Util::Subprocess::Result.new(0, "", "")) + described_class.nfs_write_exports("new content") + end + end + end end end end