Ensure no_log does not use subsets (#86939) (#86952)

* also remove unused parameters from inner function

* Ensure no_log does not use subsets

Sort strings before using so the longer ones are processed first
avoiding strings that are subsets of each other creating partial
results.
Co-authored-by: David Shrewsbury <Shrews@users.noreply.github.com>

(cherry picked from commit 5c55372345)

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.

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:

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__':

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 "$@"

test/integration/targets/no_log/secretvars.yml

@@ -30,3 +30,6 @@ s210: SECRET210
 s211: SECRET211
 s212: SECRET212
 s213: SECRET213
+
+# substring
+sec: RET

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