MODULES-10550 fix keyspec parsing to allow whitespaces in options and…

[janit42]

… comments

move parsing of the keyspec param to a new ruby function
accounts_ssh_authorized_keys_line_parser.rb and change manage_keys.pp to use
that function to fix issues with whitespaces inside the options part.

Uses the relatively well defined "keytype" to anchor the match instead
of trying to squeeze the whole keyspec line into one regex (which causes several
issues discussed in the pull request for 59ce4f8). Gets the options from
an rstriped substring derived from the position of the match.
The new parsing works with whitespaces inside both, the options and the comment
part of a keyspec and behaves more similar to openssh itself now.

Some limitations discussed in the pr for 59ce4f8 still remain:· e.g. strings
like 'ssh-' or 'ecdsh-' inside the options or comment could still break the
parsing.

Updates tests in accounts_user_spec.rb to contain more meaningful checks as
the new parser wouldn't accept the old simplified keyspecs '1 2 3' and '2 3 4'

Introduces spec tests for the new parser containing a few complex examples
taken from pr 59ce4f8 discussion, the authorized_keys manpage and the wild

[janit42]

I fixed some of the warnings generated by rubocop in e276389, but the remaining ones may be a bit more tricky: rubocop is complaining about commas without a trailing whitespace, but these are inside the keyspec string presenting an ssh authorized_keys line and whitespaces after a comma is forbidden by ssh.

Also the authorized_key examples lines are rather long (>200 characters). Trying to break them into several lines might make the code difficult to understand. There might be a rubocop linter problem complaining about putting a regex into %r{}, as this should be done only when trying to match a "/" - which is not the case here as the slashes just enclose the regex.

Would be happy for some advice here.

[david22swan]

@janit42 Hey just took a quick look, sorry for the wait. In regards to the rubocop issues that you are seeing, %r{} has just been declared a rule for our modules, rather than swapping it in when needed we keep it as the default option as we've found it to be neater overall.
As for the remaining errors, it look's like a disable for the rules in question would be your best choice if you cannot get them to match the rules in a neat fashion, just add the below snippet to your code and it will take care of it:

# rubocop:disable Layout/SpaceAfterComma, Metrics/LineLength
[...]
# rubocop:enable Layout/SpaceAfterComma, Metrics/LineLength

It's not something that we encourage but we do recognize that it is sometimes necessary.
Anyway I hope that helps :)

[janit42]

@david22swan Thanks for your suggestions! I reorganized the code a bit so disabling rubocop LineLength checks to keep the code readable for humans is only used for one spec test statement.

On my local machine with puppet 5 the changes now pass rubocop and spectests, but checks via github/travis with puppet 6 are still unsuccessful. These might be unrelated to my PR and rather coupled to the ssh_authorized_key resource type moving out of the puppet code with puppet 6? But I may be wrong ...

Please let me know what I should do to proceed with the PR.

As I'm new to contributing via github: should I throw the current branch away and resubmit the whole thing including the style fixes as one new commit?

[david22swan]

@janit42 Thanks for the good work. We've currently got some failures on the module itself, so I can't merge right now but once they are resolved I think I would be happy to do so.

[david22swan]

@janit42 Apologies it took so long to get back to this. I've taken another look at your code and a minor problem has come up. Your changes are causing some unit test failures on Puppet 6 with Unknown resource type: 'ssh_authorized_key'resource type. Taking a close look I can't see where the failure is coming from but I did find that it is fixed by adding the requisite module to the fixtures.

[janit42]

@david22swan thanks again for guiding me through this! I added the puppetlabs-sshkeys_core module to the .fixtures.yaml for puppet6 after reading how to use fixtures, rebased against the current master and squashed my changes into one commit.

Getting a bit further now, spec tests are passing, but still not there. Could you lent me a hand again?

[daianamezdrea]

Hi @janit42, it was a timed-out error, I rerunnned the test and now it's all green! Nice work, thank you! Cheers!