user module avoid conflicts ssh pub key (ansible#84165)

Remove pub key if we are going to generate private fix tests for os X
bcoca · Oct 24, 2024 · 11e4a6a · 11e4a6a
1 parent b181688
commit 11e4a6a
Show file tree

Hide file tree

Showing 5 changed files with 132 additions and 5 deletions.
diff --git a/changelogs/fragments/user_ssh_fix.yml b/changelogs/fragments/user_ssh_fix.yml
@@ -0,0 +1,4 @@
+bugfixes:
+  - user action will now require O(force) to overwrite the public part of an ssh key when generating ssh keys, as was already the case for the private part.
+security_fixes:
+  - user action won't allow ssh-keygen, chown and chmod to run on existing ssh public key file, avoiding traversal on existing symlinks (CVE-2024-9902).
diff --git a/lib/ansible/modules/user.py b/lib/ansible/modules/user.py
@@ -1220,9 +1220,11 @@ def ssh_key_gen(self):
         overwrite = None
         try:
             ssh_key_file = self.get_ssh_key_path()
+            pub_file = f'{ssh_key_file}.pub'
         except Exception as e:
             return (1, '', to_native(e))
         ssh_dir = os.path.dirname(ssh_key_file)
+
         if not os.path.exists(ssh_dir):
             if self.module.check_mode:
                 return (0, '', '')
@@ -1231,12 +1233,23 @@ def ssh_key_gen(self):
                 os.chown(ssh_dir, info[2], info[3])
             except OSError as e:
                 return (1, '', 'Failed to create %s: %s' % (ssh_dir, to_native(e)))
+
         if os.path.exists(ssh_key_file):
             if self.force:
-                # ssh-keygen doesn't support overwriting the key interactively, so send 'y' to confirm
+                self.module.warn(f'Overwriting existing ssh key private file "{ssh_key_file}"')
                 overwrite = 'y'
             else:
+                self.module.warn(f'Found existing ssh key private file "{ssh_key_file}", no force, so skipping ssh-keygen generation')
                 return (None, 'Key already exists, use "force: yes" to overwrite', '')
+
+        if os.path.exists(pub_file):
+            if self.force:
+                self.module.warn(f'Overwriting existing ssh key public file "{pub_file}"')
+                os.unlink(pub_file)
+            else:
+                self.module.warn(f'Found existing ssh key public file "{pub_file}", no force, so skipping ssh-keygen generation')
+                return (None, 'Public key already exists, use "force: yes" to overwrite', '')
+
         cmd = [self.module.get_bin_path('ssh-keygen', True)]
         cmd.append('-t')
         cmd.append(self.ssh_type)
@@ -1303,7 +1316,7 @@ def ssh_key_gen(self):
             # If the keys were successfully created, we should be able
             # to tweak ownership.
             os.chown(ssh_key_file, info[2], info[3])
-            os.chown('%s.pub' % ssh_key_file, info[2], info[3])
+            os.chown(pub_file, info[2], info[3])
         return (rc, out, err)
 
     def ssh_key_fingerprint(self):

diff --git a/test/integration/targets/user/tasks/main.yml b/test/integration/targets/user/tasks/main.yml
@@ -38,10 +38,11 @@
 - import_tasks: test_ssh_key_passphrase.yml
 - import_tasks: test_password_lock.yml
 - import_tasks: test_password_lock_new_user.yml
-- import_tasks: test_local.yml
+- include_tasks: test_local.yml
   when: not (ansible_distribution == 'openSUSE Leap' and ansible_distribution_version is version('15.4', '>='))
-- import_tasks: test_umask.yml
+- include_tasks: test_umask.yml
   when: ansible_facts.system == 'Linux'
 - import_tasks: test_inactive_new_account.yml
-- import_tasks: test_create_user_min_max.yml
+- include_tasks: test_create_user_min_max.yml
   when: ansible_facts.system == 'Linux'
+- import_tasks: ssh_keygen.yml
diff --git a/test/integration/targets/user/tasks/ssh_keygen.yml b/test/integration/targets/user/tasks/ssh_keygen.yml
@@ -0,0 +1,100 @@
+- name: user generating ssh keys tests
+  become: true
+  vars:
+    home: "{{ (ansible_facts['os_family'] == 'Darwin')|ternary('/Users/ansibulluser/', '/home/ansibulluser/')}}"
+    ssh_key_file: .ssh/ansible_test_rsa
+    pub_file: '{{ssh_key_file}}.pub'
+    key_files:
+      - '{{ssh_key_file}}'
+      - '{{pub_file}}'
+  block:
+  - name: Ensure clean/non existsing ansibulluser
+    user: name=ansibulluser state=absent
+
+  - name: Test creating ssh key creation
+    block:
+    - name: Create user with ssh key
+      user:
+        name: ansibulluser
+        state: present
+        generate_ssh_key: yes
+        ssh_key_file: '{{ ssh_key_file}}'
+
+    - name: check files exist
+      stat:
+        path: '{{home ~ item}}'
+      register: stat_keys
+      loop: '{{ key_files }}'
+
+    - name: ensure they exist
+      assert:
+        that:
+          - stat_keys.results[item].stat.exists
+      loop: [0, 1]
+
+    always:
+    - name: Clean ssh keys
+      file: path={{ home ~ item }} state=absent
+      loop: '{{ key_files }}'
+
+    - name: Ensure clean/non existsing ansibulluser
+      user: name=ansibulluser state=absent
+
+  - name: Ensure we don't break on conflicts
+    block:
+    - name: flag file for test
+      tempfile:
+      register: flagfile
+
+    - name: precreate public .ssh
+      file: path={{home ~ '.ssh'}} state=directory
+
+    - name: setup public key linked to flag file
+      file: path={{home ~ pub_file}} src={{flagfile.path}} state=link
+
+    - name: Create user with ssh key
+      user:
+        name: ansibulluser
+        state: present
+        generate_ssh_key: yes
+        ssh_key_file: '{{ ssh_key_file }}'
+      ignore_errors: true
+      register: user_no_force
+
+    - stat: path={{home ~ pub_file}}
+      register: check_pub
+
+    - name: ensure we didn't overwrite
+      assert:
+        that:
+          - check_pub.stat.exists
+          - check_pub.stat.islnk
+          - check_pub.stat.uid == 0
+
+    - name: Create user with ssh key
+      user:
+        name: ansibulluser
+        state: present
+        generate_ssh_key: yes
+        ssh_key_file: '{{ ssh_key_file }}'
+        force: true
+      ignore_errors: true
+      register: user_force
+
+    - stat: path={{home ~ pub_file}}
+      register: check_pub2
+
+    - name: ensure we failed since we didn't force overwrite
+      assert:
+        that:
+          - user_force is success
+          - check_pub2.stat.exists
+          - not check_pub2.stat.islnk
+          - check_pub2.stat.uid != 0
+    always:
+    - name: Clean up files
+      file: path={{ home ~ item }} state=absent
+      loop: '{{ key_files + [flagfile.path] }}'
+
+    - name: Ensure clean/non existsing ansibulluser
+      user: name=ansibulluser state=absent
diff --git a/test/integration/targets/user/tasks/test_local.yml b/test/integration/targets/user/tasks/test_local.yml
@@ -39,6 +39,15 @@
   tags:
     - user_test_local_mode
 
+- name: Ensure no local_ansibulluser
+  user:
+    name: local_ansibulluser
+    state: absent
+    local: yes
+    remove: true
+  tags:
+    - user_test_local_mode
+
 - name: Create local_ansibulluser
   user:
     name: local_ansibulluser