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 - Add public key and key comment validation #57993

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@MaxBab
Copy link
Contributor

commented Jun 18, 2019

SUMMARY

Add public key and key comment validation

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

openssh_keypair

@ansibot

This comment has been minimized.

@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Maybe there should be a separate output field instead? A comment is not a public key.

@ansibot ansibot removed the needs_triage label Jun 18, 2019

@MaxBab

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Good point.
Will make the change.

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from c0d99ca to 26b9284 Jun 18, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

You'll also need a changelog fragment. Will look at the rest later.

@@ -240,6 +240,7 @@ def dump(self):
# On removal this has no value
'fingerprint': self.fingerprint[1] if self.fingerprint else '',
'public_key': self.public_key,
'comment': self.comment if self.comment else '',

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Jun 18, 2019

Contributor

Probably also needs documentation in the RETURN description around line 90.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jun 18, 2019

Author Contributor

Done

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from 26b9284 to 3c0a806 Jun 18, 2019

@ansibot ansibot removed the small_patch label Jun 18, 2019

@MaxBab MaxBab changed the title openssh_keypair - Add key comment to the pubkey output openssh_keypair - Add key comment to the key return values Jun 18, 2019

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from 3c0a806 to 75130f3 Jun 18, 2019

@MaxBab

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

You'll also need a changelog fragment. Will look at the rest later.

Done.

@felixfontein
Copy link
Contributor

left a comment

Looks good to me. As a new feature, this will have to wait for Ansible 2.9 though.

@@ -240,6 +245,7 @@ def dump(self):
# On removal this has no value
'fingerprint': self.fingerprint[1] if self.fingerprint else '',
'public_key': self.public_key,
'comment': self.comment if self.comment else '',

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jun 18, 2019

Contributor

You can also simplify this:

Suggested change
'comment': self.comment if self.comment else '',
'comment': self.comment or '',
@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Does this ever return an actual comment from the file? It seems to only print out the parameter that's given...

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Actually, it does not. The module is not idempotent w.r.t. comment; if it would be, this PR would be correct.

I guess making the module more idempotent would be a good thing? That should be outside of this PR, though, because that's a bug and should be backported. The module also does not check whether the public key actually belongs to the private key.

@MaxBab

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@felixfontein
Regarding the public key belongs to the private key.
Fix me if I'm wrong, but the public key is generated directly from the private key (lines 192 and 211), which actually approves the belongs of the public to the private key.

Do you think the addition check should be implemented?

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@MaxBab it is generated from it, but when private and public key are already there, you'd have to check whether they belong together for proper idempotence checking. The following checks / actions are missing:

  1. if public key doesn't exist, or doesn't fit to private key, regenerate public key from private key;
  2. if everything exists and is fine, but comment differs, change comment of public key.

In both cases, there's no need to regenerate the private key. In case the private key is regenerated, there's no need to check the public key, since it is regenerated as well.

I have no idea how complicated this is to implement these checks with ssh-keygen though...

@ansibot ansibot added the stale_ci label Jun 27, 2019

@MaxBab

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@felixfontein I'm started to work on your suggestion and would like to hear your thoughts regarding the following, please.

For this module we have 4 methods: generate, isValid, dump and remove.
I would like to discuss 2 first methods.

All the checks of the validation regarding the existence of the public key, matching of the public key content and same for the comment, should be performed within the "isValid" method. I suppose we agree on this.
My question is regarding the actions for the public key (regeneration if needed) and key comment (update if needed).

I'm not sure it should be located within the "isValid" method as it just returns the state of the checks and not performs any action.
But I'm not sure as well it should be located within the "generate" method as the method deals with the main module task - generation of the private key.

Maybe, creation of the "update" method will fit the logic of the public key and key comment actions?

What do you think?
Thanks.

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Hmm, I guess the best way is to refactor the module somehow :) The module copies what similar modules from that namespace are doing, but all these modules only work on one output file. This module has two output files.

How about the following: isValid does not return true/false, but three values - for private key has to be regenerated, for only public key has to be regenerated, and for nothing has to be regenerated. Or there are two functions, isPrivateKeyValid and isPublicKeyValid. First call the former, if that returns True, only then the latter.

@lolcube as the author of this module, do you have any preferences, or other ideas?

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from 75130f3 to f8ad695 Jul 4, 2019

@MaxBab

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Added "isPrivateKeyValid" and "isPublicKeyValid" functions.
The "generate" function is now acting depending on the state of the above functions.

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from f8ad695 to 61d954c Jul 4, 2019

@MaxBab MaxBab changed the title openssh_keypair - Add key comment to the key return values openssh_keypair - Add public key and key comment validation and return comment in the output Jul 4, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Cool! I'll take a look at it over the weekend.


if self.comment:
try:
args = [module.get_bin_path('ssh-keygen',

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 7, 2019

Contributor

Since this also overwrites the private key (which apparently also contains the comment), you need to also include

                if os.path.exists(self.path) and not os.access(self.path, os.W_OK):
os.chmod(self.path, stat.S_IWUSR + stat.S_IRUSR)

from the if not self.isPrivateKeyValid(module, perms_required=False) or self.force: branch.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 10, 2019

Author Contributor

Will do.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 18, 2019

Author Contributor

Done.

args = [module.get_bin_path('ssh-keygen',
True), '-q', '-c', '-C', self.comment, '-f', self.path]
module.run_command(args, data='y')
except IOError:

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 7, 2019

Contributor

Now you need to update self.public_key another time.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 10, 2019

Author Contributor

Why the self.public_key needs to be updated?
Only the comment is updated within the private and public keys.
The public key itself does not change.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 10, 2019

Contributor

Ah sorry, I got confused and thought the pubkey also contains the comment. It doesn't, so everything's fine!

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 10, 2019

Author Contributor

Wait. You should not be confused. you are right. The public key contains the comment. And of course, the file is updated with the new comment once it changed.

What I meant with the question, is that in the module, the return output contains only the public key output without the comment.
The comment output will be returned separately as we agreed.

@@ -0,0 +1,2 @@
minor_changes:
- openssh_keypair - add public key and key comment validation and return values

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 7, 2019

Contributor

It might be better to split this PR up into two parts:

  1. validate public key and comment and update if necessary; this is more like a bugfix and can be backported to 2.8.x;
  2. return comment; this is a new feature and has to wait for 2.9.0.

At least if you want the improved validation in 2.8.x, that is :)

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 10, 2019

Author Contributor

Will split.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 18, 2019

Author Contributor

Split done.
The key comment return will be introduced in another PR.

path: '{{ output_dir }}/privatekey7'
comment: 'test@privatekey7'
register: privatekey7_result

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 7, 2019

Contributor

Maybe add another test here:
a) copy {{ output_dir }}/privatekey7 and its public key to {{ output_dir }}/privatekey8;
b) run openssh_keypair again to change the comment to something else.

Then in verification, check whether the public key (and thus the private key) itself (i.e. without the comment) stayed the same, and that only the comment changed.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 10, 2019

Author Contributor

ack

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 18, 2019

Author Contributor

I added the following test.
Create a key with comment. Change the comment of the key.
Verify that the public key has not been changed.

Currently, I'm unable to test the comment as the return value will be in the other PR.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 18, 2019

Contributor

You could test it by looking at the output file, or by calling ssh-keygen yourself and extract the comment from its output.

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 18, 2019

Author Contributor

If I call ssh-keygen to show the public file, I will get just the public key without the comment.
I thought to add the test for the comment when the second PR (#59268) will get accepted.
What do you think?

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from 61d954c to acd92d1 Jul 18, 2019

@MaxBab MaxBab changed the title openssh_keypair - Add public key and key comment validation and return comment in the output openssh_keypair - Add public key and key comment validation Jul 18, 2019

@@ -0,0 +1,2 @@
bugfix:

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 18, 2019

Contributor
Suggested change
bugfix:
bugfixes:

Otherwise the tool won't accept this.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/57993-openssh_keypair-add-public-key-and-key-comment-validation.yml:0:0: invalid section: bugfix

click here for bot help

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from acd92d1 to 38eb612 Jul 18, 2019

assert:
that:
- privatekey7_result.public_key == privatekey7_modified_result.publick_key

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Jul 18, 2019

Contributor
Suggested change
- privatekey7_result.public_key == privatekey7_modified_result.publick_key
- privatekey7_result.public_key == privatekey7_modified_result.public_key

This comment has been minimized.

Copy link
@MaxBab

MaxBab Jul 18, 2019

Author Contributor

Thanks for catching.
Fixed.

openssh_keypair - Add public key and key comment validation
- Split the key validation to separate private and public.
- In case public key does not exist, recreate it.
- Validate comment of the key.

@MaxBab MaxBab force-pushed the MaxBab:add_ssh_comment_to_pubkey_output branch from 38eb612 to 5fc2e4c Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.