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_cert: Implement use_agent option to get signing key from ssh-agent #117

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

dougstanley
Copy link
Contributor

SUMMARY

Implements an additional optional boolean argument to the openssh_cert module called use_agent which simply passes the additional -U flag to ssh-keygen to tell it to look for the signing key in the ssh-agent.

Fixes #116

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

openssh_cert

ADDITIONAL INFORMATION

@MarkusTeufelberger
Copy link
Contributor

Please add some integration tests for this mode.

@felixfontein felixfontein changed the title Implement use_agent option to get signing key from ssh-agent. openssh_cert: Implement use_agent option to get signing key from ssh-agent Oct 6, 2020
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.

You also need to add a changelog fragment.

plugins/modules/openssh_cert.py Show resolved Hide resolved
@dougstanley
Copy link
Contributor Author

Thanks for the feedback. I'll add the tests and changelog fragment and update the pull request.

dougstanley added a commit to dougstanley/community.crypto that referenced this pull request Oct 9, 2020
@dougstanley
Copy link
Contributor Author

So, it would seem that signing a cert using a CA stored in the ssh-agent was a feature added in openssh 7.6 and so isn't available on some older linux distros (it seems ubuntu 16.04 and centos 7 and older are the main ones).
Does that make this feature addition a no-go, or is there a way to specify that the feature is only available on certain hosts? I could make the parameter ignored or cause an error immediately on hosts that don't support it. If there are community standards for dealing with these situations, I apologize, this is my first attempt at a PR for anything ansible.

@MarkusTeufelberger
Copy link
Contributor

So far we tried to support the lowest common denominator (otherwise we would have dropped pyOpenSSL a long time ago, or support for running these modules under Python 2.6). RHEL 7 (CentOS keeps roughly the same timeline) will be supported until end of June 2024 according to https://access.redhat.com/support/policy/updates/errata/

There likely is a way to check for this feature and then act accordingly, but ideally it would be something that can be bacported in some other way and used on Ubuntu Xenial or CentOS 7 as well, so there's not too much feature disparity.

OTOH some features (e.g. handling certain key types, such as ed25519 ones) are only available using the cryptography backend which is not available on some distros.

More problematic might be that at least according to https://linux.die.net/man/1/ssh-keygen the -U switch that you use is already in use for uploading keys into a smartcard reader.

As a way forward I guess you can think of a way how you would expect this to behave on e.g. Ubuntu Xenial and what the impact would be in a typical use case (e.g. should the signing key even ever be transferred onto a remote machine?). You could for example argue that the signing should mostly take place on the Ansible control machine, not some host out there and it might be easier for users to upgrade their workstation rather than all their servers.

@felixfontein
Copy link
Contributor

I think the most important part is that the user gets a helpful error message if used with a too old ssh-keygen version, and that it doesn't accidentally do something different. That -U is used for different things with different ssh-keygen versions is worrysome.

I checked out the git history of openssh-portable. The use of -U for smartcards was removed in openssh/openssh-portable@7ea845e (so it looks like that was done for version 5.4), and the use of -U for gpg-agent was added in openssh/openssh-portable@a98339e (and this was apparently done for version 7.6).

With that information, it should be possible to implement a version test. Unfortunately, ssh-keygen itself seems to be incapable of outputting its version, so ssh -V needs to be called.

dougstanley added a commit to dougstanley/community.crypto that referenced this pull request Oct 12, 2020
dougstanley added a commit to dougstanley/community.crypto that referenced this pull request Oct 13, 2020
dougstanley added a commit to dougstanley/community.crypto that referenced this pull request Oct 14, 2020
@dougstanley
Copy link
Contributor Author

I have added tests and also put in a version check so that if some one attempts to use the use_agent parameter, it will return an error stating that the minimum ssh version is 7.6. I also put version checks on all of the tests so if the tests don't run where they're guaranteed to fail.

Please let me know if anything doesn't look right. I tried to make sure I kept all the changes similar to how other things were implemented.

Also, is there a standard way to test for a known failure? In the tests, I did one where I expected a failure and just used ignore_errors on it, and followed it with an assert to make sure that it failed as expected. Let me know if there's a better way to do that. I'm also wondering if I should have a test to make sure that it fails appropriately IF the ssh version is too old.

@felixfontein
Copy link
Contributor

Also, is there a standard way to test for a known failure? In the tests, I did one where I expected a failure and just used ignore_errors on it, and followed it with an assert to make sure that it failed as expected. Let me know if there's a better way to do that. I'm also wondering if I should have a test to make sure that it fails appropriately IF the ssh version is too old.

When the module failed, you can check result.msg for the error message. See for example https:/ansible-collections/community.crypto/blob/main/tests/integration/targets/openssh_keypair/tasks/main.yml#L139-L158

plugins/modules/openssh_cert.py Outdated Show resolved Hide resolved
plugins/module_utils/crypto/support.py Outdated Show resolved Hide resolved
plugins/modules/openssh_cert.py Outdated Show resolved Hide resolved
tests/integration/targets/openssh_cert/tasks/main.yml Outdated Show resolved Hide resolved
@felixfontein felixfontein mentioned this pull request Oct 14, 2020
10 tasks
@dougstanley dougstanley force-pushed the sshcertagent branch 2 times, most recently from 277b2fe to c5dad4f Compare October 15, 2020 23:42
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.

Looks good! Just two details :)

changelogs/fragments/117-openssh_cert-use-ssh-agent.yml Outdated Show resolved Hide resolved
tests/integration/targets/openssh_cert/tasks/main.yml Outdated Show resolved Hide resolved
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.

LGTM

@MarkusTeufelberger what do you think?

@MarkusTeufelberger
Copy link
Contributor

Yeah, looks good to me. Thanks for implementing the feature @dougstanley!

@felixfontein felixfontein merged commit b32adcc into ansible-collections:main Oct 19, 2020
@felixfontein
Copy link
Contributor

@dougstanley thanks a lot for implementing this, especially with tests :)
@MarkusTeufelberger thanks a lot for reviewing!

@dougstanley
Copy link
Contributor Author

Thanks @felixfontein and @MarkusTeufelberger for your help!

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.

openssh_cert: Make it possible to use a signing key stored in ssh-agent
3 participants