From 3ca4580cb4e2a24597c6c5108bf76bbcd48069f8 Mon Sep 17 00:00:00 2001 From: kaorihinata Date: Thu, 9 Jan 2020 16:47:57 -0500 Subject: [PATCH] Allow no_log=False to silence the no_log warnings for module parameters (#64733) As AnsibleModule._log_invocation is currently implemented, any parameter with a name that matches PASSWORD_MATCH triggers the no_log warning as a precaution against parameters that may contain sensitive data, but have not been marked as sensitive by the module author. This patch would allow module authors to explicitly mark the aforementioned parameters as not sensitive thereby bypassing an erroneous warning message, while still catching parameters which have not been marked at all by the author. Adds tests for various no_log states including True, False, and None (as extracted by AnsibleModule._log_invocation) when applied to an argument with a name that matches PASSWORD_MATCH. Fixes: #49465 #64656 --- ...-no_log-false-override-no_log-warnings.yml | 2 + .../developing_modules_documenting.rst | 3 +- .../developing_program_flow_modules.rst | 5 ++- lib/ansible/module_utils/basic.py | 11 +++--- .../module_utils/basic/test_argument_spec.py | 37 +++++++++++++++++++ 5 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 changelogs/fragments/64733-make-no_log-false-override-no_log-warnings.yml diff --git a/changelogs/fragments/64733-make-no_log-false-override-no_log-warnings.yml b/changelogs/fragments/64733-make-no_log-false-override-no_log-warnings.yml new file mode 100644 index 00000000000..bcf0567d586 --- /dev/null +++ b/changelogs/fragments/64733-make-no_log-false-override-no_log-warnings.yml @@ -0,0 +1,2 @@ +bugfixes: + - "make ``no_log=False`` on a module option silence the ``no_log`` warning (https://github.com/ansible/ansible/issues/49465 https://github.com/ansible/ansible/issues/64656)" diff --git a/docs/docsite/rst/dev_guide/developing_modules_documenting.rst b/docs/docsite/rst/dev_guide/developing_modules_documenting.rst index 3c4b5356df5..0421b4cd1ca 100644 --- a/docs/docsite/rst/dev_guide/developing_modules_documenting.rst +++ b/docs/docsite/rst/dev_guide/developing_modules_documenting.rst @@ -119,7 +119,8 @@ After the shebang, the UTF-8 coding, the copyright line, the license, and the `` Module documentation should briefly and accurately define what each module and option does, and how it works with others in the underlying system. Documentation should be written for broad audience--readable both by experts and non-experts. * Descriptions should always start with a capital letter and end with a full stop. Consistency always helps. * Verify that arguments in doc and module spec dict are identical. - * For password / secret arguments no_log=True should be set. + * For password / secret arguments ``no_log=True`` should be set. + * For arguments that seem to contain sensitive information but **do not** contain secrets, such as "password_length", set ``no_log=False`` to disable the warning message. * If an option is only sometimes required, describe the conditions. For example, "Required when I(state=present)." * If your module allows ``check_mode``, reflect this fact in the documentation. diff --git a/docs/docsite/rst/dev_guide/developing_program_flow_modules.rst b/docs/docsite/rst/dev_guide/developing_program_flow_modules.rst index 25600076485..5849fb3af5e 100644 --- a/docs/docsite/rst/dev_guide/developing_program_flow_modules.rst +++ b/docs/docsite/rst/dev_guide/developing_program_flow_modules.rst @@ -615,7 +615,10 @@ required no_log """""" -``no_log`` indicates that the value of the argument should not be logged or displayed. +``no_log`` accepts a boolean, either ``True`` or ``False``, that indicates explicitly whether or not the argument value should be masked in logs and output. + +.. note:: + In the absence of ``no_log``, if the parameter name appears to indicate that the argument value is a password or passphrase (such as "admin_password"), a warning will be shown and the value will be masked in logs but **not** output. To disable the warning and masking for parameters that do not contain sensitive information, set ``no_log`` to ``False``. aliases """"""" diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index a96ce6aa5de..4756381ac6d 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1940,15 +1940,14 @@ class AnsibleModule(object): for param in self.params: canon = self.aliases.get(param, param) arg_opts = self.argument_spec.get(canon, {}) - no_log = arg_opts.get('no_log', False) + no_log = arg_opts.get('no_log', None) - if self.boolean(no_log): - log_args[param] = 'NOT_LOGGING_PARAMETER' - # try to capture all passwords/passphrase named fields missed by no_log - elif PASSWORD_MATCH.search(param) and arg_opts.get('type', 'str') != 'bool' and not arg_opts.get('choices', False): - # skip boolean and enums as they are about 'password' state + # try to proactively capture password/passphrase fields + if no_log is None and PASSWORD_MATCH.search(param): log_args[param] = 'NOT_LOGGING_PASSWORD' self.warn('Module did not set no_log for %s' % param) + elif self.boolean(no_log): + log_args[param] = 'NOT_LOGGING_PARAMETER' else: param_val = self.params[param] if not isinstance(param_val, (text_type, binary_type)): diff --git a/test/units/module_utils/basic/test_argument_spec.py b/test/units/module_utils/basic/test_argument_spec.py index 71e4b163dbe..744df3083d9 100644 --- a/test/units/module_utils/basic/test_argument_spec.py +++ b/test/units/module_utils/basic/test_argument_spec.py @@ -593,3 +593,40 @@ class TestLoadFileCommonArguments: res = am.load_file_common_arguments(params=extended_params) assert res == final_params + + +@pytest.mark.parametrize("stdin", [{"arg_pass": "testing"}], indirect=["stdin"]) +def test_no_log_true(stdin, capfd): + """Explicitly mask an argument (no_log=True).""" + arg_spec = { + "arg_pass": {"no_log": True} + } + am = basic.AnsibleModule(arg_spec) + # no_log=True is picked up by both am._log_invocation and list_no_log_values + # (called by am._handle_no_log_values). As a result, we can check for the + # value in am.no_log_values. + assert "testing" in am.no_log_values + + +@pytest.mark.parametrize("stdin", [{"arg_pass": "testing"}], indirect=["stdin"]) +def test_no_log_false(stdin, capfd): + """Explicitly log and display an argument (no_log=False).""" + arg_spec = { + "arg_pass": {"no_log": False} + } + am = basic.AnsibleModule(arg_spec) + assert "testing" not in am.no_log_values and not am._warnings + + +@pytest.mark.parametrize("stdin", [{"arg_pass": "testing"}], indirect=["stdin"]) +def test_no_log_none(stdin, capfd): + """Allow Ansible to make the decision by matching the argument name + against PASSWORD_MATCH.""" + arg_spec = { + "arg_pass": {} + } + am = basic.AnsibleModule(arg_spec) + # Omitting no_log is only picked up by _log_invocation, so the value never + # makes it into am.no_log_values. Instead we can check for the warning + # emitted by am._log_invocation. + assert len(am._warnings) > 0