Merge pull request #9923 from chrisroberts/f-win-perms

Update SSH key file permissions handling

.travis.yml

@@ -10,9 +10,10 @@ addons:
     - bsdtar
 
 rvm:
-  - 2.3.6
+  - 2.3.7
-  - 2.4.3
+  - 2.4.4
   - 2.5.0
+  - 2.5.1
 
 branches:
   only:

lib/vagrant.rb

@@ -65,12 +65,6 @@ 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 "ffi-win32-extensions"
-  require "win32/file/security"
-end
-
 # Always make the version available
 require 'vagrant/version'
 global_logger = Log4r::Logger.new("vagrant::global")

lib/vagrant/util/powershell.rb

@@ -54,8 +54,12 @@ module Vagrant
         if opts.delete(:sudo) || opts.delete(:runas)
           powerup_command(path, args, opts)
         else
-          env = opts.delete(:env)
+          if mpath = opts.delete(:module_path)
-          if env
+            m_env = opts.fetch(:env, {})
+            m_env["PSModulePath"] = "$env:PSModulePath+';#{mpath}'"
+            opts[:env] = m_env
+          end
+          if env = opts.delete(:env)
             env = env.map{|k,v| "$env:#{k}=#{v}"}.join(";") + "; "
           end
           command = [
@@ -85,8 +89,12 @@ module Vagrant
       #   Returns stdout string if exit code is zero.
       def self.execute_cmd(command, **opts)
         validate_install!
-        env = opts.delete(:env)
+        if mpath = opts.delete(:module_path)
-        if env
+          m_env = opts.fetch(:env, {})
+          m_env["PSModulePath"] = "$env:PSModulePath+';#{mpath}'"
+          opts[:env] = m_env
+        end
+        if env = opts.delete(:env)
           env = env.map{|k,v| "$env:#{k}=#{v}"}.join(";") + "; "
         end
         c = [
@@ -112,8 +120,12 @@ module Vagrant
       # @param [Block] block Ruby block
       def self.execute_inline(*command, **opts, &block)
         validate_install!
-        env = opts.delete(:env)
+        if mpath = opts.delete(:module_path)
-        if env
+          m_env = opts.fetch(:env, {})
+          m_env["PSModulePath"] = "$env:PSModulePath+';#{mpath}'"
+          opts[:env] = m_env
+        end
+        if env = opts.delete(:env)
           env = env.map{|k,v| "$env:#{k}=#{v}"}.join(";") + "; "
         end
         c = [

plugins/communicators/ssh/communicator.rb

@@ -194,17 +194,9 @@ module VagrantPlugins
             f.write(priv)
           end
 
-          # Adjust private key file permissions
+          # Adjust private key file permissions if host provides capability
-          if Vagrant::Util::Platform.windows?
+          if @machine.env.host.capability?(:set_ssh_key_permissions)
-            begin
+            @machine.env.host.capability(:set_ssh_key_permissions, @machine.data_dir.join("private_key"))
-              priv_path = @machine.data_dir.join("private_key").to_s
-              File.set_permissions(priv_path, Etc.getlogin => File::FULL)
-            rescue => e
-              @logger.warn("Error encountered during private key permissions set - " \
-                "#{e.class}: #{e.message}")
-            end
-          else
-            @machine.data_dir.join("private_key").chmod(0600)
           end
 
           # Remove the old key if it exists

plugins/hosts/bsd/cap/ssh.rb

@@ -0,0 +1,16 @@
+module VagrantPlugins
+  module HostBSD
+    module Cap
+      class SSH
+        # Set the ownership and permissions for SSH
+        # private key
+        #
+        # @param [Vagrant::Environment] env
+        # @param [Pathname] key_path
+        def self.set_ssh_key_permissions(env, key_path)
+          key_path.chmod(0600)
+        end
+      end
+    end
+  end
+end

plugins/hosts/bsd/plugin.rb

@@ -35,6 +35,11 @@ module VagrantPlugins
         require_relative "cap/nfs"
         Cap::NFS
       end
+
+      host_capability("bsd", "set_ssh_key_permissions") do
+        require_relative "cap/ssh"
+        Cap::SSH
+      end
     end
   end
 end

plugins/hosts/linux/cap/ssh.rb

@@ -0,0 +1,16 @@
+module VagrantPlugins
+  module HostLinux
+    module Cap
+      class SSH
+        # Set the ownership and permissions for SSH
+        # private key
+        #
+        # @param [Vagrant::Environment] env
+        # @param [Pathname] key_path
+        def self.set_ssh_key_permissions(env, key_path)
+          key_path.chmod(0600)
+        end
+      end
+    end
+  end
+end

plugins/hosts/linux/plugin.rb

@@ -47,6 +47,11 @@ module VagrantPlugins
         require_relative "cap/nfs"
         Cap::NFS
       end
+
+      host_capability("linux", "set_ssh_key_permissions") do
+        require_relative "cap/ssh"
+        Cap::SSH
+      end
     end
   end
 end

plugins/hosts/windows/cap/ssh.rb

@@ -0,0 +1,26 @@
+module VagrantPlugins
+  module HostWindows
+    module Cap
+      class SSH
+        # Set the ownership and permissions for SSH
+        # private key
+        #
+        # @param [Vagrant::Environment] env
+        # @param [Pathname] key_path
+        def self.set_ssh_key_permissions(env, key_path)
+          script_path = Host.scripts_path.join("set_ssh_key_permissions.ps1")
+          result = Vagrant::Util::PowerShell.execute(
+            script_path.to_s, "-KeyPath", key_path.to_s,
+            module_path: Host.modules_path.to_s
+          )
+          if result.exit_code != 0
+            raise Vagrant::Errors::PowerShellError,
+              script: script_path,
+              stderr: result.stderr
+          end
+          result
+        end
+      end
+    end
+  end
+end

plugins/hosts/windows/host.rb

@@ -8,6 +8,16 @@ module VagrantPlugins
       def detect?(env)
         Vagrant::Util::Platform.windows?
       end
+
+      # @return [Pathname] Path to scripts directory
+      def self.scripts_path
+        Pathname.new(File.expand_path("../scripts", __FILE__))
+      end
+
+      # @return [Pathname] Path to modules directory
+      def self.modules_path
+        scripts_path.join("utils")
+      end
     end
   end
 end

plugins/hosts/windows/plugin.rb

@@ -55,6 +55,11 @@ module VagrantPlugins
         require_relative "cap/configured_ip_addresses"
         Cap::ConfiguredIPAddresses
       end
+
+      host_capability("windows", "set_ssh_key_permissions") do
+        require_relative "cap/ssh"
+        Cap::SSH
+      end
     end
   end
 end

plugins/hosts/windows/scripts/set_ssh_key_permissions.ps1

@@ -0,0 +1,17 @@
+#Requires -Modules VagrantSSH
+
+param(
+    [Parameter(Mandatory=$true)]
+    [string] $KeyPath,
+    [Parameter(Mandatory=$false)]
+    [string] $Principal=$null
+)
+
+$ErrorActionPreference = "Stop"
+
+try {
+    Set-SSHKeyPermissions -SSHKeyPath $KeyPath -Principal $Principal
+} catch {
+    Write-Error "Failed to set permissions on key: ${PSItem}"
+    exit 1
+}

plugins/hosts/windows/scripts/utils/VagrantSSH/VagrantSSH.psm1

@@ -0,0 +1,26 @@
+# Vagrant SSH capability functions
+
+function Set-SSHKeyPermissions {
+    param (
+        [parameter(Mandatory=$true)]
+        [string] $SSHKeyPath,
+        [parameter(Mandatory=$false)]
+        [string] $Principal=$null
+    )
+
+    if(!$Principal) {
+        $Principal = [System.Security.Principal.WindowsIdentity]::GetCurrent().Name
+    }
+
+    # Create the new ACL we want to apply
+    $NewAccessRule = New-Object System.Security.AccessControl.FileSystemAccessRule(
+        $Principal, "FullControl", "None", "None", "Allow")
+    $ACL = Get-ACL "${SSHKeyPath}"
+    # Disable inherited rules
+    $ACL.SetAccessRuleProtection($true, $false)
+    # Scrub all existing ACLs from the file
+    $ACL.Access | %{$ACL.RemoveAccessRule($_)}
+    # Apply the new ACL
+    $ACL.SetAccessRule($NewAccessRule)
+    Set-ACL "${SSHKeyPath}" $ACL
+}

test/unit/plugins/communicators/ssh/communicator_test.rb

@@ -34,9 +34,12 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do
     double("machine",
       config: config,
       provider: provider,
-      ui: ui
+      ui: ui,
+      env: env
     )
   end
+  let(:env){ double("env", host: host) }
+  let(:host){ double("host") }
   # SSH information of the machine
   let(:machine_ssh_info){ {host: '10.1.2.3', port: 22} }
   # Subject instance to test
@@ -89,6 +92,10 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do
     allow(communicator).to receive(:retryable).and_return(connection)
   end
 
+  before do
+    allow(host).to receive(:capability?).and_return(false)
+  end
+
   describe ".wait_for_ready" do
     before(&connection_setup)
     context "with no static config (default scenario)" do
@@ -208,41 +215,14 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do
           expect(private_key_file).to receive(:write).with(new_private_key)
         end
 
-        it "should set private key file as user readable only" do
+        it "should call the set_ssh_key_permissions host capability" do
-          expect(private_key_file).to receive(:chmod).with(0600)
+          expect(host).to receive(:capability?).with(:set_ssh_key_permissions).and_return(true)
+          expect(host).to receive(:capability).with(:set_ssh_key_permissions, private_key_file)
         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" }
-
-          before do
-            allow(private_key_file).to receive(:to_s).and_return("PRIVATE_KEY_PATH")
-            allow(File).to receive(:set_permissions)
-            allow(Vagrant::Util::Platform).to receive(:windows?).and_return(true)
-            allow(Etc).to receive(:getlogin).and_return(owner)
-            stub_const('File::FULL', :full)
-          end
-
-          it "should get set new permissions on private key file" do
-            expect(File).to receive(:set_permissions).with("PRIVATE_KEY_PATH", any_args)
-          end
-
-          it "should proceed when error is encountered" do
-            expect(File).to receive(:set_permissions).and_raise(StandardError)
-          end
-
-          context "with multiple permissions on file" do
-
-            it "should delete all non-owner permissions" do
-              expect(File).to receive(:set_permissions).with("PRIVATE_KEY_PATH",
-                owner => :full)
-            end
-          end
-        end
       end
     end
   end

test/unit/plugins/hosts/bsd/cap/ssh_test.rb

@@ -0,0 +1,15 @@
+require_relative "../../../../base"
+
+require_relative "../../../../../../plugins/hosts/bsd/cap/ssh"
+
+describe VagrantPlugins::HostBSD::Cap::SSH do
+  let(:subject){ VagrantPlugins::HostBSD::Cap::SSH }
+
+  let(:env){ double("env") }
+  let(:key_path){ double("key_path") }
+
+  it "should set file as user only read/write" do
+    expect(key_path).to receive(:chmod).with(0600)
+    subject.set_ssh_key_permissions(env, key_path)
+  end
+end

test/unit/plugins/hosts/linux/cap/ssh_test.rb

@@ -0,0 +1,15 @@
+require_relative "../../../../base"
+
+require_relative "../../../../../../plugins/hosts/linux/cap/ssh"
+
+describe VagrantPlugins::HostLinux::Cap::SSH do
+  let(:subject){ VagrantPlugins::HostLinux::Cap::SSH }
+
+  let(:env){ double("env") }
+  let(:key_path){ double("key_path") }
+
+  it "should set file as user only read/write" do
+    expect(key_path).to receive(:chmod).with(0600)
+    subject.set_ssh_key_permissions(env, key_path)
+  end
+end

test/unit/plugins/hosts/windows/cap/ssh_test.rb

@@ -0,0 +1,38 @@
+require_relative "../../../../base"
+
+require_relative "../../../../../../plugins/hosts/windows/cap/ssh"
+
+describe VagrantPlugins::HostWindows::Cap::SSH do
+  let(:subject){ VagrantPlugins::HostWindows::Cap::SSH }
+  let(:result){ Vagrant::Util::Subprocess::Result.new(exit_code, stdout, stderr) }
+  let(:exit_code){ 0 }
+  let(:stdout){ "" }
+  let(:stderr){ "" }
+
+  let(:key_path){ double("keypath", to_s: "keypath") }
+  let(:env){ double("env") }
+
+  before do
+    allow(Vagrant::Util::PowerShell).to receive(:execute).and_return(result)
+  end
+
+  it "should execute PowerShell script" do
+    expect(Vagrant::Util::PowerShell).to receive(:execute).with(
+      /set_ssh_key_permissions.ps1/, "-KeyPath", key_path.to_s, any_args
+    ).and_return(result)
+    subject.set_ssh_key_permissions(env, key_path)
+  end
+
+  it "should return the result" do
+
+    expect(subject.set_ssh_key_permissions(env, key_path)).to eq(result)
+  end
+
+  context "when command fails" do
+    let(:exit_code){ 1 }
+
+    it "should raise an error" do
+      expect{ subject.set_ssh_key_permissions(env, key_path) }.to raise_error(Vagrant::Errors::PowerShellError)
+    end
+  end
+end

test/unit/vagrant/util/powershell_test.rb

@@ -135,6 +135,14 @@ describe Vagrant::Util::PowerShell do
       end
       described_class.execute("custom-command", env: {"TEST_KEY" => "test-value"})
     end
+
+    it "should define a custom module path" do
+      expect(Vagrant::Util::Subprocess).to receive(:execute) do |*args|
+        comm = args.detect{|s| s.to_s.include?("custom-command") }
+        expect(comm.to_s).to include("$env:PSModulePath+';C:\\My-Path'")
+      end
+      described_class.execute("custom-command", module_path: "C:\\My-Path")
+    end
   end
 
   describe ".execute_cmd" do
@@ -183,6 +191,15 @@ describe Vagrant::Util::PowerShell do
       described_class.execute_cmd("custom-command", env: {"TEST_KEY" => "test-value"})
     end
 
+    it "should define a custom module path" do
+      expect(Vagrant::Util::Subprocess).to receive(:execute) do |*args|
+        comm = args.detect{|s| s.to_s.include?("custom-command") }
+        expect(comm.to_s).to include("$env:PSModulePath+';C:\\My-Path'")
+        result
+      end
+      described_class.execute_cmd("custom-command", module_path: "C:\\My-Path")
+    end
+
     context "with command output" do
       let(:stdout){ "custom-output" }
 
@@ -246,6 +263,15 @@ describe Vagrant::Util::PowerShell do
       described_class.execute_inline("custom-command", env: {"TEST_KEY" => "test-value"})
     end
 
+    it "should define a custom module path" do
+      expect(Vagrant::Util::Subprocess).to receive(:execute) do |*args|
+        comm = args.detect{|s| s.to_s.include?("custom-command") }
+        expect(comm.to_s).to include("$env:PSModulePath+';C:\\My-Path'")
+        result
+      end
+      described_class.execute_inline("custom-command", module_path: "C:\\My-Path")
+    end
+
     it "should return a result instance" do
       expect(described_class.execute_inline("cmd")).to eq(result)
     end

vagrant.gemspec

@@ -28,8 +28,6 @@ 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"