Merge branch 'main' into fix-2065

.github/workflows/tox.yml

@@ -166,7 +166,7 @@ jobs:
       WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 716
+      PYTEST_REQPASS: 701
 
     steps:
       - name: Activate WSL1

conftest.py

@@ -1,6 +1,7 @@
 """PyTest Fixtures."""
 import importlib
 import os
+import subprocess
 import sys
 from typing import Any
 
@@ -18,6 +19,14 @@
         file=sys.stderr,
     )
     sys.exit(1)
+# we need to be sure that we have the requirements installed as some tests
+# might depend on these.
+subprocess.run(
+    ["ansible-galaxy", "collection", "install", "-r", "requirements.yml"],
+    check=True,
+    text=True,
+    capture_output=True,
+)
 
 if not HAS_LIBYAML and sys.version_info >= (3, 9, 0):
     # While presence of libyaml is not required for runtime, we keep this error

examples/playbooks/rule-risky-file-permissions-fail.yml

@@ -0,0 +1,88 @@
+# Fixture for RiskyFilePermissionsRule should return 11 occurrences
+---
+- name: FAIL_INI_PRESERVE
+  hosts: all
+  tasks:
+    - name: Ini_file does not accept preserve mode
+      community.general.ini_file:
+        path: foo
+        create: true
+        mode: preserve
+
+- name: FAIL_INI_PERMISSION
+  hosts: all
+  tasks:
+    - name: Permissions needed if create is used
+      community.general.ini_file:
+        path: foo
+        create: true
+
+- name: FAIL_PRESERVE_MODE
+  hosts: all
+  tasks:
+    - name: File does not allow preserve value for mode
+      ansible.builtin.file:
+        path: foo
+        mode: preserve
+
+- name: FAIL_MISSING_PERMISSIONS_TOUCH
+  hosts: all
+  tasks:
+    - name: Permissions missing and might create file
+      file:
+        path: foo
+        state: touch
+    - name: Permissions missing and might create file (fqcn)
+      ansible.builtin.file:
+        path: foo
+        state: touch
+
+- name: FAIL_MISSING_PERMISSIONS_DIRECTORY
+  hosts: all
+  tasks:
+    - name: Permissions missing and might create directory
+      file:
+        path: foo
+        state: directory
+    - name: Lineinfile when create is true (fqcn)
+      ansible.builtin.lineinfile:
+        path: foo
+        create: true
+        line: some content here
+
+- name: FAIL_MISSING_PERMISSIONS_GET_URL
+  hosts: all
+  tasks:
+    - name: Permissions missing
+      # noqa: fqcn-builtins
+      get_url:
+        url: http://foo
+        dest: foo
+
+- name: FAIL_LINEINFILE_CREATE
+  hosts: all
+  tasks:
+    - name: Lineinfile when create is true
+      ansible.builtin.lineinfile:
+        path: foo
+        create: true
+        line: some content here
+
+- name: FAIL_REPLACE_PRESERVE
+  hosts: all
+  tasks:
+    - name: Replace does not allow preserve mode
+      replace:
+        path: foo
+        mode: preserve
+
+- name: FAIL_PERMISSION_COMMENT
+  hosts: all
+  tasks:
+    - name: Permissions is only a comment
+      file:
+        path: foo
+        owner: root
+        group: root
+        state: directory
+        # mode: 0755

examples/playbooks/rule-risky-file-permissions-pass.yml

@@ -0,0 +1,75 @@
+# Fixture for RiskyFilePermissionsRule should pass
+---
+- name: SUCCESS_PERMISSIONS_PRESENT
+  hosts: all
+  tasks:
+    - name: Permissions not missing and numeric
+      ansible.builtin.file:
+        path: foo
+        mode: 0600
+
+- name: SUCCESS_PERMISSIONS_PRESENT_GET_URL
+  hosts: all
+  tasks:
+    - name: Permissions not missing and numeric
+      ansible.builtin.get_url:
+        url: http://foo
+        dest: foo
+        mode: 0600
+
+- name: SUCCESS_ABSENT_STATE
+  hosts: all
+  tasks:
+    - name: Permissions missing while state is absent is fine
+      ansible.builtin.file:
+        path: foo
+        state: absent
+
+- name: SUCCESS_DEFAULT_STATE
+  hosts: all
+  tasks:
+    - name: Permissions missing while state is file (default) is fine
+      ansible.builtin.file:
+        path: foo
+
+- name: SUCCESS_LINK_STATE
+  hosts: all
+  tasks:
+    - name: Permissions missing while state is link is fine
+      ansible.builtin.file:
+        path: foo2
+        src: foo
+        state: link
+
+- name: SUCCESS_CREATE_FALSE
+  hosts: all
+  tasks:
+    - name: File edit when create is false
+      ansible.builtin.lineinfile:
+        path: foo
+        create: false
+        line: some content here
+
+- name: SUCCESS_REPLACE
+  hosts: all
+  tasks:
+    - name: Replace should not require mode
+      ansible.builtin.replace:
+        path: foo
+
+- name: SUCCESS_RECURSE
+  hosts: all
+  tasks:
+    - name: File with recursive does not require mode
+      ansible.builtin.file:
+        state: directory
+        recurse: true
+    - name: Permissions not missing and numeric (fqcn)
+      ansible.builtin.file:
+        path: bar
+        mode: 755 # noqa: risky-octal
+    - name: File edit when create is false (fqcn)
+      ansible.builtin.lineinfile:
+        path: foo
+        create: false
+        line: some content here

requirements.yml

@@ -6,3 +6,5 @@ collections:
   # that collection is used during ansible-lint testing for validating our
   # ability to detect non-core modules.
   - name: ansible.posix
+  # that collection is used during ansible-lint own testing
+  - name: community.general

src/ansiblelint/rules/risky_file_permissions.md

@@ -0,0 +1,52 @@
+# risky-file-permissions
+
+This rule is triggered by various modules that could end up creating new files
+on disk with permissions that might be too open, or unpredictable. Please
+read the documentation of each module carefully to understand the
+implications of using different argument values, as these make the difference
+between using the module safely or not. The fix depends on each module and
+also your particular situation.
+
+Some modules have a `create` argument that defaults to `true`. For those you
+either need to set `create: false` or provide some permissions like
+`mode: 0600` to make the behavior predictable and not dependent on the current
+system settings.
+
+Modules that are checked:
+
+- [`ansible.builtin.assemble`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/assemble_module.html)
+- [`ansible.builtin.copy`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html)
+- [`ansible.builtin.file`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/file_module.html)
+- [`ansible.builtin.get_url`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/get_url_module.html)
+- [`ansible.builtin.replace`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/replace_module.html)
+- [`ansible.builtin.template`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/template_module.html)
+- [`community.general.archive`](https://docs.ansible.com/ansible/latest/collections/community/general/archive_module.html)
+- [`community.general.ini_file`](https://docs.ansible.com/ansible/latest/collections/community/general/ini_file_module.html)
+
+## Problematic code
+
+```yaml
+---
+- name: Unsafe example of using ini_file
+  community.general.ini_file:
+    path: foo
+    create: true
+    mode: preserve
+```
+
+## Correct code
+
+```yaml
+---
+- name: Safe example of using ini_file (1st solution)
+  community.general.ini_file:
+    path: foo
+    create: false  # prevents creating a file with potentially insecure permissions
+    mode: preserve
+
+- name: Safe example of using ini_file (2nd solution)
+  community.general.ini_file:
+    path: foo
+    mode: 0600  # explicitly sets the desired permissions, to make the results predictable
+    mode: preserve
+```