authorized_key: Allow local path to a key #568
Conversation
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 33s |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 36s |
There was a problem hiding this comment.
@abakanovskii thank you for the PR!
I understand that the purpose of this modification is to load the public key on the target host and register it in ~/.ssh/authorized_keys more simple steps.
I have thought about this a bit. Instead of directly specifying the path when passing the key parameter, how about the idea of adding file:// prefix before the path, like file:///path_to_key style, similar to how http[s] URLs work?
Since the authorized_key module already has functionality for handling http[s], this seems like a more consistent option.
Additionally, to distinguish between handling local files on the controller and files on the target host, adding an option like remote_src: yes/no as in the ansible.builtin.copy module might make it more intuitive to specify local files on the controller using the lookup plugin.
In this case, I think the lookup plugin can still be used, so it should not be a disruptive modification.
I would like to hear your thoughts on this.
saito-hideki
added
needs_info
feature
Hello @saito-hideki! Thanks for review |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 41s |
There was a problem hiding this comment.
I reconsidered the implementation of remote_src. It seems that implementing it as an action plugin like copy module will also be necessary, so it seems more reasonable to limit it to the files on the target host, following your initial PR. I apologize for the confusion.
Based on your initial PR, I made some changes to the handling of the key file. I'd like to hear your thoughts.
If there are no issues, I think we can merge this PR if you can address the points I mentioned in the review, make the corrections, and squash the commits into one.
Thanks again! :)
| - name: Add key using path | ||
| ansible.posix.authorized_key: | ||
| user: root | ||
| key: "{{ key_path }}" |
There was a problem hiding this comment.
It probably needs a prefix like "file://" with key~path
There was a problem hiding this comment.
yes, I forgot about tests, thanks!
| - name: Add key using path again | ||
| ansible.posix.authorized_key: | ||
| user: root | ||
| key: "{{ key_path }}" |
There was a problem hiding this comment.
It probably needs a prefix like "file://" with key~path
plugins/modules/authorized_key.py
Outdated
There was a problem hiding this comment.
I reconsidered the implementation of remote_src. It seems that implementing it as an action plugin like copy module will also be necessary, so it seems more reasonable to limit it to the files on the target host, following your initial PR. I apologize for the confusion.
So, based on your initial commitment, I made the following modifications. I'd like to hear your thoughts:
diff --git a/plugins/modules/authorized_key.py b/plugins/modules/authorized_key.py
index ea58420..0f9abb5 100644
--- a/plugins/modules/authorized_key.py
+++ b/plugins/modules/authorized_key.py
@@ -24,7 +24,7 @@ options:
key:
description:
- The SSH public key(s), as a string or (since Ansible 1.9) url (https://github.com/username.keys).
- - You can also use V(file://) prefix to search localy or remote for a file with SSH key(s) depending on O(remote_src) value.
+ - You can also use V(file://) prefix to search remote for a file with SSH key(s).
type: str
required: true
path:
@@ -81,13 +81,6 @@ options:
- Follow path symlink instead of replacing it.
type: bool
default: false
- remote_src:
- description:
- - Influence whether key needs to be transferred or already is present remotely.
- - If V(false), it will search for src on the controller node.
- - If V(true) it will search for src on the managed (remote) node.
- type: bool
- default: false
author: Ansible Core Team
'''
@@ -109,7 +102,6 @@ EXAMPLES = r'''
user: charlie
state: present
key: file:///home/charlie/.ssh/id_rsa.pub
- remote_src: true
- name: Set authorized keys taken from url using lookup
ansible.posix.authorized_key:
@@ -234,6 +226,7 @@ import tempfile
import re
import shlex
from operator import itemgetter
+from urllib.parse import urlparse
from ansible.module_utils._text import to_native
from ansible.module_utils.basic import AnsibleModule
@@ -569,11 +562,10 @@ def enforce_state(module, params):
exclusive = params.get("exclusive", False)
comment = params.get("comment", None)
follow = params.get('follow', False)
- remote_src = params.get('remote_src', False)
error_msg = "Error getting key from: %s"
# if the key is a url or file, request it and use it as key source
- if key.startswith("http") or (key.startswith("file") and remote_src):
+ if key.startswith("http"):
try:
resp, info = fetch_url(module, key)
if info['status'] != 200:
@@ -586,6 +578,18 @@ def enforce_state(module, params):
# resp.read gives bytes on python3, convert to native string type
key = to_native(key, errors='surrogate_or_strict')
+ elif key.startswith("file"):
+ # if the key is an absolute path, check for existense and use it as a key source
+ key_path = urlparse(key).path
+ if not os.path.exists(key_path):
+ module.fail_json(msg="Path to a key file not found: %s" % key)
+ if not os.path.isfile(key_path):
+ module.fail_json(msg="Path to a key is a directory and must be a file: %s" % key)
+ try:
+ with open(key_path, 'r') as source_fh:
+ key = source_fh.read()
+ except OSError as e:
+ module.fail_json(msg="Failed to read key file %s : %s" % (key, to_native(e)))
# extract individual keys into an array, skipping blank lines and comments
new_keys = [s for s in key.splitlines() if s and not s.startswith('#')]
@@ -698,7 +702,6 @@ def main():
comment=dict(type='str'),
validate_certs=dict(type='bool', default=True),
follow=dict(type='bool', default=False),
- remote_src=dict(type='bool', default=False),
),
supports_check_mode=True,
)There was a problem hiding this comment.
Thank you for reviewing once again! Although we're adding new import now it seems more similar and intuitive than just parsing for root slash (/)
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 26s |
|
there is an issue with python2.7 and urlparse module so I can either do something likethis try:
from urllib.parse import urlparse
except ImportError:
from urlparse import urlparseor I can do what I did iin the last commit: no imports, just removing file_prefix = "file://"
if key.startswith(file_prefix):
# if the key is an absolute path, check for existense and use it as a key source
key_path = key[len(file_prefix):]@saito-hideki let me know which one suits the best |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 51s |
|
Hi @abakanovskii from ansible.module_utils.six.moves.urllib.parse import urlparsHow about using six ? I think it might work in both Python2 and Python3 environments. |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 10s |
| @@ -31,3 +31,6 @@ | |||
|
|
|||
| - name: Test for the management of comments with key | |||
| ansible.builtin.import_tasks: comments.yml | |||
|
|
|||
| - name: Test for specifying key as a path | |||
| ansible.builtin.import_tasks: setup_steps.yml | |||
There was a problem hiding this comment.
ansible.builtin.import_tasks: check_path.ymlI think it's check_path.yml :)
There was a problem hiding this comment.
yes, thank you!
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 55s |
There was a problem hiding this comment.
@abakanovskii
Looks good to me! Thank you for your contribution :)
saito-hideki
added
mergeit
|
Build succeeded (gate pipeline). ✔️ ansible-galaxy-importer SUCCESS in 5m 15s |
softwarefactory-project-zuul
bot
merged commit 0821768
into
ansible-collections:main
…novskii/feature/add_path_option_authorized_key"" This reverts commit 098b5be.
SUMMARY
Add option to specify an absolute path to file with SSH key(s) for
authorized_keyISSUE TYPE
COMPONENT NAME
authorized_keyADDITIONAL INFORMATION
Before this change you would need to get key using
ansible.builtin.slurpor something likeansible.builtin.command: cat <file>withregisterI tried to keep it as simple as possible