@sneal a few questions before I make other updates:

• no_ssl_peer_verification or ssl_peer_verification? I think it makes sense as a flag "--no-ssl-peer-verification", but I don't like the double negative of no_ssl_peer_verification: false. It is what the winrm gem calls it, though.
• Right now it forwards two ports - one for http and one for https. I don't think the winrm communicator can (or should) use both, so I think it might make more sense to just forward one, though that would mean a reload is necessary to switch between http/https.

Any thoughts on the questions above or comments in the OP?

I'm glad to see this, thanks.

I see no need to keep the naming convention of options consistent with the winrm gem, its an implementation detail that most users won't be aware of. I'm not a fan of 'no_ssl_peer_verification', I believe its only that way in the winrm gem because the other existing options start with 'no_'.

I would rather just forward one port.

Using config.winrm.transport = :ssl over a boolean leaves the door open in the future for other transports 👍

Great! I think that answers all my questions, so hopefully I can get all the changes done soon.

FYIs:

• I thought about supporting the other options knife supports, especially since it's mostly just passing thru to WinRM. But I think that's better for a later PR (esp. since I'm not sure how to test a lot of those options)
• I heard that WinRM work is also underway in test-kitchen.

@maxlinc and @sneal thanks for following up on this!

@pdericson No problem. @sneal if you merge my changes you can squash my commits, but make sure not to squash them w/ @pdericson's so he gets credit for the original work ;)

@sneal - I think I've addressed most of my comments except for forwarding two ports instead of one. Please checkout the changes and let me know if anythings looks funky. I've just been trusting the unit tests so far, but I'm going to test it with real WinRM connections for VirtualBox and/or vagrant-rackspace soon.

@sneal Also, I cleaned up shell.rb/config.rb to be more DRY - they both specified default values, and the timeout didn't match. I've got it set to 60 now... can you confirm the intended default timeout in seconds?

@sneal I think it's functional now. I tested it with vagrant-rackspace. There are some usability issues to workout, though. I noted two:

1. Whenever I use winrm (with SSL but no peer verification) it prints out:
   at depth 0 - 20: unable to get local issuer certificate multiple times. WinRM still works, so it just seems to be a warning from OpenSSL. It'd be great if someone else could test and see if this happens in other environments. I'm on OSX.

2. Error handling is bad. Bad credentials are retried repeatedly, and connection issues (like failed SSL cert validation or a connection timeout) just show up as:

   Guest-specific operations were attempted on a machine that is not
   ready for guest communication. This should not happen and a bug
   should be reported.

Any tips on the error handling? I opened WinRb/WinRM#78 because I think it might be easier if we had direct access to the HTTP response code.

@maxlinc As an idea for error handling: communicators get called with this: https://github.com/mitchellh/vagrant/blob/a0a46bfdfa27593e3924b62c96c45a07c71f1cf4/plugins/communicators/ssh/communicator.rb#L38

WinRM doesn't implement it because Vagrant has a sane default. But if you implement that, you can basically catch "auth failed 3 times in a row, credentials are probably just wrong" sort of errors if you can extract that detail from WinRM.

Let me know when you want a deeper review.

@sneal I've pretty much got this behaving the way I want, but I'm going to need your help to finish the error handling. There's two issues:

• Figuring out what's "stable" for WinRM. The error classes are different on the master and the pre-release gem (v1.3.0.dev.2). One uses status_code and the other uses response_code.
• Confirming what exceptions I can expect WinRM to throw.

Take a look at #raise_winrm_exception, which is where WinRM exceptions are being converted to Vagrant exceptions in order to display error messages and decide if they're retriable or not.

There's also a lot of similarities between SSH and WinRM exception handling, but I'd rather finish up WinRM specs first then refactor to de-duplicate the logic.

@mitchellh this is ready for deeper review. I split the SSL & error handling into separate PRs to make it easier. I see you already commented on the error handling changes in #4943.

The only issue I'm aware of is the pre-release gem, as already noted on #4943. Just waiting for WinRM 1.3.0...

P.s. While I've got your attention do you think you could enable the travis-hook for vagrant-rackspace?

@maxlinc Branch won't cleanly merge

Yeah... that's why I don't like long-lived branches (though I understand why it was necessary).

I'll try to rebase...

Fixed. It was just a trivial conflict with the version for winrm (1.3.0 vs 1.3 - I've used your choice, 1.3, which is more correct).

WOMM. @maxlinc did you want to update the test before I merge this? Its just a nit, not a requirement.

Disabling http on the guest box (leaving https enabled) while leaving the transport the default of :plaintext, it hung Vagrant. Probably existing behavior. INFO retryable: Retryable exception raised: #<HTTPClient::KeepAliveDisconnected: HTTPClient::KeepAliveDisconnected: >