diff --git a/changelogs/fragments/sort_obfuscation.yml b/changelogs/fragments/sort_obfuscation.yml new file mode 100644 index 00000000000..ad60de52fd2 --- /dev/null +++ b/changelogs/fragments/sort_obfuscation.yml @@ -0,0 +1,2 @@ +bugfixes: + - module_utils sanitize_keys and remove_value functions now sort their input to ensure matching subsets are always obscured. diff --git a/lib/ansible/module_utils/common/parameters.py b/lib/ansible/module_utils/common/parameters.py index 129bb05178a..57e93ff23c0 100644 --- a/lib/ansible/module_utils/common/parameters.py +++ b/lib/ansible/module_utils/common/parameters.py @@ -499,7 +499,7 @@ def _set_defaults(argument_spec, parameters, set_default=True): return no_log_values -def _sanitize_keys_conditions(value, no_log_strings, ignore_keys, deferred_removals): +def _sanitize_keys_conditions(value, deferred_removals): """ Helper method to :func:`sanitize_keys` to build ``deferred_removals`` and avoid deep recursion. """ if isinstance(value, (str, bytes)): return value @@ -867,8 +867,9 @@ def sanitize_keys(obj, no_log_strings, ignore_keys=frozenset()): deferred_removals = deque() - no_log_strings = [to_native(s, errors='surrogate_or_strict') for s in no_log_strings] - new_value = _sanitize_keys_conditions(obj, no_log_strings, ignore_keys, deferred_removals) + # sort ensuring we always handle longer strings vs subsets + no_log_strings = sorted([to_native(s, errors='surrogate_or_strict') for s in no_log_strings], key=len, reverse=True) + new_value = _sanitize_keys_conditions(obj, deferred_removals) while deferred_removals: old_data, new_data = deferred_removals.popleft() @@ -876,15 +877,15 @@ def sanitize_keys(obj, no_log_strings, ignore_keys=frozenset()): if isinstance(new_data, Mapping): for old_key, old_elem in old_data.items(): if old_key in ignore_keys or old_key.startswith('_ansible'): - new_data[old_key] = _sanitize_keys_conditions(old_elem, no_log_strings, ignore_keys, deferred_removals) + new_data[old_key] = _sanitize_keys_conditions(old_elem, deferred_removals) else: # Sanitize the old key. We take advantage of the sanitizing code in # _remove_values_conditions() rather than recreating it here. new_key = _remove_values_conditions(old_key, no_log_strings, None) - new_data[new_key] = _sanitize_keys_conditions(old_elem, no_log_strings, ignore_keys, deferred_removals) + new_data[new_key] = _sanitize_keys_conditions(old_elem, deferred_removals) else: for elem in old_data: - new_elem = _sanitize_keys_conditions(elem, no_log_strings, ignore_keys, deferred_removals) + new_elem = _sanitize_keys_conditions(elem, deferred_removals) if isinstance(new_data, MutableSequence): new_data.append(new_elem) elif isinstance(new_data, MutableSet): @@ -907,7 +908,8 @@ def remove_values(value, no_log_strings): deferred_removals = deque() - no_log_strings = [to_native(s, errors='surrogate_or_strict') for s in no_log_strings] + # sort ensuring we always handle longer strings vs subsets + no_log_strings = sorted([to_native(s, errors='surrogate_or_strict') for s in no_log_strings], key=len, reverse=True) new_value = _remove_values_conditions(value, no_log_strings, deferred_removals) while deferred_removals: diff --git a/test/integration/targets/no_log/library/module.py b/test/integration/targets/no_log/library/module.py index 61c2908e128..6fb101838bf 100644 --- a/test/integration/targets/no_log/library/module.py +++ b/test/integration/targets/no_log/library/module.py @@ -37,7 +37,7 @@ def main(): } ) - module.exit_json(msg='done') + module.exit_json(msg='done', values=', '.join([str(v) for v in module.params.values() if v])) if __name__ == '__main__': diff --git a/test/integration/targets/no_log/runme.sh b/test/integration/targets/no_log/runme.sh index 8e89fc97ded..3c32641ee69 100755 --- a/test/integration/targets/no_log/runme.sh +++ b/test/integration/targets/no_log/runme.sh @@ -30,3 +30,6 @@ ansible-playbook ansible_no_log_in_result.yml -vvvvv > "${OUTPUT_DIR}/output.log [ "$(ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "1" ] [ "$(ANSIBLE_NO_LOG=0 ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "1" ] [ "$(ANSIBLE_NO_LOG=1 ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "5" ] + +# Ensure module no_log masking handles secrets that are substrings of other secrets correctly +ansible-playbook sub_masking.yml -i ../../inventory -vvvvv "$@" diff --git a/test/integration/targets/no_log/secretvars.yml b/test/integration/targets/no_log/secretvars.yml index 0030d747d74..b4c7b60f657 100644 --- a/test/integration/targets/no_log/secretvars.yml +++ b/test/integration/targets/no_log/secretvars.yml @@ -30,3 +30,6 @@ s210: SECRET210 s211: SECRET211 s212: SECRET212 s213: SECRET213 + +# substring +sec: RET diff --git a/test/integration/targets/no_log/sub_masking.yml b/test/integration/targets/no_log/sub_masking.yml new file mode 100644 index 00000000000..65584789840 --- /dev/null +++ b/test/integration/targets/no_log/sub_masking.yml @@ -0,0 +1,60 @@ +- name: Ensure no_log obfuscation does not mask on substrings + hosts: all + vars_files: + - secretvars.yml + gather_facts: no + + tasks: + - name: test with option long + module: + secret: "{{ s101 }}" + subopt_dict: + str_sub_opt1: "{{ s102 }}" + nested_subopt: + n_subopt1: "{{ s103 }}" + subopt_list: + - subopt1: "{{sec}}" + register: long_first + + - name: Task with suboptions long + module: + secret: "{{ sec }}" + subopt_dict: + str_sub_opt1: '{{ s101 }}' + nested_subopt: + n_subopt1: "{{ s102 }}" + subopt_list: + - subopt1: "{{ s103 }}" + register: short_first + + - name: Task with suboptions long + module: + secret: "{{ s101 }}" + subopt_dict: + str_sub_opt1: '{{ sec }}' + nested_subopt: + n_subopt1: "{{ s102 }}" + subopt_list: + - subopt1: "{{ s103 }}" + register: middle_top + + - name: Task with suboptions long + module: + secret: "{{ s102}}" + subopt_dict: + str_sub_opt1: '{{ s102 }}' + nested_subopt: + n_subopt1: "{{ sec }}" + subopt_list: + - subopt1: "{{ s103 }}" + register: middle_bottom + + - name: check output + assert: + that: + - "'SEC' not in (q('vars', item)|to_json)" + loop: + - long_first + - short_first + - middle_top + - middle_bottom