diff --git a/lib/vagrant/action/builtin/mixin_synced_folders.rb b/lib/vagrant/action/builtin/mixin_synced_folders.rb index 08fa867a9..c2d8aabb5 100644 --- a/lib/vagrant/action/builtin/mixin_synced_folders.rb +++ b/lib/vagrant/action/builtin/mixin_synced_folders.rb @@ -97,8 +97,14 @@ module Vagrant end end + folder_data = JSON.dump(folders) + + # Scrub any register credentials from the synced folders + # configuration data to prevent accidental leakage + folder_data = Util::CredentialScrubber.desensitize(folder_data) + machine.data_dir.join("synced_folders").open("w") do |f| - f.write(JSON.dump(folders)) + f.write(folder_data) end end diff --git a/plugins/synced_folders/smb/synced_folder.rb b/plugins/synced_folders/smb/synced_folder.rb index f382c36c9..f6b669f13 100644 --- a/plugins/synced_folders/smb/synced_folder.rb +++ b/plugins/synced_folders/smb/synced_folder.rb @@ -152,6 +152,15 @@ module VagrantPlugins guest: data[:guestpath].to_s)) machine.guest.capability( :mount_smb_shared_folder, data[:smb_id], data[:guestpath], data) + + clean_folder_configuration(data) + end + end + + # Nothing to do here but ensure folder options are scrubbed + def disable(machine, folders, opts) + folders.each do |_, data| + clean_folder_configuration(data) end end @@ -160,6 +169,18 @@ module VagrantPlugins machine.env.host.capability(:smb_cleanup, machine, opts) end end + + protected + + # Remove data that should not be persisted within folder + # specific configuration + # + # @param [Hash] data Folder configuration + def clean_folder_configuration(data) + return if !data.is_a?(Hash) + data.delete(:smb_password) + nil + end end end end diff --git a/test/unit/plugins/synced_folders/smb/synced_folder_test.rb b/test/unit/plugins/synced_folders/smb/synced_folder_test.rb index 493a427f5..888eee332 100644 --- a/test/unit/plugins/synced_folders/smb/synced_folder_test.rb +++ b/test/unit/plugins/synced_folders/smb/synced_folder_test.rb @@ -36,7 +36,7 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do end end - describe ".usable?" do + describe "#usable?" do context "without supporting capabilities" do it "is not usable" do expect(subject.usable?(machine)).to be(false) @@ -66,7 +66,7 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do end end - describe ".prepare" do + describe "#prepare" do let(:host_caps){ [:smb_start, :smb_prepare] } context "with username credentials provided" do @@ -181,7 +181,7 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do end end - describe ".enable" do + describe "#enable" do it "fails when guest does not support capability" do expect{ subject.enable(machine, folders, options) @@ -229,6 +229,11 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do expect(folders["/second/path"][:smb_host]).to eq("ADDR") end + it "should scrub folder configuration" do + expect(subject).to receive(:clean_folder_configuration).at_least(:once) + subject.enable(machine, folders, options) + end + context "with smb_host option set" do let(:folders){ {"/first/path" => {smb_host: "ADDR"}, "/second/path" => {}} } @@ -251,10 +256,63 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do expect(folders["/second/path"][:group]).to eq("smbgroup") end end + + context "with smb_username and smb_password set" do + let(:folders){ { + "/first/path" => {owner: "smbowner", smb_username: "user", smb_password: "pass"}, + "/second/path" => {group: "smbgroup", smb_username: "user", smb_password: "pass"} + } } + + it "should retain non password configuration options" do + subject.enable(machine, folders, options) + folder1 = folders["/first/path"] + folder2 = folders["/second/path"] + expect(folder1.key?(:owner)).to be_truthy + expect(folder1.key?(:smb_username)).to be_truthy + expect(folder2.key?(:group)).to be_truthy + expect(folder2.key?(:smb_username)).to be_truthy + end + + it "should remove the smb_password option when set" do + subject.enable(machine, folders, options) + expect(folders["/first/path"].key?(:smb_password)).to be_falsey + expect(folders["/second/path"].key?(:smb_password)).to be_falsey + end + end end end - describe ".cleanup" do + describe "#disable" do + it "should scrub folder configuration" do + expect(subject).to receive(:clean_folder_configuration).at_least(:once) + subject.disable(machine, folders, options) + end + + context "with smb_username and smb_password set" do + let(:folders){ { + "/first/path" => {owner: "smbowner", smb_username: "user", smb_password: "pass"}, + "/second/path" => {group: "smbgroup", smb_username: "user", smb_password: "pass"} + } } + + it "should retain non password configuration options" do + subject.disable(machine, folders, options) + folder1 = folders["/first/path"] + folder2 = folders["/second/path"] + expect(folder1.key?(:owner)).to be_truthy + expect(folder1.key?(:smb_username)).to be_truthy + expect(folder2.key?(:group)).to be_truthy + expect(folder2.key?(:smb_username)).to be_truthy + end + + it "should remove the smb_password option when set" do + subject.disable(machine, folders, options) + expect(folders["/first/path"].key?(:smb_password)).to be_falsey + expect(folders["/second/path"].key?(:smb_password)).to be_falsey + end + end + end + + describe "#cleanup" do context "without supporting capability" do it "does nothing" do subject.cleanup(machine, options) @@ -270,4 +328,17 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do end end end + + describe "#clean_folder_configuration" do + it "should remove smb_password if defined" do + data = {smb_password: "password"} + subject.send(:clean_folder_configuration, data) + expect(data.key?(:smb_password)).to be_falsey + end + + it "should not error if non-hash value provided" do + expect { subject.send(:clean_folder_configuration, nil) }. + not_to raise_error + end + end end diff --git a/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb b/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb index ff7a6e7c5..e07223753 100644 --- a/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb +++ b/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb @@ -256,6 +256,66 @@ describe Vagrant::Action::Builtin::MixinSyncedFolders do end end + describe "#save_synced_folders" do + let(:folders) { {} } + let(:options) { {} } + let(:output_file) { double("output_file") } + + before do + allow(machine.data_dir).to receive(:join).with("synced_folders"). + and_return(output_file) + allow(output_file).to receive(:open).and_yield(output_file) + allow(output_file).to receive(:write) + end + + it "should write empty hash to file" do + expect(output_file).to receive(:write).with("{}") + subject.save_synced_folders(machine, folders, options) + end + + it "should call credential scrubber before writing file" do + expect(Vagrant::Util::CredentialScrubber).to receive(:desensitize).and_call_original + subject.save_synced_folders(machine, folders, options) + end + + context "when folder data is defined" do + let(:folders) { + {"root" => { + hostpath: "foo", type: "nfs", nfs__foo: "bar"}} + } + + it "should write folder information to file" do + expect(output_file).to receive(:write).with(JSON.dump(folders)) + subject.save_synced_folders(machine, folders, options) + end + + context "when folder data configuration includes sensitive data" do + let(:password) { "VAGRANT_TEST_PASSWORD" } + + before do + folders["root"][:folder_password] = password + Vagrant::Util::CredentialScrubber.sensitive(password) + end + + after { Vagrant::Util::CredentialScrubber.unsensitive(password) } + + it "should not include password when writing file" do + expect(output_file).to receive(:write) do |content| + expect(content).not_to include(password) + end + subject.save_synced_folders(machine, folders, options) + end + + it "should mask password content when writing file" do + expect(output_file).to receive(:write) do |content| + expect(content).to include(Vagrant::Util::CredentialScrubber::REPLACEMENT_TEXT) + end + subject.save_synced_folders(machine, folders, options) + end + end + end + end + describe "#synced_folders_diff" do it "sees two equal " do one = { diff --git a/test/unit/vagrant/util/ssh_test.rb b/test/unit/vagrant/util/ssh_test.rb index 9d84f0225..1a8b0a5d6 100644 --- a/test/unit/vagrant/util/ssh_test.rb +++ b/test/unit/vagrant/util/ssh_test.rb @@ -41,6 +41,10 @@ describe Vagrant::Util::SSH do let(:ssh_path) { "/usr/bin/ssh" } + before { + allow(Vagrant::Util::Which).to receive(:which).with("ssh", any_args).and_return(ssh_path) + } + it "searches original PATH for executable" do expect(Vagrant::Util::Which).to receive(:which).with("ssh", original_path: true).and_return("valid-return") allow(Vagrant::Util::SafeExec).to receive(:exec).and_return(nil)