validate-modules: make sure that options that potentially contain secret data have no_log set (#73508)

* Catch more potential errors (and increase false-positive rate).
* Flag some false-positives in lib/ansible/modules/ with no_log=False.

Co-authored-by: John Barker <john@johnrbarker.com>

changelogs/fragments/73508-validate-modules-no_log.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- "ansible-test validate-modules - option names that seem to indicate they contain secret information that should be marked ``no_log=True`` are now flagged in the validate-modules sanity test. False positives can be marked by explicitly setting ``no_log=False`` for these options in the argument spec. Please note that many false positives are expected; the assumption is that it is by far better to have false positives than false negatives (https://github.com/ansible/ansible/pull/73508)."

docs/docsite/rst/dev_guide/testing_validate-modules.rst

@@ -116,6 +116,7 @@ Codes
   multiple-utils-per-requires                                  Imports              Error                  ``Ansible.ModuleUtils`` requirements do not support multiple modules per statement
   multiple-csharp-utils-per-requires                           Imports              Error                  Ansible C# util requirements do not support multiple utils per statement
   no-default-for-required-parameter                            Documentation        Error                  Option is marked as required but specifies a default. Arguments with a default should not be marked as required
+  no-log-needed                                                Parameters           Error                  Option name suggests that the option contains a secret value, while ``no_log`` is not specified for this option in the argument spec. If this is a false positive, explicitly set ``no_log=False``
   nonexistent-parameter-documented                             Documentation        Error                  Argument is listed in DOCUMENTATION.options, but not accepted by the module
   option-incorrect-version-added                               Documentation        Error                  ``version_added`` for new option is incorrect
   option-invalid-version-added                                 Documentation        Error                  ``version_added`` for option is not a valid version number

lib/ansible/modules/apt_key.py

@@ -363,7 +363,7 @@ def main():
             url=dict(type='str'),
             data=dict(type='str'),
             file=dict(type='path'),
-            key=dict(type='str', removed_in_version='2.14', removed_from_collection='ansible.builtin'),
+            key=dict(type='str', removed_in_version='2.14', removed_from_collection='ansible.builtin', no_log=False),
             keyring=dict(type='path'),
             validate_certs=dict(type='bool', default=True),
             keyserver=dict(type='str'),

lib/ansible/modules/getent.py

@@ -99,7 +99,7 @@ def main():
     module = AnsibleModule(
         argument_spec=dict(
             database=dict(type='str', required=True),
-            key=dict(type='str'),
+            key=dict(type='str', no_log=False),
             service=dict(type='str'),
             split=dict(type='str'),
             fail_key=dict(type='bool', default=True),

lib/ansible/modules/known_hosts.py

@@ -335,7 +335,7 @@ def main():
     module = AnsibleModule(
         argument_spec=dict(
             name=dict(required=True, type='str', aliases=['host']),
-            key=dict(required=False, type='str'),
+            key=dict(required=False, type='str', no_log=False),
             path=dict(default="~/.ssh/known_hosts", type='path'),
             hash_host=dict(required=False, type='bool', default=False),
             state=dict(default='present', choices=['absent', 'present']),

lib/ansible/modules/rpm_key.py

@@ -233,7 +233,7 @@ def main():
     module = AnsibleModule(
         argument_spec=dict(
             state=dict(type='str', default='present', choices=['absent', 'present']),
-            key=dict(type='str', required=True),
+            key=dict(type='str', required=True, no_log=False),
             fingerprint=dict(type='str'),
             validate_certs=dict(type='bool', default=True),
         ),

test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py

@@ -69,6 +69,9 @@ REJECTLIST_DIRS = frozenset(('.git', 'test', '.github', '.idea'))
 INDENT_REGEX = re.compile(r'([\t]*)')
 TYPE_REGEX = re.compile(r'.*(if|or)(\s+[^"\']*|\s+)(?<!_)(?<!str\()type\([^)].*')
 SYS_EXIT_REGEX = re.compile(r'[^#]*sys.exit\s*\(.*')
+NO_LOG_REGEX = re.compile(r'(?:pass(?:[-_\s]?(?:word|phrase|wrd|wd)?)|secret|token|key)', re.I)
+
+
 REJECTLIST_IMPORTS = {
     'requests': {
         'new_only': True,
@@ -93,6 +96,25 @@ OS_CALL_REGEX = re.compile(r'os\.call.*')
 LOOSE_ANSIBLE_VERSION = LooseVersion('.'.join(ansible_version.split('.')[:3]))
 
 
+def is_potential_secret_option(option_name):
+    if not NO_LOG_REGEX.match(option_name):
+        return False
+    # If this is a count, type, algorithm, timeout, or name, it is probably not a secret
+    if option_name.endswith((
+            '_count', '_type', '_alg', '_algorithm', '_timeout', '_name', '_comment',
+            '_bits', '_id', '_identifier', '_period',
+    )):
+        return False
+    # 'key' also matches 'publickey', which is generally not secret
+    if any(part in option_name for part in (
+            'publickey', 'public_key', 'keyusage', 'key_usage', 'keyserver', 'key_server',
+            'keysize', 'key_size', 'keyservice', 'key_service', 'pub_key', 'pubkey',
+            'keyboard', 'secretary',
+    )):
+        return False
+    return True
+
+
 def compare_dates(d1, d2):
     try:
         date1 = parse_isodate(d1, allow_date=True)
@@ -1481,6 +1503,22 @@ class ModuleValidator(Validator):
                         )
                         continue
 
+            # Could this a place where secrets are leaked?
+            # If it is type: path we know it's not a secret key as it's a file path.
+            # If it is type: bool it is more likely a flag indicating that something is secret, than an actual secret.
+            if all((
+                    data.get('no_log') is None, is_potential_secret_option(arg),
+                    data.get('type') not in ("path", "bool"), data.get('choices') is None,
+            )):
+                msg = "Argument '%s' in argument_spec could be a secret, though doesn't have `no_log` set" % arg
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                self.reporter.error(
+                    path=self.object_path,
+                    code='no-log-needed',
+                    msg=msg,
+                )
+
             if not isinstance(data, dict):
                 msg = "Argument '%s' in argument_spec" % arg
                 if context: