Skip to content
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

MODULES-11100 - Add sk-ecdsa public key support, and implement tests for sk-ecdsa and ecdsa keys #388

Merged
merged 4 commits into from
Aug 10, 2021

Conversation

vollmerk
Copy link
Contributor

@vollmerk vollmerk commented Jun 14, 2021

CHANGE:

  • Adds sk-ecdsa- as a valid prefix for SSH public keys fixing support for those key types
    ADD:
  • Adds sk-ecdsa- and ecdsa Key tests

This implements new key types available in OpenSSH 8.2+

@vollmerk vollmerk requested a review from a team as a code owner June 14, 2021 17:24
@CLAassistant
Copy link

CLAassistant commented Jun 14, 2021

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

accounts_ssh_authorized_keys_line_parser is a function

Breaking changes to this file MAY impact these 1 modules (near match):

This module is declared in 3 of 576 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@sheenaajay
Copy link
Contributor

Thanks @vollmerk for your contribution.
The tests are failing with following error on puppet 6 on all platforms. Could you please take a look. Thank you.
Error: Parameter type failed on Ssh_authorized_key[hunner_sk-ecdsa-sha2-nistp256@openssh.com_vagrant4_437c5e7d82ed0e3a33f0991e7de0e102]: Invalid value "sk-ecdsa-sha2-nistp256@openssh.com". Valid values are ssh-dss, ssh-rsa, ecdsa-sha2-nistp256, ecdsa-sha2-nistp384, ecdsa-sha2-nistp521, ssh-ed25519. (file: /etc/puppetlabs/code/environments/production/modules/accounts/manifests/manage_keys.pp, line: 50)

@vollmerk
Copy link
Contributor Author

Thanks @vollmerk for your contribution.
The tests are failing with following error on puppet 6 on all platforms. Could you please take a look. Thank you.
Error: Parameter type failed on Ssh_authorized_key[hunner_sk-ecdsa-sha2-nistp256@openssh.com_vagrant4_437c5e7d82ed0e3a33f0991e7de0e102]: Invalid value "sk-ecdsa-sha2-nistp256@openssh.com". Valid values are ssh-dss, ssh-rsa, ecdsa-sha2-nistp256, ecdsa-sha2-nistp384, ecdsa-sha2-nistp521, ssh-ed25519. (file: /etc/puppetlabs/code/environments/production/modules/accounts/manifests/manage_keys.pp, line: 50)

Yep, saw this, this morning. SK keys aren't supported in older versions of SSH so I'll have to look into only testing the SK keys on openSSH versions that support them.

@sheenaajay
Copy link
Contributor

Thank you @vollmerk for the quick response.

@daianamezdrea
Copy link
Contributor

Hi @vollmerk, any updates on this PR? If you need help, let us know! Thank you

@vollmerk
Copy link
Contributor Author

Hi @vollmerk, any updates on this PR? If you need help, let us know! Thank you

Partially haven't gotten back to this, but also not sure exactly what you folks want test wise. There were incomplete tests before, so this could be fixed by just not testing the -sk keys. Or limiting the tests to modern OpenSSH versions.... or upgrade the tests to only test modern OpenSSH version... not sure which way you want to go on that.

@daianamezdrea
Copy link
Contributor

Hi @vollmerk, if you can restrict the tests to run only on supported version we will get clean run on PR level testing

@vollmerk
Copy link
Contributor Author

vollmerk commented Aug 4, 2021

Hi @vollmerk, if you can restrict the tests to run only on supported version we will get clean run on PR level testing

Gave it a shot, awaiting workflow run. It looks like the Puppet7 tests run on a new enough system that it works, so I just checked for pup version to determine key test

@daianamezdrea
Copy link
Contributor

@vollmerk, I approved the running, let's see how the tests are going

@daianamezdrea
Copy link
Contributor

Looks good, only a few rubocop errors, do you mind if you fix them? Or I can fix it

@vollmerk
Copy link
Contributor Author

vollmerk commented Aug 9, 2021

Looks good, only a few rubocop errors, do you mind if you fix them? Or I can fix it

You'll likely be able to fix them faster than I can, thanks!

@adrianiurca adrianiurca merged commit 92abac8 into puppetlabs:main Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants