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

ssh: allow all additional ssh key types #60128

Merged
merged 8 commits into from
Oct 20, 2021

Conversation

MEschenbacher
Copy link
Contributor

What does this PR do?

Propose a fix for #59429 by extending the list of allowed ssh public key types in salt/modules/ssh.py as of openssh 8.5.

What issues does this PR fix or reference?

Fixes: #59429

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@MEschenbacher MEschenbacher requested a review from a team as a code owner May 4, 2021 18:27
@MEschenbacher MEschenbacher requested review from xeacott and removed request for a team May 4, 2021 18:27
@MEschenbacher MEschenbacher force-pushed the allow_allsshkeytypes branch from d904837 to 3cecf4c Compare May 4, 2021 18:40
@sagetherage sagetherage added the Silicon v3004.0 Release code name label Aug 12, 2021
@xeacott
Copy link
Contributor

xeacott commented Aug 12, 2021

Hey @MEschenbacher are you still wanting to make more changes to this PR? We can get this into Si just need to keep moving along

@MEschenbacher
Copy link
Contributor Author

The PR should be complete as far as changes to code are concerned. Are you fine with the changes themselves?

I'm assuming in order to move along, I need to check all boxes?

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

This will require a changelog file.

@Ch3LL Ch3LL requested a review from dwoz August 12, 2021 19:12
@MEschenbacher
Copy link
Contributor Author

Okay, I'm now waiting for the final call as to whether we allow any pubkey type or continue maintaining an allowlist of types.

@dwoz
Copy link
Contributor

dwoz commented Aug 25, 2021

Okay, I'm now waiting for the final call as to whether we allow any pubkey type or continue maintaining an allowlist of types.

Continue maintaining a list for now please.

@MEschenbacher MEschenbacher force-pushed the allow_allsshkeytypes branch 5 times, most recently from 60c0d85 to 6bc50ea Compare August 27, 2021 18:22
@MEschenbacher
Copy link
Contributor Author

Maintaining an allow list for now and rebased on master.

I'm not sure if https://github.com/saltstack/salt/blob/master/salt/states/ssh_auth.py and https://github.com/saltstack/salt/blob/master/salt/states/ssh_auth.py need some care, too. The regex look to be only allowing ssh- and ecdsa- but not sk-ssh- and sk-ecdsa-.

@MEschenbacher MEschenbacher changed the title modules/ssh.py: allow all additional ssh key types WIP: modules/ssh.py: allow all additional ssh key types Aug 27, 2021
@MEschenbacher MEschenbacher force-pushed the allow_allsshkeytypes branch 2 times, most recently from f7cbae2 to 8e51968 Compare August 28, 2021 21:03
@xeacott
Copy link
Contributor

xeacott commented Aug 30, 2021

I think the debian test is the last one to address here, and there's only one, and then we should be good to go!

Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

Pre-commit and tests need to be fixed still.

@MEschenbacher
Copy link
Contributor Author

I've added a few tests and I'm not sure if they assert that all salt modules and states with a ssh key type accept the additional key types. As far as the failing test for debian jinja filters is concerned: need to look into it.

@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 22, 2021

The jinja filters tests is a know failing test that someone else is working on and is not related to this PR. The pre-commit and lint test failures will need to be cleaned up though.

@Ch3LL Ch3LL added Phosphorus v3005.0 Release code name and version and removed Silicon v3004.0 Release code name labels Sep 22, 2021
@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 30, 2021

I went ahead and pushed a fix for pre-commit and lint. Just waiting on the test results.

@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 30, 2021

@MEschenbacher is this still WIP? Or is it ready for review and merge?

@MEschenbacher MEschenbacher changed the title WIP: modules/ssh.py: allow all additional ssh key types ssh: allow all additional ssh key types Oct 3, 2021
@MEschenbacher
Copy link
Contributor Author

Ready for review and merge.

@Ch3LL Ch3LL requested a review from dwoz October 5, 2021 15:50
@Ch3LL Ch3LL merged commit 0bf0ec4 into saltstack:master Oct 20, 2021
@welcome
Copy link

welcome bot commented Oct 20, 2021

Congratulations on your first PR being merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] not all openssh public key authentication types are supported
5 participants