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.

Sign up for GitHub

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

Jump to bottom

Add comment parameter to openssl_publickey #56149

Closed
wants to merge 1 commit into from
Closed

Add comment parameter to openssl_publickey #56149

wants to merge 1 commit into from
+15 −0

Conversation

nkakouros
Copy link
Contributor

SUMMARY

This adds a new parameter to the openssl_publickey module that allows the user to have a comment at the end of the public key, similar to the -C option of ssh-keygen.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

openssl_publickey

@ansibot
Copy link
Contributor

ansibot commented May 6, 2019

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

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 community_review In order to be merged, this PR must follow the community review workflow. crypto Crypto community (ACME, openssl, letsencrypt) feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. 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 May 6, 2019
felixfontein
felixfontein suggested changes May 7, 2019
View reviewed changes
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.

First of all, how should this look like for PEM (OpenSSL) keys?

Then:

  • Idempotency checks are missing.
  • You should add integration tests for this.
  • You should add some validations, like disallowing \n in comments for OpenSSH keys. Also, what should be returned in absence of comments? Currently you're returning undefined.

lib/ansible/modules/crypto/openssl_publickey.py
description:
- A comment to append to the public key
type: str
version_added: "2.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version_added: "2.8"
version_added: "2.9"

No new features are accepted for Ansible 2.8.

lib/ansible/modules/crypto/openssl_publickey.py
@@ -288,6 +299,9 @@ def dump(self):
if self.backup_file:
result['backup_file'] = self.backup_file

if self.pubkey_comment:
result['comment'] = self.pubkey_comment
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to document this in return values if you return it.

lib/ansible/modules/crypto/openssl_publickey.py
@@ -214,6 +221,10 @@ def generate(self, module):

if self.backup:
self.backup_file = module.backup_local(self.path)

if self.pubkey_comment is not None:
publickey_content = publickey_content + (" " + self.pubkey_comment).encode('utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this only makes sense for OpenSSH keys. For OpenSSL PEM keys, you don't want to append text in the same line directly after ----- END PUBLIC KEY -----.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label May 7, 2019
@ansibot 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 May 16, 2019
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label May 23, 2019
@mattclay
Copy link
Member

bot_status

@ansibot
Copy link
Contributor

ansibot commented May 23, 2019

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

lib/ansible/modules/crypto/openssl_publickey.py:0:0: E309 version_added for new option (comment) should be '2.9'. Currently StrictVersion ('2.8')

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented May 23, 2019

Components

lib/ansible/modules/crypto/openssl_publickey.py
support: community
maintainers: MarkusTeufelberger Shaps Spredzy Xyon felixfontein puiterwijk resmo

Metadata

waiting_on: tterranigma
changes_requested_by: null
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: unstable
shippable_status: failure
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@felixfontein
Copy link
Contributor

@nkakouros are you still around?

@nkakouros
Copy link
Contributor Author

@felixfontein Yes, but I am really swamped right now.. I would like to address the issues but I don't see this happening before September.

@felixfontein
Copy link
Contributor

No problem! Just a little warning: if this isn't merged before 2.9's Feature Freeze (currently scheduled for August 29th), it will have to wait for Ansible 2.10.

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Aug 24, 2019
@ansibot ansibot added collection Related to Ansible Collections work collection:community.crypto needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md labels Apr 29, 2020
@ansibot
Copy link
Contributor

ansibot commented Aug 16, 2020

Thank you very much for your interest in Ansible. Ansible has migrated much of the content into separate repositories to allow for more rapid, independent development. We are closing this issue/PR because this content has been moved to one or more collection repositories.

For further information, please see:
https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md

@ansibot ansibot closed this Aug 16, 2020
@ansible ansible locked and limited conversation to collaborators Sep 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@felixfontein felixfontein felixfontein requested changes

Assignees
No one assigned
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bot_closed ci_verified Changes made in this PR are causing tests to fail. collection:community.crypto collection Related to Ansible Collections work crypto Crypto community (ACME, openssl, letsencrypt) feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nkakouros @ansibot @mattclay @felixfontein