Always generate a new key pair if the private key doesn't exist #598

diazona · 2023-04-29T23:48:37Z

SUMMARY

This PR updates KeypairBackend._should_generate() to first check if the original private key named by the path argument exists, and return True if it does not. This brings the code in line with the documentation, which says that a new key will always be generated if the key file doesn't already exist. It also modifies the existing tests to check for this bug.

Fixes #597

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

openssh_keypair

ADDITIONAL INFORMATION

Before this fix, using the sample playbook from #597:

$ ansible-playbook keypair.yml
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match
'all'

PLAY [localhost] ******************************************************************************************************

TASK [Gathering Facts] ************************************************************************************************
ok: [localhost]

TASK [Ensure key directory exists] ************************************************************************************
ok: [localhost]

TASK [Ensure id_ed25519 key does not exist] ***************************************************************************
ok: [localhost] => (item=id_ed25519)
ok: [localhost] => (item=id_ed25519.pub)

TASK [Generate keypair] ***********************************************************************************************
fatal: [localhost]: FAILED! => {"changed": false, "msg": "Key has wrong type and/or size. Will not proceed. To force regeneration, call the module with `generate` set to `partial_idempotence`, `full_idempotence` or `always`, or with `force=true`."}

PLAY RECAP ************************************************************************************************************
localhost                  : ok=3    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0

After the fix:

$ ansible-playbook keypair.yml
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match
'all'

PLAY [localhost] ******************************************************************************************************

TASK [Gathering Facts] ************************************************************************************************
ok: [localhost]

TASK [Ensure key directory exists] ************************************************************************************
ok: [localhost]

TASK [Ensure id_ed25519 key does not exist] ***************************************************************************
ok: [localhost] => (item=id_ed25519)
ok: [localhost] => (item=id_ed25519.pub)

TASK [Generate keypair] ***********************************************************************************************
changed: [localhost]

PLAY RECAP ************************************************************************************************************
localhost                  : ok=4    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

As an alternative to the approach implemented here, I also considered only modifying the condition in the fail branch of the if statement - this was @felixfontein's suggestion in a comment, and it was also my first thought about how to fix this - but I thought doing it the way I did would make it easier to check that the code is doing the right thing just by looking at it. That can certainly be changed if necessary.

I also considered doing something to make the logic more similar to PrivateKeyBackend.needs_regeneration() (the openssl version of this functionality), because the two are supposed to be acting the same way, but I thought that'd be going beyond the scope of just fixing this bug. If it'd be useful to make both methods work the same way, someone can refactor the code in a future commit.

felixfontein

Thanks for the PR! The code change looks good to me. Could you please add a changelog fragment? Thanks.

diazona · 2023-05-01T03:56:16Z

Done! Thanks for the pointer @felixfontein. I actually added two fragments for the two different commits, but I'm not sure if that's overkill; I can certainly remove one or combine them if you want.

Sorry I missed your comment until now.

changelogs/fragments/597-openssh_keypair-generate-new-key.yml

changelogs/fragments/597-openssh_keypair-test-key-creation.yml

…ble-collections#597) This commit updates `KeypairBackend._should_generate()` to first check if the original private key named by the `path` argument exists, and return True if it does not. This brings the code in line with the documentation, which says that a new key will always be generated if the key file doesn't already exist. As an alternative to the approach implemented here, I also considered only modifying the condition in the `fail` branch of the if statement, but I thought that would not map as cleanly to the behavior specified in the documentation, so doing it the way I did should make it easier to check that the code is doing the right thing just by looking at it. I also considered doing something to make the logic more similar to `PrivateKeyBackend.needs_regeneration()` (the openssl version of this functionality), because the two are supposed to be acting the same way, but I thought that'd be going beyond the scope of just fixing this bug. If it'd be useful to make both methods work the same way, someone can refactor the code in a future commit.

This commit changes the test task that generates new keys to use each of the different values for the `regenerate` argument, which will ensure that the module is capable of generating a key when no previous key exists regardless of the value of `regenerate`. Previously, the task would always run with the `partial_idempotence` value, and that obscured a bug (ansible-collections#597) that would occur when it was set to `fail`. The bug was fixed in the previous commit.

Ajpantuso

LGTM

felixfontein · 2023-05-01T19:16:57Z

@diazona thanks a lot for fixing this!
@Ajpantuso thanks a lot for reviewing!

patchback · 2023-05-01T19:17:07Z

Backport to stable-1: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-1/ce3299f106a7c3084a3b2a8b66be3755fe45c26f/pr-598

Backported as #599

🤖 @patchback
I'm built with octomachinery and
my source is open — https:/sanitizers/patchback-github-app.

* Always generate a new key pair if the private key doesn't exist (#597) This commit updates `KeypairBackend._should_generate()` to first check if the original private key named by the `path` argument exists, and return True if it does not. This brings the code in line with the documentation, which says that a new key will always be generated if the key file doesn't already exist. As an alternative to the approach implemented here, I also considered only modifying the condition in the `fail` branch of the if statement, but I thought that would not map as cleanly to the behavior specified in the documentation, so doing it the way I did should make it easier to check that the code is doing the right thing just by looking at it. I also considered doing something to make the logic more similar to `PrivateKeyBackend.needs_regeneration()` (the openssl version of this functionality), because the two are supposed to be acting the same way, but I thought that'd be going beyond the scope of just fixing this bug. If it'd be useful to make both methods work the same way, someone can refactor the code in a future commit. * Test different regenerate values with nonexistent keys This commit changes the test task that generates new keys to use each of the different values for the `regenerate` argument, which will ensure that the module is capable of generating a key when no previous key exists regardless of the value of `regenerate`. Previously, the task would always run with the `partial_idempotence` value, and that obscured a bug (#597) that would occur when it was set to `fail`. The bug was fixed in the previous commit. (cherry picked from commit ce3299f)

#599) * Always generate a new key pair if the private key doesn't exist (#597) This commit updates `KeypairBackend._should_generate()` to first check if the original private key named by the `path` argument exists, and return True if it does not. This brings the code in line with the documentation, which says that a new key will always be generated if the key file doesn't already exist. As an alternative to the approach implemented here, I also considered only modifying the condition in the `fail` branch of the if statement, but I thought that would not map as cleanly to the behavior specified in the documentation, so doing it the way I did should make it easier to check that the code is doing the right thing just by looking at it. I also considered doing something to make the logic more similar to `PrivateKeyBackend.needs_regeneration()` (the openssl version of this functionality), because the two are supposed to be acting the same way, but I thought that'd be going beyond the scope of just fixing this bug. If it'd be useful to make both methods work the same way, someone can refactor the code in a future commit. * Test different regenerate values with nonexistent keys This commit changes the test task that generates new keys to use each of the different values for the `regenerate` argument, which will ensure that the module is capable of generating a key when no previous key exists regardless of the value of `regenerate`. Previously, the task would always run with the `partial_idempotence` value, and that obscured a bug (#597) that would occur when it was set to `fail`. The bug was fixed in the previous commit. (cherry picked from commit ce3299f) Co-authored-by: David Zaslavsky <[email protected]>

diazona · 2023-05-02T06:53:49Z

And thank you for making the contribution process so smooth! 😄

felixfontein reviewed Apr 30, 2023

View reviewed changes

diazona force-pushed the 597-openssh-keypair-fail/1/dev branch from 10fbdd3 to 4577c21 Compare May 1, 2023 03:54

felixfontein reviewed May 1, 2023

View reviewed changes

changelogs/fragments/597-openssh_keypair-generate-new-key.yml Outdated Show resolved Hide resolved

changelogs/fragments/597-openssh_keypair-test-key-creation.yml Outdated Show resolved Hide resolved

diazona added 2 commits May 1, 2023 09:46

diazona force-pushed the 597-openssh-keypair-fail/1/dev branch from 87c304c to d88c323 Compare May 1, 2023 16:46

diazona requested a review from felixfontein May 1, 2023 16:50

Ajpantuso approved these changes May 1, 2023

View reviewed changes

felixfontein approved these changes May 1, 2023

View reviewed changes

felixfontein merged commit ce3299f into ansible-collections:main May 1, 2023

felixfontein added the backport-1 label May 1, 2023

patchback bot mentioned this pull request May 1, 2023

[PR #598/ce3299f1 backport][stable-1] Always generate a new key pair if the private key doesn't exist #599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always generate a new key pair if the private key doesn't exist #598

Always generate a new key pair if the private key doesn't exist #598

diazona commented Apr 29, 2023

felixfontein left a comment

diazona commented May 1, 2023

Ajpantuso left a comment

felixfontein commented May 1, 2023

patchback bot commented May 1, 2023 •

edited

Loading

diazona commented May 2, 2023

Always generate a new key pair if the private key doesn't exist #598

Always generate a new key pair if the private key doesn't exist #598

Conversation

diazona commented Apr 29, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

felixfontein left a comment

Choose a reason for hiding this comment

diazona commented May 1, 2023

Ajpantuso left a comment

Choose a reason for hiding this comment

felixfontein commented May 1, 2023

patchback bot commented May 1, 2023 • edited Loading

Backport to stable-1: 💚 backport PR created

diazona commented May 2, 2023

patchback bot commented May 1, 2023 •

edited

Loading