Skip to content

Conversation

@AnthyG-epias
Copy link

@AnthyG-epias AnthyG-epias commented Jun 26, 2025

Pull Request (PR) description

This Pull Request adds the required modifications to the module for it to be compatible with the Windows Capability ("Optional Feature") OpenSSH.Server.

It adds a section to the README.md with an example on how to set it up on Windows.

I decided to not add the additional installation and configuration steps to the module, because as I understand, the Windows Capability is not broadly available on every Windows Version. I believe it should be compatible with the chocolatey way of installation, but I have not tested this.

I had some problems with the windows specific permission / ownership system and I solved them by using the puppetlabs-acl puppet module, but looking at #225 it might not be necessary. But as it stands, it works, and I don't really want to invest more time unless I have more knowledge that can be easily applied. That is to say, I heartily welcome suggestions, tips, tricks, hints etc..

I actually did not see #225 up until today (when I was basically finished with my changes), and as some changes do seem to match 1to1, I consider them changes that should be merged regardless of the rest of my proposed changes. E.g. that file ownership is configurable, and that the already configurable sshd binary path is used for config file validation.

I have successfully tested my proposed changes on multiple freshly provisioned machines running Windows Server 2022.

This Pull Request (PR) fixes the following issues

Fixes #225

Things left to do

  • add descriptions for new parameters
  • update REFERENCE.md

@AnthyG-epias AnthyG-epias marked this pull request as ready for review June 30, 2025 11:57
@AnthyG-epias
Copy link
Author

how are these extra pipelines triggered, and can I run the necessary stuff locally instead of having to wait for the pipelines to run?

image

@saz
Copy link
Owner

saz commented Jul 2, 2025

@AnthyG-epias bundle exec rake spec should do it. See https://voxpupuli.org/docs/how_to_run_tests/ for helpful details.

@AnthyG-epias
Copy link
Author

anything left to do, e.g. additional tests that should exist before approve?

@saz saz force-pushed the windows-compatibility branch from b0bad5c to 01ad47b Compare October 28, 2025 06:33
@saz
Copy link
Owner

saz commented Oct 30, 2025

@AnthyG-epias Can you squash commits and rebase to the current master?

@saz
Copy link
Owner

saz commented Oct 30, 2025

@AnthyG-epias I've integrated some changes (#435 and #436) already. This should make your PR a bit easier and will only include changes related to Windows compatability.

windows-compatibility - remove mentions of `ssh::server::manage_config_permissions`

windows compatibility - fix tests

windows compatibility - review suggestions - update `REFERENCE.md`

windows compatibility - review suggestions - fix "selector inside resource block"

windows compatibility - review suggestions - fix "selector inside resource block"; proper indentation

windows compatibility - review suggestions - proper indentation

windows compatibility - review suggestions - inline variable where it's only used once

Co-authored-by: Steffen Zieger <[email protected]>

windows compatibility - review suggestions - demand string content

Co-authored-by: Steffen Zieger <[email protected]>

windows compatibility - review suggestions - remove unnecessary check

Co-authored-by: Steffen Zieger <[email protected]>

windows compatibility - apply review suggestions

Co-authored-by: Steffen Zieger <[email protected]>

windows compatibility - remove same-value copy-pasta from hiera data files

windows compatibility - update `REFERENCE.md`

windows compatibility - parameter docs

windows compatibility - fix permissions according to fresh install

windows compatibility - clean up

windows compatibility - fix `ssh::server::manage_config_permissions`

windows compatibility - update windows example

windows compatibility - make file ownership configurable

windows compatibility - let fact also parse on windows

windows compatibility - add compability support-statement to metadata.json

windows compatibility - make file ownership configurable; provide example for setup on windows
@AnthyG-epias AnthyG-epias force-pushed the windows-compatibility branch from 772cf6f to 5e2b5d6 Compare October 31, 2025 07:15
Comment on lines +404 to +405
ssh::server::config_user: null
ssh::server::config_group: null
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my original proposition, I had the additional option ssh::server::manage_config_permissions which allowed me to skip setting owner and group properties for the file and concat resources. @saz, are you able to tell me, whether it is possible to use null here instead? As far as I remember, the Microsoft OpenSSH Feature sets the required permissions itself, and overwriting them caused issues.

I might be able to test it next week, but I can't guarantee, because I'm currently working on other things. I'd also have to find out how to use puppet environments I guess.

  $config_file_ownership = $ssh::server::manage_config_permissions ? {
    false   => {},
    default => {
      owner => $ssh::server::config_user,
      group => $ssh::server::config_group,
    }
  }
    file { $ssh::server::include_dir:
      ensure  => directory,
      *       => $config_file_ownership,
      mode    => $ssh::server::include_dir_mode,
      purge   => $ssh::server::include_dir_purge,
      recurse => $ssh::server::include_dir_purge,
    }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've intentionally left this part out, as it wouldn't be required by any supported OS right now.

I suggest that this should be part of your PR.

For the testing part: I'm not able to help here, as I don't run Windows, but take your time to validate, that things are working properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants