diff options
| -rw-r--r-- | changelogs/fragments/user_ssh_fix.yml | 4 | ||||
| -rw-r--r-- | lib/ansible/modules/user.py | 17 | ||||
| -rw-r--r-- | test/integration/targets/user/tasks/main.yml | 7 | ||||
| -rw-r--r-- | test/integration/targets/user/tasks/ssh_keygen.yml | 100 | ||||
| -rw-r--r-- | test/integration/targets/user/tasks/test_local.yml | 9 |
5 files changed, 132 insertions, 5 deletions
diff --git a/changelogs/fragments/user_ssh_fix.yml b/changelogs/fragments/user_ssh_fix.yml new file mode 100644 index 0000000000..b2c47d60e3 --- /dev/null +++ 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 index 3ea95e8875..aa3bbcf68f 100644 --- a/lib/ansible/modules/user.py +++ b/lib/ansible/modules/user.py @@ -1220,9 +1220,11 @@ class User(object): 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 @@ class User(object): 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 @@ class User(object): # 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 index bb4b261b75..89dec984c0 100644 --- 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 new file mode 100644 index 0000000000..e23bc48ee8 --- /dev/null +++ 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 index c49ab0c35c..c4cdb4800f 100644 --- 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 |