From b493503e09499eee3247a0f1b371b34cba244ea3 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 25 Apr 2019 10:07:48 -0700 Subject: [PATCH 1/4] Scrub SMB credential information from folder configuration This prevents credential information from being persisted into the local data directory which is used during subsequent runs to determine folder definition changes. --- plugins/synced_folders/smb/synced_folder.rb | 21 ++++++ .../synced_folders/smb/synced_folder_test.rb | 71 +++++++++++++++++++ 2 files changed, 92 insertions(+) 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..e3383519d 100644 --- a/test/unit/plugins/synced_folders/smb/synced_folder_test.rb +++ b/test/unit/plugins/synced_folders/smb/synced_folder_test.rb @@ -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,6 +256,59 @@ 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 "#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 @@ -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 From 92e6a29bfc0d9a387584ffd414e97772dd0d918c Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 25 Apr 2019 10:09:50 -0700 Subject: [PATCH 2/4] Update naming in tests as instance methods are being referenced --- .../unit/plugins/synced_folders/smb/synced_folder_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 e3383519d..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) @@ -312,7 +312,7 @@ describe VagrantPlugins::SyncedFolderSMB::SyncedFolder do end end - describe ".cleanup" do + describe "#cleanup" do context "without supporting capability" do it "does nothing" do subject.cleanup(machine, options) From 5b94bbb49bcebe033f79007661e35315880a4493 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 25 Apr 2019 10:31:48 -0700 Subject: [PATCH 3/4] Scrub folder configuration data when persisting to disk Before writing synced folder configuration data to the local data directory run content through the credential scrubber to remove any sensitive content before write. --- .../action/builtin/mixin_synced_folders.rb | 8 ++- .../builtin/mixin_synced_folders_test.rb | 60 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) 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/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 = { From e7e8a39c554ffce1e308b07fb71ef5b07a7c6cc3 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 25 Apr 2019 10:44:25 -0700 Subject: [PATCH 4/4] Stub the ssh path used for exec Since a full path to the ssh executable is being used and is expected at a specific location, default to providing that location when looking up the executable. This prevents errors from occurring when a host system provides an `ssh` match at a different path. --- test/unit/vagrant/util/ssh_test.rb | 4 ++++ 1 file changed, 4 insertions(+) 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)