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

openssh_keypair: Adding passphrase parameter #225

Merged

Conversation

Ajpantuso
Copy link
Collaborator

@Ajpantuso Ajpantuso commented May 6, 2021

SUMMARY

Added passphrase paramter to openssh_keypair for encrypting/decrypting OpenSSH private keys.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/openssh_keypair
plugins/module_utils/openssh/cryptography_openssh

ADDITIONAL INFORMATION

openssh_keypair

  • Uses cryptography instead of ssh_keygen to avoid passing passphrases over command line
  • Maintains consistent formatting with ssh_keygen which changed in version 7.8
  • Ensures idempotency checks behave as documented when using passphrase
  • Return values are consistent with parsed ssh_keygen output
  • passphrase is used as a switch so if empty or not supplied behavior will be the same as before this feature (Some more lines are included in try blocks, but are really just variable assignments)

cryptography_openssh

  • Exposes SSH private key formatting capability as a global
  • Added option to load a lone private key and automatically generate the corresponding public key
  • Added option to explicitly select private key format
  • Added logic to reattempt loading a PEM formatted private key if the OpenSSH loader fails
  • Made it possible to set null comments
  • Added "SHA256:" prefix to fingerprint

Integration tests have been updated as well.

Unfortunately the following containers have system packages of cryptography less than what is required to replicate the installed OpenSSH versions and the local pip versions are not able to install an updated version.

For these containers currently only the test cases which do not use the passphrase option are executed.

  • CentOS 6
  • CentOS 8
  • openSUSE 15 py3
  • Ubuntu 18.04
  • RHEL 8.3
  • FreeBSD 12.1
  • FreeBSD 12.2
  • FreeBSD 13.0

@Ajpantuso Ajpantuso force-pushed the openssh_keypair_passphrase branch from 4a80b08 to 0a97d10 Compare May 7, 2021 21:18
@Ajpantuso Ajpantuso marked this pull request as ready for review May 7, 2021 21:52
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! I really like it :)

What do you think of (probably more for a follow-up PR ;) ) adding backends to openssh_keypair, similar to the cryptography vs pyopenssl backends for some of the openssl_* modules? Assuming that the code for the cryptography backend would only rely on cryptography and no longer would need ssh-keygen.

plugins/modules/openssh_keypair.py Outdated Show resolved Hide resolved
@Ajpantuso
Copy link
Collaborator Author

Thanks for this contribution! I really like it :)

What do you think of (probably more for a follow-up PR ;) ) adding backends to openssh_keypair, similar to the cryptography vs pyopenssl backends for some of the openssl_* modules? Assuming that the code for the cryptography backend would only rely on cryptography and no longer would need ssh-keygen.

Freeing this module from ssh-keygen should be possible given it's current feature set.
I've researched other libraries while doing this and cryptography stands alone in terms of the OpenSSH specific features which are separate from the core OpenSSL library. And even then there are still gaps for example no support for generating SSH certificates which is critical for the openssh_cert module. If we maintained custom implementations of the OpenSSH formats that would open things up considerably....but then we would be stuck maintaining custom implementations of the OpenSSH formats :(

@felixfontein
Copy link
Contributor

Freeing this module from ssh-keygen should be possible given it's current feature set.

Great!

I've researched other libraries while doing this and cryptography stands alone in terms of the OpenSSH specific features which are separate from the core OpenSSL library. And even then there are still gaps for example no support for generating SSH certificates which is critical for the openssh_cert module.

Well, I'd already be happy if only the openssh_keypair module has a cryptography backend and openssh_cert has not. I think there are a lot more people interested in openssh_keypair than there are in openssh_cert.

(openssl_pkcs12 is another module without cryptography backend BTW, since PKCS12 support in cryptography is very, very basic right now, and only available since 3.0 IIRC.)

If we maintained custom implementations of the OpenSSH formats that would open things up considerably....but then we would be stuck maintaining custom implementations of the OpenSSH formats :(

We definitely do not want to maintain that :-)

plugins/modules/openssh_keypair.py Outdated Show resolved Hide resolved
plugins/modules/openssh_keypair.py Outdated Show resolved Hide resolved
plugins/modules/openssh_keypair.py Show resolved Hide resolved
plugins/modules/openssh_keypair.py Outdated Show resolved Hide resolved
plugins/modules/openssh_keypair.py Outdated Show resolved Hide resolved
plugins/modules/openssh_keypair.py Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be at some point moved to tests/integration/targets/setup_bcrypt/ so it can be used by other integration tests as well. No need to do that in this PR though :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sorry was just pushing through issues with CI so there's definitely cleanup work to be done.

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Tell me whether you want to do more work on the PR right now, or in a follow-up PR.

@Ajpantuso
Copy link
Collaborator Author

Tell me whether you want to do more work on the PR right now, or in a follow-up PR.

I'm good to go with this PR for now.

@felixfontein felixfontein merged commit 6100d9b into ansible-collections:main May 10, 2021
@felixfontein
Copy link
Contributor

@Ajpantuso thanks for working on this! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants