Skip to content

Add Windows support using Chocolatey and Microsoft OpenSSH #225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

jadestorm
Copy link

Howdy! So this is a pretty huge patch but the gist of it is -- it adds Windows support to the module. There are a couple of Caveats I want to get out of the way right off the bat (and I tried to address in the README):

  1. The OpenSSH for Windows project is still in heavy development (https://github.com/PowerShell/Win32-OpenSSH)
  2. My implementation of installing it makes use of Chocolatey and assumes you have already included that class and module. The Chocolatey package in question is: https://chocolatey.org/packages/openssh

To properly support Windows, I had to rework the way file permissions and ownership happen, so there's a number of changes related to that. I love the feel of your module and wanted to make it function with OpenSSH under Windows for my own purposes. I am running this live currently and it's been working like a charm.

Please let me know if you have any questions, disagreements with the implementation, or anything. =)

@saz
Copy link
Owner

saz commented Jan 14, 2018

First of all, thanks for this pull request.

As I'll release one last version of this module which is supporting puppet 3 and then continue to migrate to puppet 4 and newer, I think it makes sense to clean this pull request up a little bit after the puppet 4 changes.

What do you think?

@jadestorm
Copy link
Author

No problem at all! Do you want to just keep this PR open until you are ready for me to patch it up? I should check back to see what changes they've done to openssh under windows since I did this as well -- they might have made things easier to work with.

@saz
Copy link
Owner

saz commented Jan 16, 2018 via email

@saz
Copy link
Owner

saz commented Jun 3, 2018

@jadestorm If you're willing to update this pull request, please let me know. I've merged all changes to finally get rid of puppet 3 support.

@jadestorm
Copy link
Author

Sure! It might be a little bit but I'll wrap back around to it. =)

@jadestorm
Copy link
Author

@saz OK I'm finally getting back around to this, sorry for the delay. Let me prefix by saying I think how I handled some of this is rather ugly so I'd love some comments -- even if they are "that's dumb do it this way" ;) Anyway I'll comment after I've made sure things look decently sane and tested locally to basically pick back up our conversation!

@jadestorm
Copy link
Author

Going to note these here before I head out for the day. These are some things I'd like some specific input on:

  • What do you think about the way I did file modes? I tried to make it easily extendable but really -- Windows is the only one that's "different".
  • I really don't like the newline thing I had to do, but if I didn't do that, the config file got created without the \r's in there and the powershell/openssh implementation did not read it properly.
  • I didn't put chocolatey in as a dependency because it's only if you are messing about with the Windows support -- it's not been clear to me whether you should put optional dependencies in metadata.json -- what do you think?

I'll do some testing tomorrow to make sure it still works as expected though.

@jadestorm
Copy link
Author

Re: the second item up there -- this is related to https://tickets.puppetlabs.com/browse/PUP-8240 which appears to be fixed in 5.5.1. (I haven't tested it yet, I will shortly) If we can get away with not having to do that crap, I'll rip it out completely and just put a note in the readme that you need to have puppet 5.5.1 or above for windows support or the config files won't work.

@jadestorm
Copy link
Author

Well that was an adventure, but it works under Windows now and has stopped failing travis-ci tests.

Going to see if I can remove the newline stuff to clean things up a little.

@jadestorm
Copy link
Author

Allrighty, thankfully the newline thing is indeed fixed. (I tested with Puppet 5.5.3) So I removed all that and put a note in the README that 5.5.1 or greater is a requirement.

So at this point I think I'm ready for you to give feedback or accept if you are happy with it. =) Thanks @saz !

@jadestorm
Copy link
Author

One little side interesting note I wanted to mention. (one of my coworkers just got back from windows hied) Looks like OpenSSH server will be a more official part of Windows Server 2019. I'll look at adapting this accordingly when the time comes. =)

@jadestorm
Copy link
Author

@saz I can update this again to not conflict with the current code -- but before I do that I'd like to know if you are ok with the path I'm taking with it. If there are things you would like to require me to change, I'd like to change them now as part of the update process.

@jadestorm
Copy link
Author

@saz Well I have a need to fix it anyway -- and I am going to refactor what I did a bit to something a bit cleaner. Not entirely happy with how it came out with the hash of perms. I'll be in touch once I have this fixed back up.

@jadestorm
Copy link
Author

@saz Ok! All fixed up and I went with a different fileperms specification model that I think is a bit cleaner/less weird at least. I also included a couple of OS X tweaks. If you really want the OS X tweaks in another PR I can separate them out, but I'd rather them just come along for the ride if it's not too much trouble. I also cleaned up the commits into one single easy to follow one. (besides things went .. weird... in the github compare before I did that) Note: I actively use this and can certainly be handy to help maintain the windows side of things. If windows related issues come up you can always @ me and I'll take a look.

On the other hand if you simply don't want to be responsible for dealing with Windows in the module -- I can always keep it as a separate fork and publish it up on puppetforge with nods to you and aiming to keep it in sync.

@saz
Copy link
Owner

saz commented Nov 9, 2019

Sorry for the late response.
I've reviewed the PR and there is one thing left:
Can you please change file owner and group to 0 if it's currently set to root?

@jadestorm
Copy link
Author

Hi @saz -- done!

@fe80
Copy link

fe80 commented Mar 18, 2020

Up ? :)

@djalai
Copy link

djalai commented Mar 18, 2020

hi @saz if you've got a little time for this one, it would be much appreciated.

@jadestorm
Copy link
Author

Hi folk, I have decided to go a different route for Windows configuration management. As such, I am no longer going to keep this PR open as I won't be available to do anything to tweak it. However, if anyone else would like to take my changes and run with them and submit your own PR, please feel free! I am closing this.

@jadestorm jadestorm closed this Apr 10, 2020
@fe80
Copy link

fe80 commented Apr 15, 2020

Hi @saz it's possible to reopen this MR for merge ? Or you need a new Merge Request ?

@jadestorm
Copy link
Author

@fe80 It will not merge as-is I'm afraid. Will need a little bit of work to catch back up with master. Someone will need to submit a new merge request if they want to pursue this.

@AnthyG-epias AnthyG-epias mentioned this pull request Jun 26, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants