Merge pull request #10811 from chrisroberts/f-synced-folder-creds

Remove configuration information from SMB synced folder data

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
 

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

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

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 = {

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)