Adding ability to specify custom ssh_key location

[ggeldenhuis]

This PR is not meant to be merged immediately but rather to kick of a discussion about the added functionality. It is currently missing spec tests but is otherwise properly functioning albeit undocumented.

In an ideal world the implementation should happen in the user class and the various but of ssh key functionality should be moved out of home_dir class. This implementation however achieves the functionality required with the minimum of changes.

In my PR I have made the assumption that when purge_sshkeys are set to true that the intent is to delete all user related keys. Thus if the user has specified a custom ssh key location, unspecified keys in the custom location and the user's home directory will be removed. I am not sure if it is worthwhile to offer a differentiated ability to purge custom and purge home directory. A potential alternative is to only purge custom specified directory.

I created PUP-8982 as a result of my testing. Additionally the ssh_authorized_key resource does not allow for changing the ownership of the sshkey file other than to the user for whom the key is being generated. This is a false assumption if you are storing keys in a different location and specifying AuthorizedKeysFile, AuthorizedKeysCommand and AuthorizedKeysCommandUser in sshd_config. Specifying these values in sshd_config allows you to centralise user ssh keys and disallow the user from modifying them.

[ggeldenhuis]

The second commit attempts to clean up the code somewhat by putting ssh key management in a separate defined resource. It also adds some example code and updates the readme to detail usage. Spec test is still absent unfortunately.

[alex-harvey-z3q]

@ggeldenhuis Note this line here.

[ggeldenhuis]

Revising my initial comment, I do believe now that this PR could be merged in terms of completeness.

[pmcmaw]

Ran on internal CI with no issues.

[pmcmaw]

Hey @ggeldenhuis

There are a few minor changes I would like to suggest, other than that your changes look great and we would love to get this functionality added to our module.

1. As there is no validation would it be possible to add Strict Types (Introduced in Puppet4) for the values defined in accounts::key_management. For Example: Optional[String|Boolean sshkey_custom_path = undef

2. Can you add in information for the README to inform the user that if purge_sshkeys` is set to true and that there isn't a custom path set all keys will be purged

3. Would it be possible to add an example below the README entry you have added just showing directly in the README how you would use a manifest to make use of this feature (just take a snippet from the example manifests you have created).

Thank you for creating this PR :-)

[ggeldenhuis]

@pmcmaw would happy affix the changes, I just wanted to query the Strict Types syntax. No other parameter currently has strict type checking. I was thinking of perhaps sticking to older methods for now and the a separate refactor of the whole module to make use of strict type checking and moving it to puppet5.

[pmcmaw]

Just thinking any new features it would be great if they had the strict type checking as all the other manifests use validation functions to ensure that they are using the correct types. Also something to note is that puppet5 doesn't really have any large language changes.

[ggeldenhuis]

@pmcmaw I have done the following changes:

1. Added strict type checking for the new variable.
2. Added an example to the README
3. Clarified the behaviour of the module when setting purge to true.

sshkeys will not be removed from the custom path if not set, since that implies puppet searching through the whole file system looking for keys in every file.

sshkeys will also not be removed from the user's home directory if a custom path is set and purge_keys are set to true. Only keys from the custom path will be removed. I did consider modifying the behaviour to remove keys from both the user's home directory and the custom path but in the end that breaks expected behaviour, since you now remove keys from a location which you did not intend to manage. It could perhaps be an additional flag in the future.

[ggeldenhuis]

I tried Optional[Stdlib::Unixpath] $sshkey_custom_path = undef but that breaks the checks. I was expecting stdlib to be included but it does not seem to work.

[pmcmaw]

Merging.
Thank you for taking the time to contribute 👍