openssh_keypair - Add password protected key check #64436

MaxBab · 2019-11-05T10:42:26Z

SUMMARY

The ssh key may be created manually prior the task execution with a
passphrase. And the task will be executed on the same key.

The module will check the private key and if the key is password
protected, the task will fail with the following message:
"The key is protected with a passphrase. Unable to proceed."

"Fixes #63910"

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

openssh_keypair

ansibot · 2019-11-05T10:43:46Z

cc @MarkusTeufelberger @Shaps @Spredzy @Xyon @felixfontein @lolcube @puiterwijk @resmo
click here for bot help

felixfontein · 2019-11-08T22:15:23Z

test/integration/targets/openssh_keypair/tasks/main.yml

+  openssh_keypair:
+    path: '{{ output_dir }}/privatekey8'
+  register: privatekey8_result
+  ignore_errors: true


Maybe add another test that in case force: yes is specified, the module won't fail.

felixfontein · 2019-11-08T22:15:30Z

@MaxBab did you have any chance to look at the failing CI tests so far? It looks like that for some versions of ssh-keygen, your change cannot distinguish between a totally broken file (in this case, a file of zero bytes) and a passphrase-protected file.

MaxBab · 2019-11-09T04:23:40Z

@felixfontein
Yes, I have noticed that the problem in the version of ssh.
I wrote the fix on the fedora os and the version of the openssh - openssh-7.9p1-6.
After I looked at the CI results I tested the fix on the rhel 7.7 os and it indeed failed. The version of openssh is - openssh-7.4p1-16.

The problem between the versions is that in the older version, the command I'm trying to get the error message with, still ask for the password and ignores the "SSH_ASKPASS=/bin/false" I'm using (line 244).

Right now, I can't think of any other way to check if the key is protected with the password.
I can't base my decision on the empty key because it could be a broken key and as a result should be regenerated.

I'm looking for a way to skip the password ask as it works in the new version and catch the error message that will tell that the password is incorrect.

I'm still looking for some way to do it, but currently, it keeps stuck on the password prompt.
Do you have any ideas, please?

MaxBab · 2019-11-17T12:24:57Z

@felixfontein
I have fixed the CI failed tests.
I changed the logic of the module regarding the broken keys in addition to the password protected.

The module will check the private key and if the key is password
protected or broken, the task will fail with the following message:
"Unable to read the key. The key is protected with a passphrase or broken. Unable to proceed."
The check of the ssh key performed by retrieves the public key from the private key.

In order to regenerate the broken key, "force" flag should be used.
The module changes the logic of execution with broken key.
Before the change, the broken key was regenerated without any ask.
Now, the broken key will be regenerated only if "force" flag is specified.

felixfontein · 2019-11-17T21:21:30Z

@MaxBab

The module will check the private key and if the key is password
protected or broken, the task will fail with the following message:
"Unable to read the key. The key is protected with a passphrase or broken. Unable to proceed."
The check of the ssh key performed by retrieves the public key from the private key.

In order to regenerate the broken key, "force" flag should be used.
The module changes the logic of execution with broken key.
Before the change, the broken key was regenerated without any ask.
Now, the broken key will be regenerated only if "force" flag is specified.

This is different than what all other crypto modules do: they regenerate automatically for broken keys (or if password doesn't match). I'm not sure whether it is a good idea to change the behavior in this way. How about simply re-generating the key in this case? Clearly the key isn't what the user expects (or should expect :D) from the module.

Are there other opinions on this?

MaxBab · 2019-11-18T10:55:02Z

@felixfontein

Let me describe to you the flow of the password-protected key check and how it involved the same behavior on the broken key as well.
Maybe we could think of a way to work around it and keep the broken key behavior flow untouched.

The old format of the ssh keys has an "ENCRYPTED" value within the private key, specifying that the private key is encrypted with a password.
The new format of the keys doesn't have this indication.

As a result, the only way for me to check if the ssh key is password-protected is to try to retrieve the public key from the private key by the following command: "ssh-keygen -P '' -yf /tmp/sshkey".
In the case of the password-protected key, I get a message, that the passphrase is incorrect. In the other case, I get the public key printed.

That's my way of checking the key.
What happens with the broken key, is that I actually get the same error telling me that the passphrase is incorrect. I have no other way to check if the key is broken. At least I didn't find it.

felixfontein · 2019-11-20T21:15:42Z

Ok, so we in general cannot distinguish between broken and passphrase protected. So how about always regenerating when one of these is the case? That solution will work in any case, and the module would behave the same as the other crypto modules.

MaxBab · 2019-12-01T13:36:32Z

@felixfontein
Agree.
Updated the patch.
Also, I added a note about this.

felixfontein

Looks good!

Could you please re-add the logic switch here? https://github.com/ansible/ansible/compare/02509801d9c7ed724435317344e6ee6d396f38ec..4fbdd671c4bf47f3075e32dd92c25a4f616544fd#diff-438137003e3ebd7c7c25344bd149a95eL174 (i.e. check self.force before calling self.isPrivateKeyValid()) This will make the module more future-proof. (In case there ever is another problem with isPrivateKeyValid, users can at least still force regeneration with force: yes instead of having to resort to calling ssh-keygen manually.)

* The ssh key may be created manually prior the task execution with a passphrase. And the task will be executed on the same key. * The ssh key may be broken and not usable. The module will check the private key and if the key is password protected or broken, it will be overridden. The check of the ssh key performed by retrieve the public key from the private key. Set the "self.force" check before the "isPrivateKeyValid" check. In case of any issue with the "isPrivateKeyValid" function, the user will be able to force the regeneration of the key with the "force: yes" argument.

MaxBab · 2019-12-02T07:00:56Z

Done.

felixfontein

LGTM

felixfontein · 2019-12-02T07:13:04Z

@MaxBab thanks a lot for fixing this! I'll create backports so the fix will find its way into 2.8.x and 2.9.x as well.

MaxBab · 2019-12-02T07:13:53Z

Cool.
Thanks for the help.

ansible#64436) * The ssh key may be created manually prior the task execution with a passphrase. And the task will be executed on the same key. * The ssh key may be broken and not usable. The module will check the private key and if the key is password protected or broken, it will be overridden. The check of the ssh key performed by retrieve the public key from the private key. Set the "self.force" check before the "isPrivateKeyValid" check. In case of any issue with the "isPrivateKeyValid" function, the user will be able to force the regeneration of the key with the "force: yes" argument. (cherry picked from commit da73bbd)

ansible#64436) * The ssh key may be created manually prior the task execution with a passphrase. And the task will be executed on the same key. * The ssh key may be broken and not usable. The module will check the private key and if the key is password protected or broken, it will be overridden. The check of the ssh key performed by retrieve the public key from the private key. Set the "self.force" check before the "isPrivateKeyValid" check. In case of any issue with the "isPrivateKeyValid" function, the user will be able to force the regeneration of the key with the "force: yes" argument.

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch 2 times, most recently from b713f36 to 5b79339 Compare November 5, 2019 10:50

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 5, 2019

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch 8 times, most recently from 6e8fab8 to 1c641f4 Compare November 5, 2019 14:31

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Nov 7, 2019

ansibot removed the needs_triage Needs a first human triage before being processed. label Nov 7, 2019

felixfontein reviewed Nov 8, 2019

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 17, 2019

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch from 1c641f4 to 56ab774 Compare November 17, 2019 09:31

ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Nov 17, 2019

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch from 56ab774 to 322ec29 Compare November 17, 2019 09:46

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch 2 times, most recently from 49d716a to 0250980 Compare November 17, 2019 12:09

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 17, 2019

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 28, 2019

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch from 0250980 to 4fbdd67 Compare December 1, 2019 12:52

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 1, 2019

felixfontein approved these changes Dec 1, 2019

View reviewed changes

MaxBab force-pushed the openssh_keypair_add_pass_protected_check branch from 4fbdd67 to 71662e2 Compare December 2, 2019 06:50

felixfontein approved these changes Dec 2, 2019

View reviewed changes

felixfontein merged commit da73bbd into ansible:devel Dec 2, 2019

MaxBab deleted the openssh_keypair_add_pass_protected_check branch December 2, 2019 07:14

This was referenced Dec 2, 2019

[DO NOT MERGE] [2.8] openssh_keypair - Add password protected key check #65406

Closed

[DO NOT MERGE] [2.9] openssh_keypair - Add password protected key check #65407

Closed

Public Ansible Project Meeting Agenda - December 2019 ansible/community#507

Closed

felixfontein mentioned this pull request Dec 8, 2019

openssh_keypair - do fail instead of regenerate for password protected and invalid keys #65638

Merged

ansible locked and limited conversation to collaborators Jan 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openssh_keypair - Add password protected key check #64436

openssh_keypair - Add password protected key check #64436

MaxBab commented Nov 5, 2019

ansibot commented Nov 5, 2019

felixfontein Nov 8, 2019

felixfontein commented Nov 8, 2019

MaxBab commented Nov 9, 2019

MaxBab commented Nov 17, 2019

felixfontein commented Nov 17, 2019

MaxBab commented Nov 18, 2019

felixfontein commented Nov 20, 2019

MaxBab commented Dec 1, 2019

felixfontein left a comment

MaxBab commented Dec 2, 2019

felixfontein left a comment

felixfontein commented Dec 2, 2019

MaxBab commented Dec 2, 2019

openssh_keypair - Add password protected key check #64436

openssh_keypair - Add password protected key check #64436

Conversation

MaxBab commented Nov 5, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibot commented Nov 5, 2019

felixfontein Nov 8, 2019

Choose a reason for hiding this comment

felixfontein commented Nov 8, 2019

MaxBab commented Nov 9, 2019

MaxBab commented Nov 17, 2019

felixfontein commented Nov 17, 2019

MaxBab commented Nov 18, 2019

felixfontein commented Nov 20, 2019

MaxBab commented Dec 1, 2019

felixfontein left a comment

Choose a reason for hiding this comment

MaxBab commented Dec 2, 2019

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented Dec 2, 2019

MaxBab commented Dec 2, 2019