From 86eefbad8dbccae3ecab16f9ab4300da05cbddc5 Mon Sep 17 00:00:00 2001 From: Rich Megginson Date: Tue, 22 Aug 2023 07:16:41 -0600 Subject: [PATCH] ad_integration - leaks credentials when in check_mode Resolves:rhbz#2223764 ad_integration - leaks credentials when in check_mode --- ...stdin-for-password-and-do-not-log-pa.patch | 127 ++++++++++++++++++ CHANGELOG.md | 1 + linux-system-roles.spec | 7 + 3 files changed, 135 insertions(+) create mode 100644 0001-fix-use-command-stdin-for-password-and-do-not-log-pa.patch diff --git a/0001-fix-use-command-stdin-for-password-and-do-not-log-pa.patch b/0001-fix-use-command-stdin-for-password-and-do-not-log-pa.patch new file mode 100644 index 0000000..bba4a2b --- /dev/null +++ b/0001-fix-use-command-stdin-for-password-and-do-not-log-pa.patch @@ -0,0 +1,127 @@ +From 1931ebccaa146bd6ee8365c664ab62d294adaa31 Mon Sep 17 00:00:00 2001 +From: Rich Megginson +Date: Fri, 18 Aug 2023 12:35:44 -0600 +Subject: [PATCH] fix: use command stdin for password, and do not log password + +Cause: The code was constructing the realm join command to be passed +via the shell module, including piping the password into the command, +and was showing the command, including the password, when using +check mode. + +Consequence: The clear text password was available in the logs when +using check mode. + +Fix: Use command with stdin for the password instead of shell. The +password is not part of the command. command with stdin is more +secure than using shell. The debug output has been changed to +show the command with the `ad_integration_join_parameters` removed, +because we cannot know if those parameters contain data which should +not be logged. Those parameters will still be passed to the actual +realm join command. + +Result: The password is not logged. The role is more secure. + +Signed-off-by: Rich Megginson +--- + tasks/main.yml | 57 ++++++++++++++++++++++++++++---------------------- + 1 file changed, 32 insertions(+), 25 deletions(-) + +diff --git a/tasks/main.yml b/tasks/main.yml +index fe2602e..265c6fe 100644 +--- a/tasks/main.yml ++++ b/tasks/main.yml +@@ -3,8 +3,7 @@ + - name: Ensure that mandatory variable ad_integration_realm is available + fail: + msg: Variable ad_integration_realm must be provided! +- when: +- - not ad_integration_realm ++ when: not ad_integration_realm + + - name: Assume managing timesync if timesource is set + set_fact: +@@ -26,8 +25,7 @@ + - name: Assume managing crypto policies if allow_rc4_crypto is set + set_fact: + ad_integration_manage_crypto_policies: true +- when: +- - ad_integration_allow_rc4_crypto | bool ++ when: ad_integration_allow_rc4_crypto | bool + + - name: Ensure manage_crypt_policies is set with crypto_allow_rc4 + fail: +@@ -141,41 +139,50 @@ + + - name: Build Command - Join to a specific Domain Controller + set_fact: +- __ad_integration_join_command: | +- set -euo pipefail +- echo {{ ad_integration_password | quote }} | realm join -U \ +- {{ ad_integration_user | quote }} --membership-software \ +- {{ ad_integration_membership_software | quote }} \ +- {{ ad_integration_join_parameters }} \ +- {{ ad_integration_join_to_dc | quote }} ++ __ad_integration_join_command: >- ++ realm join -U {{ ad_integration_user | quote }} --membership-software ++ {{ ad_integration_membership_software | quote }} ++ {{ ad_integration_join_parameters }} ++ {{ ad_integration_join_to_dc | quote }} ++ __ad_integration_debug_command: >- ++ realm join -U {{ ad_integration_user | quote }} --membership-software ++ {{ ad_integration_membership_software | quote }} ++ {{ ad_integration_join_to_dc | quote }} + no_log: true +- when: +- - ad_integration_join_to_dc is not none ++ when: ad_integration_join_to_dc is not none + + - name: Build Join Command - Perform discovery-based realm join operation + set_fact: +- __ad_integration_join_command: | +- set -euo pipefail +- echo {{ ad_integration_password | quote }} | realm join -U \ +- {{ ad_integration_user | quote }} --membership-software \ +- {{ ad_integration_membership_software | quote }} \ +- {{ ad_integration_join_parameters }} \ +- {{ ad_integration_realm | quote }} ++ __ad_integration_join_command: >- ++ realm join -U {{ ad_integration_user | quote }} --membership-software ++ {{ ad_integration_membership_software | quote }} ++ {{ ad_integration_join_parameters }} ++ {{ ad_integration_realm | quote }} ++ __ad_integration_debug_command: >- ++ realm join -U {{ ad_integration_user | quote }} --membership-software ++ {{ ad_integration_membership_software | quote }} ++ {{ ad_integration_realm | quote }} + no_log: true +- when: +- - ad_integration_join_to_dc is none ++ when: ad_integration_join_to_dc is none + + - name: Show the join command for debug + debug: +- msg: "Would run: '{{ __ad_integration_join_command }}'" ++ msg: ++ - >- ++ Would run the following command. Note that ++ ad_integration_join_parameters have been removed for security purposes, ++ the role will pass them to the actual realm join command when running ++ without check mode. ++ - "{{ __ad_integration_debug_command }}" + when: + - ad_integration_join_to_dc == __ad_integration_sample_dc + or ad_integration_realm == __ad_integration_sample_realm + or ansible_check_mode + + - name: Run realm join command +- # noqa command-instead-of-shell +- shell: "{{ __ad_integration_join_command }}" ++ command: "{{ __ad_integration_join_command }}" ++ args: ++ stdin: "{{ ad_integration_password }}" + no_log: true + when: + - ad_integration_join_to_dc != __ad_integration_sample_dc +-- +2.41.0 + diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c32be9..8274485 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ Changelog ### Bug Fixes - [ALL - facts being gathered unnecessarily](https://bugzilla.redhat.com/show_bug.cgi?id=2223032) +- [ad_integration - leaks credentials when in check_mode](https://bugzilla.redhat.com/show_bug.cgi?id=2223764) - [certificate - does not re-issue after updating key_size](https://bugzilla.redhat.com/show_bug.cgi?id=2224138) - [firewall - fix: reload on resetting to defaults](https://bugzilla.redhat.com/show_bug.cgi?id=2223764) - [firewall - Check mode fails with replacing previous rules](https://issues.redhat.com/browse/RHEL-898) diff --git a/linux-system-roles.spec b/linux-system-roles.spec index b3b0568..42af1f4 100644 --- a/linux-system-roles.spec +++ b/linux-system-roles.spec @@ -217,6 +217,8 @@ Source1004: vendoring-build.inc Source995: CHANGELOG.md +Patch2201: 0001-fix-use-command-stdin-for-password-and-do-not-log-pa.patch + BuildArch: noarch %if %{with html} @@ -329,6 +331,10 @@ if [ "$rolesdir" != "$realrolesdir" ]; then fi cd .. +cd %{rolename22} +%patch2201 -p1 +cd .. + # vendoring build steps, if any %include %{SOURCE1004} @@ -667,6 +673,7 @@ find %{buildroot}%{ansible_roles_dir} -mindepth 1 -maxdepth 1 | \ %changelog * Tue Aug 15 2023 Rich Megginson - 1.22.0-1 +- Resolves:rhbz#2223764 : ad_integration - leaks credentials when in check_mode - Resolves:rhbz#2232241 : kdump - "Write new authorized_keys if needed" task idempotency issues - Resolves:rhbz#2232231 : kdump - system role fails if kdump_ssh_user doesn't have a .ssh/authorized_keys file in home directory - Resolves RHEL-1397 : kdump - fix: ensure .ssh directory exists for kdump_ssh_user on kdump_ssh_server