diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 9db6e9dcc6..9fe7b7a768 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -71,7 +71,7 @@ jobs: WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 821 + PYTEST_REQPASS: 822 steps: - name: Activate WSL1 if: "contains(matrix.shell, 'wsl')" diff --git a/examples/playbooks/rule-partial-become-without-become-fail.yml b/examples/playbooks/rule-partial-become-without-become-fail.yml index da48b2f967..80b633d908 100644 --- a/examples/playbooks/rule-partial-become-without-become-fail.yml +++ b/examples/playbooks/rule-partial-become-without-become-fail.yml @@ -1,28 +1,27 @@ --- -- hosts: localhost - name: Use of become_user without become play +- name: Use of become_user without become at play level + hosts: localhost become_user: root tasks: - - ansible.builtin.debug: + - name: A task without issues + ansible.builtin.debug: msg: hello -- hosts: localhost - +- name: Use of become_user without become at task level + hosts: localhost tasks: - name: Use of become_user without become task ansible.builtin.command: whoami become_user: postgres changed_when: false -- hosts: localhost - +- name: Use of become_user without become at task level + hosts: localhost tasks: - name: A block with become and become_user on different tasks block: - name: Sample become - become: true - ansible.builtin.command: whoami - - name: Sample become_user - become_user: postgres ansible.builtin.command: whoami + become_user: true + changed_when: false diff --git a/examples/playbooks/rule-partial-become-without-become-pass.yml b/examples/playbooks/rule-partial-become-without-become-pass.yml index e1ae189f63..c01b1410a1 100644 --- a/examples/playbooks/rule-partial-become-without-become-pass.yml +++ b/examples/playbooks/rule-partial-become-without-become-pass.yml @@ -1,14 +1,16 @@ --- -- hosts: localhost +- name: Test play + hosts: localhost become_user: root become: true tasks: - - ansible.builtin.debug: + - name: Debug + ansible.builtin.debug: msg: hello -- hosts: localhost - +- name: Test play + hosts: localhost tasks: - name: Foo ansible.builtin.command: whoami @@ -16,20 +18,22 @@ become: true changed_when: false -- hosts: localhost - become: true +- name: Test play + hosts: localhost tasks: - name: Accepts a become from higher scope ansible.builtin.command: whoami - become_user: postgres changed_when: false -- hosts: localhost +- name: Test play + hosts: localhost become_user: postgres + become: true tasks: - name: Accepts a become from a lower scope ansible.builtin.command: whoami become: true + become_user: root changed_when: false diff --git a/examples/playbooks/tasks/partial_become.yml/main.yml b/examples/playbooks/tasks/partial_become.yml/main.yml new file mode 100644 index 0000000000..c7f1980270 --- /dev/null +++ b/examples/playbooks/tasks/partial_become.yml/main.yml @@ -0,0 +1,4 @@ +--- +- name: Included with partial become + ansible.builtin.debug: + msg: Included with partial become diff --git a/examples/playbooks/transform-partial-become.transformed.yml b/examples/playbooks/transform-partial-become.transformed.yml new file mode 100644 index 0000000000..31d2a158c4 --- /dev/null +++ b/examples/playbooks/transform-partial-become.transformed.yml @@ -0,0 +1,56 @@ +--- +# The play has become_user and the task has become +# this is fixable, copy the become_user to the task +# and remove from the play +- name: Play 1 + hosts: localhost + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true + become_user: root + +# The task has become_user but the play does not +# this is fixable, remove the become_user from the task +- name: Play 2 + hosts: localhost + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + +# The task has become_user and the play has become +# this is fixable, add become to the task +- name: Play 3 + hosts: localhost + become: true + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true + become_user: root + +# The play has become_user but has an include +# this is not fixable, the include could be called from multiple playbooks +- name: Play 4 + hosts: localhost + become_user: root + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true + + - name: Include + ansible.builtin.include_tasks: + file: ../tasks/partial_become/main.yml diff --git a/examples/playbooks/transform-partial-become.yml b/examples/playbooks/transform-partial-become.yml new file mode 100644 index 0000000000..079d1a0160 --- /dev/null +++ b/examples/playbooks/transform-partial-become.yml @@ -0,0 +1,56 @@ +--- +# The play has become_user and the task has become +# this is fixable, copy the become_user to the task +# and remove from the play +- name: Play 1 + hosts: localhost + become_user: root + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true + +# The task has become_user but the play does not +# this is fixable, remove the become_user from the task +- name: Play 2 + hosts: localhost + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become_user: root + +# The task has become_user and the play has become +# this is fixable, add become to the task +- name: Play 3 + hosts: localhost + become: true + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become_user: root + +# The play has become_user but has an include +# this is not fixable, the include could be called from multiple playbooks +- name: Play 4 + hosts: localhost + become_user: root + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true + + - name: Include + ansible.builtin.include_tasks: + file: ../tasks/partial_become/main.yml diff --git a/src/ansiblelint/rules/partial_become.md b/src/ansiblelint/rules/partial_become.md index 01f9dae546..763cb67bc9 100644 --- a/src/ansiblelint/rules/partial_become.md +++ b/src/ansiblelint/rules/partial_become.md @@ -5,6 +5,13 @@ This rule checks that privilege escalation is activated when changing users. To perform an action as a different user with the `become_user` directive, you must set `become: true`. +This rule can produce the following messages: + +- `partial-become[play]`: become_user requires become to work as expected, at + play level. +- `partial-become[task]`: become_user requires become to work as expected, at + task level. + !!! warning While Ansible inherits have of `become` and `become_user` from upper levels, @@ -19,12 +26,13 @@ must set `become: true`. --- - name: Example playbook hosts: localhost + become: true # <- Activates privilege escalation. tasks: - name: Start the httpd service as the apache user ansible.builtin.service: name: httpd state: started - become_user: apache # <- Does not change the user because "become: true" is not set. + become_user: apache # <- Does not change the user because "become: true" is not set. ``` ## Correct Code @@ -37,6 +45,78 @@ must set `become: true`. ansible.builtin.service: name: httpd state: started - become: true # <- Activates privilege escalation. - become_user: apache # <- Changes the user with the desired privileges. + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. + +# Stand alone playbook alternative, applies to all tasks + +- name: Example playbook + hosts: localhost + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. + tasks: + - name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook 1 + hosts: localhost + become: true # <- Activates privilege escalation. + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +--- +- name: Example playbook 2 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +# tasks.yml +- name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become_user: apache # <- Does not change the user because "become: true" is not set. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook 1 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +--- +- name: Example playbook 2 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +# tasks.yml +- name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become: true # <- Activates privilege escalation. + become_user: apache # <- Does not change the user because "become: true" is not set. ``` diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index d14c06f31b..b8dc417052 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -21,98 +21,214 @@ from __future__ import annotations import sys -from functools import reduce from typing import TYPE_CHECKING, Any +from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.constants import LINE_NUMBER_KEY -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: + from collections.abc import Iterator + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task -def _get_subtasks(data: dict[str, Any]) -> list[Any]: - result: list[Any] = [] - block_names = [ - "tasks", - "pre_tasks", - "post_tasks", - "handlers", - "block", - "always", - "rescue", - ] - for name in block_names: - if data and name in data: - result += data[name] or [] - return result - - -def _nested_search(term: str, data: dict[str, Any]) -> Any: - if data and term in data: - return True - return reduce( - (lambda x, y: x or _nested_search(term, y)), - _get_subtasks(data), - False, - ) - - -def _become_user_without_become(becomeuserabove: bool, data: dict[str, Any]) -> Any: - if "become" in data: - # If become is in lineage of tree then correct - return False - if "become_user" in data and _nested_search("become", data): - # If 'become_user' on tree and become somewhere below - # we must check for a case of a second 'become_user' without a - # 'become' in its lineage - subtasks = _get_subtasks(data) - return reduce( - (lambda x, y: x or _become_user_without_become(False, y)), - subtasks, - False, - ) - if _nested_search("become_user", data): - # Keep searching down if 'become_user' exists in the tree below current task - subtasks = _get_subtasks(data) - return len(subtasks) == 0 or reduce( - ( - lambda x, y: x - or _become_user_without_become( - becomeuserabove or "become_user" in data, - y, - ) - ), - subtasks, - False, - ) - # If at bottom of tree, flag up if 'become_user' existed in the lineage of the tree and - # 'become' was not. This is an error if any lineage has a 'become_user' but no become - return becomeuserabove - - -class BecomeUserWithoutBecomeRule(AnsibleLintRule): - """become_user requires become to work as expected.""" +class BecomeUserWithoutBecomeRule(AnsibleLintRule, TransformMixin): + """``become_user`` should have a corresponding ``become`` at the play or task level.""" id = "partial-become" - description = "``become_user`` without ``become`` will not actually change user" + description = "``become_user`` should have a corresponding ``become`` at the play or task level." severity = "VERY_HIGH" tags = ["unpredictability"] version_added = "historic" - def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: - if file.kind == "playbook": - result = _become_user_without_become(False, data) - if result: - return [ - self.create_matcherror( - message=self.shortdesc, - filename=file, - lineno=data[LINE_NUMBER_KEY], - ), - ] - return [] + def matchplay( + self: BecomeUserWithoutBecomeRule, + file: Lintable, + data: dict[str, Any], + ) -> list[MatchError]: + """Match become_user without become in play. + + :param file: The file to lint. + :param data: The data to lint (play) + :returns: A list of errors. + """ + if file.kind != "playbook": + return [] + errors = [] + partial = "become_user" in data and "become" not in data + if partial: + error = self.create_matcherror( + message=self.shortdesc, + filename=file, + tag=f"{self.id}[play]", + lineno=data[LINE_NUMBER_KEY], + ) + errors.append(error) + return errors + + def matchtask( + self: BecomeUserWithoutBecomeRule, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + """Match become_user without become in task. + + :param task: The task to lint. + :param file: The file to lint. + :returns: A list of errors. + """ + data = task.normalized_task + errors = [] + partial = "become_user" in data and "become" not in data + if partial: + error = self.create_matcherror( + message=self.shortdesc, + filename=file, + tag=f"{self.id}[task]", + lineno=task[LINE_NUMBER_KEY], + ) + errors.append(error) + return errors + + def _dive(self: BecomeUserWithoutBecomeRule, data: CommentedSeq) -> Iterator[Any]: + """Dive into the data and yield each item. + + :param data: The data to dive into. + :yield: Each item in the data. + """ + for item in data: + for nested in ("block", "rescue", "always"): + if nested in item: + yield from self._dive(item[nested]) + yield item + + def transform( + self: BecomeUserWithoutBecomeRule, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + """Transform the data. + + :param match: The match to transform. + :param lintable: The file to transform. + :param data: The data to transform. + """ + if not isinstance(data, CommentedSeq): + return + + obj = self.seek(match.yaml_path, data) + if "become" in obj and "become_user" in obj: + match.fixed = True + return + if "become" not in obj and "become_user" not in obj: + match.fixed = True + return + + self._transform_plays(plays=data) + + if "become" in obj and "become_user" in obj: + match.fixed = True + return + if "become" not in obj and "become_user" not in obj: + match.fixed = True + return + + def is_ineligible_for_transform( + self: BecomeUserWithoutBecomeRule, + data: CommentedMap, + ) -> bool: + """Check if the data is eligible for transformation. + + :param data: The data to check. + :returns: True if ineligible, False otherwise. + """ + if any("include" in key for key in data): + return True + if "notify" in data: + return True + return False + + def _transform_plays(self, plays: CommentedSeq) -> None: + """Transform the plays. + + :param plays: The plays to transform. + """ + for play in plays: + self._transform_play(play=play) + + def _transform_play(self, play: CommentedMap) -> None: + """Transform the play. + + :param play: The play to transform. + """ + # Ensure we have no includes in this play + task_groups = ("tasks", "pre_tasks", "post_tasks", "handlers") + for task_group in task_groups: + tasks = self._dive(play.get(task_group, [])) + for task in tasks: + if self.is_ineligible_for_transform(task): + return + remove_play_become_user = False + for task_group in task_groups: + tasks = self._dive(play.get(task_group, [])) + for task in tasks: + b_in_t = "become" in task + bu_in_t = "become_user" in task + b_in_p = "become" in play + bu_in_p = "become_user" in play + if b_in_t and not bu_in_t and bu_in_p: + # Preserve the end comment if become is the last key + comment = None + if list(task.keys())[-1] == "become" and "become" in task.ca.items: + comment = task.ca.items.pop("become") + become_index = list(task.keys()).index("become") + task.insert(become_index + 1, "become_user", play["become_user"]) + if comment: + self._attach_comment_end(task, comment) + remove_play_become_user = True + if bu_in_t and not b_in_t and b_in_p: + become_user_index = list(task.keys()).index("become_user") + task.insert(become_user_index, "become", play["become"]) + if bu_in_t and not b_in_t and not b_in_p: + # Preserve the end comment if become_user is the last key + comment = None + if ( + list(task.keys())[-1] == "become_user" + and "become_user" in task.ca.items + ): + comment = task.ca.items.pop("become_user") + task.pop("become_user") + if comment: + self._attach_comment_end(task, comment) + if remove_play_become_user: + del play["become_user"] + + def _attach_comment_end( + self, + obj: CommentedMap | CommentedSeq, + comment: Any, + ) -> None: + """Attach a comment to the end of the object. + + :param obj: The object to attach the comment to. + :param comment: The comment to attach. + """ + if isinstance(obj, CommentedMap): + last = list(obj.keys())[-1] + if not isinstance(obj[last], (CommentedSeq, CommentedMap)): + obj.ca.items[last] = comment + return + self._attach_comment_end(obj[last], comment) + elif isinstance(obj, CommentedSeq): + if not isinstance(obj[-1], (CommentedSeq, CommentedMap)): + obj.ca.items[len(obj)] = comment + return + self._attach_comment_end(obj[-1], comment) # testing code to be loaded only with pytest or when executed the rule file @@ -120,16 +236,16 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports - def test_partial_become_positive() -> None: - """Positive test for partial-become.""" + def test_partial_become_pass() -> None: + """No errors found for partial-become.""" collection = RulesCollection() collection.register(BecomeUserWithoutBecomeRule()) success = "examples/playbooks/rule-partial-become-without-become-pass.yml" good_runner = Runner(success, rules=collection) assert [] == good_runner.run() - def test_partial_become_negative() -> None: - """Negative test for partial-become.""" + def test_partial_become_fail() -> None: + """Errors found for partial-become.""" collection = RulesCollection() collection.register(BecomeUserWithoutBecomeRule()) failure = "examples/playbooks/rule-partial-become-without-become-fail.yml" diff --git a/test/test_skip_inside_yaml.py b/test/test_skip_inside_yaml.py index 363734e089..f8b889c270 100644 --- a/test/test_skip_inside_yaml.py +++ b/test/test_skip_inside_yaml.py @@ -19,7 +19,7 @@ def test_role_tasks_with_block(default_rules_collection: RulesCollection) -> Non @pytest.mark.parametrize( ("lintable", "expected"), - (pytest.param("examples/playbooks/test_skip_inside_yaml.yml", 4, id="yaml"),), + (pytest.param("examples/playbooks/test_skip_inside_yaml.yml", 6, id="yaml"),), ) def test_inline_skips( default_rules_collection: RulesCollection, diff --git a/test/test_transformer.py b/test/test_transformer.py index fd0c43546d..31a4fd97ab 100644 --- a/test/test_transformer.py +++ b/test/test_transformer.py @@ -110,6 +110,12 @@ def fixture_runner_result( True, id="key_order_transform", ), + pytest.param( + "examples/playbooks/transform-partial-become.yml", + 4, + True, + id="partial_become", + ), ), ) def test_transformer( # pylint: disable=too-many-arguments, too-many-locals diff --git a/test/test_yaml_utils.py b/test/test_yaml_utils.py index 6a6c7e7635..326816e541 100644 --- a/test/test_yaml_utils.py +++ b/test/test_yaml_utils.py @@ -386,7 +386,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 10, + 12, [1], id="4_play_playbook-first_line_in_play_2", ), @@ -404,7 +404,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 19, + 21, [2], id="4_play_playbook-first_line_in_play_3", ), @@ -422,7 +422,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 28, + 31, [3], id="4_play_playbook-first_line_in_play_4", ), @@ -603,7 +603,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 7, + 8, [0, "tasks", 0], id="4_play_playbook-play_1_first_line_task_1", ), @@ -615,7 +615,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 10, + 13, [], id="4_play_playbook-play_2_line_before_tasks", ), @@ -627,7 +627,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 13, + 15, [1, "tasks", 0], id="4_play_playbook-play_2_first_line_task_1", ), @@ -645,7 +645,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 19, + 23, [], id="4_play_playbook-play_3_line_before_tasks", ), @@ -657,7 +657,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 23, + 25, [2, "tasks", 0], id="4_play_playbook-play_3_first_line_task_1", ), @@ -675,7 +675,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 28, + 33, [], id="4_play_playbook-play_4_line_before_tasks", ), @@ -687,13 +687,13 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 32, + 35, [3, "tasks", 0], id="4_play_playbook-play_4_first_line_task_1", ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 33, + 39, [3, "tasks", 0], id="4_play_playbook-play_4_middle_line_task_1", ),