From 7ef4ae9e102122e93d28afee8ed78735f3fb9600 Mon Sep 17 00:00:00 2001 From: Matt Wrock Date: Sun, 24 Jan 2016 10:38:22 -0800 Subject: [PATCH 1/4] use NTLM/Negotiate authentication over basic authentication --- plugins/communicators/winrm/config.rb | 8 ++++++- plugins/communicators/winrm/shell.rb | 4 ++-- .../plugins/communicators/winrm/shell_test.rb | 15 ++++++++++++- vagrant.gemspec | 4 ++-- .../docs/vagrantfile/winrm_settings.html.md | 21 +++++++------------ 5 files changed, 32 insertions(+), 20 deletions(-) diff --git a/plugins/communicators/winrm/config.rb b/plugins/communicators/winrm/config.rb index 3387ca142..9c2d7fe84 100644 --- a/plugins/communicators/winrm/config.rb +++ b/plugins/communicators/winrm/config.rb @@ -12,6 +12,7 @@ module VagrantPlugins attr_accessor :transport attr_accessor :ssl_peer_verification attr_accessor :execution_time_limit + attr_accessor :basic_auth_only def initialize @username = UNSET_VALUE @@ -25,12 +26,13 @@ module VagrantPlugins @transport = UNSET_VALUE @ssl_peer_verification = UNSET_VALUE @execution_time_limit = UNSET_VALUE + @basic_auth_only = UNSET_VALUE end def finalize! @username = "vagrant" if @username == UNSET_VALUE @password = "vagrant" if @password == UNSET_VALUE - @transport = :plaintext if @transport == UNSET_VALUE + @transport = :negotiate if @transport == UNSET_VALUE @host = nil if @host == UNSET_VALUE is_ssl = @transport == :ssl @port = (is_ssl ? 5986 : 5985) if @port == UNSET_VALUE @@ -40,6 +42,7 @@ module VagrantPlugins @timeout = 1800 if @timeout == UNSET_VALUE @ssl_peer_verification = true if @ssl_peer_verification == UNSET_VALUE @execution_time_limit = "PT2H" if @execution_time_limit == UNSET_VALUE + @basic_auth_only = false if @basic_auth_only == UNSET_VALUE end def validate(machine) @@ -56,6 +59,9 @@ module VagrantPlugins unless @ssl_peer_verification == true || @ssl_peer_verification == false errors << "winrm.ssl_peer_verification must be a boolean." end + unless @basic_auth_only == true || @basic_auth_only == false + errors << "winrm.basic_auth_only must be a boolean." + end { "WinRM" => errors } end diff --git a/plugins/communicators/winrm/shell.rb b/plugins/communicators/winrm/shell.rb index 656a8b670..fefe5c9e8 100644 --- a/plugins/communicators/winrm/shell.rb +++ b/plugins/communicators/winrm/shell.rb @@ -181,7 +181,7 @@ module VagrantPlugins case @config.transport.to_sym when :ssl "https://#{@host}:#{@port}/wsman" - when :plaintext + when :plaintext, :negotiate "http://#{@host}:#{@port}/wsman" else raise Errors::WinRMInvalidTransport, transport: @config.transport @@ -193,7 +193,7 @@ module VagrantPlugins pass: @password, host: @host, port: @port, - basic_auth_only: true, + basic_auth_only: @config.basic_auth_only, no_ssl_peer_verification: !@config.ssl_peer_verification } end end #WinShell class diff --git a/test/unit/plugins/communicators/winrm/shell_test.rb b/test/unit/plugins/communicators/winrm/shell_test.rb index de7b1dc19..1ad92802e 100644 --- a/test/unit/plugins/communicators/winrm/shell_test.rb +++ b/test/unit/plugins/communicators/winrm/shell_test.rb @@ -14,6 +14,7 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do c.password = 'password' c.max_tries = 3 c.retry_delay = 0 + c.basic_auth_only = false c.finalize! end } @@ -69,7 +70,19 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do end end + context "when transport is :negotiate" do + it "should create winrm endpoint address using http" do + expect(subject.send(:endpoint)).to eq("http://localhost:5985/wsman") + end + end + context "when transport is :plaintext" do + let(:config) { + VagrantPlugins::CommunicatorWinRM::Config.new.tap do |c| + c.transport = :plaintext + c.finalize! + end + } it "should create winrm endpoint address using http" do expect(subject.send(:endpoint)).to eq("http://localhost:5985/wsman") end @@ -80,7 +93,7 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do it "should create endpoint options" do expect(subject.send(:endpoint_options)).to eq( { user: "username", pass: "password", host: "localhost", port: 5985, - basic_auth_only: true, no_ssl_peer_verification: false }) + basic_auth_only: false, no_ssl_peer_verification: false }) end end diff --git a/vagrant.gemspec b/vagrant.gemspec index 4e2e501d7..8db06ab35 100644 --- a/vagrant.gemspec +++ b/vagrant.gemspec @@ -29,8 +29,8 @@ Gem::Specification.new do |s| s.add_dependency "rb-kqueue", "~> 0.2.0" s.add_dependency "rest-client", ">= 1.6.0", "< 2.0" s.add_dependency "wdm", "~> 0.1.0" - s.add_dependency "winrm", "~> 1.3" - s.add_dependency "winrm-fs", "~> 0.2.2" + s.add_dependency "winrm", "~> 1.6" + s.add_dependency "winrm-fs", "~> 0.3.0" # We lock this down to avoid compilation issues. s.add_dependency "nokogiri", "= 1.6.3.1" diff --git a/website/source/docs/vagrantfile/winrm_settings.html.md b/website/source/docs/vagrantfile/winrm_settings.html.md index 80d7c806a..046d1854f 100644 --- a/website/source/docs/vagrantfile/winrm_settings.html.md +++ b/website/source/docs/vagrantfile/winrm_settings.html.md @@ -51,21 +51,14 @@ to use port 4567 to talk to the guest if there is no other option.
-`config.winrm.execution_time_limit` - The maximum duration that a WinRM -task can execute for. This defaults to two hours. The format of this value -must be in this [Microsoft-documented format](https://msdn.microsoft.com/en-us/library/aa382678.aspx). +`config.winrm.transport` - The transport used for WinRM communication. Valid settings include: `:negotiate`, `ssl`, and `:plaintext`. The default is `:negotiate`.
-Warning: In order for Vagrant to communicate with a Windows -guest, you must allow unencrypted WinRM connections on the guest machine -itself. Some public boxes already have this configured, but if you are -attempting to `vagrant up` a Windows box and the command hangs at -`Waiting for WinRM to become available...`, then you will need to run the -commands below on the guest machine itself, at the box setup stage, -after provisioning, or through a start up script. +`config.winrm.basic_auth_only` - Whether to use Basic Authentication. Defaults to `false`. If set to `true` you should also use the `:plaintext` transport setting and the Windows machine must be confiured appropriately. Note: It is strongly recommended that you only use basic authentication for debugging purposes. Credentials will be transferred in plain text. -``` -Set-Item WSMan:\localhost\Service\AllowUnencrypted -Value True -Set-Item WSMan:\localhost\Service\Auth\Basic -Value True -``` +
+ +`config.winrm.execution_time_limit` - The maximum duration that a WinRM +task can execute for. This defaults to two hours. The format of this value +must be in this [Microsoft-documented format](https://msdn.microsoft.com/en-us/library/aa382678.aspx). From 5b2566cd514a15aca00adb154079469896062dbf Mon Sep 17 00:00:00 2001 From: Matt Wrock Date: Sun, 24 Jan 2016 13:12:53 -0800 Subject: [PATCH 2/4] add note about potentially breaking change with uploading directories --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc2e3bd1d..331131dd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Next Version (unreleased) +BREAKING CHANGES: + + - The `winrm` communicator now shares the same upload behavior as the `ssh` cummunicator. This change should have no impact to most vagrant operations but may break behavior when uploading directories to an existing destination target. The `file` provisioner should be the only builtin provisioner affected by this change. When uploading a directory and the destination directory exists on the endpoint, the source base directory will be created below the destination directory on the endpoint and the source directory contents will be unzipped to that location. Prior to this release, the contents of the source directory would be unzipped to an existing destination directory without creating the source base directory. This new behavior is more consistent with SCP and other well known shell copy commands. + IMPROVEMENTS: - provisioners/chef: Add the ability to install on SUSE [GH-6806] From d3819d40bf8df8f79eea77597b1785ec1113bbeb Mon Sep 17 00:00:00 2001 From: Matt Wrock Date: Sun, 24 Jan 2016 13:14:15 -0800 Subject: [PATCH 3/4] pass winrm debug logging to vagrant logger --- plugins/communicators/winrm/shell.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/communicators/winrm/shell.rb b/plugins/communicators/winrm/shell.rb index fefe5c9e8..1a8d7906d 100644 --- a/plugins/communicators/winrm/shell.rb +++ b/plugins/communicators/winrm/shell.rb @@ -170,6 +170,7 @@ module VagrantPlugins client = ::WinRM::WinRMWebService.new(endpoint, @config.transport.to_sym, endpoint_options) client.set_timeout(@config.timeout) + client.logger = @logger client end From f912a81362140a72e6be7c3dd85f48994ea6c4d6 Mon Sep 17 00:00:00 2001 From: Matt Wrock Date: Sun, 24 Jan 2016 23:33:16 -0800 Subject: [PATCH 4/4] powershell and cmd calls should use commnand_executor to reuse oprn winrm shell --- plugins/communicators/winrm/communicator.rb | 1 + plugins/communicators/winrm/shell.rb | 74 +++++++++---------- .../plugins/communicators/winrm/shell_test.rb | 43 +++++++---- 3 files changed, 65 insertions(+), 53 deletions(-) diff --git a/plugins/communicators/winrm/communicator.rb b/plugins/communicators/winrm/communicator.rb index e22a4f9b0..fe884d272 100644 --- a/plugins/communicators/winrm/communicator.rb +++ b/plugins/communicators/winrm/communicator.rb @@ -142,6 +142,7 @@ module VagrantPlugins opts[:good_exit] = Array(opts[:good_exit]) command = wrap_in_scheduled_task(command, opts[:interactive]) if opts[:elevated] + @logger.debug("#{opts[:shell]} executing:\n#{command}") output = shell.send(opts[:shell], command, &block) execution_output(output, opts) end diff --git a/plugins/communicators/winrm/shell.rb b/plugins/communicators/winrm/shell.rb index 1a8d7906d..f55c8fb75 100644 --- a/plugins/communicators/winrm/shell.rb +++ b/plugins/communicators/winrm/shell.rb @@ -54,20 +54,21 @@ module VagrantPlugins end def powershell(command, &block) - # Suppress the progress stream from leaking to stderr - command = "$ProgressPreference='SilentlyContinue';\r\n" + command - command << "\r\n" # Ensure an exit code - command << "if ($?) { exit 0 } else { if($LASTEXITCODE) { exit $LASTEXITCODE } else { exit 1 } }" - execute_shell(command, :powershell, &block) + command += "\r\nif ($?) { exit 0 } else { if($LASTEXITCODE) { exit $LASTEXITCODE } else { exit 1 } }" + execute_with_rescue(executor.method("run_powershell_script"), command, &block) end def cmd(command, &block) - execute_shell(command, :cmd, &block) + execute_with_rescue(executor.method("run_cmd"), command, &block) end def wql(query, &block) - execute_shell(query, :wql, &block) + retryable(tries: @config.max_tries, on: @@exceptions_to_retry_on, sleep: @config.retry_delay) do + handle_output(session.method("run_wql"), query, &block) + end + rescue => e + raise_winrm_exception(e, "run_wql", query) end def upload(from, to) @@ -82,42 +83,35 @@ module VagrantPlugins protected - def execute_shell(command, shell=:powershell, &block) - raise Errors::WinRMInvalidShell, shell: shell unless [:powershell, :cmd, :wql].include?(shell) - - begin - execute_shell_with_retry(command, shell, &block) - rescue => e - raise_winrm_exception(e, shell, command) - end + def execute_with_rescue(method, command, &block) + handle_output(method, command, &block) + rescue => e + raise_winrm_exception(e, method.name, command) end - def execute_shell_with_retry(command, shell, &block) - retryable(tries: @config.max_tries, on: @@exceptions_to_retry_on, sleep: @config.retry_delay) do - @logger.debug("#{shell} executing:\n#{command}") - output = session.send(shell, command) do |out, err| - block.call(:stdout, out) if block_given? && out - block.call(:stderr, err) if block_given? && err - end + def handle_output(execute_method, command, &block) + output = execute_method.call(command) do |out, err| + block.call(:stdout, out) if block_given? && out + block.call(:stderr, err) if block_given? && err + end - @logger.debug("Output: #{output.inspect}") + @logger.debug("Output: #{output.inspect}") - # Verify that we didn't get a parser error, and if so we should - # set the exit code to 1. Parse errors return exit code 0 so we - # need to do this. - if output[:exitcode] == 0 - (output[:data] || []).each do |data| - next if !data[:stderr] - if data[:stderr].include?("ParserError") - @logger.warn("Detected ParserError, setting exit code to 1") - output[:exitcode] = 1 - break - end + # Verify that we didn't get a parser error, and if so we should + # set the exit code to 1. Parse errors return exit code 0 so we + # need to do this. + if output[:exitcode] == 0 + (output[:data] || []).each do |data| + next if !data[:stderr] + if data[:stderr].include?("ParserError") + @logger.warn("Detected ParserError, setting exit code to 1") + output[:exitcode] = 1 + break end end - - return output end + + return output end def raise_winrm_exception(exception, shell = nil, command = nil) @@ -178,6 +172,10 @@ module VagrantPlugins @session ||= new_session end + def executor + @executor ||= session.create_executor + end + def endpoint case @config.transport.to_sym when :ssl @@ -195,7 +193,9 @@ module VagrantPlugins host: @host, port: @port, basic_auth_only: @config.basic_auth_only, - no_ssl_peer_verification: !@config.ssl_peer_verification } + no_ssl_peer_verification: !@config.ssl_peer_verification, + retry_delay: @config.retry_delay, + retry_limit: @config.max_tries } end end #WinShell class end diff --git a/test/unit/plugins/communicators/winrm/shell_test.rb b/test/unit/plugins/communicators/winrm/shell_test.rb index 1ad92802e..5cf1f6810 100644 --- a/test/unit/plugins/communicators/winrm/shell_test.rb +++ b/test/unit/plugins/communicators/winrm/shell_test.rb @@ -6,7 +6,8 @@ require Vagrant.source_root.join("plugins/communicators/winrm/config") describe VagrantPlugins::CommunicatorWinRM::WinRMShell do include_context "unit" - let(:session) { double("winrm_session") } + let(:session) { double("winrm_session", create_executor: executor) } + let(:executor) { double("command_executor") } let(:port) { config.transport == :ssl ? 5986 : 5985 } let(:config) { VagrantPlugins::CommunicatorWinRM::Config.new.tap do |c| @@ -15,6 +16,8 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do c.max_tries = 3 c.retry_delay = 0 c.basic_auth_only = false + c.retry_delay = 1 + c.max_tries = 2 c.finalize! end } @@ -27,23 +30,12 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do describe ".powershell" do it "should call winrm powershell" do - expect(session).to receive(:powershell).with(/^dir.+/).and_return({ exitcode: 0 }) + expect(executor).to receive(:run_powershell_script).with(/^dir.+/).and_return({ exitcode: 0 }) expect(subject.powershell("dir")[:exitcode]).to eq(0) end - it "should retry when a WinRMAuthorizationError is received" do - expect(session).to receive(:powershell).with(/^dir.+/).exactly(3).times.and_raise( - # Note: The initialize for WinRMAuthorizationError may require a status_code as - # the second argument in a future WinRM release. Currently it doesn't track the - # status code. - WinRM::WinRMAuthorizationError.new("Oh no!! Unauthrorized") - ) - expect { subject.powershell("dir") }.to raise_error( - VagrantPlugins::CommunicatorWinRM::Errors::AuthenticationFailed) - end - it "should raise an execution error when an exception occurs" do - expect(session).to receive(:powershell).with(/^dir.+/).and_raise( + expect(executor).to receive(:run_powershell_script).with(/^dir.+/).and_raise( StandardError.new("Oh no! a 500 SOAP error!")) expect { subject.powershell("dir") }.to raise_error( VagrantPlugins::CommunicatorWinRM::Errors::ExecutionError) @@ -52,11 +44,29 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do describe ".cmd" do it "should call winrm cmd" do - expect(session).to receive(:cmd).with("dir").and_return({ exitcode: 0 }) + expect(executor).to receive(:run_cmd).with("dir").and_return({ exitcode: 0 }) expect(subject.cmd("dir")[:exitcode]).to eq(0) end end + describe ".wql" do + it "should call winrm wql" do + expect(session).to receive(:run_wql).with("select * from Win32_OperatingSystem").and_return({ exitcode: 0 }) + expect(subject.wql("select * from Win32_OperatingSystem")[:exitcode]).to eq(0) + end + + it "should retry when a WinRMAuthorizationError is received" do + expect(session).to receive(:run_wql).with("select * from Win32_OperatingSystem").exactly(2).times.and_raise( + # Note: The initialize for WinRMAuthorizationError may require a status_code as + # the second argument in a future WinRM release. Currently it doesn't track the + # status code. + WinRM::WinRMAuthorizationError.new("Oh no!! Unauthrorized") + ) + expect { subject.wql("select * from Win32_OperatingSystem") }.to raise_error( + VagrantPlugins::CommunicatorWinRM::Errors::AuthenticationFailed) + end + end + describe ".endpoint" do context 'when transport is :ssl' do let(:config) { @@ -93,7 +103,8 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do it "should create endpoint options" do expect(subject.send(:endpoint_options)).to eq( { user: "username", pass: "password", host: "localhost", port: 5985, - basic_auth_only: false, no_ssl_peer_verification: false }) + basic_auth_only: false, no_ssl_peer_verification: false, + retry_delay: 1, retry_limit: 2 }) end end