Add comment option to authorized_key (#27781)

* Add comment option to authorized_keys

* Update version_added for authorized_keys comment

* PEP8

* Include index rank in parsed_key_key

*  Properly display diff

Only display diff if specificed via settings

* Fix PEP8 test failure

Removed from legacy files since it is now properly formatted

* Cleanup integration test formatting and add test for new comment feature

* Correct version_added for new option

lib/ansible/modules/system/authorized_key.py

@@ -79,6 +79,14 @@ options:
     default: "yes"
     choices: ["yes", "no"]
     version_added: "2.1"
+  comment:
+    description:
+      - Change the comment on the public key. Rewriting the comment is useful in
+        cases such as fetching it from GitHub or GitLab.
+      - If no comment is specified, the existing comment will be kept.
+    required: false
+    default: None
+    version_added: "2.4"
 author: "Ansible Core Team"
 '''
 
@@ -220,6 +228,7 @@ from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.pycompat24 import get_exception
 from ansible.module_utils.urls import fetch_url
 
+
 class keydict(dict):
 
     """ a dictionary that maintains the order of keys as they are added
@@ -349,6 +358,7 @@ def keyfile(module, user, write=False, path=None, manage_dir=True):
 
     return keysfile
 
+
 def parseoptions(module, options):
     '''
     reads a string containing ssh-key options
@@ -369,6 +379,7 @@ def parseoptions(module, options):
 
     return options_dict
 
+
 def parsekey(module, raw_key, rank=None):
     '''
     parses a key, which may or may not contain a list
@@ -430,6 +441,7 @@ def parsekey(module, raw_key, rank=None):
 
     return (key, key_type, options, comment, rank)
 
+
 def readfile(filename):
 
     if not os.path.isfile(filename):
@@ -441,6 +453,7 @@ def readfile(filename):
     finally:
         f.close()
 
+
 def parsekeys(module, lines):
     keys = {}
     for rank_index, line in enumerate(lines.splitlines(True)):
@@ -454,6 +467,7 @@ def parsekeys(module, lines):
             keys[line] = (line, 'skipped', None, None, rank_index)
     return keys
 
+
 def writefile(module, filename, content):
 
     fd, tmp_path = tempfile.mkstemp('', 'tmp', os.path.dirname(filename))
@@ -467,6 +481,7 @@ def writefile(module, filename, content):
     f.close()
     module.atomic_move(tmp_path, filename)
 
+
 def serialize(keys):
     lines = []
     new_keys = keys.values()
@@ -496,11 +511,12 @@ def serialize(keys):
                 key_line = key[0]
             else:
                 key_line = "%s%s %s %s\n" % (option_str, key_type, keyhash, comment)
-        except:
+        except Exception:
             key_line = key
         lines.append(key_line)
     return ''.join(lines)
 
+
 def enforce_state(module, params):
     """
     Add or remove key.
@@ -513,6 +529,7 @@ def enforce_state(module, params):
     state = params.get("state", "present")
     key_options = params.get("key_options", None)
     exclusive = params.get("exclusive", False)
+    comment = params.get("comment", None)
     error_msg = "Error getting key from: %s"
 
     # if the key is a url, request it and use it as key source
@@ -559,6 +576,9 @@ def enforce_state(module, params):
             # rank here is the rank in the provided new keys, which may be unrelated to rank in existing_keys
             parsed_new_key = (parsed_new_key[0], parsed_new_key[1], parsed_options, parsed_new_key[3], parsed_new_key[4])
 
+        if comment is not None:
+            parsed_new_key = (parsed_new_key[0], parsed_new_key[1], parsed_new_key[2], comment, parsed_new_key[4])
+
         matched = False
         non_matching_keys = []
 
@@ -607,23 +627,28 @@ def enforce_state(module, params):
     if do_write:
         filename = keyfile(module, user, do_write, path, manage_dir)
         new_content = serialize(existing_keys)
+
+        diff = None
+        if module._diff:
             diff = {
                 'before_header': params['keyfile'],
                 'after_header': filename,
                 'before': existing_content,
                 'after': new_content,
             }
+            params['diff'] = diff
+
         if module.check_mode:
             module.exit_json(changed=True, diff=diff)
         writefile(module, filename, new_content)
         params['changed'] = True
-        params['diff'] = diff
     else:
         if module.check_mode:
             module.exit_json(changed=False)
 
     return params
 
+
 def main():
     module = AnsibleModule(
         argument_spec=dict(
@@ -635,6 +660,7 @@ def main():
             key_options=dict(required=False, type='str'),
             unique=dict(default=False, type='bool'),
             exclusive=dict(default=False, type='bool'),
+            comment=dict(required=False, default=None, type='str'),
             validate_certs=dict(default=True, type='bool'),
         ),
         supports_check_mode=True
@@ -643,5 +669,6 @@ def main():
     results = enforce_state(module, module.params)
     module.exit_json(**results)
 
+
 if __name__ == '__main__':
     main()

test/integration/targets/authorized_key/defaults/main.yml

@@ -1,39 +1,35 @@
----
-dss_key_basic: >
-  ssh-dss DATA_BASIC root@testing
-dss_key_unquoted_option: >
-  idle-timeout=5m ssh-dss DATA_UNQUOTED_OPTION root@testing
-dss_key_command: >
-  command="/bin/true" ssh-dss DATA_COMMAND root@testing
-dss_key_complex_command: >
-  command="echo foo 'bar baz'" ssh-dss DATA_COMPLEX_COMMAND root@testing
-dss_key_command_single_option: >
-  no-port-forwarding,command="/bin/true" ssh-dss DATA_COMMAND_SINGLE_OPTIONS root@testing
-dss_key_command_multiple_options: >
-  no-port-forwarding,idle-timeout=5m,command="/bin/true" ssh-dss DATA_COMMAND_MULTIPLE_OPTIONS root@testing
-dss_key_trailing: >
-  ssh-dss DATA_TRAILING root@testing foo bar baz
-rsa_key_basic: >
-  ssh-rsa DATA_BASIC root@testing
+dss_key_basic: ssh-dss DATA_BASIC root@testing
+dss_key_unquoted_option: idle-timeout=5m ssh-dss DATA_UNQUOTED_OPTION root@testing
+dss_key_command: command="/bin/true" ssh-dss DATA_COMMAND root@testing
+dss_key_complex_command: command="echo foo 'bar baz'" ssh-dss DATA_COMPLEX_COMMAND root@testing
+dss_key_command_single_option: no-port-forwarding,command="/bin/true" ssh-dss DATA_COMMAND_SINGLE_OPTIONS root@testing
+dss_key_command_multiple_options: no-port-forwarding,idle-timeout=5m,command="/bin/true" ssh-dss DATA_COMMAND_MULTIPLE_OPTIONS root@testing
+dss_key_trailing: ssh-dss DATA_TRAILING root@testing foo bar baz
+rsa_key_basic: ssh-rsa DATA_BASIC root@testing
+
 multiple_key_base: |
                     ssh-rsa DATA_BASIC 1@testing
                     ssh-dss DATA_TRAILING 2@testing foo bar baz
                     ssh-dss DATA_TRAILING 3@testing foo bar baz
                     ecdsa-sha2-nistp521 ECDSA_DATA 4@testing
+
 multiple_key_different_order: |
                               ssh-dss DATA_TRAILING 2@testing foo bar baz
                               ssh-dss DATA_TRAILING 3@testing foo bar baz
                               ssh-rsa DATA_BASIC 1@testing
                               ecdsa-sha2-nistp521 ECDSA_DATA 4@testing
+
 multiple_key_different_order_2: |
                                 ssh-dss DATA_TRAILING 2@testing foo bar baz
                                 ssh-rsa WHATEVER 2.5@testing
                                 ssh-dss DATA_TRAILING 3@testing foo bar baz
                                 ssh-rsa DATA_BASIC 1@testing
                                 ecdsa-sha2-nistp521 ECDSA_DATA 4@testing
+
 multiple_key_exclusive: |
                     ssh-rsa DATA_BASIC 1@testing
                     ecdsa-sha2-nistp521 ECDSA_DATA 4@testing
+
 multiple_keys_comments: |
   ssh-rsa DATA_BASIC 1@testing
   # I like adding comments yo-dude-this-is-not-a-key INVALID_DATA 2@testing

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

@@ -22,7 +22,9 @@
 
 
 - name: copy an existing file in place with comments
-  copy: src=existing_authorized_keys dest="{{output_dir|expanduser}}/authorized_keys"
+  copy:
+    src: existing_authorized_keys
+    dest: "{{ output_dir | expanduser }}/authorized_keys"
 
 - name: add multiple keys different order
   authorized_key:
@@ -34,6 +36,7 @@
 
 - name: get the file content
   shell: cat "{{ output_dir | expanduser }}/authorized_keys"
+  changed_when: no
   register: multiple_keys_existing
 
 - name: assert that the key was added and comments and ordering preserved
@@ -50,10 +53,14 @@
 # start afresh
 
 - name: remove file foo.txt
-  file: path="{{output_dir|expanduser}}/authorized_keys" state=absent
+  file:
+    path: "{{ output_dir | expanduser }}/authorized_keys"
+    state: absent
 
 - name: touch the authorized_keys file
-  file: dest="{{output_dir}}/authorized_keys" state=touch
+  file:
+    dest: "{{ output_dir }}/authorized_keys"
+    state: touch
   register: result
 
 - name: assert that the authorized_keys file was created
@@ -126,6 +133,7 @@
 
 - name: get the file content
   shell: cat "{{ output_dir | expanduser }}/authorized_keys"
+  changed_when: no
   register: multiple_keys_at_a_time
 
 - name: assert that the key was added
@@ -145,6 +153,7 @@
 
 - name: get the file content
   shell: cat "{{ output_dir | expanduser }}/authorized_keys"
+  changed_when: no
   register: multiple_keys_comments
 
 - name: assert that the keys exist and comment only lines were not added
@@ -390,6 +399,7 @@
 
 - name: get the file content
   shell: cat "{{ output_dir | expanduser }}/authorized_keys" | fgrep DATA_BASIC
+  changed_when: no
   register: content
 
 - name: validate content
@@ -401,7 +411,9 @@
 # check mode
 
 - name: copy an existing file in place with comments
-  copy: src=existing_authorized_keys dest="{{output_dir|expanduser}}/authorized_keys"
+  copy:
+    src: existing_authorized_keys
+    dest: "{{ output_dir | expanduser }}/authorized_keys"
 
 - authorized_key:
     user: root
@@ -411,15 +423,63 @@
   check_mode: True
   register: result
 
-- name: assert that the key would be added and that the diff is shown
-  assert:
-    that:
-      - 'result.changed'
-      - '"ssh-rsa WHATEVER 2.5@testing" in result.diff.after'
-
 - name: assert that the file was not changed
-  copy: src=existing_authorized_keys dest="{{output_dir|expanduser}}/authorized_keys"
+  copy:
+    src: existing_authorized_keys
+    dest: "{{ output_dir | expanduser }}/authorized_keys"
   register: result
+
 - assert:
     that:
       - 'result.changed == False'
+
+# -------------------------------------------------------------
+# comments
+
+- name: Add rsa key with existing comment
+  authorized_key:
+    user: root
+    key: "{{ rsa_key_basic }}"
+    state: present
+    path: "{{ output_dir | expanduser }}/authorized_keys"
+  register: result
+
+- name: Change the comment on an existing key
+  authorized_key:
+    user: root
+    key: "{{ rsa_key_basic }}"
+    comment: user@acme.com
+    state: present
+    path: "{{ output_dir | expanduser }}/authorized_keys"
+  register: result
+
+- name: get the file content
+  shell: cat "{{ output_dir | expanduser }}/authorized_keys" | fgrep DATA_BASIC
+  changed_when: no
+  register: content
+
+- name: Assert that comment on an existing key was changed
+  assert:
+    that:
+      - "'user@acme.com' in content.stdout"
+
+- name: Set the same key with comment to ensure no changes are reported
+  authorized_key:
+    user: root
+    key: "{{ rsa_key_basic }}"
+    comment: user@acme.com
+    state: present
+    path: "{{ output_dir | expanduser }}/authorized_keys"
+  register: result
+
+- name: Assert that no changes were made when running again
+  assert:
+    that:
+      - not result.changed
+
+- debug:
+    var: "{{ item }}"
+    verbosity: 1
+  with_items:
+    - result
+    - content

test/sanity/pep8/legacy-files.txt

@@ -434,7 +434,6 @@ lib/ansible/modules/storage/zfs/zfs.py
 lib/ansible/modules/system/aix_inittab.py
 lib/ansible/modules/system/alternatives.py
 lib/ansible/modules/system/at.py
-lib/ansible/modules/system/authorized_key.py
 lib/ansible/modules/system/capabilities.py
 lib/ansible/modules/system/cron.py
 lib/ansible/modules/system/cronvar.py